linux-kernel - Re: [PATCH v13] irqchip: Add driver for Cirrus Logic Madera codecs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <edb4c59d-cbfe-c5ca-8787-10dfcf4ee375@opensource.cirrus.com>
Date:   Fri, 14 Sep 2018 17:31:22 +0100
From:   Richard Fitzgerald <rf@...nsource.cirrus.com>
To:     Marc Zyngier <marc.zyngier@....com>
CC:     <tglx@...utronix.de>, <jason@...edaemon.net>,
        <patches@...nsource.cirrus.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v13] irqchip: Add driver for Cirrus Logic Madera codecs

On 14/09/18 16:55, Marc Zyngier wrote:
> On Fri, 14 Sep 2018 16:28:09 +0100,
> Richard Fitzgerald <rf@...nsource.cirrus.com> wrote:
>>
>> The Cirrus Logic Madera codecs (Cirrus Logic CS47L35/85/90/91 and WM1840)
>> are highly complex devices containing up to 7 programmable DSPs and many
>> other internal sources of interrupts plus a number of GPIOs that can be
>> used as interrupt inputs. The large number (>150) of internal interrupt
>> sources are managed by an on-board interrupt controller.
>>
>> This driver provides the handling for the interrupt controller. As the
>> codec is accessed via regmap, we can make use of the generic IRQ
>> functionality from regmap to do most of the work. Only around half of
>> the possible interrupt source are currently of interest from the driver
>> so only this subset is defined. Others can be added in future if needed.
>>
>> The KConfig options are not user-configurable because this driver is
>> mandatory so is automatically included when the parent MFD driver is
>> selected.
>>
>> Signed-off-by: Richard Fitzgerald <rf@...nsource.cirrus.com>
>> Signed-off-by: Charles Keepax <ckeepax@...nsource.cirrus.com>
>> ---
>> Only difference from v11 is the copyright headers
>> ---
>>   MAINTAINERS                        |   2 +
>>   drivers/irqchip/Kconfig            |   3 +
>>   drivers/irqchip/Makefile           |   1 +
>>   drivers/irqchip/irq-madera.c       | 256 +++++++++++++++++++++++++++++++++++++
>>   include/linux/irqchip/irq-madera.h | 132 +++++++++++++++++++
>>   5 files changed, 394 insertions(+)
>>   create mode 100644 drivers/irqchip/irq-madera.c
>>   create mode 100644 include/linux/irqchip/irq-madera.h
>>
> 
> [...]
> 
>> diff --git a/drivers/irqchip/irq-madera.c b/drivers/irqchip/irq-madera.c
>> new file mode 100644
>> index 000000000000..e9256dee1a45
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-madera.c
> 
> [...]
> 
>> +static int madera_irq_probe(struct platform_device *pdev)
>> +{
>> +	struct madera *madera = dev_get_drvdata(pdev->dev.parent);
>> +	struct irq_data *irq_data;
>> +	unsigned int irq_flags = 0;
>> +	int ret;
>> +
>> +	dev_dbg(&pdev->dev, "probe\n");
>> +
>> +	/*
>> +	 * Read the flags from the interrupt controller if not specified
>> +	 * by pdata
>> +	 */
>> +	irq_flags = madera->pdata.irq_flags;
>> +	if (!irq_flags) {
>> +		irq_data = irq_get_irq_data(madera->irq);
>> +		if (!irq_data) {
>> +			dev_err(&pdev->dev, "Invalid IRQ: %d\n", madera->irq);
>> +			return -EINVAL;
>> +		}
>> +
>> +		irq_flags = irqd_get_trigger_type(irq_data);
>> +
>> +		/* Codec defaults to trigger low, use this if no flags given */
>> +		if (irq_flags == IRQ_TYPE_NONE)
>> +			irq_flags = IRQF_TRIGGER_LOW;
>> +	}
>> +
>> +	if (irq_flags & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
>> +		dev_err(&pdev->dev, "Host interrupt not level-triggered\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * The silicon always starts at active-low, check if we need to
>> +	 * switch to active-high.
>> +	 */
>> +	if (irq_flags & IRQF_TRIGGER_HIGH) {
> 
> Is it safe to assume that the HW is in its reset state? What if the
> firmware has configured it otherwise, or if gone through kexec?
> 

The parent mfd probe always resets the silicon before registering the children.
If this driver were to be removed and reprobed without reprobing the parent mfd,
these lines of code are irrelevant because the regmap cache would still preserve
the correct setting.
The DSP firmware is not allowed to change the host IRQ configuration or reset
the silicon. Firmwares can only be loaded via ALSA/ASoC and the driver for that
cannot complete probe until the irqchip driver has probed,

>> +		ret = regmap_update_bits(madera->regmap, MADERA_IRQ1_CTRL,
>> +					 MADERA_IRQ_POL_MASK, 0);
>> +		if (ret) {
>> +			dev_err(&pdev->dev,
>> +				"Failed to set IRQ polarity: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * NOTE: regmap registers this against the OF node of the parent of
>> +	 * the regmap - that is, against the mfd driver
>> +	 */
>> +	ret = regmap_add_irq_chip(madera->regmap, madera->irq, IRQF_ONESHOT, 0,
>> +				  &madera_irq_chip, &madera->irq_data);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "add_irq_chip failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* Save dev in parent MFD struct so it is accessible to siblings */
>> +	madera->irq_dev = &pdev->dev;
>> +
>> +	return 0;
>> +}
> 
> Thanks,
> 
> 	M.
>