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 E3793C433F5 for ; Mon, 8 Nov 2021 23:43:20 +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 2776960E54 for ; Mon, 8 Nov 2021 23:43:20 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 2776960E54 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org 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 2535F83895; Tue, 9 Nov 2021 00:43:18 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="sFdNi6L/"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 886D683896; Tue, 9 Nov 2021 00:43:16 +0100 (CET) Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) (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 BB89983886 for ; Tue, 9 Nov 2021 00:43:12 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=kabel@kernel.org Received: by mail.kernel.org (Postfix) with ESMTPSA id C3B4360E54; Mon, 8 Nov 2021 23:43:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1636414990; bh=6OpQQy0ueJOsEyHESYIM9hLgQM4kAkemk1QW/HDkdqM=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=sFdNi6L/kLMsvTY5C341QmdDFoiIqlhGovHHY1TA0SAm6JarDSoLrrVh1eNKQ/I2q dfOFKQL2QLmBR5IetDE+vOzFb79EbAPDw1p0Ur2tfF9985CRSr9d21d09f3cjmzuuJ N2Aur29kph2F2ezlNNdMZ+hxxuWxmn3kmX1hMDVmyNNg47VjwgAUx7kv8DprNDaHee haR1CYwBHbvINZnesHW87Bcq9Bi9XcfIQC05q/5Dm0TzhBBeE/VGzdspRmeMefuaT2 fUYqp0jktNt+FEikQsZvon7pmgtGU8i8Q+bg0hVX7Ydb5KDCjYBIaDnmCdetSBtztZ IJmhRhZYuzd/g== Date: Tue, 9 Nov 2021 00:43:06 +0100 From: Marek =?UTF-8?B?QmVow7pu?= To: Roman Bacik Cc: U-Boot Mailing List , Pali Rohar , Bharat Gooty , Joe Hershberger , Ramon Fried Subject: Re: [PATCH v10 1/2] net: brcm: netXtreme driver Message-ID: <20211109004306.6203a36a@thinkpad> In-Reply-To: <20211108144556.v10.1.I1edaad77041c1300213c307eef6741499504047@changeid> References: <20211108144556.v10.1.I1edaad77041c1300213c307eef6741499504047@changeid> 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, some last requests from me. On Mon, 8 Nov 2021 14:46:10 -0800 Roman Bacik wrote: > +#define bnxt_down_chip(bp) bnxt_hwrm_run(down_chip, bp, 0) > +#define bnxt_bring_chip(bp) bnxt_hwrm_run(bring_chip, bp, 1) Could these be changed to functions instead of macros, please? > +int bnxt_free_rx_iob(struct bnxt *bp) > +{ > + unsigned int i; > + > + if (!(FLAG_TEST(bp->flag_hwrm, VALID_RX_IOB))) > + return STATUS_SUCCESS; Please change all STATUS_SUCCESS to 0 and STATUS_FAILURE to either -1 or appropriate -errno, as is customary in U-Boot. At first I thought that you have implemented this driver by starting from kernel's implementation. They look very similar. But it was probably an old version of kernel implementation (perhaps broadcom internal?), because many things are different now. > +static void set_rx_desc(u8 *buf, void *iob, u16 cons_id, u32 iob_idx) > +{ > + struct rx_prod_pkt_bd *desc; > + u16 off = cons_id * sizeof(struct rx_prod_pkt_bd); > + > + desc = (struct rx_prod_pkt_bd *)&buf[off]; > + desc->flags_type = RX_PROD_PKT_BD_TYPE_RX_PROD_PKT; > + desc->len = MAX_ETHERNET_PACKET_BUFFER_SIZE; What bugs me with this driver most is that it reimplements many things on its own. MAX_ETHERNET_PACKET_BUFFER_SIZE is 1536, but we have PKTSIZE_ALIGN in include/net.h for that. > + bp->link_status = STATUS_LINK_DOWN; This can be a simple bool: link is either up or down... > +typedef int (*hwrm_func_t)(struct bnxt *bp); > + > +hwrm_func_t down_chip[] = { > + bnxt_hwrm_cfa_l2_filter_free, /* Free l2 filter */ > + bnxt_free_rx_iob, /* Free rx iob */ > + bnxt_hwrm_vnic_free, /* Free vnic */ > + bnxt_hwrm_ring_free_grp, /* Free ring group */ > + bnxt_hwrm_ring_free_rx, /* Free rx ring */ > + bnxt_hwrm_ring_free_tx, /* Free tx ring */ > + bnxt_hwrm_ring_free_cq, /* Free CQ ring */ > + bnxt_hwrm_stat_ctx_free, /* Free Stat ctx */ > + bnxt_hwrm_func_drv_unrgtr, /* unreg driver */ > + NULL, > +}; > + > +hwrm_func_t bring_chip[] = { > + bnxt_hwrm_ver_get, /* HWRM_VER_GET */ > + bnxt_hwrm_func_reset_req, /* HWRM_FUNC_RESET */ > + bnxt_hwrm_func_drv_rgtr, /* HWRM_FUNC_DRV_RGTR */ > + bnxt_hwrm_func_resource_qcaps, /* HWRM_FUNC_RESOURCE_QCAPS */ > + bnxt_hwrm_func_qcfg_req, /* HWRM_FUNC_QCFG */ > + bnxt_hwrm_func_qcaps_req, /* HWRM_FUNC_QCAPS */ > + bnxt_hwrm_get_link_speed, /* HWRM_NVM_GET_VARIABLE - 203 */ > + bnxt_hwrm_port_mac_cfg, /* HWRM_PORT_MAC_CFG */ > + bnxt_qphy_link, /* HWRM_PORT_PHY_QCFG */ > + bnxt_hwrm_func_cfg_req, /* HWRM_FUNC_CFG - ring resource*/ > + bnxt_hwrm_stat_ctx_alloc, /* Allocate Stat Ctx ID */ > + bnxt_hwrm_ring_alloc_cq, /* Allocate CQ Ring */ > + bnxt_hwrm_ring_alloc_tx, /* Allocate Tx ring */ > + bnxt_hwrm_ring_alloc_rx, /* Allocate Rx Ring */ > + bnxt_hwrm_ring_alloc_grp, /* Create Ring Group */ > + post_rx_buffers, /* Post RX buffers */ > + bnxt_hwrm_set_async_event, /* ENABLES_ASYNC_EVENT_CR */ > + bnxt_hwrm_vnic_alloc, /* Alloc VNIC */ > + bnxt_hwrm_vnic_cfg, /* Config VNIC */ > + bnxt_hwrm_cfa_l2_filter_alloc, /* Alloc L2 Filter */ > + get_phy_link, /* Get Physical Link */ > + NULL, > +}; > + > +int bnxt_hwrm_run(hwrm_func_t cmds[], struct bnxt *bp, int flag) > +{ > + hwrm_func_t *ptr; > + int ret; > + int status = STATUS_SUCCESS; > + > + for (ptr = cmds; *ptr; ++ptr) { > + ret = (*ptr)(bp); > + if (ret) { > + status = STATUS_FAILURE; > + /* Continue till all cleanup routines are called */ > + if (flag) > + return STATUS_FAILURE; Please change this function to return 0 on success and the error value from last failed function on failure: int ret = 0; for (...) { ret = (*ptr)(bp); if (ret) break; } return ret; If you can, maybe return -errno codes that make sense on failures in places where you now return STATUS_FAILURE. I'll be honest that I don't like how this driver uses code construct that are many time different from that in U-Boot/Linux. I guess this is how the Broadcom people wrote it, and it probably looked this way also in Linux, but was changed since then to conform to Linux style. (Or maybe they didn't accept it until it conformed?) Anyway, these are probably the last changes that I will be suggesting. I don't like many things here, but I guess beggars cannot be choosers, and we in U-Boot are more on the side of beggars when talking about man-hours companies are willing to spend for U-Boot :( Since the driver works for you, maybe we should accept it as it is. Marek