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

[Thomas Gleixner <tglx@linutronix.de>]

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