| Message ID | 20210923140813.13541-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add Gigabit Ethernet driver support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | fail | Series longer than 15 patches |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 12 of 12 maintainers |
| 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 84 exceeds 80 columns WARNING: line length of 93 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On 9/23/21 5:07 PM, Biju Das wrote: > Rename the variable "no_ptp_cfg_active" with "no_gptp" with inverted > checks and "ptp_cfg_active" with "ccc_gac". That's not exactly rename, no? At least for the 1st case... > There is no functional change. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> > --- > drivers/net/ethernet/renesas/ravb.h | 4 ++-- > drivers/net/ethernet/renesas/ravb_main.c | 25 ++++++++++++------------ > 2 files changed, 14 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h > index 7363abae6e59..0ce0c13ef8cb 100644 > --- a/drivers/net/ethernet/renesas/ravb.h > +++ b/drivers/net/ethernet/renesas/ravb.h > @@ -1000,8 +1000,8 @@ struct ravb_hw_info { > unsigned internal_delay:1; /* AVB-DMAC has internal delays */ > 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 */ > + unsigned no_gptp:1; /* AVB-DMAC does not support gPTP feature */ Judging on the flag usage (which ensues using logical not in every case), I'd suggest to invert this flag and call it 'gptp'... [...] MBR, Sergey
Hi Sergei, Thanks for the feedback. > -----Original Message----- > Subject: Re: [RFC/PATCH 02/18] ravb: Rename the variables > "no_ptp_cfg_active" and "ptp_cfg_active" > > On 9/23/21 5:07 PM, Biju Das wrote: > > > Rename the variable "no_ptp_cfg_active" with "no_gptp" with inverted > > checks and "ptp_cfg_active" with "ccc_gac". > > That's not exactly rename, no? At least for the 1st case... This is what we agreed as per last discussion[1]. https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210825070154.14336-5-biju.das.jz@bp.renesas.com/ > > > There is no functional change. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > --- > > drivers/net/ethernet/renesas/ravb.h | 4 ++-- > > drivers/net/ethernet/renesas/ravb_main.c | 25 > > ++++++++++++------------ > > 2 files changed, 14 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > b/drivers/net/ethernet/renesas/ravb.h > > index 7363abae6e59..0ce0c13ef8cb 100644 > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -1000,8 +1000,8 @@ struct ravb_hw_info { > > unsigned internal_delay:1; /* AVB-DMAC has internal delays */ > > 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 */ > > + unsigned no_gptp:1; /* AVB-DMAC does not support gPTP > feature */ > > Judging on the flag usage (which ensues using logical not in every > case), I'd suggest to invert this flag and call it 'gptp'... We have the following cases Case 1) On R-Car Gen3, gPTP support is active in config mode. Case 2) On R-Car Gen2, gPTP support is not active in config mode. Case 3) RZ/G2L does not support the gPTP feature. And I came up with patches [1] and [2]. Then as per discussion we agreed for gPTP support active in config(ccc_gac) which take care of Case 1, no_gptp which take care of case 3 And the cases not under 1 and 3 falls to 2. [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210825070154.14336-4-biju.das.jz@bp.renesas.com/ [2] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210825070154.14336-5-biju.das.jz@bp.renesas.com/ So please clear on your proposals to accomodate the 3 use cases as mentioned below. Case 1) On R-Car Gen3, gPTP support is active in config mode. Case 2) On R-Car Gen2, gPTP support is not active in config mode. Case 3) RZ/G2L does not support the gPTP feature. Regards, Biju > > [...] > > MBR, Sergey
On 9/23/21 7:35 PM, Biju Das wrote: [...] >>> Rename the variable "no_ptp_cfg_active" with "no_gptp" with inverted >>> checks and "ptp_cfg_active" with "ccc_gac". >> >> That's not exactly rename, no? At least for the 1st case... > > This is what we agreed as per last discussion[1]. > > https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210825070154.14336-5-biju.das.jz@bp.renesas.com/ Sorry, I've changed my mind about 'no_gpgp' after seeing all the checks. I'd like to avoiud the double negations in those checks -- this should make the code more clear. My 1st idea (just 'gp[tp') turned out to be more practical, sorry about this going back-and-forth. :-< [...] MBR, Sergey
Hi Sergei, > Subject: Re: [RFC/PATCH 02/18] ravb: Rename the variables > "no_ptp_cfg_active" and "ptp_cfg_active" > > On 9/23/21 7:35 PM, Biju Das wrote: > > [...] > >>> Rename the variable "no_ptp_cfg_active" with "no_gptp" with inverted > >>> checks and "ptp_cfg_active" with "ccc_gac". > >> > >> That's not exactly rename, no? At least for the 1st case... > > > > This is what we agreed as per last discussion[1]. > > > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc > > hwork.kernel.org%2Fproject%2Flinux-renesas-soc%2Fpatch%2F2021082507015 > > 4.14336-5-biju.das.jz%40bp.renesas.com%2F&data=04%7C01%7Cbiju.das. > > jz%40bp.renesas.com%7Cec41661b87e14f9e810808d97ebbae07%7C53d82571da194 > > 7e49cb4625a166a4a2a%7C0%7C0%7C637680166814248680%7CUnknown%7CTWFpbGZsb > > 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D% > > 7C1000&sdata=ze0ica0K57exFOSQ9LyMuQ%2FFimvOW4PtH8ETxYJ8o6Y%3D& > > reserved=0 > > Sorry, I've changed my mind about 'no_gpgp' after seeing all the > checks. I'd like to avoiud the double negations in those checks -- this > should make the code more clear. My 1st idea (just 'gp[tp') turned out to > be more practical, sorry about this going back-and-forth. :-< So Just to confirm the name to be used are "ccc_gac" and "gptp". Case 1) On R-Car Gen3, gPTP support is active in config mode. (replace "ptp_cfg_active" with "ccc_gac") Case 2) On R-Car Gen2, gPTP support is not active in config mode ( replace "no_ptp_cfg_active" with "gptp") Case 3) RZ/G2L does not support the gPTP feature(if "no_gac" or "gptp" then it falls to case 3). Regards, Biju
Hi Sergei, > Subject: RE: [RFC/PATCH 02/18] ravb: Rename the variables > "no_ptp_cfg_active" and "ptp_cfg_active" > > Hi Sergei, > > > Subject: Re: [RFC/PATCH 02/18] ravb: Rename the variables > > "no_ptp_cfg_active" and "ptp_cfg_active" > > > > On 9/23/21 7:35 PM, Biju Das wrote: > > > > [...] > > >>> Rename the variable "no_ptp_cfg_active" with "no_gptp" with > > >>> inverted checks and "ptp_cfg_active" with "ccc_gac". > > >> > > >> That's not exactly rename, no? At least for the 1st case... > > > > > > This is what we agreed as per last discussion[1]. > > > > > > https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa > > > tc > > > hwork.kernel.org%2Fproject%2Flinux-renesas-soc%2Fpatch%2F20210825070 > > > 15 > > > 4.14336-5-biju.das.jz%40bp.renesas.com%2F&data=04%7C01%7Cbiju.das. > > > jz%40bp.renesas.com%7Cec41661b87e14f9e810808d97ebbae07%7C53d82571da1 > > > 94 > > > 7e49cb4625a166a4a2a%7C0%7C0%7C637680166814248680%7CUnknown%7CTWFpbGZ > > > sb > > > 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3 > > > D% > > > 7C1000&sdata=ze0ica0K57exFOSQ9LyMuQ%2FFimvOW4PtH8ETxYJ8o6Y%3D&am > > > p; > > > reserved=0 > > > > Sorry, I've changed my mind about 'no_gpgp' after seeing all the > > checks. I'd like to avoiud the double negations in those checks -- > > this should make the code more clear. My 1st idea (just 'gp[tp') > > turned out to be more practical, sorry about this going > > back-and-forth. :-< > > So Just to confirm the name to be used are "ccc_gac" and "gptp". > > Case 1) On R-Car Gen3, gPTP support is active in config mode. (replace > "ptp_cfg_active" with "ccc_gac") Case 2) On R-Car Gen2, gPTP support is > not active in config mode ( replace "no_ptp_cfg_active" with "gptp") Case > 3) RZ/G2L does not support the gPTP feature(if "no_gac" or "gptp" then it > falls to case 3). As per the above discussion, I have prepared a new patch. Regards, Biju
diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h index 7363abae6e59..0ce0c13ef8cb 100644 --- a/drivers/net/ethernet/renesas/ravb.h +++ b/drivers/net/ethernet/renesas/ravb.h @@ -1000,8 +1000,8 @@ struct ravb_hw_info { unsigned internal_delay:1; /* AVB-DMAC has internal delays */ 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 */ + unsigned no_gptp:1; /* AVB-DMAC does not support gPTP feature */ + unsigned ccc_gac:1; /* AVB-DMAC has gPTP support active in config mode */ }; struct ravb_private { diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 8f2358caef34..2422e74d9b4f 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1274,7 +1274,7 @@ static int ravb_set_ringparam(struct net_device *ndev, if (netif_running(ndev)) { netif_device_detach(ndev); /* Stop PTP Clock driver */ - if (info->no_ptp_cfg_active) + if (!info->no_gptp && !info->ccc_gac) ravb_ptp_stop(ndev); /* Wait for DMA stopping */ error = ravb_stop_dma(ndev); @@ -1306,7 +1306,7 @@ static int ravb_set_ringparam(struct net_device *ndev, ravb_emac_init(ndev); /* Initialise PTP Clock driver */ - if (info->no_ptp_cfg_active) + if (!info->no_gptp && !info->ccc_gac) ravb_ptp_init(ndev, priv->pdev); netif_device_attach(ndev); @@ -1446,7 +1446,7 @@ static int ravb_open(struct net_device *ndev) ravb_emac_init(ndev); /* Initialise PTP Clock driver */ - if (info->no_ptp_cfg_active) + if (!info->no_gptp && !info->ccc_gac) ravb_ptp_init(ndev, priv->pdev); netif_tx_start_all_queues(ndev); @@ -1460,7 +1460,7 @@ static int ravb_open(struct net_device *ndev) out_ptp_stop: /* Stop PTP Clock driver */ - if (info->no_ptp_cfg_active) + if (!info->no_gptp && !info->ccc_gac) ravb_ptp_stop(ndev); out_free_irq_nc_tx: if (!info->multi_irqs) @@ -1508,7 +1508,7 @@ static void ravb_tx_timeout_work(struct work_struct *work) netif_tx_stop_all_queues(ndev); /* Stop PTP Clock driver */ - if (info->no_ptp_cfg_active) + if (!info->no_gptp && !info->ccc_gac) ravb_ptp_stop(ndev); /* Wait for DMA stopping */ @@ -1543,7 +1543,7 @@ static void ravb_tx_timeout_work(struct work_struct *work) out: /* Initialise PTP Clock driver */ - if (info->no_ptp_cfg_active) + if (!info->no_gptp && !info->ccc_gac) ravb_ptp_init(ndev, priv->pdev); netif_tx_start_all_queues(ndev); @@ -1752,7 +1752,7 @@ static int ravb_close(struct net_device *ndev) ravb_write(ndev, 0, TIC); /* Stop PTP Clock driver */ - if (info->no_ptp_cfg_active) + if (!info->no_gptp && !info->ccc_gac) ravb_ptp_stop(ndev); /* Set the config mode to stop the AVB-DMAC's processes */ @@ -2018,7 +2018,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = { .internal_delay = 1, .tx_counters = 1, .multi_irqs = 1, - .ptp_cfg_active = 1, + .ccc_gac = 1, }; static const struct ravb_hw_info ravb_gen2_hw_info = { @@ -2037,7 +2037,6 @@ static const struct ravb_hw_info ravb_gen2_hw_info = { .stats_len = ARRAY_SIZE(ravb_gstrings_stats), .max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1, .aligned_tx = 1, - .no_ptp_cfg_active = 1, }; static const struct of_device_id ravb_match_table[] = { @@ -2080,7 +2079,7 @@ static void ravb_set_config_mode(struct net_device *ndev) struct ravb_private *priv = netdev_priv(ndev); const struct ravb_hw_info *info = priv->info; - if (info->no_ptp_cfg_active) { + if (!info->no_gptp && !info->ccc_gac) { ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG); /* Set CSEL value */ ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB); @@ -2301,7 +2300,7 @@ static int ravb_probe(struct platform_device *pdev) INIT_LIST_HEAD(&priv->ts_skb_list); /* Initialise PTP Clock driver */ - if (info->ptp_cfg_active) + if (info->ccc_gac) ravb_ptp_init(ndev, pdev); /* Debug message level */ @@ -2349,7 +2348,7 @@ static int ravb_probe(struct platform_device *pdev) priv->desc_bat_dma); /* Stop PTP Clock driver */ - if (info->ptp_cfg_active) + if (info->ccc_gac) ravb_ptp_stop(ndev); out_disable_refclk: clk_disable_unprepare(priv->refclk); @@ -2369,7 +2368,7 @@ static int ravb_remove(struct platform_device *pdev) const struct ravb_hw_info *info = priv->info; /* Stop PTP Clock driver */ - if (info->ptp_cfg_active) + if (info->ccc_gac) ravb_ptp_stop(ndev); clk_disable_unprepare(priv->refclk);
Rename the variable "no_ptp_cfg_active" with "no_gptp" with inverted checks and "ptp_cfg_active" with "ccc_gac". There is no functional change. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Suggested-by: Sergey Shtylyov <s.shtylyov@omp.ru> --- drivers/net/ethernet/renesas/ravb.h | 4 ++-- drivers/net/ethernet/renesas/ravb_main.c | 25 ++++++++++++------------ 2 files changed, 14 insertions(+), 15 deletions(-)