| Message ID | 20210825070154.14336-5-biju.das.jz@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Factorisation code to support Gigabit Ethernet driver | expand |
Hi Sergei, Thanks for the feedback. > Subject: Re: [PATCH net-next 04/13] ravb: Add ptp_cfg_active to struct > ravb_hw_info > > On 8/25/21 10:01 AM, Biju Das wrote: > > > There are some H/W differences for the gPTP feature between R-Car > > Gen3, R-Car Gen2, and RZ/G2L as below. > > > > 1) On R-Car Gen3, gPTP support is active in config mode. > > 2) On R-Car Gen2, gPTP support is not active in config mode. > > 3) RZ/G2L does not support the gPTP feature. > > > > Add a ptp_cfg_active hw feature bit to struct ravb_hw_info for > > supporting gPTP active in config mode for R-Car Gen3. > > Wait, we've just done this ion the previous patch! > > > This patch also removes enum ravb_chip_id, chip_id from both struct > > ravb_hw_info and struct ravb_private, as it is unused. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/net/ethernet/renesas/ravb.h | 8 +------- > > drivers/net/ethernet/renesas/ravb_main.c | 12 +++++------- > > 2 files changed, 6 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > b/drivers/net/ethernet/renesas/ravb.h > > index 9ecf1a8c3ca8..209e030935aa 100644 > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -979,17 +979,11 @@ struct ravb_ptp { > > struct ravb_ptp_perout perout[N_PER_OUT]; }; > > > > -enum ravb_chip_id { > > - RCAR_GEN2, > > - RCAR_GEN3, > > -}; > > - > > struct ravb_hw_info { > > const char (*gstrings_stats)[ETH_GSTRING_LEN]; > > size_t gstrings_size; > > netdev_features_t net_hw_features; > > netdev_features_t net_features; > > - enum ravb_chip_id chip_id; > > int stats_len; > > size_t max_rx_len; > > I would put the above in a spearte patch... > > > unsigned aligned_tx: 1; > > @@ -999,6 +993,7 @@ struct ravb_hw_info { > > unsigned tx_counters:1; /* E-MAC has TX counters */ > > unsigned multi_irqs:1; /* AVB-DMAC and E-MAC has multiple > irqs */ > > unsigned no_ptp_cfg_active:1; /* AVB-DMAC does not support gPTP > active in config mode */ > > + unsigned ptp_cfg_active:1; /* AVB-DMAC has gPTP support active in > config mode */ > > Huh? > > > }; > > > > struct ravb_private { > [...] > > @@ -2216,7 +2213,7 @@ static int ravb_probe(struct platform_device > *pdev) > > INIT_LIST_HEAD(&priv->ts_skb_list); > > > > /* Initialise PTP Clock driver */ > > - if (info->chip_id != RCAR_GEN2) > > + if (info->ptp_cfg_active) > > ravb_ptp_init(ndev, pdev); > > What's that? Didn't you touch this lie in patch #3? > > This seems lie a NAK bait... :-( Please refer the original patch[1] which introduced gPTP support active in config mode. I am sure this will clear all your doubts. [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/net/ethernet/renesas/ravb_main.c?h=next-20210825&id=f5d7837f96e53a8c9b6c49e1bc95cf0ae88b99e8 Regards, Biju > > MBR, Sergey
On 26.08.2021 9:20, Biju Das wrote: [...] >>> There are some H/W differences for the gPTP feature between R-Car >>> Gen3, R-Car Gen2, and RZ/G2L as below. >>> >>> 1) On R-Car Gen3, gPTP support is active in config mode. >>> 2) On R-Car Gen2, gPTP support is not active in config mode. >>> 3) RZ/G2L does not support the gPTP feature. >>> >>> Add a ptp_cfg_active hw feature bit to struct ravb_hw_info for >>> supporting gPTP active in config mode for R-Car Gen3. >> >> Wait, we've just done this ion the previous patch! >> >>> This patch also removes enum ravb_chip_id, chip_id from both struct >>> ravb_hw_info and struct ravb_private, as it is unused. >>> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >>> --- >>> drivers/net/ethernet/renesas/ravb.h | 8 +------- >>> drivers/net/ethernet/renesas/ravb_main.c | 12 +++++------- >>> 2 files changed, 6 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/renesas/ravb.h >>> b/drivers/net/ethernet/renesas/ravb.h >>> index 9ecf1a8c3ca8..209e030935aa 100644 >>> --- a/drivers/net/ethernet/renesas/ravb.h >>> +++ b/drivers/net/ethernet/renesas/ravb.h >>> @@ -979,17 +979,11 @@ struct ravb_ptp { >>> struct ravb_ptp_perout perout[N_PER_OUT]; }; >>> >>> -enum ravb_chip_id { >>> - RCAR_GEN2, >>> - RCAR_GEN3, >>> -}; >>> - >>> struct ravb_hw_info { >>> const char (*gstrings_stats)[ETH_GSTRING_LEN]; >>> size_t gstrings_size; >>> netdev_features_t net_hw_features; >>> netdev_features_t net_features; >>> - enum ravb_chip_id chip_id; >>> int stats_len; >>> size_t max_rx_len; >> >> I would put the above in a spearte patch... Separate. :-) >>> unsigned aligned_tx: 1; >>> @@ -999,6 +993,7 @@ struct ravb_hw_info { >>> unsigned tx_counters:1; /* E-MAC has TX counters */ >>> unsigned multi_irqs:1; /* AVB-DMAC and E-MAC has multiple >> irqs */ >>> unsigned no_ptp_cfg_active:1; /* AVB-DMAC does not support gPTP >> active in config mode */ >>> + unsigned ptp_cfg_active:1; /* AVB-DMAC has gPTP support active in >> config mode */ >> >> Huh? >> >>> }; >>> >>> struct ravb_private { >> [...] >>> @@ -2216,7 +2213,7 @@ static int ravb_probe(struct platform_device >> *pdev) >>> INIT_LIST_HEAD(&priv->ts_skb_list); >>> >>> /* Initialise PTP Clock driver */ >>> - if (info->chip_id != RCAR_GEN2) >>> + if (info->ptp_cfg_active) >>> ravb_ptp_init(ndev, pdev); >> >> What's that? Didn't you touch this lie in patch #3? >> >> This seems lie a NAK bait... :-( > > Please refer the original patch[1] which introduced gPTP support active in config mode. > I am sure this will clear all your doubts. It hasn't. Why do we need 2 bit fields (1 "positive" and 1 "negative") for the same feature is beyond me. > [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/net/ethernet/renesas/ravb_main.c?h=next-20210825&id=f5d7837f96e53a8c9b6c49e1bc95cf0ae88b99e8 > > Regards, > Biju MBR, Sergey
Hi Sergei, > Subject: Re: [PATCH net-next 04/13] ravb: Add ptp_cfg_active to struct > ravb_hw_info > > On 26.08.2021 9:20, Biju Das wrote: > > [...] > >>> There are some H/W differences for the gPTP feature between R-Car > >>> Gen3, R-Car Gen2, and RZ/G2L as below. > >>> > >>> 1) On R-Car Gen3, gPTP support is active in config mode. > >>> 2) On R-Car Gen2, gPTP support is not active in config mode. > >>> 3) RZ/G2L does not support the gPTP feature. > >>> > >>> Add a ptp_cfg_active hw feature bit to struct ravb_hw_info for > >>> supporting gPTP active in config mode for R-Car Gen3. > >> > >> Wait, we've just done this ion the previous patch! > >> > >>> This patch also removes enum ravb_chip_id, chip_id from both struct > >>> ravb_hw_info and struct ravb_private, as it is unused. > >>> > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>> --- > >>> drivers/net/ethernet/renesas/ravb.h | 8 +------- > >>> drivers/net/ethernet/renesas/ravb_main.c | 12 +++++------- > >>> 2 files changed, 6 insertions(+), 14 deletions(-) > >>> > >>> diff --git a/drivers/net/ethernet/renesas/ravb.h > >>> b/drivers/net/ethernet/renesas/ravb.h > >>> index 9ecf1a8c3ca8..209e030935aa 100644 > >>> --- a/drivers/net/ethernet/renesas/ravb.h > >>> +++ b/drivers/net/ethernet/renesas/ravb.h > >>> @@ -979,17 +979,11 @@ struct ravb_ptp { > >>> struct ravb_ptp_perout perout[N_PER_OUT]; }; > >>> > >>> -enum ravb_chip_id { > >>> - RCAR_GEN2, > >>> - RCAR_GEN3, > >>> -}; > >>> - > >>> struct ravb_hw_info { > >>> const char (*gstrings_stats)[ETH_GSTRING_LEN]; > >>> size_t gstrings_size; > >>> netdev_features_t net_hw_features; > >>> netdev_features_t net_features; > >>> - enum ravb_chip_id chip_id; > >>> int stats_len; > >>> size_t max_rx_len; > >> > >> I would put the above in a spearte patch... > > Separate. :-) > > >>> unsigned aligned_tx: 1; > >>> @@ -999,6 +993,7 @@ struct ravb_hw_info { > >>> unsigned tx_counters:1; /* E-MAC has TX counters */ > >>> unsigned multi_irqs:1; /* AVB-DMAC and E-MAC has > multiple > >> irqs */ > >>> unsigned no_ptp_cfg_active:1; /* AVB-DMAC does not support > gPTP > >> active in config mode */ > >>> + unsigned ptp_cfg_active:1; /* AVB-DMAC has gPTP support active in > >> config mode */ > >> > >> Huh? > >> > >>> }; > >>> > >>> struct ravb_private { > >> [...] > >>> @@ -2216,7 +2213,7 @@ static int ravb_probe(struct platform_device > >> *pdev) > >>> INIT_LIST_HEAD(&priv->ts_skb_list); > >>> > >>> /* Initialise PTP Clock driver */ > >>> - if (info->chip_id != RCAR_GEN2) > >>> + if (info->ptp_cfg_active) > >>> ravb_ptp_init(ndev, pdev); > >> > >> What's that? Didn't you touch this lie in patch #3? > >> > >> This seems lie a NAK bait... :-( > > > > Please refer the original patch[1] which introduced gPTP support active > in config mode. > > I am sure this will clear all your doubts. > > It hasn't. Why do we need 2 bit fields (1 "positive" and 1 "negative") > for the same feature is beyond me. The reason is mentioned in commit description, Do you agree 1, 2 and 3 mutually exclusive? 1) On R-Car Gen3, gPTP support is active in config mode. 2) On R-Car Gen2, gPTP support is not active in config mode. 3) RZ/G2L does not support the gPTP feature. Regards, Biju > > > [1] > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit. > > kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fnext%2Flinux-next.git% > > 2Fcommit%2Fdrivers%2Fnet%2Fethernet%2Frenesas%2Fravb_main.c%3Fh%3Dnext > > -20210825%26id%3Df5d7837f96e53a8c9b6c49e1bc95cf0ae88b99e8&data=04% > > 7C01%7Cbiju.das.jz%40bp.renesas.com%7C142c7f172b4141617e7008d9687c7881 > > %7C53d82571da1947e49cb4625a166a4a2a%7C0%7C0%7C637655706082218315%7CUnk > > nown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWw > > iLCJXVCI6Mn0%3D%7C1000&sdata=Miy2q4JGEZpwLHlkGxlEBK8P0XAPzuHUX6xib > > FS8nDs%3D&reserved=0 > > > > Regards, > > Biju > > MBR, Sergey
On 26.08.2021 13:34, Biju Das wrote: [...] >>>>> There are some H/W differences for the gPTP feature between R-Car >>>>> Gen3, R-Car Gen2, and RZ/G2L as below. >>>>> >>>>> 1) On R-Car Gen3, gPTP support is active in config mode. >>>>> 2) On R-Car Gen2, gPTP support is not active in config mode. >>>>> 3) RZ/G2L does not support the gPTP feature. >>>>> >>>>> Add a ptp_cfg_active hw feature bit to struct ravb_hw_info for >>>>> supporting gPTP active in config mode for R-Car Gen3. >>>> >>>> Wait, we've just done this ion the previous patch! >>>> >>>>> This patch also removes enum ravb_chip_id, chip_id from both struct >>>>> ravb_hw_info and struct ravb_private, as it is unused. >>>>> >>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>>>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >>>>> --- >>>>> drivers/net/ethernet/renesas/ravb.h | 8 +------- >>>>> drivers/net/ethernet/renesas/ravb_main.c | 12 +++++------- >>>>> 2 files changed, 6 insertions(+), 14 deletions(-) >>>>> >>>>> diff --git a/drivers/net/ethernet/renesas/ravb.h >>>>> b/drivers/net/ethernet/renesas/ravb.h >>>>> index 9ecf1a8c3ca8..209e030935aa 100644 >>>>> --- a/drivers/net/ethernet/renesas/ravb.h >>>>> +++ b/drivers/net/ethernet/renesas/ravb.h >>>>> @@ -979,17 +979,11 @@ struct ravb_ptp { >>>>> struct ravb_ptp_perout perout[N_PER_OUT]; }; >>>>> >>>>> -enum ravb_chip_id { >>>>> - RCAR_GEN2, >>>>> - RCAR_GEN3, >>>>> -}; >>>>> - >>>>> struct ravb_hw_info { >>>>> const char (*gstrings_stats)[ETH_GSTRING_LEN]; >>>>> size_t gstrings_size; >>>>> netdev_features_t net_hw_features; >>>>> netdev_features_t net_features; >>>>> - enum ravb_chip_id chip_id; >>>>> int stats_len; >>>>> size_t max_rx_len; >>>> >>>> I would put the above in a spearte patch... >> >> Separate. :-) >> >>>>> unsigned aligned_tx: 1; >>>>> @@ -999,6 +993,7 @@ struct ravb_hw_info { >>>>> unsigned tx_counters:1; /* E-MAC has TX counters */ >>>>> unsigned multi_irqs:1; /* AVB-DMAC and E-MAC has >> multiple >>>> irqs */ >>>>> unsigned no_ptp_cfg_active:1; /* AVB-DMAC does not support >> gPTP >>>> active in config mode */ >>>>> + unsigned ptp_cfg_active:1; /* AVB-DMAC has gPTP support active in >>>> config mode */ >>>> >>>> Huh? >>>> >>>>> }; >>>>> >>>>> struct ravb_private { >>>> [...] >>>>> @@ -2216,7 +2213,7 @@ static int ravb_probe(struct platform_device >>>> *pdev) >>>>> INIT_LIST_HEAD(&priv->ts_skb_list); >>>>> >>>>> /* Initialise PTP Clock driver */ >>>>> - if (info->chip_id != RCAR_GEN2) >>>>> + if (info->ptp_cfg_active) >>>>> ravb_ptp_init(ndev, pdev); >>>> >>>> What's that? Didn't you touch this lie in patch #3? >>>> >>>> This seems lie a NAK bait... :-( >>> >>> Please refer the original patch[1] which introduced gPTP support active >> in config mode. >>> I am sure this will clear all your doubts. >> >> It hasn't. Why do we need 2 bit fields (1 "positive" and 1 "negative") >> for the same feature is beyond me. > > The reason is mentioned in commit description, Do you agree 1, 2 and 3 mutually exclusive? > > 1) On R-Car Gen3, gPTP support is active in config mode. > 2) On R-Car Gen2, gPTP support is not active in config mode. > 3) RZ/G2L does not support the gPTP feature. No, (1) includes (2). [...] > Regards, > Biju [...] MBR, Sergey
Hi Sergei, > Subject: Re: [PATCH net-next 04/13] ravb: Add ptp_cfg_active to struct > ravb_hw_info > > On 26.08.2021 13:34, Biju Das wrote: > > [...] > >>>>> There are some H/W differences for the gPTP feature between R-Car > >>>>> Gen3, R-Car Gen2, and RZ/G2L as below. > >>>>> > >>>>> 1) On R-Car Gen3, gPTP support is active in config mode. > >>>>> 2) On R-Car Gen2, gPTP support is not active in config mode. > >>>>> 3) RZ/G2L does not support the gPTP feature. > >>>>> > >>>>> Add a ptp_cfg_active hw feature bit to struct ravb_hw_info for > >>>>> supporting gPTP active in config mode for R-Car Gen3. > >>>> > >>>> Wait, we've just done this ion the previous patch! > >>>> > >>>>> This patch also removes enum ravb_chip_id, chip_id from both > >>>>> struct ravb_hw_info and struct ravb_private, as it is unused. > >>>>> > >>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >>>>> Reviewed-by: Lad Prabhakar > >>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com> > >>>>> --- > >>>>> drivers/net/ethernet/renesas/ravb.h | 8 +------- > >>>>> drivers/net/ethernet/renesas/ravb_main.c | 12 +++++------- > >>>>> 2 files changed, 6 insertions(+), 14 deletions(-) > >>>>> > >>>>> diff --git a/drivers/net/ethernet/renesas/ravb.h > >>>>> b/drivers/net/ethernet/renesas/ravb.h > >>>>> index 9ecf1a8c3ca8..209e030935aa 100644 > >>>>> --- a/drivers/net/ethernet/renesas/ravb.h > >>>>> +++ b/drivers/net/ethernet/renesas/ravb.h > >>>>> @@ -979,17 +979,11 @@ struct ravb_ptp { > >>>>> struct ravb_ptp_perout perout[N_PER_OUT]; }; > >>>>> > >>>>> -enum ravb_chip_id { > >>>>> - RCAR_GEN2, > >>>>> - RCAR_GEN3, > >>>>> -}; > >>>>> - > >>>>> struct ravb_hw_info { > >>>>> const char (*gstrings_stats)[ETH_GSTRING_LEN]; > >>>>> size_t gstrings_size; > >>>>> netdev_features_t net_hw_features; > >>>>> netdev_features_t net_features; > >>>>> - enum ravb_chip_id chip_id; > >>>>> int stats_len; > >>>>> size_t max_rx_len; > >>>> > >>>> I would put the above in a spearte patch... > >> > >> Separate. :-) > >> > >>>>> unsigned aligned_tx: 1; > >>>>> @@ -999,6 +993,7 @@ struct ravb_hw_info { > >>>>> unsigned tx_counters:1; /* E-MAC has TX counters */ > >>>>> unsigned multi_irqs:1; /* AVB-DMAC and E-MAC has > >> multiple > >>>> irqs */ > >>>>> unsigned no_ptp_cfg_active:1; /* AVB-DMAC does not support > >> gPTP > >>>> active in config mode */ > >>>>> + unsigned ptp_cfg_active:1; /* AVB-DMAC has gPTP support > active in > >>>> config mode */ > >>>> > >>>> Huh? > >>>> > >>>>> }; > >>>>> > >>>>> struct ravb_private { > >>>> [...] > >>>>> @@ -2216,7 +2213,7 @@ static int ravb_probe(struct platform_device > >>>> *pdev) > >>>>> INIT_LIST_HEAD(&priv->ts_skb_list); > >>>>> > >>>>> /* Initialise PTP Clock driver */ > >>>>> - if (info->chip_id != RCAR_GEN2) > >>>>> + if (info->ptp_cfg_active) > >>>>> ravb_ptp_init(ndev, pdev); > >>>> > >>>> What's that? Didn't you touch this lie in patch #3? > >>>> > >>>> This seems lie a NAK bait... :-( > >>> > >>> Please refer the original patch[1] which introduced gPTP support > >>> active > >> in config mode. > >>> I am sure this will clear all your doubts. > >> > >> It hasn't. Why do we need 2 bit fields (1 "positive" and 1 > >> "negative") for the same feature is beyond me. > > > > The reason is mentioned in commit description, Do you agree 1, 2 and 3 > mutually exclusive? > > > > 1) On R-Car Gen3, gPTP support is active in config mode. > > 2) On R-Car Gen2, gPTP support is not active in config mode. > > 3) RZ/G2L does not support the gPTP feature. > > No, (1) includes (2). patch[1] is for supporting gPTP support active in config mode. Do you agree GAC register(gPTP active in Config) bit in AVB-DMAC mode register(CCC) present only in R-Car Gen3? [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/net/ethernet/renesas/ravb_main.c?h=next-20210825&id=f5d7837f96e53a8c9b6c49e1bc95cf0ae88b99e8 Regards, Biju > > [...] > > > Regards, > > Biju > > [...] > > MBR, Sergey
On 8/26/21 1:52 PM, Biju Das wrote: [...] >>>>>>> There are some H/W differences for the gPTP feature between R-Car >>>>>>> Gen3, R-Car Gen2, and RZ/G2L as below. >>>>>>> >>>>>>> 1) On R-Car Gen3, gPTP support is active in config mode. >>>>>>> 2) On R-Car Gen2, gPTP support is not active in config mode. >>>>>>> 3) RZ/G2L does not support the gPTP feature. >>>>>>> >>>>>>> Add a ptp_cfg_active hw feature bit to struct ravb_hw_info for >>>>>>> supporting gPTP active in config mode for R-Car Gen3. >>>>>> >>>>>> Wait, we've just done this ion the previous patch! >>>>>> >>>>>>> This patch also removes enum ravb_chip_id, chip_id from both >>>>>>> struct ravb_hw_info and struct ravb_private, as it is unused. >>>>>>> >>>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>>>>>> Reviewed-by: Lad Prabhakar >>>>>>> <prabhakar.mahadev-lad.rj@bp.renesas.com> >>>>>>> --- >>>>>>> drivers/net/ethernet/renesas/ravb.h | 8 +------- >>>>>>> drivers/net/ethernet/renesas/ravb_main.c | 12 +++++------- >>>>>>> 2 files changed, 6 insertions(+), 14 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/net/ethernet/renesas/ravb.h >>>>>>> b/drivers/net/ethernet/renesas/ravb.h >>>>>>> index 9ecf1a8c3ca8..209e030935aa 100644 >>>>>>> --- a/drivers/net/ethernet/renesas/ravb.h >>>>>>> +++ b/drivers/net/ethernet/renesas/ravb.h >>>>>>> @@ -979,17 +979,11 @@ struct ravb_ptp { >>>>>>> struct ravb_ptp_perout perout[N_PER_OUT]; }; >>>>>>> >>>>>>> -enum ravb_chip_id { >>>>>>> - RCAR_GEN2, >>>>>>> - RCAR_GEN3, >>>>>>> -}; >>>>>>> - >>>>>>> struct ravb_hw_info { >>>>>>> const char (*gstrings_stats)[ETH_GSTRING_LEN]; >>>>>>> size_t gstrings_size; >>>>>>> netdev_features_t net_hw_features; >>>>>>> netdev_features_t net_features; >>>>>>> - enum ravb_chip_id chip_id; >>>>>>> int stats_len; >>>>>>> size_t max_rx_len; >>>>>> >>>>>> I would put the above in a spearte patch... >>>> >>>> Separate. :-) >>>> >>>>>>> unsigned aligned_tx: 1; >>>>>>> @@ -999,6 +993,7 @@ struct ravb_hw_info { >>>>>>> unsigned tx_counters:1; /* E-MAC has TX counters */ >>>>>>> unsigned multi_irqs:1; /* AVB-DMAC and E-MAC has >>>> multiple >>>>>> irqs */ >>>>>>> unsigned no_ptp_cfg_active:1; /* AVB-DMAC does not support >>>> gPTP >>>>>> active in config mode */ >>>>>>> + unsigned ptp_cfg_active:1; /* AVB-DMAC has gPTP support >> active in >>>>>> config mode */ >>>>>> >>>>>> Huh? >>>>>> >>>>>>> }; >>>>>>> >>>>>>> struct ravb_private { >>>>>> [...] >>>>>>> @@ -2216,7 +2213,7 @@ static int ravb_probe(struct platform_device >>>>>> *pdev) >>>>>>> INIT_LIST_HEAD(&priv->ts_skb_list); >>>>>>> >>>>>>> /* Initialise PTP Clock driver */ >>>>>>> - if (info->chip_id != RCAR_GEN2) >>>>>>> + if (info->ptp_cfg_active) >>>>>>> ravb_ptp_init(ndev, pdev); >>>>>> >>>>>> What's that? Didn't you touch this lie in patch #3? >>>>>> >>>>>> This seems lie a NAK bait... :-( >>>>> >>>>> Please refer the original patch[1] which introduced gPTP support >>>>> active >>>> in config mode. >>>>> I am sure this will clear all your doubts. >>>> >>>> It hasn't. Why do we need 2 bit fields (1 "positive" and 1 >>>> "negative") for the same feature is beyond me. >>> >>> The reason is mentioned in commit description, Do you agree 1, 2 and 3 >> mutually exclusive? >>> >>> 1) On R-Car Gen3, gPTP support is active in config mode. >>> 2) On R-Car Gen2, gPTP support is not active in config mode. >>> 3) RZ/G2L does not support the gPTP feature. >> >> No, (1) includes (2). > > patch[1] is for supporting gPTP support active in config mode. Yes. > Do you agree GAC register(gPTP active in Config) bit in AVB-DMAC mode register(CCC) present only in R-Car Gen3? Yes. But you feature naming is totally misguiding, nevertheless... > [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/net/ethernet/renesas/ravb_main.c?h=next-20210825&id=f5d7837f96e53a8c9b6c49e1bc95cf0ae88b99e8 > > Regards, > Biju [...] MBR, Sergey
> > Do you agree GAC register(gPTP active in Config) bit in AVB-DMAC mode register(CCC) present only in R-Car Gen3? > > Yes. > But you feature naming is totally misguiding, nevertheless... It can still be changed. Just suggest a new name. Andrew
On 8/26/21 9:57 PM, Andrew Lunn wrote: >>> Do you agree GAC register(gPTP active in Config) bit in AVB-DMAC mode register(CCC) present only in R-Car Gen3? >> >> Yes. >> But you feature naming is totally misguiding, nevertheless... > > It can still be changed. Thank goodness, yea! > Just suggest a new name. I'd prolly go with 'gptp' for the gPTP support and 'ccc_gac' for the gPTP working also in CONFIG mode (CCC.GAC controls this feature). > Andrew MBR, Sergey
On Thu, Aug 26, 2021 at 10:02:07PM +0300, Sergey Shtylyov wrote: > On 8/26/21 9:57 PM, Andrew Lunn wrote: > > >>> Do you agree GAC register(gPTP active in Config) bit in AVB-DMAC mode register(CCC) present only in R-Car Gen3? > >> > >> Yes. > >> But you feature naming is totally misguiding, nevertheless... > > > > It can still be changed. > > Thank goodness, yea! We have to live with the first version of this in the git history, but we can add more patches fixing up whatever is broken in the unreviewed code which got merged. > > Just suggest a new name. > > I'd prolly go with 'gptp' for the gPTP support and 'ccc_gac' for the gPTP working also in CONFIG mode > (CCC.GAC controls this feature). Biju, please could you work on a couple of patches to change the names. I also suggest you post further refactoring patches as RFC. We might get a chance to review them then. Andrew
Hi Andrew, Thanks for the feedback. > Subject: Re: [PATCH net-next 04/13] ravb: Add ptp_cfg_active to struct > ravb_hw_info > > On Thu, Aug 26, 2021 at 10:02:07PM +0300, Sergey Shtylyov wrote: > > On 8/26/21 9:57 PM, Andrew Lunn wrote: > > > > >>> Do you agree GAC register(gPTP active in Config) bit in AVB-DMAC > mode register(CCC) present only in R-Car Gen3? > > >> > > >> Yes. > > >> But you feature naming is totally misguiding, nevertheless... > > > > > > It can still be changed. > > > > Thank goodness, yea! > > We have to live with the first version of this in the git history, but we > can add more patches fixing up whatever is broken in the unreviewed code > which got merged. > > > > Just suggest a new name. > > > > I'd prolly go with 'gptp' for the gPTP support and 'ccc_gac' for > > the gPTP working also in CONFIG mode (CCC.GAC controls this feature). > > Biju, please could you work on a couple of patches to change the names. Yes. Will work on the patches to change the names as suggested. > > I also suggest you post further refactoring patches as RFC. We might get a > chance to review them then. Agreed. Cheers, Biju
On 8/26/21 10:37 PM, Biju Das wrote: [...] >>>>>> Do you agree GAC register(gPTP active in Config) bit in AVB-DMAC >> mode register(CCC) present only in R-Car Gen3? >>>>> >>>>> Yes. >>>>> But you feature naming is totally misguiding, nevertheless... >>>> >>>> It can still be changed. >>> >>> Thank goodness, yea! >> >> We have to live with the first version of this in the git history, but we >> can add more patches fixing up whatever is broken in the unreviewed code >> which got merged. >> >>>> Just suggest a new name. >>> >>> I'd prolly go with 'gptp' for the gPTP support and 'ccc_gac' for >>> the gPTP working also in CONFIG mode (CCC.GAC controls this feature). >> >> Biju, please could you work on a couple of patches to change the names. > > Yes. Will work on the patches to change the names as suggested. TIA! After some more thinking, 'no_gptp' seems to suit better for the 1st case Might need to invert the checks tho... [...] > Cheers, > Biju MBR, Sergey
Hi Sergei, Thanks for the feedback. > Subject: Re: [PATCH net-next 04/13] ravb: Add ptp_cfg_active to struct > ravb_hw_info > > On 8/26/21 10:37 PM, Biju Das wrote: > [...] > > >>>>>> Do you agree GAC register(gPTP active in Config) bit in AVB-DMAC > >> mode register(CCC) present only in R-Car Gen3? > >>>>> > >>>>> Yes. > >>>>> But you feature naming is totally misguiding, nevertheless... > >>>> > >>>> It can still be changed. > >>> > >>> Thank goodness, yea! > >> > >> We have to live with the first version of this in the git history, > >> but we can add more patches fixing up whatever is broken in the > >> unreviewed code which got merged. > >> > >>>> Just suggest a new name. > >>> > >>> I'd prolly go with 'gptp' for the gPTP support and 'ccc_gac' for > >>> the gPTP working also in CONFIG mode (CCC.GAC controls this feature). > >> > >> Biju, please could you work on a couple of patches to change the names. > > > > Yes. Will work on the patches to change the names as suggested. > > TIA! > After some more thinking, 'no_gptp' seems to suit better for the 1st > case Might need to invert the checks tho... OK, Will do with invert checks. So just to conclude, 'no_gptp' and 'ccc_gac' are the suggested names changes for the previous patch and current patch. Cheers, Biju
On 27.08.2021 9:36, Biju Das wrote: [...] >>>>>>>> Do you agree GAC register(gPTP active in Config) bit in AVB-DMAC >>>> mode register(CCC) present only in R-Car Gen3? >>>>>>> >>>>>>> Yes. >>>>>>> But you feature naming is totally misguiding, nevertheless... >>>>>> >>>>>> It can still be changed. >>>>> >>>>> Thank goodness, yea! >>>> >>>> We have to live with the first version of this in the git history, >>>> but we can add more patches fixing up whatever is broken in the >>>> unreviewed code which got merged. >>>> >>>>>> Just suggest a new name. >>>>> >>>>> I'd prolly go with 'gptp' for the gPTP support and 'ccc_gac' for >>>>> the gPTP working also in CONFIG mode (CCC.GAC controls this feature). >>>> >>>> Biju, please could you work on a couple of patches to change the names. >>> >>> Yes. Will work on the patches to change the names as suggested. >> >> TIA! >> After some more thinking, 'no_gptp' seems to suit better for the 1st >> case Might need to invert the checks tho... > > OK, Will do with invert checks. > > So just to conclude, > > 'no_gptp' and 'ccc_gac' are the suggested names changes for the previous patch > and current patch. Your patches have been merged already. Might try to encompass all gPTP features with one patch (just a thought)... > Cheers, > Biju MBR, Sergey
Hi Sergei, Thanks for the feedback. > Subject: Re: [PATCH net-next 04/13] ravb: Add ptp_cfg_active to struct > ravb_hw_info > > On 27.08.2021 9:36, Biju Das wrote: > > [...] > > >>>>>>>> Do you agree GAC register(gPTP active in Config) bit in > >>>>>>>> AVB-DMAC > >>>> mode register(CCC) present only in R-Car Gen3? > >>>>>>> > >>>>>>> Yes. > >>>>>>> But you feature naming is totally misguiding, nevertheless... > >>>>>> > >>>>>> It can still be changed. > >>>>> > >>>>> Thank goodness, yea! > >>>> > >>>> We have to live with the first version of this in the git history, > >>>> but we can add more patches fixing up whatever is broken in the > >>>> unreviewed code which got merged. > >>>> > >>>>>> Just suggest a new name. > >>>>> > >>>>> I'd prolly go with 'gptp' for the gPTP support and 'ccc_gac' > >>>>> for the gPTP working also in CONFIG mode (CCC.GAC controls this > feature). > >>>> > >>>> Biju, please could you work on a couple of patches to change the > names. > >>> > >>> Yes. Will work on the patches to change the names as suggested. > >> > >> TIA! > >> After some more thinking, 'no_gptp' seems to suit better for the > >> 1st case Might need to invert the checks tho... > > > > OK, Will do with invert checks. > > > > So just to conclude, > > > > 'no_gptp' and 'ccc_gac' are the suggested names changes for the > > previous patch and current patch. > > Your patches have been merged already. Might try to encompass all > gPTP features with one patch (just a thought)... OK, in that case it will be taken care in next RFC patch set. Regards, Biju
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index 9ecf1a8c3ca8..209e030935aa 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -979,17 +979,11 @@ struct ravb_ptp { struct ravb_ptp_perout perout[N_PER_OUT]; }; -enum ravb_chip_id { - RCAR_GEN2, - RCAR_GEN3, -}; - struct ravb_hw_info { const char (*gstrings_stats)[ETH_GSTRING_LEN]; size_t gstrings_size; netdev_features_t net_hw_features; netdev_features_t net_features; - enum ravb_chip_id chip_id; int stats_len; size_t max_rx_len; unsigned aligned_tx: 1; @@ -999,6 +993,7 @@ struct ravb_hw_info { unsigned tx_counters:1; /* E-MAC has TX counters */ unsigned multi_irqs:1; /* AVB-DMAC and E-MAC has multiple irqs */ unsigned no_ptp_cfg_active:1; /* AVB-DMAC does not support gPTP active in config mode */ + unsigned ptp_cfg_active:1; /* AVB-DMAC has gPTP support active in config mode */ }; struct ravb_private { @@ -1042,7 +1037,6 @@ struct ravb_private { int msg_enable; int speed; int emac_irq; - enum ravb_chip_id chip_id; int rx_irqs[NUM_RX_QUEUE]; int tx_irqs[NUM_TX_QUEUE]; diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index e33b836218f0..883db1049882 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1941,12 +1941,12 @@ static const struct ravb_hw_info ravb_gen3_hw_info = { .gstrings_size = sizeof(ravb_gstrings_stats), .net_hw_features = NETIF_F_RXCSUM, .net_features = NETIF_F_RXCSUM, - .chip_id = RCAR_GEN3, .stats_len = ARRAY_SIZE(ravb_gstrings_stats), .max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1, .internal_delay = 1, .tx_counters = 1, .multi_irqs = 1, + .ptp_cfg_active = 1, }; static const struct ravb_hw_info ravb_gen2_hw_info = { @@ -1954,7 +1954,6 @@ static const struct ravb_hw_info ravb_gen2_hw_info = { .gstrings_size = sizeof(ravb_gstrings_stats), .net_hw_features = NETIF_F_RXCSUM, .net_features = NETIF_F_RXCSUM, - .chip_id = RCAR_GEN2, .stats_len = ARRAY_SIZE(ravb_gstrings_stats), .max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1, .aligned_tx = 1, @@ -2152,8 +2151,6 @@ static int ravb_probe(struct platform_device *pdev) } } - priv->chip_id = info->chip_id; - priv->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(priv->clk)) { error = PTR_ERR(priv->clk); @@ -2216,7 +2213,7 @@ static int ravb_probe(struct platform_device *pdev) INIT_LIST_HEAD(&priv->ts_skb_list); /* Initialise PTP Clock driver */ - if (info->chip_id != RCAR_GEN2) + if (info->ptp_cfg_active) ravb_ptp_init(ndev, pdev); /* Debug message level */ @@ -2264,7 +2261,7 @@ static int ravb_probe(struct platform_device *pdev) priv->desc_bat_dma); /* Stop PTP Clock driver */ - if (info->chip_id != RCAR_GEN2) + if (info->ptp_cfg_active) ravb_ptp_stop(ndev); out_disable_refclk: clk_disable_unprepare(priv->refclk); @@ -2280,9 +2277,10 @@ static int ravb_remove(struct platform_device *pdev) { struct net_device *ndev = platform_get_drvdata(pdev); struct ravb_private *priv = netdev_priv(ndev); + const struct ravb_hw_info *info = priv->info; /* Stop PTP Clock driver */ - if (priv->chip_id != RCAR_GEN2) + if (info->ptp_cfg_active) ravb_ptp_stop(ndev); clk_disable_unprepare(priv->refclk);