[PATCH linux dev-4.10 resend] dts: add aspeed-pwm-tacho to msn dts.
Mykola Kostenok
c_mykolak at mellanox.com
Wed Aug 30 01:05:47 AEST 2017
> -----Original Message-----
> From: Andrew Jeffery [mailto:andrew at aj.id.au]
> Sent: Tuesday, August 29, 2017 11:15 AM
> To: Mykola Kostenok <c_mykolak at mellanox.com>; Joel Stanley
> <joel at jms.id.au>; openbmc at lists.ozlabs.org
> Cc: Ohad Oz <ohado at mellanox.com>; Vadim Pasternak
> <vadimp at 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 at 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 at 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 at 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 at 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 at 1 {
> reg = <1>;
> aspeed,pwm-ch = /bits/ 8 <0>;
> };
>
> fan at 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 at 2 {
> > > + reg = <0x00>;
> > > + aspeed,fan-tach-ch = /bits/ 8 <0x02>;
> > > + };
> > +
> > > + fan at 3 {
> > > + reg = <0x00>;
> > > + aspeed,fan-tach-ch = /bits/ 8 <0x03>;
> > > + };
> > +
> > > + fan at 4 {
> > > + reg = <0x00>;
> > > + aspeed,fan-tach-ch = /bits/ 8 <0x04>;
> > > + };
> > +
> > > + fan at 5 {
> > > + reg = <0x00>;
> > > + aspeed,fan-tach-ch = /bits/ 8 <0x05>;
> > > + };
> > +
> > > + fan at 6 {
> > > + reg = <0x00>;
> > > + aspeed,fan-tach-ch = /bits/ 8 <0x06>;
> > > + };
> > +
> > > + fan at 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 at 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";
> > > + };
> > > };
> > > };
> > };
More information about the openbmc
mailing list