Date: Tue, 1 Sep 2020 15:18:39 -0700 From: John-Mark Gurney <jmg@funkthat.com> To: Alexander Mishin <mishin@mh.net.ru> Cc: freebsd-arm@freebsd.org Subject: Re: Kmod driver at iicbus. attach() and config_intrhook(9) Message-ID: <20200901221839.GY4213@funkthat.com> In-Reply-To: <0496ebc5628a015c005c150549e1e70a@mh.net.ru> <496ed13afb2327d6416664b4dacfc346@mh.net.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
Alexander Mishin wrote this message on Sat, Aug 22, 2020 at 00:59 +0400:
> Ian Lepore ?????????? 2020-08-21 02:51:
> > On Thu, 2020-08-20 at 15:39 -0700, John-Mark Gurney wrote:
> >> Alexander Mishin wrote this message on Thu, Aug 20, 2020 at 10:07
> >> +0400:
> >> > Ian Lepore ?????????? 2020-08-19 19:39:
> >> > > On Wed, 2020-08-19 at 00:24 -0700, Oleksandr Tymoshenko wrote:
> >> > > > Andriy Gapon (avg@FreeBSD.org) wrote:
> >> > > > > On 18/08/2020 22:05, Alexander Mishin wrote:
> >> > > > > > Hi
> >> > > > > > ...
> >> > > > > > But I see that some other devices (from /usr/src/sys/dev)
> >> > > > > > uses
> >> > > > > > CONFIG_INTRHOOK(9)
> >> > > > > > on attach() for initialize themselfs.
> >> > > > > > I wonder if I need this too? ...
> >> > > > >
> >> > > > > This is usually needed when a driver needs to talk to its
> >> > > > > device
> >> > > > > while
> >> > > > > attaching. E.g., to set some initial configuration or to
> >> > > > > confirm
> >> > > > > device's
> >> > > > > identity, etc.
> >> > > >
> >> > > > To extend Andriy's explanation a bit: all these operations may
> >> > > > perform
> >> > > > I2C transfers and most I2C controllers use interrupts to get
> >> > > > notified
> >> > > > about tranfer status change (finished, error, etc...). There is
> >> > > > no
> >> > > > guarantee that when driver's attach method is called interrupts
> >> > > > are
> >> > > > globally enabled. What would happen in this case is: I2C
> >> > > > controller
> >> > > > is going to initiate I2C operation and wait for an interrupt
> >> > > > that's
> >> > > > never going to be delivered. CONFIG_INTRHOOK is a solution for
> >> > > > this
> >> > > > problem, if your attach method requires interrupts - just split
> >> > > > it
> >> > > > in two parts and postpone running interrupt-dependent part
> >> > > > until
> >> > > > after
> >> > > > interrupts are globally enabled.
> >> > >
> >> > > A note about all this: It should never be necessary for an i2c
> >> > > slave
> >> > > device driver to do this. The reason it's needed is because many
> >> > > i2c
> >> > > controller drivers attach the iicbus from their attach() routine
> >> > > even
> >> > > though they can't actually do i2c IO until interrupts are
> >> > > available.
> >> > > It is these controller drivers that should have the intrhook
> >> > > logic to
> >> > > not call bus_generic_attach() until interrupts are available if
> >> > > they
> >> > > can't do IO until interrupts are available.
> >> > >
> >> > > It has long been my goal to fix all our i2c controller drivers to
> >> > > behave correctly, so that i2c slave device drivers don't all need
> >> > > the
> >> > > intrhook logic. But somehow I never get around to it.
> >> >
> >> > I think, it would be helpful, as it would be possible to return an
> >> > error
> >> > on early stage, from attach(), if there is no connection with the
> >> > configured device.
> >>
> >> Looks like there's a function bus_delayed_attach_children designed
> >> exactly for this:
> >> * Many buses can't run transactions on the bus which children need
> >> to probe and
> >> * attach until after interrupts and/or timers are running. This
> >> function
> >> * delays their attach until interrupts and timers are enabled.
> >>
> >> and it looks like a couple controllers are already using it, imx_i2c
> >> and ti_i2c...
> >>
> >> It looks like maybe a simple replace of bus_generic_attach w/
> >> bus_delayed_attach_children should be enough on those w/
> >> interrupts...
> >>
> >> Is there any argument for doing it for ALL controllers instead of
> >> just
> >> some?
> >>
> >> Poking around some, and it looks like some (one) drivers "pretend" to
> >> use interrupts, but just busy wait instead, e.g. exynos5...
> >>
> >
> > Hmm, yeah, it looks like more has been done along these lines than I
> > remembered. In fact, the work may be done.
> >
> > Some i2c controllers have to work properly before interrupts are
> > available, to control things like PMIC chips that are required very
> > early in device configuration. Typically they have some sort of
> > polling mechanism that's used early, and revert to using interrupts
> > once they're available. The allwinner and rockchip drivers work that
> > way.
> >
> Just to dot the i's, my SoC is allwinner (Orange PI PC). And the issue
> at boot time really showed itself up until config_intrhook was used.
> FreeBSD myhost.mh.net.ru 13.0-CURRENT FreeBSD 13.0-CURRENT #3 r364004
Maybe I misread this thread, but this implied to me that the device had
to do the config_intrhook work in order to work on the twsi i2c bus
driver, is this not the case?
Alexander Mishin wrote this message on Tue, Sep 01, 2020 at 19:21 +0400:
> Andriy Gapon ?????????? 2020-09-01 09:51:
> > On 01/09/2020 03:26, John-Mark Gurney wrote:
> >> Ok, then the OP shouldn't be having a problem then, maybe you can help
> >> then figure it out then?
> >
> > Was there actually any problem?
> > I thought that OP was just inquiring about the pattern commonly seen
> > in the code.
> >
> > Alexander Mishin wrote:
> >> But I see that some other devices (from /usr/src/sys/dev) uses
> >> CONFIG_INTRHOOK(9)
> >> on attach() for initialize themselfs.
> >> I wonder if I need this too? ...or maybe... when I might need it?
>
> Yes, exactly. And I quite understood about using config_intrhook(9),
> thank you all!
> ...but still interesting to read if the need for this would be removed
> for the twsi controller.
So my reading of the twsi driver is that it handles things correctly
allowing devices pre-interrupts to attach, and using interrupts for
transfers post-cold.
So, your driver does not need to do any config_intrhook magic to make
it work.
If you run across any other i2c bus controllers than need it, then that
controller should be fixed.
--
John-Mark Gurney Voice: +1 415 225 5579
"All that I will do, has been done, All that I have, has not."
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20200901221839.GY4213>