| Message ID | 1473407397-29395-4-git-send-email-kernel@martin.sperl.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 09.09.2016 um 09:49 schrieb kernel@martin.sperl.org: > From: Martin Sperl <kernel@martin.sperl.org> > > Add the node for the thermal sensor of the bcm2835-soc > to the device tree. > > Signed-off-by: Martin Sperl <kernel@martin.sperl.org> > Reviewed-by: Eric Anholt <eric@anholt.net> > --- > arch/arm/boot/dts/bcm283x.dtsi | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi > index b982522..e2e3a46 100644 > --- a/arch/arm/boot/dts/bcm283x.dtsi > +++ b/arch/arm/boot/dts/bcm283x.dtsi > @@ -186,6 +186,12 @@ > interrupts = <2 14>; /* pwa1 */ > }; > > + thermal: thermal@0x7e212000 { > + compatible = "brcm,bcm2835-thermal"; > + reg = <0x7e212000 0x8>; > + clocks = <&clocks BCM2835_CLOCK_TSENS>; > + }; > + Since the driver handles 3 different SoC (2835, 2836, 2837). This node should be defined in the SoC specific dtsi files, because the BCM2836 includes bcm283x.dtsi too. Be aware the patch for bcm2837 must go to ARM64. > aux: aux@0x7e215000 { > compatible = "brcm,bcm2835-aux"; > #clock-cells = <1>;
> On 09.09.2016, at 16:25, Stefan Wahren <stefan.wahren@i2se.com> wrote: > >> Am 09.09.2016 um 09:49 schrieb kernel@martin.sperl.org: >> From: Martin Sperl <kernel@martin.sperl.org> >> >> Add the node for the thermal sensor of the bcm2835-soc >> to the device tree. >> >> Signed-off-by: Martin Sperl <kernel@martin.sperl.org> >> Reviewed-by: Eric Anholt <eric@anholt.net> >> --- >> arch/arm/boot/dts/bcm283x.dtsi | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi >> index b982522..e2e3a46 100644 >> --- a/arch/arm/boot/dts/bcm283x.dtsi >> +++ b/arch/arm/boot/dts/bcm283x.dtsi >> @@ -186,6 +186,12 @@ >> interrupts = <2 14>; /* pwa1 */ >> }; >> >> + thermal: thermal@0x7e212000 { >> + compatible = "brcm,bcm2835-thermal"; >> + reg = <0x7e212000 0x8>; >> + clocks = <&clocks BCM2835_CLOCK_TSENS>; >> + }; >> + > > Since the driver handles 3 different SoC (2835, 2836, 2837). This node > should be defined in the SoC specific dtsi files, because the BCM2836 > includes bcm283x.dtsi too. > > Be aware the patch for bcm2837 must go to ARM64. I can not really follow: * the node is defined in the dtsi included by all 3 soc, and it is available on all so it sits where for example spi0 or uart0 is located * as for arm64: this describes the registers that are identical for arm and arm64 and the bcm2837.dtsi is also including ../../../../arm/boot/dts/bcm283x.dtsi So what is the problem? Martin P.s the patches apply cleanly against master.
> Martin Sperl <kernel@martin.sperl.org> hat am 9. September 2016 um 16:58 > geschrieben: > > > > > On 09.09.2016, at 16:25, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > > >> Am 09.09.2016 um 09:49 schrieb kernel@martin.sperl.org: > >> From: Martin Sperl <kernel@martin.sperl.org> > >> > >> Add the node for the thermal sensor of the bcm2835-soc > >> to the device tree. > >> > >> Signed-off-by: Martin Sperl <kernel@martin.sperl.org> > >> Reviewed-by: Eric Anholt <eric@anholt.net> > >> --- > >> arch/arm/boot/dts/bcm283x.dtsi | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/arch/arm/boot/dts/bcm283x.dtsi > >> b/arch/arm/boot/dts/bcm283x.dtsi > >> index b982522..e2e3a46 100644 > >> --- a/arch/arm/boot/dts/bcm283x.dtsi > >> +++ b/arch/arm/boot/dts/bcm283x.dtsi > >> @@ -186,6 +186,12 @@ > >> interrupts = <2 14>; /* pwa1 */ > >> }; > >> > >> + thermal: thermal@0x7e212000 { > >> + compatible = "brcm,bcm2835-thermal"; > >> + reg = <0x7e212000 0x8>; > >> + clocks = <&clocks BCM2835_CLOCK_TSENS>; > >> + }; > >> + > > > > Since the driver handles 3 different SoC (2835, 2836, 2837). This node > > should be defined in the SoC specific dtsi files, because the BCM2836 > > includes bcm283x.dtsi too. > > > > Be aware the patch for bcm2837 must go to ARM64. > > I can not really follow: > * the node is defined in the dtsi included by all 3 soc, > and it is available on all so it sits where for example > spi0 or uart0 is located > * as for arm64: this describes the registers that are > identical for arm and arm64 and the bcm2837.dtsi > is also including ../../../../arm/boot/dts/bcm283x.dtsi > > So what is the problem? The thermal driver specifies 3 different compatibles with partially different settings. If this patch is applied, all SoC (bcm2835, bcm2836, bcm2837) would use the bcm2835 settings. I can't believe this is intended. Why does the binding contains 3 different compatibles if only one is used? > > Martin > > P.s the patches apply cleanly against master. >
> On 09.09.2016, at 17:36, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > >> Martin Sperl <kernel@martin.sperl.org> hat am 9. September 2016 um 16:58 >> geschrieben: >> >> >> >>>> On 09.09.2016, at 16:25, Stefan Wahren <stefan.wahren@i2se.com> wrote: >>>> >>>> Am 09.09.2016 um 09:49 schrieb kernel@martin.sperl.org: >>>> From: Martin Sperl <kernel@martin.sperl.org> >>>> >>>> Add the node for the thermal sensor of the bcm2835-soc >>>> to the device tree. >>>> >>>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org> >>>> Reviewed-by: Eric Anholt <eric@anholt.net> >>>> --- >>>> arch/arm/boot/dts/bcm283x.dtsi | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/arch/arm/boot/dts/bcm283x.dtsi >>>> b/arch/arm/boot/dts/bcm283x.dtsi >>>> index b982522..e2e3a46 100644 >>>> --- a/arch/arm/boot/dts/bcm283x.dtsi >>>> +++ b/arch/arm/boot/dts/bcm283x.dtsi >>>> @@ -186,6 +186,12 @@ >>>> interrupts = <2 14>; /* pwa1 */ >>>> }; >>>> >>>> + thermal: thermal@0x7e212000 { >>>> + compatible = "brcm,bcm2835-thermal"; >>>> + reg = <0x7e212000 0x8>; >>>> + clocks = <&clocks BCM2835_CLOCK_TSENS>; >>>> + }; >>>> + >>> >>> Since the driver handles 3 different SoC (2835, 2836, 2837). This node >>> should be defined in the SoC specific dtsi files, because the BCM2836 >>> includes bcm283x.dtsi too. >>> >>> Be aware the patch for bcm2837 must go to ARM64. >> >> I can not really follow: >> * the node is defined in the dtsi included by all 3 soc, >> and it is available on all so it sits where for example >> spi0 or uart0 is located >> * as for arm64: this describes the registers that are >> identical for arm and arm64 and the bcm2837.dtsi >> is also including ../../../../arm/boot/dts/bcm283x.dtsi >> >> So what is the problem? > > The thermal driver specifies 3 different compatibles with partially different > settings. > If this patch is applied, all SoC (bcm2835, bcm2836, bcm2837) would use the > bcm2835 settings. > I can't believe this is intended. > > Why does the binding contains 3 different compatibles if only one is used? Now I understand - did not think of that: good catch! To minimize the patch: what about setting it to 2837 by default (in 283x.dtsi) and only change compatible in 2835.dtsi and 2836.dtsi - that is 3 files changed instead of 4 or 5... Or any other ideas how to detect which Soc we run on during runtime? I.e: query the root compatible string in the device-tree? Just before I create a new patch set to fix that. Martin
Hi Martin, > Martin Sperl <kernel@martin.sperl.org> hat am 9. September 2016 um 20:12 > geschrieben: > > > > > On 09.09.2016, at 17:36, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > > > > >> Martin Sperl <kernel@martin.sperl.org> hat am 9. September 2016 um 16:58 > >> geschrieben: > >> > >> > >> > >>>> On 09.09.2016, at 16:25, Stefan Wahren <stefan.wahren@i2se.com> wrote: > >>>> > >>>> Am 09.09.2016 um 09:49 schrieb kernel@martin.sperl.org: > >>>> From: Martin Sperl <kernel@martin.sperl.org> > >>>> > >>>> Add the node for the thermal sensor of the bcm2835-soc > >>>> to the device tree. > >>>> > >>>> Signed-off-by: Martin Sperl <kernel@martin.sperl.org> > >>>> Reviewed-by: Eric Anholt <eric@anholt.net> > >>>> --- > >>>> arch/arm/boot/dts/bcm283x.dtsi | 6 ++++++ > >>>> 1 file changed, 6 insertions(+) > >>>> > >>>> diff --git a/arch/arm/boot/dts/bcm283x.dtsi > >>>> b/arch/arm/boot/dts/bcm283x.dtsi > >>>> index b982522..e2e3a46 100644 > >>>> --- a/arch/arm/boot/dts/bcm283x.dtsi > >>>> +++ b/arch/arm/boot/dts/bcm283x.dtsi > >>>> @@ -186,6 +186,12 @@ > >>>> interrupts = <2 14>; /* pwa1 */ > >>>> }; > >>>> > >>>> + thermal: thermal@0x7e212000 { please remove 0x here. > >>>> + compatible = "brcm,bcm2835-thermal"; > >>>> + reg = <0x7e212000 0x8>; > >>>> + clocks = <&clocks BCM2835_CLOCK_TSENS>; > >>>> + }; > >>>> + > >>> > >>> Since the driver handles 3 different SoC (2835, 2836, 2837). This node > >>> should be defined in the SoC specific dtsi files, because the BCM2836 > >>> includes bcm283x.dtsi too. > >>> > >>> Be aware the patch for bcm2837 must go to ARM64. > >> > >> I can not really follow: > >> * the node is defined in the dtsi included by all 3 soc, > >> and it is available on all so it sits where for example > >> spi0 or uart0 is located > >> * as for arm64: this describes the registers that are > >> identical for arm and arm64 and the bcm2837.dtsi > >> is also including ../../../../arm/boot/dts/bcm283x.dtsi > >> > >> So what is the problem? > > > > The thermal driver specifies 3 different compatibles with partially > > different > > settings. > > If this patch is applied, all SoC (bcm2835, bcm2836, bcm2837) would use the > > bcm2835 settings. > > I can't believe this is intended. > > > > Why does the binding contains 3 different compatibles if only one is used? > > Now I understand - did not think of that: good catch! > > To minimize the patch: what about setting it to 2837 by default (in 283x.dtsi) > and only change compatible in 2835.dtsi and 2836.dtsi - that is 3 files > changed instead of 4 or 5... Please don't do this in order avoid to confusion. We have compatible strings for each SoC and we have dtsi files for each SoC. So i think the right way would be the following: bcm283x.dtsi thermal: thermal@7e212000 { reg = <0x7e212000 0x8>; clocks = <&clocks BCM2835_CLOCK_TSENS>; }; bcm2835.dtsi &thermal { compatible = "brcm,bcm2835-thermal"; status = "okay"; }; bcm2836.dtsi &thermal { compatible = "brcm,bcm2836-thermal"; status = "okay"; }; bcm2837.dtsi &thermal { compatible = "brcm,bcm2837-thermal"; status = "okay"; }; > > Or any other ideas how to detect which Soc we run on during runtime? > I.e: query the root compatible string in the device-tree? > > Just before I create a new patch set to fix that. > > Martin > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 09.09.2016 21:02, Stefan Wahren wrote: > bcm2837.dtsi > > &thermal { > compatible = "brcm,bcm2837-thermal"; > status = "okay"; > }; Note that there is only a bcm2837.dtsi in arm64/boot/dts/broadcom/ but not in arm/boot/dts - at least with the current upstream kernel. So I will just patch those dtsi that exist right now. Martin
Am 13.09.2016 um 11:06 schrieb Martin Sperl: > > > On 09.09.2016 21:02, Stefan Wahren wrote: >> bcm2837.dtsi >> >> &thermal { >> compatible = "brcm,bcm2837-thermal"; >> status = "okay"; >> }; > > Note that there is only a bcm2837.dtsi in arm64/boot/dts/broadcom/ > but not in arm/boot/dts - at least with the current upstream kernel. > > So I will just patch those dtsi that exist right now. That's correct as long as the bcm2837.dtsi change is a separate patch for ARM64 and this was the reason for my "warning" in my first reply. Stefan > > Martin
diff --git a/arch/arm/boot/dts/bcm283x.dtsi b/arch/arm/boot/dts/bcm283x.dtsi index b982522..e2e3a46 100644 --- a/arch/arm/boot/dts/bcm283x.dtsi +++ b/arch/arm/boot/dts/bcm283x.dtsi @@ -186,6 +186,12 @@ interrupts = <2 14>; /* pwa1 */ }; + thermal: thermal@0x7e212000 { + compatible = "brcm,bcm2835-thermal"; + reg = <0x7e212000 0x8>; + clocks = <&clocks BCM2835_CLOCK_TSENS>; + }; + aux: aux@0x7e215000 { compatible = "brcm,bcm2835-aux"; #clock-cells = <1>;