[PATCH linux dev-4.10 resend] dts: add aspeed-pwm-tacho to msn dts.

[Andrew Jeffery]

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 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?

> > +	#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 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.

> > +		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.

> > +		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.

> +
> >  	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.
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";
> > +			};
> >  		};
> >  	};
>  };
-------------- 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/20170829/f44a3142/attachment.sig>