From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0DCB4C433F5 for ; Sat, 30 Oct 2021 15:00:17 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 34F3460FC4 for ; Sat, 30 Oct 2021 15:00:16 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 34F3460FC4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=nic.cz Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 0D43582DA1; Sat, 30 Oct 2021 17:00:13 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=nic.cz Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; secure) header.d=nic.cz header.i=@nic.cz header.b="MtTMDU9a"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id C5A478291E; Sat, 30 Oct 2021 17:00:10 +0200 (CEST) Received: from mail.nic.cz (mail.nic.cz [217.31.204.67]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 2E8EC833D8 for ; Sat, 30 Oct 2021 17:00:06 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=nic.cz Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=marek.behun@nic.cz Received: from thinkpad (unknown [172.20.6.87]) by mail.nic.cz (Postfix) with ESMTPSA id 90C1714143F; Sat, 30 Oct 2021 17:00:05 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=nic.cz; s=default; t=1635606005; bh=i3i+JwzZi+Z3uPH+RaoFW/oEet+gy9Xhe8r/EkugMjM=; h=Date:From:To; b=MtTMDU9atCB2cMhEpReoVZt6BRBrQJem9xaPcUNOnSMfksiFIp+a2tMSkN1Xz/ImY zHg3Wn8pE3ObgXqBQ65aIswcCWmM8QKv7b1ElXQ2Ud5hBJlN1ZtSLyWt+gJe1621dI 76qJlWo1CQf3NTAtwc53UqhKDAXX/t01io4P5dc4= Date: Sat, 30 Oct 2021 17:00:03 +0200 From: Marek =?UTF-8?B?QmVow7pu?= To: Roman Bacik Cc: U-Boot Mailing List , Bharat Gooty , Joe Hershberger , Ramon Fried , pali@kernel.org Subject: Re: [PATCH v4 1/2] net: brcm: netXtreme driver Message-ID: <20211030170003.20219284@thinkpad> In-Reply-To: <20211028232929.16607-1-roman.bacik@broadcom.com> References: <20211028232929.16607-1-roman.bacik@broadcom.com> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean Hello Roman, On Thu, 28 Oct 2021 16:29:28 -0700 Roman Bacik 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