| Message ID | 1410274470-12712-5-git-send-email-wsa@the-dreams.de |
|---|---|
| State | RFC |
| Headers | show |
cc'ing: devicetree Am Dienstag, 9. September 2014, 16:54:30 schrieb Wolfram Sang: > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > > Not for upstream! > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > arch/arm/boot/dts/r8a7790-lager.dts | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/boot/dts/r8a7790-lager.dts > b/arch/arm/boot/dts/r8a7790-lager.dts index 856b4236b674..12aa2f21e6fa > 100644 > --- a/arch/arm/boot/dts/r8a7790-lager.dts > +++ b/arch/arm/boot/dts/r8a7790-lager.dts > @@ -205,9 +205,9 @@ > renesas,function = "msiof1"; > }; > > - iic1_pins: iic1 { > - renesas,groups = "iic1"; > - renesas,function = "iic1"; > + i2c1_pins: i2c1 { > + renesas,groups = "i2c1"; > + renesas,function = "i2c1"; > }; > > iic2_pins: iic2 { > @@ -356,10 +356,15 @@ > status = "ok"; > }; > > -&iic1 { > +&i2c1 { > status = "ok"; > - pinctrl-0 = <&iic1_pins>; > + pinctrl-0 = <&i2c1_pins>; > pinctrl-names = "default"; > + > + eeprom@64 { > + compatible = "slave-24c02"; a "real" compatible value should have vendor,device syntax. > + reg = <0x64>; we had some discussions in the past how to represent i2c master devices in device tree. The outcome was to use to a special representation to distinguish them from slave devices, e.g. something like reg = <0x1 0x64>, where the 0x1 would mean "master" device (0x0 -> slave). If nothing is added, then it's also slave. The adapter address cell would also need to be changed if this extension is used. An alternative would be to use a special property (e.g. slave address), but that would encode the same value twice (or even three times). Marc -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 11, 2014 at 02:17:16PM +0200, Marc Dietrich wrote: > cc'ing: devicetree Thanks, I forgot that! > > -&iic1 { > > +&i2c1 { > > status = "ok"; > > - pinctrl-0 = <&iic1_pins>; > > + pinctrl-0 = <&i2c1_pins>; > > pinctrl-names = "default"; > > + > > + eeprom@64 { > > + compatible = "slave-24c02"; > > a "real" compatible value should have vendor,device syntax. I know. This is maybe a grey line between "configuration" and "hardware description". If we want an I2C slave, we need to things: a) HW support in the I2C master driver b) SW support by giving the slave callback some functionality (like my eeprom simulator) a) should be compiled in with the master driver and does not need any DT binding, so this is easy. b) could be seen as a configuration thing since the functionality backend could be changed at runtime even. Think of being a usb-gadget here. However, since I2C is multi-master and not hot-pluggable, some other master might be depending on what I2C slave is actually used. So, it could be seen as hardware description, too? Not entirely sure. The drawback you also noticed is that there is no vendor for these functionalities. Maybe "wsa" since I did the functionality driver :) To be honest, the above DT snipplet was done to get easily started. If DT maintainers think this is more configuration, not much is lost. The only difference is that the userspace instantiation method should be used then: # echo slave-24c02 0x64 > /sys/bus/i2c/devices/i2c-0/new_device > > > + reg = <0x64>; > > we had some discussions in the past how to represent i2c master devices in > device tree. The outcome was to use to a special representation to distinguish > them from slave devices, e.g. something like reg = <0x1 0x64>, where the 0x1 > would mean "master" device (0x0 -> slave). If nothing is added, then it's also > slave. The adapter address cell would also need to be changed if this > extension is used. Please don't change the reg-layout which is around since forever. It will break all existing devicetrees. NAK. > An alternative would be to use a special property (e.g. slave address), but > that would encode the same value twice (or even three times). As said, if all breaks we handle it at runtime. Thanks, Wolfram
> b) could be seen as a configuration thing since the functionality > backend could be changed at runtime even. Come to think of it, not only the functionality, also the address can be changed at runtime. This makes me think it should really not be in DT after all. People will probably find out that it is possible to put it in DT, but it should not be encouraged and they should do it on their own risk.
Am Donnerstag, 11. September 2014, 16:12:58 schrieb Wolfram Sang: > On Thu, Sep 11, 2014 at 02:17:16PM +0200, Marc Dietrich wrote: > > > + reg = <0x64>; > > > > we had some discussions in the past how to represent i2c master devices in > > device tree. The outcome was to use to a special representation to > > distinguish them from slave devices, e.g. something like reg = <0x1 > > 0x64>, where the 0x1 would mean "master" device (0x0 -> slave). If > > nothing is added, then it's also slave. The adapter address cell would > > also need to be changed if this extension is used. > > Please don't change the reg-layout which is around since forever. > It will break all existing devicetrees. NAK. it won't break existing device trees. It would only need to be extended for systems where the adapter uses master and slave modes (or if you add a master device to the i2c client list). So you need to change the device tree anyway. All other setups can continue to use a single reg. Thinking more about it, are there any i2c clients parsing the reg property directly? In this case I would agree, otherwise, only i2c core needs some update - right? Marc > > An alternative would be to use a special property (e.g. slave address), > > but > > that would encode the same value twice (or even three times). > > As said, if all breaks we handle it at runtime. > > Thanks, > > Wolfram
Am Donnerstag, 11. September 2014, 16:40:04 schrieb Wolfram Sang: > > b) could be seen as a configuration thing since the functionality > > backend could be changed at runtime even. > > Come to think of it, not only the functionality, also the address can be > changed at runtime. This makes me think it should really not be in DT > after all. even worse, there can be multiple masters and slaves changing their role on the fly AFAIK. So the best dt can do is to provide an initial configuration, so all drivers know where they are and where to start. Everything else can be changed during runtime. > People will probably find out that it is possible to put it in DT, but > it should not be encouraged and they should do it on their own risk.
On Thu, Sep 11, 2014 at 04:52:22PM +0200, Marc Dietrich wrote: > Am Donnerstag, 11. September 2014, 16:40:04 schrieb Wolfram Sang: > > > b) could be seen as a configuration thing since the functionality > > > backend could be changed at runtime even. > > > > Come to think of it, not only the functionality, also the address can be > > changed at runtime. This makes me think it should really not be in DT > > after all. > > even worse, there can be multiple masters and slaves changing their role on > the fly AFAIK. So the best dt can do is to provide an initial configuration, > so all drivers know where they are and where to start. Everything else can be > changed during runtime. Why do you want DT to be involved at all?
Am Donnerstag, 11. September 2014, 16:54:22 schrieb Wolfram Sang: > On Thu, Sep 11, 2014 at 04:52:22PM +0200, Marc Dietrich wrote: > > Am Donnerstag, 11. September 2014, 16:40:04 schrieb Wolfram Sang: > > > > b) could be seen as a configuration thing since the functionality > > > > backend could be changed at runtime even. > > > > > > Come to think of it, not only the functionality, also the address can be > > > changed at runtime. This makes me think it should really not be in DT > > > after all. > > > > even worse, there can be multiple masters and slaves changing their role > > on > > the fly AFAIK. So the best dt can do is to provide an initial > > configuration, so all drivers know where they are and where to start. > > Everything else can be changed during runtime. > > Why do you want DT to be involved at all? Imagine a device which supports both, slave or master mode. The driver needs to know in which mode it should operate. This cannot be hard coded, because on different boards, different modes can be used. The point is, that if we define a dt binding for master device on slave adapters it will be there forever. So even if it makes no sense for the example eeprom simulator (or even our embedded controller), it may make sense for other or future devices. On the other hand, we had some painful discussion about that issue in the past, so if no one else steps up to propose a good alternative binding, I will be the last to criticize your approach. Marc
Hi Marc, On Fri, Sep 12, 2014 at 9:51 AM, Marc Dietrich <marvin24@gmx.de> wrote: > Am Donnerstag, 11. September 2014, 16:54:22 schrieb Wolfram Sang: >> On Thu, Sep 11, 2014 at 04:52:22PM +0200, Marc Dietrich wrote: >> > Am Donnerstag, 11. September 2014, 16:40:04 schrieb Wolfram Sang: >> > > > b) could be seen as a configuration thing since the functionality >> > > > backend could be changed at runtime even. >> > > >> > > Come to think of it, not only the functionality, also the address can be >> > > changed at runtime. This makes me think it should really not be in DT >> > > after all. >> > >> > even worse, there can be multiple masters and slaves changing their role >> > on >> > the fly AFAIK. So the best dt can do is to provide an initial >> > configuration, so all drivers know where they are and where to start. >> > Everything else can be changed during runtime. >> >> Why do you want DT to be involved at all? > > Imagine a device which supports both, slave or master mode. The driver needs > to know in which mode it should operate. This cannot be hard coded, because on > different boards, different modes can be used. > > The point is, that if we define a dt binding for master device on slave > adapters it will be there forever. So even if it makes no sense for the > example eeprom simulator (or even our embedded controller), it may make sense > for other or future devices. DT describes the hardware. The most you can do is describe an i2c adapter, and an i2c bus with (physical) i2c slaves, if any, and (other) i2c masters[*], if any. If the description contains i2c slaves, the i2c adapter can operate in (but may be not limited to) master mode. I2c slaves implemented in Linux are not physical i2c slaves, but just a piece of software. They can appear/disappear/change at any time. Hence I think this is something to be configured by userspace, like "tie the i2c-eeprom-slave driver to address 64 on i2c bus 3". And then you can have the mixed case, where the i2c adapter is both a master, talking to the physical slaves, and a slave, being talked to by another master. But that's just a superposition of the two cases above, right? [*] Are there bindings for other masters on an i2c bus? Just my 0.02€ (as long as those coins still exist). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Why do you want DT to be involved at all? > > Imagine a device which supports both, slave or master mode. The driver needs > to know in which mode it should operate. This cannot be hard coded, because on > different boards, different modes can be used. Okay, it sounds weird to me that a device is not able to switch between master and slave, but if you say so. Also, solving this issue would also handle potential weird IP blocks which can be slave only, right? What if you use two different adapter drivers or compatibles? One for master-mode, one for slave-mode (slave could leave algo->master_xfer empty, so the slave mode driver cannot send packets). I'm brainstorming here, so while it should work IMO I will probably need a second thought. So, in the DT you would have a block registering the I2C slave core which binds to a simple driver providing reg_slave/unreg_slave and pass on the slave-event. Then you could instantiate slave clients as said before. Maybe we need a real world example? > The point is, that if we define a dt binding for master device on slave > adapters it will be there forever. So even if it makes no sense for the > example eeprom simulator (or even our embedded controller), it may make sense > for other or future devices. I don't know what you mean here. Again, an example might help? Thanks, Wolfram
Am Freitag, 12. September 2014, 10:33:48 schrieb Wolfram Sang: > > > Why do you want DT to be involved at all? > > > > Imagine a device which supports both, slave or master mode. The driver > > needs to know in which mode it should operate. This cannot be hard coded, > > because on different boards, different modes can be used. > > Okay, it sounds weird to me that a device is not able to switch between > master and slave, but if you say so. Also, solving this issue would also > handle potential weird IP blocks which can be slave only, right? I didn't said device. I meant the driver (software) needs to know if which mode to operate the device. DT has to provide just enough information to select the right mode depending on the board implementation. > What if you use two different adapter drivers or compatibles? One for > master-mode, one for slave-mode (slave could leave algo->master_xfer > empty, so the slave mode driver cannot send packets). I'm brainstorming > here, so while it should work IMO I will probably need a second thought. > > So, in the DT you would have a block registering the I2C slave core > which binds to a simple driver providing reg_slave/unreg_slave and > pass on the slave-event. Then you could instantiate slave clients as > said before. > > Maybe we need a real world example? ok, take our embedded controller driver (in staging/nvec) as an example. It's basicly an MFD connecting keyboard, mouse, power, gpio, and some other stuff to the soc. The MFD operates in master mode while the SOC is the I2C slave. Theoretically, these roles could also switch (but that's not defined in the nvec protocol). So the i2c client driver sits in between the adapter (in slave or master mode) and the the MFD children (keyboard, mouse, ...) and provides read/write/cb functions (nvec uses a side channel gpio to be able to initiate writes on its own) to its children. The MFD children don't care about how the i2c communication is done, but the client driver needs to decide in which mode it should talk to the EC. If this is not autodetectable, the mode needs to be provided by the device tree. In this case you cannot use separate drivers for master and slave mode (or you would need another layer to hide the driver multiplexing). > > The point is, that if we define a dt binding for master device on slave > > adapters it will be there forever. So even if it makes no sense for the > > example eeprom simulator (or even our embedded controller), it may make > > sense for other or future devices. > > I don't know what you mean here. Again, an example might help? I just wanted to say that the discussion here should find a *generic* way to distinguish slave and master clients on the i2c bus by using a proper DT binding. Marc
> ok, take our embedded controller driver (in staging/nvec) as an example. It's > basicly an MFD connecting keyboard, mouse, power, gpio, and some other stuff > to the soc. The MFD operates in master mode while the SOC is the I2C slave. > Theoretically, these roles could also switch (but that's not defined in the > nvec protocol). I see these cases currently: 1) my current case The I2C slave is not needed for board bringup, mainly for development or playing around. It can have this or that functionality on this or that address. -> does not belong into DT, should be done in userspace 2) Slave mode is needed for board bringup Some other components need a specific I2C slave to be present before userspace is available, otherwise the system is unusable. This is IMO then a hardware description and justifies DT entries: DT pseudocode: i2c { compatible = "nvidia, tegra-i2c"; ec-slave@42 { compatible = "nvidia, ax100-ec-slave"; reg = <0x42>; }; }; Of course, an MFD driver providing "nvidia, ax100-ec-slave" is needed which uses the I2C slave mode of the tegra controller. 3) Master + slave mode is needed for board bringup: Again, IMO a hardware description, so we could use: i2c { compatible = "nvidia, tegra-i2c"; ec@64 { compatible = "nvidia, ax100-ec"; reg = <0x64>; }; }; This is a standard I2C device driver (using the MFD framework) where i2c-tegra would act as a master on the client for 0x64. However, its probe function can fill an i2c_board_device (the driver should know the slave device address because of the protocol), get a new client using i2c_new_device, and register that as a I2C slave client. It then has an address where it listens and an address where it can send to. When to do what is protocol implementation. Am I missing something? Board properties can be encoded within the compatible entries ("ax100-ec", "ax200-ec"...). I'd think this means mostly different protocols, though.
Hi Wolfram, On Fri, Sep 12, 2014 at 11:58 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > 2) Slave mode is needed for board bringup > > Some other components need a specific I2C slave to be present before > userspace is available, otherwise the system is unusable. This is IMO > then a hardware description and justifies DT entries: OK, I didn't think this case, which is similar to an external device requiring a GPIO to be at a specific level. > DT pseudocode: > > i2c { > compatible = "nvidia, tegra-i2c"; > > ec-slave@42 { > compatible = "nvidia, ax100-ec-slave"; > reg = <0x42>; > }; > }; So distinguishing between drivers providing slave services (using slave mode) and drivers driving actual slave hardware (using master mode) is done based on the compatible-property. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > DT pseudocode: > > > > i2c { > > compatible = "nvidia, tegra-i2c"; > > > > ec-slave@42 { > > compatible = "nvidia, ax100-ec-slave"; > > reg = <0x42>; > > }; > > }; > > So distinguishing between drivers providing slave services (using slave > mode) and drivers driving actual slave hardware (using master mode) is > done based on the compatible-property. We describe users of one specific I2C bus. And the drivers that go along with them. No?
Am Freitag, 12. September 2014, 11:58:21 schrieb Wolfram Sang: > > ok, take our embedded controller driver (in staging/nvec) as an example. > > It's basicly an MFD connecting keyboard, mouse, power, gpio, and some > > other stuff to the soc. The MFD operates in master mode while the SOC is > > the I2C slave. Theoretically, these roles could also switch (but that's > > not defined in the nvec protocol). > > I see these cases currently: > > 1) my current case > > The I2C slave is not needed for board bringup, mainly for development or > playing around. It can have this or that functionality on this or that > address. -> does not belong into DT, should be done in userspace > > 2) Slave mode is needed for board bringup > > Some other components need a specific I2C slave to be present before > userspace is available, otherwise the system is unusable. This is IMO > then a hardware description and justifies DT entries: > > DT pseudocode: > > i2c { > compatible = "nvidia, tegra-i2c"; > > ec-slave@42 { > compatible = "nvidia, ax100-ec-slave"; > reg = <0x42>; > }; > }; > > Of course, an MFD driver providing "nvidia, ax100-ec-slave" is needed > which uses the I2C slave mode of the tegra controller. > > 3) Master + slave mode is needed for board bringup: > > Again, IMO a hardware description, so we could use: > > i2c { > compatible = "nvidia, tegra-i2c"; > > ec@64 { > compatible = "nvidia, ax100-ec"; > reg = <0x64>; > }; > }; > > This is a standard I2C device driver (using the MFD framework) where > i2c-tegra would act as a master on the client for 0x64. However, its > probe function can fill an i2c_board_device (the driver should know the > slave device address because of the protocol), get a new client using > i2c_new_device, and register that as a I2C slave client. It then has an > address where it listens and an address where it can send to. When to do > what is protocol implementation. > > Am I missing something? Board properties can be encoded within the > compatible entries ("ax100-ec", "ax200-ec"...). I'd think this means > mostly different protocols, though. well, ac100 is the name of the board and nvec is the name of the protocol. Anyway, I'm fine with encoding the mode to use in the compatible property. Like you said before, a hypothetical driver supporting both modes could also register two compatibility strings (eg. nvidia,nvec and nvidia,nvec-slave) and use the mode (and hence the meaning of the reg value) according to this string. Thanks, Marc
> > Am I missing something? Board properties can be encoded within the > > compatible entries ("ax100-ec", "ax200-ec"...). I'd think this means > > mostly different protocols, though. > > well, ac100 is the name of the board and nvec is the name of the protocol. Yes, the driver could be named nvec, yet the compatible-strings should be similar to above. From that, the driver can deduce the protocol variant and possible initial settings.
diff --git a/arch/arm/boot/dts/r8a7790-lager.dts b/arch/arm/boot/dts/r8a7790-lager.dts index 856b4236b674..12aa2f21e6fa 100644 --- a/arch/arm/boot/dts/r8a7790-lager.dts +++ b/arch/arm/boot/dts/r8a7790-lager.dts @@ -205,9 +205,9 @@ renesas,function = "msiof1"; }; - iic1_pins: iic1 { - renesas,groups = "iic1"; - renesas,function = "iic1"; + i2c1_pins: i2c1 { + renesas,groups = "i2c1"; + renesas,function = "i2c1"; }; iic2_pins: iic2 { @@ -356,10 +356,15 @@ status = "ok"; }; -&iic1 { +&i2c1 { status = "ok"; - pinctrl-0 = <&iic1_pins>; + pinctrl-0 = <&i2c1_pins>; pinctrl-names = "default"; + + eeprom@64 { + compatible = "slave-24c02"; + reg = <0x64>; + }; }; &iic2 {