Re: [PATCH] irqchip/qcom-pdc: Workaround hardware register bug on X1E80100

Thomas Gleixner
2025-02-13T23:29:18Z

On Thu, Feb 13 2025 at 18:04, Stephan Gerhold wrote:
> +
> +static void _pdc_reg_write(void __iomem *base, int reg, u32 i, u32 val)

Please use two leading underscores to make this easy to
distinguish. But ideally you provide a proper function name which makes
it clear that this function operates on a given base address contrary to
pdc_reg_write() which uses pdc_base unconditionally.

> +{
> + writel_relaxed(val, base + reg + i * sizeof(u32));
> +}
>
> static void pdc_reg_write(int reg, u32 i, u32 val)
> {
> - writel_relaxed(val, pdc_base + reg + i * sizeof(u32));
> + _pdc_reg_write(pdc_base, reg, i, val);
> }
>
> static u32 pdc_reg_read(int reg, u32 i)
> @@ -60,6 +69,26 @@ static u32 pdc_reg_read(int reg, u32 i)
> return readl_relaxed(pdc_base + reg + i * sizeof(u32));
> }
>
> +static void pdc_x1e_irq_enable_write(u32 bank, u32 enable)
> +{
> + void __iomem *base = pdc_base; /* DRV2 */

Please do not use tail comments. Also what is DRV2?

> +
> + /*
> + * Workaround hardware bug in the register logic on X1E80100:
> + * - For bank 0-1, writes need to be made to DRV1, bank 3 and 4.
> + * - For bank 2-4, writes need to be made to DRV2, bank 0-2.
> + * - Bank 5 works as expected.
> + */
> + if (bank <= 1) {
> + base = pdc_drv1;
> + bank += 3;
> + } else if (bank <= 4) {
> + bank -= 2;
> + }

This is confusing at best. You map two different base addresses:

1) The existing pdc_base, which magically is associated to DRV2
(whatever that means)

2) A new magic pdc_drv1 mapping

Then you implement the workaround logic in a pretty uncomprehensible
way. I had to stare at it more than once to make sure that it matches
the comment. What about:

/* Remap the bank access to work around the X1E hardware bug. */
switch (bank) {
case 0..1:
/* Use special mapping and shift to bank 3-4 */
base = pdc_base_x1e_quirk;
bank += 3;
break;
case 2..4:
/* Use regular mapping and shift to bank 0-2 */
base = pdc_base;
bank -= 2;
break;
case 5:
/* No fixup required */
base = pdc_base;
break;
}

which makes it obvious what this is about. Hmm?

> + _pdc_reg_write(base, IRQ_ENABLE_BANK, bank, enable);

> @@ -324,10 +357,21 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
> if (res_size > resource_size(&res))
> pr_warn("%pOF: invalid reg size, please fix DT\n", node);
>
> + if (of_device_is_compatible(node, "qcom,x1e80100-pdc")) {
> + pdc_drv1 = ioremap(res.start - PDC_DRV_OFFSET, IRQ_ENABLE_BANK_MAX);

What? This maps outside of the resource region. That's neither documented in
the change log nor explained here.

I assume this can't be properly fixed in the device tree for backwards
compability reasons, but leaving this undocumented is a recipe for head
scratching three month down the road.

PDC_DRV_OFFSET is not obvious either.

You really want to explain this properly at least in the change log,
i.e.:

X1E80100 has a hardware bug related to the address decoding of write
accesses to the IRQ_ENABLE_BANK register.

Correct implementations access it linear from the base address:

addr[bank] = base_addr + IRQ_ENABLE_BANK + bank * sizeof(u32);

The X1E80100 bug requires the following address mangling:

addr[bank0] = base_addr - 0x10000 + IRQ_ENABLE_BANK + 3 * sizeof(u32);
addr[bank1] = base_addr - 0x10000 + IRQ_ENABLE_BANK + 4 * sizeof(u32);
addr[bank2] = base_addr + IRQ_ENABLE_BANK + 0 * sizeof(u32);
addr[bank3] = base_addr + IRQ_ENABLE_BANK + 1 * sizeof(u32);
addr[bank4] = base_addr + IRQ_ENABLE_BANK + 2 * sizeof(u32);
addr[bank5] = base_addr + IRQ_ENABLE_BANK + 5 * sizeof(u32);

The offset (0x10000) is outside of the resource region and requires
therefore a seperate mapping. This can't be expressed in the device
tree for $sensible reasons.

Mapping this region is safe because $sensible reason.

I might have oracled this out of the patch/change log incorrectly, but
you get the idea.

Thanks,

tglx