Re: [PATCH V5 2/2] mailbox: arm_mhuv2: Add driver

[Viresh Kumar]

On 20-11-20, 13:06, Jassi Brar wrote:
> On Tue, Nov 17, 2020 at 4:02 AM Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> 
> > +
> > +/**
> > + * struct mhuv2 - MHUv2 mailbox controller data
> > + *
> > + * @mbox:      Mailbox controller belonging to the MHU frame.
> > + * @send/recv: Base address of the register mapping region.
> > + * @frame:     Frame type: RECEIVER_FRAME or SENDER_FRAME.
> > + * @irq:       Interrupt.
> > + * @windows:   Channel windows implemented by the platform.
> > + * @minor:     Minor version of the controller.
> > + * @length:    Length of the protocols array in bytes.
> > + * @protocols: Raw protocol information, derived from device tree.
> > + * @doorbell_pending_lock: spinlock required for correct operation of Tx
> > + *             interrupt for doorbells.
> > + */
> > +struct mhuv2 {
> > +       struct mbox_controller mbox;
> > +       union {
> > +               struct mhu2_send_frame_reg __iomem *send;
> > +               struct mhu2_recv_frame_reg __iomem *recv;
> > +       };
> > +       enum mhuv2_frame frame;
> > +       unsigned int irq;
> > +       unsigned int windows;
> > +       unsigned int minor;
> > +       unsigned int length;
> > +       u32 *protocols;
> > +
> > +       spinlock_t doorbell_pending_lock;
> >
> Can you please explain the need of this lock? Some usecase?
> It should be unnecessary if the controller natively supports doorbell mode.

Yes, the controller does have native doorbell support, it provides bit
wise access of the registers and interrupt gets generated for every
bit of the register.

This lock is needed only in the case where an interrupt is provided in
the tx case as well (since minor version 1). This is how hardware
works in this case:

- Sender (Linux) writes to a bit of 'stat_set' register to do a
  doorbell operation.

- The receiver receives an interrupt for it and sets the same bit in
  'stat_clear' register in order to clear the interrupt.

- With this the sender gets an interrupt and needs to check which bit
  of 'stat' register went from 1 to 0 to know which doorbell's
  operation got finished.  The sender clears the interrupt for the
  entire window by writing 1 to int_clr register of the corresponding
  window.

Now another doorbell transfer can start right after we read the 'stat'
register in the tx interrupt handler and we will see that a bit went
from 1 to 0 and think that the transfer has completed, while it may
not have as the bit may still be 1. And so we need to make sure to not
start a new transfer for the same window while interrupt handler is
processing the doorbell tx interrupt.

Another way of doing this lockless would be to not use the tx
interrupt and rather poll the bit, but that would be less efficient.

-- 
viresh