Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file

[Serge Semin <fancer.lancer@gmail.com>]

On Mon, Dec 12, 2016 at 05:04:31PM -0600, Rob Herring <robh@kernel.org> wrote:
> On Mon, Dec 5, 2016 at 1:04 PM, Serge Semin <fancer.lancer@gmail.com> wrote:
> > On Mon, Dec 05, 2016 at 11:27:07AM -0600, Rob Herring <robh@kernel.org> wrote:
> >> On Mon, Dec 5, 2016 at 9:25 AM, Serge Semin <fancer.lancer@gmail.com> wrote:
> >> > On Mon, Dec 05, 2016 at 08:46:21AM -0600, Rob Herring <robh@kernel.org> wrote:
> >> >> On Tue, Nov 29, 2016 at 01:38:21AM +0300, Serge Semin wrote:
> >> >> > See cover-letter for changelog
> >> >> >
> >> >> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> >> >> >
> >> >> > ---
> >> >> >  .../devicetree/bindings/misc/idt_89hpesx.txt       | 41 ++++++++++++++++++++++
> >> >>
> >> >> There's not a better location for this? I can't tell because you don't
> >> >> describe what the device is.
> >> >>
> >> >
> >> > The device is PCIe-switch EEPROM driver with additional debug-interface to
> >> > access the switch CSRs. EEPROM is accesses via a separate i2c-slave
> >> > interface of the switch.
> >> >
> >> > There might be another place to put the binding file in. There is a special
> >> > location for EEPROM drivers bindings - Documentation/devicetree/bindings/eeprom/ .
> >> > But as far as I understood from the files put in there, it's intended for
> >> > pure EEPROM drivers only. On the other hand the directory I've chosen:
> >> > Documentation/devicetree/bindings/misc/
> >> > mostly intended for some unusual devices. My device isn't usual, since it
> >> > has CSRs debug-interface as well. Additionally I've found
> >> > eeprom-93xx46.txt binding file there, which describes EEPROM bindings.
> >> >
> >> > Anyway if you find the file should be placed in
> >> > Documentation/devicetree/bindings/eeprom/ instead, I'll move it, it's not
> >> > that a big problem.
> >> >
> >
> > What about this comment? Shall the file be left at the path I placed it?
> >
> >> >> >  1 file changed, 41 insertions(+)
> >> >> >  create mode 100644 Documentation/devicetree/bindings/misc/idt_89hpesx.txt
> >> >> >
> >> >> > diff --git a/Documentation/devicetree/bindings/misc/idt_89hpesx.txt b/Documentation/devicetree/bindings/misc/idt_89hpesx.txt
> >> >> > index 0000000..469cc93
> >> >> > --- /dev/null
> >> >> > +++ b/Documentation/devicetree/bindings/misc/idt_89hpesx.txt
> >> >> > @@ -0,0 +1,41 @@
> >> >> > +EEPROM / CSR SMBus-slave interface of IDT 89HPESx devices
> >> >> > +
> >> >> > +Required properties:
> >> >> > +  - compatible : should be "<manufacturer>,<type>"
> >> >> > +            Basically there is only one manufacturer: idt, but some
> >> >> > +            compatible devices may be produced in future. Following devices
> >> >> > +            are supported: 89hpes8nt2, 89hpes12nt3, 89hpes24nt6ag2,
> >> >> > +            89hpes32nt8ag2, 89hpes32nt8bg2, 89hpes12nt12g2, 89hpes16nt16g2,
> >> >> > +            89hpes24nt24g2, 89hpes32nt24ag2, 89hpes32nt24bg2;
> >> >> > +            89hpes12n3, 89hpes12n3a, 89hpes24n3, 89hpes24n3a;
> >> >> > +            89hpes32h8, 89hpes32h8g2, 89hpes48h12, 89hpes48h12g2,
> >> >> > +            89hpes48h12ag2, 89hpes16h16, 89hpes22h16, 89hpes22h16g2,
> >> >> > +            89hpes34h16, 89hpes34h16g2, 89hpes64h16, 89hpes64h16g2,
> >> >> > +            89hpes64h16ag2;
> >> >> > +            89hpes12t3g2, 89hpes24t3g2, 89hpes16t4, 89hpes4t4g2,
> >> >> > +            89hpes10t4g2, 89hpes16t4g2, 89hpes16t4ag2, 89hpes5t5,
> >> >> > +            89hpes6t5, 89hpes8t5, 89hpes8t5a, 89hpes24t6, 89hpes6t6g2,
> >> >> > +            89hpes24t6g2, 89hpes16t7, 89hpes32t8, 89hpes32t8g2,
> >> >> > +            89hpes48t12, 89hpes48t12g2.
> >> >> > +            Current implementation of the driver doesn't have any device-
> >> >>
> >> >> Driver capabilties are irrelevant to bindings.
> >> >>
> >> >
> >> > Why? I've told in the comment, that the devices actually differ by the CSRs
> >> > map. Even though it's not reflected in the code at the moment, the CSRs
> >> > read/write restrictions can be added by some concerned programmer in
> >> > future. But If I left something like "compatible : idt,89hpesx" device
> >> > only, it will be problematic to add that functionality.
> >>
> >> Bindings describe the h/w, not what the Linux, FreeBSD, etc. driver
> >> does. You don't want to be changing the binding doc when the driver
> >> changes.
> >>
> >> > Howbeit If you think it's not necessary and "compatible = idt,89hpesx" is
> >> > ok, it's perfectly fine for me to make it this way. The property will be
> >> > even simpler, than current approach.
> >>
> >> NO! That's not at all what I'm suggesting. Specific compatible strings
> >> are the right way to go for the reasons you give. You just don't need
> >> to state why here (because it is true for all bindings).
> >>
> >
> > Oh, I just misunderstood what you said. I'll discard the comment.
> >
> >> >> > +            specific functionalities. But since each of them differs
> >> >> > +            by registers mapping, CSRs read/write restrictions can be
> >> >> > +            added in future.
> >> >> > +  - reg :   I2C address of the IDT 89HPES device.
> >> >> > +
> >> >> > +Optional properties:
> >> >> > +  - read-only :     Parameterless property disables writes to the EEPROM
> >> >> > +  - idt,eesize : Size of EEPROM device connected to IDT 89HPES i2c-master bus
> >> >> > +            (default value is 4096 bytes if option isn't specified)
> >> >> > +  - idt,eeaddr : Custom address of EEPROM device
> >> >> > +            (If not specified IDT 89HPESx device will try to communicate
> >> >> > +             with EEPROM sited by default address - 0x50)
> >> >>
> >> >> Don't we already have standard EEPROM properties that could be used
> >> >> here?
> >> >>
> >> >
> >> > If we do, just tell me which one. There are standard options:
> >>
> >> You can grep thru bindings as easily as I can. I can't do that for
> >> everyone's binding.
> >>
> >
> > It won't be necessary due to the next comment.
> >
> >> > "compatible, reg, pagesize, read-only". There isn't any connected with
> >> > EEPROM actual size.
> >> > Why so? Because standard EEPROM-drivers determine the device size from the
> >> > compatible-string name. Such approach won't work in this case, because
> >> > PCIe-switch and it EEPROM are actually two different devices. Look at the
> >> > chain of the usual platform board design:
> >> > Host <--- i2c ----> i2c-slave iface |PCIe-switch| i2c-master iface <--- i2c ---> EEPROM
> >> >
> >> > As you cas see the Host reaches EEPROM through the set of PCIe-switch
> >> > i2c-interfaces. In order to properly get data from it my driver needs actual
> >> > EEPROM size and it address in the i2c-master bus of the PCIe-switch, in
> >> > addition to the standard reg-field, which is address of PCIe-switch i2c-slave
> >> > interface and read-only parameter if EEPROM-device has got WP pin asserted.
> >>
> >> Ah, this needs to be much different than I thought. You need to model
> >> (i.e. use the same binding) the EEPROM node just like it was directly
> >> attached to the host. So this means you need the 2nd i2c bus modeled
> >> which means you need the PCIe switch modeled. A rough outline of the
> >> nodes would look like this:
> >>
> >> host-i2c: i2c {
> >>   compatible ="host-i2c"
> >> };
> >>
> >> pcie {
> >>     pcie-switch {
> >>         i2c-bus = <&host-i2c>;
> >>         i2c-bus {
> >>             eeprom@50 {
> >>             };
> >>         };
> >>     };
> >> };
> >>
> >> So this models the PCIe switch as a PCIe device, it has a phandle back
> >> to it's controller since it's not a child of the i2c controller. Then
> >> the devices on switches i2c bus are modeled as children of the switch.
> >>
> >> Alternatively, it could be described all as children of host-i2c node.
> >> It's common for i2c devices to have downstream i2c buses. I2C muxes
> >> are one example and there are bindings defined for all this. There's
> >> also chips like mpu-6050 that have slave buses.
> >>
> >> Rob
> >
> > I think I understand what you says. However let me just bring some details
> > to make things clear.
> >
> > First of all the driver doesn't do any PCI-Express-related work. The device
> > !IDT PCI Express switch! just has two additional i2c interfaces: i2c-slave
> > and i2c-master. As it is obvious from the bus-names i2c-slave is the interface,
> > where IDT PCIe-switch device is actually slave. This interface can be reached
> > from the host by ordinary i2c buses. i2c-master interface is connected to an
> > i2c-bus, where IDT PCIe-switch is single master. This bus can have just one
> > EEPROM device to store some initialization data. Host can send some specific
> > smbus-packets to i2c-slave interface of IDT PCIe-switch in order to
> > preinitialize EEPROM data, connected to i2c-master interface of the device.
> >
> > Additionally IDT PCIe-switch handles some special smbus packets coming to it
> > i2c-slave interface to read/write its internal CSR. This interface can be
> > used to debug the device, when there are problems with it usual PCI Express
> > related functioning.
> >
> > So to speak, it wouldn't be good to have PCIe-switch declared in dts as a
> > PCI-device, since PCI-bus is actually dynamically populated by PCI-core
> > subsystem.
> 
> Why not? The DT is just extra data for what is not discoverable. Is
> the device actually hotplugable and in a dynamic location/slot? If
> not, then describing the device in DT is not uncommon. If it is
> hotplugable, you still have same problem of knowing which I2C bus it
> is on. Even if you know for your design, generally speaking you may
> not know.
> 

Device isn't hotplugable, it's always placed on the circuit. So to speak it is
always known which i2c bus it's placed on. That's why I placed the device
description in the dts.

> > According to what you said and the device/driver design I described, the
> > following bindings can be suggested:
> >
> > i2c0: i2c@FFFF0000 {
> >         compatible = "vendor,i2c-adapter";
> >         #address-cells = <1>;
> >         #size-cells = <0>;
> >
> >         idt_i2c_iface: idt@60 {
> >                 compatible = "idt,89hpes32nt8ag2";
> >                 reg = <0x60>;
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >
> >                 eeprom@51 {
> >                         compatible = "at,24c64";
> >                         reg = <0x51>;
> >                         read-only;
> >                 };
> >         };
> > };
> >
> > Suppose there is some host-i2c adapter like "vendor,i2c-adapter" and
> > i2c-slave interface of IDT PCIe-switch is connected to it. In this way
> > i2c-slave interface will be visible like ordinary i2c-device with just
> > one subnode. This subnode explains the actual EEPROM connected to
> > IDT PCIe-switch i2c-master interface.
> >
> > Does it look acceptable? It seems like your last suggestion. Is it?
> 
> That is the 2nd option. My concern is this may work for your immediate
> case, but if you started to need to describe the PCIe interface it
> would not work. Similarly, we started out describing USB hubs with I2C
> interfaces this way and it has proven to be in adequate for some
> cases. So we're moving to describing the USB hierarchy in DT. I'm
> concerned that while it may work for you, if the PCIe interface has
> any dependencies like regulators or something, then you would need to
> have a PCIe node.
> 
> Rob

Alright then. I'll develop the second option and resend the patchset.

-Sergey