Re: [PATCHv2 net-next 2/4] net: dsa: mv88e6xxx: Monitor and Management tables

[Vivien Didelot <vivien.didelot@savoirfairelinux.com>]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Fri, Dec 02, 2016 at 02:32:39PM -0500, Vivien Didelot wrote:
>> Hi Andrew,
>> 
>> Andrew Lunn <andrew@lunn.ch> writes:
>> 
>> > @@ -3184,6 +3186,8 @@ static const struct mv88e6xxx_ops mv88e6085_ops = {
>> >  	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
>> >  	.stats_get_strings = mv88e6095_stats_get_strings,
>> >  	.stats_get_stats = mv88e6095_stats_get_stats,
>> > +	.g1_set_cpu_port = mv88e6095_g1_set_cpu_port,
>> > +	.g1_set_egress_port = mv88e6095_g1_set_egress_port,
>> >  };
>> 
>> I like the implementation in this version better. But please explain me
>> why you are prefixing these operations with g1_?
>
> The prefix gives some basic grouping. port_ indicates it operates on a
> port, and is likely to be found in port.c. stats_ indicates it
> operates on statistics, ppu that is operates on the phy polling unit.

Yes, port_* operations operate on ports. But the port.c file is there to
implement the function of "Port Registers". "Port" can be confusing, but
it refers to the SMI internal device at address 0xsomething.

"port_", "ppu_", "stats_", in the mv88e6xxx_ops structure just give
implicit namespaces for the **features**, not their location!

> We are going to have some things which don't fall into a simple
> category, like these two. But it would however be nice to group them,
> so i picked which register bank they are in. These operations are
> always in g1. It is a useful hint as to where to find the different
> variants.

Absolutely not!

    .set_egress_port = mv88e6095_g1_set_egress_port,

                                 ^
                                 That is the useful hint!

At the higher level of chip.c, we don't care about where is implemented
the switch MAC setter. We just have to call the correctly defined
.set_switch_mac routine.

However if you do care to know, its _ops.set_switch_mac pointer will
tell you (_g1 vs _g2 prefix).

>> But let's imagine we can set the CPU port in some Global 2 registers.
>> You are going to wrap this in chip.c with something like:
>> 
>>     int mv88e6xxx_set_cpu_port(struct mv88e6xxx_chip *chip, int port)
>>     {
>>         if (chip->info->ops->g2_set_cpu_port)
>>             return chip->info->ops->g2_set_cpu_port(chip, port);
>>         else if (chip->info->ops->g1_set_cpu_port)
>>             return chip->info->ops->g1_set_cpu_port(chip, port);
>>         else
>>             return -EOPNOTSUPP;
>>     }
>
> I answered in one of my other emails. Frames with reserved MAC
> addresses can be forwarded to the CPU. For most devices, this is a g2
> operation. However, for 6390, it is a g1. In that case, my code does
> not use a prefix. Not having a prefix, when all the others do, also
> gives you information. It means the ops are spread around and you need
> to make a bigger effort to go find them.

Again, absolutely not. This is your interpretation of having a prefix or
not. A chip has only one way to access a feature, not two. Since you
seem to be focused on the Rsvd2CPU feature, here's an example with it:

What's the point of writing this:

    /* Consider the given MAC as MGMT */
    int mv88e6xxx_reserve_mac(struct mv88e6xxx_chip *chip, u8 *addr)
    {
            if (mac_is_0x(addr)) {
                    if (chip->info->ops->g1_set_rsvd2cpu0)
                            return chip->info->ops->g1_set_rsvd2cpu0(...);
                    else if (chip->info->ops->g2_set_rsvd2cpu0)
                            return chip->info->ops->g2_set_rsvd2cpu0(...);
            } else if (mac_is_2x(addr)) {
                    if (chip->info->ops->g1_set_rsvd2cpu2)
                            return chip->info->ops->g1_set_rsvd2cpu2(...);
                    else if (chip->info->ops->g2_set_rsvd2cpu2)
                            return chip->info->ops->g2_set_rsvd2cpu2(...);
            }

            return mv88e6xxx_atu_load(chip, addr, MGMT);
    }

Compared to this:

    /* Consider the given MAC as MGMT */
    int mv88e6xxx_reserve_mac(struct mv88e6xxx_chip *chip, u8 *addr)
    {
            if (mac_is_0x(addr)) {
                    if (chip->info->ops->set_rsvd2cpu0)
                            return chip->info->ops->set_rsvd2cpu0(...);
            } else if (mac_is_2x(addr)) {
                    if (chip->info->ops->set_rsvd2cpu2)
                            return chip->info->ops->set_rsvd2cpu2(...);
            }

            return mv88e6xxx_atu_load(chip, addr, MGMT);
    }

Your higher level API (chip.c) doesn't need to know where is implemented
a given feature. It just needs to know if it supports it or not.

Thanks,

        Vivien