Message-ID: <20211130023151.GD10105@dragon>
Date:   Tue, 30 Nov 2021 10:31:52 +0800
From:   Shawn Guo <shawn.guo@...aro.org>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Maulik Shah <quic_mkshah@...cinc.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Loic Poulain <loic.poulain@...aro.org>,
        devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver

+ 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

Powered by blists - more mailing lists