[PATCH linux dev-4.10 resend] dts: add aspeed-pwm-tacho to msn dts.
Andrew Jeffery
andrew at aj.id.au
Wed Aug 30 14:49:52 AEST 2017
Hi Mykola,
On Tue, 2017-08-29 at 15:05 +0000, Mykola Kostenok wrote:
> > -----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.
I understand. I'm discussing fans with upstream anyway, so I'll bring this
issue up.
>
> > > > > > > > + 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.
Right; something we should fix. I'll include this with my upstream discussion
on fans.
Cheers,
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170830/dad4da71/attachment-0001.sig>
More information about the openbmc
mailing list