Re: [PATCH v2 2/2] irqchip: add driver for imx-irqsteer controller

[Marc Zyngier <marc.zyngier@arm.com>]

Hi Lucas,

On 17/12/2018 10:09, Lucas Stach wrote:
> Hi Marc,
> 
> Am Samstag, den 15.12.2018, 09:45 +0000 schrieb Marc Zyngier:
>> Hi Lucas,
>>
>> On Fri, 14 Dec 2018 10:22:44 +0000,
>>> Lucas Stach <l.stach@pengutronix.de> wrote:
>>>
>>> The irqsteer block is a interrupt multiplexer/remapper found on the
>>> i.MX8 line of SoCs.
>>>
>>>>> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
>>>>> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
>>> ---
>>> v2:
>>>  - use raw_spinlock
>>>  - add proper clk error handling
>>>  - align register offset calculation with reality
>>>  - use correct SPDX identifier
>>>  - simplify DT configuration
>>> ---
> 
> [...]
> 
>>> +static const struct irq_domain_ops imx_irqsteer_domain_ops = {
>>>>>>> +	.map		= imx_irqsteer_irq_map,
>>> +	.xlate		= irq_domain_xlate_twocell,
>>
>> I've tried to understand why you are using two cells, but couldn't
>> find an explanation from the DT binding. As far as I can see, you only
>> support LEVEL_HIGH signalling, meaning that you could perfectly
>> encode this in a single cell. What am I missing?
> 
> Nothing. At this point I'm sure that the controller only support level-
> high IRQs. Will switch to a single cell in the next spin.

OK, good.

> 
> [...]
> 
>>> +static int imx_irqsteer_probe(struct platform_device *pdev)
>>> +{
>>>>> +	struct device_node *np = pdev->dev.of_node;
>>>>> +	struct irqsteer_data *data;
>>>>> +	struct resource *res;
>>>>> +	int ret;
>>> +
>>>>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>>>>> +	if (!data)
>>>>> +		return -ENOMEM;
>>> +
>>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>>>>> +	if (IS_ERR(data->regs)) {
>>>>> +		dev_err(&pdev->dev, "failed to initialize reg\n");
>>>>> +		return PTR_ERR(data->regs);
>>>>> +	}
>>> +
>>>>> +	data->irq = platform_get_irq(pdev, 0);
>>>>> +	if (data->irq <= 0) {
>>>>> +		dev_err(&pdev->dev, "failed to get irq\n");
>>>>> +		return -ENODEV;
>>>>> +	}
>>> +
>>>>> +	data->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
>>>>> +	if (IS_ERR(data->ipg_clk)) {
>>>>> +		ret = PTR_ERR(data->ipg_clk);
>>>>> +		if (ret != -EPROBE_DEFER)
>>>>> +			dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret);
>>>>> +		return ret;
>>>>> +	}
>>> +
>>>>> +	raw_spin_lock_init(&data->lock);
>>> +
>>>>> +	of_property_read_u32(np, "fsl,irq-groups", &data->irq_groups);
>>>>> +	of_property_read_u32(np, "fsl,channel", &data->channel);
>>> +
>>>>> +	if (IS_ENABLED(CONFIG_PM_SLEEP)) {
>>>>> +		data->saved_reg = devm_kzalloc(&pdev->dev,
>>>>> +					sizeof(u32) * data->irq_groups * 2,
>>>>> +					GFP_KERNEL);
>>>>> +		if (!data->saved_reg)
>>>>> +			return -ENOMEM;
>>>>> +	}
>>> +
>>>>> +	ret = clk_prepare_enable(data->ipg_clk);
>>>>> +	if (ret) {
>>>>> +		dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret);
>>>>> +		return ret;
>>>>> +	}
>>> +
>>>>> +	/* steer all IRQs into configured channel */
>>> +	writel_relaxed(BIT(data->channel), data->regs + CHANCTRL);
>>
>> Could you explain what this channel is exactly?
> 
> I've tired in the cover letter, but seems I still failed, so let me try
> again. ;)
> 
> Each irqsteer instance can be connected to multiple upstream IRQ lines,
> but only one of them can be used at runtime. This register controls
> which output IRQ line will be used by this controller instance. With
> multiple controller instances in the system it's a way to offload the
> decision of the IRQ routing from the hardware to the software guys.
> 
> Let's try to add an example: Suppose there are 2 instances of the
> irqsteer controller. Both are connected to upstream GIC IRQs 20 and 21.
> The channel controls which of those IRQs are used by each instance, so
> there are 2 valid DT configurations in this scenario:
> 
> This is valid:
> 
> irqsteer@0 {
> 	interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>
> 	fsl,channel = <0>;
> };
> 
> irqsteer@1 {
> 	interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>
> 	fsl,channel = <1>;
> };
> 
> As well as this:
> 
> irqsteer@0 {
> 	interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>
> 	fsl,channel = <1>;
> };
> 
> irqsteer@1 {
> 	interrupts = <GIC_SPI 20 IRQ_TYPE_LEVEL_HIGH>
> 	fsl,channel = <0>;
> };

OK, this is now making sense, thanks for that. I'm wondering if it'd
make sense to expose both IRQs in the DT for each irqsteer, and use
fsl,channel as the selector? It doesn't change much in the driver, but
seems to describe the HW in a more complete way.

I don't care much either way, and I'll leave it for you and the DT folks
to decide.

> 	
> [...]
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static void imx_irqsteer_save_regs(struct irqsteer_data *data)
>>> +{
>>>>> +	int num;
>>> +
>>>>> +	for (num = 0; num < data->irq_groups; num++)
>>>>> +		data->saved_reg[num + 1] = readl_relaxed(data->regs +
>>> +						CHANMASK(num, data->irq_groups));
>>
>> You seem to only use one u32 per IRQ group. Yet, you've allocated
>> twice as much memory to that effect. Why? Also, you're avoiding to use
>> index 0, which I find rather odd.
>>
>> Given that the DT binding states that each group manages 64
>> interrupts, I have the feeling that you're missing some interrupt
>> enable state.
> 
> And you are right with this observation. I failed to properly rework
> this part of the code when switching to the irq-group indexing.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...