Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Wed, 13 Dec 2023 06:38:19 -0600
From:   Nishanth Menon <nm@ti.com>
To:     Siddharth Vadapalli <s-vadapalli@ti.com>
CC:     <vigneshr@ti.com>, <kristo@kernel.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <danishanwar@ti.com>,
        <r-gunasekaran@ti.com>, <srk@ti.com>
Subject: Re: [PATCH v2] arm64: dts: ti: k3-am654-icssg2: Enable PHY
 interrupts for ICSSG2
Message-ID: <20231213123819.tqh3lm2ceir3qjbk@swimmer>
References: <20231213080216.1710730-1-s-vadapalli@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20231213080216.1710730-1-s-vadapalli@ti.com>
Precedence: bulk

On 13:32-20231213, Siddharth Vadapalli wrote:
> Enable interrupt mode of operation of the DP83867 Ethernet PHY which is
> used by ICSSG2. The DP83867 PHY driver already supports interrupt handling
> for interrupts generated by the PHY. Thus, add the necessary device-tree
> support to enable it.
> 
> Since the GPIO1_87 line is muxed with EXT_REFCLK1 and SYNC1_OUT, update
> the pinmux to select GPIO1_87 for routing the interrupt.
> 
> As the same interrupt line and therefore the same pinmux configuration is
> applicable to both Ethernet PHYs used by ICSSG2, allocate the pinmux
> resource to the first Ethernet PHY alone.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> Reviewed-by: MD Danish Anwar <danishanwar@ti.com>
> ---
> Hello,
> 
> This patch is based on linux-next tagged next-20231213.
> 
> v1:
> https://lore.kernel.org/r/20231120063159.539306-1-s-vadapalli@ti.com/
> Changes since v1:
> - Rebased patch on next-20231213.
> - Collected Reviewed-by tag from
>   MD Danish Anwar <danishanwar@ti.com>
> - Moved pinctrl from MDIO node to Ethernet PHY node based on feedback from
>   Nishanth Menon <nm@ti.com>
> - Replaced the hard-coded value 0x2 with IRQ_TYPE_EDGE_FALLING for
>   setting the interrupt trigger type and level flag based on feedback from
>   Nishanth Menon <nm@ti.com>
> - Included dt-bindings/interrupt-controller/irq.h in the overlay.
> - Updated commit message with details of the pinmux resource allocation.
> 
> Regards,
> Siddharth.
> 
>  arch/arm64/boot/dts/ti/k3-am654-icssg2.dtso | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am654-icssg2.dtso b/arch/arm64/boot/dts/ti/k3-am654-icssg2.dtso
> index ec8cf20ca3ac..6eabdfa0d602 100644
> --- a/arch/arm64/boot/dts/ti/k3-am654-icssg2.dtso
> +++ b/arch/arm64/boot/dts/ti/k3-am654-icssg2.dtso
> @@ -8,6 +8,7 @@
>  /dts-v1/;
>  /plugin/;
>  
> +#include <dt-bindings/interrupt-controller/irq.h>
>  #include <dt-bindings/net/ti-dp83867.h>
>  #include "k3-pinctrl.h"
>  
> @@ -124,6 +125,15 @@ AM65X_IOPAD(0x0088, PIN_INPUT, 2) /* (AG17) PRG2_PRU0_GPO4.PRG2_RGMII1_RX_CTL */
>  	};
>  };
>  
> +&main_pmx1 {
> +	/* Select GPIO1_87 for ICSSG2 PHY interrupt */
> +	icssg2_phy_irq_pins_default: icssg2-phy-irq-default-pins {
> +		pinctrl-single,pins = <
> +			AM65X_IOPAD(0x0014, PIN_INPUT, 7) /* (A22) EXT_REFCLK1.GPIO1_87 */
> +		>;
> +	};
> +};
> +
>  &icssg2_mdio {
>  	status = "okay";
>  	pinctrl-names = "default";
> @@ -132,13 +142,20 @@ &icssg2_mdio {
>  	#size-cells = <0>;
>  
>  	icssg2_phy0: ethernet-phy@0 {
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&icssg2_phy_irq_pins_default>;
> +
>  		reg = <0>;
> +		interrupt-parent = <&main_gpio1>;
> +		interrupts = <87 IRQ_TYPE_EDGE_FALLING>;
>  		ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
>  		ti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
>  	};
>  
>  	icssg2_phy1: ethernet-phy@3 {
>  		reg = <3>;
> +		interrupt-parent = <&main_gpio1>;
> +		interrupts = <87 IRQ_TYPE_EDGE_FALLING>;

https://www.ti.com/lit/ds/symlink/dp83867ir.pdf -> it looks like the
interrupt pin is level event. but drivers/gpio/gpio-davinci.c::
gpio_irq_type() -> The SoC cannot handle level, only edge.

A bit confused here..  GPIO 87 is shared between two phys. isn't it a
case of race?

PHY1 assets low
phy1 handler starts, but before the driver it clears the condition:
PHY2 asserts low - but since the signal is already low, there is no
pulse
phy1 handler clears phy1 condition, but signal is still low due to phy2?
now phy2 OR phy1 never gets handled since there is never a pulse event
ever again.


>  		ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
>  		ti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
>  	};
> -- 
> 2.34.1
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D