+ Maulik
On Mon, Nov 29, 2021 at 03:24:39PM +0000, Marc Zyngier wrote:
[...]
> > > > @@ -430,6 +430,14 @@ config QCOM_PDC
> > > > Power Domain Controller driver to manage and configure wakeup
> > > > IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
> > > >
> > > > +config QCOM_MPM
> > > > + bool "QCOM MPM"
> > >
> > > Can't be built as a module?
> >
> > The driver is implemented as a builtin_platform_driver().
>
> This, on its own, shouldn't preclude the driver from being built as a
> module. However, the config option only allows it to be built in. Why?
I just tried to build it as a module, and it seems that "irq_to_desc" is
only available for built-in build.
>
> [...]
>
> > > > +static inline void
> > > > +qcom_mpm_write(struct qcom_mpm_priv *priv, unsigned int reg,
> > > > + unsigned int index, u32 val)
> > > > +{
> > > > + unsigned int offset = (reg * priv->reg_stride + index + 2) * 4;
> > > > + u32 r_val;
> > > > +
> > > > + writel(val, priv->base + offset);
> > > > +
> > > > + do {
> > > > + r_val = readl(priv->base + offset);
> > > > + udelay(5);
> > > > + } while (r_val != val);
> > >
> > > What? Is this waiting for a bit to clear? Why isn't this one of the
> > > read*_poll_timeout*() function instead? Surely you can't wait forever
> > > here.
> >
> > This is taken from downstream, and it seems to double check the written
> > value by reading it back. But to be honest, I'm not really this is
> > necessary. I will do some testing with the read-back check dropped.
>
> How about asking for specs instead? There are QC people on Cc, and
> many more reading the list. Hopefully they can explain what this is
> all about.
Maulik,
If you have some information about this, that would be great.
>
> > >
> > > > +}
> > > > +
> > > > +static inline void qcom_mpm_enable_irq(struct irq_data *d, bool en)
> > > > +{
> > > > + struct qcom_mpm_priv *priv = d->domain->host_data;
> > >
> > > This really should be stored in d->chip_data.
> >
> > OK.
> >
> > >
> > > > + int pin = d->hwirq;
> > > > + unsigned int index = pin / 32;
> > > > + unsigned int shift = pin % 32;
> > > > + unsigned long flags;
> > > > + u32 val;
> > > > +
> > > > + priv->pin_to_irq[pin] = d->irq;
> > >
> > > This makes no sense. This is only reinventing the very notion of an
> > > irq domain, which is to lookup the Linux interrupt based on a context
> > > and a signal number.
> >
> > The uniqueness of this driver is that it has two irq domains. This
> > little lookup table is created to avoid resolving mapping on each of
> > these domains, which is less efficient. But you think this is really
> > nonsense, I can change.
>
> "efficient"? You are taking an interrupt to... err... reinject some
> other interrupts. I'm sorry, but the efficiency argument sailed once
> someone built this wonderful piece of HW. The first mistake was to
> involve SW here, so let's not optimise for something that really
> doesn't need it.
OK.
>
> Furthermore, why would you look up anywhere other than the wake-up
> domain? My impression was that only these interrupts would require
> being re-triggered.
Both domains have MPM pins that could wake up system.
>
> [...]
>
> > > > +static inline void
> > > > +mpm_set_type(struct qcom_mpm_priv *priv, bool set, unsigned int reg,
> > > > + unsigned int index, unsigned int shift)
> > > > +{
> > > > + u32 val;
> > > > +
> > > > + val = qcom_mpm_read(priv, reg, index);
> > > > + if (set)
> > > > + val |= 1 << shift;
> > > > + else
> > > > + val &= ~(1 << shift);
> > > > + qcom_mpm_write(priv, reg, index, val);
> > > > +}
> > > > +
> > > > +static int qcom_mpm_set_type(struct irq_data *d, unsigned int type)
> > > > +{
> > > > + struct qcom_mpm_priv *priv = d->domain->host_data;
> > > > + int pin = d->hwirq;
> > > > + unsigned int index = pin / 32;
> > > > + unsigned int shift = pin % 32;
> > > > + unsigned long flags;
> > > > +
> > > > + spin_lock_irqsave(&priv->lock, flags);
> > > > +
> > > > + mpm_set_type(priv, (type & IRQ_TYPE_EDGE_RISING) ? 1 : 0,
> > >
> > > You have a bool type as the second parameter. Why the convoluted ?:
> > > operator?
> >
> > Will change to !!(type & IRQ_TYPE_EDGE_RISING).
> >
> > >
> > > > + MPM_REG_RISING_EDGE, index, shift);
> > > > + mpm_set_type(priv, (type & IRQ_TYPE_EDGE_FALLING) ? 1 : 0,
> > > > + MPM_REG_FALLING_EDGE, index, shift);
> > > > + mpm_set_type(priv, (type & IRQ_TYPE_LEVEL_HIGH) ? 1 : 0,
> > > > + MPM_REG_POLARITY, index, shift);
> > >
> > > Why does this mean for an edge interrupt?
> >
> > Edge polarity is configured above on MPM_REG_RISING_EDGE and
> > MPM_REG_FALLING_EDGE. So I guess MPM_REG_POLARITY doesn't have an
> > impact on edge interrupt. I do not have any document or information
> > other than downstream code to be sure though.
>
> A well formed 'type' will have that bit clear when any of the EDGE
> flags is set. So this will always be 0. It would also be much better
> if you expressed the whole thing as a switch/case.
OK.
>
> [...]
>
> > > > +static int qcom_mpm_probe(struct platform_device *pdev)
> > > > +{
> > > > + struct irq_domain *parent_domain, *mpm_gic_domain, *mpm_gpio_domain;
> > > > + struct device *dev = &pdev->dev;
> > > > + struct device_node *np = dev->of_node;
> > > > + struct device_node *parent = of_irq_find_parent(np);
> > > > + struct qcom_mpm_priv *priv;
> > > > + unsigned int pin_num;
> > > > + int irq;
> > > > + int ret;
> > > > +
> > > > + /* See comments in platform_irqchip_probe() */
> > > > + if (parent && !irq_find_matching_host(parent, DOMAIN_BUS_ANY))
> > > > + return -EPROBE_DEFER;
> > >
> > > So why aren't you using that infrastructure?
> >
> > Because I need to populate .pm of platform_driver and use match data to
> > pass mpm_data.
>
> Then I'd rather you improve the existing infrastructure to pass the
> bit of extra data you need, instead than reinventing your own.
OK. I will see what I can do here.
>
> [...]
>
> > > > + priv->mbox_client.dev = dev;
> > > > + priv->mbox_chan = mbox_request_channel(&priv->mbox_client, 0);
> > > > + if (IS_ERR(priv->mbox_chan)) {
> > > > + ret = PTR_ERR(priv->mbox_chan);
> > > > + dev_err(dev, "failed to acquire IPC channel: %d\n", ret);
> > > > + goto remove_gpio_domain;
> > >
> > > Why don't you request this first, before all the allocations?
> >
> > Then I will need to call mbox_free_channel() for any allocation failures
> > afterward.
>
> Which would be fine. Checking for dependencies before allocating
> resources is good practice, specially when this can result in a probe
> deferral.
Good point, thanks!
Shawn
