Hello Roman,
On Thu, 28 Oct 2021 16:29:28 -0700 Roman Bacik roman.bacik@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