From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jae Hyun Yoo Subject: Re: [v4 07/11] dt-bindings: hwmon: Add documents for PECI hwmon client drivers Date: Wed, 23 May 2018 14:56:43 -0700 Message-ID: <8b56c103-fdab-fc98-f4d8-9bf435a9b59b@linux.intel.com> References: <20180521195905.27983-1-jae.hyun.yoo@linux.intel.com> <20180522164230.GA11707@rob-hp-laptop> <51752610-d2c1-7fbc-101e-e99346fa29e4@linux.intel.com> <22bd0e23-69ad-5858-656e-16c77007913c@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <22bd0e23-69ad-5858-656e-16c77007913c@linux.intel.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Rob Herring Cc: Mark Rutland , Haiyue Wang , Vernon Mauery , James Feist , devicetree@vger.kernel.org, "linux-kernel@vger.kernel.org" , Andrew Jeffery , Arnd Bergmann , Jason M Biils , Joel Stanley List-Id: devicetree@vger.kernel.org On 5/23/2018 1:03 PM, Jae Hyun Yoo wrote: > On 5/23/2018 12:33 PM, Rob Herring wrote: >> On Wed, May 23, 2018 at 11:37 AM, Jae Hyun Yoo >> wrote: >>> On 5/23/2018 8:11 AM, Rob Herring wrote: >>>> >>>> On Tue, May 22, 2018 at 12:18 PM, Jae Hyun Yoo >>>> wrote: >>>>> >>>>> On 5/22/2018 9:42 AM, Rob Herring wrote: >>>>>> >>>>>> >>>>>> On Mon, May 21, 2018 at 12:59:05PM -0700, Jae Hyun Yoo wrote: >>>>>>> >>>>>>> >>>>>>> This commit adds dt-bindings documents for PECI hwmon client >>>>>>> drivers. >>>>>>> >>>>>>> Signed-off-by: Jae Hyun Yoo >>>>>>> Reviewed-by: Haiyue Wang >>>>>>> Reviewed-by: James Feist >>>>>>> Reviewed-by: Vernon Mauery >>>>>>> Cc: Andrew Jeffery >>>>>>> Cc: Arnd Bergmann >>>>>>> Cc: Jason M Biils >>>>>>> Cc: Joel Stanley >>>>>>> --- >>>>>>>     .../bindings/hwmon/peci-cputemp.txt           | 23 >>>>>>> ++++++++++++++++++ >>>>>>>     .../bindings/hwmon/peci-dimmtemp.txt          | 24 >>>>>>> +++++++++++++++++++ >>>>>>>     2 files changed, 47 insertions(+) >>>>>>>     create mode 100644 >>>>>>> Documentation/devicetree/bindings/hwmon/peci-cputemp.txt >>>>>>>     create mode 100644 >>>>>>> Documentation/devicetree/bindings/hwmon/peci-dimmtemp.txt >>>>>>> >>>>>>> diff --git >>>>>>> a/Documentation/devicetree/bindings/hwmon/peci-cputemp.txt >>>>>>> b/Documentation/devicetree/bindings/hwmon/peci-cputemp.txt >>>>>>> new file mode 100644 >>>>>>> index 000000000000..2f59aee12d9e >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/devicetree/bindings/hwmon/peci-cputemp.txt >>>>>>> @@ -0,0 +1,23 @@ >>>>>>> +Bindings for Intel PECI (Platform Environment Control Interface) >>>>>>> cputemp >>>>>>> driver. >>>>>>> + >>>>>>> +Required properties: >>>>>>> +- compatible : Should be "intel,peci-cputemp". >>>>>>> +- reg        : Should contain address of a client CPU. Address >>>>>>> range >>>>>>> of >>>>>>> CPU >>>>>>> +              clients is starting from 0x30 based on PECI >>>>>>> specification. >>>>>>> + >>>>>>> +Example: >>>>>>> +       peci-bus@0 { >>>>>>> +               #address-cells = <1>; >>>>>>> +               #size-cells = <0>; >>>>>>> +               < more properties > >>>>>>> + >>>>>>> +               peci-cputemp@30 { >>>>>>> +                       compatible = "intel,peci-cputemp"; >>>>>>> +                       reg = <0x30>; >>>>>>> +               }; >>>>>> >>>>>> >>>>>> >>>>>> [...] >>>>>> >>>>>>> +               peci-dimmtemp@30 { >>>>>>> +                       compatible = "intel,peci-dimmtemp"; >>>>>>> +                       reg = <0x30>; >>>>>>> +               }; >>>>>> >>>>>> >>>>>> >>>>>> As I said in the prior version, 2 nodes at the same address is wrong. >>>>>> >>>>>> Rob >>>>>> >>>>> >>>>> In PECI bus, 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. >>>> >>>> >>>> Yes, I get that. A single host still has to address slave devices. >>>> >>>>> 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. >>>> >>>> >>>> Multiple clients to a single device is common, but that is a software >>>> problem and doesn't belong in DT. >>>> >>>> I don't think there is a single other case in the kernel where >>>> multiple drivers can bind to the same device at a given bus address. >>>> That is why we have things like MFD. Though in this case, why can't >>>> one hwmon driver register multiple hwmon devices (cpu and dimm temps)? >>>> >>> >>> It was implemented as a single driver until v2 but dimm temps need >>> delayed creation unlikely the cpu temps on hwmon subsystem because of >>> memory training behavior of remote x86 cpus. Since hwmon doesn't allow >>> incremental creation, I had to divide it into two, cputemp and dimmtemp, >>> so that cputemp can be registered immediately when the remote x86 cpu >>> turns on and dimmtemp can be registered by delayed creation. It is the >>> reason why I had to make the two hwmon driver modules that sharing a >>> single device address. >> >> That all sounds like kernel problems to me. Stop designing your DT >> binding around what the kernel can or can't *currently* support. >> >>> Additionally, PECI isn't limited for temperature >>> monitoring feature but it can be used for other functions such as >>> platform management, cpu interface tuning and diagnostics and failure >>> analysis, so in case of adding a new driver for the functions, we should >>> add an another DT node which is sharing the same cpu address. >> >> No, the driver should add support for those additional functions. >> Perhaps you will need to use MFD. >> > > Do you mean that the device address sharing is acceptable if I make > these nodes under "simple-mfd"? > > Thanks, > > -Jae Hi Rob, I'm planning to change the whole PECI node like below: peci: peci@1e78b000 { compatible = "simple-bus"; #address-cells = <1>; #size-cells = <1>; ranges = <0x0 0x1e78b000 0x60>; peci0: peci-bus@0 { compatible = "aspeed,ast2500-peci"; reg = <0x0 0x60>; #address-cells = <1>; #size-cells = <0>; interrupts = <15>; clocks = <&syscon ASPEED_CLK_GATE_REFCLK>; resets = <&syscon ASPEED_RESET_PECI>; clock-frequency = <24000000>; msg-timing = <1>; addr-timing = <1>; rd-sampling-point = <8>; cmd-timeout-ms = <1000>; status = "disabled"; peci-client@30 { compatible = "simple-mfd", "syscon"; reg = <0x30>; cputemp: cputemp { compatible = "intel,peci-cputemp"; }; dimmtemp: dimmtemp { compatible = "intel,peci-dimmtemp"; }; }; peci-client@31 { compatible = "simple-mfd", "syscon"; reg = <0x31>; cputemp: cputemp { compatible = "intel,peci-cputemp"; }; dimmtemp: dimmtemp { compatible = "intel,peci-dimmtemp"; }; }; }; }; Can you please check whether it is acceptable or not? Thanks, -Jae