diff mbox series

[v3,5/9] riscv: dts: starfive: jh7100-common: Setup pinmux and enable gmac

Message ID 20231215204050.2296404-6-cristian.ciocaltea@collabora.com (mailing list archive)
State Not Applicable, archived
Headers show
Series Enable networking support for StarFive JH7100 SoC | expand

Commit Message

Cristian Ciocaltea Dec. 15, 2023, 8:40 p.m. UTC
Add pinmux configuration for DWMAC found on the JH7100 based boards and
enable the related DT node, providing a basic PHY configuration.

Co-developed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 .../boot/dts/starfive/jh7100-common.dtsi      | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)

Comments

Emil Renner Berthing Dec. 16, 2023, 7:38 p.m. UTC | #1
Cristian Ciocaltea wrote:
> Add pinmux configuration for DWMAC found on the JH7100 based boards and
> enable the related DT node, providing a basic PHY configuration.
>
> Co-developed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  .../boot/dts/starfive/jh7100-common.dtsi      | 85 +++++++++++++++++++
>  1 file changed, 85 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> index 42fb61c36068..5cafe8f5c2e7 100644
> --- a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> @@ -72,7 +72,92 @@ wifi_pwrseq: wifi-pwrseq {
>  	};
>  };
>
> +&gmac {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&gmac_pins>;
> +	phy-mode = "rgmii-id";
> +	phy-handle = <&phy>;

I'm not sure if it's a generic policy or not, but I don't really like adding a
reference to a non-existant node here. I'd move this property to the board
files where the phy node is actually defined.

/Emil
Conor Dooley Dec. 17, 2023, 9:03 p.m. UTC | #2
On Sat, Dec 16, 2023 at 11:38:53AM -0800, Emil Renner Berthing wrote:
> Cristian Ciocaltea wrote:
> > Add pinmux configuration for DWMAC found on the JH7100 based boards and
> > enable the related DT node, providing a basic PHY configuration.
> >
> > Co-developed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> > Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> > ---
> >  .../boot/dts/starfive/jh7100-common.dtsi      | 85 +++++++++++++++++++
> >  1 file changed, 85 insertions(+)
> >
> > diff --git a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> > index 42fb61c36068..5cafe8f5c2e7 100644
> > --- a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> > +++ b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
> > @@ -72,7 +72,92 @@ wifi_pwrseq: wifi-pwrseq {
> >  	};
> >  };
> >
> > +&gmac {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&gmac_pins>;
> > +	phy-mode = "rgmii-id";
> > +	phy-handle = <&phy>;
> 
> I'm not sure if it's a generic policy or not, but I don't really like adding a
> reference to a non-existant node here. I'd move this property to the board
> files where the phy node is actually defined.

FWIW, I don't like the reference-in-the-wrong-place thing either.
Cristian Ciocaltea Dec. 18, 2023, 11:41 a.m. UTC | #3
On 12/16/23 21:38, Emil Renner Berthing wrote:
> Cristian Ciocaltea wrote:
>> Add pinmux configuration for DWMAC found on the JH7100 based boards and
>> enable the related DT node, providing a basic PHY configuration.
>>
>> Co-developed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
>> Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  .../boot/dts/starfive/jh7100-common.dtsi      | 85 +++++++++++++++++++
>>  1 file changed, 85 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
>> index 42fb61c36068..5cafe8f5c2e7 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
>> @@ -72,7 +72,92 @@ wifi_pwrseq: wifi-pwrseq {
>>  	};
>>  };
>>
>> +&gmac {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&gmac_pins>;
>> +	phy-mode = "rgmii-id";
>> +	phy-handle = <&phy>;
> 
> I'm not sure if it's a generic policy or not, but I don't really like adding a
> reference to a non-existant node here. I'd move this property to the board
> files where the phy node is actually defined.

Totally agree, I simply went too far while dropping duplicated code and
didn't realize the mistake.  Thanks for noticing!
Cristian Ciocaltea Dec. 18, 2023, 11:48 a.m. UTC | #4
On 12/17/23 23:03, Conor Dooley wrote:
> On Sat, Dec 16, 2023 at 11:38:53AM -0800, Emil Renner Berthing wrote:
>> Cristian Ciocaltea wrote:
>>> Add pinmux configuration for DWMAC found on the JH7100 based boards and
>>> enable the related DT node, providing a basic PHY configuration.
>>>
>>> Co-developed-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
>>> Signed-off-by: Emil Renner Berthing <emil.renner.berthing@canonical.com>
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>> ---
>>>  .../boot/dts/starfive/jh7100-common.dtsi      | 85 +++++++++++++++++++
>>>  1 file changed, 85 insertions(+)
>>>
>>> diff --git a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
>>> index 42fb61c36068..5cafe8f5c2e7 100644
>>> --- a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
>>> +++ b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
>>> @@ -72,7 +72,92 @@ wifi_pwrseq: wifi-pwrseq {
>>>  	};
>>>  };
>>>
>>> +&gmac {
>>> +	pinctrl-names = "default";
>>> +	pinctrl-0 = <&gmac_pins>;
>>> +	phy-mode = "rgmii-id";
>>> +	phy-handle = <&phy>;
>>
>> I'm not sure if it's a generic policy or not, but I don't really like adding a
>> reference to a non-existant node here. I'd move this property to the board
>> files where the phy node is actually defined.
> 
> FWIW, I don't like the reference-in-the-wrong-place thing either.

Yeah, as I already mentioned this was unintentional, will fix in v4.

Thanks,
Cristian
diff mbox series

Patch

diff --git a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
index 42fb61c36068..5cafe8f5c2e7 100644
--- a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
@@ -72,7 +72,92 @@  wifi_pwrseq: wifi-pwrseq {
 	};
 };
 
+&gmac {
+	pinctrl-names = "default";
+	pinctrl-0 = <&gmac_pins>;
+	phy-mode = "rgmii-id";
+	phy-handle = <&phy>;
+	status = "okay";
+
+	mdio: mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "snps,dwmac-mdio";
+	};
+};
+
 &gpio {
+	gmac_pins: gmac-0 {
+		gtxclk-pins {
+			pins = <PAD_FUNC_SHARE(115)>;
+			bias-pull-up;
+			drive-strength = <35>;
+			input-enable;
+			input-schmitt-enable;
+			slew-rate = <0>;
+		};
+		miitxclk-pins {
+			pins = <PAD_FUNC_SHARE(116)>;
+			bias-pull-up;
+			drive-strength = <14>;
+			input-enable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+		tx-pins {
+			pins = <PAD_FUNC_SHARE(117)>,
+			       <PAD_FUNC_SHARE(119)>,
+			       <PAD_FUNC_SHARE(120)>,
+			       <PAD_FUNC_SHARE(121)>,
+			       <PAD_FUNC_SHARE(122)>,
+			       <PAD_FUNC_SHARE(123)>,
+			       <PAD_FUNC_SHARE(124)>,
+			       <PAD_FUNC_SHARE(125)>,
+			       <PAD_FUNC_SHARE(126)>;
+			bias-pull-up;
+			drive-strength = <35>;
+			input-disable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+		rxclk-pins {
+			pins = <PAD_FUNC_SHARE(127)>;
+			bias-pull-up;
+			drive-strength = <14>;
+			input-enable;
+			input-schmitt-disable;
+			slew-rate = <6>;
+		};
+		rxer-pins {
+			pins = <PAD_FUNC_SHARE(129)>;
+			bias-pull-up;
+			drive-strength = <14>;
+			input-enable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+		rx-pins {
+			pins = <PAD_FUNC_SHARE(128)>,
+			       <PAD_FUNC_SHARE(130)>,
+			       <PAD_FUNC_SHARE(131)>,
+			       <PAD_FUNC_SHARE(132)>,
+			       <PAD_FUNC_SHARE(133)>,
+			       <PAD_FUNC_SHARE(134)>,
+			       <PAD_FUNC_SHARE(135)>,
+			       <PAD_FUNC_SHARE(136)>,
+			       <PAD_FUNC_SHARE(137)>,
+			       <PAD_FUNC_SHARE(138)>,
+			       <PAD_FUNC_SHARE(139)>,
+			       <PAD_FUNC_SHARE(140)>,
+			       <PAD_FUNC_SHARE(141)>;
+			bias-pull-up;
+			drive-strength = <14>;
+			input-enable;
+			input-schmitt-enable;
+			slew-rate = <0>;
+		};
+	};
+
 	i2c0_pins: i2c0-0 {
 		i2c-pins {
 			pinmux = <GPIOMUX(62, GPO_LOW,