Message ID | 20231213080216.1710730-1-s-vadapalli@ti.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:3b04:b0:fb:cd0c:d3e with SMTP id c4csp7615030dys; Wed, 13 Dec 2023 00:02:50 -0800 (PST) X-Google-Smtp-Source: AGHT+IEJ4aVZSWQtL5erGMAV/n0V5mRBpIOtCwMa+2GyQ/sOUxISWSolujfDjib21Ga1RuQqdQif X-Received: by 2002:a05:6a00:809:b0:6ce:6ac3:91e8 with SMTP id m9-20020a056a00080900b006ce6ac391e8mr10916080pfk.20.1702454570521; Wed, 13 Dec 2023 00:02:50 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1702454570; cv=none; d=google.com; s=arc-20160816; b=AzV+uUnc1eXB9EzUPUzqziqw0xx7EKT5zAh5KlhgY/0XN/txJu4dsCPIoe9jWpnBBS uDpURgv5UBq7keasZHfPbzGC27oFOEakbsIpTrKt9x1alGV/0wX7FdBOhiykpflBB8Ks CWprHiqEMVxZVAYWRLb38yxSGClKvYW7QkZgwGQO0Evzjtlb0gnr7gjdJOALAOn0mufB EUCLhgQNekdLAwTW27njzDUuglsDBLaVT2DcaUQ8ZEufiNweBbb+agaAWWaN2fBWftR1 hqzCQWIuUcR2lM19nsJrewSuH66Fh3Co3daq1UU0ObqNBzSfEuHLhFk2eHU5baWNTzkm 2MGg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=MBKsgsuYGX4k9jFCM4rlwljZIghlRoJN70MfqBeb5vE=; fh=8zTfjgTag9Xv32xZq9k5V8cSuvbvzlDqi8km3U7s+R0=; b=pm7r6LLHYYPlnyTc4a2nA7Q39+olEyGBVFGFbveb70SnuiLPdoaEVfWOxjg41fe1ID iffkxT3RZQwci1R69XL0td5rT/+Pzus0EOpjwdqRzMWY6a1iosPaapYM63JIT/5RQugc XJQiB2IIE5mxB584dtjZj840BPUSEPtfB6wcgNeYDIprG1LS3tj+BWhrTlv7Qv4GOvAY cueWdD4nK6t5fhg73LgC5gRfbum/+Zbw6PUIBQmqYjOvxprdoddJsJYhMMsCzOZYL2RS yJcghZEuC0ahA/KmnasoGCmCx2PBRawmmjEhlqrPqstn/FvR8GavZ0DyZ/MlSzx3ZX/2 oyDA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=YUK5ZNUq; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: from fry.vger.email (fry.vger.email. [23.128.96.38]) by mx.google.com with ESMTPS id kp12-20020a056a00464c00b006ce45456a0esi9298821pfb.46.2023.12.13.00.02.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 13 Dec 2023 00:02:50 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38; Authentication-Results: mx.google.com; dkim=pass header.i=@ti.com header.s=ti-com-17Q1 header.b=YUK5ZNUq; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=ti.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by fry.vger.email (Postfix) with ESMTP id D315380874C3; Wed, 13 Dec 2023 00:02:47 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1378715AbjLMIC3 (ORCPT <rfc822;dexuan.linux@gmail.com> + 99 others); Wed, 13 Dec 2023 03:02:29 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56714 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232673AbjLMIC3 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 13 Dec 2023 03:02:29 -0500 Received: from fllv0015.ext.ti.com (fllv0015.ext.ti.com [198.47.19.141]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3DA2CD0; Wed, 13 Dec 2023 00:02:35 -0800 (PST) Received: from lelv0266.itg.ti.com ([10.180.67.225]) by fllv0015.ext.ti.com (8.15.2/8.15.2) with ESMTP id 3BD82M9m002149; Wed, 13 Dec 2023 02:02:22 -0600 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ti.com; s=ti-com-17Q1; t=1702454542; bh=MBKsgsuYGX4k9jFCM4rlwljZIghlRoJN70MfqBeb5vE=; h=From:To:CC:Subject:Date; b=YUK5ZNUqD6b2dti6512xGmw2HQ7IJkClY5Wpyzd6r6lQOaooZYgLPkoh7wEfrHFca 50RiyyjvAfQWnpbBrpF0leDLnRLpPfzfym7a/Y1Auhyf8Q7ZltCNNFpVjI1oUilbnO Ob7bVL6A1/dulOtTp/8ms1hHKnAKelGWWiB4G90A= Received: from DLEE103.ent.ti.com (dlee103.ent.ti.com [157.170.170.33]) by lelv0266.itg.ti.com (8.15.2/8.15.2) with ESMTPS id 3BD82MmU126291 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 13 Dec 2023 02:02:22 -0600 Received: from DLEE100.ent.ti.com (157.170.170.30) by DLEE103.ent.ti.com (157.170.170.33) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23; Wed, 13 Dec 2023 02:02:21 -0600 Received: from lelvsmtp5.itg.ti.com (10.180.75.250) by DLEE100.ent.ti.com (157.170.170.30) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2507.23 via Frontend Transport; Wed, 13 Dec 2023 02:02:21 -0600 Received: from uda0492258.dhcp.ti.com (uda0492258.dhcp.ti.com [172.24.227.9]) by lelvsmtp5.itg.ti.com (8.15.2/8.15.2) with ESMTP id 3BD82HU8024879; Wed, 13 Dec 2023 02:02:18 -0600 From: Siddharth Vadapalli <s-vadapalli@ti.com> To: <nm@ti.com>, <vigneshr@ti.com>, <kristo@kernel.org>, <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org> CC: <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>, <s-vadapalli@ti.com> Subject: [PATCH v2] arm64: dts: ti: k3-am654-icssg2: Enable PHY interrupts for ICSSG2 Date: Wed, 13 Dec 2023 13:32:16 +0530 Message-ID: <20231213080216.1710730-1-s-vadapalli@ti.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-EXCLAIMER-MD-CONFIG: e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180 X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Wed, 13 Dec 2023 00:02:48 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1785153003178298939 X-GMAIL-MSGID: 1785153003178298939 |
Series |
[v2] arm64: dts: ti: k3-am654-icssg2: Enable PHY interrupts for ICSSG2
|
|
Commit Message
Siddharth Vadapalli
Dec. 13, 2023, 8:02 a.m. UTC
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(+)
Comments
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 >
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 >> >
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.
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.
> 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
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>; };