[PATCH 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[PATCH 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Paul Walmsley]

For Tegra132 and later chips, we can now use the correct hardware base
address for the Tegra AHB IP block in the DT data.  Update the DT binding
documentation to reflect this change.

Signed-off-by: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
Cc: Paul Walmsley <pwalmsley-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cc: Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Eduardo Valentin <edubezval-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Hiroshi DOYU <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cc: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>
Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: Paul Walmsley <pwalmsley-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cc: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 .../bindings/arm/tegra/nvidia,tegra20-ahb.txt      |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
index 067c979..7692b4c 100644
--- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
@@ -2,10 +2,15 @@ NVIDIA Tegra AHB
 
 Required properties:
 - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".  For
-  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must contain
-  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
-  tegra132, or tegra210.
-- reg : Should contain 1 register ranges(address and length)
+  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and Tegra124, must
+  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra114
+  or tegra124.  For Tegra132, the compatible string must contain
+  "nvidia,tegra132-ahb".
+
+- reg : Should contain 1 register ranges(address and length).  On Tegra20,
+  Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical base
+  address of the IP block must end in 0x04.  On DT files for later chips, the
+  actual hardware base address of the IP block should be used.
 
 Example:
 	ahb: ahb@6000c004 {

Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Russell King - ARM Linux]

On Tue, Mar 17, 2015 at 01:32:21AM -0700, Paul Walmsley wrote:
>  Required properties:
>  - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".  For
> -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must contain
> -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
> -  tegra132, or tegra210.
> -- reg : Should contain 1 register ranges(address and length)
> +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and Tegra124, must
> +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra114
> +  or tegra124.  For Tegra132, the compatible string must contain
> +  "nvidia,tegra132-ahb".
> +
> +- reg : Should contain 1 register ranges(address and length).  On Tegra20,
> +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical base
> +  address of the IP block must end in 0x04.  On DT files for later chips, the
> +  actual hardware base address of the IP block should be used.

You could check that in the driver.  If you can check it in the driver,
you can also decide to ignore it if it were offset by 0x04 (possibly
printing a warning.)  That opens up the ability to fix the older Tegra
DT files going forward while still remaining compatible with existing
DT files, and avoiding the need for a complex note about this.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Paul Walmsley]

On Tue, 17 Mar 2015, Russell King - ARM Linux wrote:

> On Tue, Mar 17, 2015 at 01:32:21AM -0700, Paul Walmsley wrote:
> >  Required properties:
> >  - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".  For
> > -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must contain
> > -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
> > -  tegra132, or tegra210.
> > -- reg : Should contain 1 register ranges(address and length)
> > +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and Tegra124, must
> > +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra114
> > +  or tegra124.  For Tegra132, the compatible string must contain
> > +  "nvidia,tegra132-ahb".
> > +
> > +- reg : Should contain 1 register ranges(address and length).  On Tegra20,
> > +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical base
> > +  address of the IP block must end in 0x04.  On DT files for later chips, the
> > +  actual hardware base address of the IP block should be used.
> 
> You could check that in the driver.  If you can check it in the driver,
> you can also decide to ignore it if it were offset by 0x04 (possibly
> printing a warning.)  That opens up the ability to fix the older Tegra
> DT files going forward while still remaining compatible with existing
> DT files, and avoiding the need for a complex note about this.

That's fine, I'll do that and drop this patch.

- Paul

Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Stephen Warren]

On 03/19/2015 09:26 AM, Paul Walmsley wrote:
> On Tue, 17 Mar 2015, Russell King - ARM Linux wrote:
>
>> On Tue, Mar 17, 2015 at 01:32:21AM -0700, Paul Walmsley wrote:
>>>   Required properties:
>>>   - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".  For
>>> -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must contain
>>> -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
>>> -  tegra132, or tegra210.
>>> -- reg : Should contain 1 register ranges(address and length)
>>> +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and Tegra124, must
>>> +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra114
>>> +  or tegra124.  For Tegra132, the compatible string must contain
>>> +  "nvidia,tegra132-ahb".
>>> +
>>> +- reg : Should contain 1 register ranges(address and length).  On Tegra20,
>>> +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical base
>>> +  address of the IP block must end in 0x04.  On DT files for later chips, the
>>> +  actual hardware base address of the IP block should be used.
>>
>> You could check that in the driver.  If you can check it in the driver,
>> you can also decide to ignore it if it were offset by 0x04 (possibly
>> printing a warning.)  That opens up the ability to fix the older Tegra
>> DT files going forward while still remaining compatible with existing
>> DT files, and avoiding the need for a complex note about this.
>
> That's fine, I'll do that and drop this patch.

Don't we still want to update the DT binding documentation to state what 
the preferred base address (or at least set of legal base addresses) is/are?

Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Paul Walmsley]

On Thu, 19 Mar 2015, Stephen Warren wrote:

> On 03/19/2015 09:26 AM, Paul Walmsley wrote:
> > On Tue, 17 Mar 2015, Russell King - ARM Linux wrote:
> > 
> > > On Tue, Mar 17, 2015 at 01:32:21AM -0700, Paul Walmsley wrote:
> > > >   Required properties:
> > > >   - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".  For
> > > > -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must contain
> > > > -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
> > > > -  tegra132, or tegra210.
> > > > -- reg : Should contain 1 register ranges(address and length)
> > > > +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and
> > > > Tegra124, must
> > > > +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is
> > > > tegra114
> > > > +  or tegra124.  For Tegra132, the compatible string must contain
> > > > +  "nvidia,tegra132-ahb".
> > > > +
> > > > +- reg : Should contain 1 register ranges(address and length).  On
> > > > Tegra20,
> > > > +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical
> > > > base
> > > > +  address of the IP block must end in 0x04.  On DT files for later
> > > > chips, the
> > > > +  actual hardware base address of the IP block should be used.
> > > 
> > > You could check that in the driver.  If you can check it in the driver,
> > > you can also decide to ignore it if it were offset by 0x04 (possibly
> > > printing a warning.)  That opens up the ability to fix the older Tegra
> > > DT files going forward while still remaining compatible with existing
> > > DT files, and avoiding the need for a complex note about this.
> > 
> > That's fine, I'll do that and drop this patch.
> 
> Don't we still want to update the DT binding documentation to state what the
> preferred base address (or at least set of legal base addresses) is/are?

As far as I know, the DT binding documents are intended to be a 
reference for IP block integration data like base addresses.  At least, 
that is not how they've been used in the past, in the cases that I'm 
familiar with.  

I can see some marginal utility in changing the base address in the 
example.  But since the worst possible outcome of using the old address is 
a warning message at boot, that margin seems quite small indeed.  Anyone 
who would blindly use the base address from the example to create a DT 
file for a new Tegra SoC isn't doing it correctly.


- Paul

Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Paul Walmsley]

On Thu, 19 Mar 2015, Paul Walmsley wrote:

> On Thu, 19 Mar 2015, Stephen Warren wrote:
> 
> > On 03/19/2015 09:26 AM, Paul Walmsley wrote:
> > > On Tue, 17 Mar 2015, Russell King - ARM Linux wrote:
> > > 
> > > > On Tue, Mar 17, 2015 at 01:32:21AM -0700, Paul Walmsley wrote:
> > > > >   Required properties:
> > > > >   - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".  For
> > > > > -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must contain
> > > > > -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
> > > > > -  tegra132, or tegra210.
> > > > > -- reg : Should contain 1 register ranges(address and length)
> > > > > +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and
> > > > > Tegra124, must
> > > > > +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is
> > > > > tegra114
> > > > > +  or tegra124.  For Tegra132, the compatible string must contain
> > > > > +  "nvidia,tegra132-ahb".
> > > > > +
> > > > > +- reg : Should contain 1 register ranges(address and length).  On
> > > > > Tegra20,
> > > > > +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical
> > > > > base
> > > > > +  address of the IP block must end in 0x04.  On DT files for later
> > > > > chips, the
> > > > > +  actual hardware base address of the IP block should be used.
> > > > 
> > > > You could check that in the driver.  If you can check it in the driver,
> > > > you can also decide to ignore it if it were offset by 0x04 (possibly
> > > > printing a warning.)  That opens up the ability to fix the older Tegra
> > > > DT files going forward while still remaining compatible with existing
> > > > DT files, and avoiding the need for a complex note about this.
> > > 
> > > That's fine, I'll do that and drop this patch.
> > 
> > Don't we still want to update the DT binding documentation to state what the
> > preferred base address (or at least set of legal base addresses) is/are?
> 
> As far as I know, the DT binding documents are intended to be a 
> reference for IP block integration data like base addresses. 

The above is missing an important word: it should have read "are _not_ 
intended"


- Paul

Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Stephen Warren]

On 03/19/2015 10:17 AM, Paul Walmsley wrote:
> On Thu, 19 Mar 2015, Stephen Warren wrote:
>
>> On 03/19/2015 09:26 AM, Paul Walmsley wrote:
>>> On Tue, 17 Mar 2015, Russell King - ARM Linux wrote:
>>>
>>>> On Tue, Mar 17, 2015 at 01:32:21AM -0700, Paul Walmsley wrote:
>>>>>    Required properties:
>>>>>    - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".  For
>>>>> -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must contain
>>>>> -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
>>>>> -  tegra132, or tegra210.
>>>>> -- reg : Should contain 1 register ranges(address and length)
>>>>> +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and
>>>>> Tegra124, must
>>>>> +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is
>>>>> tegra114
>>>>> +  or tegra124.  For Tegra132, the compatible string must contain
>>>>> +  "nvidia,tegra132-ahb".
>>>>> +
>>>>> +- reg : Should contain 1 register ranges(address and length).  On
>>>>> Tegra20,
>>>>> +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical
>>>>> base
>>>>> +  address of the IP block must end in 0x04.  On DT files for later
>>>>> chips, the
>>>>> +  actual hardware base address of the IP block should be used.
>>>>
>>>> You could check that in the driver.  If you can check it in the driver,
>>>> you can also decide to ignore it if it were offset by 0x04 (possibly
>>>> printing a warning.)  That opens up the ability to fix the older Tegra
>>>> DT files going forward while still remaining compatible with existing
>>>> DT files, and avoiding the need for a complex note about this.
>>>
>>> That's fine, I'll do that and drop this patch.
>>
>> Don't we still want to update the DT binding documentation to state what the
>> preferred base address (or at least set of legal base addresses) is/are?
>
> As far as I know, the DT binding documents are intended to be a
> reference for IP block integration data like base addresses.  At least,
> that is not how they've been used in the past, in the cases that I'm
> familiar with.
>
> I can see some marginal utility in changing the base address in the
> example.  But since the worst possible outcome of using the old address is
> a warning message at boot, that margin seems quite small indeed.  Anyone
> who would blindly use the base address from the example to create a DT
> file for a new Tegra SoC isn't doing it correctly.

The binding document is supposed to say what value the reg property 
should have. If we require some unusual offset in the reg property (i.e. 
something other than what the HW documentation describes as the module 
base address), that ought to be documented. We do have this situation 
for this module at present, although the documentation unfortunately 
doesn't explicitly call this out even though the example alludes to it.

I do think we should at least fix the example so it isn't confusing and 
inconsistent with expected practice. We could either switch the example 
to Tegra210 so we only provide the best example going forward, or have 
separate examples for Tegra20/210 to highlight the difference.

We should also add documentation that Chips before Tegra210 (or 
Tegra132?) *require* the extra offset. Any code or DT written to the 
existing (admittedly slightly implicit) binding needs to continue to 
work, so we should document this unusual requirement, even if we enhance 
the Linux driver to accept either mode of operation. Other OSs and old 
versions of Linux will still need the exception for older SoCs.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Paul Walmsley]

On Thu, 19 Mar 2015, Stephen Warren wrote:

> The binding document is supposed to say what value the reg property should
> have. 

If you look at other DT binding documentation in the kernel, this is 
generally not the case.  Consider these examples:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/i2c-omap.txt
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt

For example, the bcm2835 I2C binding documentation only mentions one of 
the two I2C controllers apparently available on the system:

$ fgrep -r i2c arch/arm/boot/dts/ | fgrep bcm2835 | fgrep \@
arch/arm/boot/dts/bcm2835.dtsi:		i2c0: i2c@20205000 {
arch/arm/boot/dts/bcm2835.dtsi:		i2c1: i2c@7e804000 {
$

The Exynos documentation contains only one address of many I2C controllers 
on the various SoCs:

$ fgrep -r i2c arch/arm/boot/dts/ | fgrep exynos | fgrep \@
...
arch/arm/boot/dts/exynos4415.dtsi:		i2c_0: i2c@13860000 {
arch/arm/boot/dts/exynos4415.dtsi:		i2c_1: i2c@13870000 {
arch/arm/boot/dts/exynos4415.dtsi:		i2c_2: i2c@13880000 {
arch/arm/boot/dts/exynos4415.dtsi:		i2c_3: i2c@13890000 {
arch/arm/boot/dts/exynos4415.dtsi:		i2c_4: i2c@138A0000 {
arch/arm/boot/dts/exynos4415.dtsi:		i2c_5: i2c@138B0000 {
arch/arm/boot/dts/exynos4415.dtsi:		i2c_6: i2c@138C0000 {
arch/arm/boot/dts/exynos4415.dtsi:		i2c_7: i2c@138D0000 {
arch/arm/boot/dts/exynos5250.dtsi:	i2c_0: i2c@12C60000 {
arch/arm/boot/dts/exynos5250.dtsi:	i2c_1: i2c@12C70000 {
arch/arm/boot/dts/exynos5250.dtsi:	i2c_2: i2c@12C80000 {
arch/arm/boot/dts/exynos5250.dtsi:	i2c_3: i2c@12C90000 {
arch/arm/boot/dts/exynos5250.dtsi:	i2c_4: i2c@12CA0000 {
arch/arm/boot/dts/exynos5250.dtsi:	i2c_5: i2c@12CB0000 {
arch/arm/boot/dts/exynos5250.dtsi:	i2c_6: i2c@12CC0000 {
arch/arm/boot/dts/exynos5250.dtsi:	i2c_7: i2c@12CD0000 {
arch/arm/boot/dts/exynos5250.dtsi:	i2c_8: i2c@12CE0000 {
arch/arm/boot/dts/exynos5250.dtsi:	i2c_9: i2c@121D0000 {
arch/arm/boot/dts/exynos5420.dtsi:	i2c_0: i2c@12C60000 {
arch/arm/boot/dts/exynos5420.dtsi:	i2c_1: i2c@12C70000 {
arch/arm/boot/dts/exynos5420.dtsi:	i2c_2: i2c@12C80000 {
arch/arm/boot/dts/exynos5420.dtsi:	i2c_3: i2c@12C90000 {
arch/arm/boot/dts/exynos5420.dtsi:	hsi2c_4: i2c@12CA0000 {
arch/arm/boot/dts/exynos5420.dtsi:	hsi2c_5: i2c@12CB0000 {
arch/arm/boot/dts/exynos5420.dtsi:	hsi2c_6: i2c@12CC0000 {
arch/arm/boot/dts/exynos5420.dtsi:	hsi2c_7: i2c@12CD0000 {
arch/arm/boot/dts/exynos5420.dtsi:	hsi2c_8: i2c@12E00000 {
arch/arm/boot/dts/exynos5420.dtsi:	hsi2c_9: i2c@12E10000 {
arch/arm/boot/dts/exynos5420.dtsi:	hsi2c_10: i2c@12E20000 {
arch/arm/boot/dts/exynos5440.dtsi:	i2c@F0000 {
arch/arm/boot/dts/exynos5440.dtsi:	i2c@10000 {
...
$

And there are many other integration details that would need to be 
specified in the documentation using the approach that you advocate - for 
example, interrupt and DMA IDs, etc.

> If we require some unusual offset in the reg property (i.e. something
> other than what the HW documentation describes as the module base address),
> that ought to be documented. We do have this situation for this module at
> present, although the documentation unfortunately doesn't explicitly call this
> out even though the example alludes to it.
>
> I do think we should at least fix the example so it isn't confusing and
> inconsistent with expected practice. We could either switch the example to
> Tegra210 so we only provide the best example going forward, or have separate
> examples for Tegra20/210 to highlight the difference.
> 
> We should also add documentation that Chips before Tegra210 (or 
> Tegra132?) *require* the extra offset. Any code or DT written to the 
> existing (admittedly slightly implicit) binding needs to continue to 
> work, so we should document this unusual requirement, even if we enhance 
> the Linux driver to accept either mode of operation.

After the two driver patches (after rmk's requested changes) are applied, 
no unusual offset will be required, but if the legacy offset is specified, 
it will be transparently handled.

As I see it, there are three possible cases:

1. the legacy, incorrect base address is used, in which case everything 
will still work but there will be a warning;

2. the correct base address (from a hardware SoC integration point of 
view) is used, in which case everything will work with no warnings,

3. a novel, completely incorrect base address is used, in which case the 
IP block won't work at all and the driver will fail completely

After the patches, the driver now handles the first two cases.  If you 
would like the DT binding documentation practice changed to attempt to 
address the third case, by requiring DT binding documentation to contain 
lists of the correct IP integration data for every possible chip that 
contains that IP block, as you mention above, such a change would be a 
major delta from existing kernel practice, so would certainly mandate 
submitting a patch for the common DT binding documentation file at

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/submitting-patches.txt

> Other OSs and old versions of Linux will still need the exception for 
> older SoCs.

How about this: I will send a patch for the DT binding documentation to 
note that versions of Linux prior to v4.1 (unless Torvalds runs another 
poll) require the four-byte-offset base address.  Is that sufficient to 
address your concerns with this series?


- Paul

Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Stephen Warren]

On 03/19/2015 11:55 AM, Paul Walmsley wrote:
> On Thu, 19 Mar 2015, Stephen Warren wrote:
>
>> The binding document is supposed to say what value the reg property should
>> have.
>
> If you look at other DT binding documentation in the kernel, this is
> generally not the case.  Consider these examples:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/i2c-omap.txt
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt

That is because there are no special requirements for the reg values 
beyond the HW documentation.

However, if we need the reg value to contain something other than the 
base address that's in the HW documentation, we clearly need to document 
that exception. How else would anyone know about the exception?

The example doesn't count because (a) it's not normative (b) an example 
wouldn't explain why an exception needs to be made or how to calculate 
the exception value for cases other than the specific example given.

...
>> If we require some unusual offset in the reg property (i.e. something
>> other than what the HW documentation describes as the module base address),
>> that ought to be documented. We do have this situation for this module at
>> present, although the documentation unfortunately doesn't explicitly call this
>> out even though the example alludes to it.
>>
>> I do think we should at least fix the example so it isn't confusing and
>> inconsistent with expected practice. We could either switch the example to
>> Tegra210 so we only provide the best example going forward, or have separate
>> examples for Tegra20/210 to highlight the difference.
>>
>> We should also add documentation that Chips before Tegra210 (or
>> Tegra132?) *require* the extra offset. Any code or DT written to the
>> existing (admittedly slightly implicit) binding needs to continue to
>> work, so we should document this unusual requirement, even if we enhance
>> the Linux driver to accept either mode of operation.
>
> After the two driver patches (after rmk's requested changes) are applied,
> no unusual offset will be required, but if the legacy offset is specified,
> it will be transparently handled.
>
> As I see it, there are three possible cases:
>
> 1. the legacy, incorrect base address is used, in which case everything
> will still work but there will be a warning;
>
> 2. the correct base address (from a hardware SoC integration point of
> view) is used, in which case everything will work with no warnings,
>
> 3. a novel, completely incorrect base address is used, in which case the
> IP block won't work at all and the driver will fail completely
>
> After the patches, the driver now handles the first two cases.  If you
> would like the DT binding documentation practice changed to attempt to
> address the third case, by requiring DT binding documentation to contain
> lists of the correct IP integration data for every possible chip that
> contains that IP block, as you mention above, such a change would be a
> major delta from existing kernel practice, so would certainly mandate
> submitting a patch for the common DT binding documentation file at

That's not what I'm asking for. I want exceptions to standard practice 
documented, which is that reg contains whatever the HW documentation 
says it should. There's no need to enumerate all the valid values; the 
HW documentation does that. However, if the DT binding requires 
something other than what the HW documentation says, we must document that.

> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/submitting-patches.txt
>
>> Other OSs and old versions of Linux will still need the exception for
>> older SoCs.
>
> How about this: I will send a patch for the DT binding documentation to
> note that versions of Linux prior to v4.1 (unless Torvalds runs another
> poll) require the four-byte-offset base address.  Is that sufficient to
> address your concerns with this series?

Almost yes.

We should not document Linux 4.1 as the cut-off. DT bindings are 
supposed to be OS agnostic. While it's practically unlikely, it is 
entirely possible for some other OS to have already implemented support 
for this binding, and the current binding is an ABI. We have no control 
over if/when any non-Linux code is updated to add support for a 0-based 
offset for existing SoCs, and certainly no versions of Linux or any 
other OS can be updated retro-actively except perhaps a few linux-stable 
versions. We can however write the binding in such a way as support for 
new SoCs requires the new 0-based address, since there is no binding 
specification for those new chips yet, and the time when you add the new 
binding documentation is the first time any OS could possibly add 
conformant support for it.

In summary, I believe the binding document must state that 
T20/30/114/124 require the offset of 4 in reg value, and newer chips 
require no offset in the reg value. We can still always accept either in 
the Linux kernel going forward based on the principle of being lenient 
re: input data.

Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Paul Walmsley]

On Thu, 19 Mar 2015, Stephen Warren wrote:

> We should not document Linux 4.1 as the cut-off. DT bindings are supposed to
> be OS agnostic. While it's practically unlikely, it is entirely possible for
> some other OS to have already implemented support for this binding, and the
> current binding is an ABI. We have no control over if/when any non-Linux code
> is updated to add support for a 0-based offset for existing SoCs, and
> certainly no versions of Linux or any other OS can be updated retro-actively
> except perhaps a few linux-stable versions. We can however write the binding
> in such a way as support for new SoCs requires the new 0-based address, since
> there is no binding specification for those new chips yet, and the time when
> you add the new binding documentation is the first time any OS could possibly
> add conformant support for it.
> 
> In summary, I believe the binding document must state that T20/30/114/124
> require the offset of 4 in reg value, and newer chips require no offset in the
> reg value. We can still always accept either in the Linux kernel going forward
> based on the principle of being lenient re: input data.

That's fine.  I'll send a patch for that.


- Paul

Re: [PATCH 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Stephen Warren]

On 03/17/2015 02:32 AM, Paul Walmsley wrote:
> For Tegra132 and later chips, we can now use the correct hardware base
> address for the Tegra AHB IP block in the DT data.  Update the DT binding
> documentation to reflect this change.

> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> index 067c979..7692b4c 100644
> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> @@ -2,10 +2,15 @@ NVIDIA Tegra AHB
>
>   Required properties:
>   - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".  For
> -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must contain
> -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
> -  tegra132, or tegra210.
> -- reg : Should contain 1 register ranges(address and length)
> +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and Tegra124, must
> +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra114
> +  or tegra124.  For Tegra132, the compatible string must contain
> +  "nvidia,tegra132-ahb".
> +
> +- reg : Should contain 1 register ranges(address and length).  On Tegra20,
> +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical base
> +  address of the IP block must end in 0x04.  On DT files for later chips, the
> +  actual hardware base address of the IP block should be used.

A table-based approach rather than prose might make this more legible?

- compatible: Must contain the following:
   Tegra20:  "nvidia,tegra20-ahb"
   Tegra30:  "nvidia,tegra30-ahb"
   Tegra114: "nvidia,tegra114-ahb", "nvidia,tegra30-ahb"
   Tegra124: "nvidia,tegra124-ahb", "nvidia,tegra30-ahb"
   Tegra132: "nvidia,tegra132-ahb"
   Tegra210: "nvidia,tegra210-ahb", "nvidia,tegra132-ahb"

With any luck, we can extend that final item for future chips to be:

   Tegra210, TegraNNN:
             "nvidia,tegra<chip>-ahb", "nvidia,tegra132-ahb"

Perhaps we format the 114/124 entry that way too.


As a historical note, I thought this issue might have been caused by the 
HW lumping together a bunch of vaguely related features into the same 
address window, with offset 0 being unrelated to AHB. This is certainly 
the explanation for some other interesting reg value in DT on other HW 
modules in Tegra. However as best I can tell that isn't the case for 
this HW module; we just mapped the start of the reg value to the first 
defined register for some reason rather than aligning it with the entry 
in the address map. So, this series makes sense to me.

The series,
Acked-by: Stephen Warren <swarren@nvidia.com>

(it'd be nice if the DT doc was modified as I described above, but not 
strictly necessary. I like Russell's ideas how to make this more 
transparent/automatic so old DTs can be converted too).

Re: [PATCH 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Paul Walmsley]

On Tue, 17 Mar 2015, Stephen Warren wrote:

> On 03/17/2015 02:32 AM, Paul Walmsley wrote:
> > For Tegra132 and later chips, we can now use the correct hardware base
> > address for the Tegra AHB IP block in the DT data.  Update the DT binding
> > documentation to reflect this change.
> 
> > diff --git
> > a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > index 067c979..7692b4c 100644
> > --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > @@ -2,10 +2,15 @@ NVIDIA Tegra AHB
> > 
> >   Required properties:
> >   - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".  For
> > -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must contain
> > -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
> > -  tegra132, or tegra210.
> > -- reg : Should contain 1 register ranges(address and length)
> > +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and Tegra124,
> > must
> > +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is
> > tegra114
> > +  or tegra124.  For Tegra132, the compatible string must contain
> > +  "nvidia,tegra132-ahb".
> > +
> > +- reg : Should contain 1 register ranges(address and length).  On Tegra20,
> > +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical base
> > +  address of the IP block must end in 0x04.  On DT files for later chips,
> > the
> > +  actual hardware base address of the IP block should be used.
> 
> A table-based approach rather than prose might make this more legible?
> 
> - compatible: Must contain the following:
>   Tegra20:  "nvidia,tegra20-ahb"
>   Tegra30:  "nvidia,tegra30-ahb"
>   Tegra114: "nvidia,tegra114-ahb", "nvidia,tegra30-ahb"
>   Tegra124: "nvidia,tegra124-ahb", "nvidia,tegra30-ahb"
>   Tegra132: "nvidia,tegra132-ahb"
>   Tegra210: "nvidia,tegra210-ahb", "nvidia,tegra132-ahb"
> 
> With any luck, we can extend that final item for future chips to be:
> 
>   Tegra210, TegraNNN:
>             "nvidia,tegra<chip>-ahb", "nvidia,tegra132-ahb"
> 
> Perhaps we format the 114/124 entry that way too.

I think I'm just going to drop this patch, since Russell prefers that the 
workaround is applied in the driver.

With regards to using tables rather than narrative descriptions: perhaps 
consider a patch to 
Documentation/devicetree/bindings/submitting-patches.txt ?  I don't know 
what the DT binding documentation maintainers' future plans are with 
regards to automated documentation reflow, etc., but submitting a patch 
there would stimulate at least some coordination on the issue.

> As a historical note, I thought this issue might have been caused by the HW
> lumping together a bunch of vaguely related features into the same address
> window, with offset 0 being unrelated to AHB. This is certainly the
> explanation for some other interesting reg value in DT on other HW modules in
> Tegra. However as best I can tell that isn't the case for this HW module; we
> just mapped the start of the reg value to the first defined register for some
> reason rather than aligning it with the entry in the address map. So, this
> series makes sense to me.
> 
> The series,
> Acked-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> (it'd be nice if the DT doc was modified as I described above, but not
> strictly necessary. I like Russell's ideas how to make this more
> transparent/automatic so old DTs can be converted too).

I'll add the ack on the first patch, but since the second one has been 
changed, I won't add one there yet.


- Paul
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Stephen Warren]

On 03/19/2015 09:33 AM, Paul Walmsley wrote:
> On Tue, 17 Mar 2015, Stephen Warren wrote:
>
>> On 03/17/2015 02:32 AM, Paul Walmsley wrote:
>>> For Tegra132 and later chips, we can now use the correct hardware base
>>> address for the Tegra AHB IP block in the DT data.  Update the DT binding
>>> documentation to reflect this change.
>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
>>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
>>> index 067c979..7692b4c 100644
>>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
>>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
>>> @@ -2,10 +2,15 @@ NVIDIA Tegra AHB
>>>
>>>    Required properties:
>>>    - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".  For
>>> -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must contain
>>> -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
>>> -  tegra132, or tegra210.
>>> -- reg : Should contain 1 register ranges(address and length)
>>> +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and Tegra124,
>>> must
>>> +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is
>>> tegra114
>>> +  or tegra124.  For Tegra132, the compatible string must contain
>>> +  "nvidia,tegra132-ahb".
>>> +
>>> +- reg : Should contain 1 register ranges(address and length).  On Tegra20,
>>> +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical base
>>> +  address of the IP block must end in 0x04.  On DT files for later chips,
>>> the
>>> +  actual hardware base address of the IP block should be used.
>>
>> A table-based approach rather than prose might make this more legible?
>>
>> - compatible: Must contain the following:
>>    Tegra20:  "nvidia,tegra20-ahb"
>>    Tegra30:  "nvidia,tegra30-ahb"
>>    Tegra114: "nvidia,tegra114-ahb", "nvidia,tegra30-ahb"
>>    Tegra124: "nvidia,tegra124-ahb", "nvidia,tegra30-ahb"
>>    Tegra132: "nvidia,tegra132-ahb"
>>    Tegra210: "nvidia,tegra210-ahb", "nvidia,tegra132-ahb"
>>
>> With any luck, we can extend that final item for future chips to be:
>>
>>    Tegra210, TegraNNN:
>>              "nvidia,tegra<chip>-ahb", "nvidia,tegra132-ahb"
>>
>> Perhaps we format the 114/124 entry that way too.
>
> I think I'm just going to drop this patch, since Russell prefers that the
> workaround is applied in the driver.
>
> With regards to using tables rather than narrative descriptions: perhaps
> consider a patch to
> Documentation/devicetree/bindings/submitting-patches.txt ?  I don't know
> what the DT binding documentation maintainers' future plans are with
> regards to automated documentation reflow, etc., but submitting a patch
> there would stimulate at least some coordination on the issue.

I don't think it's appropriate for that file to dictate that, in the 
same way that coding style documentation generally doesn't address that 
kind of detail regarding code structure. Rather, the code review process 
hopefully homes in on the best representation case-by-case. A table 
seems more succinct and unambiguous in this case. Most DT bindings don't 
need to specify this level of detail since there aren't so many 
inconsistent options; the generic rules apply.

Re: [PATCH 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Paul Walmsley]

On Thu, 19 Mar 2015, Stephen Warren wrote:

> On 03/19/2015 09:33 AM, Paul Walmsley wrote:
> > On Tue, 17 Mar 2015, Stephen Warren wrote:
> > 
> > > On 03/17/2015 02:32 AM, Paul Walmsley wrote:
> > > > For Tegra132 and later chips, we can now use the correct hardware base
> > > > address for the Tegra AHB IP block in the DT data.  Update the DT
> > > > binding
> > > > documentation to reflect this change.
> > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > > > b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > > > index 067c979..7692b4c 100644
> > > > --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > > > @@ -2,10 +2,15 @@ NVIDIA Tegra AHB
> > > > 
> > > >    Required properties:
> > > >    - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".  For
> > > > -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must contain
> > > > -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
> > > > -  tegra132, or tegra210.
> > > > -- reg : Should contain 1 register ranges(address and length)
> > > > +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and
> > > > Tegra124,
> > > > must
> > > > +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is
> > > > tegra114
> > > > +  or tegra124.  For Tegra132, the compatible string must contain
> > > > +  "nvidia,tegra132-ahb".
> > > > +
> > > > +- reg : Should contain 1 register ranges(address and length).  On
> > > > Tegra20,
> > > > +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical
> > > > base
> > > > +  address of the IP block must end in 0x04.  On DT files for later
> > > > chips,
> > > > the
> > > > +  actual hardware base address of the IP block should be used.
> > > 
> > > A table-based approach rather than prose might make this more legible?
> > > 
> > > - compatible: Must contain the following:
> > >    Tegra20:  "nvidia,tegra20-ahb"
> > >    Tegra30:  "nvidia,tegra30-ahb"
> > >    Tegra114: "nvidia,tegra114-ahb", "nvidia,tegra30-ahb"
> > >    Tegra124: "nvidia,tegra124-ahb", "nvidia,tegra30-ahb"
> > >    Tegra132: "nvidia,tegra132-ahb"
> > >    Tegra210: "nvidia,tegra210-ahb", "nvidia,tegra132-ahb"
> > > 
> > > With any luck, we can extend that final item for future chips to be:
> > > 
> > >    Tegra210, TegraNNN:
> > >              "nvidia,tegra<chip>-ahb", "nvidia,tegra132-ahb"
> > > 
> > > Perhaps we format the 114/124 entry that way too.
> > 
> > I think I'm just going to drop this patch, since Russell prefers that the
> > workaround is applied in the driver.
> > 
> > With regards to using tables rather than narrative descriptions: perhaps
> > consider a patch to
> > Documentation/devicetree/bindings/submitting-patches.txt ?  I don't know
> > what the DT binding documentation maintainers' future plans are with
> > regards to automated documentation reflow, etc., but submitting a patch
> > there would stimulate at least some coordination on the issue.
> 
> I don't think it's appropriate for that file to dictate that, in the same way
> that coding style documentation generally doesn't address that kind of detail
> regarding code structure.

We do indeed specify details like this in our documentation guidelines.  
Here are two examples:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kernel-doc-nano-HOWTO.txt#n103

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle#n464

> Rather, the code review process hopefully homes in on the best 
> representation case-by-case. A table seems more succinct and unambiguous 
> in this case. 

I understand that your point of view in this case is that the table format 
is superior.  My point of view on this issue, which is that any potential 
improvement in using a table format is outweighed by the time involved in 
doing it.  Additionally I believe that the presence of the information is 
the most important criterion.  

My point of view is also that formatting guidelines should be coordinated 
at a kernel-wide level with the DT maintainers and placed into common 
policy documentation.  That way everyone's expectations are aligned.  
Reviews can be based on shared rules, rather than individual whim.  Such 
an approach also codifies expectations for programs that operate on 
documentation data, like the scripts/kernel-doc code.


- Paul

Re: [PATCH 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Stephen Warren]

On 03/19/2015 10:34 AM, Paul Walmsley wrote:
> On Thu, 19 Mar 2015, Stephen Warren wrote:
>
>> On 03/19/2015 09:33 AM, Paul Walmsley wrote:
>>> On Tue, 17 Mar 2015, Stephen Warren wrote:
>>>
>>>> On 03/17/2015 02:32 AM, Paul Walmsley wrote:
>>>>> For Tegra132 and later chips, we can now use the correct hardware base
>>>>> address for the Tegra AHB IP block in the DT data.  Update the DT
>>>>> binding
>>>>> documentation to reflect this change.
>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
>>>>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
>>>>> index 067c979..7692b4c 100644
>>>>> --- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
>>>>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
>>>>> @@ -2,10 +2,15 @@ NVIDIA Tegra AHB
>>>>>
>>>>>     Required properties:
>>>>>     - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".  For
>>>>> -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must contain
>>>>> -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
>>>>> -  tegra132, or tegra210.
>>>>> -- reg : Should contain 1 register ranges(address and length)
>>>>> +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and
>>>>> Tegra124,
>>>>> must
>>>>> +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is
>>>>> tegra114
>>>>> +  or tegra124.  For Tegra132, the compatible string must contain
>>>>> +  "nvidia,tegra132-ahb".
>>>>> +
>>>>> +- reg : Should contain 1 register ranges(address and length).  On
>>>>> Tegra20,
>>>>> +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical
>>>>> base
>>>>> +  address of the IP block must end in 0x04.  On DT files for later
>>>>> chips,
>>>>> the
>>>>> +  actual hardware base address of the IP block should be used.
>>>>
>>>> A table-based approach rather than prose might make this more legible?
>>>>
>>>> - compatible: Must contain the following:
>>>>     Tegra20:  "nvidia,tegra20-ahb"
>>>>     Tegra30:  "nvidia,tegra30-ahb"
>>>>     Tegra114: "nvidia,tegra114-ahb", "nvidia,tegra30-ahb"
>>>>     Tegra124: "nvidia,tegra124-ahb", "nvidia,tegra30-ahb"
>>>>     Tegra132: "nvidia,tegra132-ahb"
>>>>     Tegra210: "nvidia,tegra210-ahb", "nvidia,tegra132-ahb"
>>>>
>>>> With any luck, we can extend that final item for future chips to be:
>>>>
>>>>     Tegra210, TegraNNN:
>>>>               "nvidia,tegra<chip>-ahb", "nvidia,tegra132-ahb"
>>>>
>>>> Perhaps we format the 114/124 entry that way too.
>>>
>>> I think I'm just going to drop this patch, since Russell prefers that the
>>> workaround is applied in the driver.
>>>
>>> With regards to using tables rather than narrative descriptions: perhaps
>>> consider a patch to
>>> Documentation/devicetree/bindings/submitting-patches.txt ?  I don't know
>>> what the DT binding documentation maintainers' future plans are with
>>> regards to automated documentation reflow, etc., but submitting a patch
>>> there would stimulate at least some coordination on the issue.
>>
>> I don't think it's appropriate for that file to dictate that, in the same way
>> that coding style documentation generally doesn't address that kind of detail
>> regarding code structure.
>
> We do indeed specify details like this in our documentation guidelines.
> Here are two examples:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kernel-doc-nano-HOWTO.txt#n103
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle#n464

Perhaps I phrased my point slightly differently form the core of what I 
meant.

I'm not aware that review feedback can't address topics that are not 
already addressed by the documentation. Is there such a rule?

Equally, I think if you want the documentation to address a particular 
point, it's appropriate for you to submit a patch to the documentation 
to update it, rather than ask the reviewer to do so before accepting the 
review feedback.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Paul Walmsley]

On Thu, 19 Mar 2015, Stephen Warren wrote:

> On 03/19/2015 10:34 AM, Paul Walmsley wrote:
> > On Thu, 19 Mar 2015, Stephen Warren wrote:
> > 
> > > On 03/19/2015 09:33 AM, Paul Walmsley wrote:
> > > > On Tue, 17 Mar 2015, Stephen Warren wrote:
> > > > 
> > > > > On 03/17/2015 02:32 AM, Paul Walmsley wrote:
> > > > > > For Tegra132 and later chips, we can now use the correct hardware
> > > > > > base
> > > > > > address for the Tegra AHB IP block in the DT data.  Update the DT
> > > > > > binding
> > > > > > documentation to reflect this change.
> > > > > 
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > > > > > b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > > > > > index 067c979..7692b4c 100644
> > > > > > ---
> > > > > > a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > > > > > +++
> > > > > > b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > > > > > @@ -2,10 +2,15 @@ NVIDIA Tegra AHB
> > > > > > 
> > > > > >     Required properties:
> > > > > >     - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".
> > > > > > For
> > > > > > -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must
> > > > > > contain
> > > > > > -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is
> > > > > > tegra124,
> > > > > > -  tegra132, or tegra210.
> > > > > > -- reg : Should contain 1 register ranges(address and length)
> > > > > > +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and
> > > > > > Tegra124,
> > > > > > must
> > > > > > +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip>
> > > > > > is
> > > > > > tegra114
> > > > > > +  or tegra124.  For Tegra132, the compatible string must contain
> > > > > > +  "nvidia,tegra132-ahb".
> > > > > > +
> > > > > > +- reg : Should contain 1 register ranges(address and length).  On
> > > > > > Tegra20,
> > > > > > +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the
> > > > > > physical
> > > > > > base
> > > > > > +  address of the IP block must end in 0x04.  On DT files for later
> > > > > > chips,
> > > > > > the
> > > > > > +  actual hardware base address of the IP block should be used.
> > > > > 
> > > > > A table-based approach rather than prose might make this more legible?
> > > > > 
> > > > > - compatible: Must contain the following:
> > > > >     Tegra20:  "nvidia,tegra20-ahb"
> > > > >     Tegra30:  "nvidia,tegra30-ahb"
> > > > >     Tegra114: "nvidia,tegra114-ahb", "nvidia,tegra30-ahb"
> > > > >     Tegra124: "nvidia,tegra124-ahb", "nvidia,tegra30-ahb"
> > > > >     Tegra132: "nvidia,tegra132-ahb"
> > > > >     Tegra210: "nvidia,tegra210-ahb", "nvidia,tegra132-ahb"
> > > > > 
> > > > > With any luck, we can extend that final item for future chips to be:
> > > > > 
> > > > >     Tegra210, TegraNNN:
> > > > >               "nvidia,tegra<chip>-ahb", "nvidia,tegra132-ahb"
> > > > > 
> > > > > Perhaps we format the 114/124 entry that way too.
> > > > 
> > > > I think I'm just going to drop this patch, since Russell prefers that
> > > > the
> > > > workaround is applied in the driver.
> > > > 
> > > > With regards to using tables rather than narrative descriptions: perhaps
> > > > consider a patch to
> > > > Documentation/devicetree/bindings/submitting-patches.txt ?  I don't know
> > > > what the DT binding documentation maintainers' future plans are with
> > > > regards to automated documentation reflow, etc., but submitting a patch
> > > > there would stimulate at least some coordination on the issue.
> > > 
> > > I don't think it's appropriate for that file to dictate that, in the same
> > > way
> > > that coding style documentation generally doesn't address that kind of
> > > detail
> > > regarding code structure.
> > 
> > We do indeed specify details like this in our documentation guidelines.
> > Here are two examples:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kernel-doc-nano-HOWTO.txt#n103
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle#n464
> 
> Perhaps I phrased my point slightly differently form the core of what I meant.
> 
> I'm not aware that review feedback can't address topics that are not already
> addressed by the documentation. Is there such a rule?

Not that I'm aware of, but I'm not sure that I understand the point you're 
making.  Care to rephrase to make it more explicit?

> Equally, I think if you want the documentation to address a particular point,
> it's appropriate for you to submit a patch to the documentation to update it,
> rather than ask the reviewer to do so before accepting the review feedback.

I guess my question is this: do you intend that the table-based 
documentation approach you describe should apply generally to other DT 
binding documents with similar per-chip support lists?  Or is there 
something about the Tegra AHB specifically that merits this format?

If the former was intended -- in other words, you are proposing a policy 
that should be followed in the general case -- then I would suggest that 
the documentation policy should be described in a shared DT binding 
CodingStyle or submitting-patches document, as we do elsewhere in the 
kernel.

For example, the guidance could read[*], using your earlier example:

--- 
If different values of a DT property are required for different chips 
or different situations, these should be listed in the binding 
documentation in the following format:

- compatible: Must contain the following:
    Tegra20:  "nvidia,tegra20-ahb"
    Tegra30:  "nvidia,tegra30-ahb"
    Tegra114: "nvidia,tegra114-ahb", "nvidia,tegra30-ahb"
(etc.)

Each line in the list should be indented from the start of the section 
describing the DT property by four spaces.  There should be no blank
lines between each list row.
---

That way, the community can align on a common format for this table-based 
format.  Any automated parsing tools that read the DT documentation can 
know what to expect; anyone who disagrees can speak up as the patch is 
being considered; and the issue no longer needs to be a matter of taste: 
it can be transformed into a matter of fact.

Once the documentation format becomes a matter of fact, then patch 
submitters have clear guidance to follow.  Submitters can get the patches 
right the first time and avoid wasting their time and reviewers' time.  
Otherwise, there is the (quite present) risk that 'n' different reviewers 
of the DT binding documentation could have 'n' different opinions about 
how the data should be formatted, with each opinion conveying 
minimal-to-no technical advantage over another.  This just results in a 
waste of time for everyone, time that is better spent on code.  In my 
view, every moment I spend reformatting documentation to standards that 
aren't shared is not only wasted, it's time that's subtracted from my 
ability to improve our actual upstream code and work on something that's 
actually useful.


- Paul

[*] I am neutral about the format or whether a narrative vs. a table 
approach is best.  Whatever it should be, it should just be common 
guidance.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Stephen Warren]

I guess pretend like I never made the suggestion.

On 03/19/2015 12:42 PM, Paul Walmsley wrote:
> On Thu, 19 Mar 2015, Stephen Warren wrote:
>
>> On 03/19/2015 10:34 AM, Paul Walmsley wrote:
>>> On Thu, 19 Mar 2015, Stephen Warren wrote:
>>>
>>>> On 03/19/2015 09:33 AM, Paul Walmsley wrote:
>>>>> On Tue, 17 Mar 2015, Stephen Warren wrote:
>>>>>
>>>>>> On 03/17/2015 02:32 AM, Paul Walmsley wrote:
>>>>>>> For Tegra132 and later chips, we can now use the correct hardware
>>>>>>> base
>>>>>>> address for the Tegra AHB IP block in the DT data.  Update the DT
>>>>>>> binding
>>>>>>> documentation to reflect this change.
>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
>>>>>>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
>>>>>>> index 067c979..7692b4c 100644
>>>>>>> ---
>>>>>>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
>>>>>>> +++
>>>>>>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
>>>>>>> @@ -2,10 +2,15 @@ NVIDIA Tegra AHB
>>>>>>>
>>>>>>>      Required properties:
>>>>>>>      - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".
>>>>>>> For
>>>>>>> -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must
>>>>>>> contain
>>>>>>> -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is
>>>>>>> tegra124,
>>>>>>> -  tegra132, or tegra210.
>>>>>>> -- reg : Should contain 1 register ranges(address and length)
>>>>>>> +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and
>>>>>>> Tegra124,
>>>>>>> must
>>>>>>> +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip>
>>>>>>> is
>>>>>>> tegra114
>>>>>>> +  or tegra124.  For Tegra132, the compatible string must contain
>>>>>>> +  "nvidia,tegra132-ahb".
>>>>>>> +
>>>>>>> +- reg : Should contain 1 register ranges(address and length).  On
>>>>>>> Tegra20,
>>>>>>> +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the
>>>>>>> physical
>>>>>>> base
>>>>>>> +  address of the IP block must end in 0x04.  On DT files for later
>>>>>>> chips,
>>>>>>> the
>>>>>>> +  actual hardware base address of the IP block should be used.
>>>>>>
>>>>>> A table-based approach rather than prose might make this more legible?
>>>>>>
>>>>>> - compatible: Must contain the following:
>>>>>>      Tegra20:  "nvidia,tegra20-ahb"
>>>>>>      Tegra30:  "nvidia,tegra30-ahb"
>>>>>>      Tegra114: "nvidia,tegra114-ahb", "nvidia,tegra30-ahb"
>>>>>>      Tegra124: "nvidia,tegra124-ahb", "nvidia,tegra30-ahb"
>>>>>>      Tegra132: "nvidia,tegra132-ahb"
>>>>>>      Tegra210: "nvidia,tegra210-ahb", "nvidia,tegra132-ahb"
>>>>>>
>>>>>> With any luck, we can extend that final item for future chips to be:
>>>>>>
>>>>>>      Tegra210, TegraNNN:
>>>>>>                "nvidia,tegra<chip>-ahb", "nvidia,tegra132-ahb"
>>>>>>
>>>>>> Perhaps we format the 114/124 entry that way too.
>>>>>
>>>>> I think I'm just going to drop this patch, since Russell prefers that
>>>>> the
>>>>> workaround is applied in the driver.
>>>>>
>>>>> With regards to using tables rather than narrative descriptions: perhaps
>>>>> consider a patch to
>>>>> Documentation/devicetree/bindings/submitting-patches.txt ?  I don't know
>>>>> what the DT binding documentation maintainers' future plans are with
>>>>> regards to automated documentation reflow, etc., but submitting a patch
>>>>> there would stimulate at least some coordination on the issue.
>>>>
>>>> I don't think it's appropriate for that file to dictate that, in the same
>>>> way
>>>> that coding style documentation generally doesn't address that kind of
>>>> detail
>>>> regarding code structure.
>>>
>>> We do indeed specify details like this in our documentation guidelines.
>>> Here are two examples:
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kernel-doc-nano-HOWTO.txt#n103
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle#n464
>>
>> Perhaps I phrased my point slightly differently form the core of what I meant.
>>
>> I'm not aware that review feedback can't address topics that are not already
>> addressed by the documentation. Is there such a rule?
>
> Not that I'm aware of, but I'm not sure that I understand the point you're
> making.  Care to rephrase to make it more explicit?
>
>> Equally, I think if you want the documentation to address a particular point,
>> it's appropriate for you to submit a patch to the documentation to update it,
>> rather than ask the reviewer to do so before accepting the review feedback.
>
> I guess my question is this: do you intend that the table-based
> documentation approach you describe should apply generally to other DT
> binding documents with similar per-chip support lists?  Or is there
> something about the Tegra AHB specifically that merits this format?
>
> If the former was intended -- in other words, you are proposing a policy
> that should be followed in the general case -- then I would suggest that
> the documentation policy should be described in a shared DT binding
> CodingStyle or submitting-patches document, as we do elsewhere in the
> kernel.
>
> For example, the guidance could read[*], using your earlier example:
>
> ---
> If different values of a DT property are required for different chips
> or different situations, these should be listed in the binding
> documentation in the following format:
>
> - compatible: Must contain the following:
>      Tegra20:  "nvidia,tegra20-ahb"
>      Tegra30:  "nvidia,tegra30-ahb"
>      Tegra114: "nvidia,tegra114-ahb", "nvidia,tegra30-ahb"
> (etc.)
>
> Each line in the list should be indented from the start of the section
> describing the DT property by four spaces.  There should be no blank
> lines between each list row.
> ---
>
> That way, the community can align on a common format for this table-based
> format.  Any automated parsing tools that read the DT documentation can
> know what to expect; anyone who disagrees can speak up as the patch is
> being considered; and the issue no longer needs to be a matter of taste:
> it can be transformed into a matter of fact.
>
> Once the documentation format becomes a matter of fact, then patch
> submitters have clear guidance to follow.  Submitters can get the patches
> right the first time and avoid wasting their time and reviewers' time.
> Otherwise, there is the (quite present) risk that 'n' different reviewers
> of the DT binding documentation could have 'n' different opinions about
> how the data should be formatted, with each opinion conveying
> minimal-to-no technical advantage over another.  This just results in a
> waste of time for everyone, time that is better spent on code.  In my
> view, every moment I spend reformatting documentation to standards that
> aren't shared is not only wasted, it's time that's subtracted from my
> ability to improve our actual upstream code and work on something that's
> actually useful.
>
>
> - Paul
>
> [*] I am neutral about the format or whether a narrative vs. a table
> approach is best.  Whatever it should be, it should just be common
> guidance.
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html