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