Different Marvell DSA switches support different size of max frame
bytes to be sent.
For example mv88e6185 supports max 1632 bytes, which is now in-driver
standard value. On the other hand - mv88e6250 supports 2048 bytes.
As this value is internal and may be different for each switch IC,
new entry in struct mv88e6xxx_info has been added to store it.
Signed-off-by: Lukasz Majewski <[email protected]>
---
Changes for v2:
- Define max_frame_size with default value of 1632 bytes,
- Set proper value for the mv88e6250 switch SoC (linkstreet) family
Changes for v3:
- Add default value for 1632B of the max frame size (to avoid problems
with not defined values)
---
drivers/net/dsa/mv88e6xxx/chip.c | 13 ++++++++++++-
drivers/net/dsa/mv88e6xxx/chip.h | 1 +
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 242b8b325504..19668e549391 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
if (chip->info->ops->port_set_jumbo_size)
return 10240 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
else if (chip->info->ops->set_max_frame_size)
- return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
+ return (max_t(int, chip->info->max_frame_size, 1632)
+ - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN);
+
return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
}
@@ -4955,6 +4957,7 @@ static const struct mv88e6xxx_ops mv88e6250_ops = {
.avb_ops = &mv88e6352_avb_ops,
.ptp_ops = &mv88e6250_ptp_ops,
.phylink_get_caps = mv88e6250_phylink_get_caps,
+ .set_max_frame_size = mv88e6185_g1_set_max_frame_size,
};
static const struct mv88e6xxx_ops mv88e6290_ops = {
@@ -5574,6 +5577,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
.atu_move_port_mask = 0xf,
.multi_chip = true,
.ops = &mv88e6095_ops,
+ .max_frame_size = 1632,
},
[MV88E6097] = {
@@ -5598,6 +5602,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
.multi_chip = true,
.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
.ops = &mv88e6097_ops,
+ .max_frame_size = 1632,
},
[MV88E6123] = {
@@ -5622,6 +5627,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
.multi_chip = true,
.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
.ops = &mv88e6123_ops,
+ .max_frame_size = 1632,
},
[MV88E6131] = {
@@ -5692,6 +5698,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
.ptp_support = true,
.ops = &mv88e6161_ops,
+ .max_frame_size = 1632,
},
[MV88E6165] = {
@@ -5716,6 +5723,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
.multi_chip = true,
.ptp_support = true,
.ops = &mv88e6165_ops,
+ .max_frame_size = 1632,
},
[MV88E6171] = {
@@ -5835,6 +5843,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
.multi_chip = true,
.edsa_support = MV88E6XXX_EDSA_SUPPORTED,
.ops = &mv88e6185_ops,
+ .max_frame_size = 1632,
},
[MV88E6190] = {
@@ -5968,6 +5977,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
.num_internal_phys = 2,
.invalid_port_mask = BIT(2) | BIT(3) | BIT(4),
.max_vid = 4095,
+ .max_frame_size = 2048,
.port_base_addr = 0x08,
.phy_base_addr = 0x00,
.global1_addr = 0x0f,
@@ -6015,6 +6025,7 @@ static const struct mv88e6xxx_info mv88e6xxx_table[] = {
.num_ports = 7,
.num_internal_phys = 5,
.max_vid = 4095,
+ .max_frame_size = 2048,
.port_base_addr = 0x08,
.phy_base_addr = 0x00,
.global1_addr = 0x0f,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index e693154cf803..55948ef56cd0 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -132,6 +132,7 @@ struct mv88e6xxx_info {
unsigned int num_gpio;
unsigned int max_vid;
unsigned int max_sid;
+ unsigned int max_frame_size;
unsigned int port_base_addr;
unsigned int phy_base_addr;
unsigned int global1_addr;
--
2.20.1
> @@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct dsa_switch *ds, int port)
> if (chip->info->ops->port_set_jumbo_size)
> return 10240 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> else if (chip->info->ops->set_max_frame_size)
> - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> + return (max_t(int, chip->info->max_frame_size, 1632)
> + - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN);
> +
> return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
I would also prefer if all this if/else logic is removed, and the code
simply returned chip->info->max_frame_size - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> @@ -132,6 +132,7 @@ struct mv88e6xxx_info {
> unsigned int num_gpio;
> unsigned int max_vid;
> unsigned int max_sid;
> + unsigned int max_frame_size;
It might be worth adding a comment here what this value actually
represents. We don't want any mixups where the value already has the
frame checksum removed for example.
Andrew
On Mon, Jan 02, 2023 at 04:02:07PM +0100, Lukasz Majewski wrote:
> Different Marvell DSA switches support different size of max frame
> bytes to be sent.
>
> For example mv88e6185 supports max 1632 bytes, which is now in-driver
> standard value. On the other hand - mv88e6250 supports 2048 bytes.
>
> As this value is internal and may be different for each switch IC,
> new entry in struct mv88e6xxx_info has been added to store it.
>
> Signed-off-by: Lukasz Majewski <[email protected]>
>
> ---
> Changes for v2:
> - Define max_frame_size with default value of 1632 bytes,
> - Set proper value for the mv88e6250 switch SoC (linkstreet) family
>
> Changes for v3:
> - Add default value for 1632B of the max frame size (to avoid problems
> with not defined values)
I would add a WARN_ON_ONCE(!chip->info->max_frame_size) so a missing
value is detected. You can then skip the complexity of a default.
Andrew
Hi Andrew,
> > @@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct
> > dsa_switch *ds, int port) if (chip->info->ops->port_set_jumbo_size)
> > return 10240 - VLAN_ETH_HLEN - EDSA_HLEN -
> > ETH_FCS_LEN; else if (chip->info->ops->set_max_frame_size)
> > - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN -
> > ETH_FCS_LEN;
> > + return (max_t(int, chip->info->max_frame_size,
> > 1632)
> > + - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN);
> > +
> > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
>
> I would also prefer if all this if/else logic is removed, and the code
> simply returned chip->info->max_frame_size - VLAN_ETH_HLEN -
> EDSA_HLEN - ETH_FCS_LEN;
>
So then the mv88e6xxx_get_max_mtu shall look like:
WARN_ON_ONCE(!chip->info->max_frame_size)
if (chip->info->ops->port_set_jumbo_size)
...
else
return chip->info->max_frame_size - VLAN_ETH_HLEN -
EDSA_HLEN - ETH_FCS_LEN;
Or shall I put WARN_ON_ONCE to the mv88e6xxx_probe() function?
The above approach is contrary to one proposed by Alexander, who wanted
to improve the defensive approach in this driver (to avoid situation
where the max_frame_size callback is not defined and max_frame_size
member of *_info struct is not added by developer).
Which approach is the recommended one for this driver?
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> > @@ -132,6 +132,7 @@ struct mv88e6xxx_info {
> > unsigned int num_gpio;
> > unsigned int max_vid;
> > unsigned int max_sid;
> > + unsigned int max_frame_size;
>
> It might be worth adding a comment here what this value actually
> represents.
Ok. I will add proper comment.
> We don't want any mixups where the value already has the
> frame checksum removed for example.
Could you be more specific here about this use case?
The max_frame_size is the maximal size of the ethernet frame for which
the IC designer provided specified amount of RAM (it is a different
value for different SoCs in the Link Street family).
>
> Andrew
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: [email protected]
On Tue, Jan 03, 2023 at 10:02:51AM +0100, Lukasz Majewski wrote:
> Hi Andrew,
>
> > > @@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct
> > > dsa_switch *ds, int port) if (chip->info->ops->port_set_jumbo_size)
> > > return 10240 - VLAN_ETH_HLEN - EDSA_HLEN -
> > > ETH_FCS_LEN; else if (chip->info->ops->set_max_frame_size)
> > > - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN -
> > > ETH_FCS_LEN;
> > > + return (max_t(int, chip->info->max_frame_size,
> > > 1632)
> > > + - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN);
> > > +
> > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> >
> > I would also prefer if all this if/else logic is removed, and the code
> > simply returned chip->info->max_frame_size - VLAN_ETH_HLEN -
> > EDSA_HLEN - ETH_FCS_LEN;
> >
>
> So then the mv88e6xxx_get_max_mtu shall look like:
>
> WARN_ON_ONCE(!chip->info->max_frame_size)
>
> if (chip->info->ops->port_set_jumbo_size)
> ...
> else
> return chip->info->max_frame_size - VLAN_ETH_HLEN -
> EDSA_HLEN - ETH_FCS_LEN;
I think it should go even further:
{
WARN_ON_ONCE(!chip->info->max_frame_size)
return chip->info->max_frame_size - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
}
If we are going to use info->max_frame_size, we should always use
info->max_frame_size.
Andrew
Hi Andrew, Alexander,
> Hi Andrew,
>
> > > @@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct
> > > dsa_switch *ds, int port) if
> > > (chip->info->ops->port_set_jumbo_size) return 10240 -
> > > VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; else if
> > > (chip->info->ops->set_max_frame_size)
> > > - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN -
> > > ETH_FCS_LEN;
> > > + return (max_t(int, chip->info->max_frame_size,
> > > 1632)
> > > + - VLAN_ETH_HLEN - EDSA_HLEN -
> > > ETH_FCS_LEN); +
> > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> > >
> >
> > I would also prefer if all this if/else logic is removed, and the
> > code simply returned chip->info->max_frame_size - VLAN_ETH_HLEN -
> > EDSA_HLEN - ETH_FCS_LEN;
> >
>
> So then the mv88e6xxx_get_max_mtu shall look like:
>
> WARN_ON_ONCE(!chip->info->max_frame_size)
>
> if (chip->info->ops->port_set_jumbo_size)
> ...
> else
> return chip->info->max_frame_size - VLAN_ETH_HLEN -
> EDSA_HLEN - ETH_FCS_LEN;
>
>
> Or shall I put WARN_ON_ONCE to the mv88e6xxx_probe() function?
>
>
> The above approach is contrary to one proposed by Alexander, who
> wanted to improve the defensive approach in this driver (to avoid
> situation where the max_frame_size callback is not defined and
> max_frame_size member of *_info struct is not added by developer).
>
> Which approach is the recommended one for this driver?
Is there any decision regarding the preferred approach to rewrite this
code?
>
> > > +++ b/drivers/net/dsa/mv88e6xxx/chip.h
> > > @@ -132,6 +132,7 @@ struct mv88e6xxx_info {
> > > unsigned int num_gpio;
> > > unsigned int max_vid;
> > > unsigned int max_sid;
> > > + unsigned int max_frame_size;
> >
> > It might be worth adding a comment here what this value actually
> > represents.
>
> Ok. I will add proper comment.
>
> > We don't want any mixups where the value already has the
> > frame checksum removed for example.
>
> Could you be more specific here about this use case?
>
> The max_frame_size is the maximal size of the ethernet frame for which
> the IC designer provided specified amount of RAM (it is a different
> value for different SoCs in the Link Street family).
>
> >
> > Andrew
>
>
> Best regards,
>
> Lukasz Majewski
>
> --
>
> DENX Software Engineering GmbH, Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> [email protected]
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: [email protected]
On Thu, Jan 5, 2023 at 2:37 AM Lukasz Majewski <[email protected]> wrote:
>
> Hi Andrew, Alexander,
>
> > Hi Andrew,
> >
> > > > @@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct
> > > > dsa_switch *ds, int port) if
> > > > (chip->info->ops->port_set_jumbo_size) return 10240 -
> > > > VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; else if
> > > > (chip->info->ops->set_max_frame_size)
> > > > - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN -
> > > > ETH_FCS_LEN;
> > > > + return (max_t(int, chip->info->max_frame_size,
> > > > 1632)
> > > > + - VLAN_ETH_HLEN - EDSA_HLEN -
> > > > ETH_FCS_LEN); +
> > > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> > > >
> > >
> > > I would also prefer if all this if/else logic is removed, and the
> > > code simply returned chip->info->max_frame_size - VLAN_ETH_HLEN -
> > > EDSA_HLEN - ETH_FCS_LEN;
> > >
> >
> > So then the mv88e6xxx_get_max_mtu shall look like:
> >
> > WARN_ON_ONCE(!chip->info->max_frame_size)
> >
> > if (chip->info->ops->port_set_jumbo_size)
> > ...
> > else
> > return chip->info->max_frame_size - VLAN_ETH_HLEN -
> > EDSA_HLEN - ETH_FCS_LEN;
> >
> >
> > Or shall I put WARN_ON_ONCE to the mv88e6xxx_probe() function?
> >
> >
> > The above approach is contrary to one proposed by Alexander, who
> > wanted to improve the defensive approach in this driver (to avoid
> > situation where the max_frame_size callback is not defined and
> > max_frame_size member of *_info struct is not added by developer).
> >
> > Which approach is the recommended one for this driver?
>
> Is there any decision regarding the preferred approach to rewrite this
> code?
I would defer to what Andrew proposed since he has more experience
with the DSA code than I do.
Thanks,
- Alex
Hi Alexander
> On Thu, Jan 5, 2023 at 2:37 AM Lukasz Majewski <[email protected]> wrote:
> >
> > Hi Andrew, Alexander,
> >
> > > Hi Andrew,
> > >
> > > > > @@ -3548,7 +3548,9 @@ static int mv88e6xxx_get_max_mtu(struct
> > > > > dsa_switch *ds, int port) if
> > > > > (chip->info->ops->port_set_jumbo_size) return 10240 -
> > > > > VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN; else if
> > > > > (chip->info->ops->set_max_frame_size)
> > > > > - return 1632 - VLAN_ETH_HLEN - EDSA_HLEN -
> > > > > ETH_FCS_LEN;
> > > > > + return (max_t(int, chip->info->max_frame_size,
> > > > > 1632)
> > > > > + - VLAN_ETH_HLEN - EDSA_HLEN -
> > > > > ETH_FCS_LEN); +
> > > > > return 1522 - VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> > > > >
> > > >
> > > > I would also prefer if all this if/else logic is removed, and
> > > > the code simply returned chip->info->max_frame_size -
> > > > VLAN_ETH_HLEN - EDSA_HLEN - ETH_FCS_LEN;
> > > >
> > >
> > > So then the mv88e6xxx_get_max_mtu shall look like:
> > >
> > > WARN_ON_ONCE(!chip->info->max_frame_size)
> > >
> > > if (chip->info->ops->port_set_jumbo_size)
> > > ...
> > > else
> > > return chip->info->max_frame_size - VLAN_ETH_HLEN -
> > > EDSA_HLEN - ETH_FCS_LEN;
> > >
> > >
> > > Or shall I put WARN_ON_ONCE to the mv88e6xxx_probe() function?
> > >
> > >
> > > The above approach is contrary to one proposed by Alexander, who
> > > wanted to improve the defensive approach in this driver (to avoid
> > > situation where the max_frame_size callback is not defined and
> > > max_frame_size member of *_info struct is not added by developer).
> > >
> > > Which approach is the recommended one for this driver?
> >
> > Is there any decision regarding the preferred approach to rewrite
> > this code?
>
> I would defer to what Andrew proposed since he has more experience
> with the DSA code than I do.
>
Ok, then I will prepare v4 according to Andrew suggestions.
Thanks for the update :-)
> Thanks,
>
> - Alex
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: [email protected]