diff mbox series

[net-next,v2,1/8] ravb: Add struct ravb_hw_info to driver data

Message ID 20210802102654.5996-2-biju.das.jz@bp.renesas.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Add Gigabit Ethernet driver support | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 3 maintainers not CCed: yangyingliang@huawei.com rikard.falkeborn@gmail.com f.fainelli@gmail.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Biju Das Aug. 2, 2021, 10:26 a.m. UTC
The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC are
similar to the R-Car Ethernet AVB IP. With a few changes in the driver we
can support both IPs.

Currently a runtime decision based on the chip type is used to distinguish
the HW differences between the SoC families.

The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2 and
RZ/G2L it is 2. For cases like this it is better to select the number of
TX descriptors by using a structure with a value, rather than a runtime
decision based on the chip type.

This patch adds the num_tx_desc variable to struct ravb_hw_info and also
replaces the driver data chip type with struct ravb_hw_info by moving chip
type to it.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
v2:
 * Incorporated Andrew and Sergei's review comments for making it smaller patch
   and provided detailed description.
---
 drivers/net/ethernet/renesas/ravb.h      |  7 +++++
 drivers/net/ethernet/renesas/ravb_main.c | 38 +++++++++++++++---------
 2 files changed, 31 insertions(+), 14 deletions(-)

Comments

Andrew Lunn Aug. 2, 2021, 3:02 p.m. UTC | #1
On Mon, Aug 02, 2021 at 11:26:47AM +0100, Biju Das wrote:
> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC are
> similar to the R-Car Ethernet AVB IP. With a few changes in the driver we
> can support both IPs.
> 
> Currently a runtime decision based on the chip type is used to distinguish
> the HW differences between the SoC families.
> 
> The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2 and
> RZ/G2L it is 2. For cases like this it is better to select the number of
> TX descriptors by using a structure with a value, rather than a runtime
> decision based on the chip type.
> 
> This patch adds the num_tx_desc variable to struct ravb_hw_info and also
> replaces the driver data chip type with struct ravb_hw_info by moving chip
> type to it.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi Biju

This is better. A lot clearer what is going on. I personally would of
done the num_tx_desc change as a separate patch, but this is O.K.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Sergey Shtylyov Aug. 2, 2021, 8:42 p.m. UTC | #2
On 8/2/21 1:26 PM, Biju Das wrote:

> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC are
> similar to the R-Car Ethernet AVB IP. With a few changes in the driver we
> can support both IPs.
> 
> Currently a runtime decision based on the chip type is used to distinguish
> the HW differences between the SoC families.
> 
> The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2 and
> RZ/G2L it is 2. For cases like this it is better to select the number of
> TX descriptors by using a structure with a value, rather than a runtime
> decision based on the chip type.
> 
> This patch adds the num_tx_desc variable to struct ravb_hw_info and also
> replaces the driver data chip type with struct ravb_hw_info by moving chip
> type to it.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2:
>  * Incorporated Andrew and Sergei's review comments for making it smaller patch
>    and provided detailed description.
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  7 +++++
>  drivers/net/ethernet/renesas/ravb_main.c | 38 +++++++++++++++---------
>  2 files changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 80e62ca2e3d3..cfb972c05b34 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -988,6 +988,11 @@ enum ravb_chip_id {
>  	RCAR_GEN3,
>  };
>  
> +struct ravb_hw_info {
> +	enum ravb_chip_id chip_id;
> +	int num_tx_desc;

   I think this is rather the driver's choice, than the h/w feature... Perhaps a rename
would help with that? :-)

[...]

MBR, Sergei
Biju Das Aug. 3, 2021, 5:57 a.m. UTC | #3
Hi Sergei,

Thanks for the feedback.

> Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to
> driver data
> 
> On 8/2/21 1:26 PM, Biju Das wrote:
> 
> > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC
> > are similar to the R-Car Ethernet AVB IP. With a few changes in the
> > driver we can support both IPs.
> >
> > Currently a runtime decision based on the chip type is used to
> > distinguish the HW differences between the SoC families.
> >
> > The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2
> > and RZ/G2L it is 2. For cases like this it is better to select the
> > number of TX descriptors by using a structure with a value, rather
> > than a runtime decision based on the chip type.
> >
> > This patch adds the num_tx_desc variable to struct ravb_hw_info and
> > also replaces the driver data chip type with struct ravb_hw_info by
> > moving chip type to it.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > v2:
> >  * Incorporated Andrew and Sergei's review comments for making it
> smaller patch
> >    and provided detailed description.
> > ---
> >  drivers/net/ethernet/renesas/ravb.h      |  7 +++++
> >  drivers/net/ethernet/renesas/ravb_main.c | 38
> > +++++++++++++++---------
> >  2 files changed, 31 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > b/drivers/net/ethernet/renesas/ravb.h
> > index 80e62ca2e3d3..cfb972c05b34 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -988,6 +988,11 @@ enum ravb_chip_id {
> >  	RCAR_GEN3,
> >  };
> >
> > +struct ravb_hw_info {
> > +	enum ravb_chip_id chip_id;
> > +	int num_tx_desc;
> 
>    I think this is rather the driver's choice, than the h/w feature...
> Perhaps a rename would help with that? :-)

It is consistent with current naming convention used by the driver. NUM_TX_DESC macro is replaced by num_tx_desc and  the below run time decision based on chip type for H/W configuration for Gen2/Gen3 is replaced by info->num_tx_desc.

priv->num_tx_desc = chip_id == RCAR_GEN2 ? NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3;

Please let me know, if I am missing anything,

Previously there is a suggestion to change the generic struct ravb_driver_data(which holds driver differences and HW features) with struct ravb_hw_info. 

Regards,
Biju

> 
> [...]
> 
> MBR, Sergei
Biju Das Aug. 3, 2021, 6:36 a.m. UTC | #4
Hi Sergei,

> Subject: RE: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to
> driver data
> 
> Hi Sergei,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to
> > driver data
> >
> > On 8/2/21 1:26 PM, Biju Das wrote:
> >
> > > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC
> > > are similar to the R-Car Ethernet AVB IP. With a few changes in the
> > > driver we can support both IPs.
> > >
> > > Currently a runtime decision based on the chip type is used to
> > > distinguish the HW differences between the SoC families.
> > >
> > > The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car
> > > Gen2 and RZ/G2L it is 2. For cases like this it is better to select
> > > the number of TX descriptors by using a structure with a value,
> > > rather than a runtime decision based on the chip type.
> > >
> > > This patch adds the num_tx_desc variable to struct ravb_hw_info and
> > > also replaces the driver data chip type with struct ravb_hw_info by
> > > moving chip type to it.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > v2:
> > >  * Incorporated Andrew and Sergei's review comments for making it
> > smaller patch
> > >    and provided detailed description.
> > > ---
> > >  drivers/net/ethernet/renesas/ravb.h      |  7 +++++
> > >  drivers/net/ethernet/renesas/ravb_main.c | 38
> > > +++++++++++++++---------
> > >  2 files changed, 31 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/renesas/ravb.h
> > > b/drivers/net/ethernet/renesas/ravb.h
> > > index 80e62ca2e3d3..cfb972c05b34 100644
> > > --- a/drivers/net/ethernet/renesas/ravb.h
> > > +++ b/drivers/net/ethernet/renesas/ravb.h
> > > @@ -988,6 +988,11 @@ enum ravb_chip_id {
> > >  	RCAR_GEN3,
> > >  };
> > >
> > > +struct ravb_hw_info {
> > > +	enum ravb_chip_id chip_id;
> > > +	int num_tx_desc;
> >
> >    I think this is rather the driver's choice, than the h/w feature...
> > Perhaps a rename would help with that? :-)
> 
> It is consistent with current naming convention used by the driver.
> NUM_TX_DESC macro is replaced by num_tx_desc.
      
So the name should be ok. 

Indeed we are agreed to add function pointers to struct ravb_hw_info to avoid another level of indirection.

If the concern is related to duplication of data(ie,priv->num_tx_desc vs info->num_tx_desc)
I have a plan to remove priv->num_tx_desc with info->num_tx_desc  later.

Regards,
Biju
Sergey Shtylyov Aug. 4, 2021, 7:27 p.m. UTC | #5
On 8/3/21 8:57 AM, Biju Das wrote:

>> Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to
>> driver data
>>
>> On 8/2/21 1:26 PM, Biju Das wrote:
>>
>>> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC
>>> are similar to the R-Car Ethernet AVB IP. With a few changes in the
>>> driver we can support both IPs.
>>>
>>> Currently a runtime decision based on the chip type is used to
>>> distinguish the HW differences between the SoC families.
>>>
>>> The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2
>>> and RZ/G2L it is 2. For cases like this it is better to select the
>>> number of TX descriptors by using a structure with a value, rather
>>> than a runtime decision based on the chip type.
>>>
>>> This patch adds the num_tx_desc variable to struct ravb_hw_info and
>>> also replaces the driver data chip type with struct ravb_hw_info by
>>> moving chip type to it.
>>>
>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>> ---
>>> v2:
>>>  * Incorporated Andrew and Sergei's review comments for making it
>> smaller patch
>>>    and provided detailed description.
>>> ---
>>>  drivers/net/ethernet/renesas/ravb.h      |  7 +++++
>>>  drivers/net/ethernet/renesas/ravb_main.c | 38
>>> +++++++++++++++---------
>>>  2 files changed, 31 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb.h
>>> b/drivers/net/ethernet/renesas/ravb.h
>>> index 80e62ca2e3d3..cfb972c05b34 100644
>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>> @@ -988,6 +988,11 @@ enum ravb_chip_id {
>>>  	RCAR_GEN3,
>>>  };
>>>
>>> +struct ravb_hw_info {
>>> +	enum ravb_chip_id chip_id;
>>> +	int num_tx_desc;

   How about leaving that field in the *struct* ravb_private? And adding the following instead:

	unsigned unaligned_tx: 1;

>>    I think this is rather the driver's choice, than the h/w feature...
>> Perhaps a rename would help with that? :-)
> 
> It is consistent with current naming convention used by the driver. NUM_TX_DESC macro is replaced by num_tx_desc and  the below run time decision based on chip type for H/W configuration for Gen2/Gen3 is replaced by info->num_tx_desc.
> 
> priv->num_tx_desc = chip_id == RCAR_GEN2 ? NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3;

   .. and then:

	priv->num_tx_desc =  info->unaligned_tx ? NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3;

> Please let me know, if I am missing anything,
> 
> Previously there is a suggestion to change the generic struct ravb_driver_data (which holds driver differences and HW features) with struct ravb_hw_info.

    Well, my plan was to place all the hardware features supported into the *struct* ravb_hw_info and leave all
the driver's software data in the *struct* ravb_private.

> Regards,
> Biju
[...]

MBR, Sergei
Sergey Shtylyov Aug. 4, 2021, 8:27 p.m. UTC | #6
On 8/4/21 10:27 PM, Sergei Shtylyov wrote:

>>> Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to
>>> driver data
>>>
>>> On 8/2/21 1:26 PM, Biju Das wrote:
>>>
>>>> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC
>>>> are similar to the R-Car Ethernet AVB IP. With a few changes in the
>>>> driver we can support both IPs.
>>>>
>>>> Currently a runtime decision based on the chip type is used to
>>>> distinguish the HW differences between the SoC families.
>>>>
>>>> The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2
>>>> and RZ/G2L it is 2. For cases like this it is better to select the
>>>> number of TX descriptors by using a structure with a value, rather
>>>> than a runtime decision based on the chip type.
>>>>
>>>> This patch adds the num_tx_desc variable to struct ravb_hw_info and
>>>> also replaces the driver data chip type with struct ravb_hw_info by
>>>> moving chip type to it.
>>>>
>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>>>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>>> ---
>>>> v2:
>>>>  * Incorporated Andrew and Sergei's review comments for making it
>>> smaller patch
>>>>    and provided detailed description.
>>>> ---
>>>>  drivers/net/ethernet/renesas/ravb.h      |  7 +++++
>>>>  drivers/net/ethernet/renesas/ravb_main.c | 38
>>>> +++++++++++++++---------
>>>>  2 files changed, 31 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/renesas/ravb.h
>>>> b/drivers/net/ethernet/renesas/ravb.h
>>>> index 80e62ca2e3d3..cfb972c05b34 100644
>>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>>> @@ -988,6 +988,11 @@ enum ravb_chip_id {
>>>>  	RCAR_GEN3,
>>>>  };
>>>>
>>>> +struct ravb_hw_info {
>>>> +	enum ravb_chip_id chip_id;
>>>> +	int num_tx_desc;
> 
>    How about leaving that field in the *struct* ravb_private? And adding the following instead:
> 
> 	unsigned unaligned_tx: 1;

   Or aligned_tx, so that gen2 has it set, and gen3 has it cleared.

> 
>>>    I think this is rather the driver's choice, than the h/w feature...
>>> Perhaps a rename would help with that? :-)
>>
>> It is consistent with current naming convention used by the driver. NUM_TX_DESC macro is replaced by num_tx_desc and  the below run time decision based on chip type for H/W configuration for Gen2/Gen3 is replaced by info->num_tx_desc.
>>
>> priv->num_tx_desc = chip_id == RCAR_GEN2 ? NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3;
> 
>    .. and then:
> 
> 	priv->num_tx_desc =  info->unaligned_tx ? NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3;

   Sorry, mixed the values, should have been:

	priv->num_tx_desc =  info->unaligned_tx ? NUM_TX_DESC_GEN3 : NUM_TX_DESC_GEN2;

>> Please let me know, if I am missing anything,
>>
>> Previously there is a suggestion to change the generic struct ravb_driver_data (which holds driver differences and HW features) with struct ravb_hw_info.
> 
>     Well, my plan was to place all the hardware features supported into the *struct* ravb_hw_info and leave all
> the driver's software data in the *struct* ravb_private.

   ... just like *struct* sh_eth_cpu_data and sh_eth_private in the sh_eth driver.

>> Regards,
>> Biju

MBR, Sergei
Geert Uytterhoeven Aug. 9, 2021, 12:06 p.m. UTC | #7
Hi Biju,

On Mon, Aug 2, 2021 at 12:27 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC are
> similar to the R-Car Ethernet AVB IP. With a few changes in the driver we
> can support both IPs.
>
> Currently a runtime decision based on the chip type is used to distinguish
> the HW differences between the SoC families.
>
> The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2 and
> RZ/G2L it is 2. For cases like this it is better to select the number of
> TX descriptors by using a structure with a value, rather than a runtime
> decision based on the chip type.
>
> This patch adds the num_tx_desc variable to struct ravb_hw_info and also
> replaces the driver data chip type with struct ravb_hw_info by moving chip
> type to it.
>
> 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!

> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -988,6 +988,11 @@ enum ravb_chip_id {
>         RCAR_GEN3,
>  };
>
> +struct ravb_hw_info {
> +       enum ravb_chip_id chip_id;
> +       int num_tx_desc;

Why not "unsigned int"? ...
This comment applies to a few more subsequent patches.

> +};
> +
>  struct ravb_private {
>         struct net_device *ndev;
>         struct platform_device *pdev;
> @@ -1040,6 +1045,8 @@ struct ravb_private {
>         unsigned txcidm:1;              /* TX Clock Internal Delay Mode */
>         unsigned rgmii_override:1;      /* Deprecated rgmii-*id behavior */
>         int num_tx_desc;                /* TX descriptors per packet */

... oh, here's the original culprit.

> +
> +       const struct ravb_hw_info *info;
>  };
>

Gr{oetje,eeting}s,

                        Geert
Biju Das Aug. 12, 2021, 7:26 a.m. UTC | #8
Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to
> driver data
> 
> Hi Biju,
> 
> On Mon, Aug 2, 2021 at 12:27 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC
> > are similar to the R-Car Ethernet AVB IP. With a few changes in the
> > driver we can support both IPs.
> >
> > Currently a runtime decision based on the chip type is used to
> > distinguish the HW differences between the SoC families.
> >
> > The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2
> > and RZ/G2L it is 2. For cases like this it is better to select the
> > number of TX descriptors by using a structure with a value, rather
> > than a runtime decision based on the chip type.
> >
> > This patch adds the num_tx_desc variable to struct ravb_hw_info and
> > also replaces the driver data chip type with struct ravb_hw_info by
> > moving chip type to it.
> >
> > 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!
> 
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -988,6 +988,11 @@ enum ravb_chip_id {
> >         RCAR_GEN3,
> >  };
> >
> > +struct ravb_hw_info {
> > +       enum ravb_chip_id chip_id;
> > +       int num_tx_desc;
> 
> Why not "unsigned int"? ...
> This comment applies to a few more subsequent patches.

To avoid signed and unsigned comparison warnings.

> 
> > +};
> > +
> >  struct ravb_private {
> >         struct net_device *ndev;
> >         struct platform_device *pdev;
> > @@ -1040,6 +1045,8 @@ struct ravb_private {
> >         unsigned txcidm:1;              /* TX Clock Internal Delay Mode
> */
> >         unsigned rgmii_override:1;      /* Deprecated rgmii-*id behavior
> */
> >         int num_tx_desc;                /* TX descriptors per packet */
> 
> ... oh, here's the original culprit.

Exactly, this the reason. 

Do you want me to change this into unsigned int? Please let me know.

Regards,
Biju

> 
> > +
> > +       const struct ravb_hw_info *info;
> >  };
> >
> 
> 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
Geert Uytterhoeven Aug. 12, 2021, 7:58 a.m. UTC | #9
Hi Biju,

On Thu, Aug 12, 2021 at 9:26 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > -----Original Message-----
> > On Mon, Aug 2, 2021 at 12:27 PM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L SoC
> > > are similar to the R-Car Ethernet AVB IP. With a few changes in the
> > > driver we can support both IPs.
> > >
> > > Currently a runtime decision based on the chip type is used to
> > > distinguish the HW differences between the SoC families.
> > >
> > > The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car Gen2
> > > and RZ/G2L it is 2. For cases like this it is better to select the
> > > number of TX descriptors by using a structure with a value, rather
> > > than a runtime decision based on the chip type.
> > >
> > > This patch adds the num_tx_desc variable to struct ravb_hw_info and
> > > also replaces the driver data chip type with struct ravb_hw_info by
> > > moving chip type to it.
> > >
> > > 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!
> >
> > > --- a/drivers/net/ethernet/renesas/ravb.h
> > > +++ b/drivers/net/ethernet/renesas/ravb.h
> > > @@ -988,6 +988,11 @@ enum ravb_chip_id {
> > >         RCAR_GEN3,
> > >  };
> > >
> > > +struct ravb_hw_info {
> > > +       enum ravb_chip_id chip_id;
> > > +       int num_tx_desc;
> >
> > Why not "unsigned int"? ...
> > This comment applies to a few more subsequent patches.
>
> To avoid signed and unsigned comparison warnings.
>
> >
> > > +};
> > > +
> > >  struct ravb_private {
> > >         struct net_device *ndev;
> > >         struct platform_device *pdev;
> > > @@ -1040,6 +1045,8 @@ struct ravb_private {
> > >         unsigned txcidm:1;              /* TX Clock Internal Delay Mode
> > */
> > >         unsigned rgmii_override:1;      /* Deprecated rgmii-*id behavior
> > */
> > >         int num_tx_desc;                /* TX descriptors per packet */
> >
> > ... oh, here's the original culprit.
>
> Exactly, this the reason.
>
> Do you want me to change this into unsigned int? Please let me know.

Up to you (or the maintainer? ;-)

For new fields (in the other patches), I would use unsigned for all
unsigned values.  Signed values have more pitfalls related to
undefined behavior.

Gr{oetje,eeting}s,

                        Geert
Biju Das Aug. 12, 2021, 8:13 a.m. UTC | #10
Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to
> driver data
> 
> Hi Biju,
> 
> On Thu, Aug 12, 2021 at 9:26 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > -----Original Message-----
> > > On Mon, Aug 2, 2021 at 12:27 PM Biju Das
> > > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L
> > > > SoC are similar to the R-Car Ethernet AVB IP. With a few changes
> > > > in the driver we can support both IPs.
> > > >
> > > > Currently a runtime decision based on the chip type is used to
> > > > distinguish the HW differences between the SoC families.
> > > >
> > > > The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car
> > > > Gen2 and RZ/G2L it is 2. For cases like this it is better to
> > > > select the number of TX descriptors by using a structure with a
> > > > value, rather than a runtime decision based on the chip type.
> > > >
> > > > This patch adds the num_tx_desc variable to struct ravb_hw_info
> > > > and also replaces the driver data chip type with struct
> > > > ravb_hw_info by moving chip type to it.
> > > >
> > > > 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!
> > >
> > > > --- a/drivers/net/ethernet/renesas/ravb.h
> > > > +++ b/drivers/net/ethernet/renesas/ravb.h
> > > > @@ -988,6 +988,11 @@ enum ravb_chip_id {
> > > >         RCAR_GEN3,
> > > >  };
> > > >
> > > > +struct ravb_hw_info {
> > > > +       enum ravb_chip_id chip_id;
> > > > +       int num_tx_desc;
> > >
> > > Why not "unsigned int"? ...
> > > This comment applies to a few more subsequent patches.
> >
> > To avoid signed and unsigned comparison warnings.
> >
> > >
> > > > +};
> > > > +
> > > >  struct ravb_private {
> > > >         struct net_device *ndev;
> > > >         struct platform_device *pdev; @@ -1040,6 +1045,8 @@ struct
> > > > ravb_private {
> > > >         unsigned txcidm:1;              /* TX Clock Internal Delay
> Mode
> > > */
> > > >         unsigned rgmii_override:1;      /* Deprecated rgmii-*id
> behavior
> > > */
> > > >         int num_tx_desc;                /* TX descriptors per packet
> */
> > >
> > > ... oh, here's the original culprit.
> >
> > Exactly, this the reason.
> >
> > Do you want me to change this into unsigned int? Please let me know.
> 
> Up to you (or the maintainer? ;-)
> 
> For new fields (in the other patches), I would use unsigned for all
> unsigned values.  Signed values have more pitfalls related to undefined
> behavior.

Sergei, What is your thoughts here? Please let me know.

Cheers,
Biju
Biju Das Aug. 17, 2021, 11:24 a.m. UTC | #11
Hi all,

> Subject: RE: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to
> driver data
> 
> Hi Geert,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to
> > driver data
> >
> > Hi Biju,
> >
> > On Thu, Aug 12, 2021 at 9:26 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > -----Original Message-----
> > > > On Mon, Aug 2, 2021 at 12:27 PM Biju Das
> > > > <biju.das.jz@bp.renesas.com>
> > > > wrote:
> > > > > The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L
> > > > > SoC are similar to the R-Car Ethernet AVB IP. With a few changes
> > > > > in the driver we can support both IPs.
> > > > >
> > > > > Currently a runtime decision based on the chip type is used to
> > > > > distinguish the HW differences between the SoC families.
> > > > >
> > > > > The number of TX descriptors for R-Car Gen3 is 1 whereas on
> > > > > R-Car
> > > > > Gen2 and RZ/G2L it is 2. For cases like this it is better to
> > > > > select the number of TX descriptors by using a structure with a
> > > > > value, rather than a runtime decision based on the chip type.
> > > > >
> > > > > This patch adds the num_tx_desc variable to struct ravb_hw_info
> > > > > and also replaces the driver data chip type with struct
> > > > > ravb_hw_info by moving chip type to it.
> > > > >
> > > > > 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!
> > > >
> > > > > --- a/drivers/net/ethernet/renesas/ravb.h
> > > > > +++ b/drivers/net/ethernet/renesas/ravb.h
> > > > > @@ -988,6 +988,11 @@ enum ravb_chip_id {
> > > > >         RCAR_GEN3,
> > > > >  };
> > > > >
> > > > > +struct ravb_hw_info {
> > > > > +       enum ravb_chip_id chip_id;
> > > > > +       int num_tx_desc;
> > > >
> > > > Why not "unsigned int"? ...
> > > > This comment applies to a few more subsequent patches.
> > >
> > > To avoid signed and unsigned comparison warnings.
> > >
> > > >
> > > > > +};
> > > > > +
> > > > >  struct ravb_private {
> > > > >         struct net_device *ndev;
> > > > >         struct platform_device *pdev; @@ -1040,6 +1045,8 @@
> > > > > struct ravb_private {
> > > > >         unsigned txcidm:1;              /* TX Clock Internal Delay
> > Mode
> > > > */
> > > > >         unsigned rgmii_override:1;      /* Deprecated rgmii-*id
> > behavior
> > > > */
> > > > >         int num_tx_desc;                /* TX descriptors per
> packet
> > */
> > > >
> > > > ... oh, here's the original culprit.
> > >
> > > Exactly, this the reason.
> > >
> > > Do you want me to change this into unsigned int? Please let me know.
> >
> > Up to you (or the maintainer? ;-)
> >
> > For new fields (in the other patches), I would use unsigned for all
> > unsigned values.  Signed values have more pitfalls related to
> > undefined behavior.
> 
> Sergei, What is your thoughts here? Please let me know.

Here is my plan.

I will split this patch into two as Andrew suggested and 

Then on the second patch will add as info->unaligned_tx as Sergei suggested.

Now the only open point is related to the data type of "int num_tx_desc"
and to align with sh_eth driver I will keep int.

Regards,
Biju
Sergey Shtylyov Aug. 17, 2021, 8:11 p.m. UTC | #12
On 8/17/21 2:24 PM, Biju Das wrote:

[...]
>>>>> -----Original Message-----
>>>>> On Mon, Aug 2, 2021 at 12:27 PM Biju Das
>>>>> <biju.das.jz@bp.renesas.com>
>>>>> wrote:
>>>>>> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L
>>>>>> SoC are similar to the R-Car Ethernet AVB IP. With a few changes
>>>>>> in the driver we can support both IPs.
>>>>>>
>>>>>> Currently a runtime decision based on the chip type is used to
>>>>>> distinguish the HW differences between the SoC families.
>>>>>>
>>>>>> The number of TX descriptors for R-Car Gen3 is 1 whereas on
>>>>>> R-Car
>>>>>> Gen2 and RZ/G2L it is 2. For cases like this it is better to
>>>>>> select the number of TX descriptors by using a structure with a
>>>>>> value, rather than a runtime decision based on the chip type.
>>>>>>
>>>>>> This patch adds the num_tx_desc variable to struct ravb_hw_info
>>>>>> and also replaces the driver data chip type with struct
>>>>>> ravb_hw_info by moving chip type to it.
>>>>>>
>>>>>> 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!
>>>>>
>>>>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>>>>> @@ -988,6 +988,11 @@ enum ravb_chip_id {
>>>>>>         RCAR_GEN3,
>>>>>>  };
>>>>>>
>>>>>> +struct ravb_hw_info {
>>>>>> +       enum ravb_chip_id chip_id;
>>>>>> +       int num_tx_desc;
>>>>>
>>>>> Why not "unsigned int"? ...
>>>>> This comment applies to a few more subsequent patches.
>>>>
>>>> To avoid signed and unsigned comparison warnings.
>>>>
>>>>>
>>>>>> +};
>>>>>> +
>>>>>>  struct ravb_private {
>>>>>>         struct net_device *ndev;
>>>>>>         struct platform_device *pdev; @@ -1040,6 +1045,8 @@
>>>>>> struct ravb_private {
>>>>>>         unsigned txcidm:1;              /* TX Clock Internal Delay
>>> Mode
>>>>> */
>>>>>>         unsigned rgmii_override:1;      /* Deprecated rgmii-*id
>>> behavior
>>>>> */
>>>>>>         int num_tx_desc;                /* TX descriptors per
>> packet
>>> */
>>>>>
>>>>> ... oh, here's the original culprit.
>>>>
>>>> Exactly, this the reason.
>>>>
>>>> Do you want me to change this into unsigned int? Please let me know.
>>>
>>> Up to you (or the maintainer? ;-)
>>>
>>> For new fields (in the other patches), I would use unsigned for all
>>> unsigned values.  Signed values have more pitfalls related to
>>> undefined behavior.
>>
>> Sergei, What is your thoughts here? Please let me know.
> 
> Here is my plan.
> 
> I will split this patch into two as Andrew suggested and 

   If you mran changing the ravb_private::num_tx_desc to *unsigned*, it'll be
a good cleanup. What's would be the 2nd part tho?

> Then on the second patch will add as info->unaligned_tx as Sergei suggested.

   OK.

> Now the only open point is related to the data type of "int num_tx_desc"
> and to align with sh_eth driver I will keep int.

   The sh_eth driver simply doesn't have this -- it always use 1 descriptor.

> Regards,
> Biju

MBR, Sergey
Biju Das Aug. 18, 2021, 6:29 a.m. UTC | #13
Hi Sergei,

Thanks for the feedback.

> Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to
> driver data
> 
> On 8/17/21 2:24 PM, Biju Das wrote:
> 
> [...]
> >>>>> -----Original Message-----
> >>>>> On Mon, Aug 2, 2021 at 12:27 PM Biju Das
> >>>>> <biju.das.jz@bp.renesas.com>
> >>>>> wrote:
> >>>>>> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L
> >>>>>> SoC are similar to the R-Car Ethernet AVB IP. With a few changes
> >>>>>> in the driver we can support both IPs.
> >>>>>>
> >>>>>> Currently a runtime decision based on the chip type is used to
> >>>>>> distinguish the HW differences between the SoC families.
> >>>>>>
> >>>>>> The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car
> >>>>>> Gen2 and RZ/G2L it is 2. For cases like this it is better to
> >>>>>> select the number of TX descriptors by using a structure with a
> >>>>>> value, rather than a runtime decision based on the chip type.
> >>>>>>
> >>>>>> This patch adds the num_tx_desc variable to struct ravb_hw_info
> >>>>>> and also replaces the driver data chip type with struct
> >>>>>> ravb_hw_info by moving chip type to it.
> >>>>>>
> >>>>>> 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!
> >>>>>
> >>>>>> --- a/drivers/net/ethernet/renesas/ravb.h
> >>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h
> >>>>>> @@ -988,6 +988,11 @@ enum ravb_chip_id {
> >>>>>>         RCAR_GEN3,
> >>>>>>  };
> >>>>>>
> >>>>>> +struct ravb_hw_info {
> >>>>>> +       enum ravb_chip_id chip_id;
> >>>>>> +       int num_tx_desc;
> >>>>>
> >>>>> Why not "unsigned int"? ...
> >>>>> This comment applies to a few more subsequent patches.
> >>>>
> >>>> To avoid signed and unsigned comparison warnings.
> >>>>
> >>>>>
> >>>>>> +};
> >>>>>> +
> >>>>>>  struct ravb_private {
> >>>>>>         struct net_device *ndev;
> >>>>>>         struct platform_device *pdev; @@ -1040,6 +1045,8 @@
> >>>>>> struct ravb_private {
> >>>>>>         unsigned txcidm:1;              /* TX Clock Internal Delay
> >>> Mode
> >>>>> */
> >>>>>>         unsigned rgmii_override:1;      /* Deprecated rgmii-*id
> >>> behavior
> >>>>> */
> >>>>>>         int num_tx_desc;                /* TX descriptors per
> >> packet
> >>> */
> >>>>>
> >>>>> ... oh, here's the original culprit.
> >>>>
> >>>> Exactly, this the reason.
> >>>>
> >>>> Do you want me to change this into unsigned int? Please let me know.
> >>>
> >>> Up to you (or the maintainer? ;-)
> >>>
> >>> For new fields (in the other patches), I would use unsigned for all
> >>> unsigned values.  Signed values have more pitfalls related to
> >>> undefined behavior.
> >>
> >> Sergei, What is your thoughts here? Please let me know.
> >
> > Here is my plan.
> >
> > I will split this patch into two as Andrew suggested and
> 
>    If you mran changing the ravb_private::num_tx_desc to *unsigned*, it'll
> be a good cleanup. What's would be the 2nd part tho?

OK in that case, I will split this patch into 3.

First patch for adding struct ravb_hw_info to driver data and replace 
driver data chip type with struct ravb_hw_info

Second patch for changing ravb_private::num_tx_desc from int to unsigned int.

Third patch for adding aligned_tx to struct ravb_hw_info.

Regards,
Biju
Sergey Shtylyov Aug. 18, 2021, 10:11 a.m. UTC | #14
Hello!

On 18.08.2021 9:29, Biju Das wrote:

[...]
>>>>>>> -----Original Message-----
>>>>>>> On Mon, Aug 2, 2021 at 12:27 PM Biju Das
>>>>>>> <biju.das.jz@bp.renesas.com>
>>>>>>> wrote:
>>>>>>>> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L
>>>>>>>> SoC are similar to the R-Car Ethernet AVB IP. With a few changes
>>>>>>>> in the driver we can support both IPs.
>>>>>>>>
>>>>>>>> Currently a runtime decision based on the chip type is used to
>>>>>>>> distinguish the HW differences between the SoC families.
>>>>>>>>
>>>>>>>> The number of TX descriptors for R-Car Gen3 is 1 whereas on R-Car
>>>>>>>> Gen2 and RZ/G2L it is 2. For cases like this it is better to
>>>>>>>> select the number of TX descriptors by using a structure with a
>>>>>>>> value, rather than a runtime decision based on the chip type.
>>>>>>>>
>>>>>>>> This patch adds the num_tx_desc variable to struct ravb_hw_info
>>>>>>>> and also replaces the driver data chip type with struct
>>>>>>>> ravb_hw_info by moving chip type to it.
>>>>>>>>
>>>>>>>> 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!
>>>>>>>
>>>>>>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>>>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>>>>>>> @@ -988,6 +988,11 @@ enum ravb_chip_id {
>>>>>>>>          RCAR_GEN3,
>>>>>>>>   };
>>>>>>>>
>>>>>>>> +struct ravb_hw_info {
>>>>>>>> +       enum ravb_chip_id chip_id;
>>>>>>>> +       int num_tx_desc;
>>>>>>>
>>>>>>> Why not "unsigned int"? ...
>>>>>>> This comment applies to a few more subsequent patches.
>>>>>>
>>>>>> To avoid signed and unsigned comparison warnings.
>>>>>>
>>>>>>>
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>   struct ravb_private {
>>>>>>>>          struct net_device *ndev;
>>>>>>>>          struct platform_device *pdev; @@ -1040,6 +1045,8 @@
>>>>>>>> struct ravb_private {
>>>>>>>>          unsigned txcidm:1;              /* TX Clock Internal Delay
>>>>> Mode
>>>>>>> */
>>>>>>>>          unsigned rgmii_override:1;      /* Deprecated rgmii-*id
>>>>> behavior
>>>>>>> */
>>>>>>>>          int num_tx_desc;                /* TX descriptors per
>>>> packet
>>>>> */
>>>>>>>
>>>>>>> ... oh, here's the original culprit.
>>>>>>
>>>>>> Exactly, this the reason.
>>>>>>
>>>>>> Do you want me to change this into unsigned int? Please let me know.
>>>>>
>>>>> Up to you (or the maintainer? ;-)
>>>>>
>>>>> For new fields (in the other patches), I would use unsigned for all
>>>>> unsigned values.  Signed values have more pitfalls related to
>>>>> undefined behavior.
>>>>
>>>> Sergei, What is your thoughts here? Please let me know.
>>>
>>> Here is my plan.
>>>
>>> I will split this patch into two as Andrew suggested and
>>
>>     If you mran changing the ravb_private::num_tx_desc to *unsigned*, it'll
>> be a good cleanup. What's would be the 2nd part tho?
> 
> OK in that case, I will split this patch into 3.
> 
> First patch for adding struct ravb_hw_info to driver data and replace
> driver data chip type with struct ravb_hw_info

    Couldn't this be a 2nd patch?..

> Second patch for changing ravb_private::num_tx_desc from int to unsigned int.

    ... and this one the 1st?

> Third patch for adding aligned_tx to struct ravb_hw_info.
> 
> Regards,
> Biju

MBR, Sergey
Biju Das Aug. 18, 2021, 10:23 a.m. UTC | #15
Hi Sergei,

> -----Original Message-----
> Subject: Re: [PATCH net-next v2 1/8] ravb: Add struct ravb_hw_info to
> driver data
> 
> Hello!
> 
> On 18.08.2021 9:29, Biju Das wrote:
> 
> [...]
> >>>>>>> -----Original Message-----
> >>>>>>> On Mon, Aug 2, 2021 at 12:27 PM Biju Das
> >>>>>>> <biju.das.jz@bp.renesas.com>
> >>>>>>> wrote:
> >>>>>>>> The DMAC and EMAC blocks of Gigabit Ethernet IP found on RZ/G2L
> >>>>>>>> SoC are similar to the R-Car Ethernet AVB IP. With a few
> >>>>>>>> changes in the driver we can support both IPs.
> >>>>>>>>
> >>>>>>>> Currently a runtime decision based on the chip type is used to
> >>>>>>>> distinguish the HW differences between the SoC families.
> >>>>>>>>
> >>>>>>>> The number of TX descriptors for R-Car Gen3 is 1 whereas on
> >>>>>>>> R-Car
> >>>>>>>> Gen2 and RZ/G2L it is 2. For cases like this it is better to
> >>>>>>>> select the number of TX descriptors by using a structure with a
> >>>>>>>> value, rather than a runtime decision based on the chip type.
> >>>>>>>>
> >>>>>>>> This patch adds the num_tx_desc variable to struct ravb_hw_info
> >>>>>>>> and also replaces the driver data chip type with struct
> >>>>>>>> ravb_hw_info by moving chip type to it.
> >>>>>>>>
> >>>>>>>> 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!
> >>>>>>>
> >>>>>>>> --- a/drivers/net/ethernet/renesas/ravb.h
> >>>>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h
> >>>>>>>> @@ -988,6 +988,11 @@ enum ravb_chip_id {
> >>>>>>>>          RCAR_GEN3,
> >>>>>>>>   };
> >>>>>>>>
> >>>>>>>> +struct ravb_hw_info {
> >>>>>>>> +       enum ravb_chip_id chip_id;
> >>>>>>>> +       int num_tx_desc;
> >>>>>>>
> >>>>>>> Why not "unsigned int"? ...
> >>>>>>> This comment applies to a few more subsequent patches.
> >>>>>>
> >>>>>> To avoid signed and unsigned comparison warnings.
> >>>>>>
> >>>>>>>
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>>   struct ravb_private {
> >>>>>>>>          struct net_device *ndev;
> >>>>>>>>          struct platform_device *pdev; @@ -1040,6 +1045,8 @@
> >>>>>>>> struct ravb_private {
> >>>>>>>>          unsigned txcidm:1;              /* TX Clock Internal
> Delay
> >>>>> Mode
> >>>>>>> */
> >>>>>>>>          unsigned rgmii_override:1;      /* Deprecated rgmii-*id
> >>>>> behavior
> >>>>>>> */
> >>>>>>>>          int num_tx_desc;                /* TX descriptors per
> >>>> packet
> >>>>> */
> >>>>>>>
> >>>>>>> ... oh, here's the original culprit.
> >>>>>>
> >>>>>> Exactly, this the reason.
> >>>>>>
> >>>>>> Do you want me to change this into unsigned int? Please let me
> know.
> >>>>>
> >>>>> Up to you (or the maintainer? ;-)
> >>>>>
> >>>>> For new fields (in the other patches), I would use unsigned for
> >>>>> all unsigned values.  Signed values have more pitfalls related to
> >>>>> undefined behavior.
> >>>>
> >>>> Sergei, What is your thoughts here? Please let me know.
> >>>
> >>> Here is my plan.
> >>>
> >>> I will split this patch into two as Andrew suggested and
> >>
> >>     If you mran changing the ravb_private::num_tx_desc to *unsigned*,
> >> it'll be a good cleanup. What's would be the 2nd part tho?
> >
> > OK in that case, I will split this patch into 3.
> >
> > First patch for adding struct ravb_hw_info to driver data and replace
> > driver data chip type with struct ravb_hw_info
> 
>     Couldn't this be a 2nd patch?..
> 
> > Second patch for changing ravb_private::num_tx_desc from int to unsigned
> int.
> 
>     ... and this one the 1st?
> 
> > Third patch for adding aligned_tx to struct ravb_hw_info.

Sure. Will do.

Cheers,
Biju
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 80e62ca2e3d3..cfb972c05b34 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -988,6 +988,11 @@  enum ravb_chip_id {
 	RCAR_GEN3,
 };
 
+struct ravb_hw_info {
+	enum ravb_chip_id chip_id;
+	int num_tx_desc;
+};
+
 struct ravb_private {
 	struct net_device *ndev;
 	struct platform_device *pdev;
@@ -1040,6 +1045,8 @@  struct ravb_private {
 	unsigned txcidm:1;		/* TX Clock Internal Delay Mode */
 	unsigned rgmii_override:1;	/* Deprecated rgmii-*id behavior */
 	int num_tx_desc;		/* TX descriptors per packet */
+
+	const struct ravb_hw_info *info;
 };
 
 static inline u32 ravb_read(struct net_device *ndev, enum ravb_reg reg)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index f4dfe9f71d06..ffbd224d8780 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1924,12 +1924,22 @@  static int ravb_mdio_release(struct ravb_private *priv)
 	return 0;
 }
 
+static const struct ravb_hw_info ravb_gen3_hw_info = {
+	.chip_id = RCAR_GEN3,
+	.num_tx_desc = NUM_TX_DESC_GEN3,
+};
+
+static const struct ravb_hw_info ravb_gen2_hw_info = {
+	.chip_id = RCAR_GEN2,
+	.num_tx_desc = NUM_TX_DESC_GEN2,
+};
+
 static const struct of_device_id ravb_match_table[] = {
-	{ .compatible = "renesas,etheravb-r8a7790", .data = (void *)RCAR_GEN2 },
-	{ .compatible = "renesas,etheravb-r8a7794", .data = (void *)RCAR_GEN2 },
-	{ .compatible = "renesas,etheravb-rcar-gen2", .data = (void *)RCAR_GEN2 },
-	{ .compatible = "renesas,etheravb-r8a7795", .data = (void *)RCAR_GEN3 },
-	{ .compatible = "renesas,etheravb-rcar-gen3", .data = (void *)RCAR_GEN3 },
+	{ .compatible = "renesas,etheravb-r8a7790", .data = &ravb_gen2_hw_info },
+	{ .compatible = "renesas,etheravb-r8a7794", .data = &ravb_gen2_hw_info },
+	{ .compatible = "renesas,etheravb-rcar-gen2", .data = &ravb_gen2_hw_info },
+	{ .compatible = "renesas,etheravb-r8a7795", .data = &ravb_gen3_hw_info },
+	{ .compatible = "renesas,etheravb-rcar-gen3", .data = &ravb_gen3_hw_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ravb_match_table);
@@ -2034,8 +2044,8 @@  static void ravb_set_delay_mode(struct net_device *ndev)
 static int ravb_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
+	const struct ravb_hw_info *info;
 	struct ravb_private *priv;
-	enum ravb_chip_id chip_id;
 	struct net_device *ndev;
 	int error, irq, q;
 	struct resource *res;
@@ -2058,9 +2068,9 @@  static int ravb_probe(struct platform_device *pdev)
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
-	chip_id = (enum ravb_chip_id)of_device_get_match_data(&pdev->dev);
+	info = of_device_get_match_data(&pdev->dev);
 
-	if (chip_id == RCAR_GEN3)
+	if (info->chip_id == RCAR_GEN3)
 		irq = platform_get_irq_byname(pdev, "ch22");
 	else
 		irq = platform_get_irq(pdev, 0);
@@ -2073,6 +2083,7 @@  static int ravb_probe(struct platform_device *pdev)
 	SET_NETDEV_DEV(ndev, &pdev->dev);
 
 	priv = netdev_priv(ndev);
+	priv->info = info;
 	priv->ndev = ndev;
 	priv->pdev = pdev;
 	priv->num_tx_ring[RAVB_BE] = BE_TX_RING_SIZE;
@@ -2099,7 +2110,7 @@  static int ravb_probe(struct platform_device *pdev)
 	priv->avb_link_active_low =
 		of_property_read_bool(np, "renesas,ether-link-active-low");
 
-	if (chip_id == RCAR_GEN3) {
+	if (info->chip_id == RCAR_GEN3) {
 		irq = platform_get_irq_byname(pdev, "ch24");
 		if (irq < 0) {
 			error = irq;
@@ -2124,7 +2135,7 @@  static int ravb_probe(struct platform_device *pdev)
 		}
 	}
 
-	priv->chip_id = chip_id;
+	priv->chip_id = info->chip_id;
 
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
@@ -2142,8 +2153,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 = chip_id == RCAR_GEN2 ?
-		NUM_TX_DESC_GEN2 : NUM_TX_DESC_GEN3;
+	priv->num_tx_desc = info->num_tx_desc;
 
 	/* Set function */
 	ndev->netdev_ops = &ravb_netdev_ops;
@@ -2184,7 +2194,7 @@  static int ravb_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&priv->ts_skb_list);
 
 	/* Initialise PTP Clock driver */
-	if (chip_id != RCAR_GEN2)
+	if (info->chip_id != RCAR_GEN2)
 		ravb_ptp_init(ndev, pdev);
 
 	/* Debug message level */
@@ -2232,7 +2242,7 @@  static int ravb_probe(struct platform_device *pdev)
 			  priv->desc_bat_dma);
 
 	/* Stop PTP Clock driver */
-	if (chip_id != RCAR_GEN2)
+	if (info->chip_id != RCAR_GEN2)
 		ravb_ptp_stop(ndev);
 out_disable_refclk:
 	clk_disable_unprepare(priv->refclk);