[PATCH v3 0/2] rtc: omap: update AM335x rtc ip revision

[PATCH v3 0/2] rtc: omap: update AM335x rtc ip revision

[Hebbar, Gururaja]

The syntax of compatible property in DT is to mention the Most specific
match to most generic match.

Since AM335x is the platform with latest IP revision, add it 1st in
the device id table.

This way, we can add new matching compatible as 1st and maintain old
compatible string for backwards compatibility.

ex:
	compatible = "ti,am3352-rtc", "ti,da830-rtc";

Also, update am335x .dtsi file (am33xx.dtsi) as above.

Note:
This is a part of previously submitted patch-set [1]. Out of 4 patches,
3 was accepted and 1 got an acceptable NAK. Hence resubmitting only the
rejected + updated changes.

Changes in V3:
	- As per Mark Rutland's suggestion [2], maintain old compatible
	  string and add the new matching string as the 1st compatible.

Changes in V2:
	- Coding style corrections (remove extra space, use lower case
	  for hex numbers
	- use prefix ARM: for commit subject keeping with arch/arm
	  convention)
	- use "[AM/am]3352" instead of "[AM/am]335x" to keep the all
	  usages in sync.
	- Use index defined for struct members so they remain in sync
	- Add new compatible to existing one so that when driver
	  supports enhanced features of hardware, they are available
	  to the user else the basic functionality still works

[2]
https://lkml.org/lkml/2013/7/3/74
[1]
https://lkml.org/lkml/2013/8/1/442

Hebbar Gururaja (1):
  ARM: dts: AM33XX: update rtc node compatibility

Hebbar, Gururaja (1):
  rtc: omap: update of_device_id to reflect latest ip revisions

 arch/arm/boot/dts/am33xx.dtsi |    2 +-
 drivers/rtc/rtc-omap.c        |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

[PATCH v3 1/2] rtc: omap: update of_device_id to reflect latest ip revisions

[Hebbar, Gururaja]

The syntax of compatible property in DT is to mention the Most specific
match to most generic match.

Since AM335x is the platform with latest IP revision, add it 1st in
the device id table.

This way, we can add new matching compatible as 1st and maintain old
compatible string for backwards compatibility.

ex:
	compatible = "ti,am3352-rtc", "ti,da830-rtc";

Signed-off-by: Hebbar, Gururaja <gururaja.hebbar@ti.com>
CC: mark.rutland at arm.com
---
Changes in v3:
	- new patch

:100644 100644 dc62cc3... 2f0968c... M	drivers/rtc/rtc-omap.c
 drivers/rtc/rtc-omap.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
index dc62cc3..2f0968c 100644
--- a/drivers/rtc/rtc-omap.c
+++ b/drivers/rtc/rtc-omap.c
@@ -330,12 +330,12 @@ static struct platform_device_id omap_rtc_devtype[] = {
 MODULE_DEVICE_TABLE(platform, omap_rtc_devtype);
 
 static const struct of_device_id omap_rtc_of_match[] = {
-	{	.compatible	= "ti,da830-rtc",
-		.data		= &omap_rtc_devtype[OMAP_RTC_DATA_DA830_IDX],
-	},
 	{	.compatible	= "ti,am3352-rtc",
 		.data		= &omap_rtc_devtype[OMAP_RTC_DATA_AM335X_IDX],
 	},
+	{	.compatible	= "ti,da830-rtc",
+		.data		= &omap_rtc_devtype[OMAP_RTC_DATA_DA830_IDX],
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, omap_rtc_of_match);
-- 
1.7.0.4

[PATCH v3 1/2] rtc: omap: update of_device_id to reflect latest ip revisions

[Benoit Cousson]

Hi Gururaja,

On 16/08/2013 13:36, Hebbar, Gururaja wrote:
> The syntax of compatible property in DT is to mention the Most specific
> match to most generic match.
>
> Since AM335x is the platform with latest IP revision, add it 1st in
> the device id table.

I don't understand why? The order should not matter at all.

I've tried to follow the thread you had with Mark on the v2, but AFAIK, 
you've never answered to his latest question.

Moreover, checking the differences between the Davinci and the am3352 
RTC IP, I would not claim that both are compatible.

Sure you can use the am3352 with the Davinci driver, but you will lose 
the wakeup functionality without even being notify about that.

For my point of view, compatible mean that the HW will still be fully 
functional with both versions of the driver, which is not the case here.

am3352 DTS must use the ti,am3352-rtc to have the expected behavior. 
Using the ti,da830-rtc version will not make the board working as 
expected. So we cannot claim the compatibility.

> This way, we can add new matching compatible as 1st and maintain old
> compatible string for backwards compatibility.
>
> ex:
> 	compatible = "ti,am3352-rtc", "ti,da830-rtc";
>
> Signed-off-by: Hebbar, Gururaja <gururaja.hebbar@ti.com>
> CC: mark.rutland at arm.com
> ---
> Changes in v3:
> 	- new patch
>
> :100644 100644 dc62cc3... 2f0968c... M	drivers/rtc/rtc-omap.c
>   drivers/rtc/rtc-omap.c |    6 +++---
>   1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
> index dc62cc3..2f0968c 100644
> --- a/drivers/rtc/rtc-omap.c
> +++ b/drivers/rtc/rtc-omap.c
> @@ -330,12 +330,12 @@ static struct platform_device_id omap_rtc_devtype[] = {
>   MODULE_DEVICE_TABLE(platform, omap_rtc_devtype);
>
>   static const struct of_device_id omap_rtc_of_match[] = {
> -	{	.compatible	= "ti,da830-rtc",
> -		.data		= &omap_rtc_devtype[OMAP_RTC_DATA_DA830_IDX],
> -	},
>   	{	.compatible	= "ti,am3352-rtc",
>   		.data		= &omap_rtc_devtype[OMAP_RTC_DATA_AM335X_IDX],
>   	},
> +	{	.compatible	= "ti,da830-rtc",
> +		.data		= &omap_rtc_devtype[OMAP_RTC_DATA_DA830_IDX],
> +	},
>   	{},
>   };
>   MODULE_DEVICE_TABLE(of, omap_rtc_of_match);

Bottom-line, if you get rid of the old compatible entry, you will not 
have to play some trick with the entries order.

Regards,
Benoit

[PATCH v3 1/2] rtc: omap: update of_device_id to reflect latest ip revisions

[Sekhar Nori]

On 8/16/2013 7:45 PM, Benoit Cousson wrote:
> Hi Gururaja,
> 
> On 16/08/2013 13:36, Hebbar, Gururaja wrote:
>> The syntax of compatible property in DT is to mention the Most specific
>> match to most generic match.
>>
>> Since AM335x is the platform with latest IP revision, add it 1st in
>> the device id table.
> 
> I don't understand why? The order should not matter at all.

Yes, it should not. We are trying to work around a bug in the kernel
where the compatible order is not honored (instead the order in
of_match[] array matters). There were patches being discussed to fix this.

> 
> I've tried to follow the thread you had with Mark on the v2, but AFAIK,
> you've never answered to his latest question.
> 
> Moreover, checking the differences between the Davinci and the am3352
> RTC IP, I would not claim that both are compatible.
> 
> Sure you can use the am3352 with the Davinci driver, but you will lose
> the wakeup functionality without even being notify about that.

When the kernel is fixed for the bug pointed out above, this should not
happen with properly defined compatible property.

> 
> For my point of view, compatible mean that the HW will still be fully
> functional with both versions of the driver, which is not the case here.

I do not think that's the interpretation of compatible. Its goes from
most specific to most generic per the ePAPR spec. That in itself says
that 100% functionality is not expected if you don't find a match for
the more specific property.

> 
> am3352 DTS must use the ti,am3352-rtc to have the expected behavior.

Yes, that's what patch 2/2 does.

> Using the ti,da830-rtc version will not make the board working as
> expected. So we cannot claim the compatibility.

Ideally, the DT file was *always* written as

	compatible = "ti,am3352-rtc", "ti,da830-rtc";

even when there was no kernel support for AM3352 RTC.

That way, there is no need to update the .dts[i] file. As kernel gains
functionality, more features (like rtc wake) is available to users.
Otherwise they get plain RTC functionality - but at least they get
something instead of no RTC.

Thanks,
Sekhar

[PATCH v3 1/2] rtc: omap: update of_device_id to reflect latest ip revisions

[Benoit Cousson]

Hi Sekhar,

On 16/08/2013 17:41, Sekhar Nori wrote:
> 
> On 8/16/2013 7:45 PM, Benoit Cousson wrote:
>> Hi Gururaja,
>>
>> On 16/08/2013 13:36, Hebbar, Gururaja wrote:
>>> The syntax of compatible property in DT is to mention the Most specific
>>> match to most generic match.
>>>
>>> Since AM335x is the platform with latest IP revision, add it 1st in
>>> the device id table.
>>
>> I don't understand why? The order should not matter at all.
> 
> Yes, it should not. We are trying to work around a bug in the kernel
> where the compatible order is not honored (instead the order in
> of_match[] array matters). There were patches being discussed to fix this.
> 
>>
>> I've tried to follow the thread you had with Mark on the v2, but AFAIK,
>> you've never answered to his latest question.
>>
>> Moreover, checking the differences between the Davinci and the am3352
>> RTC IP, I would not claim that both are compatible.
>>
>> Sure you can use the am3352 with the Davinci driver, but you will lose
>> the wakeup functionality without even being notify about that.
> 
> When the kernel is fixed for the bug pointed out above, this should not
> happen with properly defined compatible property.
> 
>>
>> For my point of view, compatible mean that the HW will still be fully
>> functional with both versions of the driver, which is not the case here.
> 
> I do not think that's the interpretation of compatible. Its goes from
> most specific to most generic per the ePAPR spec. That in itself says
> that 100% functionality is not expected if you don't find a match for
> the more specific property.

Well, to be honest I checked as well the official documentation, and this is far from being clear:

page 21 of the ePAPR:

"
Property: compatible
Value type: <stringlist>

Description:
 The compatible property value consists of one or more strings that define the specific 
 programming model for the device. This list of strings should be used by a client program for
 device driver selection. The property value consists of a concatenated list of null terminated
 strings, from most specific to most general. They allow a device to express its compatibility
 with a family of similar devices, potentially allowing a single device driver to match against
 several devices.
 
 The recommended format is ?manufacturer,model?, where manufacturer is a
 string describing the name of the manufacturer (such as a stock ticker symbol), and model
 specifies the model number.

Example:
 compatible = ?fsl,mpc8641-uart?, ?ns16550";
 In this example, an operating system would first try to locate a device driver that supported
 fsl,mpc8641-uart. If a driver was not found, it would then try to locate a driver that supported
 the more general ns16550 device type.
"

In this example, the generic vs specific is just about the driver. You could have one driver that manage 10 different UARTs or a specific one that manage only your HW. 

There is no assumption about the lost of functionality by using the generic version of the driver. How the user is supposed to know the amount of functionality he will lose, and if this is acceptable to him.
I'd rather have an error at boot saying that the driver is not compatible with the HW and thus I will have to upgrade the kernel, than booting a kernel that will not work as expected because some functionality are missing for that specific HW.

Claiming that a piece of HW is compatible with an other one that is just a subset in term of functionality is wrong. You can claim that the ti,da830-rtc is compatible with the ti,am3352-rtc driver, but not the opposite.

>> am3352 DTS must use the ti,am3352-rtc to have the expected behavior.
> 
> Yes, that's what patch 2/2 does.
> 
>> Using the ti,da830-rtc version will not make the board working as
>> expected. So we cannot claim the compatibility.
> 
> Ideally, the DT file was *always* written as
> 
> 	compatible = "ti,am3352-rtc", "ti,da830-rtc";
> 
> even when there was no kernel support for AM3352 RTC.

You could do that only for the davinci DTS, because it is a subset of the am3352 but you cannot do that for the am3352 as explained before.

> That way, there is no need to update the .dts[i] file. As kernel gains
> functionality, more features (like rtc wake) is available to users.

Well, you cannot anticipate the HW evolution anyway and you did not know when Davinci DTS was done that an am3352 will exist in the future and reuse the same IP.
Moreover DT is a ABI, so extending it according to the HW feature evolution is absolutely normal.

Every SoC that are using a RTC with the same programing model than the da830 can claim the compatibility. A SoC that is using a newer version of the IP with an extended programming model cannot claim that.

> Otherwise they get plain RTC functionality - but at least they get
> something instead of no RTC.

Because you assume that this feature is not important and thus you can use the plain RTC, but what if someone is buying that HW because of this new feature. Without that feature his system will just not work properly.
Saying that this is compatible whereas you lost functionality is lying to the customer for my point of view.

Regards,
Benoit

[PATCH v3 1/2] rtc: omap: update of_device_id to reflect latest ip revisions

[Sekhar Nori]

Hi Benoit,

Did you get a chance to think about this, I have provided some replies
below.

On Friday 16 August 2013 10:03 PM, Benoit Cousson wrote:
> Hi Sekhar,
> 
> On 16/08/2013 17:41, Sekhar Nori wrote:
>>
>> On 8/16/2013 7:45 PM, Benoit Cousson wrote:
>>> Hi Gururaja,
>>>
>>> On 16/08/2013 13:36, Hebbar, Gururaja wrote:
>>>> The syntax of compatible property in DT is to mention the Most specific
>>>> match to most generic match.
>>>>
>>>> Since AM335x is the platform with latest IP revision, add it 1st in
>>>> the device id table.
>>>
>>> I don't understand why? The order should not matter at all.
>>
>> Yes, it should not. We are trying to work around a bug in the kernel
>> where the compatible order is not honored (instead the order in
>> of_match[] array matters). There were patches being discussed to fix this.
>>
>>>
>>> I've tried to follow the thread you had with Mark on the v2, but AFAIK,
>>> you've never answered to his latest question.
>>>
>>> Moreover, checking the differences between the Davinci and the am3352
>>> RTC IP, I would not claim that both are compatible.
>>>
>>> Sure you can use the am3352 with the Davinci driver, but you will lose
>>> the wakeup functionality without even being notify about that.
>>
>> When the kernel is fixed for the bug pointed out above, this should not
>> happen with properly defined compatible property.
>>
>>>
>>> For my point of view, compatible mean that the HW will still be fully
>>> functional with both versions of the driver, which is not the case here.
>>
>> I do not think that's the interpretation of compatible. Its goes from
>> most specific to most generic per the ePAPR spec. That in itself says
>> that 100% functionality is not expected if you don't find a match for
>> the more specific property.
> 
> Well, to be honest I checked as well the official documentation, and this is far from being clear:
> 
> page 21 of the ePAPR:
> 
> "
> Property: compatible
> Value type: <stringlist>
> 
> Description:
>  The compatible property value consists of one or more strings that define the specific 
>  programming model for the device. This list of strings should be used by a client program for
>  device driver selection. The property value consists of a concatenated list of null terminated
>  strings, from most specific to most general. They allow a device to express its compatibility
>  with a family of similar devices, potentially allowing a single device driver to match against
>  several devices.
>  
>  The recommended format is ?manufacturer,model?, where manufacturer is a
>  string describing the name of the manufacturer (such as a stock ticker symbol), and model
>  specifies the model number.
> 
> Example:
>  compatible = ?fsl,mpc8641-uart?, ?ns16550";
>  In this example, an operating system would first try to locate a device driver that supported
>  fsl,mpc8641-uart. If a driver was not found, it would then try to locate a driver that supported
>  the more general ns16550 device type.
> "
> 
> In this example, the generic vs specific is just about the driver. You could have one driver that manage 10 different UARTs or a specific one that manage only your HW. 

Right, the assumption is that a specific driver is written because there
are parts of the hardware that cannot be serviced by generic driver. So
ultimately its all driven by changes in hardware.

> 
> There is no assumption about the lost of functionality by using the generic version of the driver. How the user is supposed to know the amount of functionality he will lose, and if this is acceptable to him.

I suppose the generic driver would return some error code like -ENOTSUP
for the functionality it cannot provide.

> I'd rather have an error at boot saying that the driver is not compatible with the HW and thus I will have to upgrade the kernel, than booting a kernel that will not work as expected because some functionality are missing for that specific HW.

If you have an update available, you can always upgrade to it. But what
if there is no update available? Would you rather not have the user use
the available functionality in the meanwhile while he waits for the
kernel support to be developed.

> 
> Claiming that a piece of HW is compatible with an other one that is just a subset in term of functionality is wrong. You can claim that the ti,da830-rtc is compatible with the ti,am3352-rtc driver, but not the opposite.

If this is wrong, then what is the use of compatible property? Why would
someone write a driver supporting ?fsl,mpc8641-uart? if 100% of its
hardware features are also supported by ?ns16550" driver?

>>> am3352 DTS must use the ti,am3352-rtc to have the expected behavior.
>>
>> Yes, that's what patch 2/2 does.
>>
>>> Using the ti,da830-rtc version will not make the board working as
>>> expected. So we cannot claim the compatibility.
>>
>> Ideally, the DT file was *always* written as
>>
>> 	compatible = "ti,am3352-rtc", "ti,da830-rtc";
>>
>> even when there was no kernel support for AM3352 RTC.
> 
> You could do that only for the davinci DTS, because it is a subset of the am3352 but you cannot do that for the am3352 as explained before.

DaVinci DTS will use compatible = "ti,da830-rtc"; Thats not changing
with this patchset.

> 
>> That way, there is no need to update the .dts[i] file. As kernel gains
>> functionality, more features (like rtc wake) is available to users.
> 
> Well, you cannot anticipate the HW evolution anyway and you did not know when Davinci DTS was done that an am3352 will exist in the future and reuse the same IP.

Yeah, I am not claiming DaVinci DTS should be updated. But when
am335x.dtsi was written, there was knowledge that a (potentially) new
version of RTC IP is available and therefore the .dtsi should have been
written as

	compatible = "ti,am3352-rtc", "ti,da830-rtc";

instead of

	compatible = "ti,da830-rtc";

that it is today. Thing is, you can never know if the IP needs any
additional handling even after reading the spec. We keep discovering new
features/quirks. So, when writing <new-soc>.dtsi its safer to always use

	compatible = "ti,<new-soc>-rtc", "ti,<compatible-soc>-rtc";

even though _today_ you may not have code that specifically handles
"ti,<new-soc>-rtc".

> Moreover DT is a ABI, so extending it according to the HW feature evolution is absolutely normal.

You mean the DT bindings and not specifically the .dts[i], right? Then I
agree. Ideally the .dts[i] is written once per hardware. I know its
almost impossible to get that right.

> 
> Every SoC that are using a RTC with the same programing model than the da830 can claim the compatibility. A SoC that is using a newer version of the IP with an extended programming model cannot claim that.

We differ here. As long as programming model is _extended_, new IP is
still compatible with old. If the programming model is _changed_ then
its not.

> 
>> Otherwise they get plain RTC functionality - but at least they get
>> something instead of no RTC.
> 
> Because you assume that this feature is not important and thus you can use the plain RTC, but what if someone is buying that HW because of this new feature. Without that feature his system will just not work properly.

Right, but thats not a problem DT can solve. He/she needs to hassle TI
for updated drivers. But there could be 10 other customers who are just
okay with plain RTC. So till the time driver is updated to use
ti,am3352-rtc", are you saying no one should be able to use the RTC on
the SoC at all even though 90% features are available?

> Saying that this is compatible whereas you lost functionality is lying to the customer for my point of view.

If 100% functionality is required for compatibility then I am afraid
there is nothing like "compatibility". There are just different isolated
versions.

Thanks,
Sekhar

[PATCH v3 1/2] rtc: omap: update of_device_id to reflect latest ip revisions

[Benoit Cousson]

Hi Sekhar,

On 23/08/2013 10:50, Sekhar Nori wrote:
> Hi Benoit,
>
> Did you get a chance to think about this, I have provided some
> replies below.
>
> On Friday 16 August 2013 10:03 PM, Benoit Cousson wrote:
>> Hi Sekhar,
>>
>> On 16/08/2013 17:41, Sekhar Nori wrote:
>>>
>>> On 8/16/2013 7:45 PM, Benoit Cousson wrote:
>>>> Hi Gururaja,
>>>>
>>>> On 16/08/2013 13:36, Hebbar, Gururaja wrote:
>>>>> The syntax of compatible property in DT is to mention the
>>>>> Most specific match to most generic match.
>>>>>
>>>>> Since AM335x is the platform with latest IP revision, add it
>>>>> 1st in the device id table.
>>>>
>>>> I don't understand why? The order should not matter at all.
>>>
>>> Yes, it should not. We are trying to work around a bug in the
>>> kernel where the compatible order is not honored (instead the
>>> order in of_match[] array matters). There were patches being
>>> discussed to fix this.
>>>
>>>>
>>>> I've tried to follow the thread you had with Mark on the v2,
>>>> but AFAIK, you've never answered to his latest question.
>>>>
>>>> Moreover, checking the differences between the Davinci and the
>>>> am3352 RTC IP, I would not claim that both are compatible.
>>>>
>>>> Sure you can use the am3352 with the Davinci driver, but you
>>>> will lose the wakeup functionality without even being notify
>>>> about that.
>>>
>>> When the kernel is fixed for the bug pointed out above, this
>>> should not happen with properly defined compatible property.
>>>
>>>>
>>>> For my point of view, compatible mean that the HW will still be
>>>> fully functional with both versions of the driver, which is not
>>>> the case here.
>>>
>>> I do not think that's the interpretation of compatible. Its goes
>>> from most specific to most generic per the ePAPR spec. That in
>>> itself says that 100% functionality is not expected if you don't
>>> find a match for the more specific property.
>>
>> Well, to be honest I checked as well the official documentation,
>> and this is far from being clear:
>>
>> page 21 of the ePAPR:
>>
>> " Property: compatible Value type: <stringlist>
>>
>> Description: The compatible property value consists of one or more
>> strings that define the specific programming model for the device.
>> This list of strings should be used by a client program for device
>> driver selection. The property value consists of a concatenated
>> list of null terminated strings, from most specific to most
>> general. They allow a device to express its compatibility with a
>> family of similar devices, potentially allowing a single device
>> driver to match against several devices.
>>
>> The recommended format is ?manufacturer,model?, where manufacturer
>> is a string describing the name of the manufacturer (such as a
>> stock ticker symbol), and model specifies the model number.
>>
>> Example: compatible = ?fsl,mpc8641-uart?, ?ns16550"; In this
>> example, an operating system would first try to locate a device
>> driver that supported fsl,mpc8641-uart. If a driver was not found,
>> it would then try to locate a driver that supported the more
>> general ns16550 device type. "
>>
>> In this example, the generic vs specific is just about the driver.
>> You could have one driver that manage 10 different UARTs or a
>> specific one that manage only your HW.
>
> Right, the assumption is that a specific driver is written because
> there are parts of the hardware that cannot be serviced by generic
> driver. So ultimately its all driven by changes in hardware.

Yeah, that example remind me the omap-serial driver we had done to 
handle the DMA mode that was not supported by the generic one.

>> There is no assumption about the lost of functionality by using the
>> generic version of the driver. How the user is supposed to know the
>> amount of functionality he will lose, and if this is acceptable to
>> him.
>
> I suppose the generic driver would return some error code like
> -ENOTSUP for the functionality it cannot provide.

Well, I guess it depends of the added functionality add how far the IP 
version is forward compatible with the old driver. If the new IP version 
is just improving the performance by adding a DMA mode instead of the 
PIO, then the driver API can remain the same.
But if you add a new functionality that will require an extended API, 
there is no way you can report such error. Except if the API itself is 
done with a good forward compatibility support.

And if the new functionality is mandatory to make the new system 
operational, returning an error might just make the system not working 
at all.

>> I'd rather have an error at boot saying that the driver is not
>> compatible with the HW and thus I will have to upgrade the kernel,
>> than booting a kernel that will not work as expected because some
>> functionality are missing for that specific HW.
>
> If you have an update available, you can always upgrade to it. But
> what if there is no update available? Would you rather not have the
> user use the available functionality in the meanwhile while he waits
> for the kernel support to be developed.

It depends... In the UART example, that's just a matter of performance 
improvement, so keeping the old version is fine.
As soon as the driver is usable, and the new feature is not mandatory to 
ensure the system will work properly, I guess it is fine.

>> Claiming that a piece of HW is compatible with an other one that is
>> just a subset in term of functionality is wrong. You can claim that
>> the ti,da830-rtc is compatible with the ti,am3352-rtc driver, but
>> not the opposite.
>
> If this is wrong, then what is the use of compatible property?

That's a good question :-) And that's why I was always confused with 
that property.

> Why
> would someone write a driver supporting ?fsl,mpc8641-uart? if 100% of
> its hardware features are also supported by ?ns16550" driver?

That's still doable, if you want to reduce the size of a big generic 
driver into a smaller specific driver. The point is that the compatible 
value does not have any assumption about the driver that will handle it.

The other issue is that we are supposed to always use the latest SoC 
version even if the IP is 100% the same. Like

omap5-timer {
	compatible = "omap5-timer", "omap4-timer";
}

So how are you suppose to differentiate the same IP, and a compatible IP???

That's what I don't like in that compatible string definition. You do 
not necessarily know the amount of compatibility you are talking about.

>>>> am3352 DTS must use the ti,am3352-rtc to have the expected
>>>> behavior.
>>>
>>> Yes, that's what patch 2/2 does.
>>>
>>>> Using the ti,da830-rtc version will not make the board working
>>>> as expected. So we cannot claim the compatibility.
>>>
>>> Ideally, the DT file was *always* written as
>>>
>>> compatible = "ti,am3352-rtc", "ti,da830-rtc";
>>>
>>> even when there was no kernel support for AM3352 RTC.
>>
>> You could do that only for the davinci DTS, because it is a subset
>> of the am3352 but you cannot do that for the am3352 as explained
>> before.
>
> DaVinci DTS will use compatible = "ti,da830-rtc"; Thats not changing
> with this patchset.
>
>>
>>> That way, there is no need to update the .dts[i] file. As kernel
>>> gains functionality, more features (like rtc wake) is available
>>> to users.
>>
>> Well, you cannot anticipate the HW evolution anyway and you did not
>> know when Davinci DTS was done that an am3352 will exist in the
>> future and reuse the same IP.
>
> Yeah, I am not claiming DaVinci DTS should be updated. But when
> am335x.dtsi was written, there was knowledge that a (potentially)
> new version of RTC IP is available and therefore the .dtsi should
> have been written as
>
> compatible = "ti,am3352-rtc", "ti,da830-rtc";
>
> instead of
>
> compatible = "ti,da830-rtc";
>
> that it is today. Thing is, you can never know if the IP needs any
> additional handling even after reading the spec. We keep discovering
> new features/quirks. So, when writing <new-soc>.dtsi its safer to
> always use
>
> compatible = "ti,<new-soc>-rtc", "ti,<compatible-soc>-rtc";
>
> even though _today_ you may not have code that specifically handles
> "ti,<new-soc>-rtc".

Well, we can do that, but honestly, I do not see the need. You can 
always update the dts later. Why would we add hundreds more compatible 
strings just in case few devices will need special handling. That's 
overkill.
If was maybe easy and harmless in the good old PPC time when the DTS 
files were relatively small, but the ARM DTS files are much bigger.

In the name of the Keep It Simple Principle, I would just avoid adding 
something just because it might be useful in 5 % of the cases.

>> Moreover DT is a ABI, so extending it according to the HW feature
>> evolution is absolutely normal.
>
> You mean the DT bindings and not specifically the .dts[i], right?

Yes, you're right, sorry for the confusing sentence.

> Then I agree. Ideally the .dts[i] is written once per hardware. I
> know its almost impossible to get that right.
>
>>
>> Every SoC that are using a RTC with the same programing model than
>> the da830 can claim the compatibility. A SoC that is using a newer
>> version of the IP with an extended programming model cannot claim
>> that.
>
> We differ here. As long as programming model is _extended_, new IP
> is still compatible with old. If the programming model is _changed_
> then its not.

Yes, I was assuming compatible as a 100% match which is not really 
compatible but identical in that case. But the current syntax, does not 
allow you anyway to differentiate a new SoC with the exact same IP 
version to a different for compatible IP.

>>> Otherwise they get plain RTC functionality - but at least they
>>> get something instead of no RTC.
>>
>> Because you assume that this feature is not important and thus you
>> can use the plain RTC, but what if someone is buying that HW
>> because of this new feature. Without that feature his system will
>> just not work properly.
>
> Right, but thats not a problem DT can solve. He/she needs to hassle
> TI for updated drivers. But there could be 10 other customers who are
> just okay with plain RTC. So till the time driver is updated to use
> ti,am3352-rtc", are you saying no one should be able to use the RTC
> on the SoC at all even though 90% features are available?

Again, if it will not prevent the system to work properly, then it is 
fine. But let's assume that without the wakeup RTC functionality, you 
just cannot wakeup from suspend an am3352 board. Then you end up with a 
non functional system for the PM point of view.

Someone who is not aware that the compatible RTC is not supporting that 
feature might spend some time figuring out why he cannot wakeup from 
suspend on a RTC alarm.

In general, I'd rather have nothing than something that is working at 50%.
But maybe that's just me :-)

>> Saying that this is compatible whereas you lost functionality is
>> lying to the customer for my point of view.
>
> If 100% functionality is required for compatibility then I am afraid
> there is nothing like "compatibility". There are just different
> isolated versions.

I guess you are right.

Bottom-line, I'm really disappointed but that lack of accuracy in the 
compatible string, but I guess, it was done for what you guys are doing.
And maybe, it is something that should just be well documented in the 
bindings to avoid confusing the users.

Regards,
Benoit

[PATCH v3 1/2] rtc: omap: update of_device_id to reflect latest ip revisions

[Sekhar Nori]

On 8/23/2013 8:40 PM, Benoit Cousson wrote:

>>> There is no assumption about the lost of functionality by using the
>>> generic version of the driver. How the user is supposed to know the
>>> amount of functionality he will lose, and if this is acceptable to
>>> him.
>>
>> I suppose the generic driver would return some error code like
>> -ENOTSUP for the functionality it cannot provide.
> 
> Well, I guess it depends of the added functionality add how far the IP
> version is forward compatible with the old driver. If the new IP version
> is just improving the performance by adding a DMA mode instead of the
> PIO, then the driver API can remain the same.
> But if you add a new functionality that will require an extended API,
> there is no way you can report such error. Except if the API itself is
> done with a good forward compatibility support.
> 
> And if the new functionality is mandatory to make the new system
> operational, returning an error might just make the system not working
> at all.

If the driver cannot work at all, then yes, I would not claim
compatibility. At least a subset of functionality should work.

> 
>> Why
>> would someone write a driver supporting ?fsl,mpc8641-uart? if 100% of
>> its hardware features are also supported by ?ns16550" driver?
> 
> That's still doable, if you want to reduce the size of a big generic
> driver into a smaller specific driver. The point is that the compatible

Thats plausible. Although I have to admit I have never seen a new driver
being written just because another existing driver is bloated.

> value does not have any assumption about the driver that will handle it.

Right. Its all driven by hardware changes.

> 
> The other issue is that we are supposed to always use the latest SoC
> version even if the IP is 100% the same. Like
> 
> omap5-timer {
>     compatible = "omap5-timer", "omap4-timer";
> }
> 
> So how are you suppose to differentiate the same IP, and a compatible IP???
> 
> That's what I don't like in that compatible string definition. You do
> not necessarily know the amount of compatibility you are talking about.

That's correct. The strings themselves tell very little how much OMAP5
timer works when compatible = "omap4-timer" is passed. Some known
limitations can perhaps be documented in binding definition.

>> that it is today. Thing is, you can never know if the IP needs any
>> additional handling even after reading the spec. We keep discovering
>> new features/quirks. So, when writing <new-soc>.dtsi its safer to
>> always use
>>
>> compatible = "ti,<new-soc>-rtc", "ti,<compatible-soc>-rtc";
>>
>> even though _today_ you may not have code that specifically handles
>> "ti,<new-soc>-rtc".
> 
> Well, we can do that, but honestly, I do not see the need. You can
> always update the dts later. Why would we add hundreds more compatible
> strings just in case few devices will need special handling. That's
> overkill.
> If was maybe easy and harmless in the good old PPC time when the DTS
> files were relatively small, but the ARM DTS files are much bigger.
> 
> In the name of the Keep It Simple Principle, I would just avoid adding
> something just because it might be useful in 5 % of the cases.

It certainly seems overkill today. If and when the .dts[i] files are
maintained as a separate project it will become painful to keep kernel
and .dtb in sync. This will then become important, as bootloader
independence today is.

>>>> Otherwise they get plain RTC functionality - but at least they
>>>> get something instead of no RTC.
>>>
>>> Because you assume that this feature is not important and thus you
>>> can use the plain RTC, but what if someone is buying that HW
>>> because of this new feature. Without that feature his system will
>>> just not work properly.
>>
>> Right, but thats not a problem DT can solve. He/she needs to hassle
>> TI for updated drivers. But there could be 10 other customers who are
>> just okay with plain RTC. So till the time driver is updated to use
>> ti,am3352-rtc", are you saying no one should be able to use the RTC
>> on the SoC at all even though 90% features are available?
> 
> Again, if it will not prevent the system to work properly, then it is
> fine. But let's assume that without the wakeup RTC functionality, you
> just cannot wakeup from suspend an am3352 board. Then you end up with a
> non functional system for the PM point of view.

Correct, but this is because of lack of kernel support for a feature.
Not because of the way compatible is written.

> Someone who is not aware that the compatible RTC is not supporting that
> feature might spend some time figuring out why he cannot wakeup from
> suspend on a RTC alarm.

Right. DT/compatible does not make this problem better or worse. Even
using platform device model, you would still end up with a partially
working system.

>>> Saying that this is compatible whereas you lost functionality is
>>> lying to the customer for my point of view.
>>
>> If 100% functionality is required for compatibility then I am afraid
>> there is nothing like "compatibility". There are just different
>> isolated versions.
> 
> I guess you are right.
> 
> Bottom-line, I'm really disappointed but that lack of accuracy in the
> compatible string, but I guess, it was done for what you guys are doing.
> And maybe, it is something that should just be well documented in the
> bindings to avoid confusing the users.

Okay. Can you please see if you can take 2/2 for v3.12? It can be taken
independently of 1/2 (which I guess akpm will pick up).

Thanks,
Sekhar

[PATCH v3 1/2] rtc: omap: update of_device_id to reflect latest ip revisions

[Mark Rutland]

Hi Benoit,

On Fri, Aug 16, 2013 at 03:15:57PM +0100, Benoit Cousson wrote:
> Hi Gururaja,
> 
> On 16/08/2013 13:36, Hebbar, Gururaja wrote:
> > The syntax of compatible property in DT is to mention the Most specific
> > match to most generic match.
> >
> > Since AM335x is the platform with latest IP revision, add it 1st in
> > the device id table.
> 
> I don't understand why? The order should not matter at all.
> 
> I've tried to follow the thread you had with Mark on the v2, but AFAIK, 
> you've never answered to his latest question.
> 
> Moreover, checking the differences between the Davinci and the am3352 
> RTC IP, I would not claim that both are compatible.
> 
> Sure you can use the am3352 with the Davinci driver, but you will lose 
> the wakeup functionality without even being notify about that.

Could you describe the wakeup functionality, and how it differs between
the am3352-rtc and the da830-rtc?

As I understand it, the am3352 functionality is a superset of the da830
functionality. You can use the old driver, and get some functionality,
or use the new driver and get it all.

That means that am3352-rtc is compatible with da830. As long as the
kernel first checks for am3352-rtc, there will be *no* loss of
functionality. All this does is enable older kernels to use the hardware
in some fashion, and given the older kernel didn't have support for the
am3352-rtc features, this is a *gain* in functionality, not a loss.

> 
> For my point of view, compatible mean that the HW will still be fully 
> functional with both versions of the driver, which is not the case here.

What? A driver for any entry in the compatible list should be able to
drive the hardware to *some* level of functionality. We list from
most-specific to most-general to allow a graceful degradation from fully
supported to bare minimum functionality.

> 
> am3352 DTS must use the ti,am3352-rtc to have the expected behavior. 
> Using the ti,da830-rtc version will not make the board working as 
> expected. So we cannot claim the compatibility.

Please can you explain what you mean by "expected behaviour"?

> 
> > This way, we can add new matching compatible as 1st and maintain old
> > compatible string for backwards compatibility.
> >
> > ex:
> > 	compatible = "ti,am3352-rtc", "ti,da830-rtc";
> >
> > Signed-off-by: Hebbar, Gururaja <gururaja.hebbar@ti.com>
> > CC: mark.rutland at arm.com
> > ---
> > Changes in v3:
> > 	- new patch
> >
> > :100644 100644 dc62cc3... 2f0968c... M	drivers/rtc/rtc-omap.c
> >   drivers/rtc/rtc-omap.c |    6 +++---
> >   1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/rtc/rtc-omap.c b/drivers/rtc/rtc-omap.c
> > index dc62cc3..2f0968c 100644
> > --- a/drivers/rtc/rtc-omap.c
> > +++ b/drivers/rtc/rtc-omap.c
> > @@ -330,12 +330,12 @@ static struct platform_device_id omap_rtc_devtype[] = {
> >   MODULE_DEVICE_TABLE(platform, omap_rtc_devtype);
> >
> >   static const struct of_device_id omap_rtc_of_match[] = {
> > -	{	.compatible	= "ti,da830-rtc",
> > -		.data		= &omap_rtc_devtype[OMAP_RTC_DATA_DA830_IDX],
> > -	},
> >   	{	.compatible	= "ti,am3352-rtc",
> >   		.data		= &omap_rtc_devtype[OMAP_RTC_DATA_AM335X_IDX],
> >   	},
> > +	{	.compatible	= "ti,da830-rtc",
> > +		.data		= &omap_rtc_devtype[OMAP_RTC_DATA_DA830_IDX],
> > +	},
> >   	{},
> >   };
> >   MODULE_DEVICE_TABLE(of, omap_rtc_of_match);
> 
> Bottom-line, if you get rid of the old compatible entry, you will not 
> have to play some trick with the entries order.

I don't think it's playing tricks. It's ensuring compatibility, and it's
a simple change in ordering.

Thanks,
Mark.

[PATCH v3 1/2] rtc: omap: update of_device_id to reflect latest ip revisions

[Benoit Cousson]

Hi Mark,

On 16/08/2013 19:20, Mark Rutland wrote:
> Hi Benoit,
>
> On Fri, Aug 16, 2013 at 03:15:57PM +0100, Benoit Cousson wrote:
>> Hi Gururaja,
>>
>> On 16/08/2013 13:36, Hebbar, Gururaja wrote:
>>> The syntax of compatible property in DT is to mention the Most specific
>>> match to most generic match.
>>>
>>> Since AM335x is the platform with latest IP revision, add it 1st in
>>> the device id table.
>>
>> I don't understand why? The order should not matter at all.
>>
>> I've tried to follow the thread you had with Mark on the v2, but AFAIK,
>> you've never answered to his latest question.
>>
>> Moreover, checking the differences between the Davinci and the am3352
>> RTC IP, I would not claim that both are compatible.
>>
>> Sure you can use the am3352 with the Davinci driver, but you will lose
>> the wakeup functionality without even being notify about that.
>
> Could you describe the wakeup functionality, and how it differs between
> the am3352-rtc and the da830-rtc?

AFAIK, da830-rtc does not have that functionality at all. This is 
something that was added to the am3352-rtc.

> As I understand it, the am3352 functionality is a superset of the da830
> functionality. You can use the old driver, and get some functionality,
> or use the new driver and get it all.

Mmm, what your are saying now seems to make sense to me as well. So I'm 
even more confused :-)

> That means that am3352-rtc is compatible with da830. As long as the
> kernel first checks for am3352-rtc, there will be *no* loss of
> functionality. All this does is enable older kernels to use the hardware
> in some fashion, and given the older kernel didn't have support for the
> am3352-rtc features, this is a *gain* in functionality, not a loss.
>
>>
>> For my point of view, compatible mean that the HW will still be fully
>> functional with both versions of the driver, which is not the case here.
>
> What? A driver for any entry in the compatible list should be able to
> drive the hardware to *some* level of functionality. We list from
> most-specific to most-general to allow a graceful degradation from fully
> supported to bare minimum functionality.

OK, but where is it written in the DT spec that this is what the 
compatible is supposed to mean?

I'm quoting it again:
"
The compatible property value consists of one or more strings that 
define the specific programming model for the device. This list of 
strings should be used by a client program for device driver selection. 
The property value consists of a concatenated list of null terminated
strings, from most specific to most general. They allow a device to 
express its compatibility with a family of similar devices, potentially 
allowing a single device driver to match against several devices.
"

The graceful degradation or the loss of functionality is not something 
that I really understand in that text.

Anyway, I'm probably too tired... I'll go back home, and think about 
that after the week-end.

Regards,
Benoit

[PATCH v3 1/2] rtc: omap: update of_device_id to reflect latest ip revisions

[Mark Rutland]

On Fri, Aug 16, 2013 at 07:12:46PM +0100, Benoit Cousson wrote:
> Hi Mark,
> 
> On 16/08/2013 19:20, Mark Rutland wrote:
> > Hi Benoit,
> >
> > On Fri, Aug 16, 2013 at 03:15:57PM +0100, Benoit Cousson wrote:
> >> Hi Gururaja,
> >>
> >> On 16/08/2013 13:36, Hebbar, Gururaja wrote:
> >>> The syntax of compatible property in DT is to mention the Most specific
> >>> match to most generic match.
> >>>
> >>> Since AM335x is the platform with latest IP revision, add it 1st in
> >>> the device id table.
> >>
> >> I don't understand why? The order should not matter at all.
> >>
> >> I've tried to follow the thread you had with Mark on the v2, but AFAIK,
> >> you've never answered to his latest question.
> >>
> >> Moreover, checking the differences between the Davinci and the am3352
> >> RTC IP, I would not claim that both are compatible.
> >>
> >> Sure you can use the am3352 with the Davinci driver, but you will lose
> >> the wakeup functionality without even being notify about that.
> >
> > Could you describe the wakeup functionality, and how it differs between
> > the am3352-rtc and the da830-rtc?
> 
> AFAIK, da830-rtc does not have that functionality at all. This is 
> something that was added to the am3352-rtc.

Ok. So the am3352-rtc can be driven with the full functionality of the
da830-rtc (ie. it's compatible with the da830-rtc programming model), or
it can be driven as an am3352-rtc, for the OS to gain wakeup
functionality in addition to the da830-rtc features. :)

> 
> > As I understand it, the am3352 functionality is a superset of the da830
> > functionality. You can use the old driver, and get some functionality,
> > or use the new driver and get it all.
> 
> Mmm, what your are saying now seems to make sense to me as well. So I'm 
> even more confused :-)

I'll convince you yet :)

> 
> > That means that am3352-rtc is compatible with da830. As long as the
> > kernel first checks for am3352-rtc, there will be *no* loss of
> > functionality. All this does is enable older kernels to use the hardware
> > in some fashion, and given the older kernel didn't have support for the
> > am3352-rtc features, this is a *gain* in functionality, not a loss.
> >
> >>
> >> For my point of view, compatible mean that the HW will still be fully
> >> functional with both versions of the driver, which is not the case here.
> >
> > What? A driver for any entry in the compatible list should be able to
> > drive the hardware to *some* level of functionality. We list from
> > most-specific to most-general to allow a graceful degradation from fully
> > supported to bare minimum functionality.
> 
> OK, but where is it written in the DT spec that this is what the 
> compatible is supposed to mean?
> 
> I'm quoting it again:
> "
> The compatible property value consists of one or more strings that 
> define the specific programming model for the device. This list of 
> strings should be used by a client program for device driver selection. 
> The property value consists of a concatenated list of null terminated
> strings, from most specific to most general. They allow a device to 
> express its compatibility with a family of similar devices, potentially 
> allowing a single device driver to match against several devices.
> "
> 
> The graceful degradation or the loss of functionality is not something 
> that I really understand in that text.

I think it's implicit in the example that follows, where a failure to
match against a specific device results in the OS falling back to a
"more general" device. The "more general" device may not have all the
features of a more specific device (conversely, the more general device
may have more optional features that a more specific device is known not
to implement).

> 
> Anyway, I'm probably too tired... I'll go back home, and think about 
> that after the week-end.

Ok, let me know what you think. :)

Thanks,
Mark.

[PATCH v3 2/2] ARM: dts: AM33XX: update rtc node compatibility

[Hebbar, Gururaja]

From: Hebbar Gururaja <gururaja.hebbar@ti.com>

Since AM33xx RTC IP has RTC_IRQWAKEEN to support Alarm Wake-up, update
the rtc compatible property to enable handling of this feature inside
rtc-omap driver.

Signed-off-by: Hebbar Gururaja <gururaja.hebbar@ti.com>
CC: mark.rutland at arm.com
---
Changes in V3:
	- Instead of replacing the old matching string, add new
	  matching compatible as 1st and maintain old compatible string
	  for backwards compatibility 

Changes in V2:
	- Add new compatible to existing one so that when driver
	  supports enhanced features of hardware, they are available
	  to the user else the basic functionality still works
:100644 100644 0aaf8ce... daeafa0... M	arch/arm/boot/dts/am33xx.dtsi
 arch/arm/boot/dts/am33xx.dtsi |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 0aaf8ce..daeafa0 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -317,7 +317,7 @@
 		};
 
 		rtc at 44e3e000 {
-			compatible = "ti,da830-rtc";
+			compatible = "ti,am3352-rtc", "ti,da830-rtc";
 			reg = <0x44e3e000 0x1000>;
 			interrupts = <75
 				      76>;
-- 
1.7.0.4

[PATCH v3 0/2] rtc: omap: update AM335x rtc ip revision

[Gururaja Hebbar]

Hi All,

Kindly ignore this series. there is mistake/typo because of which it
will not directly apply. I will resend the patch-set.

sorry for the trouble.

regards
Gururaja

On 8/16/2013 5:06 PM, Hebbar, Gururaja wrote:
> The syntax of compatible property in DT is to mention the Most specific
> match to most generic match.
> 
> Since AM335x is the platform with latest IP revision, add it 1st in
> the device id table.
> 
> This way, we can add new matching compatible as 1st and maintain old
> compatible string for backwards compatibility.
> 
> ex:
> 	compatible = "ti,am3352-rtc", "ti,da830-rtc";
> 
> Also, update am335x .dtsi file (am33xx.dtsi) as above.
> 
> Note:
> This is a part of previously submitted patch-set [1]. Out of 4 patches,
> 3 was accepted and 1 got an acceptable NAK. Hence resubmitting only the
> rejected + updated changes.
> 
> Changes in V3:
> 	- As per Mark Rutland's suggestion [2], maintain old compatible
> 	  string and add the new matching string as the 1st compatible.
> 
> Changes in V2:
> 	- Coding style corrections (remove extra space, use lower case
> 	  for hex numbers
> 	- use prefix ARM: for commit subject keeping with arch/arm
> 	  convention)
> 	- use "[AM/am]3352" instead of "[AM/am]335x" to keep the all
> 	  usages in sync.
> 	- Use index defined for struct members so they remain in sync
> 	- Add new compatible to existing one so that when driver
> 	  supports enhanced features of hardware, they are available
> 	  to the user else the basic functionality still works
> 
> [2]
> https://lkml.org/lkml/2013/7/3/74
> [1]
> https://lkml.org/lkml/2013/8/1/442
> 
> Hebbar Gururaja (1):
>   ARM: dts: AM33XX: update rtc node compatibility
> 
> Hebbar, Gururaja (1):
>   rtc: omap: update of_device_id to reflect latest ip revisions
> 
>  arch/arm/boot/dts/am33xx.dtsi |    2 +-
>  drivers/rtc/rtc-omap.c        |    6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
>