On X1E80100, there is a hardware bug in the register logic of the
IRQ_ENABLE_BANK register. While read accesses work on the normal address,
all write accesses must be made to a shifted address. Without a workaround
for this, the wrong interrupt gets enabled in the PDC and it is impossible
to wakeup from deep suspend (CX collapse).

This has not caused problems so far, because the deep suspend state was not
enabled. We need a workaround now since work is ongoing to fix this.

Introduce a workaround for the problem by matching the qcom,x1e80100-pdc
compatible and shift the write address by the necessary offset.

Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
---
 drivers/irqchip/qcom-pdc.c | 51 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 74b2f124116e3415d77269959c1ed5e7d7efd671..9ce44121db21ac05b370046feb09c17122f80b19 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -21,9 +21,11 @@
 #include <linux/types.h>
 
 #define PDC_MAX_GPIO_IRQS	256
+#define PDC_DRV_OFFSET		0x10000
 
 /* Valid only on HW version < 3.2 */
 #define IRQ_ENABLE_BANK		0x10
+#define IRQ_ENABLE_BANK_MAX	(IRQ_ENABLE_BANK + BITS_TO_BYTES(PDC_MAX_GPIO_IRQS))
 #define IRQ_i_CFG		0x110
 
 /* Valid only on HW version >= 3.2 */
@@ -46,13 +48,20 @@ struct pdc_pin_region {
 
 static DEFINE_RAW_SPINLOCK(pdc_lock);
 static void __iomem *pdc_base;
+static void __iomem *pdc_drv1;
 static struct pdc_pin_region *pdc_region;
 static int pdc_region_cnt;
 static unsigned int pdc_version;
+static bool pdc_x1e_quirk;
+
+static void _pdc_reg_write(void __iomem *base, int reg, u32 i, u32 val)
+{
+	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 */
+
+	/*
+	 * 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;
+	}
+
+	_pdc_reg_write(base, IRQ_ENABLE_BANK, bank, enable);
+}
+
 static void __pdc_enable_intr(int pin_out, bool on)
 {
 	unsigned long enable;
@@ -72,7 +101,11 @@ static void __pdc_enable_intr(int pin_out, bool on)
 
 		enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
 		__assign_bit(mask, &enable, on);
-		pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
+
+		if (pdc_x1e_quirk)
+			pdc_x1e_irq_enable_write(index, enable);
+		else
+			pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
 	} else {
 		enable = pdc_reg_read(IRQ_i_CFG, pin_out);
 		__assign_bit(IRQ_i_CFG_IRQ_ENABLE, &enable, on);
@@ -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);
+		if (!pdc_drv1) {
+			pr_err("%pOF: unable to map PDC DRV1 region\n", node);
+			return -ENXIO;
+		}
+
+		pdc_x1e_quirk = true;
+	}
+
 	pdc_base = ioremap(res.start, res_size);
 	if (!pdc_base) {
 		pr_err("%pOF: unable to map PDC registers\n", node);
-		return -ENXIO;
+		ret = -ENXIO;
+		goto fail;
 	}
 
 	pdc_version = pdc_reg_read(PDC_VERSION_REG, 0);
@@ -363,6 +407,7 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
 fail:
 	kfree(pdc_region);
 	iounmap(pdc_base);
+	iounmap(pdc_drv1);
 	return ret;
 }
 

---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250211-x1e80100-pdc-hw-wa-b738d99ef459

Best regards,
-- 
Stephan Gerhold <stephan.gerhold@linaro.org>

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

On Thu, Feb 13, 2025 at 06:04:00PM +0100, Stephan Gerhold wrote:
> On X1E80100, there is a hardware bug in the register logic of the
> IRQ_ENABLE_BANK register. While read accesses work on the normal address,
> all write accesses must be made to a shifted address. Without a workaround
> for this, the wrong interrupt gets enabled in the PDC and it is impossible
> to wakeup from deep suspend (CX collapse).
> 
> This has not caused problems so far, because the deep suspend state was not
> enabled. We need a workaround now since work is ongoing to fix this.
> 
> Introduce a workaround for the problem by matching the qcom,x1e80100-pdc
> compatible and shift the write address by the necessary offset.
> 
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>
> ---
>  drivers/irqchip/qcom-pdc.c | 51 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 48 insertions(+), 3 deletions(-)
> 
> @@ -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);

Please mention it in the commit message that you are mapping the memory
outside of the defined device's region.

> +		if (!pdc_drv1) {
> +			pr_err("%pOF: unable to map PDC DRV1 region\n", node);
> +			return -ENXIO;
> +		}
> +
> +		pdc_x1e_quirk = true;
> +	}
> +
>  	pdc_base = ioremap(res.start, res_size);
>  	if (!pdc_base) {
>  		pr_err("%pOF: unable to map PDC registers\n", node);
> -		return -ENXIO;
> +		ret = -ENXIO;
> +		goto fail;
>  	}
>  
>  	pdc_version = pdc_reg_read(PDC_VERSION_REG, 0);
> @@ -363,6 +407,7 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
>  fail:
>  	kfree(pdc_region);
>  	iounmap(pdc_base);
> +	iounmap(pdc_drv1);
>  	return ret;
>  }
>  
> 
> ---
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> change-id: 20250211-x1e80100-pdc-hw-wa-b738d99ef459
> 
> Best regards,
> -- 
> Stephan Gerhold <stephan.gerhold@linaro.org>
> 

-- 
With best wishes
Dmitry

On Thu, Feb 13, 2025 at 06:04:00PM +0100, Stephan Gerhold wrote:
> On X1E80100, there is a hardware bug in the register logic of the
> IRQ_ENABLE_BANK register. While read accesses work on the normal address,
> all write accesses must be made to a shifted address. Without a workaround
> for this, the wrong interrupt gets enabled in the PDC and it is impossible
> to wakeup from deep suspend (CX collapse).
> 
> This has not caused problems so far, because the deep suspend state was not
> enabled. We need a workaround now since work is ongoing to fix this.
> 
> Introduce a workaround for the problem by matching the qcom,x1e80100-pdc
> compatible and shift the write address by the necessary offset.
> 
> Signed-off-by: Stephan Gerhold <stephan.gerhold@linaro.org>

I've been running with this patch for a while now and it allows me to
wake up from deep suspend on the X1E CRD using the power button (or
GPIO interrupts with further patches):

Tested-by: Johan Hovold <johan+linaro@kernel.org>

Johan