| Message ID | 20210923140813.13541-6-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 | success | total: 0 errors, 0 warnings, 0 checks, 99 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On 9/23/21 5:08 PM, Biju Das wrote: > R-Car supports gPTP feature whereas RZ/G2L does not support it. > This patch excludes gtp feature support for RZ/G2L by enabling > no_gptp feature bit. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > drivers/net/ethernet/renesas/ravb_main.c | 46 ++++++++++++++---------- > 1 file changed, 28 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index d38fc33a8e93..8663d83507a0 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c [...] > @@ -953,7 +954,7 @@ static irqreturn_t ravb_interrupt(int irq, void *dev_id) > } > > /* gPTP interrupt status summary */ > - if (iss & ISS_CGIS) { Isn't this bit always 0 on RZ/G2L? > + if (!info->no_gptp && (iss & ISS_CGIS)) { > ravb_ptp_interrupt(ndev); > result = IRQ_HANDLED; > } > @@ -1378,6 +1379,7 @@ static int ravb_get_ts_info(struct net_device *ndev, > struct ethtool_ts_info *info) > { > struct ravb_private *priv = netdev_priv(ndev); > + const struct ravb_hw_info *hw_info = priv->info; > > info->so_timestamping = > SOF_TIMESTAMPING_TX_SOFTWARE | > @@ -1391,7 +1393,8 @@ static int ravb_get_ts_info(struct net_device *ndev, > (1 << HWTSTAMP_FILTER_NONE) | > (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | > (1 << HWTSTAMP_FILTER_ALL); > - info->phc_index = ptp_clock_index(priv->ptp.clock); > + if (!hw_info->no_gptp) > + info->phc_index = ptp_clock_index(priv->ptp.clock); > > return 0; > } > @@ -2116,6 +2119,7 @@ static const struct ravb_hw_info rgeth_hw_info = { > .emac_init = ravb_rgeth_emac_init, > .aligned_tx = 1, > .tx_counters = 1, > + .no_gptp = 1, Mhm, I definitely don't like the way you "extend" the GbEthernet info structure. All the applicable flags should be set in the last patch of the series, not amidst of it. [...] MBR, Sergey
Hi Sergei, Thanks for the review. > -----Original Message----- > From: Sergey Shtylyov <s.shtylyov@omp.ru> > Sent: 23 September 2021 20:00 > To: Biju Das <biju.das.jz@bp.renesas.com>; David S. Miller > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org> > Cc: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; > Andrew Lunn <andrew@lunn.ch>; Sergei Shtylyov <sergei.shtylyov@gmail.com>; > Geert Uytterhoeven <geert+renesas@glider.be>; Adam Ford > <aford173@gmail.com>; Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com>; netdev@vger.kernel.org; linux-renesas- > soc@vger.kernel.org; Chris Paterson <Chris.Paterson2@renesas.com>; Biju > Das <biju.das@bp.renesas.com> > Subject: Re: [RFC/PATCH 05/18] ravb: Exclude gPTP feature support for > RZ/G2L > > On 9/23/21 5:08 PM, Biju Das wrote: > > > R-Car supports gPTP feature whereas RZ/G2L does not support it. > > This patch excludes gtp feature support for RZ/G2L by enabling no_gptp > > feature bit. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > drivers/net/ethernet/renesas/ravb_main.c | 46 > > ++++++++++++++---------- > > 1 file changed, 28 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index d38fc33a8e93..8663d83507a0 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > [...] > > @@ -953,7 +954,7 @@ static irqreturn_t ravb_interrupt(int irq, void > *dev_id) > > } > > > > /* gPTP interrupt status summary */ > > - if (iss & ISS_CGIS) { > > Isn't this bit always 0 on RZ/G2L? This CGIM bit(BIT13) which is present on R-Car Gen3 is not present in RZ/G2L. As per the HW manual BIT13 is reserved bit and read is always 0. > > > + if (!info->no_gptp && (iss & ISS_CGIS)) { > > ravb_ptp_interrupt(ndev); > > result = IRQ_HANDLED; > > } > > @@ -1378,6 +1379,7 @@ static int ravb_get_ts_info(struct net_device > *ndev, > > struct ethtool_ts_info *info) > > { > > struct ravb_private *priv = netdev_priv(ndev); > > + const struct ravb_hw_info *hw_info = priv->info; > > > > info->so_timestamping = > > SOF_TIMESTAMPING_TX_SOFTWARE | > > @@ -1391,7 +1393,8 @@ static int ravb_get_ts_info(struct net_device > *ndev, > > (1 << HWTSTAMP_FILTER_NONE) | > > (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | > > (1 << HWTSTAMP_FILTER_ALL); > > - info->phc_index = ptp_clock_index(priv->ptp.clock); > > + if (!hw_info->no_gptp) > > + info->phc_index = ptp_clock_index(priv->ptp.clock); > > > > return 0; > > } > > @@ -2116,6 +2119,7 @@ static const struct ravb_hw_info rgeth_hw_info = { > > .emac_init = ravb_rgeth_emac_init, > > .aligned_tx = 1, > > .tx_counters = 1, > > + .no_gptp = 1, > > Mhm, I definitely don't like the way you "extend" the GbEthernet info > structure. All the applicable flags should be set in the last patch of the > series, not amidst of it. According to me, It is clearer with smaller patches like, what we have done with previous 2 patch sets for factorisation. Please correct me, if any one have different opinion. Regards, Biju > > [...] > > MBR, Sergey
On 9/23/21 10:13 PM, Biju Das wrote: [...] >>> R-Car supports gPTP feature whereas RZ/G2L does not support it. >>> This patch excludes gtp feature support for RZ/G2L by enabling no_gptp >>> feature bit. >>> >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>> --- >>> drivers/net/ethernet/renesas/ravb_main.c | 46 >>> ++++++++++++++---------- >>> 1 file changed, 28 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c >>> b/drivers/net/ethernet/renesas/ravb_main.c >>> index d38fc33a8e93..8663d83507a0 100644 >>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> [...] >>> @@ -953,7 +954,7 @@ static irqreturn_t ravb_interrupt(int irq, void >> *dev_id) >>> } >>> >>> /* gPTP interrupt status summary */ >>> - if (iss & ISS_CGIS) { >> >> Isn't this bit always 0 on RZ/G2L? > > This CGIM bit(BIT13) which is present on R-Car Gen3 is not present in RZ/G2L. As per the HW manual > BIT13 is reserved bit and read is always 0. > >> >>> + if (!info->no_gptp && (iss & ISS_CGIS)) { Then extending this check doesn't seem necessary? >>> ravb_ptp_interrupt(ndev); >>> result = IRQ_HANDLED; >>> } [...] >>> @@ -2116,6 +2119,7 @@ static const struct ravb_hw_info rgeth_hw_info = { >>> .emac_init = ravb_rgeth_emac_init, >>> .aligned_tx = 1, >>> .tx_counters = 1, >>> + .no_gptp = 1, >> >> Mhm, I definitely don't like the way you "extend" the GbEthernet info >> structure. All the applicable flags should be set in the last patch of the >> series, not amidst of it. > > According to me, It is clearer with smaller patches like, what we have done with previous 2 patch sets for factorisation. > Please correct me, if any one have different opinion. I'm afraid you'd get a partly functioning device with the RZ/G2 info introduced amidst of the series and then the necessary flags/values added to it. This should definitely be avoided. > Regards, > Biju MBR, Sergey
Hi Sergei, > Subject: Re: [RFC/PATCH 05/18] ravb: Exclude gPTP feature support for > RZ/G2L > > On 9/23/21 10:13 PM, Biju Das wrote: > > [...] > >>> R-Car supports gPTP feature whereas RZ/G2L does not support it. > >>> This patch excludes gtp feature support for RZ/G2L by enabling > >>> no_gptp feature bit. > >>> > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > >>> --- > >>> drivers/net/ethernet/renesas/ravb_main.c | 46 > >>> ++++++++++++++---------- > >>> 1 file changed, 28 insertions(+), 18 deletions(-) > >>> > >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c > >>> b/drivers/net/ethernet/renesas/ravb_main.c > >>> index d38fc33a8e93..8663d83507a0 100644 > >>> --- a/drivers/net/ethernet/renesas/ravb_main.c > >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c > >> [...] > >>> @@ -953,7 +954,7 @@ static irqreturn_t ravb_interrupt(int irq, void > >> *dev_id) > >>> } > >>> > >>> /* gPTP interrupt status summary */ > >>> - if (iss & ISS_CGIS) { > >> > >> Isn't this bit always 0 on RZ/G2L? > > > > This CGIM bit(BIT13) which is present on R-Car Gen3 is not present in > > RZ/G2L. As per the HW manual > > BIT13 is reserved bit and read is always 0. > > > >> > >>> + if (!info->no_gptp && (iss & ISS_CGIS)) { > > Then extending this check doesn't seem necessary? > > >>> ravb_ptp_interrupt(ndev); > >>> result = IRQ_HANDLED; > >>> } > [...] > >>> @@ -2116,6 +2119,7 @@ static const struct ravb_hw_info rgeth_hw_info = > { > >>> .emac_init = ravb_rgeth_emac_init, > >>> .aligned_tx = 1, > >>> .tx_counters = 1, > >>> + .no_gptp = 1, > >> > >> Mhm, I definitely don't like the way you "extend" the GbEthernet > >> info structure. All the applicable flags should be set in the last > >> patch of the series, not amidst of it. > > > > According to me, It is clearer with smaller patches like, what we have > done with previous 2 patch sets for factorisation. > > Please correct me, if any one have different opinion. > > I'm afraid you'd get a partly functioning device with the RZ/G2 info > introduced amidst of the series and then the necessary flags/values added > to it. This should definitely be avoided. It is ok, It is understood, After replacing all the place holders only we get full functionality. That is the reason place holders added in first patch, so that we can fill each function at later stage By smaller patcher. Same case for feature bits. Regards, Biju
Hi Sergei, > Subject: RE: [RFC/PATCH 05/18] ravb: Exclude gPTP feature support for > RZ/G2L > > Hi Sergei, > > > Subject: Re: [RFC/PATCH 05/18] ravb: Exclude gPTP feature support for > > RZ/G2L > > > > On 9/23/21 10:13 PM, Biju Das wrote: > > > > [...] > > >>> R-Car supports gPTP feature whereas RZ/G2L does not support it. > > >>> This patch excludes gtp feature support for RZ/G2L by enabling > > >>> no_gptp feature bit. > > >>> > > >>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > >>> --- > > >>> drivers/net/ethernet/renesas/ravb_main.c | 46 > > >>> ++++++++++++++---------- > > >>> 1 file changed, 28 insertions(+), 18 deletions(-) > > >>> > > >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > >>> b/drivers/net/ethernet/renesas/ravb_main.c > > >>> index d38fc33a8e93..8663d83507a0 100644 > > >>> --- a/drivers/net/ethernet/renesas/ravb_main.c > > >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c > > >> [...] > > >>> @@ -953,7 +954,7 @@ static irqreturn_t ravb_interrupt(int irq, > > >>> void > > >> *dev_id) > > >>> } > > >>> > > >>> /* gPTP interrupt status summary */ > > >>> - if (iss & ISS_CGIS) { > > >> > > >> Isn't this bit always 0 on RZ/G2L? > > > > > > This CGIM bit(BIT13) which is present on R-Car Gen3 is not present > > > in RZ/G2L. As per the HW manual > > > BIT13 is reserved bit and read is always 0. > > > > > >> > > >>> + if (!info->no_gptp && (iss & ISS_CGIS)) { > > > > Then extending this check doesn't seem necessary? I have dropped this check in new version. > > > > >>> ravb_ptp_interrupt(ndev); > > >>> result = IRQ_HANDLED; > > >>> } > > [...] > > >>> @@ -2116,6 +2119,7 @@ static const struct ravb_hw_info > > >>> rgeth_hw_info = > > { > > >>> .emac_init = ravb_rgeth_emac_init, > > >>> .aligned_tx = 1, > > >>> .tx_counters = 1, > > >>> + .no_gptp = 1, > > >> > > >> Mhm, I definitely don't like the way you "extend" the GbEthernet > > >> info structure. All the applicable flags should be set in the last > > >> patch of the series, not amidst of it. > > > > > > According to me, It is clearer with smaller patches like, what we > > > have > > done with previous 2 patch sets for factorisation. > > > Please correct me, if any one have different opinion. > > > > I'm afraid you'd get a partly functioning device with the RZ/G2 > > info introduced amidst of the series and then the necessary > > flags/values added to it. This should definitely be avoided. > > It is ok, It is understood, After replacing all the place holders only we > get full functionality. > That is the reason place holders added in first patch, so that we can fill > each function at later stage By smaller patcher. Same case for feature > bits. > OK, the new patch excluded gPTP support for RZ/G2L and Also as per your suggestion,dropped timestamp feature bit and merged that code in this patch. Regards, Biju
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index d38fc33a8e93..8663d83507a0 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -918,6 +918,7 @@ static irqreturn_t ravb_interrupt(int irq, void *dev_id) { struct net_device *ndev = dev_id; struct ravb_private *priv = netdev_priv(ndev); + const struct ravb_hw_info *info = priv->info; irqreturn_t result = IRQ_NONE; u32 iss; @@ -953,7 +954,7 @@ static irqreturn_t ravb_interrupt(int irq, void *dev_id) } /* gPTP interrupt status summary */ - if (iss & ISS_CGIS) { + if (!info->no_gptp && (iss & ISS_CGIS)) { ravb_ptp_interrupt(ndev); result = IRQ_HANDLED; } @@ -1378,6 +1379,7 @@ static int ravb_get_ts_info(struct net_device *ndev, struct ethtool_ts_info *info) { struct ravb_private *priv = netdev_priv(ndev); + const struct ravb_hw_info *hw_info = priv->info; info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE | @@ -1391,7 +1393,8 @@ static int ravb_get_ts_info(struct net_device *ndev, (1 << HWTSTAMP_FILTER_NONE) | (1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) | (1 << HWTSTAMP_FILTER_ALL); - info->phc_index = ptp_clock_index(priv->ptp.clock); + if (!hw_info->no_gptp) + info->phc_index = ptp_clock_index(priv->ptp.clock); return 0; } @@ -2116,6 +2119,7 @@ static const struct ravb_hw_info rgeth_hw_info = { .emac_init = ravb_rgeth_emac_init, .aligned_tx = 1, .tx_counters = 1, + .no_gptp = 1, }; static const struct of_device_id ravb_match_table[] = { @@ -2159,13 +2163,15 @@ 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_gptp && !info->ccc_gac) { + if (info->no_gptp) { ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG); - /* Set CSEL value */ - ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB); - } else { + } else if (info->ccc_gac) { ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG | CCC_GAC | CCC_CSEL_HPB); + } else { + ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG); + /* Set CSEL value */ + ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB); } } @@ -2348,13 +2354,15 @@ static int ravb_probe(struct platform_device *pdev) /* Set AVB config mode */ ravb_set_config_mode(ndev); - /* Set GTI value */ - error = ravb_set_gti(ndev); - if (error) - goto out_disable_refclk; + if (!info->no_gptp) { + /* Set GTI value */ + error = ravb_set_gti(ndev); + if (error) + goto out_disable_refclk; - /* Request GTI loading */ - ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); + /* Request GTI loading */ + ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); + } if (info->internal_delay) { ravb_parse_delay_mode(np, ndev); @@ -2547,13 +2555,15 @@ static int __maybe_unused ravb_resume(struct device *dev) /* Set AVB config mode */ ravb_set_config_mode(ndev); - /* Set GTI value */ - ret = ravb_set_gti(ndev); - if (ret) - return ret; + if (!info->no_gptp) { + /* Set GTI value */ + ret = ravb_set_gti(ndev); + if (ret) + return ret; - /* Request GTI loading */ - ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); + /* Request GTI loading */ + ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI); + } if (info->internal_delay) ravb_set_delay_mode(ndev);
R-Car supports gPTP feature whereas RZ/G2L does not support it. This patch excludes gtp feature support for RZ/G2L by enabling no_gptp feature bit. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- drivers/net/ethernet/renesas/ravb_main.c | 46 ++++++++++++++---------- 1 file changed, 28 insertions(+), 18 deletions(-)