[PATCH v4 1/2] net: brcm: netXtreme driver

[Roman Bacik]

Hi Marek,

On Sat, Oct 30, 2021 at 8:00 AM Marek Behún <marek.behun at nic.cz> wrote:
>
> Hello Roman,
>
> On Thu, 28 Oct 2021 16:29:28 -0700
> Roman Bacik <roman.bacik at broadcom.com> wrote:
>
> > +void bnxt_env_set_ethaddr(struct udevice *dev)
> > +{
> > +     struct eth_pdata *plat = dev_get_plat(dev);
> > +     char cmd[100];
> > +     char var[32];
> > +     u8 mac_env[ARP_HLEN];
> > +
> > +     eth_env_get_enetaddr_by_index("eth", dev_seq(dev), mac_env);
> > +     if (!memcmp(plat->enetaddr, mac_env, ARP_HLEN))
> > +             return;
> > +
> > +     sprintf(var, dev_seq(dev) ? "%s%daddr" : "%saddr", "eth", dev_seq(dev));
> > +     sprintf(cmd, "%s %s %pM", "env set -f", var, plat->enetaddr);
> > +     run_command(cmd, CMD_FLAG_ENV);
> > +}
> > +
> > +void bnxt_env_del_ethaddr(struct udevice *dev)
> > +{
> > +     struct eth_pdata *plat = dev_get_plat(dev);
> > +     char cmd[100];
> > +     char var[32];
> > +
> > +     sprintf(var, dev_seq(dev) ? "%s%daddr" : "%saddr", "eth", dev_seq(dev));
> > +     sprintf(cmd, "%s %s %pM", "env delete -f", var, plat->enetaddr);
> > +     run_command(cmd, CMD_FLAG_ENV);
> > +}
>
> And then in bnxt_eth_probe():
> > +     eth_env_get_enetaddr_by_index("eth", dev_seq(dev), bp->mac_set);
> ...
> > +     memcpy(plat->enetaddr, bp->mac_set, ETH_ALEN);
> > +     bnxt_env_set_ethaddr(dev);
>
> So if I understand this correctly, in bnxt_eth_probe(), you read env
> variable ethNaddr into bp->mac_set. Then bnxt_bring_chip() is called,
> which calls various functions, including bnxt_hwrm_func_qcaps_req(),
> which may overwrite bp->mac_set. Then bp->mac_set is copied into
> plat->enetaddr.
>
> Then bnxt_env_set_ethaddr() is called, which reads the env variable
> ethNaddr again, and compares it with value in plat->enetaddr, and if
> they are different, it overwrites the value in ethNaddr with
> plat->enetaddr.
>
> I have this to say:
> - could you please explain why this is done so? I mean the logic behind
>   this...

We read env to bp->mac_set in case hw does not have eth addr so it can
use the random value from uboot. The bnxt_bring_chip will read mac
programmed to hw nvram and set it into bp if available. The adress in hw and
address in plat must be the same in the end.

> - it seems to me that you haven't read the documentation for struct
>   env_ops in include/net.h: there are methods read_rom_hwaddr() and
>   write_hwaddr(), which you could use, instead of implementing this

The mac address can be read from bp only after bnxt_bring_chip is
called, which is after read_rom_hwaddr is called. Theoretically we can
call bnxt_bring_chip in bind or in read_rom_hwaddr and then one can
use this method if desired.

>   whole mechanism ad-hoc. You should use those or explain your reasons
>   why you aren't doing this
> - why do you need the plat structure? Why not use bp->mac_set directly,
>   without plat->enetaddr?
> - the way you set and delete ethNaddr variable, by running U-Boot
>   commands, is very weird, when there is a direct function for setting
>   the value:
>     eth_env_set_enetaddr_by_index()

We can change and use this method.

>   As for deleting:
>   - why do you need it in the first place? I mean you are removing the
>     variable upon driver removal. No other ethernet driver does that...
>   - instead of assembling the "env delete" command it would make far
>     more sense to include patch that adds env_del() function into
>     env/common.c

You are right, we can remove deleting. It is there because our earlier
logic depended on it.

>
> I have another question: does this driver support adapters with SFP
> cages only, or also those with RJ-45 ports?

Boards we have use SFP ports only. But theoretically one can use 10G
traffic with RJ45. Lower than 10G speeds like 1G traffic are not
supported by HW.

>
> Thank you.
>
> Marek

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.