| Message ID | 1329923841-32017-8-git-send-email-thierry.reding@avionic-design.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
Thierry Reding wrote at Wednesday, February 22, 2012 8:17 AM: > Add auxdata to instantiate a device tree for the PWFM controller and > include a corresponding node in the device tree. > diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi > + pwm: pwm@7000a000 { > + compatible = "nvidia,tegra20-pwm"; > + reg = <0x7000a000 0x100>; > + #pwm-cells = <2>; > + }; The compatible value probably should list both Tegra30 and Tegra20, so we can know exactly which HW is present, just in case we need to turn on some bug-fix only for one of the variants: compatible = "nvidia,tegra30-pwm", "nvidia,tegra20-pwm"; Could you also write binding documentation, in particular explaining what the two pwm-cells are specifically for Tegra: Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt (although perhaps that'd be part of the previous patch which implements the driver)
* Stephen Warren wrote: > Thierry Reding wrote at Wednesday, February 22, 2012 8:17 AM: > > Add auxdata to instantiate a device tree for the PWFM controller and > > include a corresponding node in the device tree. > > > diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi > > > + pwm: pwm@7000a000 { > > + compatible = "nvidia,tegra20-pwm"; > > + reg = <0x7000a000 0x100>; > > + #pwm-cells = <2>; > > + }; > > The compatible value probably should list both Tegra30 and Tegra20, so > we can know exactly which HW is present, just in case we need to turn > on some bug-fix only for one of the variants: > > compatible = "nvidia,tegra30-pwm", "nvidia,tegra20-pwm"; I'm confused. If I know exactly that the hardware is Tegra30 (which it definitely should be if I include tegra30.dtsi), then why list "tegra20-pwm" as compatible? Or did you mean to list tegra30-pwm as compatible value in the PWM driver? > Could you also write binding documentation, in particular explaining > what the two pwm-cells are specifically for Tegra: > > Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt > > (although perhaps that'd be part of the previous patch which implements > the driver) Actually for Tegra the values would be those documented in the generic binding because Tegra uses of_pwm_simple_xlate(). Does it still make sense to add a Tegra-specific binding? Thierry
On Saturday 03 March 2012, Thierry Reding wrote: > Not enough information to check signature validity. Show Details > * Stephen Warren wrote: > > Thierry Reding wrote at Wednesday, February 22, 2012 8:17 AM: > > > Add auxdata to instantiate a device tree for the PWFM controller and > > > include a corresponding node in the device tree. > > > > > diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi > > > > > + pwm: pwm@7000a000 { > > > + compatible = "nvidia,tegra20-pwm"; > > > + reg = <0x7000a000 0x100>; > > > + #pwm-cells = <2>; > > > + }; > > > > The compatible value probably should list both Tegra30 and Tegra20, so > > we can know exactly which HW is present, just in case we need to turn > > on some bug-fix only for one of the variants: > > > > compatible = "nvidia,tegra30-pwm", "nvidia,tegra20-pwm"; > > I'm confused. If I know exactly that the hardware is Tegra30 (which it > definitely should be if I include tegra30.dtsi), then why list "tegra20-pwm" > as compatible? > > Or did you mean to list tegra30-pwm as compatible value in the PWM driver? We generally list the oldest version of the device that a specific version is compatible to, in order to use the same driver unmodified. E.g. an ns16550a serial port would list both ns16550a and i8250 in its compatible fields so it works with operating systems that only know about i8250. It's not actually all that useful when you add device tree support for two versions of the same device at the same time, but I think that it's good style anyway, in particular when you consider other users of the device tree data besides the Linux kernel. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thierry Reding wrote at Saturday, March 03, 2012 3:54 PM: > * Stephen Warren wrote: > > Thierry Reding wrote at Wednesday, February 22, 2012 8:17 AM: > > > Add auxdata to instantiate a device tree for the PWFM controller and > > > include a corresponding node in the device tree. > > > > > diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi > > > > > + pwm: pwm@7000a000 { > > > + compatible = "nvidia,tegra20-pwm"; > > > + reg = <0x7000a000 0x100>; > > > + #pwm-cells = <2>; > > > + }; > > > > The compatible value probably should list both Tegra30 and Tegra20, so > > we can know exactly which HW is present, just in case we need to turn > > on some bug-fix only for one of the variants: > > > > compatible = "nvidia,tegra30-pwm", "nvidia,tegra20-pwm"; > > I'm confused. If I know exactly that the hardware is Tegra30 (which it > definitely should be if I include tegra30.dtsi), then why list "tegra20-pwm" > as compatible? > > Or did you mean to list tegra30-pwm as compatible value in the PWM driver? Standard practice is to list the exact model of the HW as the first entry in compatible in the .dts file: nvidia,tegra30-pwm This is so that the DT always describes exactly which HW model is actually present, so that if HW-model-specific WARs/... are required in the future, the DT already lists that information up-front. Then additionally list any older HW models that this HW is also compatible with: nvidia,tegra20-pwm This allows the driver to list just nvidia,tegra20-pwm but still bind to DT nodes that are for later HW. So, in other words, you end up with the following in the .dts/.dtsi file: compatible = "nvidia,tegra30-pwm", "nvidia,tegra20-pwm"; > > Could you also write binding documentation, in particular explaining > > what the two pwm-cells are specifically for Tegra: > > > > Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt > > > > (although perhaps that'd be part of the previous patch which implements > > the driver) > > Actually for Tegra the values would be those documented in the generic > binding because Tegra uses of_pwm_simple_xlate(). Does it still make sense to > add a Tegra-specific binding? There should still be a Tegra-specific binding. Without it, there's no definite way to know whether the "standard" properties actually apply, or someone simply forgot to document it.
* Stephen Warren wrote: > Thierry Reding wrote at Saturday, March 03, 2012 3:54 PM: > > I'm confused. If I know exactly that the hardware is Tegra30 (which it > > definitely should be if I include tegra30.dtsi), then why list "tegra20-pwm" > > as compatible? > > > > Or did you mean to list tegra30-pwm as compatible value in the PWM driver? > > Standard practice is to list the exact model of the HW as the first entry > in compatible in the .dts file: > > nvidia,tegra30-pwm > > This is so that the DT always describes exactly which HW model is actually > present, so that if HW-model-specific WARs/... are required in the future, > the DT already lists that information up-front. > > Then additionally list any older HW models that this HW is also compatible > with: > > nvidia,tegra20-pwm > > > This allows the driver to list just nvidia,tegra20-pwm but still bind > to DT nodes that are for later HW. > > So, in other words, you end up with the following in the .dts/.dtsi file: > > compatible = "nvidia,tegra30-pwm", "nvidia,tegra20-pwm"; Okay, that makes sense now. For some reason I thought you were suggesting to put the same into tegra20.dtsi as well. I'll add nvidia,tegra30-pwm to the compatible list in the driver and list both values in the tegra30.dtsi. Thanks for explaining. > > > Could you also write binding documentation, in particular explaining > > > what the two pwm-cells are specifically for Tegra: > > > > > > Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt > > > > > > (although perhaps that'd be part of the previous patch which implements > > > the driver) > > > > Actually for Tegra the values would be those documented in the generic > > binding because Tegra uses of_pwm_simple_xlate(). Does it still make sense to > > add a Tegra-specific binding? > > There should still be a Tegra-specific binding. Without it, there's no > definite way to know whether the "standard" properties actually apply, > or someone simply forgot to document it. Understood. I assume that both the Tegra20 and Tegra30 variants should have explicit bindings then as well, right? Thierry
Thierry Reding wrote at Monday, March 05, 2012 11:16 AM: > * Stephen Warren wrote: > > Thierry Reding wrote at Saturday, March 03, 2012 3:54 PM: ... > > > > Could you also write binding documentation, in particular explaining > > > > what the two pwm-cells are specifically for Tegra: > > > > > > > > Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt > > > > > > > > (although perhaps that'd be part of the previous patch which implements > > > > the driver) > > > > > > Actually for Tegra the values would be those documented in the generic > > > binding because Tegra uses of_pwm_simple_xlate(). Does it still make sense to > > > add a Tegra-specific binding? > > > > There should still be a Tegra-specific binding. Without it, there's no > > definite way to know whether the "standard" properties actually apply, > > or someone simply forgot to document it. > > Understood. I assume that both the Tegra20 and Tegra30 variants should have > explicit bindings then as well, right? To date, the bindings for the two chips have been similar enough we've only written a single bindings document that covers both. For the different compatible values, I've been assuming that's such a base part of standard DT usage that those differences didn't even require documenting in the individual bindings. For example, I've written e.g.: Required properties: - compatible : "nvidia,tegra20-i2s" Or: Required properties: - compatible: Should be "nvidia,<chip>-apbdma" And assumed these would be expanded as appropriate by the DT author. Of course, if there were significant bindings differences (rather than HW differences that were largely transparent to bindings) then yes I'd expected separate nvidia,tegra20-foo.txt and nvidia,tegra30-foo.txt.
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi index ec1f010..c34e6c3 100644 --- a/arch/arm/boot/dts/tegra20.dtsi +++ b/arch/arm/boot/dts/tegra20.dtsi @@ -148,6 +148,12 @@ interrupts = < 0 91 0x04 >; }; + pwm: pwm@7000a000 { + compatible = "nvidia,tegra20-pwm"; + reg = <0x7000a000 0x100>; + #pwm-cells = <2>; + }; + emc@7000f400 { #address-cells = <1>; #size-cells = <0>; diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi index ac4b75c..77bd985 100644 --- a/arch/arm/boot/dts/tegra30.dtsi +++ b/arch/arm/boot/dts/tegra30.dtsi @@ -146,6 +146,12 @@ interrupts = < 0 91 0x04 >; }; + pwm: pwm@7000a000 { + compatible = "nvidia,tegra20-pwm"; + reg = <0x7000a000 0x100>; + #pwm-cells = <2>; + }; + sdhci@78000000 { compatible = "nvidia,tegra30-sdhci", "nvidia,tegra20-sdhci"; reg = <0x78000000 0x200>; diff --git a/arch/arm/mach-tegra/board-dt-tegra20.c b/arch/arm/mach-tegra/board-dt-tegra20.c index 7a95e0b..bad0dd1 100644 --- a/arch/arm/mach-tegra/board-dt-tegra20.c +++ b/arch/arm/mach-tegra/board-dt-tegra20.c @@ -73,6 +73,7 @@ struct of_dev_auxdata tegra20_auxdata_lookup[] __initdata = { &tegra_ehci2_device.dev.platform_data), OF_DEV_AUXDATA("nvidia,tegra20-ehci", TEGRA_USB3_BASE, "tegra-ehci.2", &tegra_ehci3_device.dev.platform_data), + OF_DEV_AUXDATA("nvidia,tegra20-pwm", TEGRA_PWFM_BASE, "tegra-pwm", NULL), {} }; diff --git a/arch/arm/mach-tegra/board-dt-tegra30.c b/arch/arm/mach-tegra/board-dt-tegra30.c index b4124b1..194ae1e 100644 --- a/arch/arm/mach-tegra/board-dt-tegra30.c +++ b/arch/arm/mach-tegra/board-dt-tegra30.c @@ -33,6 +33,8 @@ #include <asm/mach/arch.h> #include <asm/hardware/gic.h> +#include <mach/iomap.h> + #include "board.h" #include "clock.h" @@ -51,6 +53,7 @@ struct of_dev_auxdata tegra30_auxdata_lookup[] __initdata = { OF_DEV_AUXDATA("nvidia,tegra20-i2c", 0x7000C500, "tegra-i2c.2", NULL), OF_DEV_AUXDATA("nvidia,tegra20-i2c", 0x7000C700, "tegra-i2c.3", NULL), OF_DEV_AUXDATA("nvidia,tegra20-i2c", 0x7000D000, "tegra-i2c.4", NULL), + OF_DEV_AUXDATA("nvidia,tegra20-pwm", TEGRA_PWFM_BASE, "tegra-pwm", NULL), {} };
Add auxdata to instantiate a device tree for the PWFM controller and include a corresponding node in the device tree. Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> --- Changes in v3: - add missing include for mach/iomap.h Changes in v2: - none arch/arm/boot/dts/tegra20.dtsi | 6 ++++++ arch/arm/boot/dts/tegra30.dtsi | 6 ++++++ arch/arm/mach-tegra/board-dt-tegra20.c | 1 + arch/arm/mach-tegra/board-dt-tegra30.c | 3 +++ 4 files changed, 16 insertions(+), 0 deletions(-)