Message-ID: <884d3c16-84e8-e38a-731a-313ba7105176@linux.intel.com>
Date:   Wed, 18 Apr 2018 14:57:09 -0700
From:   Jae Hyun Yoo <jae.hyun.yoo@...ux.intel.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Alan Cox <alan@...ux.intel.com>, Andrew Jeffery <andrew@...id.au>,
        Andrew Lunn <andrew@...n.ch>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Arnd Bergmann <arnd@...db.de>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Fengguang Wu <fengguang.wu@...el.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Haiyue Wang <haiyue.wang@...ux.intel.com>,
        James Feist <james.feist@...ux.intel.com>,
        Jason M Biils <jason.m.bills@...ux.intel.com>,
        Jean Delvare <jdelvare@...e.com>,
        Joel Stanley <joel@....id.au>,
        Julia Cartwright <juliac@....teric.us>,
        Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
        Milton Miller II <miltonm@...ibm.com>,
        Pavel Machek <pavel@....cz>,
        Randy Dunlap <rdunlap@...radead.org>,
        Stef van Os <stef.van.os@...drive-technologies.com>,
        Sumeet R Pawnikar <sumeet.r.pawnikar@...el.com>,
        Vernon Mauery <vernon.mauery@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        linux-doc@...r.kernel.org, devicetree@...r.kernel.org,
        Linux HWMON List <linux-hwmon@...r.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>
Subject: Re: [PATCH v3 07/10] Documentation: dt-bindings: Add documents for
 PECI hwmon client drivers

On 4/18/2018 2:28 PM, Rob Herring wrote:
> On Wed, Apr 18, 2018 at 3:28 PM, Jae Hyun Yoo
> <jae.hyun.yoo@...ux.intel.com> wrote:
>> On 4/18/2018 7:32 AM, Rob Herring wrote:
>>>
>>> On Tue, Apr 17, 2018 at 3:40 PM, Jae Hyun Yoo
>>> <jae.hyun.yoo@...ux.intel.com> wrote:
>>>>
>>>> On 4/16/2018 4:51 PM, Jae Hyun Yoo wrote:
>>>>>
>>>>>
>>>>> On 4/16/2018 4:22 PM, Jae Hyun Yoo wrote:
>>>>>>
>>>>>>
>>>>>> On 4/16/2018 11:14 AM, Rob Herring wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Apr 10, 2018 at 11:32:09AM -0700, Jae Hyun Yoo wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> This commit adds dt-bindings documents for PECI cputemp and dimmtemp
>>>>>>>> client
>>>>>>>> drivers.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>
>>>> [...]
>>>>
>>>>>>>> +Example:
>>>>>>>> +    peci-bus@0 {
>>>>>>>> +        #address-cells = <1>;
>>>>>>>> +        #size-cells = <0>;
>>>>>>>> +        < more properties >
>>>>>>>> +
>>>>>>>> +        peci-dimmtemp@...0 {
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> unit-address is wrong.
>>>>>>>
>>>>>>
>>>>>> Will fix it using the reg value.
>>>>>>
>>>>>>> It is a different bus from cputemp? Otherwise, you have conflicting
>>>>>>> addresses. If that's the case, probably should make it clear by
>>>>>>> showing
>>>>>>> different host adapters for each example.
>>>>>>>
>>>>>>
>>>>>> It could be the same bus with cputemp. Also, client address sharing is
>>>>>> possible by PECI core if the functionality is different. I mean,
>>>>>> cputemp and
>>>>>> dimmtemp targeting the same client is possible case like this.
>>>>>> peci-cputemp@30
>>>>>> peci-dimmtemp@30
>>>>>>
>>>>>
>>>>> Oh, I got your point. Probably, I should change these separate settings
>>>>> into one like
>>>>>
>>>>> peci-client@30 {
>>>>>        compatible = "intel,peci-client";
>>>>>        reg = <0x30>;
>>>>> };
>>>>>
>>>>> Then cputemp and dimmtemp drivers could refer the same compatible
>>>>> string.
>>>>> Will rewrite it.
>>>>>
>>>>
>>>> I've checked it again and realized that it should use function based node
>>>> name like:
>>>>
>>>> peci-cputemp@30
>>>> peci-dimmtemp@30
>>>>
>>>> If it use the same string like 'peci-client@30', the drivers cannot be
>>>> selectively enabled. The client address sharing way is well handled in
>>>> PECI
>>>> core and this way would be better for the future implementations of other
>>>> PECI functional drivers such as crash dump driver and so on. So I'm going
>>>> change the unit-address only.
>>>
>>>
>>> 2 nodes at the same address is wrong (and soon dtc will warn you on
>>> this). You have 2 potential options. The first is you need additional
>>> address information in the DT if these are in fact 2 independent
>>> devices. This could be something like a function number to use
>>> something from PCI addressing. From what I found on PECI, it doesn't
>>> seem to have anything like that. The 2nd option is you have a single
>>> DT node which registers multiple hwmon devices. DT nodes and drivers
>>> don't have to be 1-1. Don't design your DT nodes from how you want to
>>> partition drivers in some OS.
>>>
>>> Rob
>>>
>>
>> Please correct me if I'm wrong but I'm still thinking that it is
>> possible. Also, I did compile it but dtc doesn't make a warning. Let me
>> show an another use case which is similar to this case:
> 
> I did say *soon*. It's in dtc repo, but not the kernel copy yet.
> 
>> In arch/arm/boot/dts/aspeed-g5.dtsi
>> [...]
>> lpc_host: lpc-host@80 {
>>          compatible = "aspeed,ast2500-lpc-host", "simple-mfd", "syscon";
>>          reg = <0x80 0x1e0>;
>>          reg-io-width = <4>;
>>
>>          #address-cells = <1>;
>>          #size-cells = <1>;
>>          ranges = <0x0 0x80 0x1e0>;
>>
>>          lpc_ctrl: lpc-ctrl@0 {
>>                  compatible = "aspeed,ast2500-lpc-ctrl";
>>                  reg = <0x0 0x80>;
>>                  clocks = <&syscon ASPEED_CLK_GATE_LCLK>;
>>                  status = "disabled";
>>          };
>>
>>          lpc_snoop: lpc-snoop@0 {
>>                  compatible = "aspeed,ast2500-lpc-snoop";
>>                  reg = <0x0 0x80>;
>>                  interrupts = <8>;
>>                  status = "disabled";
>>          };
>> }
>> [...]
>>
>> This is device tree setting for LPC interface and its child nodes.
>> LPC interface can be used as a multi-functional interface such as
>> snoop 80, KCS, SIO and so on. In this use case, lpc-ctrl@0 and
>> lpc-snoop@0 are sharing their address range from their individual
>> driver modules and they can be registered quite well through both
>> static dt or dynamic dtoverlay. PECI is also a multi-functional
>> interface which is similar to the above case, I think.
> 
> This case too is poor design and should be fixed as well. Simply put,
> you can have 2 devices on a bus at the same address without some sort
> of mux or arbitration device in the middle. If you have a device/block
> with multiple functions provided to the OS, then it is the OS's
> problem to arbitrate access. It is not a DT problem because OS's can
> vary in how they handle that both from OS to OS and over time.
> 
> Rob
> 

If I change it to a single DT node which registers 2 hwmon devices using
the 2nd option above, then I still have 2 devices on a bus at the same
address. Does it also make a problem to the OS then?

Jae

Powered by blists - more mailing lists