Re: [PATCH 1/3] dt-bindings: thermal: Add binding document for SR thermal

[Srinath Mannam]

Hi Rob Herring,

Please find my comments in line..

On Fri, Jun 1, 2018 at 7:59 PM, Rob Herring <robh@xxxxxxxxxx> wrote:
> On Fri, Jun 1, 2018 at 3:51 AM, Srinath Mannam
> <srinath.mannam@xxxxxxxxxxxx> wrote:
>> Hi Rob Herring,
>>
>> Thank you for the review.
>> Please find my answers inline..
>>
>> On Thu, May 31, 2018 at 10:18 PM, Rob Herring <robh@xxxxxxxxxx> wrote:
>>> On Mon, May 28, 2018 at 11:11:22AM +0530, Srinath Mannam wrote:
>>>> From: Pramod Kumar <pramod.kumar@xxxxxxxxxxxx>
>>>>
>>>> Add binding document for supported thermal implementation
>>>> in Stingray.
>>>>
>>>> Signed-off-by: Pramod Kumar <pramod.kumar@xxxxxxxxxxxx>
>>>> Reviewed-by: Ray Jui <ray.jui@xxxxxxxxxxxx>
>>>> Reviewed-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
>>>> Reviewed-by: Srinath Mannam <srinath.mannam@xxxxxxxxxxxx>
>>>> ---
>>>>  .../bindings/thermal/brcm,sr-thermal.txt           | 45 ++++++++++++++++++++++
>>>>  1 file changed, 45 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/thermal/brcm,sr-thermal.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/thermal/brcm,sr-thermal.txt b/Documentation/devicetree/bindings/thermal/brcm,sr-thermal.txt
>>>> new file mode 100644
>>>> index 0000000..33f9e11
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/thermal/brcm,sr-thermal.txt
>>>> @@ -0,0 +1,45 @@
>>>> +* Broadcom Stingray Thermal
>>>> +
>>>> +This binding describes thermal sensors that is part of Stingray SoCs.
>>>> +
>>>> +Required properties:
>>>> +- compatible : Must be "brcm,sr-thermal"
>>>> +- reg : memory where tmon data will be available.
>>>
>>> What type of memory is this?
>> This is shared memory(cache-able) which is shared between two micro controllers.
>
> What else is in this shared memory? You should probably be
> representing that as a whole somewhere in DT.
Nothing else except 4 byte memory containing temperature value of one
thermal zone.
At present we configured 6 thermal zones. So six separate 4byte
memories provided in
shared memory.
>
>> One micro controller update temperature of all thermal zones in this
>> shared memory.
>> thermal driver running on another micro controller monitors
>> temperature data of all thermal zones.
>>>
>>>> +
>>>> +Example:
>>>> +     tmons {
>>>> +             compatible = "simple-bus";
>>>> +             #address-cells = <1>;
>>>> +             #size-cells = <1>;
>>>> +             ranges;
>>>> +
>>>> +             tmon_ihost0: thermal@8f100000 {
>>>> +                     compatible = "brcm,sr-thermal";
>>>> +                     reg = <0x8f100000 0x4>;
>>>> +             };
>>>
>>> Convince me that you need a node per register. This can all be
>>> accomplished with a single node and either a single reg entry or a
>>> series of reg entries.
>>>
>> In our system we have 6 different thermal zones. each node belongs to
>> one thermal zone.
>> thermal driver detects each node as separate thermal zone and monitors
>> separately.
>> register entry is only 4byte memory which contains the temperature
>> value of respective thermal zone.
>> Based on requirement we can increase or decrease the number thermal
>> zones need to monitor.
>
> Still, all this can be accomplished with a single node. Having a node
> per register in DT doesn't scale.
Our approach is like we have individual controllers have one driver
with multiple DT nodes.
In coming future we will add more thermal zones based on our
requirement. then we can just
add new DT node, instead of having driver changes.
With this approach we can configure each thermal zone at different trip values.
So we must have all thermal zones are independent.
>
> Rob