Add support to get mac from device-tree using of_get_mac_address.
Signed-off-by: Clément Léger <[email protected]>
---
drivers/net/ethernet/mscc/ocelot_vsc7514.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
index d51f799e4e86..c39118e5b3ee 100644
--- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
+++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
@@ -526,7 +526,10 @@ static int ocelot_chip_init(struct ocelot *ocelot, const struct ocelot_ops *ops)
ocelot_pll5_init(ocelot);
- eth_random_addr(ocelot->base_mac);
+ ret = of_get_mac_address(ocelot->dev->of_node, ocelot->base_mac);
+ if (ret)
+ eth_random_addr(ocelot->base_mac);
+
ocelot->base_mac[5] &= 0xf0;
return 0;
--
2.33.0
On Thu, Oct 28, 2021 at 03:49:30PM +0200, Cl?ment L?ger wrote:
> Add support to get mac from device-tree using of_get_mac_address.
>
> Signed-off-by: Cl?ment L?ger <[email protected]>
> ---
> drivers/net/ethernet/mscc/ocelot_vsc7514.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> index d51f799e4e86..c39118e5b3ee 100644
> --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> @@ -526,7 +526,10 @@ static int ocelot_chip_init(struct ocelot *ocelot, const struct ocelot_ops *ops)
>
> ocelot_pll5_init(ocelot);
>
> - eth_random_addr(ocelot->base_mac);
> + ret = of_get_mac_address(ocelot->dev->of_node, ocelot->base_mac);
Why not per port? This is pretty strange, I think.
> + if (ret)
> + eth_random_addr(ocelot->base_mac);
> +
> ocelot->base_mac[5] &= 0xf0;
>
> return 0;
> --
> 2.33.0
>
Le Thu, 28 Oct 2021 14:06:12 +0000,
Vladimir Oltean <[email protected]> a écrit :
> On Thu, Oct 28, 2021 at 03:49:30PM +0200, Clément Léger wrote:
> > Add support to get mac from device-tree using of_get_mac_address.
> >
> > Signed-off-by: Clément Léger <[email protected]>
> > ---
> > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index
> > d51f799e4e86..c39118e5b3ee 100644 ---
> > a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++
> > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -526,7 +526,10 @@
> > static int ocelot_chip_init(struct ocelot *ocelot, const struct
> > ocelot_ops *ops) ocelot_pll5_init(ocelot);
> >
> > - eth_random_addr(ocelot->base_mac);
> > + ret = of_get_mac_address(ocelot->dev->of_node,
> > ocelot->base_mac);
>
> Why not per port? This is pretty strange, I think.
Hi Vladimir,
Currently, all ports share the same base mac address (5 first
bytes). The final mac address per port is computed in ocelot_probe_port
by adding the port number as the last byte of the mac_address provided.
Clément
>
> > + if (ret)
> > + eth_random_addr(ocelot->base_mac);
> > +
> > ocelot->base_mac[5] &= 0xf0;
> >
> > return 0;
> > --
> > 2.33.0
>
On Thu, Oct 28, 2021 at 04:15:22PM +0200, Cl?ment L?ger wrote:
> Le Thu, 28 Oct 2021 14:06:12 +0000,
> Vladimir Oltean <[email protected]> a ?crit :
>
> > On Thu, Oct 28, 2021 at 03:49:30PM +0200, Cl?ment L?ger wrote:
> > > Add support to get mac from device-tree using of_get_mac_address.
> > >
> > > Signed-off-by: Cl?ment L?ger <[email protected]>
> > > ---
> > > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index
> > > d51f799e4e86..c39118e5b3ee 100644 ---
> > > a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++
> > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -526,7 +526,10 @@
> > > static int ocelot_chip_init(struct ocelot *ocelot, const struct
> > > ocelot_ops *ops) ocelot_pll5_init(ocelot);
> > >
> > > - eth_random_addr(ocelot->base_mac);
> > > + ret = of_get_mac_address(ocelot->dev->of_node,
> > > ocelot->base_mac);
> >
> > Why not per port? This is pretty strange, I think.
>
> Hi Vladimir,
>
> Currently, all ports share the same base mac address (5 first
> bytes). The final mac address per port is computed in ocelot_probe_port
> by adding the port number as the last byte of the mac_address provided.
>
> Cl?ment
Yes, I know that, but that's not my point.
Every switch port should be pretty much compliant with
ethernet-controller.yaml, if it could inherit that it would be even
better. And since mac-address is an ethernet-controller.yaml property,
it is pretty much non-obvious at all that you put the mac-address
property directly under the switch, and manually add 0, 1, 2, 3 etc to it.
My request was to parse the mac-address property of each port. Like
this:
base_mac = random;
for_each_port() {
err = of_get_mac_address(port_dn, &port_mac);
if (err)
port_mac = base_mac + port;
}
> > > + if (ret)
> > > + eth_random_addr(ocelot->base_mac);
> > > +
> > > ocelot->base_mac[5] &= 0xf0;
> > >
> > > return 0;
> > > --
> > > 2.33.0
> >
>
Le Thu, 28 Oct 2021 14:22:55 +0000,
Vladimir Oltean <[email protected]> a écrit :
> On Thu, Oct 28, 2021 at 04:15:22PM +0200, Clément Léger wrote:
> > Le Thu, 28 Oct 2021 14:06:12 +0000,
> > Vladimir Oltean <[email protected]> a écrit :
> >
> > > On Thu, Oct 28, 2021 at 03:49:30PM +0200, Clément Léger wrote:
> > > > Add support to get mac from device-tree using
> > > > of_get_mac_address.
> > > >
> > > > Signed-off-by: Clément Léger <[email protected]>
> > > > ---
> > > > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 5 ++++-
> > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> > > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index
> > > > d51f799e4e86..c39118e5b3ee 100644 ---
> > > > a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++
> > > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -526,7 +526,10
> > > > @@ static int ocelot_chip_init(struct ocelot *ocelot, const
> > > > struct ocelot_ops *ops) ocelot_pll5_init(ocelot);
> > > >
> > > > - eth_random_addr(ocelot->base_mac);
> > > > + ret = of_get_mac_address(ocelot->dev->of_node,
> > > > ocelot->base_mac);
> > >
> > > Why not per port? This is pretty strange, I think.
> >
> > Hi Vladimir,
> >
> > Currently, all ports share the same base mac address (5 first
> > bytes). The final mac address per port is computed in
> > ocelot_probe_port by adding the port number as the last byte of the
> > mac_address provided.
> >
> > Clément
>
> Yes, I know that, but that's not my point.
> Every switch port should be pretty much compliant with
> ethernet-controller.yaml, if it could inherit that it would be even
> better. And since mac-address is an ethernet-controller.yaml property,
> it is pretty much non-obvious at all that you put the mac-address
> property directly under the switch, and manually add 0, 1, 2, 3 etc
> to it. My request was to parse the mac-address property of each port.
> Like this:
>
> base_mac = random;
>
> for_each_port() {
> err = of_get_mac_address(port_dn, &port_mac);
> if (err)
> port_mac = base_mac + port;
> }
Ok indeed. So I will parse each port for a mac-address property. Do you
also want a fallback to use the switch base mac if not specified in
port or should I keep the use of a default random mac as the base
address anyway ?
>
> > > > + if (ret)
> > > > + eth_random_addr(ocelot->base_mac);
> > > > +
> > > > ocelot->base_mac[5] &= 0xf0;
> > > >
> > > > return 0;
> > > > --
> > > > 2.33.0
> > >
>
On Thu, Oct 28, 2021 at 04:38:25PM +0200, Cl?ment L?ger wrote:
> Le Thu, 28 Oct 2021 14:22:55 +0000,
> Vladimir Oltean <[email protected]> a ?crit :
>
> > On Thu, Oct 28, 2021 at 04:15:22PM +0200, Cl?ment L?ger wrote:
> > > Le Thu, 28 Oct 2021 14:06:12 +0000,
> > > Vladimir Oltean <[email protected]> a ?crit :
> > >
> > > > On Thu, Oct 28, 2021 at 03:49:30PM +0200, Cl?ment L?ger wrote:
> > > > > Add support to get mac from device-tree using
> > > > > of_get_mac_address.
> > > > >
> > > > > Signed-off-by: Cl?ment L?ger <[email protected]>
> > > > > ---
> > > > > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 5 ++++-
> > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> > > > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index
> > > > > d51f799e4e86..c39118e5b3ee 100644 ---
> > > > > a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++
> > > > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -526,7 +526,10
> > > > > @@ static int ocelot_chip_init(struct ocelot *ocelot, const
> > > > > struct ocelot_ops *ops) ocelot_pll5_init(ocelot);
> > > > >
> > > > > - eth_random_addr(ocelot->base_mac);
> > > > > + ret = of_get_mac_address(ocelot->dev->of_node,
> > > > > ocelot->base_mac);
> > > >
> > > > Why not per port? This is pretty strange, I think.
> > >
> > > Hi Vladimir,
> > >
> > > Currently, all ports share the same base mac address (5 first
> > > bytes). The final mac address per port is computed in
> > > ocelot_probe_port by adding the port number as the last byte of the
> > > mac_address provided.
> > >
> > > Cl?ment
> >
> > Yes, I know that, but that's not my point.
> > Every switch port should be pretty much compliant with
> > ethernet-controller.yaml, if it could inherit that it would be even
> > better. And since mac-address is an ethernet-controller.yaml property,
> > it is pretty much non-obvious at all that you put the mac-address
> > property directly under the switch, and manually add 0, 1, 2, 3 etc
> > to it. My request was to parse the mac-address property of each port.
> > Like this:
> >
> > base_mac = random;
> >
> > for_each_port() {
> > err = of_get_mac_address(port_dn, &port_mac);
> > if (err)
> > port_mac = base_mac + port;
> > }
>
> Ok indeed. So I will parse each port for a mac-address property. Do you
> also want a fallback to use the switch base mac if not specified in
> port or should I keep the use of a default random mac as the base
> address anyway ?
Isn't the pseudocode I posted above explicit enough? Sorry...
Keep doing what the driver is doing right now, with an optional
mac-address override per port.
Why would we read the mac-address property of the switch? Which other
switch driver does that? Are there device trees in circulation where
this is being done?
> > > > > + if (ret)
> > > > > + eth_random_addr(ocelot->base_mac);
> > > > > +
> > > > > ocelot->base_mac[5] &= 0xf0;
> > > > >
> > > > > return 0;
> > > > > --
> > > > > 2.33.0
> > > >
> >
>
Le Thu, 28 Oct 2021 14:51:43 +0000,
Vladimir Oltean <[email protected]> a écrit :
> On Thu, Oct 28, 2021 at 04:38:25PM +0200, Clément Léger wrote:
> > Le Thu, 28 Oct 2021 14:22:55 +0000,
> > Vladimir Oltean <[email protected]> a écrit :
> >
> > > On Thu, Oct 28, 2021 at 04:15:22PM +0200, Clément Léger wrote:
> > > > Le Thu, 28 Oct 2021 14:06:12 +0000,
> > > > Vladimir Oltean <[email protected]> a écrit :
> > > >
> > > > > On Thu, Oct 28, 2021 at 03:49:30PM +0200, Clément Léger
> > > > > wrote:
> > > > > > Add support to get mac from device-tree using
> > > > > > of_get_mac_address.
> > > > > >
> > > > > > Signed-off-by: Clément Léger <[email protected]>
> > > > > > ---
> > > > > > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 5 ++++-
> > > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> > > > > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index
> > > > > > d51f799e4e86..c39118e5b3ee 100644 ---
> > > > > > a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++
> > > > > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -526,7
> > > > > > +526,10 @@ static int ocelot_chip_init(struct ocelot
> > > > > > *ocelot, const struct ocelot_ops *ops)
> > > > > > ocelot_pll5_init(ocelot);
> > > > > > - eth_random_addr(ocelot->base_mac);
> > > > > > + ret = of_get_mac_address(ocelot->dev->of_node,
> > > > > > ocelot->base_mac);
> > > > >
> > > > > Why not per port? This is pretty strange, I think.
> > > >
> > > > Hi Vladimir,
> > > >
> > > > Currently, all ports share the same base mac address (5 first
> > > > bytes). The final mac address per port is computed in
> > > > ocelot_probe_port by adding the port number as the last byte of
> > > > the mac_address provided.
> > > >
> > > > Clément
> > >
> > > Yes, I know that, but that's not my point.
> > > Every switch port should be pretty much compliant with
> > > ethernet-controller.yaml, if it could inherit that it would be
> > > even better. And since mac-address is an ethernet-controller.yaml
> > > property, it is pretty much non-obvious at all that you put the
> > > mac-address property directly under the switch, and manually add
> > > 0, 1, 2, 3 etc to it. My request was to parse the mac-address
> > > property of each port. Like this:
> > >
> > > base_mac = random;
> > >
> > > for_each_port() {
> > > err = of_get_mac_address(port_dn, &port_mac);
> > > if (err)
> > > port_mac = base_mac + port;
> > > }
> >
> > Ok indeed. So I will parse each port for a mac-address property. Do
> > you also want a fallback to use the switch base mac if not
> > specified in port or should I keep the use of a default random mac
> > as the base address anyway ?
>
> Isn't the pseudocode I posted above explicit enough? Sorry...
> Keep doing what the driver is doing right now, with an optional
> mac-address override per port.
> Why would we read the mac-address property of the switch? Which other
> switch driver does that? Are there device trees in circulation where
> this is being done?
Sorry, guess I'm a bit tired and missed out the base_mac = random...
Acked for the whole modification requested.
>
> > > > > > + if (ret)
> > > > > > + eth_random_addr(ocelot->base_mac);
> > > > > > +
> > > > > > ocelot->base_mac[5] &= 0xf0;
> > > > > >
> > > > > > return 0;
> > > > > > --
> > > > > > 2.33.0
> > > > >
> > >
>
Le Thu, 28 Oct 2021 14:51:43 +0000,
Vladimir Oltean <[email protected]> a écrit :
> On Thu, Oct 28, 2021 at 04:38:25PM +0200, Clément Léger wrote:
> > Le Thu, 28 Oct 2021 14:22:55 +0000,
> > Vladimir Oltean <[email protected]> a écrit :
> >
> > > On Thu, Oct 28, 2021 at 04:15:22PM +0200, Clément Léger wrote:
> > > > Le Thu, 28 Oct 2021 14:06:12 +0000,
> > > > Vladimir Oltean <[email protected]> a écrit :
> > > >
> > > > > On Thu, Oct 28, 2021 at 03:49:30PM +0200, Clément Léger
> > > > > wrote:
> > > > > > Add support to get mac from device-tree using
> > > > > > of_get_mac_address.
> > > > > >
> > > > > > Signed-off-by: Clément Léger <[email protected]>
> > > > > > ---
> > > > > > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 5 ++++-
> > > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> > > > > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index
> > > > > > d51f799e4e86..c39118e5b3ee 100644 ---
> > > > > > a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++
> > > > > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -526,7
> > > > > > +526,10 @@ static int ocelot_chip_init(struct ocelot
> > > > > > *ocelot, const struct ocelot_ops *ops)
> > > > > > ocelot_pll5_init(ocelot);
> > > > > > - eth_random_addr(ocelot->base_mac);
> > > > > > + ret = of_get_mac_address(ocelot->dev->of_node,
> > > > > > ocelot->base_mac);
> > > > >
> > > > > Why not per port? This is pretty strange, I think.
> > > >
> > > > Hi Vladimir,
> > > >
> > > > Currently, all ports share the same base mac address (5 first
> > > > bytes). The final mac address per port is computed in
> > > > ocelot_probe_port by adding the port number as the last byte of
> > > > the mac_address provided.
> > > >
> > > > Clément
> > >
> > > Yes, I know that, but that's not my point.
> > > Every switch port should be pretty much compliant with
> > > ethernet-controller.yaml, if it could inherit that it would be
> > > even better. And since mac-address is an ethernet-controller.yaml
> > > property, it is pretty much non-obvious at all that you put the
> > > mac-address property directly under the switch, and manually add
> > > 0, 1, 2, 3 etc to it. My request was to parse the mac-address
> > > property of each port. Like this:
> > >
> > > base_mac = random;
> > >
> > > for_each_port() {
> > > err = of_get_mac_address(port_dn, &port_mac);
> > > if (err)
> > > port_mac = base_mac + port;
> > > }
> >
> > Ok indeed. So I will parse each port for a mac-address property. Do
> > you also want a fallback to use the switch base mac if not
> > specified in port or should I keep the use of a default random mac
> > as the base address anyway ?
>
> Isn't the pseudocode I posted above explicit enough? Sorry...
> Keep doing what the driver is doing right now, with an optional
> mac-address override per port.
> Why would we read the mac-address property of the switch? Which other
> switch driver does that? Are there device trees in circulation where
> this is being done?
BTW, this is actually done on sparx5 switch driver.
>
> > > > > > + if (ret)
> > > > > > + eth_random_addr(ocelot->base_mac);
> > > > > > +
> > > > > > ocelot->base_mac[5] &= 0xf0;
> > > > > >
> > > > > > return 0;
> > > > > > --
> > > > > > 2.33.0
> > > > >
> > >
>
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com
On Fri, Oct 29, 2021 at 08:09:37AM +0200, Cl?ment L?ger wrote:
> Le Thu, 28 Oct 2021 14:51:43 +0000,
> Vladimir Oltean <[email protected]> a ?crit :
>
> > On Thu, Oct 28, 2021 at 04:38:25PM +0200, Cl?ment L?ger wrote:
> > > Le Thu, 28 Oct 2021 14:22:55 +0000,
> > > Vladimir Oltean <[email protected]> a ?crit :
> > >
> > > > On Thu, Oct 28, 2021 at 04:15:22PM +0200, Cl?ment L?ger wrote:
> > > > > Le Thu, 28 Oct 2021 14:06:12 +0000,
> > > > > Vladimir Oltean <[email protected]> a ?crit :
> > > > >
> > > > > > On Thu, Oct 28, 2021 at 03:49:30PM +0200, Cl?ment L?ger
> > > > > > wrote:
> > > > > > > Add support to get mac from device-tree using
> > > > > > > of_get_mac_address.
> > > > > > >
> > > > > > > Signed-off-by: Cl?ment L?ger <[email protected]>
> > > > > > > ---
> > > > > > > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 5 ++++-
> > > > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> > > > > > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index
> > > > > > > d51f799e4e86..c39118e5b3ee 100644 ---
> > > > > > > a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++
> > > > > > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -526,7
> > > > > > > +526,10 @@ static int ocelot_chip_init(struct ocelot
> > > > > > > *ocelot, const struct ocelot_ops *ops)
> > > > > > > ocelot_pll5_init(ocelot);
> > > > > > > - eth_random_addr(ocelot->base_mac);
> > > > > > > + ret = of_get_mac_address(ocelot->dev->of_node,
> > > > > > > ocelot->base_mac);
> > > > > >
> > > > > > Why not per port? This is pretty strange, I think.
> > > > >
> > > > > Hi Vladimir,
> > > > >
> > > > > Currently, all ports share the same base mac address (5 first
> > > > > bytes). The final mac address per port is computed in
> > > > > ocelot_probe_port by adding the port number as the last byte of
> > > > > the mac_address provided.
> > > > >
> > > > > Cl?ment
> > > >
> > > > Yes, I know that, but that's not my point.
> > > > Every switch port should be pretty much compliant with
> > > > ethernet-controller.yaml, if it could inherit that it would be
> > > > even better. And since mac-address is an ethernet-controller.yaml
> > > > property, it is pretty much non-obvious at all that you put the
> > > > mac-address property directly under the switch, and manually add
> > > > 0, 1, 2, 3 etc to it. My request was to parse the mac-address
> > > > property of each port. Like this:
> > > >
> > > > base_mac = random;
> > > >
> > > > for_each_port() {
> > > > err = of_get_mac_address(port_dn, &port_mac);
> > > > if (err)
> > > > port_mac = base_mac + port;
> > > > }
> > >
> > > Ok indeed. So I will parse each port for a mac-address property. Do
> > > you also want a fallback to use the switch base mac if not
> > > specified in port or should I keep the use of a default random mac
> > > as the base address anyway ?
> >
> > Isn't the pseudocode I posted above explicit enough? Sorry...
> > Keep doing what the driver is doing right now, with an optional
> > mac-address override per port.
> > Why would we read the mac-address property of the switch? Which other
> > switch driver does that? Are there device trees in circulation where
> > this is being done?
>
> BTW, this is actually done on sparx5 switch driver.
Highly inconsistent, but true. I'm saying that because
Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml says
that "mac-address" should be under "switch", but then proceeds to put it
under "port@64" in the example.
Most likely not caught during review, I'm not sure if this could be
considered good practice.
> > > > > > > + if (ret)
> > > > > > > + eth_random_addr(ocelot->base_mac);
> > > > > > > +
> > > > > > > ocelot->base_mac[5] &= 0xf0;
> > > > > > >
> > > > > > > return 0;
> > > > > > > --
> > > > > > > 2.33.0
> > > > > >
> > > >
> >
>
>
>
> --
> Cl?ment L?ger,
> Embedded Linux and Kernel engineer at Bootlin
> https://bootlin.com/
Le Fri, 29 Oct 2021 11:35:43 +0000,
Vladimir Oltean <[email protected]> a écrit :
> On Fri, Oct 29, 2021 at 08:09:37AM +0200, Clément Léger wrote:
> > Le Thu, 28 Oct 2021 14:51:43 +0000,
> > Vladimir Oltean <[email protected]> a écrit :
> >
> > > On Thu, Oct 28, 2021 at 04:38:25PM +0200, Clément Léger wrote:
> > > > Le Thu, 28 Oct 2021 14:22:55 +0000,
> > > > Vladimir Oltean <[email protected]> a écrit :
> > > >
> > > > > On Thu, Oct 28, 2021 at 04:15:22PM +0200, Clément Léger
> > > > > wrote:
> > > > > > Le Thu, 28 Oct 2021 14:06:12 +0000,
> > > > > > Vladimir Oltean <[email protected]> a écrit :
> > > > > >
> > > > > > > On Thu, Oct 28, 2021 at 03:49:30PM +0200, Clément Léger
> > > > > > > wrote:
> > > > > > > > Add support to get mac from device-tree using
> > > > > > > > of_get_mac_address.
> > > > > > > >
> > > > > > > > Signed-off-by: Clément Léger <[email protected]>
> > > > > > > > ---
> > > > > > > > drivers/net/ethernet/mscc/ocelot_vsc7514.c | 5 ++++-
> > > > > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> > > > > > > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c index
> > > > > > > > d51f799e4e86..c39118e5b3ee 100644 ---
> > > > > > > > a/drivers/net/ethernet/mscc/ocelot_vsc7514.c +++
> > > > > > > > b/drivers/net/ethernet/mscc/ocelot_vsc7514.c @@ -526,7
> > > > > > > > +526,10 @@ static int ocelot_chip_init(struct ocelot
> > > > > > > > *ocelot, const struct ocelot_ops *ops)
> > > > > > > > ocelot_pll5_init(ocelot);
> > > > > > > > - eth_random_addr(ocelot->base_mac);
> > > > > > > > + ret = of_get_mac_address(ocelot->dev->of_node,
> > > > > > > > ocelot->base_mac);
> > > > > > >
> > > > > > > Why not per port? This is pretty strange, I think.
> > > > > >
> > > > > > Hi Vladimir,
> > > > > >
> > > > > > Currently, all ports share the same base mac address (5
> > > > > > first bytes). The final mac address per port is computed in
> > > > > > ocelot_probe_port by adding the port number as the last
> > > > > > byte of the mac_address provided.
> > > > > >
> > > > > > Clément
> > > > >
> > > > > Yes, I know that, but that's not my point.
> > > > > Every switch port should be pretty much compliant with
> > > > > ethernet-controller.yaml, if it could inherit that it would be
> > > > > even better. And since mac-address is an
> > > > > ethernet-controller.yaml property, it is pretty much
> > > > > non-obvious at all that you put the mac-address property
> > > > > directly under the switch, and manually add 0, 1, 2, 3 etc to
> > > > > it. My request was to parse the mac-address property of each
> > > > > port. Like this:
> > > > >
> > > > > base_mac = random;
> > > > >
> > > > > for_each_port() {
> > > > > err = of_get_mac_address(port_dn, &port_mac);
> > > > > if (err)
> > > > > port_mac = base_mac + port;
> > > > > }
> > > >
> > > > Ok indeed. So I will parse each port for a mac-address
> > > > property. Do you also want a fallback to use the switch base
> > > > mac if not specified in port or should I keep the use of a
> > > > default random mac as the base address anyway ?
> > >
> > > Isn't the pseudocode I posted above explicit enough? Sorry...
> > > Keep doing what the driver is doing right now, with an optional
> > > mac-address override per port.
> > > Why would we read the mac-address property of the switch? Which
> > > other switch driver does that? Are there device trees in
> > > circulation where this is being done?
> >
> > BTW, this is actually done on sparx5 switch driver.
>
> Highly inconsistent, but true. I'm saying that because
> Documentation/devicetree/bindings/net/microchip,sparx5-switch.yaml
> says that "mac-address" should be under "switch", but then proceeds
> to put it under "port@64" in the example.
Agreed, additionally the driver uses the "mac-adress" property from the
switch node to initialize the base mac.
Anyway, I changed my patch to use mac-adress for each port and modified
the yaml bindings to include ethernet-controller.yaml and use a
"mac-address" property per port.
>
> Most likely not caught during review, I'm not sure if this could be
> considered good practice.
>
> > > > > > > > + if (ret)
> > > > > > > > + eth_random_addr(ocelot->base_mac);
> > > > > > > > +
> > > > > > > > ocelot->base_mac[5] &= 0xf0;
> > > > > > > >
> > > > > > > > return 0;
> > > > > > > > --
> > > > > > > > 2.33.0
> > > > > > >
> > > > >
> > >
> >
> >
> >
> > --
> > Clément Léger,
> > Embedded Linux and Kernel engineer at Bootlin
> > https://bootlin.com
--
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com