Date: Thu, 20 Aug 2020 16:53:01 -0700 From: John-Mark Gurney <jmg@funkthat.com> To: Ian Lepore <ian@freebsd.org> Cc: Alexander Mishin <mishin@mh.net.ru>, freebsd-arm@freebsd.org Subject: Re: Kmod driver at iicbus. attach() and config_intrhook(9) Message-ID: <20200820235301.GE4213@funkthat.com> In-Reply-To: <d709ba1d3473f5d4353dc1109c198f8acb8ffa28.camel@freebsd.org> References: <7fabb65d99aaa74775c1daa91bffb873@mh.net.ru> <3249fa7e-554a-83ef-57b2-7c38aa0b4591@FreeBSD.org> <20200819072409.GA59949@bluezbox.com> <05145b71692af74b103bb226a2e93a15e1e851cb.camel@freebsd.org> <d603cf3b852d5e18fa5faa9066f643a3@mh.net.ru> <20200820223918.GC4213@funkthat.com> <d709ba1d3473f5d4353dc1109c198f8acb8ffa28.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Ian Lepore wrote this message on Thu, Aug 20, 2020 at 16:51 -0600:
> 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
Ahh, yeah, forgot about PMICs...
> polling mechanism that's used early, and revert to using interrupts
> once they're available. The allwinner and rockchip drivers work that
> way.
So, sounds like any controller that is found to not be doing this, needs
to be fixed to use the above function, and then the remaining ones that
either poll, or use the hybrid approach can keep using
bus_generic_attach...
Back to original question, no, that additional logic should not be
needed, and any controller that requires it needs to be fixed
instead...
--
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?20200820235301.GE4213>