| Message ID | 20221116200150.4657-5-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [linux-next-v2,1/5] arm64: dts: rockchip: Fix gmac phy mode to rgmii on Rock 3A SBC. | expand |
Hi Anand, On 11/16/22 21:01, Anand Moon wrote: > As per the shematic gmac1 support gpio interrupt controller Typo "shematic" -> "schematic" > GMAC1_INT/PMEB_GPIO3_A7 add the support for this. Maybe split the commit message into two proper sentences. > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > --- > v2: new patch added > --- > arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts > index 5378254c57ca..9f84a23a8789 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts > +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts > @@ -588,10 +588,14 @@ rgmii_phy1: ethernet-phy@0 { > compatible = "ethernet-phy-ieee802.3-c22"; > reg = <0x0>; > pinctrl-names = "default"; > - pinctrl-0 = <ð_phy_rst>; > + pinctrl-0 = <ð_phy_rst>, <ð_phy_int>; > reset-assert-us = <20000>; > reset-deassert-us = <100000>; > reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>; > + interrupt-parent = <&gpio3>; > + /* GMAC1_INT/PMEB_GPIO3_A7 */ This comment is pretty superfluous. > + interrupts = <RK_PA7 IRQ_TYPE_LEVEL_LOW>; > + #interrupt-cells = <1>; I am not an expert here, but I believe #interrupt-cells = <1>; means that the phy provides an array of interrupts. Are you sure this is correct? I find it strange that the phy driver consumes one interrupt and provides N interrupts?! > }; > }; > > @@ -630,6 +634,10 @@ vcc_mipi_en: vcc_mipi_en { > }; > > ethernet { > + eth_phy_int: eth-phy-int { > + rockchip,pins = <3 RK_PA7 RK_FUNC_GPIO &pcfg_pull_down>; Interrupt is active low and you pull down the line here? There is a pull up resistor on sheet 11 of the schematic, so this does not seem right at all. Best regards, Michael > + }; > + > eth_phy_rst: eth_phy_rst { > rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>; > };
Hi Michael, Thanks for your review comments. On Tue, 22 Nov 2022 at 21:40, Michael Riesch <michael.riesch@wolfvision.net> wrote: > > Hi Anand, > > On 11/16/22 21:01, Anand Moon wrote: > > As per the shematic gmac1 support gpio interrupt controller > > Typo "shematic" -> "schematic" Ok, > > > GMAC1_INT/PMEB_GPIO3_A7 add the support for this. > > Maybe split the commit message into two proper sentences. Ok this Interrupt is used for Power Management Event (supports 3.3V pull up). Set low if received a magic packet or wake up frame; active low. so this interrupt is used for suspend / resume. > > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > > --- > > v2: new patch added > > --- > > arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts > > index 5378254c57ca..9f84a23a8789 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts > > +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts > > @@ -588,10 +588,14 @@ rgmii_phy1: ethernet-phy@0 { > > compatible = "ethernet-phy-ieee802.3-c22"; > > reg = <0x0>; > > pinctrl-names = "default"; > > - pinctrl-0 = <ð_phy_rst>; > > + pinctrl-0 = <ð_phy_rst>, <ð_phy_int>; > > reset-assert-us = <20000>; > > reset-deassert-us = <100000>; > > reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>; > > + interrupt-parent = <&gpio3>; > > + /* GMAC1_INT/PMEB_GPIO3_A7 */ > > This comment is pretty superfluous. Ok will drop this. > > > + interrupts = <RK_PA7 IRQ_TYPE_LEVEL_LOW>; > > + #interrupt-cells = <1>; > > I am not an expert here, but I believe #interrupt-cells = <1>; means > that the phy provides an array of interrupts. Are you sure this is > correct? I find it strange that the phy driver consumes one interrupt > and provides N interrupts?! > Ok will drop this. > > }; > > }; > > > > @@ -630,6 +634,10 @@ vcc_mipi_en: vcc_mipi_en { > > }; > > > > ethernet { > > + eth_phy_int: eth-phy-int { > > + rockchip,pins = <3 RK_PA7 RK_FUNC_GPIO &pcfg_pull_down>; > > Interrupt is active low and you pull down the line here? There is a pull > up resistor on sheet 11 of the schematic, so this does not seem right at > all. > Actually this GPIO3_A7_d hence pull_down, this pin level triggered So I will set this as &pcfg_pull_none. I don't have PoE set up to verify these changes as of now. > Best regards, > Michael Thanks -Anand
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts index 5378254c57ca..9f84a23a8789 100644 --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts @@ -588,10 +588,14 @@ rgmii_phy1: ethernet-phy@0 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <0x0>; pinctrl-names = "default"; - pinctrl-0 = <ð_phy_rst>; + pinctrl-0 = <ð_phy_rst>, <ð_phy_int>; reset-assert-us = <20000>; reset-deassert-us = <100000>; reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_LOW>; + interrupt-parent = <&gpio3>; + /* GMAC1_INT/PMEB_GPIO3_A7 */ + interrupts = <RK_PA7 IRQ_TYPE_LEVEL_LOW>; + #interrupt-cells = <1>; }; }; @@ -630,6 +634,10 @@ vcc_mipi_en: vcc_mipi_en { }; ethernet { + eth_phy_int: eth-phy-int { + rockchip,pins = <3 RK_PA7 RK_FUNC_GPIO &pcfg_pull_down>; + }; + eth_phy_rst: eth_phy_rst { rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>; };
As per the shematic gmac1 support gpio interrupt controller GMAC1_INT/PMEB_GPIO3_A7 add the support for this. Signed-off-by: Anand Moon <linux.amoon@gmail.com> --- v2: new patch added --- arch/arm64/boot/dts/rockchip/rk3568-rock-3a.dts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)