So is this an FPGA soft core PS2 device? Is there any kind of version
attached to the soft core? The compatible value should specify an
exact version of the implementation that this driver works with.
(Newer core versions can claim compatibility with older ones, so the
driver's compatible list doesn't need to be exhaustive).
Otherwise, this patch looks correct.
g.
So is this an FPGA soft core PS2 device? Is there any kind of version
attached to the soft core? The compatible value should specify an
exact version of the implementation that this driver works with.
(Newer core versions can claim compatibility with older ones, so the
driver's compatible list doesn't need to be exhaustive).
What's the preferred way of versioning components in a device-tree?
Quite a few components inside an fpga will get a new version number with
every release of the tools. For example components supplied by Altera
will get a new number with every release of their IP library (approx.
twice a year) even when (at least from a software point of view) there
is nothing changed in the core. Should we add the number to the
"compatible" name and possibly get slightly more bulky drivers, or add a
version tag to the components where a driver can make decisions based on
the version of the core (if needed)?
Another way to reduce the number of lines in a compatible section would
be to add both their versioned and unversioned compatible entry in the
dts so drivers not needing a specific version don't need to supply the
entire list.
We do have the version numbers available when generating the DTS and
NiosII is still quite new to device-tree so we are still flexible in
fixing this in the best possible way.
So is this an FPGA soft core PS2 device? Is there any kind of version
attached to the soft core? The compatible value should specify an
exact version of the implementation that this driver works with.
(Newer core versions can claim compatibility with older ones, so the
driver's compatible list doesn't need to be exhaustive).
What's the preferred way of versioning components in a device-tree?
Quite a few components inside an fpga will get a new version number with
every release of the tools. For example components supplied by Altera
will get a new number with every release of their IP library (approx.
twice a year) even when (at least from a software point of view) there
is nothing changed in the core. Should we add the number to the
"compatible" name and possibly get slightly more bulky drivers, or add a
version tag to the components where a driver can make decisions based on
the version of the core (if needed)?
Another way to reduce the number of lines in a compatible section would
be to add both their versioned and unversioned compatible entry in the
dts so drivers not needing a specific version don't need to supply the
entire list.
We do have the version numbers available when generating the DTS and
NiosII is still quite new to device-tree so we are still flexible in
fixing this in the best possible way.
A good rule of thumb is to always choose compatible values that
reflect real working hardware. ie. "xlnx,xps-uartlite-1.00.a" instead
of trying to define a generic "xlnx,uartlite". You can see this value
in drivers/serial/uartlite.c, and write the driver to match the value
of the device that you actually worked with.
Then, when you produce a .dts for a design, each device must specify
exactly what it is (the specific version) plus an optional list of
devices that it is 100% register-level backwards compatible with. In
the example above, the uartlite has retained the exact same interface,
so all designs claim compatibility with xlnx,xps-uartlite-1.00.a
regardless of the actual version.
To take the example of the altera ps2 core; say altera has released
versions 1, 2, 3, 4 and 5 of the core, and say the behaviour changed
in a non-compatible way in version 3. Then the driver might do
something like:
static struct of_device_id altera_ps2_match[] = {
{ .compatible = "altera,altera_ps2-1", .data = altera_ps2_1_ops, },
{ .compatible = "altera,altera_ps2-3", .data = altera_ps2_3_ops, },
{ }
};
Then the compatible values for each version in a .dts file would be:
v1: compatible = "altera,altera_ps2-1";
v2: compatible = "altera,altera_ps2-2", "altera,altera_ps2-1";
v3: compatible = "altera,altera_ps2-3";
v4: compatible = "altera,altera_ps2-4", "altera,altera_ps2-3";
v5: compatible = "altera,altera_ps2-5", "altera,altera_ps2-3";
so, instead of trying to us a 'generic' value like "altera,altera_ps2"
which has a bunch of ambiguity about which hardware actually
implements the behaviour, the values "altera,altera_ps2-1" and
"altera,altera_ps2-3" become the de-facto 'generic' values without any
messiness or ambiguity about what they mean.
Plus, each .dts file still specifies the exact version that is
implemented on the board so that the driver can still fixup
version-specific bugs if any are discovered in the future.
g.
So is this an FPGA soft core PS2 device? Is there any kind of version
attached to the soft core? The compatible value should specify an
exact version of the implementation that this driver works with.
(Newer core versions can claim compatibility with older ones, so the
driver's compatible list doesn't need to be exhaustive).
What's the preferred way of versioning components in a device-tree?
Quite a few components inside an fpga will get a new version number with
every release of the tools. For example components supplied by Altera
will get a new number with every release of their IP library (approx.
twice a year) even when (at least from a software point of view) there
is nothing changed in the core. Should we add the number to the
"compatible" name and possibly get slightly more bulky drivers, or add a
version tag to the components where a driver can make decisions based on
the version of the core (if needed)?
Another way to reduce the number of lines in a compatible section would
be to add both their versioned and unversioned compatible entry in the
dts so drivers not needing a specific version don't need to supply the
entire list.
We do have the version numbers available when generating the DTS and
NiosII is still quite new to device-tree so we are still flexible in
fixing this in the best possible way.
A good rule of thumb is to always choose compatible values that
reflect real working hardware. ie. "xlnx,xps-uartlite-1.00.a" instead
of trying to define a generic "xlnx,uartlite". You can see this value
in drivers/serial/uartlite.c, and write the driver to match the value
of the device that you actually worked with.
Then, when you produce a .dts for a design, each device must specify
exactly what it is (the specific version) plus an optional list of
devices that it is 100% register-level backwards compatible with. In
the example above, the uartlite has retained the exact same interface,
so all designs claim compatibility with xlnx,xps-uartlite-1.00.a
regardless of the actual version.
To take the example of the altera ps2 core; say altera has released
versions 1, 2, 3, 4 and 5 of the core, and say the behaviour changed
in a non-compatible way in version 3. Then the driver might do
something like:
static struct of_device_id altera_ps2_match[] = {
{ .compatible = "altera,altera_ps2-1", .data = altera_ps2_1_ops, },
{ .compatible = "altera,altera_ps2-3", .data = altera_ps2_3_ops, },
{ }
};
Then the compatible values for each version in a .dts file would be:
v1: compatible = "altera,altera_ps2-1";
v2: compatible = "altera,altera_ps2-2", "altera,altera_ps2-1";
v3: compatible = "altera,altera_ps2-3";
v4: compatible = "altera,altera_ps2-4", "altera,altera_ps2-3";
v5: compatible = "altera,altera_ps2-5", "altera,altera_ps2-3";
so, instead of trying to us a 'generic' value like "altera,altera_ps2"
which has a bunch of ambiguity about which hardware actually
implements the behaviour, the values "altera,altera_ps2-1" and
"altera,altera_ps2-3" become the de-facto 'generic' values without any
messiness or ambiguity about what they mean.
Plus, each .dts file still specifies the exact version that is
implemented on the board so that the driver can still fixup
version-specific bugs if any are discovered in the future.
Ahh ok.
That does look like a good solution. I'll try and cook something up for
that.
Thanks for the explanation!
Walter
From: Thomas Chou <hidden> Date: 2011-01-18 14:26:37
On 01/18/2011 07:27 AM, Walter Goossens wrote:
quoted
so, instead of trying to us a 'generic' value like "altera,altera_ps2"
which has a bunch of ambiguity about which hardware actually
implements the behaviour, the values "altera,altera_ps2-1" and
"altera,altera_ps2-3" become the de-facto 'generic' values without any
messiness or ambiguity about what they mean.
Plus, each .dts file still specifies the exact version that is
implemented on the board so that the driver can still fixup
version-specific bugs if any are discovered in the future.
Ahh ok.
That does look like a good solution. I'll try and cook something up for
that.
Hi Walter,
Can we use "kind" and "version" attribute of a sopcinfo file to build
the match string? This way, your dts converter won't need update for
each new component.
eg,
kind="altera_avalon_spi" version="9.0" ==> "altera","avalon-spi-9.0"
kind="altera_up_avalon_ps2_classic" version="6.1" ==>
"altera","up-avalon-ps2-classic-6.1"
- Thomas
@@ -189,13 +197,12 @@ static int __init altera_ps2_init(void){returnplatform_driver_register(&altera_ps2_driver);}+module_init(altera_ps2_init);staticvoid__exitaltera_ps2_exit(void){platform_driver_unregister(&altera_ps2_driver);}--module_init(altera_ps2_init);module_exit(altera_ps2_exit);MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
@@ -173,6 +174,12 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)return0;}+staticconststructof_device_idaltera_ps2_match[]={+{.compatible="altr,ps2-1.0",},
I thought I had seen 'altera' instead of an abbreviation being used in
a previous patch. I don't care much whether 'altr' or 'altera' is
used, but I'd like to know that there is consensus from the Altera
users so that all the drivers use the same prefix.
At the moment, this patch can only be applied against my
devicetree/next branch since there is a #ifdef CONFIG_OF around
of_match_table in mainline, and the patch to remove it is in my next
branch. I'm okay with taking it through my tree if Dmitry agrees.
g.
quoted hunk
},
};
@@ -189,13 +197,12 @@ static int __init altera_ps2_init(void) { return platform_driver_register(&altera_ps2_driver); }+module_init(altera_ps2_init); static void __exit altera_ps2_exit(void) { platform_driver_unregister(&altera_ps2_driver); }--module_init(altera_ps2_init); module_exit(altera_ps2_exit); MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
@@ -173,6 +174,12 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)return0;}+staticconststructof_device_idaltera_ps2_match[]={+{.compatible="altr,ps2-1.0",},
I thought I had seen 'altera' instead of an abbreviation being used in
a previous patch. I don't care much whether 'altr' or 'altera' is
used, but I'd like to know that there is consensus from the Altera
users so that all the drivers use the same prefix.
Oh, and this patch should add documentation to
Documentation/devicetree/bindings/spi specifying exactly what device
this compatible string represents and what properties are expected to
be in the node (if any) beyond the standard reg, interrupts, etc. It
doesn't need to be huge and detailed, but it at least needs to show
that the string is in use. This comment goes for the other patches
that add bindings too.
g.
@@ -189,13 +197,12 @@ static int __init altera_ps2_init(void){returnplatform_driver_register(&altera_ps2_driver);}+module_init(altera_ps2_init);staticvoid__exitaltera_ps2_exit(void){platform_driver_unregister(&altera_ps2_driver);}--module_init(altera_ps2_init);module_exit(altera_ps2_exit);MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
@@ -173,6 +174,12 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)return0;}+staticconststructof_device_idaltera_ps2_match[]={+{.compatible="ALTR,ps2-1.0",},+{},+}+MODULE_DEVICE_TABLE(of,altera_ps2_match);+
Should be:
#ifdef CONFIG_OF
static const struct of_device_id altera_ps2_match[] = {
{ .compatible = "ALTR,ps2-1.0", },
{},
}
MODULE_DEVICE_TABLE(of, altera_ps2_match);
#else /* CONFIG_OF */
#define altera_ps2_match NULL
#endif /* CONFIG_OF */
Otherwise the driver will be advertising that it provides device tree
support when CONFIG_OF is disabled.
Otherwise, looks good to me. Feel free to add my acked-by line.
g.
@@ -173,6 +174,12 @@ static int __devexit altera_ps2_remove(struct platform_device *pdev)return0;}+staticconststructof_device_idaltera_ps2_match[]={+{.compatible="ALTR,ps2-1.0",},+{},+}+MODULE_DEVICE_TABLE(of,altera_ps2_match);+
Should be:
#ifdef CONFIG_OF
static const struct of_device_id altera_ps2_match[] = {
{ .compatible = "ALTR,ps2-1.0", },
{},
}
MODULE_DEVICE_TABLE(of, altera_ps2_match);
#else /* CONFIG_OF */
#define altera_ps2_match NULL
#endif /* CONFIG_OF */
Otherwise the driver will be advertising that it provides device tree
support when CONFIG_OF is disabled.
Thanks. Will update all related drivers.
- Thomas
Otherwise, looks good to me. Feel free to add my acked-by line.
g.
@@ -189,13 +201,12 @@ static int __init altera_ps2_init(void){returnplatform_driver_register(&altera_ps2_driver);}+module_init(altera_ps2_init);staticvoid__exitaltera_ps2_exit(void){platform_driver_unregister(&altera_ps2_driver);}--module_init(altera_ps2_init);module_exit(altera_ps2_exit);MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
On Mon, Feb 14, 2011 at 10:06:42AM +0800, Thomas Chou wrote:
From: Walter Goossens <redacted>
Signed-off-by: Walter Goossens <redacted>
Signed-off-by: Thomas Chou <redacted>
Acked-by: Grant Likely <redacted>
Acked-by: Dmitry Torokhov <redacted>
---
v2 use const and add compat version.
v3 change compatible vendor to ALTR.
add dts binding doc.
v4 condition module device table export for of.
Grant, please take it through your tree as Dmitry agreed.
@@ -189,13 +201,12 @@ static int __init altera_ps2_init(void){returnplatform_driver_register(&altera_ps2_driver);}+module_init(altera_ps2_init);staticvoid__exitaltera_ps2_exit(void){platform_driver_unregister(&altera_ps2_driver);}--module_init(altera_ps2_init);module_exit(altera_ps2_exit);MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
@@ -189,13 +201,12 @@ static int __init altera_ps2_init(void){returnplatform_driver_register(&altera_ps2_driver);}+module_init(altera_ps2_init);staticvoid__exitaltera_ps2_exit(void){platform_driver_unregister(&altera_ps2_driver);}--module_init(altera_ps2_init);module_exit(altera_ps2_exit);MODULE_DESCRIPTION("Altera University Program PS2 controller driver");
At the moment, this patch can only be applied against my
devicetree/next branch since there is a #ifdef CONFIG_OF around
of_match_table in mainline, and the patch to remove it is in my next
branch. I'm okay with taking it through my tree if Dmitry agrees.
Actually I was going to ask you if you could do just that.
Thanks.
--
Dmitry
I thought I had seen 'altera' instead of an abbreviation being used in
a previous patch. I don't care much whether 'altr' or 'altera' is
used, but I'd like to know that there is consensus from the Altera
users so that all the drivers use the same prefix.
We had discussed on nios2-dev mailing list, and decided to use 'altr' as
Walter suggested that it saves space.
About the documentation on dts binding, shall we have a single
altera.txt to describe all Altera related binding, and opencores.txt to
describe all OpenCores binding? Or separate file for each core in its
driver class?
- Thomas
I thought I had seen 'altera' instead of an abbreviation being used in
a previous patch. I don't care much whether 'altr' or 'altera' is
used, but I'd like to know that there is consensus from the Altera
users so that all the drivers use the same prefix.
We had discussed on nios2-dev mailing list, and decided to use 'altr' as
Walter suggested that it saves space.
About the documentation on dts binding, shall we have a single
altera.txt to describe all Altera related binding, and opencores.txt to
describe all OpenCores binding? Or separate file for each core in its
driver class?
As I will submit the device tree support for the altera_uart driver
(and the altera_jtaguart driver sometime in the future) I was asking
myself the same question. I'd vote for the altera.txt solution, as we
could use that for documenting the uart drivers, other Altera component
specific drivers and also the Nios2 arch specific parameters in the
future.
Cheers
Tobias
I thought I had seen 'altera' instead of an abbreviation being used in
a previous patch. I don't care much whether 'altr' or 'altera' is
used, but I'd like to know that there is consensus from the Altera
users so that all the drivers use the same prefix.
We had discussed on nios2-dev mailing list, and decided to use
'altr' as Walter suggested that it saves space.
About the documentation on dts binding, shall we have a single
altera.txt to describe all Altera related binding, and opencores.txt
to describe all OpenCores binding? Or separate file for each core in
its driver class?
Since I'm reorganizing the binding documentation to reflect subsystems
(bindings/spi, bindings/i2c, bindings/powerpc, etc) I'd prefer to see
a separate file for each type of core.
Eventually, I'd like to define a record format for documenting
bindings that can generate searchable and crosslinked output, but I've
not spent any time looking at that seriously yet.
g.
I thought I had seen 'altera' instead of an abbreviation being used in
a previous patch. I don't care much whether 'altr' or 'altera' is
used, but I'd like to know that there is consensus from the Altera
users so that all the drivers use the same prefix.
We had discussed on nios2-dev mailing list, and decided to use
'altr' as Walter suggested that it saves space.
About the documentation on dts binding, shall we have a single
altera.txt to describe all Altera related binding, and opencores.txt
to describe all OpenCores binding? Or separate file for each core in
its driver class?
Since I'm reorganizing the binding documentation to reflect subsystems
(bindings/spi, bindings/i2c, bindings/powerpc, etc) I'd prefer to see
a separate file for each type of core.
OK. We will use separate files. I will send an updated patch to include
the dts binding.
As for nios2 cpu related binding, we shall use binding/nios2 dir. Right?
- Thomas
Eventually, I'd like to define a record format for documenting
bindings that can generate searchable and crosslinked output, but I've
not spent any time looking at that seriously yet.
g.
I thought I had seen 'altera' instead of an abbreviation being used in
a previous patch. I don't care much whether 'altr' or 'altera' is
used, but I'd like to know that there is consensus from the Altera
users so that all the drivers use the same prefix.
We had discussed on nios2-dev mailing list, and decided to use
'altr' as Walter suggested that it saves space.
Is altr the stock ticker symbol? The convention is to either use the
stock ticker in all uppercase (although the uppercase bit hasn't been
consistently applied), or to use the full name in lowercase.
g.
I thought I had seen 'altera' instead of an abbreviation being used in
a previous patch. I don't care much whether 'altr' or 'altera' is
used, but I'd like to know that there is consensus from the Altera
users so that all the drivers use the same prefix.
We had discussed on nios2-dev mailing list, and decided to use
'altr' as Walter suggested that it saves space.
Is altr the stock ticker symbol? The convention is to either use the
stock ticker in all uppercase (although the uppercase bit hasn't been
consistently applied), or to use the full name in lowercase.
Yes, it is the stock ticker symbol. Then we shall follow the convention,
and change it to uppercase.
- Thomas
I thought I had seen 'altera' instead of an abbreviation being used in
a previous patch. I don't care much whether 'altr' or 'altera' is
used, but I'd like to know that there is consensus from the Altera
users so that all the drivers use the same prefix.
We had discussed on nios2-dev mailing list, and decided to use
'altr' as Walter suggested that it saves space.
Is altr the stock ticker symbol? The convention is to either use the
stock ticker in all uppercase (although the uppercase bit hasn't been
consistently applied), or to use the full name in lowercase.
g.
Risking my limbs here by breaking in this late in the discussion... (I
wasn't able to reply earlier) but where does it state it needs to be
uppercase? I found a bunch of microblaze code which seems to use the
lowercase xlnx and freescale seems happy with fsl. Unless I'm missing
something obvious here I guess ALTR would actually be the first to use
uppercase.
The only reference to uppercase I found in the ePAPR docs was chapter
1.6 that talks about uppercase hex-characters as an OUI.
Not that I terribly mind either way, but I want to double-check before
we go ahead and change all altera-related devicetree stuff to uppercase.
Greetz
Walter
I thought I had seen 'altera' instead of an abbreviation being used in
a previous patch. I don't care much whether 'altr' or 'altera' is
used, but I'd like to know that there is consensus from the Altera
users so that all the drivers use the same prefix.
We had discussed on nios2-dev mailing list, and decided to use
'altr' as Walter suggested that it saves space.
Is altr the stock ticker symbol? The convention is to either use the
stock ticker in all uppercase (although the uppercase bit hasn't been
consistently applied), or to use the full name in lowercase.
g.
Risking my limbs here by breaking in this late in the discussion... (I
wasn't able to reply earlier) but where does it state it needs to be
uppercase? I found a bunch of microblaze code which seems to use the
lowercase xlnx and freescale seems happy with fsl. Unless I'm missing
something obvious here I guess ALTR would actually be the first to use
uppercase.
The only reference to uppercase I found in the ePAPR docs was chapter
1.6 that talks about uppercase hex-characters as an OUI.
Not that I terribly mind either way, but I want to double-check before
we go ahead and change all altera-related devicetree stuff to uppercase.
The relevant text in IEEE 1275-1994 is in the description of the "name"
property in Annex A. If a node name begins with a sequence of from one
to five uppercase letters followed by a comma, that means a stock symbol
on some exchange whose names do not conflict with NYSE or NASDAQ.
A lower-case prefix is okay, but it does not necessarily mean that it is
a ticker symbol. So in some sense, a lower case prefix provides less
protection against collisions than an upper case prefix, which comes
from an externally-arbitrated name space. Case is explicitly
significant in node names.
In practice, the important thing is that names must not conflict. Name
collisions haven't been much of a problem so far.
I thought I had seen 'altera' instead of an abbreviation being
used in
a previous patch. I don't care much whether 'altr' or 'altera' is
used, but I'd like to know that there is consensus from the Altera
users so that all the drivers use the same prefix.
We had discussed on nios2-dev mailing list, and decided to use
'altr' as Walter suggested that it saves space.
Is altr the stock ticker symbol? The convention is to either use the
stock ticker in all uppercase (although the uppercase bit hasn't been
consistently applied), or to use the full name in lowercase.
g.
Risking my limbs here by breaking in this late in the discussion... (I
wasn't able to reply earlier) but where does it state it needs to be
uppercase? I found a bunch of microblaze code which seems to use the
lowercase xlnx and freescale seems happy with fsl. Unless I'm missing
something obvious here I guess ALTR would actually be the first to use
uppercase.
The only reference to uppercase I found in the ePAPR docs was chapter
1.6 that talks about uppercase hex-characters as an OUI.
Not that I terribly mind either way, but I want to double-check before
we go ahead and change all altera-related devicetree stuff to uppercase.
The relevant text in IEEE 1275-1994 is in the description of the
"name" property in Annex A. If a node name begins with a sequence of
from one to five uppercase letters followed by a comma, that means a
stock symbol on some exchange whose names do not conflict with NYSE or
NASDAQ.
A lower-case prefix is okay, but it does not necessarily mean that it
is a ticker symbol. So in some sense, a lower case prefix provides
less protection against collisions than an upper case prefix, which
comes from an externally-arbitrated name space. Case is explicitly
significant in node names.
In practice, the important thing is that names must not conflict.
Name collisions haven't been much of a problem so far.
Excellent!
Must have missed that one. Good reason. We'll change 'm!
Thanks
Walter