Hello Roman,
On Thu, 28 Oct 2021 16:29:28 -0700
Roman Bacik <roman.ba...@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...
- 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
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()
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
I have another question: does this driver support adapters with SFP
cages only, or also those with RJ-45 ports?
Thank you.
Marek
