Hi Ray, Could you please check Dhananjay comments and update your thoughts. On Fri, Nov 6, 2020 at 11:11 PM Dhananjay Phadke <dphadke@xxxxxxxxxxxxxxxxxxx> wrote: > > On Thu, 5 Nov 2020 15:13:04 +0530, Rayagonda Kokatanur wrote: > >> So the suggestion was to set HW threshold for rx fifo interrupt, not > >> really a SW property. By setting it in DT, makes it easier to > >> customize for target system, module param needs or ioctl makes it > >> dependent on userpsace to configure it. > >> > >> The need for tasklet seems to arise from the fact that many bytes are > >> left in the fifo. If there's a common problem here, such tasklet would be > >> needed in i2c subsys rather than controller specific tweak, akin to > >> how networking uses NAPI or adding block transactions to the interface? > >> > >> For master write-read event, it seems both IS_S_RD_EVENT_SHIFT and > >> IS_S_RX_EVENT_SHIFT are detected, which implies that core is late to > >> drain rx fifo i.e. write is complete and the read has started on the bus? > > > >Yes it's true that for master write-read events both > >IS_S_RD_EVENT_SHIFT and IS_S_RX_EVENT_SHIFT are coming together. > >So before the slave starts transmitting data to the master, it should > >first read all data from rx-fifo i.e. complete master write and then > >process master read. > > > >To minimise interrupt overhead, we are batching 64bytes. > >To keep isr running for less time, we are using a tasklet. > >Again to keep the tasklet not running for more than 20u, we have set > >max of 10 bytes data read from rx-fifo per tasklet run. > > > >If we start processing everything in isr and using rx threshold > >interrupt, then isr will run for a longer time and this may hog the > >system. > >For example, to process 10 bytes it takes 20us, to process 30 bytes it > >takes 60us and so on. > >So is it okay to run isr for so long ? > > > >Keeping all this in mind we thought a tasklet would be a good option > >and kept max of 10 bytes read per tasklet. > > > >Please let me know if you still feel we should not use a tasklet and > >don't batch 64 bytes. > > Deferring to tasklet is OK, could use a kernel thread (i.e. threaded_irq) > as i2c rate is quite low. > > But do enable rx_threshold and read out early. This will avoid fifo full > or master write-read situation where lot of bytes must be drained from rx > fifo before serving tx fifo (avoid tx underrun). > > Best would have been setting up DMA into mem (some controllers seem capable). > In absence of that, it's a trade off: if rx intr threshold is low, > there will be more interrupts, but less time spent in each. Default could > still be 64B or no-thresh (allow override in dtb). > > Few other comments - > > >+ /* schedule tasklet to read data later */ > >+ tasklet_schedule(&iproc_i2c->slave_rx_tasklet); > >+ > >+ /* clear only IS_S_RX_EVENT_SHIFT interrupt */ > >+ iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, > >+ BIT(IS_S_RX_EVENT_SHIFT)); > >+ } > > Why clearing one rx interrupt bit here after scheduling tasklet? Should all that > be done by tasklet? Also should just return after scheduling tasklet? > > Thanks, > Dhananjay
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
