* [PATCH v2] arm64: dts: ti: k3-am654-icssg2: Enable PHY interrupts for ICSSG2
@ 2023-12-13 8:02 Siddharth Vadapalli
2023-12-13 12:38 ` Nishanth Menon
0 siblings, 1 reply; 6+ messages in thread
From: Siddharth Vadapalli @ 2023-12-13 8:02 UTC (permalink / raw)
To: nm, vigneshr, kristo, robh+dt, krzysztof.kozlowski+dt, conor+dt
Cc: devicetree, linux-kernel, linux-arm-kernel, danishanwar,
r-gunasekaran, srk, s-vadapalli
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>;
ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
ti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
};
--
2.34.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] arm64: dts: ti: k3-am654-icssg2: Enable PHY interrupts for ICSSG2
2023-12-13 8:02 [PATCH v2] arm64: dts: ti: k3-am654-icssg2: Enable PHY interrupts for ICSSG2 Siddharth Vadapalli
@ 2023-12-13 12:38 ` Nishanth Menon
2023-12-14 5:44 ` Siddharth Vadapalli
0 siblings, 1 reply; 6+ messages in thread
From: Nishanth Menon @ 2023-12-13 12:38 UTC (permalink / raw)
To: Siddharth Vadapalli
Cc: vigneshr, kristo, robh+dt, krzysztof.kozlowski+dt, conor+dt,
devicetree, linux-kernel, linux-arm-kernel, danishanwar,
r-gunasekaran, srk
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] arm64: dts: ti: k3-am654-icssg2: Enable PHY interrupts for ICSSG2
2023-12-13 12:38 ` Nishanth Menon
@ 2023-12-14 5:44 ` Siddharth Vadapalli
2023-12-14 12:17 ` Nishanth Menon
2023-12-17 17:44 ` Andrew Lunn
0 siblings, 2 replies; 6+ messages in thread
From: Siddharth Vadapalli @ 2023-12-14 5:44 UTC (permalink / raw)
To: Nishanth Menon
Cc: vigneshr, kristo, robh+dt, krzysztof.kozlowski+dt, conor+dt,
devicetree, linux-kernel, linux-arm-kernel, danishanwar,
r-gunasekaran, srk, s-vadapalli
Hello Nishanth,
On 13/12/23 18:08, Nishanth Menon wrote:
> 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.
...
>
> 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.
Yes, you are right! Edge-Triggered interrupts shouldn't be shared. I missed
noticing this. Thank you for pointing it out. Since the SoC only supports
Edge-Triggered interrupts, I believe that the correct decision would be to use
the interrupt for only one of the two PHYs, while leaving the other PHY in
polled mode of operation which is the default.
Kindly let me know if this is acceptable and I shall update this patch accordingly.
>
>
>> ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
>> ti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
>> };
>> --
>> 2.34.1
>>
>
--
Regards,
Siddharth.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: dts: ti: k3-am654-icssg2: Enable PHY interrupts for ICSSG2
2023-12-14 5:44 ` Siddharth Vadapalli
@ 2023-12-14 12:17 ` Nishanth Menon
2023-12-14 13:50 ` Siddharth Vadapalli
2023-12-17 17:44 ` Andrew Lunn
1 sibling, 1 reply; 6+ messages in thread
From: Nishanth Menon @ 2023-12-14 12:17 UTC (permalink / raw)
To: Siddharth Vadapalli
Cc: vigneshr, kristo, robh+dt, krzysztof.kozlowski+dt, conor+dt,
devicetree, linux-kernel, linux-arm-kernel, danishanwar,
r-gunasekaran, srk
On 11:14-20231214, Siddharth Vadapalli wrote:
> Hello Nishanth,
>
> On 13/12/23 18:08, Nishanth Menon wrote:
> > 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.
>
> ...
>
> >
> > 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.
>
> Yes, you are right! Edge-Triggered interrupts shouldn't be shared. I missed
> noticing this. Thank you for pointing it out. Since the SoC only supports
> Edge-Triggered interrupts, I believe that the correct decision would be to use
> the interrupt for only one of the two PHYs, while leaving the other PHY in
> polled mode of operation which is the default.
>
> Kindly let me know if this is acceptable and I shall update this patch accordingly.
Sounds like a bug in board design there (due to an choice of IP
limitation) - I suggest getting it noted in board documentation and
refer to the errata in the second phy (else folks will wonder why we
aren't using interrupts on the second phy.
>
> >
> >
> >> ti,rx-internal-delay = <DP83867_RGMIIDCTL_2_00_NS>;
> >> ti,fifo-depth = <DP83867_PHYCR_FIFO_DEPTH_4_B_NIB>;
> >> };
> >> --
> >> 2.34.1
> >>
> >
>
> --
> Regards,
> Siddharth.
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: dts: ti: k3-am654-icssg2: Enable PHY interrupts for ICSSG2
2023-12-14 12:17 ` Nishanth Menon
@ 2023-12-14 13:50 ` Siddharth Vadapalli
0 siblings, 0 replies; 6+ messages in thread
From: Siddharth Vadapalli @ 2023-12-14 13:50 UTC (permalink / raw)
To: Nishanth Menon
Cc: vigneshr, kristo, robh+dt, krzysztof.kozlowski+dt, conor+dt,
devicetree, linux-kernel, linux-arm-kernel, danishanwar,
r-gunasekaran, srk, s-vadapalli
On 14-12-2023 17:47, Nishanth Menon wrote:
...
>>
>> Yes, you are right! Edge-Triggered interrupts shouldn't be shared. I missed
>> noticing this. Thank you for pointing it out. Since the SoC only supports
>> Edge-Triggered interrupts, I believe that the correct decision would be to use
>> the interrupt for only one of the two PHYs, while leaving the other PHY in
>> polled mode of operation which is the default.
>>
>> Kindly let me know if this is acceptable and I shall update this patch accordingly.
>
> Sounds like a bug in board design there (due to an choice of IP
> limitation) - I suggest getting it noted in board documentation and
> refer to the errata in the second phy (else folks will wonder why we
> aren't using interrupts on the second phy.
Thank you for your suggestion on the next steps to be taken. I will
ensure that the board documentation is updated. Additionally, in the v3
patch I will add a comment within the "icssg2_phy0" node indicating that
the interrupt mode of operation is only being enabled for "icssg2_phy0"
due to the interrupt being an edge-triggered interrupt which cannot be
shared among both the PHYs. And for that reason "icssg2_phy1" is being
left in the default polled mode of operation.
--
Regards,
Siddharth.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] arm64: dts: ti: k3-am654-icssg2: Enable PHY interrupts for ICSSG2
2023-12-14 5:44 ` Siddharth Vadapalli
2023-12-14 12:17 ` Nishanth Menon
@ 2023-12-17 17:44 ` Andrew Lunn
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Lunn @ 2023-12-17 17:44 UTC (permalink / raw)
To: Siddharth Vadapalli
Cc: Nishanth Menon, vigneshr, kristo, robh+dt,
krzysztof.kozlowski+dt, conor+dt, devicetree, linux-kernel,
linux-arm-kernel, danishanwar, r-gunasekaran, srk
> Yes, you are right! Edge-Triggered interrupts shouldn't be shared. I missed
> noticing this. Thank you for pointing it out. Since the SoC only supports
> Edge-Triggered interrupts, I believe that the correct decision would be to use
> the interrupt for only one of the two PHYs, while leaving the other PHY in
> polled mode of operation which is the default.
No, if the PHY is using level interrupts, you need the SoC to use
level interrupts. Otherwise you are going to miss interrupts.
The board design is just broken and you cannot use interrupts at all.
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-17 17:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-13 8:02 [PATCH v2] arm64: dts: ti: k3-am654-icssg2: Enable PHY interrupts for ICSSG2 Siddharth Vadapalli
2023-12-13 12:38 ` Nishanth Menon
2023-12-14 5:44 ` Siddharth Vadapalli
2023-12-14 12:17 ` Nishanth Menon
2023-12-14 13:50 ` Siddharth Vadapalli
2023-12-17 17:44 ` Andrew Lunn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®