diff mbox series

[net-next,2/2] ARM: dts: aspeed: elbert: Enable mac3 controller

Message ID 20220905235634.20957-3-rentao.bupt@gmail.com (mailing list archive)
State New, archived
Headers show
Series net: ftgmac100: support fixed link | expand

Commit Message

Tao Ren Sept. 5, 2022, 11:56 p.m. UTC
From: Tao Ren <rentao.bupt@gmail.com>

Enable mac3 controller in Elbert dts: Elbert MAC3 is connected to the
onboard switch directly (fixed link).

Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
---
 arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Andrew Lunn Sept. 6, 2022, 12:22 a.m. UTC | #1
On Mon, Sep 05, 2022 at 04:56:34PM -0700, rentao.bupt@gmail.com wrote:
> From: Tao Ren <rentao.bupt@gmail.com>
> 
> Enable mac3 controller in Elbert dts: Elbert MAC3 is connected to the
> onboard switch directly (fixed link).

What is the switch? Could you also add a DT node for it?

> 
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> ---
>  arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
> index 27b43fe099f1..52cb617783ac 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
> @@ -183,3 +183,14 @@ imux31: i2c@7 {
>  &i2c11 {
>  	status = "okay";
>  };
> +
> +&mac3 {
> +	status = "okay";
> +	phy-mode = "rgmii";

'rgmii' is suspicious, though not necessarily wrong. This value is
normally passed to the PHY, so the PHY inserts the RGMII delay. You
however don't have a PHY. So i assume the switch is inserting the
delay? Again, being able to see the DT properties for the switch would
be useful.

   Andrew
Tao Ren Sept. 6, 2022, 1:41 a.m. UTC | #2
Hi Andrew,

On Tue, Sep 06, 2022 at 02:22:50AM +0200, Andrew Lunn wrote:
> On Mon, Sep 05, 2022 at 04:56:34PM -0700, rentao.bupt@gmail.com wrote:
> > From: Tao Ren <rentao.bupt@gmail.com>
> > 
> > Enable mac3 controller in Elbert dts: Elbert MAC3 is connected to the
> > onboard switch directly (fixed link).
> 
> What is the switch? Could you also add a DT node for it?
> 
> > 
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > ---
> >  arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
> > index 27b43fe099f1..52cb617783ac 100644
> > --- a/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
> > +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
> > @@ -183,3 +183,14 @@ imux31: i2c@7 {
> >  &i2c11 {
> >  	status = "okay";
> >  };
> > +
> > +&mac3 {
> > +	status = "okay";
> > +	phy-mode = "rgmii";
> 
> 'rgmii' is suspicious, though not necessarily wrong. This value is
> normally passed to the PHY, so the PHY inserts the RGMII delay. You
> however don't have a PHY. So i assume the switch is inserting the
> delay? Again, being able to see the DT properties for the switch would
> be useful.
> 
>    Andrew

Thank you for the quick review!

The BMC mac3 is connected to BCM53134P's IMP_RGMII port, and there is no
PHY between BMC MAC and BCM53134P. BCM53134P loads configurations from
its EEPROM when the chip is powered.

Could you please point me an example showing how to describe the switch in
dts? Anyhow I will need to improve the patch description and comments in
v2.


Thanks,

- Tao
Andrew Lunn Sept. 6, 2022, 11:55 a.m. UTC | #3
On Mon, Sep 05, 2022 at 06:41:33PM -0700, Tao Ren wrote:
> Hi Andrew,
> 
> On Tue, Sep 06, 2022 at 02:22:50AM +0200, Andrew Lunn wrote:
> > On Mon, Sep 05, 2022 at 04:56:34PM -0700, rentao.bupt@gmail.com wrote:
> > > From: Tao Ren <rentao.bupt@gmail.com>
> > > 
> > > Enable mac3 controller in Elbert dts: Elbert MAC3 is connected to the
> > > onboard switch directly (fixed link).
> > 
> > What is the switch? Could you also add a DT node for it?
> > 
> > > 
> > > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > > ---
> > >  arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
> > > index 27b43fe099f1..52cb617783ac 100644
> > > --- a/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
> > > +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
> > > @@ -183,3 +183,14 @@ imux31: i2c@7 {
> > >  &i2c11 {
> > >  	status = "okay";
> > >  };
> > > +
> > > +&mac3 {
> > > +	status = "okay";
> > > +	phy-mode = "rgmii";
> > 
> > 'rgmii' is suspicious, though not necessarily wrong. This value is
> > normally passed to the PHY, so the PHY inserts the RGMII delay. You
> > however don't have a PHY. So i assume the switch is inserting the
> > delay? Again, being able to see the DT properties for the switch would
> > be useful.
> > 
> >    Andrew
> 
> Thank you for the quick review!
> 
> The BMC mac3 is connected to BCM53134P's IMP_RGMII port, and there is no
> PHY between BMC MAC and BCM53134P. BCM53134P loads configurations from
> its EEPROM when the chip is powered.

So i assume you have the switch RGMII port doing the delays. That is
fine.

> Could you please point me an example showing how to describe the switch in
> dts? Anyhow I will need to improve the patch description and comments in
> v2.

It looks like drivers/net/dsa/b53 does not support this particular
switch. You could consider extending the driver. See

Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml

for documentation of the binding.

    Andrew
Florian Fainelli Sept. 6, 2022, 5:05 p.m. UTC | #4
On 9/6/2022 4:55 AM, Andrew Lunn wrote:
> On Mon, Sep 05, 2022 at 06:41:33PM -0700, Tao Ren wrote:
>> Hi Andrew,
>>
>> On Tue, Sep 06, 2022 at 02:22:50AM +0200, Andrew Lunn wrote:
>>> On Mon, Sep 05, 2022 at 04:56:34PM -0700, rentao.bupt@gmail.com wrote:
>>>> From: Tao Ren <rentao.bupt@gmail.com>
>>>>
>>>> Enable mac3 controller in Elbert dts: Elbert MAC3 is connected to the
>>>> onboard switch directly (fixed link).
>>>
>>> What is the switch? Could you also add a DT node for it?
>>>
>>>>
>>>> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
>>>> ---
>>>>   arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts | 11 +++++++++++
>>>>   1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
>>>> index 27b43fe099f1..52cb617783ac 100644
>>>> --- a/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
>>>> +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
>>>> @@ -183,3 +183,14 @@ imux31: i2c@7 {
>>>>   &i2c11 {
>>>>   	status = "okay";
>>>>   };
>>>> +
>>>> +&mac3 {
>>>> +	status = "okay";
>>>> +	phy-mode = "rgmii";
>>>
>>> 'rgmii' is suspicious, though not necessarily wrong. This value is
>>> normally passed to the PHY, so the PHY inserts the RGMII delay. You
>>> however don't have a PHY. So i assume the switch is inserting the
>>> delay? Again, being able to see the DT properties for the switch would
>>> be useful.
>>>
>>>     Andrew
>>
>> Thank you for the quick review!
>>
>> The BMC mac3 is connected to BCM53134P's IMP_RGMII port, and there is no
>> PHY between BMC MAC and BCM53134P. BCM53134P loads configurations from
>> its EEPROM when the chip is powered.
> 
> So i assume you have the switch RGMII port doing the delays. That is
> fine.
> 
>> Could you please point me an example showing how to describe the switch in
>> dts? Anyhow I will need to improve the patch description and comments in
>> v2.
> 
> It looks like drivers/net/dsa/b53 does not support this particular
> switch. You could consider extending the driver. See
> 
> Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
> 
> for documentation of the binding.

Correct the 53134 is not supported at the moment by the b53 driver, 
however it should not be too hard to support it, if you would be willing 
to add it, I would be glad to review patches.
Tao Ren Sept. 6, 2022, 11:35 p.m. UTC | #5
Hi Florian and Andrew,

On Tue, Sep 06, 2022 at 10:05:46AM -0700, Florian Fainelli wrote:
> 
> 
> On 9/6/2022 4:55 AM, Andrew Lunn wrote:
> > On Mon, Sep 05, 2022 at 06:41:33PM -0700, Tao Ren wrote:
> > > Hi Andrew,
> > > 
> > > On Tue, Sep 06, 2022 at 02:22:50AM +0200, Andrew Lunn wrote:
> > > > On Mon, Sep 05, 2022 at 04:56:34PM -0700, rentao.bupt@gmail.com wrote:
> > > > > From: Tao Ren <rentao.bupt@gmail.com>
> > > > > 
> > > > > Enable mac3 controller in Elbert dts: Elbert MAC3 is connected to the
> > > > > onboard switch directly (fixed link).
> > > > 
> > > > What is the switch? Could you also add a DT node for it?
> > > > 
> > > > > 
> > > > > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > > > > ---
> > > > >   arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts | 11 +++++++++++
> > > > >   1 file changed, 11 insertions(+)
> > > > > 
> > > > > diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
> > > > > index 27b43fe099f1..52cb617783ac 100644
> > > > > --- a/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
> > > > > +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
> > > > > @@ -183,3 +183,14 @@ imux31: i2c@7 {
> > > > >   &i2c11 {
> > > > >   	status = "okay";
> > > > >   };
> > > > > +
> > > > > +&mac3 {
> > > > > +	status = "okay";
> > > > > +	phy-mode = "rgmii";
> > > > 
> > > > 'rgmii' is suspicious, though not necessarily wrong. This value is
> > > > normally passed to the PHY, so the PHY inserts the RGMII delay. You
> > > > however don't have a PHY. So i assume the switch is inserting the
> > > > delay? Again, being able to see the DT properties for the switch would
> > > > be useful.
> > > > 
> > > >     Andrew
> > > 
> > > Thank you for the quick review!
> > > 
> > > The BMC mac3 is connected to BCM53134P's IMP_RGMII port, and there is no
> > > PHY between BMC MAC and BCM53134P. BCM53134P loads configurations from
> > > its EEPROM when the chip is powered.
> > 
> > So i assume you have the switch RGMII port doing the delays. That is
> > fine.
> > 
> > > Could you please point me an example showing how to describe the switch in
> > > dts? Anyhow I will need to improve the patch description and comments in
> > > v2.
> > 
> > It looks like drivers/net/dsa/b53 does not support this particular
> > switch. You could consider extending the driver. See
> > 
> > Documentation/devicetree/bindings/net/dsa/brcm,b53.yaml
> > 
> > for documentation of the binding.
> 
> Correct the 53134 is not supported at the moment by the b53 driver, however
> it should not be too hard to support it, if you would be willing to add it,
> I would be glad to review patches.
> -- 
> Florian

Thank you for the inputs, and I do have plan to dig into dsa drivers and
work out patches when I get chance, because most FBOSS switches have
onboard switch chips connecting BMC and Host to the front-panel
management port.

Specific to this Elbert platform, we don't have plan to configure
BCM53134 via OpenBMC MDIO (dts), because we expect the switch always
loads configurations from its EEPROM.

I've sent out v2 with more comments in the dts file; could you please
check and let me know if it's reasonable/acceptable? Thank you.


Thanks,

Tao
Andrew Lunn Sept. 7, 2022, 12:36 a.m. UTC | #6
> Specific to this Elbert platform, we don't have plan to configure
> BCM53134 via OpenBMC MDIO (dts), because we expect the switch always
> loads configurations from its EEPROM.

DSA offers more than configuration. You can also get interface
statistics, and knowledge of if an interface is up/down. And since the
PHY of the switch becomes normal Linux PHYs, you can do cable testing,
if the PHY has support, etc.

Do you have spanning tree to break L2 network loops? Linux will
provide that as well.

However, if you are happy with dumb switch, then what you posted is
sufficient.

	Andrew
Tao Ren Sept. 7, 2022, 1:08 a.m. UTC | #7
Hi Andrew,

On Wed, Sep 07, 2022 at 02:36:33AM +0200, Andrew Lunn wrote:
> > Specific to this Elbert platform, we don't have plan to configure
> > BCM53134 via OpenBMC MDIO (dts), because we expect the switch always
> > loads configurations from its EEPROM.
> 
> DSA offers more than configuration. You can also get interface
> statistics, and knowledge of if an interface is up/down. And since the
> PHY of the switch becomes normal Linux PHYs, you can do cable testing,
> if the PHY has support, etc.
> 
> Do you have spanning tree to break L2 network loops? Linux will
> provide that as well.
> 
> However, if you are happy with dumb switch, then what you posted is
> sufficient.
> 
> 	Andrew

Apprently I have very limited knowledge in DSA area, because I didn't
know these features.

I will keep the patch series as is for now, to at least enable ethernet
in Elbert OpenBMC, and will work out new patch series to add BCM53134 in
DSA later.

Thanks again for the review.


Cheers,

Tao
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
index 27b43fe099f1..52cb617783ac 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-elbert.dts
@@ -183,3 +183,14 @@  imux31: i2c@7 {
 &i2c11 {
 	status = "okay";
 };
+
+&mac3 {
+	status = "okay";
+	phy-mode = "rgmii";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_rgmii4_default>;
+	fixed-link {
+		speed = <1000>;
+		full-duplex;
+	};
+};