diff mbox series

pinctrl: qcom: Add intr_target_width to define intr_target_bit field width

Message ID 20230714061010.15817-1-quic_ninanaik@quicinc.com
State New
Headers show
Series pinctrl: qcom: Add intr_target_width to define intr_target_bit field width | expand

Commit Message

Ninad Naik July 14, 2023, 6:10 a.m. UTC
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 <quic_ninanaik@quicinc.com>
---
 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(-)

Comments

Prasad Sodagudi July 14, 2023, 9:04 p.m. UTC | #1
> -----Original Message-----
> From: Andrew Halaney <ahalaney@redhat.com>
> Sent: Friday, July 14, 2023 11:17 AM
> To: Ninad Naik (QUIC) <quic_ninanaik@quicinc.com>
> Cc: andersson@kernel.org; agross@kernel.org; konrad.dybcio@linaro.org; linux-arm-msm@vger.kernel.org; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org; Parikshit Pareek (QUIC) <quic_ppareek@quicinc.com>; Prasad Sodagudi <psodagud@quicinc.com>; Prasanna Kumar (QUIC) <quic_kprasan@quicinc.com>
> Subject: Re: [PATCH] pinctrl: qcom: Add intr_target_width to define intr_target_bit field width
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> 
> 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 <quic_ninanaik@quicinc.com>
> 
> Thanks for the patch. Naive question (without really reading the code), but what practical affect does this have?
> 

Target bits configures irq destination processor on Qualcomm SoC. 
g->intr_target_kpss_val(0x3) routes the gpio IRQ to application process.
On this SoCs target bits length is changed from 3 bits to 4 bits in hw 
and reset value of g->intr_target_reg register value is 0x1E2. So reset 
value of target bits is 0xf.  With old logic, when writing 
g->intr_target_kpss_val(0x3) value result is 0xb instead of 0x3 as top 
most bit is not getting cleared out and leading to IRQ is not getting 
fired on application processor. 0xb value is unused on current HW and 
IRQ would not be fired.

> 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".

Hi Andrew,

I don't have complete idea on ethernet phy gpio irq issue and I will 
sync up with offline. Please try this patch for ethernet use case.

> 
> Thanks,
> Andrew
>
Andrew Halaney July 14, 2023, 10:39 p.m. UTC | #2
On Fri, Jul 14, 2023 at 02:04:05PM -0700, Prasad Sodagudi wrote:
> 
> 
> > -----Original Message-----
> > From: Andrew Halaney <ahalaney@redhat.com>
> > Sent: Friday, July 14, 2023 11:17 AM
> > To: Ninad Naik (QUIC) <quic_ninanaik@quicinc.com>
> > Cc: andersson@kernel.org; agross@kernel.org; konrad.dybcio@linaro.org; linux-arm-msm@vger.kernel.org; linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org; Parikshit Pareek (QUIC) <quic_ppareek@quicinc.com>; Prasad Sodagudi <psodagud@quicinc.com>; Prasanna Kumar (QUIC) <quic_kprasan@quicinc.com>
> > Subject: Re: [PATCH] pinctrl: qcom: Add intr_target_width to define intr_target_bit field width
> > 
> > WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros.
> > 
> > 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 <quic_ninanaik@quicinc.com>

Tested-by: Andrew Halaney <ahalaney@redhat.com> # sa8775p-ride

> > 
> > Thanks for the patch. Naive question (without really reading the code), but what practical affect does this have?
> > 
> 
> Target bits configures irq destination processor on Qualcomm SoC.
> g->intr_target_kpss_val(0x3) routes the gpio IRQ to application process.
> On this SoCs target bits length is changed from 3 bits to 4 bits in hw and
> reset value of g->intr_target_reg register value is 0x1E2. So reset value of
> target bits is 0xf.  With old logic, when writing
> g->intr_target_kpss_val(0x3) value result is 0xb instead of 0x3 as top most
> bit is not getting cleared out and leading to IRQ is not getting fired on
> application processor. 0xb value is unused on current HW and IRQ would not
> be fired.
> 

Thanks all for the explanations, that makes a lot of sense.

Perhaps briefly summarizing that without this fix platforms using
more than 3 bits could fail to set/clear the remaining bits, resulting in
routing the IRQ to the wrong processor subsystem?

And with that in mind.. I think this deserves a Fixes tag:

    Fixes: f365be092572 ("pinctrl: Add Qualcomm TLMM driver")

seems overzealous since until 8775p afaik no upstream platform would
fall in the > 3 bits category. But, it wouldn't hurt if someone was bringing
up such a platform on an LTS kernel that has this pinctrl driver.

At the very least I think:

    Fixes: 4b6b18559927 ("pinctrl: qcom: add the tlmm driver sa8775p platforms")

would be a nice addition in v2.

This definitely works, applying this patch to enable the IRQ for the ethernet
phy (posting in case I get hit by a bus this weekend, I'll spin properly
next week):

    ahalaney@halaney-x13s ~/git/linux-next (git)-[remotes/ahalaney/ethernet-phy-irq] % git show                                                                                                                                            :(
    commit c6d01507db84dcb205e2cd92fb74cdb328f6fcad (HEAD, ahalaney/ethernet-phy-irq)
    Author: Andrew Halaney <ahalaney@redhat.com>
    Date:   Fri Jul 14 16:07:01 2023 -0500

        arm64: dts: qcom: sa8775p-ride: Describe ethernet phy irq

        There's an irq hooked up, so let's describe it.

        Prior to TODO UPDATE WITH FINAL PATCH NAME
        SHA ("pinctrl: qcom: Add intr_target_width to define intr_target_bit field width")
        one would not see the IRQ fire, despite some (invasive) debugging
        showing that the GPIO was in fact asserted, resulting in the interface
        staying down.

        Now that the IRQ is properly routed we can describe it.

        Signed-off-by: Andrew Halaney <ahalaney@redhat.com>

    diff --git a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
    index b2aa16037707..2b7ef7ad01d5 100644
    --- a/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
    +++ b/arch/arm64/boot/dts/qcom/sa8775p-ride.dts
    @@ -286,6 +286,8 @@ mdio {
                    sgmii_phy: phy@8 {
                            reg = <0x8>;
                            device_type = "ethernet-phy";
    +
    +                       interrupts-extended = <&tlmm 7 IRQ_TYPE_EDGE_FALLING>;
                    };
            };



Then testing without/with this pinctrl change...

Without this pinctrl change:

    [root@dhcp19-243-201 ~]# # Ethernet MAC of question
    [root@dhcp19-243-201 ~]# ls -lah /sys/class/net/eth0
    lrwxrwxrwx 1 root root 0 May 18 00:00 /sys/class/net/eth0 -> ../../devices/platform/soc@0/23040000.ethernet/net/eth0
    [root@dhcp19-243-201 ~]# ip -s link show eth0
    2: eth0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc mq state DOWN mode DEFAULT group default qlen 1000
        link/ether b2:79:1f:47:c8:45 brd ff:ff:ff:ff:ff:ff
        RX:  bytes packets errors dropped  missed   mcast
                 0       0      0       0       0       0
        TX:  bytes packets errors dropped carrier collsns
                 0       0      0       0       0       0
    [root@dhcp19-243-201 ~]# cat /proc/interrupts | grep stmmac-0
    238:          0          0          0          0          0          0          0          0   msmgpio   7 Edge      stmmac-0:08
    [root@dhcp19-243-201 ~]#

With it:

    [root@dhcp19-243-28 ~]# ls -lah /sys/class/net/eth0
    lrwxrwxrwx 1 root root 0 May 18 00:00 /sys/class/net/eth0 -> ../../devices/platform/soc@0/23040000.ethernet/net/eth0
    [root@dhcp19-243-28 ~]# ip -s link show eth0
    2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP mode DEFAULT group default qlen 1000
        link/ether ae:27:eb:55:e4:d0 brd ff:ff:ff:ff:ff:ff
        RX:  bytes packets errors dropped  missed   mcast
            588222    9538      0      43       0       0
        TX:  bytes packets errors dropped carrier collsns
             16046     181      0       0       0       0
    [root@dhcp19-243-28 ~]# cat /proc/interrupts | grep stmmac-0
    237:          1          0          0          0          0          0          0          0   msmgpio   7 Edge      stmmac-0:08
    [root@dhcp19-243-28 ~]#

Boy can one IRQ and bit make a difference! I spent a few days trying to
understand why my forward port was failing, and this IRQ was the ultimate issue.
Thanks for the fix!

- Andrew
Ninad Naik July 18, 2023, 5:08 a.m. UTC | #3
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 <quic_ninanaik@quicinc.com>
> 
> Very nice, Ninad.
> 
> Reviewed-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> 
>> ---
>>   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
diff mbox series

Patch

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,		\