Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined?

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined?

Lee D
Hi Everyone,

I am back to working on my Zynq I2C and M41T82 RTC chip drivers.  I am
still using 11.0.1.

It turns out that the Zynq I2C hardware is buggy and it doesn't really
fit in with the FreeBSD paradigm of issuing discrete bus transactions
(start, stop, etc.)

I am trying to work around this by writing my own version of
iicbus_transfer_gen(), copied from src/sys/dev/iicbus/iiconf.c

My question is, where is iicbus_get_nostop() defined?  I can't seem to
find it with grep.

"nostop" seems to get turned on at some point, and while I could just
ignore it, I'd like to know where and how it is happening.

Thanks,

Lee
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined?

Ian Lepore-3
On Sun, 2018-03-18 at 00:46 -0400, Lee D wrote:

> Hi Everyone,
>
> I am back to working on my Zynq I2C and M41T82 RTC chip drivers.  I am
> still using 11.0.1.
>
> It turns out that the Zynq I2C hardware is buggy and it doesn't really
> fit in with the FreeBSD paradigm of issuing discrete bus transactions
> (start, stop, etc.)
>
> I am trying to work around this by writing my own version of
> iicbus_transfer_gen(), copied from src/sys/dev/iicbus/iiconf.c
>
> My question is, where is iicbus_get_nostop() defined?  I can't seem to
> find it with grep.
>
> "nostop" seems to get turned on at some point, and while I could just
> ignore it, I'd like to know where and how it is happening.
>
> Thanks,
>
> Lee

Nostop is an ivar of the child device, so iicbus_get_nostop() is formed
by the IICBUS_ACCESSOR macro in iicbus.h.

Now for the bad news:  don't use it.  It doesn't work.  It's 100% a bug
in the code that maybe kinda-sorta seemed to work for whoever added it,
because accidentally the right garbage was on the stack in the local
nostop var.  The generic transfer code doesn't check that the accessor
failed so it ends up using stack garbage for nostop.  The reason
there's g'teed to be no such ivar is because the code is asking the
wrong device, and it doesn't even have a handle to the right child
device to get the info it wants.

So the bottom line is, write your transfer routine to work however it
has to work.  That might mean ignoring the stop/nostop flags and just
doing whatever your hardware does.  Like if a write transaction is
handled by the hardware by putting the slave address and the offse-
within-slave values into registers and it does a write of the offset
then a read from the slave and you get no control over whether it does
that as two transactions or as 1 transaction with a repeat-start
between the read and write, then just silently do it that way.

I had forgotten about the nostop mess, which I discovered some time
last year.  It really needs to be fixed the right way, which is to
remove the nostop hack, remove all uses of the NOSTOP flag everywhere
in the code, and make the default behavior that all back-to-back
operations in the same array of cmds handed to a single transfer call
have implicit repeat-start behavior when changing slave or direction.
 We could add a specific STOP flag to force a stop/start between two
commands, but even that's not really needed.

The only example of a transfer-only driver I know of offhand is the rpi
driver (arm/broadcom/bcm2835/bcm2835_bsc.c).  Unfortunately, bugs in
the rpi silicon complicate the code and make it a messy example.

-- Ian

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined?

Andriy Gapon
On 18/03/2018 16:30, Ian Lepore wrote:
> Now for the bad news:  don't use it.  It doesn't work.  It's 100% a bug
> in the code that maybe kinda-sorta seemed to work for whoever added it,
> because accidentally the right garbage was on the stack in the local
> nostop var.  The generic transfer code doesn't check that the accessor
> failed so it ends up using stack garbage for nostop.  The reason
> there's g'teed to be no such ivar is because the code is asking the
> wrong device, and it doesn't even have a handle to the right child
> device to get the info it wants.


Oh, indeed.
I think that there never was an intention to make "nostop" a property of an i2c
slave, a child of an iicbus device.  I think that instead it was supposed to be
a property of the iicbus's parent device, an actual i2c adapter driver.
I guess that the reason that "nostop" became an ivar in iicbus was an incorrect
understanding of how a "bit-banger" device (something implementing iicbb_if),
iicbb device and iicbus device are connected.  I think that I was among the
reviewers and I probably had a bit of confusion back then.


It seems that the only place where iicbus_get_nostop() is used is
iicbus_transfer_gen().  iicbus_transfer_gen is used in several i2c drivers as an
iicbus_transfer method.  it's also used in iicbb, thinly wrapped by
iicbb_transfer().
So, iicbus_get_nostop() actually translates to a call to BUS_READ_IVAR(parent,
device, 1, &v) where I already substituted one for IICBUS_IVAR_NOSTOP.
Here we can see that while the accessor functions lok quite nice they get
expanded to generic calls without much context.  So, whether that
BUS_READ_IVAR() call succeeds and what value it gets depends on whether the
parent bus defines an ivar with a value of 1 and what value that ivar might have
for the driver device.  Many buses define at least a couple of ivars.
So, a return value of iicbus_get_nostop() could be consistent for a particular
driver while still being arbitrary.  But it can be a piece of stack garbage too,
just as you said.

The only place where iicbus_set_nostop() is used is intel_iicbb_attach().
In that case the ivar is "set" on a wrong device at all (the bit-banger, not iicbb).


This definitely needs to be fixed / reworked.
Perhaps nostop should become a new interface in iicbus_if and iicbb_if...

--
Andriy Gapon
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined?

Ian Lepore-3
On Fri, 2018-03-23 at 11:56 +0200, Andriy Gapon wrote:

> On 18/03/2018 16:30, Ian Lepore wrote:
> >
> > Now for the bad news:  don't use it.  It doesn't work.  It's 100% a bug
> > in the code that maybe kinda-sorta seemed to work for whoever added it,
> > because accidentally the right garbage was on the stack in the local
> > nostop var.  The generic transfer code doesn't check that the accessor
> > failed so it ends up using stack garbage for nostop.  The reason
> > there's g'teed to be no such ivar is because the code is asking the
> > wrong device, and it doesn't even have a handle to the right child
> > device to get the info it wants.
>
> Oh, indeed.
> I think that there never was an intention to make "nostop" a property of an i2c
> slave, a child of an iicbus device.  I think that instead it was supposed to be
> a property of the iicbus's parent device, an actual i2c adapter driver.
> I guess that the reason that "nostop" became an ivar in iicbus was an incorrect
> understanding of how a "bit-banger" device (something implementing iicbb_if),
> iicbb device and iicbus device are connected.  I think that I was among the
> reviewers and I probably had a bit of confusion back then.
>
>
> It seems that the only place where iicbus_get_nostop() is used is
> iicbus_transfer_gen().  iicbus_transfer_gen is used in several i2c drivers as an
> iicbus_transfer method.  it's also used in iicbb, thinly wrapped by
> iicbb_transfer().
> So, iicbus_get_nostop() actually translates to a call to BUS_READ_IVAR(parent,
> device, 1, &v) where I already substituted one for IICBUS_IVAR_NOSTOP.
> Here we can see that while the accessor functions lok quite nice they get
> expanded to generic calls without much context.  So, whether that
> BUS_READ_IVAR() call succeeds and what value it gets depends on whether the
> parent bus defines an ivar with a value of 1 and what value that ivar might have
> for the driver device.  Many buses define at least a couple of ivars.
> So, a return value of iicbus_get_nostop() could be consistent for a particular
> driver while still being arbitrary.  But it can be a piece of stack garbage too,
> just as you said.
>
> The only place where iicbus_set_nostop() is used is intel_iicbb_attach().
> In that case the ivar is "set" on a wrong device at all (the bit-banger, not iicbb).
>
>
> This definitely needs to be fixed / reworked.
> Perhaps nostop should become a new interface in iicbus_if and iicbb_if...
>

I think our whole interface for transfers is conceptually broken. I
would like to just redefine the behavior of the interface. I think what
I want is basically the same thing that nostop ivar was trying to
achieve:

    Ignore the existing start/stop flags in the transfer structures.
    When you pass an array of transfers, there is one bus START at the
    beginning, one bus STOP at the end, and an automatic REPEAT_START
    between any two transfers in the array where the slave address or
    direction changes. When there are two adjacent transfers in the
    array with the same address and direction, that's just one long
    continuous flow of bytes -- effectively, it's scatter/gather IO.

As an optimization, we could define an IICBUS_STOP flag that could be
added to any transfer in the array to force a full STOP/START sequence
after that transfer and before the next. That would amount to basically
a minor optimization... it would be identical to just calling transfer
twice. So I'm not even sure it's a good idea.

We might need to leave the current broken interface in place for out-
of-tree code like proprietary drivers, and define a new transfer method
that works the new way.

When I started looking at all existing callers of iicbus_transfer() I
came to the conclusion that almost all of them followed such identical
patterns that I wrote the iicdev_readfrom()/writeto() functions. I
figured I would run through the system converting everything I could to
those new functions, then whatever changes need to be made to the
interface would almost all be in just a couple functions. But that
project bogged down then other things came along and I forgot all about
it.

-- Ian
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined?

Andriy Gapon
On 24/03/2018 17:43, Ian Lepore wrote:

> I think our whole interface for transfers is conceptually broken. I
> would like to just redefine the behavior of the interface. I think what
> I want is basically the same thing that nostop ivar was trying to
> achieve:
>
>     Ignore the existing start/stop flags in the transfer structures.
>     When you pass an array of transfers, there is one bus START at the
>     beginning, one bus STOP at the end, and an automatic REPEAT_START
>     between any two transfers in the array where the slave address or
>     direction changes. When there are two adjacent transfers in the
>     array with the same address and direction, that's just one long
>     continuous flow of bytes -- effectively, it's scatter/gather IO.

I completely agree.  If you have a transfer / transaction, then why have
STOP+START in the middle it.  The current default behavior just does not make
sense to me.

> As an optimization, we could define an IICBUS_STOP flag that could be
> added to any transfer in the array to force a full STOP/START sequence
> after that transfer and before the next. That would amount to basically
> a minor optimization... it would be identical to just calling transfer
> twice. So I'm not even sure it's a good idea.

Linux has I2C_M_STOP to force a stop.  I guess that it's a workaround for some
broken slaves that are confused by repeated start.  But as you say, just
splitting the transfers would work as well.

> We might need to leave the current broken interface in place for out-
> of-tree code like proprietary drivers, and define a new transfer method
> that works the new way.

That would make sense to do.

> When I started looking at all existing callers of iicbus_transfer() I
> came to the conclusion that almost all of them followed such identical
> patterns that I wrote the iicdev_readfrom()/writeto() functions. I
> figured I would run through the system converting everything I could to
> those new functions, then whatever changes need to be made to the
> interface would almost all be in just a couple functions. But that
> project bogged down then other things came along and I forgot all about
> it.

Indeed, these are the most used patterns, so makes sense to have common routines
for them.

--
Andriy Gapon
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined?

Lee D
In reply to this post by Ian Lepore-3
On Sun, Mar 18, 2018 at 10:30 AM, Ian Lepore <[hidden email]> wrote:

> Nostop is an ivar of the child device, so iicbus_get_nostop() is formed
> by the IICBUS_ACCESSOR macro in iicbus.h.
>
> Now for the bad news:  don't use it.  It doesn't work.  It's 100% a bug
> in the code that maybe kinda-sorta seemed to work for whoever added it,
> because accidentally the right garbage was on the stack in the local
> nostop var.  The generic transfer code doesn't check that the accessor
> failed so it ends up using stack garbage for nostop.  The reason
> there's g'teed to be no such ivar is because the code is asking the
> wrong device, and it doesn't even have a handle to the right child
> device to get the info it wants.
>
> So the bottom line is, write your transfer routine to work however it
> has to work.  That might mean ignoring the stop/nostop flags and just
> doing whatever your hardware does.  Like if a write transaction is
> handled by the hardware by putting the slave address and the offse-
> within-slave values into registers and it does a write of the offset
> then a read from the slave and you get no control over whether it does
> that as two transactions or as 1 transaction with a repeat-start
> between the read and write, then just silently do it that way.
>
> I had forgotten about the nostop mess, which I discovered some time
> last year.  It really needs to be fixed the right way, which is to
> remove the nostop hack, remove all uses of the NOSTOP flag everywhere
> in the code, and make the default behavior that all back-to-back
> operations in the same array of cmds handed to a single transfer call
> have implicit repeat-start behavior when changing slave or direction.
>  We could add a specific STOP flag to force a stop/start between two
> commands, but even that's not really needed.
>
> The only example of a transfer-only driver I know of offhand is the rpi
> driver (arm/broadcom/bcm2835/bcm2835_bsc.c).  Unfortunately, bugs in
> the rpi silicon complicate the code and make it a messy example.
>
> -- Ian
>

First let me apologize for taking so long to respond.

What you've told me is actually good news as far as I am concerned,
that I can just do what needs to be done in my driver.

So thanks!

Lee
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"