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

[Lucas Stach]

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@xxxxxxxxxxxxxx> wrote:
> > 
> > The irqsteer block is a interrupt multiplexer/remapper found on the
> > i.MX8 line of SoCs.
> > 
> > > > Signed-off-by: Fugang Duan <fugang.duan@xxxxxxx>
> > > > Signed-off-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx>
> > ---
> > 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.

[...]

> > +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>;
};
	
[...]
> > +#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.

Regards,
Lucas