| Message ID | 20230923152653.3371216-1-cyrevolt@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Conor Dooley |
| Headers | show |
| Series | riscv: dts: starfive: Add VisionFive 2 PHY supplies | expand |
Daniel Maslowski wrote: > I checked the schematics to see what power supplies are needed. > This allows for using ethernet without other drivers enabled > that would coincidentally enable the same power supply. ⚡ > > Signed-off-by: Daniel Maslowski <cyrevolt@gmail.com> > --- > .../boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts > index d4ea4a2c0b9b..bc1ac12bc71b 100644 > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts > @@ -16,12 +16,14 @@ &gmac0 { > starfive,tx-use-rgmii-clk; > assigned-clocks = <&aoncrg JH7110_AONCLK_GMAC0_TX>; > assigned-clock-parents = <&aoncrg JH7110_AONCLK_GMAC0_RMII_RTX>; > + phy-supply = <&vcc_3v3>; > }; > > &gmac1 { > starfive,tx-use-rgmii-clk; > assigned-clocks = <&syscrg JH7110_SYSCLK_GMAC1_TX>; > assigned-clock-parents = <&syscrg JH7110_SYSCLK_GMAC1_RMII_RTX>; > + phy-supply = <&vcc_3v3>; > }; Hi Daniel, Thank you for the patch! I've looked into this a bit now and I found 3 things: 1) The vcc_3v3 regulator (DCDC1 on the PMIC) already has the regulator-always-on property, so should never be turned off even if there are no consumers. 2) As far as I can tell the schematic for both the 1.2A and 1.3B says the PHYs are powered by DCDC6 (and not DCDC1) on the PMIC via the VDD33_IO line. 3) There doesn't seem to be any generic code in the dwmac driver handling the phy-supply property, only the Rockchip wrapper (dwmac-rk.c) does that. So all in all I'm a little confused how this change can make a difference for you. @Walker does the above and 2) in particular seem correct to you? I'd expect something like the following instead (although split in 3 patches). Daniel, does this work for you? diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml index 5e7cfbbebce6..9ce877782419 100644 --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml @@ -54,6 +54,9 @@ properties: minItems: 3 maxItems: 3 + phy-supply: + description: PHY regulator + resets: items: - description: MAC Reset signal. diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi index 2209d5348928..6633c8d29be7 100644 --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi @@ -101,6 +101,7 @@ &tdm_ext { &gmac0 { phy-handle = <&phy0>; phy-mode = "rgmii-id"; + phy-supply = <&vdd33_io>; status = "okay"; mdio { @@ -117,6 +118,7 @@ phy0: ethernet-phy@0 { &gmac1 { phy-handle = <&phy1>; phy-mode = "rgmii-id"; + phy-supply = <&vdd33_io>; status = "okay"; mdio { @@ -182,6 +184,13 @@ vdd_cpu: dcdc2 { regulator-name = "vdd-cpu"; }; + vdd33_io: dcdc6 { + regulator-boot-on; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + regulator-name = "vdd33_io"; + }; + emmc_vdd: aldo4 { regulator-boot-on; regulator-always-on; diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c index 9289bb87c3e3..2ce349421981 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c @@ -12,6 +12,7 @@ #include <linux/property.h> #include <linux/mfd/syscon.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> #include "stmmac_platform.h" @@ -97,6 +98,7 @@ static int starfive_dwmac_probe(struct platform_device *pdev) struct plat_stmmacenet_data *plat_dat; struct stmmac_resources stmmac_res; struct starfive_dwmac *dwmac; + struct regulator *reg_phy; struct clk *clk_gtx; int err; @@ -124,6 +126,11 @@ static int starfive_dwmac_probe(struct platform_device *pdev) return dev_err_probe(&pdev->dev, PTR_ERR(clk_gtx), "error getting gtx clock\n"); + reg_phy = devm_regulator_get_enable_optional(&pdev->dev, "phy"); + if (IS_ERR(reg_phy)) + return dev_err_probe(&pdev->dev, PTR_ERR(reg_phy), + "error getting phy regulator\n"); + /* Generally, the rgmii_tx clock is provided by the internal clock, * which needs to match the corresponding clock frequency according * to different speeds. If the rgmii_tx clock is provided by the
On 2023/9/24 20:22, Emil Renner Berthing wrote: > Daniel Maslowski wrote: >> I checked the schematics to see what power supplies are needed. >> This allows for using ethernet without other drivers enabled >> that would coincidentally enable the same power supply. ⚡ >> >> Signed-off-by: Daniel Maslowski <cyrevolt@gmail.com> >> --- >> .../boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts >> index d4ea4a2c0b9b..bc1ac12bc71b 100644 >> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts >> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts >> @@ -16,12 +16,14 @@ &gmac0 { >> starfive,tx-use-rgmii-clk; >> assigned-clocks = <&aoncrg JH7110_AONCLK_GMAC0_TX>; >> assigned-clock-parents = <&aoncrg JH7110_AONCLK_GMAC0_RMII_RTX>; >> + phy-supply = <&vcc_3v3>; >> }; >> >> &gmac1 { >> starfive,tx-use-rgmii-clk; >> assigned-clocks = <&syscrg JH7110_SYSCLK_GMAC1_TX>; >> assigned-clock-parents = <&syscrg JH7110_SYSCLK_GMAC1_RMII_RTX>; >> + phy-supply = <&vcc_3v3>; >> }; > > Hi Daniel, > > Thank you for the patch! I've looked into this a bit now and I found 3 things: > > 1) The vcc_3v3 regulator (DCDC1 on the PMIC) already has the > regulator-always-on property, so should never be turned off even if there > are no consumers. > > 2) As far as I can tell the schematic for both the 1.2A and 1.3B says the PHYs > are powered by DCDC6 (and not DCDC1) on the PMIC via the VDD33_IO line. > > 3) There doesn't seem to be any generic code in the dwmac driver handling the > phy-supply property, only the Rockchip wrapper (dwmac-rk.c) does that. > > So all in all I'm a little confused how this change can make a difference for > you. > > @Walker does the above and 2) in particular seem correct to you? I checked the schematic of VisionFive2 again, the GMAC PHY is powered by DCDC6 via the VDD33_IO line, not DCDC1. So there's no need to add the property 'phy-supply' and handler code for regulator in probe(). > > I'd expect something like the following instead (although split in 3 patches). > Daniel, does this work for you? > > diff --git a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml > b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml > index 5e7cfbbebce6..9ce877782419 100644 > --- a/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml > +++ b/Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml > @@ -54,6 +54,9 @@ properties: > minItems: 3 > maxItems: 3 > > + phy-supply: > + description: PHY regulator > + > resets: > items: > - description: MAC Reset signal. > diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi > b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi > index 2209d5348928..6633c8d29be7 100644 > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi > @@ -101,6 +101,7 @@ &tdm_ext { > &gmac0 { > phy-handle = <&phy0>; > phy-mode = "rgmii-id"; > + phy-supply = <&vdd33_io>; > status = "okay"; > > mdio { > @@ -117,6 +118,7 @@ phy0: ethernet-phy@0 { > &gmac1 { > phy-handle = <&phy1>; > phy-mode = "rgmii-id"; > + phy-supply = <&vdd33_io>; > status = "okay"; > > mdio { > @@ -182,6 +184,13 @@ vdd_cpu: dcdc2 { > regulator-name = "vdd-cpu"; > }; > > + vdd33_io: dcdc6 { > + regulator-boot-on; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + regulator-name = "vdd33_io"; > + }; > + > emmc_vdd: aldo4 { > regulator-boot-on; > regulator-always-on; > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c > b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c > index 9289bb87c3e3..2ce349421981 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c > @@ -12,6 +12,7 @@ > #include <linux/property.h> > #include <linux/mfd/syscon.h> > #include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > > #include "stmmac_platform.h" > > @@ -97,6 +98,7 @@ static int starfive_dwmac_probe(struct platform_device *pdev) > struct plat_stmmacenet_data *plat_dat; > struct stmmac_resources stmmac_res; > struct starfive_dwmac *dwmac; > + struct regulator *reg_phy; > struct clk *clk_gtx; > int err; > > @@ -124,6 +126,11 @@ static int starfive_dwmac_probe(struct > platform_device *pdev) > return dev_err_probe(&pdev->dev, PTR_ERR(clk_gtx), > "error getting gtx clock\n"); > > + reg_phy = devm_regulator_get_enable_optional(&pdev->dev, "phy"); > + if (IS_ERR(reg_phy)) > + return dev_err_probe(&pdev->dev, PTR_ERR(reg_phy), > + "error getting phy regulator\n"); > + > /* Generally, the rgmii_tx clock is provided by the internal clock, > * which needs to match the corresponding clock frequency according > * to different speeds. If the rgmii_tx clock is provided by the
diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts index d4ea4a2c0b9b..bc1ac12bc71b 100644 --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts @@ -16,12 +16,14 @@ &gmac0 { starfive,tx-use-rgmii-clk; assigned-clocks = <&aoncrg JH7110_AONCLK_GMAC0_TX>; assigned-clock-parents = <&aoncrg JH7110_AONCLK_GMAC0_RMII_RTX>; + phy-supply = <&vcc_3v3>; }; &gmac1 { starfive,tx-use-rgmii-clk; assigned-clocks = <&syscrg JH7110_SYSCLK_GMAC1_TX>; assigned-clock-parents = <&syscrg JH7110_SYSCLK_GMAC1_RMII_RTX>; + phy-supply = <&vcc_3v3>; }; &phy0 {
I checked the schematics to see what power supplies are needed. This allows for using ethernet without other drivers enabled that would coincidentally enable the same power supply. ⚡ Signed-off-by: Daniel Maslowski <cyrevolt@gmail.com> --- .../boot/dts/starfive/jh7110-starfive-visionfive-2-v1.3b.dts | 2 ++ 1 file changed, 2 insertions(+)