Message-ID: <411481f1-78fe-d53f-cf56-c58a5ce954f3@linux.intel.com>
Date:   Thu, 19 Apr 2018 12:48:55 -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:57 PM, Jae Hyun Yoo wrote:
> 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

Additionally, I need to explain that there is one and only bus host
(adapter) and multiple clients on a PECI bus, and PECI spec doesn't
allow multiple originators so only the host device can originate
message. In this implementation, all message transactions on a bus from
client driver modules and user space will be serialized well in the PECI
core bus driver so bus occupation and traffic arbitration will be
managed well in the PECI core bus driver even in case of a bus has 2 
client drivers at the same address. I'm sure that this implementation 
doesn't make that kind of problem to OS.

Jae

Powered by blists - more mailing lists