I2C adapters protocol deviation

I2C adapters protocol deviation

[Maxime Ripard]

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

Hi Wolfram,

On the Allwinner A31, the PMIC communicates with the SoC through a bus
looking quite similar to I2C, while being pretty different.

The communication starts with the PMIC through the regular I2C
protocol, but it's only used to initialize the PMIC, and switch to a
mode called "Push/Pull 2 Wire Interface".

That bus is using SDA and SCL, with the start and stop conditions
exactly like I2C does, but:
  - Once the start condition has been issued, the address isn't sent,
    only a direction bit. Hence, it does not support multiple devices
    anymore.
  - Once that direction bit has been sent, the master sends the
    register it wants to read from/write to, over 8 bits, followed by
    one parity bit.
  - Whenever you're writing, the master then sends the data over 8
    bits, followed once again by a parity bit. Then, and only then, an
    ACK is issued by the slave.
  - Whenever you're reading, the master then clocks SDL and the slave
    drives SDA for 8 bits plus 1 parity bit. If there was some kind of
    error, the slave will pull SDA up for 9 cycles, resulting in a
    parity error. Like with I2C though, since it is the only and last
    byte it's receiving, the master won't issue an ACK.

Obviously, to go ahead with the PMIC support, we need to support this
controller and bus first. I can't really decide whether it's within
the scope of the allowed protocol deviations of I2C or if we should
create a whole new bus for it.

Since it's quite similar to and starts as an I2C bus, I'd go for the
former, but I'm really not definitive about it, and wanted to have
your feedback.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: I2C adapters protocol deviation

[Hans de Goede]

Hi,

On 04/03/2014 04:55 PM, Maxime Ripard wrote:
> Hi Wolfram,
> 
> On the Allwinner A31, the PMIC communicates with the SoC through a bus
> looking quite similar to I2C, while being pretty different.
> 
> The communication starts with the PMIC through the regular I2C
> protocol, but it's only used to initialize the PMIC, and switch to a
> mode called "Push/Pull 2 Wire Interface".
> 
> That bus is using SDA and SCL, with the start and stop conditions
> exactly like I2C does, but:
>   - Once the start condition has been issued, the address isn't sent,
>     only a direction bit. Hence, it does not support multiple devices
>     anymore.
>   - Once that direction bit has been sent, the master sends the
>     register it wants to read from/write to, over 8 bits, followed by
>     one parity bit.
>   - Whenever you're writing, the master then sends the data over 8
>     bits, followed once again by a parity bit. Then, and only then, an
>     ACK is issued by the slave.
>   - Whenever you're reading, the master then clocks SDL and the slave
>     drives SDA for 8 bits plus 1 parity bit. If there was some kind of
>     error, the slave will pull SDA up for 9 cycles, resulting in a
>     parity error. Like with I2C though, since it is the only and last
>     byte it's receiving, the master won't issue an ACK.
> 
> Obviously, to go ahead with the PMIC support, we need to support this
> controller and bus first. I can't really decide whether it's within
> the scope of the allowed protocol deviations of I2C or if we should
> create a whole new bus for it.

I've been thinking about this too. In the mean time I have slept quite a
few nights on this now and swung my opinion from use i2c subsys to do our
own bus and back again. But since I've actually added support for this
to u-boot and I now know the device better I believe using the i2c
subsys is the best solution for this.

The PMIC does seem to start in i2c, and then gets switched to this new
push-pull 2 wire i2c-ish protocol on init. So it does have a slave address,
but that is used only once, and then the bus is for a single device only.

Note there seems to be no way to use the host in a regular i2c mode. It has
a setup mode where it does a single regular i2c write (or so I believe) and
then it is in its own custom mode from there on.

So what we really have is a single slave i2c host sort of. At least
we could model it like that. The host could be told which address to
listen to (and which single i2c write to do to init the pmic) through
devicetree and then all the differences would be hidden in the host
driver, ie we would check the slave-address and if it is not the single
one we support, we just return the appropriate error for a device not
acking, and everything should work as a regular i2c host which
only supports i2c_smbus_read_byte and i2c_smbus_write_byte.

IOW the i2c_algorithm struct for the host would not set master_xfer,
only smbus_xfer and its functionality function would return
I2C_FUNC_SMBUS_BYTE

Regards,

Hans

Re: I2C adapters protocol deviation

[Maxime Ripard]

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

On Thu, Apr 03, 2014 at 05:30:09PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 04/03/2014 04:55 PM, Maxime Ripard wrote:
> > Hi Wolfram,
> > 
> > On the Allwinner A31, the PMIC communicates with the SoC through a bus
> > looking quite similar to I2C, while being pretty different.
> > 
> > The communication starts with the PMIC through the regular I2C
> > protocol, but it's only used to initialize the PMIC, and switch to a
> > mode called "Push/Pull 2 Wire Interface".
> > 
> > That bus is using SDA and SCL, with the start and stop conditions
> > exactly like I2C does, but:
> >   - Once the start condition has been issued, the address isn't sent,
> >     only a direction bit. Hence, it does not support multiple devices
> >     anymore.
> >   - Once that direction bit has been sent, the master sends the
> >     register it wants to read from/write to, over 8 bits, followed by
> >     one parity bit.
> >   - Whenever you're writing, the master then sends the data over 8
> >     bits, followed once again by a parity bit. Then, and only then, an
> >     ACK is issued by the slave.
> >   - Whenever you're reading, the master then clocks SDL and the slave
> >     drives SDA for 8 bits plus 1 parity bit. If there was some kind of
> >     error, the slave will pull SDA up for 9 cycles, resulting in a
> >     parity error. Like with I2C though, since it is the only and last
> >     byte it's receiving, the master won't issue an ACK.
> > 
> > Obviously, to go ahead with the PMIC support, we need to support this
> > controller and bus first. I can't really decide whether it's within
> > the scope of the allowed protocol deviations of I2C or if we should
> > create a whole new bus for it.
> 
> I've been thinking about this too. In the mean time I have slept quite a
> few nights on this now and swung my opinion from use i2c subsys to do our
> own bus and back again. But since I've actually added support for this
> to u-boot and I now know the device better I believe using the i2c
> subsys is the best solution for this.
> 
> The PMIC does seem to start in i2c, and then gets switched to this new
> push-pull 2 wire i2c-ish protocol on init. So it does have a slave address,
> but that is used only once, and then the bus is for a single device only.
> 
> Note there seems to be no way to use the host in a regular i2c mode. It has
> a setup mode where it does a single regular i2c write (or so I believe) and
> then it is in its own custom mode from there on.

Well, in theory, you could. It has a register where you can control
the levels of SDA and SCL, so you could bitbang the bus using
them. But I'm not sure we want to do that :)

> So what we really have is a single slave i2c host sort of. At least
> we could model it like that. The host could be told which address to
> listen to (and which single i2c write to do to init the pmic) through
> devicetree and then all the differences would be hidden in the host
> driver, ie we would check the slave-address and if it is not the single
> one we support, we just return the appropriate error for a device not
> acking, and everything should work as a regular i2c host which
> only supports i2c_smbus_read_byte and i2c_smbus_write_byte.
> 
> IOW the i2c_algorithm struct for the host would not set master_xfer,
> only smbus_xfer and its functionality function would return
> I2C_FUNC_SMBUS_BYTE

Yep, it would make sense.

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: I2C adapters protocol deviation

[Wolfram Sang]

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


> So what we really have is a single slave i2c host sort of. At least
> we could model it like that. The host could be told which address to
> listen to (and which single i2c write to do to init the pmic) through
> devicetree and then all the differences would be hidden in the host
> driver, ie we would check the slave-address and if it is not the single
> one we support, we just return the appropriate error for a device not
> acking, and everything should work as a regular i2c host which
> only supports i2c_smbus_read_byte and i2c_smbus_write_byte.

I'd think we need a new message flag like I2C_M_PUSHPULL which says that
this message has only the direction bit instead of the address and needs
a parity bit afterwards. In addition to that, we need a new
functionality flag I2C_FUNC_PUSHPULL which means the host driver can
handle those messages. So, the PMIC driver could query support for
I2C_FUNC_SMBUS_BYTE | I2C_FUNC_PUSHPULL and if successful send messages
using smbus functions with the new flag set.

Not sure about the I2C-to-PushPull switch: Is it 100% host configuration
or does it also depend on the one slave attached? Are there some
datasheets available?


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

Re: I2C adapters protocol deviation

[Hans de Goede]

Hi,

On 04/04/2014 02:26 PM, Wolfram Sang wrote:
> 
>> So what we really have is a single slave i2c host sort of. At least
>> we could model it like that. The host could be told which address to
>> listen to (and which single i2c write to do to init the pmic) through
>> devicetree and then all the differences would be hidden in the host
>> driver, ie we would check the slave-address and if it is not the single
>> one we support, we just return the appropriate error for a device not
>> acking, and everything should work as a regular i2c host which
>> only supports i2c_smbus_read_byte and i2c_smbus_write_byte.
> 
> I'd think we need a new message flag like I2C_M_PUSHPULL which says that
> this message has only the direction bit instead of the address and needs
> a parity bit afterwards. In addition to that, we need a new
> functionality flag I2C_FUNC_PUSHPULL which means the host driver can
> handle those messages. So, the PMIC driver could query support for
> I2C_FUNC_SMBUS_BYTE | I2C_FUNC_PUSHPULL and if successful send messages
> using smbus functions with the new flag set.

Thanks for the input this sounds good, I guess we'll give this a shot
when we get around to coding up support for the p2wi block in the A31.

> Not sure about the I2C-to-PushPull switch: Is it 100% host configuration
> or does it also depend on the one slave attached? 

The datasheet we've suggests that it actually influences the one slave
attached. Note that u-boot on this machines will likely already have made
the switch, but I guess we don't want to count on that.

> Are there some datasheets available?

The AXP221 is documented here:
http://linux-sunxi.org/AXP221
This is translated by one of our community members from a Chinese datasheet.

The P2WI interface is (somewhat) documented in the A31 datasheet, but we cannot
share that in public.

Regards,

Hans

Re: I2C adapters protocol deviation

[Wolfram Sang]

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

On Sun, Apr 06, 2014 at 04:01:52PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 04/04/2014 02:26 PM, Wolfram Sang wrote:
> > 
> >> So what we really have is a single slave i2c host sort of. At least
> >> we could model it like that. The host could be told which address to
> >> listen to (and which single i2c write to do to init the pmic) through
> >> devicetree and then all the differences would be hidden in the host
> >> driver, ie we would check the slave-address and if it is not the single
> >> one we support, we just return the appropriate error for a device not
> >> acking, and everything should work as a regular i2c host which
> >> only supports i2c_smbus_read_byte and i2c_smbus_write_byte.
> > 
> > I'd think we need a new message flag like I2C_M_PUSHPULL which says that
> > this message has only the direction bit instead of the address and needs
> > a parity bit afterwards. In addition to that, we need a new
> > functionality flag I2C_FUNC_PUSHPULL which means the host driver can
> > handle those messages. So, the PMIC driver could query support for
> > I2C_FUNC_SMBUS_BYTE | I2C_FUNC_PUSHPULL and if successful send messages
> > using smbus functions with the new flag set.
> 
> Thanks for the input this sounds good, I guess we'll give this a shot
> when we get around to coding up support for the p2wi block in the A31.

On a second thought, maybe more granularity is better. Like using
I2C_M_DROP_ADDRESS and I2C_M_ADD_PARITY and then make
I2C_CLIENT_PUSHPULL involve I2C_M_DROP_ADDRESS | I2C_M_ADD_PARITY.

> > Not sure about the I2C-to-PushPull switch: Is it 100% host configuration
> > or does it also depend on the one slave attached? 
> 
> The datasheet we've suggests that it actually influences the one slave
> attached. Note that u-boot on this machines will likely already have made
> the switch, but I guess we don't want to count on that.

Can we detect if this switching was already made?

> > Are there some datasheets available?
> 
> The AXP221 is documented here:
> http://linux-sunxi.org/AXP221
> This is translated by one of our community members from a Chinese datasheet.
> 
> The P2WI interface is (somewhat) documented in the A31 datasheet, but we cannot
> share that in public.

Any chance for me to get it if I sign something?

Regards,

   Wolfram


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

Re: I2C adapters protocol deviation

[Hans de Goede]

Hi,

On 04/06/2014 05:37 PM, Wolfram Sang wrote:
> On Sun, Apr 06, 2014 at 04:01:52PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 04/04/2014 02:26 PM, Wolfram Sang wrote:
>>>
>>>> So what we really have is a single slave i2c host sort of. At least
>>>> we could model it like that. The host could be told which address to
>>>> listen to (and which single i2c write to do to init the pmic) through
>>>> devicetree and then all the differences would be hidden in the host
>>>> driver, ie we would check the slave-address and if it is not the single
>>>> one we support, we just return the appropriate error for a device not
>>>> acking, and everything should work as a regular i2c host which
>>>> only supports i2c_smbus_read_byte and i2c_smbus_write_byte.
>>>
>>> I'd think we need a new message flag like I2C_M_PUSHPULL which says that
>>> this message has only the direction bit instead of the address and needs
>>> a parity bit afterwards. In addition to that, we need a new
>>> functionality flag I2C_FUNC_PUSHPULL which means the host driver can
>>> handle those messages. So, the PMIC driver could query support for
>>> I2C_FUNC_SMBUS_BYTE | I2C_FUNC_PUSHPULL and if successful send messages
>>> using smbus functions with the new flag set.
>>
>> Thanks for the input this sounds good, I guess we'll give this a shot
>> when we get around to coding up support for the p2wi block in the A31.
> 
> On a second thought, maybe more granularity is better. Like using
> I2C_M_DROP_ADDRESS and I2C_M_ADD_PARITY and then make
> I2C_CLIENT_PUSHPULL involve I2C_M_DROP_ADDRESS | I2C_M_ADD_PARITY.

Hmm, I'm not completely sold on the whole idea of having special
flags, esp. since it seems that ie the AXP221 may operate in normal
i2c mode in some designs too. So ideally we would just hide from
clients that this is something else then plain i2c. So that we can have
an axp221 driver which is not even aware about this weird i2c-variant and
will just work independent on how the axp221 is hooked up.

Likewise it would be useful to have the i2cdump utility just work, etc.

So maybe a flag which is a hint that this is special on the controller,
but I don't think we should be checking for special flags in the messages
on the controller side. Basically the whole p2wi allows reading / writing
byte registers, so I2C_FUNC_SMBUS_BYTE is a 1:1 mapping of the functionality,
as for the address, we can just check it is the one address used to do
the initial setup, and if it is not then just return an error.

>>> Not sure about the I2C-to-PushPull switch: Is it 100% host configuration
>>> or does it also depend on the one slave attached? 
>>
>> The datasheet we've suggests that it actually influences the one slave
>> attached. Note that u-boot on this machines will likely already have made
>> the switch, but I guess we don't want to count on that.
> 
> Can we detect if this switching was already made?

I don't think we can. But I think doing the switch a second time is ok /
does not result in an error.

Regards,

Hans

Re: I2C adapters protocol deviation

[Boris BREZILLON]

Hi,

On 06/04/2014 19:18, Hans de Goede wrote:
> Hi,
>
> On 04/06/2014 05:37 PM, Wolfram Sang wrote:
>> On Sun, Apr 06, 2014 at 04:01:52PM +0200, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 04/04/2014 02:26 PM, Wolfram Sang wrote:
>>>>> So what we really have is a single slave i2c host sort of. At least
>>>>> we could model it like that. The host could be told which address to
>>>>> listen to (and which single i2c write to do to init the pmic) through
>>>>> devicetree and then all the differences would be hidden in the host
>>>>> driver, ie we would check the slave-address and if it is not the single
>>>>> one we support, we just return the appropriate error for a device not
>>>>> acking, and everything should work as a regular i2c host which
>>>>> only supports i2c_smbus_read_byte and i2c_smbus_write_byte.
>>>> I'd think we need a new message flag like I2C_M_PUSHPULL which says that
>>>> this message has only the direction bit instead of the address and needs
>>>> a parity bit afterwards. In addition to that, we need a new
>>>> functionality flag I2C_FUNC_PUSHPULL which means the host driver can
>>>> handle those messages. So, the PMIC driver could query support for
>>>> I2C_FUNC_SMBUS_BYTE | I2C_FUNC_PUSHPULL and if successful send messages
>>>> using smbus functions with the new flag set.
>>> Thanks for the input this sounds good, I guess we'll give this a shot
>>> when we get around to coding up support for the p2wi block in the A31.

Hans, I started to work on the p2wi driver, but let me know if you
already have a working driver (I'll either rebase my work on yours or
let you finish your implementation).

>> On a second thought, maybe more granularity is better. Like using
>> I2C_M_DROP_ADDRESS and I2C_M_ADD_PARITY and then make
>> I2C_CLIENT_PUSHPULL involve I2C_M_DROP_ADDRESS | I2C_M_ADD_PARITY.

For the moment I only make use of the I2C_CLIENT_PUSHPULL and
I2C_M_PUSHPULL flags, but I can add those flags (and check them in the
smbus_xfer callback) if you think this is relevant.

> Hmm, I'm not completely sold on the whole idea of having special
> flags, esp. since it seems that ie the AXP221 may operate in normal
> i2c mode in some designs too. So ideally we would just hide from
> clients that this is something else then plain i2c. So that we can have
> an axp221 driver which is not even aware about this weird i2c-variant and
> will just work independent on how the axp221 is hooked up.

Hans, can't we use the same PMIC driver but with different i2c flags for
the p2wi case.

IMHO, the real question here is: do we want to support the p2wi protocol
on other i2c bus implementations (like the i2c bitbanging driver) ?
If this is the case we definitely need those specific flags.

> Likewise it would be useful to have the i2cdump utility just work, etc.
>
> So maybe a flag which is a hint that this is special on the controller,
> but I don't think we should be checking for special flags in the messages
> on the controller side. Basically the whole p2wi allows reading / writing
> byte registers, so I2C_FUNC_SMBUS_BYTE is a 1:1 mapping of the functionality,
> as for the address, we can just check it is the one address used to do
> the initial setup, and if it is not then just return an error.
>
>>>> Not sure about the I2C-to-PushPull switch: Is it 100% host configuration
>>>> or does it also depend on the one slave attached? 
>>> The datasheet we've suggests that it actually influences the one slave
>>> attached. Note that u-boot on this machines will likely already have made
>>> the switch, but I guess we don't want to count on that.
>> Can we detect if this switching was already made?
> I don't think we can. But I think doing the switch a second time is ok /
> does not result in an error.

AFAICT you cannot detect if the i2c to p2wi switch has already taken
place, and re launching the switch process might have unexpected effects:
let's say the i2c address you're trying to access maps to a valid PMIC
register, as the address should be dropped in p2wi mode, you might write
an erroneous value to this register.


Best Regards,

Boris
> Regards,
>
> Hans

Re: I2C adapters protocol deviation

[Maxime Ripard]

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

On Sun, Apr 06, 2014 at 05:37:29PM +0200, Wolfram Sang wrote:
> On Sun, Apr 06, 2014 at 04:01:52PM +0200, Hans de Goede wrote:
> > Hi,
> > 
> > On 04/04/2014 02:26 PM, Wolfram Sang wrote:
> > > 
> > >> So what we really have is a single slave i2c host sort of. At least
> > >> we could model it like that. The host could be told which address to
> > >> listen to (and which single i2c write to do to init the pmic) through
> > >> devicetree and then all the differences would be hidden in the host
> > >> driver, ie we would check the slave-address and if it is not the single
> > >> one we support, we just return the appropriate error for a device not
> > >> acking, and everything should work as a regular i2c host which
> > >> only supports i2c_smbus_read_byte and i2c_smbus_write_byte.
> > > 
> > > I'd think we need a new message flag like I2C_M_PUSHPULL which says that
> > > this message has only the direction bit instead of the address and needs
> > > a parity bit afterwards. In addition to that, we need a new
> > > functionality flag I2C_FUNC_PUSHPULL which means the host driver can
> > > handle those messages. So, the PMIC driver could query support for
> > > I2C_FUNC_SMBUS_BYTE | I2C_FUNC_PUSHPULL and if successful send messages
> > > using smbus functions with the new flag set.
> > 
> > Thanks for the input this sounds good, I guess we'll give this a shot
> > when we get around to coding up support for the p2wi block in the A31.
> 
> On a second thought, maybe more granularity is better. Like using
> I2C_M_DROP_ADDRESS and I2C_M_ADD_PARITY and then make
> I2C_CLIENT_PUSHPULL involve I2C_M_DROP_ADDRESS | I2C_M_ADD_PARITY.

I'd agree with that. Other clients/adapters might need only one of
these. Note that you'd probably need a I2C_M_DELAYED_ACK or something.

> > > Not sure about the I2C-to-PushPull switch: Is it 100% host configuration
> > > or does it also depend on the one slave attached? 
> > 
> > The datasheet we've suggests that it actually influences the one slave
> > attached. Note that u-boot on this machines will likely already have made
> > the switch, but I guess we don't want to count on that.
> 
> Can we detect if this switching was already made?

None that we're aware of. Since the PMIC is already most likely
initialised, I think we can just use it as if it was already in P2WI
mode.

> > > Are there some datasheets available?
> > 
> > The AXP221 is documented here:
> > http://linux-sunxi.org/AXP221
> > This is translated by one of our community members from a Chinese datasheet.
> > 
> > The P2WI interface is (somewhat) documented in the A31 datasheet, but we cannot
> > share that in public.
> 
> Any chance for me to get it if I sign something?

Let me see what I can do.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: I2C adapters protocol deviation

[Maxime Ripard]

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

On Sun, Apr 06, 2014 at 07:18:25PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 04/06/2014 05:37 PM, Wolfram Sang wrote:
> > On Sun, Apr 06, 2014 at 04:01:52PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 04/04/2014 02:26 PM, Wolfram Sang wrote:
> >>>
> >>>> So what we really have is a single slave i2c host sort of. At least
> >>>> we could model it like that. The host could be told which address to
> >>>> listen to (and which single i2c write to do to init the pmic) through
> >>>> devicetree and then all the differences would be hidden in the host
> >>>> driver, ie we would check the slave-address and if it is not the single
> >>>> one we support, we just return the appropriate error for a device not
> >>>> acking, and everything should work as a regular i2c host which
> >>>> only supports i2c_smbus_read_byte and i2c_smbus_write_byte.
> >>>
> >>> I'd think we need a new message flag like I2C_M_PUSHPULL which says that
> >>> this message has only the direction bit instead of the address and needs
> >>> a parity bit afterwards. In addition to that, we need a new
> >>> functionality flag I2C_FUNC_PUSHPULL which means the host driver can
> >>> handle those messages. So, the PMIC driver could query support for
> >>> I2C_FUNC_SMBUS_BYTE | I2C_FUNC_PUSHPULL and if successful send messages
> >>> using smbus functions with the new flag set.
> >>
> >> Thanks for the input this sounds good, I guess we'll give this a shot
> >> when we get around to coding up support for the p2wi block in the A31.
> > 
> > On a second thought, maybe more granularity is better. Like using
> > I2C_M_DROP_ADDRESS and I2C_M_ADD_PARITY and then make
> > I2C_CLIENT_PUSHPULL involve I2C_M_DROP_ADDRESS | I2C_M_ADD_PARITY.
> 
> Hmm, I'm not completely sold on the whole idea of having special
> flags, esp. since it seems that ie the AXP221 may operate in normal
> i2c mode in some designs too. So ideally we would just hide from
> clients that this is something else then plain i2c. So that we can have
> an axp221 driver which is not even aware about this weird i2c-variant and
> will just work independent on how the axp221 is hooked up.

I don't think we actually saw in real life an AXP221 connected only
using i2c. I'd say we shouldn't worry too much about a theorical
corner case that we never saw, until we actually see it.

> Likewise it would be useful to have the i2cdump utility just work, etc.

I'm not sure I want the i2c-tools to start poking around the PMIC.

> So maybe a flag which is a hint that this is special on the controller,
> but I don't think we should be checking for special flags in the messages
> on the controller side. Basically the whole p2wi allows reading / writing
> byte registers, so I2C_FUNC_SMBUS_BYTE is a 1:1 mapping of the functionality,
> as for the address, we can just check it is the one address used to do
> the initial setup, and if it is not then just return an error.

Yes, we obviously have to check for the address in the xfer function.

> >>> Not sure about the I2C-to-PushPull switch: Is it 100% host configuration
> >>> or does it also depend on the one slave attached? 
> >>
> >> The datasheet we've suggests that it actually influences the one slave
> >> attached. Note that u-boot on this machines will likely already have made
> >> the switch, but I guess we don't want to count on that.
> > 
> > Can we detect if this switching was already made?
> 
> I don't think we can. But I think doing the switch a second time is ok /
> does not result in an error.

And it will probably mangle the PMIC configuration. I'm not very
comfortable with that..

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: I2C adapters protocol deviation

[Hans de Goede]

Hi,

On 04/07/2014 09:49 AM, Boris BREZILLON wrote:
> Hi,
> 
> On 06/04/2014 19:18, Hans de Goede wrote:
>> Hi,
>>
>> On 04/06/2014 05:37 PM, Wolfram Sang wrote:
>>> On Sun, Apr 06, 2014 at 04:01:52PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 04/04/2014 02:26 PM, Wolfram Sang wrote:
>>>>>> So what we really have is a single slave i2c host sort of. At least
>>>>>> we could model it like that. The host could be told which address to
>>>>>> listen to (and which single i2c write to do to init the pmic) through
>>>>>> devicetree and then all the differences would be hidden in the host
>>>>>> driver, ie we would check the slave-address and if it is not the single
>>>>>> one we support, we just return the appropriate error for a device not
>>>>>> acking, and everything should work as a regular i2c host which
>>>>>> only supports i2c_smbus_read_byte and i2c_smbus_write_byte.
>>>>> I'd think we need a new message flag like I2C_M_PUSHPULL which says that
>>>>> this message has only the direction bit instead of the address and needs
>>>>> a parity bit afterwards. In addition to that, we need a new
>>>>> functionality flag I2C_FUNC_PUSHPULL which means the host driver can
>>>>> handle those messages. So, the PMIC driver could query support for
>>>>> I2C_FUNC_SMBUS_BYTE | I2C_FUNC_PUSHPULL and if successful send messages
>>>>> using smbus functions with the new flag set.
>>>> Thanks for the input this sounds good, I guess we'll give this a shot
>>>> when we get around to coding up support for the p2wi block in the A31.
> 
> Hans, I started to work on the p2wi driver, but let me know if you
> already have a working driver (I'll either rebase my work on yours or
> let you finish your implementation).

I've pushed a working (polling, no irqs) implementation to the u-boot-sunxi
repository. Other then that I've no code, so if you're working on a kernel
driver that is great!

> 
>>> On a second thought, maybe more granularity is better. Like using
>>> I2C_M_DROP_ADDRESS and I2C_M_ADD_PARITY and then make
>>> I2C_CLIENT_PUSHPULL involve I2C_M_DROP_ADDRESS | I2C_M_ADD_PARITY.
> 
> For the moment I only make use of the I2C_CLIENT_PUSHPULL and
> I2C_M_PUSHPULL flags, but I can add those flags (and check them in the
> smbus_xfer callback) if you think this is relevant.
> 
>> Hmm, I'm not completely sold on the whole idea of having special
>> flags, esp. since it seems that ie the AXP221 may operate in normal
>> i2c mode in some designs too. So ideally we would just hide from
>> clients that this is something else then plain i2c. So that we can have
>> an axp221 driver which is not even aware about this weird i2c-variant and
>> will just work independent on how the axp221 is hooked up.
> 
> Hans, can't we use the same PMIC driver but with different i2c flags for
> the p2wi case.

Sure, but that would complicate the PMIC code a bit. Anyways just do what
you think is best and we'll see from there.

> IMHO, the real question here is: do we want to support the p2wi protocol
> on other i2c bus implementations (like the i2c bitbanging driver) ?
> If this is the case we definitely need those specific flags.

I don't think so. This whole p2wi thing seems like a one-off experiment by
Allwinner they have already abandoned it for their next SOC.

>> Likewise it would be useful to have the i2cdump utility just work, etc.
>>
>> So maybe a flag which is a hint that this is special on the controller,
>> but I don't think we should be checking for special flags in the messages
>> on the controller side. Basically the whole p2wi allows reading / writing
>> byte registers, so I2C_FUNC_SMBUS_BYTE is a 1:1 mapping of the functionality,
>> as for the address, we can just check it is the one address used to do
>> the initial setup, and if it is not then just return an error.
>>
>>>>> Not sure about the I2C-to-PushPull switch: Is it 100% host configuration
>>>>> or does it also depend on the one slave attached? 
>>>> The datasheet we've suggests that it actually influences the one slave
>>>> attached. Note that u-boot on this machines will likely already have made
>>>> the switch, but I guess we don't want to count on that.
>>> Can we detect if this switching was already made?
>> I don't think we can. But I think doing the switch a second time is ok /
>> does not result in an error.
> 
> AFAICT you cannot detect if the i2c to p2wi switch has already taken
> place, and re launching the switch process might have unexpected effects:
> let's say the i2c address you're trying to access maps to a valid PMIC
> register, as the address should be dropped in p2wi mode, you might write
> an erroneous value to this register.

I think / expect that this is taken care of somehow at the hardware level.
IE if the SOC gets reset through the watchdog, the PMIC won't know this and
the bootloader will re-init the P2WI telling it to do the switch. So I think
that doing the switch twice will be harmless. Actually doing the switch twice
is exactly what my current u-boot code does more or less. For now we boot the
A31 using Allwinner's boot0 + boot1 bootloader which then chain-loads u-boot,
and boot0/boot1 already talk to the PMIC, so has already made the switch, and
my u-boot code switches it over again.

Regards,

Hans

Re: I2C adapters protocol deviation

[Hans de Goede]

Hi,

On 04/07/2014 10:15 AM, Maxime Ripard wrote:
> On Sun, Apr 06, 2014 at 07:18:25PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 04/06/2014 05:37 PM, Wolfram Sang wrote:
>>> On Sun, Apr 06, 2014 at 04:01:52PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 04/04/2014 02:26 PM, Wolfram Sang wrote:
>>>>>
>>>>>> So what we really have is a single slave i2c host sort of. At least
>>>>>> we could model it like that. The host could be told which address to
>>>>>> listen to (and which single i2c write to do to init the pmic) through
>>>>>> devicetree and then all the differences would be hidden in the host
>>>>>> driver, ie we would check the slave-address and if it is not the single
>>>>>> one we support, we just return the appropriate error for a device not
>>>>>> acking, and everything should work as a regular i2c host which
>>>>>> only supports i2c_smbus_read_byte and i2c_smbus_write_byte.
>>>>>
>>>>> I'd think we need a new message flag like I2C_M_PUSHPULL which says that
>>>>> this message has only the direction bit instead of the address and needs
>>>>> a parity bit afterwards. In addition to that, we need a new
>>>>> functionality flag I2C_FUNC_PUSHPULL which means the host driver can
>>>>> handle those messages. So, the PMIC driver could query support for
>>>>> I2C_FUNC_SMBUS_BYTE | I2C_FUNC_PUSHPULL and if successful send messages
>>>>> using smbus functions with the new flag set.
>>>>
>>>> Thanks for the input this sounds good, I guess we'll give this a shot
>>>> when we get around to coding up support for the p2wi block in the A31.
>>>
>>> On a second thought, maybe more granularity is better. Like using
>>> I2C_M_DROP_ADDRESS and I2C_M_ADD_PARITY and then make
>>> I2C_CLIENT_PUSHPULL involve I2C_M_DROP_ADDRESS | I2C_M_ADD_PARITY.
>>
>> Hmm, I'm not completely sold on the whole idea of having special
>> flags, esp. since it seems that ie the AXP221 may operate in normal
>> i2c mode in some designs too. So ideally we would just hide from
>> clients that this is something else then plain i2c. So that we can have
>> an axp221 driver which is not even aware about this weird i2c-variant and
>> will just work independent on how the axp221 is hooked up.
> 
> I don't think we actually saw in real life an AXP221 connected only
> using i2c. I'd say we shouldn't worry too much about a theorical
> corner case that we never saw, until we actually see it.
> 
>> Likewise it would be useful to have the i2cdump utility just work, etc.
> 
> I'm not sure I want the i2c-tools to start poking around the PMIC.

Having i2cdump is very very useful in my experience with developing
i2c hwmon drivers.

> 
>> So maybe a flag which is a hint that this is special on the controller,
>> but I don't think we should be checking for special flags in the messages
>> on the controller side. Basically the whole p2wi allows reading / writing
>> byte registers, so I2C_FUNC_SMBUS_BYTE is a 1:1 mapping of the functionality,
>> as for the address, we can just check it is the one address used to do
>> the initial setup, and if it is not then just return an error.
> 
> Yes, we obviously have to check for the address in the xfer function.
> 
>>>>> Not sure about the I2C-to-PushPull switch: Is it 100% host configuration
>>>>> or does it also depend on the one slave attached? 
>>>>
>>>> The datasheet we've suggests that it actually influences the one slave
>>>> attached. Note that u-boot on this machines will likely already have made
>>>> the switch, but I guess we don't want to count on that.
>>>
>>> Can we detect if this switching was already made?
>>
>> I don't think we can. But I think doing the switch a second time is ok /
>> does not result in an error.
> 
> And it will probably mangle the PMIC configuration. I'm not very
> comfortable with that..

I think / expect that this is taken care of somehow at the hardware level.
IE if the SOC gets reset through the watchdog, the PMIC won't know this and
the bootloader will re-init the P2WI telling it to do the switch. So I think
that doing the switch twice will be harmless. Actually doing the switch twice
is exactly what my current u-boot code does more or less. For now we boot the
A31 using Allwinner's boot0 + boot1 bootloader which then chain-loads u-boot,
and boot0/boot1 already talk to the PMIC, so has already made the switch, and
my u-boot code switches it over again.

Regards,

Hans