Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 14 Jul 2023 17:39:15 -0500
From:   Andrew Halaney <ahalaney@redhat.com>
To:     Prasad Sodagudi <quic_psodagud@quicinc.com>
Cc:     "Ninad Naik (QUIC)" <quic_ninanaik@quicinc.com>,
        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: FW: [PATCH] pinctrl: qcom: Add intr_target_width to define
 intr_target_bit field width
Message-ID: <20230714223915.4q3pmbqxajkjtltb@halaney-x13s>
References: <20230714061010.15817-1-quic_ninanaik@quicinc.com>
 <p5vyji2weclwgshyxbf2evzpzefckzyecjnoada7dvzbuenxsb@55abp36rrbub>
 <BY5PR02MB6339C6B8A53C5D0CB55E58B7C034A@BY5PR02MB6339.namprd02.prod.outlook.com>
 <d43a0ce0-c915-a5d2-1d01-549ae08c678e@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d43a0ce0-c915-a5d2-1d01-549ae08c678e@quicinc.com>
Precedence: bulk

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