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

[Rayagonda Kokatanur]

On Thu, Nov 5, 2020 at 1:16 PM Dhananjay Phadke
<dphadke@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, 4 Nov 2020 10:01:06 -0800, Ray Jui wrote:
>
> >>>> +#define MAX_SLAVE_RX_PER_INT         10
> >>>>
> >>>
> >>>> In patch [3/6], you've enabled IS_S_RX_THLD_SHIFT in slave ISR bitmask,
> >>>> however it's not actually used in processing rx events.
> >>>>
> >>>> Instead of hardcoding this threshold here, it's better to add a
> >>>> device-tree knob for rx threshold, program it in controller and handle
> >>>> that RX_THLD interrupt. This will give more flexibility to drain the rx
> >>>> fifo earlier than -
> >>>> (1) waiting for FIFO_FULL interrupt for transactions > 64B.
> >>>> (2) waiting for start of read transaction in case of master write-read.
> >>
> >> Yes this is one way to implement.
> >> But do you see any issue in batching 64 bytes at a time in case of
> >> transaction > 64 Bytes.
> >> I feel batching will be more efficient as it avoids more number of
> >> interrupts and hence context switch.
> >>
> >>>
> >>> The Device Tree is really intended to describe the hardware FIFO size,
> >>> not watermarks, as those tend to be more of a policy/work load decision.
> >>> Maybe this is something that can be added as a module parameter, or
> >>> configurable via ioctl() at some point.
> >>
> >
> >Yes, DT can have properties to describe the FIFO size, if there happens
> >to be some variants in the HW blocks in different versions. But that is
> >not the case here. DT should not be used to control SW/use case specific
> >behavior.
>
> 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.

Thanks
Rayagonda
>
>
> Thanks,
> Dhananjay
>
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature