Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Marc Dietrich]

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

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2343 bytes --]

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


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]


> 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.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Marc Dietrich]

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 751 bytes --]

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?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Marc Dietrich]

[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Geert Uytterhoeven]

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

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1395 bytes --]


> > 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


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Marc Dietrich]

[-- Attachment #1: Type: text/plain, Size: 2785 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2024 bytes --]


> 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.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Geert Uytterhoeven]

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

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]


> > 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?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Marc Dietrich]

[-- Attachment #1: Type: text/plain, Size: 2542 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]


> > 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.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 4/4] ARM: shmobile: r8a7790: adapt DTS for I2C slave support

[Marc Dietrich]

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]