SA8775 and newer target have added support for an increased number of
interrupt targets. To implement this change, the intr_target field, which
is used to configure the interrupt target in the interrupt configuration
register is increased from 3 bits to 4 bits.
In accordance to these updates, a new intr_target_width member is
introduced in msm_pingroup structure. This member stores the value of
width of intr_target field in the interrupt configuration register. This
value is used to dynamically calculate and generate mask for setting the
intr_target field. By default, this mask is set to 3 bit wide, to ensure
backward compatibility with the older targets.
Signed-off-by: Ninad Naik <[email protected]>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 9 ++++++---
drivers/pinctrl/qcom/pinctrl-msm.h | 2 ++
drivers/pinctrl/qcom/pinctrl-sa8775p.c | 1 +
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 2585ef2b2793..6ebcaa2220af 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1038,6 +1038,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
const struct msm_pingroup *g;
+ u32 intr_target_mask = 0x7;
unsigned long flags;
bool was_enabled;
u32 val;
@@ -1074,13 +1075,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
* With intr_target_use_scm interrupts are routed to
* application cpu using scm calls.
*/
+ if (g->intr_target_width)
+ intr_target_mask = GENMASK(g->intr_target_width - 1, 0);
+
if (pctrl->intr_target_use_scm) {
u32 addr = pctrl->phys_base[0] + g->intr_target_reg;
int ret;
qcom_scm_io_readl(addr, &val);
-
- val &= ~(7 << g->intr_target_bit);
+ val &= ~(intr_target_mask << g->intr_target_bit);
val |= g->intr_target_kpss_val << g->intr_target_bit;
ret = qcom_scm_io_writel(addr, val);
@@ -1090,7 +1093,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
d->hwirq);
} else {
val = msm_readl_intr_target(pctrl, g);
- val &= ~(7 << g->intr_target_bit);
+ val &= ~(intr_target_mask << g->intr_target_bit);
val |= g->intr_target_kpss_val << g->intr_target_bit;
msm_writel_intr_target(val, pctrl, g);
}
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index 5e4410bed823..1d2f2e904da1 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -59,6 +59,7 @@ struct pinctrl_pin_desc;
* @intr_status_bit: Offset in @intr_status_reg for reading and acking the interrupt
* status.
* @intr_target_bit: Offset in @intr_target_reg for configuring the interrupt routing.
+ * @intr_target_width: Number of bits used for specifying interrupt routing target.
* @intr_target_kpss_val: Value in @intr_target_bit for specifying that the interrupt from
* this gpio should get routed to the KPSS processor.
* @intr_raw_status_bit: Offset in @intr_cfg_reg for the raw status bit.
@@ -100,6 +101,7 @@ struct msm_pingroup {
unsigned intr_ack_high:1;
unsigned intr_target_bit:5;
+ unsigned intr_target_width:5;
unsigned intr_target_kpss_val:5;
unsigned intr_raw_status_bit:5;
unsigned intr_polarity_bit:5;
diff --git a/drivers/pinctrl/qcom/pinctrl-sa8775p.c b/drivers/pinctrl/qcom/pinctrl-sa8775p.c
index 8a5cd15512b9..8fdea25d8d67 100644
--- a/drivers/pinctrl/qcom/pinctrl-sa8775p.c
+++ b/drivers/pinctrl/qcom/pinctrl-sa8775p.c
@@ -46,6 +46,7 @@
.intr_enable_bit = 0, \
.intr_status_bit = 0, \
.intr_target_bit = 5, \
+ .intr_target_width = 4, \
.intr_target_kpss_val = 3, \
.intr_raw_status_bit = 4, \
.intr_polarity_bit = 1, \
--
2.41.0
On 14.07.2023 08:10, Ninad Naik wrote:
> SA8775 and newer target have added support for an increased number of
> interrupt targets. To implement this change, the intr_target field, which
> is used to configure the interrupt target in the interrupt configuration
> register is increased from 3 bits to 4 bits.
>
> In accordance to these updates, a new intr_target_width member is
> introduced in msm_pingroup structure. This member stores the value of
> width of intr_target field in the interrupt configuration register. This
> value is used to dynamically calculate and generate mask for setting the
> intr_target field. By default, this mask is set to 3 bit wide, to ensure
> backward compatibility with the older targets.
>
> Signed-off-by: Ninad Naik <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>
one nit below
> drivers/pinctrl/qcom/pinctrl-msm.c | 9 ++++++---
> drivers/pinctrl/qcom/pinctrl-msm.h | 2 ++
> drivers/pinctrl/qcom/pinctrl-sa8775p.c | 1 +
> 3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 2585ef2b2793..6ebcaa2220af 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -1038,6 +1038,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> const struct msm_pingroup *g;
> + u32 intr_target_mask = 0x7;
This could be GENMASK(2, 0)
Konrad
> unsigned long flags;
> bool was_enabled;
> u32 val;
> @@ -1074,13 +1075,15 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> * With intr_target_use_scm interrupts are routed to
> * application cpu using scm calls.
> */
> + if (g->intr_target_width)
> + intr_target_mask = GENMASK(g->intr_target_width - 1, 0);
> +
> if (pctrl->intr_target_use_scm) {
> u32 addr = pctrl->phys_base[0] + g->intr_target_reg;
> int ret;
>
> qcom_scm_io_readl(addr, &val);
> -
> - val &= ~(7 << g->intr_target_bit);
> + val &= ~(intr_target_mask << g->intr_target_bit);
> val |= g->intr_target_kpss_val << g->intr_target_bit;
>
> ret = qcom_scm_io_writel(addr, val);
> @@ -1090,7 +1093,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> d->hwirq);
> } else {
> val = msm_readl_intr_target(pctrl, g);
> - val &= ~(7 << g->intr_target_bit);
> + val &= ~(intr_target_mask << g->intr_target_bit);
> val |= g->intr_target_kpss_val << g->intr_target_bit;
> msm_writel_intr_target(val, pctrl, g);
> }
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index 5e4410bed823..1d2f2e904da1 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -59,6 +59,7 @@ struct pinctrl_pin_desc;
> * @intr_status_bit: Offset in @intr_status_reg for reading and acking the interrupt
> * status.
> * @intr_target_bit: Offset in @intr_target_reg for configuring the interrupt routing.
> + * @intr_target_width: Number of bits used for specifying interrupt routing target.
> * @intr_target_kpss_val: Value in @intr_target_bit for specifying that the interrupt from
> * this gpio should get routed to the KPSS processor.
> * @intr_raw_status_bit: Offset in @intr_cfg_reg for the raw status bit.
> @@ -100,6 +101,7 @@ struct msm_pingroup {
> unsigned intr_ack_high:1;
>
> unsigned intr_target_bit:5;
> + unsigned intr_target_width:5;
> unsigned intr_target_kpss_val:5;
> unsigned intr_raw_status_bit:5;
> unsigned intr_polarity_bit:5;
> diff --git a/drivers/pinctrl/qcom/pinctrl-sa8775p.c b/drivers/pinctrl/qcom/pinctrl-sa8775p.c
> index 8a5cd15512b9..8fdea25d8d67 100644
> --- a/drivers/pinctrl/qcom/pinctrl-sa8775p.c
> +++ b/drivers/pinctrl/qcom/pinctrl-sa8775p.c
> @@ -46,6 +46,7 @@
> .intr_enable_bit = 0, \
> .intr_status_bit = 0, \
> .intr_target_bit = 5, \
> + .intr_target_width = 4, \
> .intr_target_kpss_val = 3, \
> .intr_raw_status_bit = 4, \
> .intr_polarity_bit = 1, \
On Fri, Jul 14, 2023 at 11:40:09AM +0530, Ninad Naik wrote:
> SA8775 and newer target have added support for an increased number of
> interrupt targets. To implement this change, the intr_target field, which
> is used to configure the interrupt target in the interrupt configuration
> register is increased from 3 bits to 4 bits.
>
> In accordance to these updates, a new intr_target_width member is
> introduced in msm_pingroup structure. This member stores the value of
> width of intr_target field in the interrupt configuration register. This
> value is used to dynamically calculate and generate mask for setting the
> intr_target field. By default, this mask is set to 3 bit wide, to ensure
> backward compatibility with the older targets.
>
> Signed-off-by: Ninad Naik <[email protected]>
Thanks for the patch. Naive question (without really reading the code),
but what practical affect does this have?
i.e. does this change behavior of how IRQs were handled before this
patch vs after on this platform?
To shed some light on the question, there's a GPIO IRQ for the ethernet
phy on this platform that is purposely _not_ described because it didn't
ever trigger, resulting in the interface staying down. Things work
fine without the IRQ (the driver goes into polling mode).
The explanation I got was very brief and attributed it to a "hardware issue".
I'm wondering if I should re-evaluate that, and if this was the
"hardware issue".
Thanks,
Andrew
On Fri, Jul 14, 2023 at 01:17:12PM -0500, Andrew Halaney wrote:
> On Fri, Jul 14, 2023 at 11:40:09AM +0530, Ninad Naik wrote:
> > SA8775 and newer target have added support for an increased number of
> > interrupt targets. To implement this change, the intr_target field, which
> > is used to configure the interrupt target in the interrupt configuration
> > register is increased from 3 bits to 4 bits.
> >
> > In accordance to these updates, a new intr_target_width member is
> > introduced in msm_pingroup structure. This member stores the value of
> > width of intr_target field in the interrupt configuration register. This
> > value is used to dynamically calculate and generate mask for setting the
> > intr_target field. By default, this mask is set to 3 bit wide, to ensure
> > backward compatibility with the older targets.
> >
> > Signed-off-by: Ninad Naik <[email protected]>
>
> Thanks for the patch. Naive question (without really reading the code),
> but what practical affect does this have?
>
> i.e. does this change behavior of how IRQs were handled before this
> patch vs after on this platform?
>
Yes, the affected field denotes where interrupts should be routed and
without this patch not all the bits where updated.
> To shed some light on the question, there's a GPIO IRQ for the ethernet
> phy on this platform that is purposely _not_ described because it didn't
> ever trigger, resulting in the interface staying down. Things work
> fine without the IRQ (the driver goes into polling mode).
> The explanation I got was very brief and attributed it to a "hardware issue".
>
> I'm wondering if I should re-evaluate that, and if this was the
> "hardware issue".
>
It's plausible that your interrupt fired elsewhere. Definitely worth
giving that scenario another test run.
Regards,
Bjorn
On Fri, Jul 14, 2023 at 11:40:09AM +0530, Ninad Naik wrote:
> SA8775 and newer target have added support for an increased number of
> interrupt targets. To implement this change, the intr_target field, which
> is used to configure the interrupt target in the interrupt configuration
> register is increased from 3 bits to 4 bits.
>
> In accordance to these updates, a new intr_target_width member is
> introduced in msm_pingroup structure. This member stores the value of
> width of intr_target field in the interrupt configuration register. This
> value is used to dynamically calculate and generate mask for setting the
> intr_target field. By default, this mask is set to 3 bit wide, to ensure
> backward compatibility with the older targets.
>
> Signed-off-by: Ninad Naik <[email protected]>
Very nice, Ninad.
Reviewed-by: Bjorn Andersson <[email protected]>
> ---
> drivers/pinctrl/qcom/pinctrl-msm.c | 9 ++++++---
> drivers/pinctrl/qcom/pinctrl-msm.h | 2 ++
> drivers/pinctrl/qcom/pinctrl-sa8775p.c | 1 +
> 3 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 2585ef2b2793..6ebcaa2220af 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -1038,6 +1038,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
> const struct msm_pingroup *g;
> + u32 intr_target_mask = 0x7;
I like Konrad's suggestion about making this GENMASK(2, 0).
Please update that and include our R-b tags in v2.
Regards,
Bjorn
Hi,
Thank you all for the reviews.
On 7/15/2023 2:08 AM, Bjorn Andersson wrote:
> On Fri, Jul 14, 2023 at 11:40:09AM +0530, Ninad Naik wrote:
>> SA8775 and newer target have added support for an increased number of
>> interrupt targets. To implement this change, the intr_target field, which
>> is used to configure the interrupt target in the interrupt configuration
>> register is increased from 3 bits to 4 bits.
>>
>> In accordance to these updates, a new intr_target_width member is
>> introduced in msm_pingroup structure. This member stores the value of
>> width of intr_target field in the interrupt configuration register. This
>> value is used to dynamically calculate and generate mask for setting the
>> intr_target field. By default, this mask is set to 3 bit wide, to ensure
>> backward compatibility with the older targets.
>>
>> Signed-off-by: Ninad Naik <[email protected]>
>
> Very nice, Ninad.
>
> Reviewed-by: Bjorn Andersson <[email protected]>
>
>> ---
>> drivers/pinctrl/qcom/pinctrl-msm.c | 9 ++++++---
>> drivers/pinctrl/qcom/pinctrl-msm.h | 2 ++
>> drivers/pinctrl/qcom/pinctrl-sa8775p.c | 1 +
>> 3 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>> index 2585ef2b2793..6ebcaa2220af 100644
>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>> @@ -1038,6 +1038,7 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>> struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
>> const struct msm_pingroup *g;
>> + u32 intr_target_mask = 0x7;
>
> I like Konrad's suggestion about making this GENMASK(2, 0).
>
> Please update that and include our R-b tags in v2.
>
Sure, I'll change this to GENMASK and update all the relevant tags
(Fixes and R-b) as suggested in the review comments.
> Regards,
> Bjorn
Thanks a lot!
Regards,
Ninad