From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753615AbcLMOIj (ORCPT ); Tue, 13 Dec 2016 09:08:39 -0500 Received: from mail-lf0-f65.google.com ([209.85.215.65]:35811 "EHLO mail-lf0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752383AbcLMOIg (ORCPT ); Tue, 13 Dec 2016 09:08:36 -0500 Date: Tue, 13 Dec 2016 17:08:31 +0300 From: Serge Semin To: Rob Herring Cc: Greg Kroah-Hartman , Srinivas Kandagatla , Andrew Lunn , Mark Rutland , Sergey.Semin@t-platforms.ru, "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" Subject: Re: [PATCH v2 2/2] eeprom: Add IDT 89HPESx driver bindings file Message-ID: <20161213140831.GA1670@mobilestation> References: <1475450025-29507-1-git-send-email-fancer.lancer@gmail.com> <1480372701-30560-1-git-send-email-fancer.lancer@gmail.com> <1480372701-30560-3-git-send-email-fancer.lancer@gmail.com> <20161205144621.yxwr7spmsvyilan5@rob-hp-laptop> <20161205152502.GA17538@mobilestation> <20161205190456.GA25116@mobilestation> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Dec 12, 2016 at 05:04:31PM -0600, Rob Herring wrote: > On Mon, Dec 5, 2016 at 1:04 PM, Serge Semin wrote: > > On Mon, Dec 05, 2016 at 11:27:07AM -0600, Rob Herring wrote: > >> On Mon, Dec 5, 2016 at 9:25 AM, Serge Semin wrote: > >> > On Mon, Dec 05, 2016 at 08:46:21AM -0600, Rob Herring wrote: > >> >> On Tue, Nov 29, 2016 at 01:38:21AM +0300, Serge Semin wrote: > >> >> > See cover-letter for changelog > >> >> > > >> >> > Signed-off-by: Serge Semin > >> >> > > >> >> > --- > >> >> > .../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 "," > >> >> > + 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