Re: FW: [PATCH] pinctrl: qcom: Add intr_target_width to define intr_target_bit field width
From: Andrew Halaney
Date: Fri Jul 14 2023 - 18:40:12 EST
On Fri, Jul 14, 2023 at 02:04:05PM -0700, Prasad Sodagudi wrote:
>
>
> > -----Original Message-----
> > From: Andrew Halaney <ahalaney@xxxxxxxxxx>
> > Sent: Friday, July 14, 2023 11:17 AM
> > To: Ninad Naik (QUIC) <quic_ninanaik@xxxxxxxxxxx>
> > Cc: andersson@xxxxxxxxxx; agross@xxxxxxxxxx; konrad.dybcio@xxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Parikshit Pareek (QUIC) <quic_ppareek@xxxxxxxxxxx>; Prasad Sodagudi <psodagud@xxxxxxxxxxx>; Prasanna Kumar (QUIC) <quic_kprasan@xxxxxxxxxxx>
> > 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@xxxxxxxxxxx>
Tested-by: Andrew Halaney <ahalaney@xxxxxxxxxx> # 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@xxxxxxxxxx>
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@xxxxxxxxxx>
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