drivers/irqchip/qcom-pdc.c | 61 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 48 insertions(+), 13 deletions(-)
Starting from HW version 3.2 the IRQ_ENABLE bit has moved to the
IRQ_i_CFG register and requires a change of the driver to avoid
writing into an undefined register address.
Get the HW version from registers and set the IRQ_ENABLE bit to the
correct register depending on the HW version.
Reviewed-by: Maulik Shah <quic_mkshah@quicinc.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v3:
- Simplify qcom_pdc_gic_set_type()
- Used __assign_bit in pdc_setup_pin_mapping()
- remove BIT() from IRQ_i_CFG_IRQ_ENABLE to be used with __assign_bit()
- Add Reviewed-by tag
- Link to v2: https://lore.kernel.org/r/20230822-topic-sm8x50-upstream-pdc-ver-v2-1-3035b8d388f7@linaro.org
Changes in v2:
- Changed IRQ_ENABLE handling based on Maulik's comments
- Link to v1: https://lore.kernel.org/r/20230821-topic-sm8x50-upstream-pdc-ver-v1-1-6d7f4dd95719@linaro.org
---
drivers/irqchip/qcom-pdc.c | 61 ++++++++++++++++++++++++++++++++++++----------
1 file changed, 48 insertions(+), 13 deletions(-)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index d96916cf6a41..5f60a23686c9 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -23,9 +23,22 @@
#define PDC_MAX_GPIO_IRQS 256
+/* Valid only on HW version < 3.2 */
#define IRQ_ENABLE_BANK 0x10
#define IRQ_i_CFG 0x110
+/* Valid only on HW version >= 3.2 */
+#define IRQ_i_CFG_IRQ_ENABLE 3
+
+#define IRQ_i_CFG_TYPE_MASK GENMASK(2, 0)
+
+#define PDC_VERSION 0x1000
+
+/* Notable PDC versions */
+enum {
+ PDC_VERSION_3_2 = 0x30200,
+};
+
struct pdc_pin_region {
u32 pin_base;
u32 parent_base;
@@ -38,6 +51,7 @@ static DEFINE_RAW_SPINLOCK(pdc_lock);
static void __iomem *pdc_base;
static struct pdc_pin_region *pdc_region;
static int pdc_region_cnt;
+static unsigned int pdc_version;
static void pdc_reg_write(int reg, u32 i, u32 val)
{
@@ -54,15 +68,22 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
int pin_out = d->hwirq;
unsigned long enable;
unsigned long flags;
- u32 index, mask;
-
- index = pin_out / 32;
- mask = pin_out % 32;
raw_spin_lock_irqsave(&pdc_lock, flags);
- enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
- __assign_bit(mask, &enable, on);
- pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
+ if (pdc_version < PDC_VERSION_3_2) {
+ u32 index, mask;
+
+ index = pin_out / 32;
+ mask = pin_out % 32;
+
+ enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
+ __assign_bit(mask, &enable, on);
+ 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);
+ pdc_reg_write(IRQ_i_CFG, pin_out, enable);
+ }
raw_spin_unlock_irqrestore(&pdc_lock, flags);
}
@@ -143,6 +164,7 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
}
old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
+ pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK);
pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
ret = irq_chip_set_type_parent(d, type);
@@ -247,7 +269,7 @@ static const struct irq_domain_ops qcom_pdc_ops = {
static int pdc_setup_pin_mapping(struct device_node *np)
{
int ret, n, i;
- u32 irq_index, reg_index, val;
+ unsigned long val;
n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
if (n <= 0 || n % 3)
@@ -278,11 +300,22 @@ static int pdc_setup_pin_mapping(struct device_node *np)
return ret;
for (i = 0; i < pdc_region[n].cnt; i++) {
- reg_index = (i + pdc_region[n].pin_base) >> 5;
- irq_index = (i + pdc_region[n].pin_base) & 0x1f;
- val = pdc_reg_read(IRQ_ENABLE_BANK, reg_index);
- val &= ~BIT(irq_index);
- pdc_reg_write(IRQ_ENABLE_BANK, reg_index, val);
+ if (pdc_version < PDC_VERSION_3_2) {
+ u32 irq_index, reg_index;
+
+ reg_index = (i + pdc_region[n].pin_base) >> 5;
+ irq_index = (i + pdc_region[n].pin_base) & 0x1f;
+ val = pdc_reg_read(IRQ_ENABLE_BANK, reg_index);
+ __assign_bit(irq_index, &val, 0);
+ pdc_reg_write(IRQ_ENABLE_BANK, reg_index, val);
+ } else {
+ u32 irq;
+
+ irq = i + pdc_region[n].pin_base;
+ val = pdc_reg_read(IRQ_i_CFG, irq);
+ __assign_bit(IRQ_i_CFG_IRQ_ENABLE, &val, 0);
+ pdc_reg_write(IRQ_i_CFG, irq, val);
+ }
}
}
@@ -300,6 +333,8 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
return -ENXIO;
}
+ pdc_version = pdc_reg_read(PDC_VERSION, 0);
+
parent_domain = irq_find_host(parent);
if (!parent_domain) {
pr_err("%pOF: unable to find PDC's parent domain\n", node);
---
base-commit: 47d9bb711707d15b19fad18c8e2b4b027a264a3a
change-id: 20230821-topic-sm8x50-upstream-pdc-ver-114ceb45e1ee
Best regards,
--
Neil Armstrong <neil.armstrong@linaro.org>On Wed, 23 Aug 2023 at 12:49, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> Starting from HW version 3.2 the IRQ_ENABLE bit has moved to the
> IRQ_i_CFG register and requires a change of the driver to avoid
> writing into an undefined register address.
>
> Get the HW version from registers and set the IRQ_ENABLE bit to the
> correct register depending on the HW version.
>
> Reviewed-by: Maulik Shah <quic_mkshah@quicinc.com>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> Changes in v3:
> - Simplify qcom_pdc_gic_set_type()
> - Used __assign_bit in pdc_setup_pin_mapping()
> - remove BIT() from IRQ_i_CFG_IRQ_ENABLE to be used with __assign_bit()
> - Add Reviewed-by tag
> - Link to v2: https://lore.kernel.org/r/20230822-topic-sm8x50-upstream-pdc-ver-v2-1-3035b8d388f7@linaro.org
>
> Changes in v2:
> - Changed IRQ_ENABLE handling based on Maulik's comments
> - Link to v1: https://lore.kernel.org/r/20230821-topic-sm8x50-upstream-pdc-ver-v1-1-6d7f4dd95719@linaro.org
> ---
> drivers/irqchip/qcom-pdc.c | 61 ++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 48 insertions(+), 13 deletions(-)
This patch in linux-next broke sm8150. On that platform the PDC region
has size 0x400, so reading the version crashes the kernel.
I'll send a patch fixing device tree, but we'd still need to handle
this in a driver too.
>
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index d96916cf6a41..5f60a23686c9 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -23,9 +23,22 @@
>
> #define PDC_MAX_GPIO_IRQS 256
>
> +/* Valid only on HW version < 3.2 */
> #define IRQ_ENABLE_BANK 0x10
> #define IRQ_i_CFG 0x110
>
> +/* Valid only on HW version >= 3.2 */
> +#define IRQ_i_CFG_IRQ_ENABLE 3
> +
> +#define IRQ_i_CFG_TYPE_MASK GENMASK(2, 0)
> +
> +#define PDC_VERSION 0x1000
> +
> +/* Notable PDC versions */
> +enum {
> + PDC_VERSION_3_2 = 0x30200,
> +};
> +
> struct pdc_pin_region {
> u32 pin_base;
> u32 parent_base;
> @@ -38,6 +51,7 @@ static DEFINE_RAW_SPINLOCK(pdc_lock);
> static void __iomem *pdc_base;
> static struct pdc_pin_region *pdc_region;
> static int pdc_region_cnt;
> +static unsigned int pdc_version;
>
> static void pdc_reg_write(int reg, u32 i, u32 val)
> {
> @@ -54,15 +68,22 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
> int pin_out = d->hwirq;
> unsigned long enable;
> unsigned long flags;
> - u32 index, mask;
> -
> - index = pin_out / 32;
> - mask = pin_out % 32;
>
> raw_spin_lock_irqsave(&pdc_lock, flags);
> - enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
> - __assign_bit(mask, &enable, on);
> - pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
> + if (pdc_version < PDC_VERSION_3_2) {
> + u32 index, mask;
> +
> + index = pin_out / 32;
> + mask = pin_out % 32;
> +
> + enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
> + __assign_bit(mask, &enable, on);
> + 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);
> + pdc_reg_write(IRQ_i_CFG, pin_out, enable);
> + }
> raw_spin_unlock_irqrestore(&pdc_lock, flags);
> }
>
> @@ -143,6 +164,7 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
> }
>
> old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
> + pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK);
> pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>
> ret = irq_chip_set_type_parent(d, type);
> @@ -247,7 +269,7 @@ static const struct irq_domain_ops qcom_pdc_ops = {
> static int pdc_setup_pin_mapping(struct device_node *np)
> {
> int ret, n, i;
> - u32 irq_index, reg_index, val;
> + unsigned long val;
>
> n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
> if (n <= 0 || n % 3)
> @@ -278,11 +300,22 @@ static int pdc_setup_pin_mapping(struct device_node *np)
> return ret;
>
> for (i = 0; i < pdc_region[n].cnt; i++) {
> - reg_index = (i + pdc_region[n].pin_base) >> 5;
> - irq_index = (i + pdc_region[n].pin_base) & 0x1f;
> - val = pdc_reg_read(IRQ_ENABLE_BANK, reg_index);
> - val &= ~BIT(irq_index);
> - pdc_reg_write(IRQ_ENABLE_BANK, reg_index, val);
> + if (pdc_version < PDC_VERSION_3_2) {
> + u32 irq_index, reg_index;
> +
> + reg_index = (i + pdc_region[n].pin_base) >> 5;
> + irq_index = (i + pdc_region[n].pin_base) & 0x1f;
> + val = pdc_reg_read(IRQ_ENABLE_BANK, reg_index);
> + __assign_bit(irq_index, &val, 0);
> + pdc_reg_write(IRQ_ENABLE_BANK, reg_index, val);
> + } else {
> + u32 irq;
> +
> + irq = i + pdc_region[n].pin_base;
> + val = pdc_reg_read(IRQ_i_CFG, irq);
> + __assign_bit(IRQ_i_CFG_IRQ_ENABLE, &val, 0);
> + pdc_reg_write(IRQ_i_CFG, irq, val);
> + }
> }
> }
>
> @@ -300,6 +333,8 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
> return -ENXIO;
> }
>
> + pdc_version = pdc_reg_read(PDC_VERSION, 0);
> +
> parent_domain = irq_find_host(parent);
> if (!parent_domain) {
> pr_err("%pOF: unable to find PDC's parent domain\n", node);
>
> ---
> base-commit: 47d9bb711707d15b19fad18c8e2b4b027a264a3a
> change-id: 20230821-topic-sm8x50-upstream-pdc-ver-114ceb45e1ee
>
> Best regards,
> --
> Neil Armstrong <neil.armstrong@linaro.org>
>
--
With best wishes
Dmitry
On Fri, 25 Aug 2023 19:43:34 +0100, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Wed, 23 Aug 2023 at 12:49, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > > > Starting from HW version 3.2 the IRQ_ENABLE bit has moved to the > > IRQ_i_CFG register and requires a change of the driver to avoid > > writing into an undefined register address. > > > > Get the HW version from registers and set the IRQ_ENABLE bit to the > > correct register depending on the HW version. > > > > Reviewed-by: Maulik Shah <quic_mkshah@quicinc.com> > > Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> > > --- > > Changes in v3: > > - Simplify qcom_pdc_gic_set_type() > > - Used __assign_bit in pdc_setup_pin_mapping() > > - remove BIT() from IRQ_i_CFG_IRQ_ENABLE to be used with __assign_bit() > > - Add Reviewed-by tag > > - Link to v2: https://lore.kernel.org/r/20230822-topic-sm8x50-upstream-pdc-ver-v2-1-3035b8d388f7@linaro.org > > > > Changes in v2: > > - Changed IRQ_ENABLE handling based on Maulik's comments > > - Link to v1: https://lore.kernel.org/r/20230821-topic-sm8x50-upstream-pdc-ver-v1-1-6d7f4dd95719@linaro.org > > --- > > drivers/irqchip/qcom-pdc.c | 61 ++++++++++++++++++++++++++++++++++++---------- > > 1 file changed, 48 insertions(+), 13 deletions(-) > > This patch in linux-next broke sm8150. On that platform the PDC region > has size 0x400, so reading the version crashes the kernel. > I'll send a patch fixing device tree, but we'd still need to handle > this in a driver too. Thanks for reporting the breakage. Given that this has direct platform implications, I've dropped the patch from my tree until people figure out whether the proposed fix is the correct one and take the time it takes to test it. Please work with Neil to post an updated series that contains the fixed support for v3.2 and the DT churn. Thanks, M. -- Without deviation from the norm, progress is not possible.
Hi, On 26/08/2023 12:09, Marc Zyngier wrote: > On Fri, 25 Aug 2023 19:43:34 +0100, > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: >> >> On Wed, 23 Aug 2023 at 12:49, Neil Armstrong <neil.armstrong@linaro.org> wrote: >>> >>> Starting from HW version 3.2 the IRQ_ENABLE bit has moved to the >>> IRQ_i_CFG register and requires a change of the driver to avoid >>> writing into an undefined register address. >>> >>> Get the HW version from registers and set the IRQ_ENABLE bit to the >>> correct register depending on the HW version. >>> >>> Reviewed-by: Maulik Shah <quic_mkshah@quicinc.com> >>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>> --- >>> Changes in v3: >>> - Simplify qcom_pdc_gic_set_type() >>> - Used __assign_bit in pdc_setup_pin_mapping() >>> - remove BIT() from IRQ_i_CFG_IRQ_ENABLE to be used with __assign_bit() >>> - Add Reviewed-by tag >>> - Link to v2: https://lore.kernel.org/r/20230822-topic-sm8x50-upstream-pdc-ver-v2-1-3035b8d388f7@linaro.org >>> >>> Changes in v2: >>> - Changed IRQ_ENABLE handling based on Maulik's comments >>> - Link to v1: https://lore.kernel.org/r/20230821-topic-sm8x50-upstream-pdc-ver-v1-1-6d7f4dd95719@linaro.org >>> --- >>> drivers/irqchip/qcom-pdc.c | 61 ++++++++++++++++++++++++++++++++++++---------- >>> 1 file changed, 48 insertions(+), 13 deletions(-) >> >> This patch in linux-next broke sm8150. On that platform the PDC region >> has size 0x400, so reading the version crashes the kernel. >> I'll send a patch fixing device tree, but we'd still need to handle >> this in a driver too. > > Thanks for reporting the breakage. > > Given that this has direct platform implications, I've dropped the > patch from my tree until people figure out whether the proposed fix is > the correct one and take the time it takes to test it. > > Please work with Neil to post an updated series that contains the > fixed support for v3.2 and the DT churn. Thanks for dropping the change, Neil > > > Thanks, > > M. >
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: bc82cc42644b19a23567c2e06d084e3557ee2711
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/bc82cc42644b19a23567c2e06d084e3557ee2711
Author: Neil Armstrong <neil.armstrong@linaro.org>
AuthorDate: Wed, 23 Aug 2023 11:42:19 +02:00
Committer: Marc Zyngier <maz@kernel.org>
CommitterDate: Thu, 24 Aug 2023 16:24:58 +01:00
irqchip/qcom-pdc: Add support for v3.2 HW
Starting from HW version 3.2 the IRQ_ENABLE bit has moved to the
IRQ_i_CFG register and requires a change of the driver to avoid
writing into an undefined register address.
Get the HW version from registers and set the IRQ_ENABLE bit to the
correct register depending on the HW version.
Reviewed-by: Maulik Shah <quic_mkshah@quicinc.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20230823-topic-sm8x50-upstream-pdc-ver-v3-1-aa7d9ab862e4@linaro.org
---
drivers/irqchip/qcom-pdc.c | 61 +++++++++++++++++++++++++++++--------
1 file changed, 48 insertions(+), 13 deletions(-)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index a32c0d2..9bb6951 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -22,9 +22,22 @@
#define PDC_MAX_GPIO_IRQS 256
+/* Valid only on HW version < 3.2 */
#define IRQ_ENABLE_BANK 0x10
#define IRQ_i_CFG 0x110
+/* Valid only on HW version >= 3.2 */
+#define IRQ_i_CFG_IRQ_ENABLE 3
+
+#define IRQ_i_CFG_TYPE_MASK GENMASK(2, 0)
+
+#define PDC_VERSION 0x1000
+
+/* Notable PDC versions */
+enum {
+ PDC_VERSION_3_2 = 0x30200,
+};
+
struct pdc_pin_region {
u32 pin_base;
u32 parent_base;
@@ -37,6 +50,7 @@ static DEFINE_RAW_SPINLOCK(pdc_lock);
static void __iomem *pdc_base;
static struct pdc_pin_region *pdc_region;
static int pdc_region_cnt;
+static unsigned int pdc_version;
static void pdc_reg_write(int reg, u32 i, u32 val)
{
@@ -53,15 +67,22 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
int pin_out = d->hwirq;
unsigned long enable;
unsigned long flags;
- u32 index, mask;
-
- index = pin_out / 32;
- mask = pin_out % 32;
raw_spin_lock_irqsave(&pdc_lock, flags);
- enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
- __assign_bit(mask, &enable, on);
- pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
+ if (pdc_version < PDC_VERSION_3_2) {
+ u32 index, mask;
+
+ index = pin_out / 32;
+ mask = pin_out % 32;
+
+ enable = pdc_reg_read(IRQ_ENABLE_BANK, index);
+ __assign_bit(mask, &enable, on);
+ 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);
+ pdc_reg_write(IRQ_i_CFG, pin_out, enable);
+ }
raw_spin_unlock_irqrestore(&pdc_lock, flags);
}
@@ -142,6 +163,7 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
}
old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
+ pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK);
pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
ret = irq_chip_set_type_parent(d, type);
@@ -246,7 +268,7 @@ static const struct irq_domain_ops qcom_pdc_ops = {
static int pdc_setup_pin_mapping(struct device_node *np)
{
int ret, n, i;
- u32 irq_index, reg_index, val;
+ unsigned long val;
n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
if (n <= 0 || n % 3)
@@ -277,11 +299,22 @@ static int pdc_setup_pin_mapping(struct device_node *np)
return ret;
for (i = 0; i < pdc_region[n].cnt; i++) {
- reg_index = (i + pdc_region[n].pin_base) >> 5;
- irq_index = (i + pdc_region[n].pin_base) & 0x1f;
- val = pdc_reg_read(IRQ_ENABLE_BANK, reg_index);
- val &= ~BIT(irq_index);
- pdc_reg_write(IRQ_ENABLE_BANK, reg_index, val);
+ if (pdc_version < PDC_VERSION_3_2) {
+ u32 irq_index, reg_index;
+
+ reg_index = (i + pdc_region[n].pin_base) >> 5;
+ irq_index = (i + pdc_region[n].pin_base) & 0x1f;
+ val = pdc_reg_read(IRQ_ENABLE_BANK, reg_index);
+ __assign_bit(irq_index, &val, 0);
+ pdc_reg_write(IRQ_ENABLE_BANK, reg_index, val);
+ } else {
+ u32 irq;
+
+ irq = i + pdc_region[n].pin_base;
+ val = pdc_reg_read(IRQ_i_CFG, irq);
+ __assign_bit(IRQ_i_CFG_IRQ_ENABLE, &val, 0);
+ pdc_reg_write(IRQ_i_CFG, irq, val);
+ }
}
}
@@ -299,6 +332,8 @@ static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
return -ENXIO;
}
+ pdc_version = pdc_reg_read(PDC_VERSION, 0);
+
parent_domain = irq_find_host(parent);
if (!parent_domain) {
pr_err("%pOF: unable to find PDC's parent domain\n", node);© 2016 - 2025 Red Hat, Inc.