| Message ID | 20170823073313.1294-1-c_mykolak@mellanox.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Mykola, Sorry for taking so long to review this, thanks for the prompt with the resend. Unfortunately the patch doesn't apply cleanly to dev-4.10 - can you please rebase it and send a v2? I have some additional queries below. On Wed, 2017-08-23 at 10:33 +0300, Mykola Kostenok wrote: > Add basic pwm-tacho-controller node to ast-g5 dtsi. > > Add pwm-tacho to aspeed-bmc-mellanox-msn.dts. > > > Signed-off-by: Mykola Kostenok <c_mykolak@mellanox.com> > --- > arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts | 49 +++++++++++++++++++++++++++ > arch/arm/boot/dts/aspeed-g5.dtsi | 17 ++++++++++ > 2 files changed, 66 insertions(+) > > diff --git a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts > index 0774959..d790927 100644 > --- a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts > +++ b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts > @@ -139,3 +139,52 @@ > > status = "okay"; > }; > > +&pwm_tacho { > > + status = "okay"; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_pwm0_default>; Just to confirm: One PWM pin is driving 8 fans? > > + #address-cells = <1>; > > + #size-cells = <0>; We shouldn't need to re-specify either of these as they're done in the dtsi. However I have a question on #size-cells in the dtsi (see below). > > + #cooling-cells = <2>; > + > > + fan@0 { > > + reg = <0x00>; > > + cooling-levels = /bits/ 8 <125 151 177 203 229 255>; The bindings documentation says cooling-levels is a required property for each subnode, yet only fan@0 is defining it. Is this somehow related to only muxing PWM0 above? Regardless I think you should meet the expectation of the bindings, even if the information is redundant. > > + aspeed,fan-tach-ch = /bits/ 8 <0x00>; > > + }; > + > > + fan@1 { > > + reg = <0x00>; Reg should match the unit address in the node name, so what you've done here expectations of the dts. However, it doesn't appear to give a compiler warning. Overall I think it's worth a comment. In fact, I'm going to go out on a limb and claim the bindings are completely backwards. You may use the one PWM to drive multiple fans, but you're never going to have multiple fans feed one tacho input. As such I'd have reg represent the tacho value, and aspeed,pwm-ch map the associated PWM channel. This way we'd have: ... fan@1 { reg = <1>; aspeed,pwm-ch = /bits/ 8 <0>; }; fan@2 { reg = <2>; aspeed,pwm-ch = /bits/ 8 <0>; }; ... Looking at the datasheet we find that the hardware defines "Tach Source Registers". Putting aside that the behaviour of the registers is pretty much insane, this aligns exactly with the alternative bindings I suggested above: PWM inputs are configured for tach channels (tach channels dominate). Effectively my reg value would be an index into the fields of those registers. It's annoying that the ship has probably sailed. We should start a conversation with upstream, as your devicetree exposes the failure of the bindings. > > + aspeed,fan-tach-ch = /bits/ 8 <0x01>; > > + }; > + > > + fan@2 { > > + reg = <0x00>; > > + aspeed,fan-tach-ch = /bits/ 8 <0x02>; > > + }; > + > > + fan@3 { > > + reg = <0x00>; > > + aspeed,fan-tach-ch = /bits/ 8 <0x03>; > > + }; > + > > + fan@4 { > > + reg = <0x00>; > > + aspeed,fan-tach-ch = /bits/ 8 <0x04>; > > + }; > + > > + fan@5 { > > + reg = <0x00>; > > + aspeed,fan-tach-ch = /bits/ 8 <0x05>; > > + }; > + > > + fan@6 { > > + reg = <0x00>; > > + aspeed,fan-tach-ch = /bits/ 8 <0x06>; > > + }; > + > > + fan@7 { > > + reg = <0x00>; > > + aspeed,fan-tach-ch = /bits/ 8 <0x07>; > > + }; > +}; > diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi > index 72b6638..4c012af 100644 > --- a/arch/arm/boot/dts/aspeed-g5.dtsi > +++ b/arch/arm/boot/dts/aspeed-g5.dtsi > @@ -40,6 +40,14 @@ > > serial4 = &uart5; > > }; > > > + clocks { > > + pwm_tacho_fixed_clk: fixedclk { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-frequency = <24000000>; > > + }; > > + }; Happy to take this as it is what was done for the ast2400. Joel is getting the clock driver sorted out so hopefully we can remove it in the near future. > + > > ahb { > > compatible = "simple-bus"; > > #address-cells = <1>; > @@ -372,6 +380,15 @@ > > #size-cells = <1>; > > ranges = <0 0x1e78a000 0x1000>; > > }; > + > > > + pwm_tacho: pwm-tacho-controller@1e786000 { > > + compatible = "aspeed,ast2500-pwm-tacho"; > > + #address-cells = <1>; > > + #size-cells = <0>; The bindings documentation claims #size-cells should be 1, however both the qanta dts and your patch set this to zero. The child nodes corroborate the value of 0 - we're specifying the PWM channel which has no need for a length. We should update the bindings, though I'm not sure how happy upstream will be about that. If I recall correctly Rob already quizzed you about why the bindings were being changed not long after they were applied. Still, better to be right. Maybe we can rev the bindings and fix the PWM/tach mapping above? Cheers for an interesting patch. Andrew > > + reg = <0x1e786000 0x1000>; > > + clocks = <&pwm_tacho_fixed_clk>; > > + status = "disabled"; > > + }; > > }; > > }; > };
> -----Original Message----- > From: Andrew Jeffery [mailto:andrew@aj.id.au] > Sent: Tuesday, August 29, 2017 11:15 AM > To: Mykola Kostenok <c_mykolak@mellanox.com>; Joel Stanley > <joel@jms.id.au>; openbmc@lists.ozlabs.org > Cc: Ohad Oz <ohado@mellanox.com>; Vadim Pasternak > <vadimp@mellanox.com> > Subject: Re: [PATCH linux dev-4.10 resend] dts: add aspeed-pwm-tacho to > msn dts. > > Hi Mykola, > > Sorry for taking so long to review this, thanks for the prompt with the > resend. > > Unfortunately the patch doesn't apply cleanly to dev-4.10 - can you please > rebase it and send a v2? Hi, Andrew. Yes, sure. > > I have some additional queries below. > > On Wed, 2017-08-23 at 10:33 +0300, Mykola Kostenok wrote: > > Add basic pwm-tacho-controller node to ast-g5 dtsi. > > > > Add pwm-tacho to aspeed-bmc-mellanox-msn.dts. > > > > > Signed-off-by: Mykola Kostenok <c_mykolak@mellanox.com> > > --- > > arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts | 49 > > +++++++++++++++++++++++++++ > > arch/arm/boot/dts/aspeed-g5.dtsi | 17 ++++++++++ > > 2 files changed, 66 insertions(+) > > > > diff --git a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts > > b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts > > index 0774959..d790927 100644 > > --- a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts > > +++ b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts > > @@ -139,3 +139,52 @@ > > > status = "okay"; > > }; > > > > +&pwm_tacho { > > > + status = "okay"; > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_pwm0_default>; > > Just to confirm: One PWM pin is driving 8 fans? > Yes, in Mellanox msn system open PWM driving 8 fans. > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > We shouldn't need to re-specify either of these as they're done in the dtsi. > However I have a question on #size-cells in the dtsi (see below). > Acked. > > > + #cooling-cells = <2>; > > + > > > + fan@0 { > > > + reg = <0x00>; > > > + cooling-levels = /bits/ 8 <125 151 177 203 229 255>; > > The bindings documentation says cooling-levels is a required property for > each subnode, yet only fan@0 is defining it. > > Is this somehow related to only muxing PWM0 above? > > Regardless I think you should meet the expectation of the bindings, even if > the information is redundant. Oh, it's a mistake in docs. cooling-levels is not required - it's optional. We tried to make separated node for pwm cooling-levels, but it was not acked by Rob Herring. Now it can be set only once for each PWM. As we use only PWM0 we set it once. And nothing bad happens with another systems without cooling-levels. > > > > + aspeed,fan-tach-ch = /bits/ 8 <0x00>; > > > + }; > > + > > > + fan@1 { > > > + reg = <0x00>; > > Reg should match the unit address in the node name, so what you've done > here expectations of the dts. However, it doesn't appear to give a compiler > warning. > > Overall I think it's worth a comment. > > In fact, I'm going to go out on a limb and claim the bindings are completely > backwards. You may use the one PWM to drive multiple fans, but you're > never going to have multiple fans feed one tacho input. As such I'd have reg > represent the tacho value, and aspeed,pwm-ch map the associated PWM > channel. > This way we'd have: > > ... > fan@1 { > reg = <1>; > aspeed,pwm-ch = /bits/ 8 <0>; > }; > > fan@2 { > reg = <2>; > aspeed,pwm-ch = /bits/ 8 <0>; > }; > ... > > Looking at the datasheet we find that the hardware defines "Tach Source > Registers". Putting aside that the behaviour of the registers is pretty much > insane, this aligns exactly with the alternative bindings I suggested above: > PWM inputs are configured for tach channels (tach channels dominate). > Effectively my reg value would be an index into the fields of those registers. > > It's annoying that the ship has probably sailed. We should start a > conversation with upstream, as your devicetree exposes the failure of the > bindings. > We just take it as it was. > > > + aspeed,fan-tach-ch = /bits/ 8 <0x01>; > > > + }; > > + > > > + fan@2 { > > > + reg = <0x00>; > > > + aspeed,fan-tach-ch = /bits/ 8 <0x02>; > > > + }; > > + > > > + fan@3 { > > > + reg = <0x00>; > > > + aspeed,fan-tach-ch = /bits/ 8 <0x03>; > > > + }; > > + > > > + fan@4 { > > > + reg = <0x00>; > > > + aspeed,fan-tach-ch = /bits/ 8 <0x04>; > > > + }; > > + > > > + fan@5 { > > > + reg = <0x00>; > > > + aspeed,fan-tach-ch = /bits/ 8 <0x05>; > > > + }; > > + > > > + fan@6 { > > > + reg = <0x00>; > > > + aspeed,fan-tach-ch = /bits/ 8 <0x06>; > > > + }; > > + > > > + fan@7 { > > > + reg = <0x00>; > > > + aspeed,fan-tach-ch = /bits/ 8 <0x07>; > > > + }; > > +}; > > diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi > > b/arch/arm/boot/dts/aspeed-g5.dtsi > > index 72b6638..4c012af 100644 > > --- a/arch/arm/boot/dts/aspeed-g5.dtsi > > +++ b/arch/arm/boot/dts/aspeed-g5.dtsi > > @@ -40,6 +40,14 @@ > > > serial4 = &uart5; > > > }; > > > > > + clocks { > > > + pwm_tacho_fixed_clk: fixedclk { > > > + compatible = "fixed-clock"; > > > + #clock-cells = <0>; > > > + clock-frequency = <24000000>; > > > + }; > > > + }; > > Happy to take this as it is what was done for the ast2400. Joel is getting the > clock driver sorted out so hopefully we can remove it in the near future. > Ok. > > + > > > ahb { > > > compatible = "simple-bus"; > > > #address-cells = <1>; > > @@ -372,6 +380,15 @@ > > > #size-cells = <1>; > > > ranges = <0 0x1e78a000 0x1000>; > > > }; > > + > > > > + pwm_tacho: pwm-tacho-controller@1e786000 { > > > + compatible = "aspeed,ast2500-pwm-tacho"; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > The bindings documentation claims #size-cells should be 1, however both the > qanta dts and your patch set this to zero. The child nodes corroborate the > value of 0 - we're specifying the PWM channel which has no need for a > length. I think it’s a mistake in documentation. > We should update the bindings, though I'm not sure how happy upstream > will be about that. If I recall correctly Rob already quizzed you about why the > bindings were being changed not long after they were applied. Still, better to > be right. Maybe we can rev the bindings and fix the PWM/tach mapping > above? > > Cheers for an interesting patch. > > Andrew > It's ok with us. Maybe it will be next step later. Best regards, Mykola Kostenok. > > > + reg = <0x1e786000 0x1000>; > > > + clocks = <&pwm_tacho_fixed_clk>; > > > + status = "disabled"; > > > + }; > > > }; > > > }; > > };
Hi Mykola, On Tue, 2017-08-29 at 15:05 +0000, Mykola Kostenok wrote: > > -----Original Message----- > > > > From: Andrew Jeffery [mailto:andrew@aj.id.au] > > Sent: Tuesday, August 29, 2017 11:15 AM > > > > To: Mykola Kostenok <c_mykolak@mellanox.com>; Joel Stanley > > > > <joel@jms.id.au>; openbmc@lists.ozlabs.org > > > > Cc: Ohad Oz <ohado@mellanox.com>; Vadim Pasternak > > > > <vadimp@mellanox.com> > > Subject: Re: [PATCH linux dev-4.10 resend] dts: add aspeed-pwm-tacho to > > msn dts. > > > > Hi Mykola, > > > > Sorry for taking so long to review this, thanks for the prompt with the > > resend. > > > > Unfortunately the patch doesn't apply cleanly to dev-4.10 - can you please > > rebase it and send a v2? > > Hi, Andrew. > Yes, sure. > > > > > I have some additional queries below. > > > > On Wed, 2017-08-23 at 10:33 +0300, Mykola Kostenok wrote: > > > Add basic pwm-tacho-controller node to ast-g5 dtsi. > > > > > > Add pwm-tacho to aspeed-bmc-mellanox-msn.dts. > > > > > > > Signed-off-by: Mykola Kostenok <c_mykolak@mellanox.com> > > > > > > --- > > > arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts | 49 > > > +++++++++++++++++++++++++++ > > > arch/arm/boot/dts/aspeed-g5.dtsi | 17 ++++++++++ > > > 2 files changed, 66 insertions(+) > > > > > > diff --git a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts > > > b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts > > > index 0774959..d790927 100644 > > > --- a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts > > > +++ b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts > > > @@ -139,3 +139,52 @@ > > > > status = "okay"; > > > > > > }; > > > > > > +&pwm_tacho { > > > > > > > > + status = "okay"; > > > > > > > > + pinctrl-names = "default"; > > > > + pinctrl-0 = <&pinctrl_pwm0_default>; > > > > Just to confirm: One PWM pin is driving 8 fans? > > > > Yes, in Mellanox msn system open PWM driving 8 fans. > > > > > > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > We shouldn't need to re-specify either of these as they're done in the dtsi. > > However I have a question on #size-cells in the dtsi (see below). > > > > Acked. > > > > > + #cooling-cells = <2>; > > > > > > + > > > > > > > > + fan@0 { > > > > > > > > + reg = <0x00>; > > > > + cooling-levels = /bits/ 8 <125 151 177 203 229 255>; > > > > The bindings documentation says cooling-levels is a required property for > > > > each subnode, yet only fan@0 is defining it. > > > > Is this somehow related to only muxing PWM0 above? > > > > Regardless I think you should meet the expectation of the bindings, even if > > the information is redundant. > > Oh, it's a mistake in docs. cooling-levels is not required - it's optional. > We tried to make separated node for pwm cooling-levels, but it was not acked by Rob Herring. > Now it can be set only once for each PWM. As we use only PWM0 we set it once. > And nothing bad happens with another systems without cooling-levels. > > > > > > > > > > > + aspeed,fan-tach-ch = /bits/ 8 <0x00>; > > > > + }; > > > > > > + > > > > > > > > + fan@1 { > > > > + reg = <0x00>; > > > > Reg should match the unit address in the node name, so what you've done > > here expectations of the dts. However, it doesn't appear to give a compiler > > warning. > > > > Overall I think it's worth a comment. > > > > In fact, I'm going to go out on a limb and claim the bindings are completely > > backwards. You may use the one PWM to drive multiple fans, but you're > > never going to have multiple fans feed one tacho input. As such I'd have reg > > represent the tacho value, and aspeed,pwm-ch map the associated PWM > > channel. > > This way we'd have: > > > > ... > > > > fan@1 { > > > > reg = <1>; > > > > aspeed,pwm-ch = /bits/ 8 <0>; > > }; > > > > > > fan@2 { > > > > reg = <2>; > > > > aspeed,pwm-ch = /bits/ 8 <0>; > > }; > > ... > > > > Looking at the datasheet we find that the hardware defines "Tach Source > > Registers". Putting aside that the behaviour of the registers is pretty much > > insane, this aligns exactly with the alternative bindings I suggested above: > > PWM inputs are configured for tach channels (tach channels dominate). > > Effectively my reg value would be an index into the fields of those registers. > > > > It's annoying that the ship has probably sailed. We should start a > > conversation with upstream, as your devicetree exposes the failure of the > > bindings. > > > > We just take it as it was. I understand. I'm discussing fans with upstream anyway, so I'll bring this issue up. > > > > > > > > > + aspeed,fan-tach-ch = /bits/ 8 <0x01>; > > > > + }; > > > > > > + > > > > > > > > + fan@2 { > > > > > > > > + reg = <0x00>; > > > > > > > > + aspeed,fan-tach-ch = /bits/ 8 <0x02>; > > > > + }; > > > > > > + > > > > > > > > + fan@3 { > > > > > > > > + reg = <0x00>; > > > > > > > > + aspeed,fan-tach-ch = /bits/ 8 <0x03>; > > > > + }; > > > > > > + > > > > > > > > + fan@4 { > > > > > > > > + reg = <0x00>; > > > > > > > > + aspeed,fan-tach-ch = /bits/ 8 <0x04>; > > > > + }; > > > > > > + > > > > > > > > + fan@5 { > > > > > > > > + reg = <0x00>; > > > > > > > > + aspeed,fan-tach-ch = /bits/ 8 <0x05>; > > > > + }; > > > > > > + > > > > > > > > + fan@6 { > > > > > > > > + reg = <0x00>; > > > > > > > > + aspeed,fan-tach-ch = /bits/ 8 <0x06>; > > > > + }; > > > > > > + > > > > > > > > + fan@7 { > > > > > > > > + reg = <0x00>; > > > > > > > > + aspeed,fan-tach-ch = /bits/ 8 <0x07>; > > > > + }; > > > > > > +}; > > > diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi > > > b/arch/arm/boot/dts/aspeed-g5.dtsi > > > index 72b6638..4c012af 100644 > > > --- a/arch/arm/boot/dts/aspeed-g5.dtsi > > > +++ b/arch/arm/boot/dts/aspeed-g5.dtsi > > > @@ -40,6 +40,14 @@ > > > > > > > > serial4 = &uart5; > > > > > > > > }; > > > > > > > > + clocks { > > > > > > > > + pwm_tacho_fixed_clk: fixedclk { > > > > > > > > + compatible = "fixed-clock"; > > > > > > > > + #clock-cells = <0>; > > > > > > > > + clock-frequency = <24000000>; > > > > > > > > + }; > > > > + }; > > > > Happy to take this as it is what was done for the ast2400. Joel is getting the > > clock driver sorted out so hopefully we can remove it in the near future. > > > > Ok. > > > > + > > > > > > > > ahb { > > > > > > > > compatible = "simple-bus"; > > > > #address-cells = <1>; > > > > > > @@ -372,6 +380,15 @@ > > > > > > > > #size-cells = <1>; > > > > > > > > ranges = <0 0x1e78a000 0x1000>; > > > > }; > > > > > > + > > > > > + pwm_tacho: pwm-tacho-controller@1e786000 { > > > > > > > > > > > > + compatible = "aspeed,ast2500-pwm-tacho"; > > > > > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > The bindings documentation claims #size-cells should be 1, however both the > > qanta dts and your patch set this to zero. The child nodes corroborate the > > value of 0 - we're specifying the PWM channel which has no need for a > > length. > > I think it’s a mistake in documentation. Right; something we should fix. I'll include this with my upstream discussion on fans. Cheers, Andrew
diff --git a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts index 0774959..d790927 100644 --- a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts +++ b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts @@ -139,3 +139,52 @@ status = "okay"; }; +&pwm_tacho { + status = "okay"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_pwm0_default>; + #address-cells = <1>; + #size-cells = <0>; + #cooling-cells = <2>; + + fan@0 { + reg = <0x00>; + cooling-levels = /bits/ 8 <125 151 177 203 229 255>; + aspeed,fan-tach-ch = /bits/ 8 <0x00>; + }; + + fan@1 { + reg = <0x00>; + aspeed,fan-tach-ch = /bits/ 8 <0x01>; + }; + + fan@2 { + reg = <0x00>; + aspeed,fan-tach-ch = /bits/ 8 <0x02>; + }; + + fan@3 { + reg = <0x00>; + aspeed,fan-tach-ch = /bits/ 8 <0x03>; + }; + + fan@4 { + reg = <0x00>; + aspeed,fan-tach-ch = /bits/ 8 <0x04>; + }; + + fan@5 { + reg = <0x00>; + aspeed,fan-tach-ch = /bits/ 8 <0x05>; + }; + + fan@6 { + reg = <0x00>; + aspeed,fan-tach-ch = /bits/ 8 <0x06>; + }; + + fan@7 { + reg = <0x00>; + aspeed,fan-tach-ch = /bits/ 8 <0x07>; + }; +}; diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi index 72b6638..4c012af 100644 --- a/arch/arm/boot/dts/aspeed-g5.dtsi +++ b/arch/arm/boot/dts/aspeed-g5.dtsi @@ -40,6 +40,14 @@ serial4 = &uart5; }; + clocks { + pwm_tacho_fixed_clk: fixedclk { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <24000000>; + }; + }; + ahb { compatible = "simple-bus"; #address-cells = <1>; @@ -372,6 +380,15 @@ #size-cells = <1>; ranges = <0 0x1e78a000 0x1000>; }; + + pwm_tacho: pwm-tacho-controller@1e786000 { + compatible = "aspeed,ast2500-pwm-tacho"; + #address-cells = <1>; + #size-cells = <0>; + reg = <0x1e786000 0x1000>; + clocks = <&pwm_tacho_fixed_clk>; + status = "disabled"; + }; }; }; };
Add basic pwm-tacho-controller node to ast-g5 dtsi. Add pwm-tacho to aspeed-bmc-mellanox-msn.dts. Signed-off-by: Mykola Kostenok <c_mykolak@mellanox.com> --- arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts | 49 +++++++++++++++++++++++++++ arch/arm/boot/dts/aspeed-g5.dtsi | 17 ++++++++++ 2 files changed, 66 insertions(+)