Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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@1386 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_1: i2c@1387 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_2: i2c@1388 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_3: i2c@1389 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_4: i2c@138A {
arch/arm/boot/dts/exynos4415.dtsi: i2c_5: i2c@138B {
arch/arm/boot/dts/exynos4415.dtsi: i2c_6: i2c@138C {
arch/arm/boot/dts/exynos4415.dtsi: i2c_7: i2c@138D {
arch/arm/boot/dts/exynos5250.dtsi: i2c_0: i2c@12C6 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_1: i2c@12C7 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_2: i2c@12C8 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_3: i2c@12C9 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_4: i2c@12CA {
arch/arm/boot/dts/exynos5250.dtsi: i2c_5: i2c@12CB {
arch/arm/boot/dts/exynos5250.dtsi: i2c_6: i2c@12CC {
arch/arm/boot/dts/exynos5250.dtsi: i2c_7: i2c@12CD {
arch/arm/boot/dts/exynos5250.dtsi: i2c_8: i2c@12CE {
arch/arm/boot/dts/exynos5250.dtsi: i2c_9: i2c@121D {
arch/arm/boot/dts/exynos5420.dtsi: i2c_0: i2c@12C6 {
arch/arm/boot/dts/exynos5420.dtsi: i2c_1: i2c@12C7 {
arch/arm/boot/dts/exynos5420.dtsi: i2c_2: i2c@12C8 {
arch/arm/boot/dts/exynos5420.dtsi: i2c_3: i2c@12C9 {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_4: i2c@12CA {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_5: i2c@12CB {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_6: i2c@12CC {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_7: i2c@12CD {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_8: i2c@12E0 {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_9: i2c@12E1 {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_10: i2c@12E2 {
arch/arm/boot/dts/exynos5440.dtsi: i2c@F {
arch/arm/boot/dts/exynos5440.dtsi: i2c@1 {
...
$
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
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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,-ahb", "nvidia,tegra30-ahb"' where 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,-ahb", "nvidia,tegra30-ahb"' where 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 linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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,-ahb", "nvidia,tegra30-ahb"' where 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,-ahb", "nvidia,tegra30-ahb"' where 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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,-ahb", "nvidia,tegra30-ahb"' where 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,-ahb", "nvidia,tegra30-ahb"' where 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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,-ahb", "nvidia,tegra30-ahb"' where 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,-ahb", "nvidia,tegra30-ahb"' where 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? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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,-ahb", "nvidia,tegra30-ahb"' where 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,-ahb", "nvidia,tegra30-ahb"' where 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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 linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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@1386 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_1: i2c@1387 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_2: i2c@1388 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_3: i2c@1389 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_4: i2c@138A {
arch/arm/boot/dts/exynos4415.dtsi: i2c_5: i2c@138B {
arch/arm/boot/dts/exynos4415.dtsi: i2c_6: i2c@138C {
arch/arm/boot/dts/exynos4415.dtsi: i2c_7: i2c@138D {
arch/arm/boot/dts/exynos5250.dtsi: i2c_0: i2c@12C6 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_1: i2c@12C7 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_2: i2c@12C8 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_3: i2c@12C9 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_4: i2c@12CA {
arch/arm/boot/dts/exynos5250.dtsi: i2c_5: i2c@12CB {
arch/arm/boot/dts/exynos5250.dtsi: i2c_6: i2c@12CC {
arch/arm/boot/dts/exynos5250.dtsi: i2c_7: i2c@12CD {
arch/arm/boot/dts/exynos5250.dtsi: i2c_8: i2c@12CE {
arch/arm/boot/dts/exynos5250.dtsi: i2c_9: i2c@121D {
arch/arm/boot/dts/exynos5420.dtsi: i2c_0: i2c@12C6 {
arch/arm/boot/dts/exynos5420.dtsi: i2c_1: i2c@12C7 {
arch/arm/boot/dts/exynos5420.dtsi: i2c_2: i2c@12C8 {
arch/arm/boot/dts/exynos5420.dtsi: i2c_3: i2c@12C9 {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_4: i2c@12CA {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_5: i2c@12CB {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_6: i2c@12CC {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_7: i2c@12CD {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_8: i2c@12E0 {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_9: i2c@12E1 {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_10: i2c@12E2 {
arch/arm/boot/dts/exynos5440.dtsi: i2c@F {
arch/arm/boot/dts/exynos5440.dtsi: i2c@1 {
...
$
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
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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,-ahb", "nvidia,tegra30-ahb"' where 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,-ahb", "nvidia,tegra30-ahb"' where 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
