drivers/irqchip/qcom-pdc.c | 73 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 15 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.
Signed-off-by: Neil Armstrong <neil.armstrong@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 | 73 ++++++++++++++++++++++++++++++++++++----------
1 file changed, 58 insertions(+), 15 deletions(-)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index d96916cf6a41..010a499ecd88 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 BIT(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);
}
@@ -142,8 +163,17 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
return -EINVAL;
}
- old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
- pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
+ if (pdc_version < PDC_VERSION_3_2) {
+ old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
+ pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
+ } else {
+ u32 val;
+
+ val = pdc_reg_read(IRQ_i_CFG, d->hwirq);
+ old_pdc_type = val & IRQ_i_CFG_TYPE_MASK;
+ pdc_reg_write(IRQ_i_CFG, d->hwirq,
+ pdc_type | (val & IRQ_i_CFG_IRQ_ENABLE));
+ }
ret = irq_chip_set_type_parent(d, type);
if (ret)
@@ -247,7 +277,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;
+ u32 val;
n = of_property_count_elems_of_size(np, "qcom,pdc-ranges", sizeof(u32));
if (n <= 0 || n % 3)
@@ -278,11 +308,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);
+ val &= ~BIT(irq_index);
+ 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);
+ val &= ~IRQ_i_CFG_IRQ_ENABLE;
+ pdc_reg_write(IRQ_i_CFG, irq, val);
+ }
}
}
@@ -300,6 +341,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>Hi Neil,
@@ -142,8 +163,17 @@ static int qcom_pdc_gic_set_type(struct irq_data
*d, unsigned int type)
> return -EINVAL;
> }
>
> - old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
> - pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
> + if (pdc_version < PDC_VERSION_3_2) {
> + old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
> + pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
> + } else {
> + u32 val;
> +
> + val = pdc_reg_read(IRQ_i_CFG, d->hwirq);
> + old_pdc_type = val & IRQ_i_CFG_TYPE_MASK;
> + pdc_reg_write(IRQ_i_CFG, d->hwirq,
> + pdc_type | (val & IRQ_i_CFG_IRQ_ENABLE));
> + }
While above is correct, i don't think we need version check in
qcom_pdc_gic_set_type() as bits 0-2 are always for the type in old/new
version as mentioned in v1.
Adding one line after reading old_pdc_type should be good enough.
old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
+ pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK);
> + 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);
> + val &= ~BIT(irq_index);
can use __assign_bit(irq_index, &val, 0); here.
> + 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);
> + val &= ~IRQ_i_CFG_IRQ_ENABLE;
__assign_bit(IRQ_i_CFG_IRQ_ENABLE, &val, 0); here.
other than this..
Reviewed-by: Maulik Shah <quic_mkshah@quicinc.com>
Thanks,
Maulik
Hi,
On 23/08/2023 07:35, Maulik Shah (mkshah) wrote:
> Hi Neil,
>
> @@ -142,8 +163,17 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>> return -EINVAL;
>> }
>> - old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>> - pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>> + if (pdc_version < PDC_VERSION_3_2) {
>> + old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>> + pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>> + } else {
>> + u32 val;
>> +
>> + val = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>> + old_pdc_type = val & IRQ_i_CFG_TYPE_MASK;
>> + pdc_reg_write(IRQ_i_CFG, d->hwirq,
>> + pdc_type | (val & IRQ_i_CFG_IRQ_ENABLE));
>> + }
> While above is correct, i don't think we need version check in qcom_pdc_gic_set_type() as bits 0-2 are always for the type in old/new version as mentioned in v1.
>
> Adding one line after reading old_pdc_type should be good enough.
Yes I understood, but while looking at the IRQ_i_CFG bits, I wanted to keep the original
driver behavior intact by setting remaining bits to 0.
Adding this single line changes that behavior and keeps bits 3-31
to the default register value, which may have some consequences.
If you consider it's an ok change, then I'll reduce it to this single line.
>
> old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
> + pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK);
>> + 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);
>> + val &= ~BIT(irq_index);
> can use __assign_bit(irq_index, &val, 0); here.
>> + 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);
>> + val &= ~IRQ_i_CFG_IRQ_ENABLE;
>
> __assign_bit(IRQ_i_CFG_IRQ_ENABLE, &val, 0); here.
Ack I'll update those.
>
> other than this..
> Reviewed-by: Maulik Shah <quic_mkshah@quicinc.com>
>
> Thanks,
> Maulik
>
Thanks,
Neil
Hi,
On 8/23/2023 1:16 PM, Neil Armstrong wrote:
> Hi,
>
> On 23/08/2023 07:35, Maulik Shah (mkshah) wrote:
>> Hi Neil,
>>
>> @@ -142,8 +163,17 @@ static int qcom_pdc_gic_set_type(struct irq_data
>> *d, unsigned int type)
>>> return -EINVAL;
>>> }
>>> - old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>>> - pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>>> + if (pdc_version < PDC_VERSION_3_2) {
>>> + old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>>> + pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>>> + } else {
>>> + u32 val;
>>> +
>>> + val = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>>> + old_pdc_type = val & IRQ_i_CFG_TYPE_MASK;
>>> + pdc_reg_write(IRQ_i_CFG, d->hwirq,
>>> + pdc_type | (val & IRQ_i_CFG_IRQ_ENABLE));
>>> + }
>> While above is correct, i don't think we need version check in
>> qcom_pdc_gic_set_type() as bits 0-2 are always for the type in
>> old/new version as mentioned in v1.
>>
>> Adding one line after reading old_pdc_type should be good enough.
>
> Yes I understood, but while looking at the IRQ_i_CFG bits, I wanted to
> keep the original
> driver behavior intact by setting remaining bits to 0.
>
> Adding this single line changes that behavior and keeps bits 3-31
> to the default register value, which may have some consequences.
>
> If you consider it's an ok change, then I'll reduce it to this single
> line.
Yes this ok change to have single line and should not have any
consequences.
Thanks,
Maulik
On 23/08/2023 10:25, Maulik Shah (mkshah) wrote:
> Hi,
>
> On 8/23/2023 1:16 PM, Neil Armstrong wrote:
>> Hi,
>>
>> On 23/08/2023 07:35, Maulik Shah (mkshah) wrote:
>>> Hi Neil,
>>>
>>> @@ -142,8 +163,17 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>>>> return -EINVAL;
>>>> }
>>>> - old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>>>> - pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>>>> + if (pdc_version < PDC_VERSION_3_2) {
>>>> + old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>>>> + pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>>>> + } else {
>>>> + u32 val;
>>>> +
>>>> + val = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>>>> + old_pdc_type = val & IRQ_i_CFG_TYPE_MASK;
>>>> + pdc_reg_write(IRQ_i_CFG, d->hwirq,
>>>> + pdc_type | (val & IRQ_i_CFG_IRQ_ENABLE));
>>>> + }
>>> While above is correct, i don't think we need version check in qcom_pdc_gic_set_type() as bits 0-2 are always for the type in old/new version as mentioned in v1.
>>>
>>> Adding one line after reading old_pdc_type should be good enough.
>>
>> Yes I understood, but while looking at the IRQ_i_CFG bits, I wanted to keep the original
>> driver behavior intact by setting remaining bits to 0.
>>
>> Adding this single line changes that behavior and keeps bits 3-31
>> to the default register value, which may have some consequences.
>>
>> If you consider it's an ok change, then I'll reduce it to this single line.
> Yes this ok change to have single line and should not have any consequences.
I also remember why, it's about the final check:
184 if (old_pdc_type != pdc_type)
185 irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
We need to strip out remaining bits of old_pdc_type of this won't work as
expected, so I'll change it to :
+ old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
+ pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK);
+ old_pdc_type &= IRQ_i_CFG_TYPE_MASK;
+ pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
Is it ok for you ?
Thanks,
Neil
>
> Thanks,
> Maulik
>
Hi Neil,
On 8/23/2023 2:21 PM, neil.armstrong@linaro.org wrote:
> On 23/08/2023 10:25, Maulik Shah (mkshah) wrote:
>> Hi,
>>
>> On 8/23/2023 1:16 PM, Neil Armstrong wrote:
>>> Hi,
>>>
>>> On 23/08/2023 07:35, Maulik Shah (mkshah) wrote:
>>>> Hi Neil,
>>>>
>>>> @@ -142,8 +163,17 @@ static int qcom_pdc_gic_set_type(struct
>>>> irq_data *d, unsigned int type)
>>>>> return -EINVAL;
>>>>> }
>>>>> - old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>>>>> - pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>>>>> + if (pdc_version < PDC_VERSION_3_2) {
>>>>> + old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>>>>> + pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>>>>> + } else {
>>>>> + u32 val;
>>>>> +
>>>>> + val = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>>>>> + old_pdc_type = val & IRQ_i_CFG_TYPE_MASK;
>>>>> + pdc_reg_write(IRQ_i_CFG, d->hwirq,
>>>>> + pdc_type | (val & IRQ_i_CFG_IRQ_ENABLE));
>>>>> + }
>>>> While above is correct, i don't think we need version check in
>>>> qcom_pdc_gic_set_type() as bits 0-2 are always for the type in
>>>> old/new version as mentioned in v1.
>>>>
>>>> Adding one line after reading old_pdc_type should be good enough.
>>>
>>> Yes I understood, but while looking at the IRQ_i_CFG bits, I wanted
>>> to keep the original
>>> driver behavior intact by setting remaining bits to 0.
>>>
>>> Adding this single line changes that behavior and keeps bits 3-31
>>> to the default register value, which may have some consequences.
>>>
>>> If you consider it's an ok change, then I'll reduce it to this
>>> single line.
>> Yes this ok change to have single line and should not have any
>> consequences.
>
> I also remember why, it's about the final check:
>
> 184 if (old_pdc_type != pdc_type)
> 185 irq_chip_set_parent_state(d,
> IRQCHIP_STATE_PENDING, false);
>
> We need to strip out remaining bits of old_pdc_type of this won't work as
> expected, so I'll change it to :
>
> + old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
> + pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK);
> + old_pdc_type &= IRQ_i_CFG_TYPE_MASK;
> + pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>
> Is it ok for you ?
No.
old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
+ pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK);
Adding above suggested single line is sufficient to make final check
properly compare both old_pdc_type and new pdc_type, right?
But with your above change, It will end up comparing only bits 0-2 of
old_pdc_type with updated pdc_type (which just got the other bits (3 to
31) of IRQ_i_CFG register by the ORing it with old_pdc_type).
Thanks,
Maulik
On 23/08/2023 11:19, Maulik Shah (mkshah) wrote:
> Hi Neil,
>
> On 8/23/2023 2:21 PM, neil.armstrong@linaro.org wrote:
>> On 23/08/2023 10:25, Maulik Shah (mkshah) wrote:
>>> Hi,
>>>
>>> On 8/23/2023 1:16 PM, Neil Armstrong wrote:
>>>> Hi,
>>>>
>>>> On 23/08/2023 07:35, Maulik Shah (mkshah) wrote:
>>>>> Hi Neil,
>>>>>
>>>>> @@ -142,8 +163,17 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
>>>>>> return -EINVAL;
>>>>>> }
>>>>>> - old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>>>>>> - pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>>>>>> + if (pdc_version < PDC_VERSION_3_2) {
>>>>>> + old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>>>>>> + pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>>>>>> + } else {
>>>>>> + u32 val;
>>>>>> +
>>>>>> + val = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>>>>>> + old_pdc_type = val & IRQ_i_CFG_TYPE_MASK;
>>>>>> + pdc_reg_write(IRQ_i_CFG, d->hwirq,
>>>>>> + pdc_type | (val & IRQ_i_CFG_IRQ_ENABLE));
>>>>>> + }
>>>>> While above is correct, i don't think we need version check in qcom_pdc_gic_set_type() as bits 0-2 are always for the type in old/new version as mentioned in v1.
>>>>>
>>>>> Adding one line after reading old_pdc_type should be good enough.
>>>>
>>>> Yes I understood, but while looking at the IRQ_i_CFG bits, I wanted to keep the original
>>>> driver behavior intact by setting remaining bits to 0.
>>>>
>>>> Adding this single line changes that behavior and keeps bits 3-31
>>>> to the default register value, which may have some consequences.
>>>>
>>>> If you consider it's an ok change, then I'll reduce it to this single line.
>>> Yes this ok change to have single line and should not have any consequences.
>>
>> I also remember why, it's about the final check:
>>
>> 184 if (old_pdc_type != pdc_type)
>> 185 irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, false);
>>
>> We need to strip out remaining bits of old_pdc_type of this won't work as
>> expected, so I'll change it to :
>>
>> + old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>> + pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK);
>> + old_pdc_type &= IRQ_i_CFG_TYPE_MASK;
>> + pdc_reg_write(IRQ_i_CFG, d->hwirq, pdc_type);
>>
>> Is it ok for you ?
>
> No.
>
> old_pdc_type = pdc_reg_read(IRQ_i_CFG, d->hwirq);
>
> + pdc_type |= (old_pdc_type & ~IRQ_i_CFG_TYPE_MASK);
>
> Adding above suggested single line is sufficient to make final check properly compare both old_pdc_type and new pdc_type, right?
>
> But with your above change, It will end up comparing only bits 0-2 of old_pdc_type with updated pdc_type (which just got the other bits (3 to 31) of IRQ_i_CFG register by the ORing it with old_pdc_type).
Oh yeah indeed it's right, I had my previous code in mind.
I'll stick with the single line then,
Thanks,
Neil
>
> Thanks,
> Maulik
© 2016 - 2025 Red Hat, Inc.