Re: [PATCH v3 5/6] i2c: iproc: handle master read request

[Ray Jui <ray.jui@broadcom.com>]

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



On 11/19/2020 9:59 PM, Rayagonda Kokatanur wrote:
> Hi Ray and Dhananjay,
> 
> All review comments are scattered now, please let me know what has to be
> done further,
> Are we going to change the tasklet to irq thread ?

It really depends on the time it takes to read data out of the FIFO.
Dhananjay pointed out that your comment indicates reading 10 bytes of
data takes 20 us, i.e., 2 us per byte read. Do you know why it took so
long? The APB bus should be a lot faster than that (in the hundreds of
ns range). I am making the assumption that by the time when you try to
read data out of the FIFO, the data is of course already in the FIFO, so
it's not like you are waiting for data from the I2C bus and I cannot
understand why it took this long.


> Are we going to remove batching 64 packets if transaction > 64B and use
> rx fifo threshold ?
> 

I don't see any issue with batching. It's more efficient and less
context switch overhead.

> I don't see any issue with current code but if it has to change we need
> a valid reason for the same.

I think we need to confirm the exact time it takes to fetch data from
FIFO. Once that's done, we can make a decision between keeping the
tasklet based approach vs irq thread.

Thanks.


> If nothing to be done, please acknowledge the patch.
>  
> Best regards,
> Raygonda
> 
> 
> On Sat, Nov 14, 2020 at 6:47 AM Dhananjay Phadke
> <dphadke@linux.microsoft.com <mailto:dphadke@linux.microsoft.com>> wrote:
> 
>     On Tue, 10 Nov 2020 11:24:36 -0800, Ray Jui wrote:
> 
>     >>>> 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.
>     >>>
>     >
>     >kernel thread was proposed in the internal review. I don't see much
>     >benefit of using tasklet. If a thread is blocked from running for more
>     >than several tenth of ms, that's really a system-level issue than an
>     >issue with this driver.
>     >
>     >IMO, it's an overkill to use tasklet here but we can probably leave it
>     >as it is since it does not have a adverse effect and the code ran in
>     >tasklet is short.
>     >
>     >How much time is expected to read 64 bytes from an RX FIFO? Even with
>     >APB bus each register read is expected to be in the tenth or
>     hundreds of
>     >nanosecond range. Reading the entire FIFO of 64 bytes should take less
>     >than 10 us. The interrupt context switch overhead is probably longer
>     >than that. It's much more effective to read all of them in a single
>     >batch than breaking them into multiple batches.
> 
>     OK, there's a general discussions towards removing tasklets, if this
>     fix works with threaded isr, strongly recommend that route.
> 
>     This comment in the code suggested that register reads take long time to
>     drain 64 bytes.
> 
>     >+/*
>     >+ * It takes ~18us to reading 10bytes of data, hence to keep tasklet
>     >+ * running for less time, max slave read per tasklet is set to 10
>     >bytes.
> 
>     @Rayagonda, please take care of hand-off mentioned below, once the
>     tasklet
>     is scheduled, isr should just return and clear status at the end of
>     tasklet.
> 
>     >>
>     >> 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?
> 
>     Regards,
>     Dhananjay
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4151 bytes --]