Re: [PATCH v10 1/2] net: brcm: netXtreme driver
On Tue, Nov 9, 2021 at 5:24 PM Roman Bacik wrote: > > On Tue, Nov 9, 2021 at 12:37 AM Ramon Fried wrote: > > > > On Tue, Nov 9, 2021 at 4:55 AM Marek Behún wrote: > > > > > > On Mon, 8 Nov 2021 18:20:43 -0800 > > > Roman Bacik wrote: > > > > > > > On Mon, Nov 8, 2021 at 5:12 PM Marek Behún wrote: > > > > > > > > > > On Mon, 8 Nov 2021 16:48:33 -0800 > > > > > Roman Bacik wrote: > > > > > > > > > > > To be honest changing status codes coming from FW does not seem > > > > > > right. But > > > > > > we will try to make the requested changes. > > > > > > > > > > I looked at kernel's implementation of this driver and these hwrm > > > > > functions and they don't return STATUS_*. > > > > > > > > > > Marek > > > > > > > > Marek, > > > > > > > > This is quite a different driver and it was written for uboot. > > > > > > Hello Roman > > > > > > The drivers clearly have a common ancestor, there are far too many > > > similarities. It clearly wasn't written from scratch for U-Boot. > > > > > > > If the > > > > main objection is that Linux driver is different then maybe we should > > > > use v10 as is. Currently hwrm methods return HW status and bnxt > > > > methods return uboot error codes consistently. > > > > > > I will leave this to U-Boot's network subsystem maintainers. As I said, > > > beggars cannot be choosers in U-Boot. As long as the driver does not > > > introduce vendor specific stuff to the user API (in U-Boot command > > > line), then I guess I'll have to be satisfied. > > > > > > Marek > > I'll accept the v10, you all made good comments and surely the patch > > looks much better then now. > > Thanks, > > Ramon. > > Ramon, Marek, Pali, > > Thank you very much for your comments and reviewing our code. > Regards, > > Roman > > -- > 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. Applied to u-boot-net/next, Thanks.
Re: [PATCH v10 1/2] net: brcm: netXtreme driver
On Tue, Nov 9, 2021 at 12:37 AM Ramon Fried wrote: > > On Tue, Nov 9, 2021 at 4:55 AM Marek Behún wrote: > > > > On Mon, 8 Nov 2021 18:20:43 -0800 > > Roman Bacik wrote: > > > > > On Mon, Nov 8, 2021 at 5:12 PM Marek Behún wrote: > > > > > > > > On Mon, 8 Nov 2021 16:48:33 -0800 > > > > Roman Bacik wrote: > > > > > > > > > To be honest changing status codes coming from FW does not seem > > > > > right. But > > > > > we will try to make the requested changes. > > > > > > > > I looked at kernel's implementation of this driver and these hwrm > > > > functions and they don't return STATUS_*. > > > > > > > > Marek > > > > > > Marek, > > > > > > This is quite a different driver and it was written for uboot. > > > > Hello Roman > > > > The drivers clearly have a common ancestor, there are far too many > > similarities. It clearly wasn't written from scratch for U-Boot. > > > > > If the > > > main objection is that Linux driver is different then maybe we should > > > use v10 as is. Currently hwrm methods return HW status and bnxt > > > methods return uboot error codes consistently. > > > > I will leave this to U-Boot's network subsystem maintainers. As I said, > > beggars cannot be choosers in U-Boot. As long as the driver does not > > introduce vendor specific stuff to the user API (in U-Boot command > > line), then I guess I'll have to be satisfied. > > > > Marek > I'll accept the v10, you all made good comments and surely the patch > looks much better then now. > Thanks, > Ramon. Ramon, Marek, Pali, Thank you very much for your comments and reviewing our code. Regards, Roman -- 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.
Re: [PATCH v10 1/2] net: brcm: netXtreme driver
On Tue, Nov 9, 2021 at 4:55 AM Marek Behún wrote: > > On Mon, 8 Nov 2021 18:20:43 -0800 > Roman Bacik wrote: > > > On Mon, Nov 8, 2021 at 5:12 PM Marek Behún wrote: > > > > > > On Mon, 8 Nov 2021 16:48:33 -0800 > > > Roman Bacik wrote: > > > > > > > To be honest changing status codes coming from FW does not seem right. > > > > But > > > > we will try to make the requested changes. > > > > > > I looked at kernel's implementation of this driver and these hwrm > > > functions and they don't return STATUS_*. > > > > > > Marek > > > > Marek, > > > > This is quite a different driver and it was written for uboot. > > Hello Roman > > The drivers clearly have a common ancestor, there are far too many > similarities. It clearly wasn't written from scratch for U-Boot. > > > If the > > main objection is that Linux driver is different then maybe we should > > use v10 as is. Currently hwrm methods return HW status and bnxt > > methods return uboot error codes consistently. > > I will leave this to U-Boot's network subsystem maintainers. As I said, > beggars cannot be choosers in U-Boot. As long as the driver does not > introduce vendor specific stuff to the user API (in U-Boot command > line), then I guess I'll have to be satisfied. > > Marek I'll accept the v10, you all made good comments and surely the patch looks much better then now. Thanks, Ramon.
Re: [PATCH v10 1/2] net: brcm: netXtreme driver
On Mon, 8 Nov 2021 18:20:43 -0800 Roman Bacik wrote: > On Mon, Nov 8, 2021 at 5:12 PM Marek Behún wrote: > > > > On Mon, 8 Nov 2021 16:48:33 -0800 > > Roman Bacik wrote: > > > > > To be honest changing status codes coming from FW does not seem right. But > > > we will try to make the requested changes. > > > > I looked at kernel's implementation of this driver and these hwrm > > functions and they don't return STATUS_*. > > > > Marek > > Marek, > > This is quite a different driver and it was written for uboot. Hello Roman The drivers clearly have a common ancestor, there are far too many similarities. It clearly wasn't written from scratch for U-Boot. > If the > main objection is that Linux driver is different then maybe we should > use v10 as is. Currently hwrm methods return HW status and bnxt > methods return uboot error codes consistently. I will leave this to U-Boot's network subsystem maintainers. As I said, beggars cannot be choosers in U-Boot. As long as the driver does not introduce vendor specific stuff to the user API (in U-Boot command line), then I guess I'll have to be satisfied. Marek
Re: [PATCH v10 1/2] net: brcm: netXtreme driver
On Mon, Nov 8, 2021 at 5:12 PM Marek Behún wrote: > > On Mon, 8 Nov 2021 16:48:33 -0800 > Roman Bacik wrote: > > > To be honest changing status codes coming from FW does not seem right. But > > we will try to make the requested changes. > > I looked at kernel's implementation of this driver and these hwrm > functions and they don't return STATUS_*. > > Marek Marek, This is quite a different driver and it was written for uboot. If the main objection is that Linux driver is different then maybe we should use v10 as is. Currently hwrm methods return HW status and bnxt methods return uboot error codes consistently. Thanks, Roman -- 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.
Re: [PATCH v10 1/2] net: brcm: netXtreme driver
On Mon, 8 Nov 2021 16:48:33 -0800 Roman Bacik wrote: > To be honest changing status codes coming from FW does not seem right. But > we will try to make the requested changes. I looked at kernel's implementation of this driver and these hwrm functions and they don't return STATUS_*. Marek
RE: [PATCH v10 1/2] net: brcm: netXtreme driver
Hi Marek,
> -Original Message-
> From: Marek Behún
> Sent: Monday, November 8, 2021 3:43 PM
> 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
>
> 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?
Ok, we will make the change.
>
> > +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.
This status is returned from our HW/FW so it would make more sense to keep
it as is.
But we will change it per your request and we will replace STATUS_SUCCESS
with 0 and STATUS_FAILURE with -1 and hence loose status codes actually
returned by HW/FW.
>
> 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...
The bp structure is passed to our HW/FW, which expects a valid integer
link_status. We cannot make this change and have the driver working.
>
>
> > +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 Physic
Re: [PATCH v10 1/2] net: brcm: netXtreme driver
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 thing
