Re: [PATCH V2 02/10] mailbox: tegra-hsp: Add HSP(Hardware Synchronization Primitives) driver

[Stephen Warren]

On 07/06/2016 03:06 AM, Joseph Lo wrote:
> On 07/06/2016 03:05 PM, Alexandre Courbot wrote:
> > On Tue, Jul 5, 2016 at 6:04 PM, Joseph Lo <josephl@xxxxxxxxxx> wrote:
> > > The Tegra HSP mailbox driver implements the signaling doorbell-based
> > > interprocessor communication (IPC) for remote processors currently. The
> > > HSP HW modules support some different features for that, which are
> > > shared mailboxes, shared semaphores, arbitrated semaphores, and
> > > doorbells. And there are multiple HSP HW instances on the chip. So the
> > > driver is extendable to support more features for different IPC
> > > requirement.
> > >
> > > The driver of remote processor can use it as a mailbox client and deal
> > > with the IPC protocol to synchronize the data communications.

> > > diff --git a/drivers/mailbox/tegra-hsp.c b/drivers/mailbox/tegra-hsp.c

> > > +static irqreturn_t hsp_db_irq(int irq, void *p)
> > > +{
> > > +       struct tegra_hsp_mbox *hsp_mbox = p;
> > > +       ulong val;
> > > +       int master_id;
> > > +
> > > +       val = (ulong)hsp_readl(hsp_mbox->db_base[HSP_DB_CCPLEX],
> > > +                              HSP_DB_REG_PENDING);
> > > +       hsp_writel(hsp_mbox->db_base[HSP_DB_CCPLEX],
> > > HSP_DB_REG_PENDING, val);
> > > +
> > > +       spin_lock(&hsp_mbox->lock);
> > > +       for_each_set_bit(master_id, &val, MAX_NUM_HSP_CHAN) {
> > > +               struct mbox_chan *chan;
> > > +               struct tegra_hsp_mbox_chan *mchan;
> > > +               int i;
> > > +
> > > +               for (i = 0; i < MAX_NUM_HSP_CHAN; i++) {
> >
> > I wonder if this could not be optimized. You are doing a double loop
> > on MAX_NUM_HSP_CHAN to look for an identical master_id. Since it seems
> > like the same master_id cannot be used twice (considering that the
> > inner loop only processes the first match), couldn't you just select
> > the free channel in of_hsp_mbox_xlate() by doing
> > &mbox->chans[master_id] (and returning an error if it is already
> > used), then simply getting chan as &hsp_mbox->mbox->chans[master_id]
> > instead of having the inner loop below? That would remove the need for
> > the second loop.
>
> That was exactly what I did in the V1, which only supported one HSP
> sub-module per HSP HW block. So we can just use the master_id as the
> mbox channel ID.
>
> Meanwhile, the V2 is purposed to support multiple HSP sub-modules to be
> running on the same HSP HW block. The "ID" between different modules
> could be conflict. So I dropped the mechanism that used the master_id as
> the mbox channel ID.

I haven't looked at the code in this patch since I'm mainly concerned 
about the DT bindings. However, I will say that nothing in the change to 
the mailbox specifier in DT should have required /any/ changes to the 
code, except to add a single check to validate that the "mailbox type" 
encoded into the top 16 bits of the mailbox ID were 0, and hence 
represented a doorbell rather than anything else. Any enhancements to 
support other mailbox types could have happened later, and I doubt would 
require anything dynamic even then.

> > > +static int tegra_hsp_db_init(struct tegra_hsp_mbox *hsp_mbox,
> > > +                            struct mbox_chan *mchan, int master_id)
> > > +{
> > > +       struct platform_device *pdev = to_platform_device(hsp_mbox->mbox->dev);
> > > +       struct tegra_hsp_mbox_chan *hsp_mbox_chan;
> > > +       int ret;
> > > +
> > > +       if (!hsp_mbox->db_irq) {
> > > +               int i;
> > > +
> > > +               hsp_mbox->db_irq = platform_get_irq_byname(pdev, "doorbell");
> >
> > Getting the IRQ sounds more like a job for probe() - I don't see the
> > benefit of lazy-doing it?
>
> We only need the IRQ when the client is requesting the DB service. For
> other HSP sub-modules, they are using different IRQ. So I didn't do that
> at probe time.

All resources provided by other devices/drivers must be acquired at 
probe time, since that's the only time it's possible to defer probe if 
the provider of the resource is not available.

If you don't follow that rule, what happens is:

1) This driver probes.

2) Some other driver calls tegra_hsp_db_init(), and it fails since the 
provider of the IRQ is not yet available. This likely ends up returning 
something other than -EPROBE_DEFER since the HSP driver was found 
successfully (thus there is no deferred probe situation as far as the 
mailbox core is concerned), it's just that the mailbox channel 
lookup/init/... failed.

3) The other driver's probe() fails due to this, but since the error 
wasn't a probe deferral, the other driver's probe() is never retried.