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

[Rob Herring]

On Mon, Dec 5, 2016 at 1:04 PM, Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
> On Mon, Dec 05, 2016 at 11:27:07AM -0600, Rob Herring <robh@xxxxxxxxxx> wrote:
>> On Mon, Dec 5, 2016 at 9:25 AM, Serge Semin <fancer.lancer@xxxxxxxxx> wrote:
>> > On Mon, Dec 05, 2016 at 08:46:21AM -0600, Rob Herring <robh@xxxxxxxxxx> 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@xxxxxxxxx>
>> >> >
>> >> > ---
>> >> >  .../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.

> 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