| Message ID | 20210818190800.20191-4-biju.das.jz@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Gigabit Ethernet driver support | expand |
On 8/18/21 10:07 PM, Biju Das wrote: > R-Car Gen2 needs a 4byte aligned address for the transmission buffer, > whereas R-Car Gen3 doesn't have any such restriction. > > Add aligned_tx to struct ravb_hw_info to select the driver to choose > between aligned and unaligned tx buffers. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > v3: > * New patch Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] MBR, Sergey
(Resending from the proper email.) On 8/18/21 10:07 PM, Biju Das wrote: > R-Car Gen2 needs a 4byte aligned address for the transmission buffer, > whereas R-Car Gen3 doesn't have any such restriction. > > Add aligned_tx to struct ravb_hw_info to select the driver to choose > between aligned and unaligned tx buffers. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > v3: > * New patch Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] MBR, Sergey
Hi Biju, On Wed, Aug 18, 2021 at 9:08 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > R-Car Gen2 needs a 4byte aligned address for the transmission buffer, > whereas R-Car Gen3 doesn't have any such restriction. > > Add aligned_tx to struct ravb_hw_info to select the driver to choose > between aligned and unaligned tx buffers. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Thanks for your patch, which is now commit 68ca3c923213b908 ("ravb: Add aligned_tx to struct ravb_hw_info") in net-next. > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -990,6 +990,7 @@ enum ravb_chip_id { > > struct ravb_hw_info { > enum ravb_chip_id chip_id; > + unsigned aligned_tx: 1; > }; > > struct ravb_private { > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index b6554e5e13af..dbccf2cd89b2 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -1930,6 +1930,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = { > > static const struct ravb_hw_info ravb_gen2_hw_info = { > .chip_id = RCAR_GEN2, > + .aligned_tx = 1, > }; > > static const struct of_device_id ravb_match_table[] = { > @@ -2140,7 +2141,7 @@ static int ravb_probe(struct platform_device *pdev) > ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN); > ndev->min_mtu = ETH_MIN_MTU; > > - priv->num_tx_desc = info->chip_id == RCAR_GEN2 ? > + priv->num_tx_desc = info->aligned_tx ? > NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3; At first look, this change does not seem to match the patch description. Upon a deeper look, it is correct, as num_tx_desc is also used to control alignment. But now NUM_TX_DESC_GEN[23] no longer match their use. Perhaps they should be renamed, or replaced by hardcoded values, with a comment? /* * FIXME: Explain the relationship between alignment and number of buffers */ priv->num_tx_desc = info->aligned_tx ? 2 : 1; > > /* Set function */ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, Thanks for the feedback > Subject: Re: [PATCH net-next v3 3/9] ravb: Add aligned_tx to struct > ravb_hw_info > > Hi Biju, > > On Wed, Aug 18, 2021 at 9:08 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > R-Car Gen2 needs a 4byte aligned address for the transmission buffer, > > whereas R-Car Gen3 doesn't have any such restriction. > > > > Add aligned_tx to struct ravb_hw_info to select the driver to choose > > between aligned and unaligned tx buffers. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Thanks for your patch, which is now commit 68ca3c923213b908 ("ravb: > Add aligned_tx to struct ravb_hw_info") in net-next. > > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -990,6 +990,7 @@ enum ravb_chip_id { > > > > struct ravb_hw_info { > > enum ravb_chip_id chip_id; > > + unsigned aligned_tx: 1; > > }; > > > > struct ravb_private { > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index b6554e5e13af..dbccf2cd89b2 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -1930,6 +1930,7 @@ static const struct ravb_hw_info > > ravb_gen3_hw_info = { > > > > static const struct ravb_hw_info ravb_gen2_hw_info = { > > .chip_id = RCAR_GEN2, > > + .aligned_tx = 1, > > }; > > > > static const struct of_device_id ravb_match_table[] = { @@ -2140,7 > > +2141,7 @@ static int ravb_probe(struct platform_device *pdev) > > ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN); > > ndev->min_mtu = ETH_MIN_MTU; > > > > - priv->num_tx_desc = info->chip_id == RCAR_GEN2 ? > > + priv->num_tx_desc = info->aligned_tx ? > > NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3; > > At first look, this change does not seem to match the patch description. > Upon a deeper look, it is correct, as num_tx_desc is also used to control > alignment. This changes introduced by the commit [1], where it used num_tx_desc related to 4byte alignment restriction. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/ethernet/renesas/ravb_main.c?h=v5.14-rc7&id=f543305da9b5a5efaf6fa61606e8bbc8977f406d > > But now NUM_TX_DESC_GEN[23] no longer match their use. > Perhaps they should be renamed, or replaced by hardcoded values, with a > comment? OK, will replace this macros with hardcoded values with a comment. Regards, Biju > > /* > * FIXME: Explain the relationship between alignment and number of > buffers > */ > priv->num_tx_desc = info->aligned_tx ? 2 : 1; > > > > > /* Set function */ > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > In personal conversations with technical people, I call myself a hacker. > But when I'm talking to journalists I just say "programmer" or something > like that. > -- Linus Torvalds
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index 6ff0b2626708..4f71e5699ca1 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -990,6 +990,7 @@ enum ravb_chip_id { struct ravb_hw_info { enum ravb_chip_id chip_id; + unsigned aligned_tx: 1; }; struct ravb_private { diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index b6554e5e13af..dbccf2cd89b2 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1930,6 +1930,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = { static const struct ravb_hw_info ravb_gen2_hw_info = { .chip_id = RCAR_GEN2, + .aligned_tx = 1, }; static const struct of_device_id ravb_match_table[] = { @@ -2140,7 +2141,7 @@ static int ravb_probe(struct platform_device *pdev) ndev->max_mtu = 2048 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN); ndev->min_mtu = ETH_MIN_MTU; - priv->num_tx_desc = info->chip_id == RCAR_GEN2 ? + priv->num_tx_desc = info->aligned_tx ? NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3; /* Set function */