Date:   Fri, 9 Dec 2016 04:57:24 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Mark Rutland <mark.rutland@....com>,
        Sergey.Semin@...latforms.ru,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file

Rob,
Could you please respond on these comments? I've got some free time, so I wanna
rewrite the code until I've not got busy again.

Regards,
-Sergey

On Mon, Dec 05, 2016 at 10:04:56PM +0300, Serge Semin <fancer.lancer@...il.com> wrote:
> On Mon, Dec 05, 2016 at 11:27:07AM -0600, Rob Herring <robh@...nel.org> wrote:
> > On Mon, Dec 5, 2016 at 9:25 AM, Serge Semin <fancer.lancer@...il.com> wrote:
> > > On Mon, Dec 05, 2016 at 08:46:21AM -0600, Rob Herring <robh@...nel.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@...il.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.
> 
> According to what you said and the device/driver design I described, the
> following bindings can be suggested:
> 
> i2c0: i2c@...F0000 {
> 	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?
> 
> Thanks,
> -Sergey
> 

Powered by blists - more mailing lists