Re: [PATCH v3 07/10] Documentation: dt-bindings: Add documents for PECI hwmon client drivers

[Jae Hyun Yoo]

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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@cpu0 {
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > 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