RE: [PATCH V10 1/4] dt-bindings: fsl: scu: add thermal binding

[Anson Huang]

Hi, Rob/Eduardo

Best Regards!
Anson Huang

> -----Original Message-----
> From: Rob Herring [mailto:robh@xxxxxxxxxx]
> Sent: 2019å2æ28æ 22:49
> To: Anson Huang <anson.huang@xxxxxxx>
> Cc: mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> catalin.marinas@xxxxxxx; will.deacon@xxxxxxx; rui.zhang@xxxxxxxxx;
> edubezval@xxxxxxxxx; daniel.lezcano@xxxxxxxxxx; Aisheng Dong
> <aisheng.dong@xxxxxxx>; ulf.hansson@xxxxxxxxxx; sboyd@xxxxxxxxxx;
> Daniel Baluta <daniel.baluta@xxxxxxx>; Andy Gross
> <andy.gross@xxxxxxxxxx>; horms+renesas@xxxxxxxxxxxx; heiko@xxxxxxxxx;
> arnd@xxxxxxxx; maxime.ripard@xxxxxxxxxxx; bjorn.andersson@xxxxxxxxxx;
> jagan@xxxxxxxxxxxxxxxxxxxx; enric.balletbo@xxxxxxxxxxxxx;
> marc.w.gonzalez@xxxxxxx; olof@xxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> pm@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> Subject: Re: [PATCH V10 1/4] dt-bindings: fsl: scu: add thermal binding
> 
> On Wed, Feb 27, 2019 at 6:48 PM Anson Huang <anson.huang@xxxxxxx>
> wrote:
> >
> > Hi, Rob
> >
> > Best Regards!
> > Anson Huang
> >
> > > -----Original Message-----
> > > From: Rob Herring [mailto:robh@xxxxxxxxxx]
> > > Sent: 2019å2æ28æ 7:55
> > > To: Anson Huang <anson.huang@xxxxxxx>
> > > Cc: mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx;
> > > s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> > > catalin.marinas@xxxxxxx; will.deacon@xxxxxxx; rui.zhang@xxxxxxxxx;
> > > edubezval@xxxxxxxxx; daniel.lezcano@xxxxxxxxxx; Aisheng Dong
> > > <aisheng.dong@xxxxxxx>; ulf.hansson@xxxxxxxxxx; sboyd@xxxxxxxxxx;
> > > Daniel Baluta <daniel.baluta@xxxxxxx>; Andy Gross
> > > <andy.gross@xxxxxxxxxx>; horms+renesas@xxxxxxxxxxxx;
> > > heiko@xxxxxxxxx; arnd@xxxxxxxx; maxime.ripard@xxxxxxxxxxx;
> > > bjorn.andersson@xxxxxxxxxx; jagan@xxxxxxxxxxxxxxxxxxxx;
> > > enric.balletbo@xxxxxxxxxxxxx; marc.w.gonzalez@xxxxxxx;
> > > olof@xxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > > linux- pm@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> > > Subject: Re: [PATCH V10 1/4] dt-bindings: fsl: scu: add thermal
> > > binding
> > >
> > > On Wed, Feb 27, 2019 at 08:46:21AM +0000, Anson Huang wrote:
> > > > NXP i.MX8QXP is an ARMv8 SoC with a Cortex-M4 core inside as
> > > > system controller, the system controller is in charge of system
> > > > power, clock and thermal sensors etc. management, Linux kernel has
> > > > to communicate with system controller via MU (message unit) IPC to
> > > > get temperature from thermal sensors, this patch adds binding doc
> > > > for i.MX system controller thermal driver.
> > > >
> > > > Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
> > > > ---
> > > > Changes since V9:
> > > >     - change #thermal-sensor-cells value in example to 1, since
> > > > there are
> > > other
> > > >       thermal sensors inside system controller, it is just because
> > > > there are
> > > still
> > > >       some issue, so system controller does NOT expose them for
> > > > now,
> > > they could
> > > >       be exposed later, so it should be 1 from HW perspective.
> > > > ---
> > > >  .../devicetree/bindings/arm/freescale/fsl,scu.txt   | 21
> > > +++++++++++++++++++++
> > > >  1 file changed, 21 insertions(+)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > index 72d481c..855270b 100644
> > > > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
> > > > @@ -122,6 +122,21 @@ RTC bindings based on SCU Message Protocol
> > > > Required properties:
> > > >  - compatible: should be "fsl,imx8qxp-sc-rtc";
> > > >
> > > > +Thermal bindings based on SCU Message Protocol
> > > > +------------------------------------------------------------
> > > > +
> > > > +Required properties:
> > > > +- compatible:                      Should be :
> > > > +                             "fsl,imx8qxp-sc-thermal"
> > > > +                           followed by "fsl,imx-sc-thermal";
> > > > +
> > > > +- #thermal-sensor-cells:   See
> > > Documentation/devicetree/bindings/thermal/thermal.txt
> > > > +                           for a description.
> > > > +
> > > > +- imx,sensor-resource-id:  A single integer for single thermal
> > > > +zone's
> > > resource ID or
> > > > +                           an array of integers to specify each
> > > > + thermal
> > > zone's sensor
> > > > +                           resource ID.
> > >
> > > Can't you put the resource ids in the thermal-sensor cells? Why do
> > > you need to list them here?
> >
> > For the thermal-sensor cells, if you meant the argument of tsens
> > phandle, then the reason is that argument is for sensor index starting
> > from 0, previous I use it for our resource ID, but it looks confused,
> > since user will think there are many sensors there per Eduardo's comment.
> >
> > +                       thermal-sensors = <&tsens 0>;
> >
> > If you meant putting it in each thermal sensor node instead of "tsens"
> > node, then in previous patch series, I put this "
> > imx,sensor-resource-id " property in each thermal sensor node, but the
> > thermal sensor nodes are parsed by common thermal framework, thermal
> > driver will need to find the thermal zone node and go through every
> > child node to get the resource id again, so Eduardo suggested to put it in
> our platform tsens node, that makes our thermal driver code much more
> simple.
> 
> The phandle args are meant to be an id typically. There's absolutely no
> requirement they are 0-N based. They often are because things like
> interrupts are 0-N or clocks have no h/w id. If you already have an id, use it.
> Don't invent your own.

At the beginning, I put the HW resource ID in the "tsens" phandle argument of "thermal-sensors" node,
see  below patch I sent before, the benefit is I do NOT need to add new property for passing
HW resource ID to driver, the disadvantage is, I have to parse the thermal_zones' each child node
and get the HW resource ID from phandle argument(searing thermal_zones node and go through all its
child node, and get the phandle argument), they are by default ONLY parsed by thermal core
driver. When we register thermal zone, we have to pass the HW resource ID when calling
devm_thermal_zone_of_sensor_register(), if we add our own property to pass the HW resource ID,
then no need to do so, we just pass the index 0-N for each thermal sensors in devicetree which
also with phandle argument 0-N. So using our own property makes the driver much more simple.

So, @Eduardo, which direction I should go? Looks like Rob suggests just put the HW resource ID
in the phandle argument like what I did at the beginning, can you advise?

Thanks,
Anson.

https://patchwork.kernel.org/patch/10703849/
> +	thermal_zones: thermal-zones {
> +		cpu-thermal0 {
> +			polling-delay-passive = <250>;
> +			polling-delay = <2000>;
> +			thermal-sensors = <&tsens 355>;
> +			trips {
> +				cpu_alert0: trip0 {
> +					temperature = <107000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};

> 
> Rob