Hi Sergei, Thanks for the feedback. > Subject: Re: [PATCH net-next 06/18] ravb: Factorise ptp feature > > HEllo! > > On 7/22/21 5:13 PM, Biju Das wrote: > > > Gptp is active in CONFIG mode for R-Car Gen3, where as it is not > > It's gPTP, the manuals say. :-) Ok. > > > active in CONFIG mode for R-Car Gen2. Add feature bits to handle both > > cases. > > I have no idea why this single diff requires 2 fetaure bits.... Basically this is a HW feature. 1) for R-Car Gen3, gPTP is active in config mode (R-Car Gen3) 2) for R-Car Gen2, gPTP is not active in config mode (R-Car Gen2) 3) RZ/G2L does not support ptp feature. > > > RZ/G2L does not support ptp feature. > > Ah, that explains it. :-) > It doesn't explain why we should bother with the 2nd bit in the same > patch tho... See above it is HW feature diff between R-Car Gen3 and R-Car Gen2. > > > Factorise ptp feature > > specific to R-Car. > > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> > > --- > > drivers/net/ethernet/renesas/ravb.h | 1 + > > drivers/net/ethernet/renesas/ravb_main.c | 81 > > ++++++++++++++++-------- > > 2 files changed, 56 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/net/ethernet/renesas/ravb.h > > b/drivers/net/ethernet/renesas/ravb.h > > index 0ed21262f26b..a474ed68db22 100644 > > --- a/drivers/net/ethernet/renesas/ravb.h > > +++ b/drivers/net/ethernet/renesas/ravb.h > > @@ -998,6 +998,7 @@ struct ravb_drv_data { > > size_t skb_sz; > > u8 num_tx_desc; > > enum ravb_chip_id chip_id; > > + u32 features; > > You didn't like bitfelds (in sh_eth) so much? :-) OK. I will change to bit fields, if there is no objection. > > [...] > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index 84ebd6fef711..e966b76df32c 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -40,6 +40,14 @@ > > NETIF_MSG_RX_ERR | \ > > NETIF_MSG_TX_ERR) > > > > +#define RAVB_PTP_CONFIG_ACTIVE BIT(0) > > +#define RAVB_PTP_CONFIG_INACTIVE BIT(1) > > If both bits are 0, it means GbEth? Yes that is correct. ptp is not supported in GbEth. > > > + > > +#define RAVB_PTP (RAVB_PTP_CONFIG_ACTIVE | > RAVB_PTP_CONFIG_INACTIVE) > > Hm? > > > + > > +#define RAVB_RCAR_GEN3_FEATURES RAVB_PTP_CONFIG_ACTIVE > > +#define RAVB_RCAR_GEN2_FEATURES RAVB_PTP_CONFIG_INACTIVE > > Not sure whtehr these are necessary... If there is no objection for using bit fields, then the above definitions not required. > > [...] > > } > > > > /* gPTP interrupt status summary */ > > - if (iss & ISS_CGIS) { > > + if ((info->features & RAVB_PTP) && (iss & ISS_CGIS)) { > > This is not a transparent change -- the fearture check came fromn > nownere... I have added commit statement to make it clear, "RZ/G2L does not support ptp feature. Factorise ptp feature specific to R-Car". Do you see any issues with this? If needed we can factorize this portion of the code again to make It simpler. First patch is ptp feature for r-car differences(R-Car Gen3 and R-Car Gen2) and second patch( with "transparent" comments you have mentioned in this patch) Please let me know. > > > ravb_ptp_interrupt(ndev); > > result = IRQ_HANDLED; > > } > [...] > > @@ -1275,7 +1286,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 (data->features & RAVB_PTP) > > Again, not transparent... See above. > > > + info->phc_index = ptp_clock_index(priv->ptp.clock); > > > > return 0; > > } > [...] > > @@ -1992,14 +2009,20 @@ static int ravb_set_gti(struct net_device > > *ndev) static void ravb_set_config_mode(struct net_device *ndev) { > > struct ravb_private *priv = netdev_priv(ndev); > > + const struct ravb_drv_data *info = priv->info; > > > > - if (priv->chip_id == RCAR_GEN2) { > > + switch (info->features & RAVB_PTP) { > > + case RAVB_PTP_CONFIG_INACTIVE: > > ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG); > > /* Set CSEL value */ > > ravb_modify(ndev, CCC, CCC_CSEL, CCC_CSEL_HPB); > > - } else { > > + break; > > + case RAVB_PTP_CONFIG_ACTIVE: > > ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG | > > CCC_GAC | CCC_CSEL_HPB); > > + break; > > + default: > > + ravb_modify(ndev, CCC, CCC_OPC, CCC_OPC_CONFIG); > Not trasparent again... See above. First Case is for R-Car Gen3 where ptp is active in config mode. Second Case is for R-Car Gen2 where ptp is not active in config mode. Third Case is default for RZ/G2L where ptp is not present. Do you see any issues with this? > > [...] > > @@ -2182,13 +2205,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->features & RAVB_PTP) { > > Not transparent enough yet again... See above. > > > + /* 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 (priv->chip_id != RCAR_GEN2) { > > ravb_parse_delay_mode(np, ndev); > [...] > > @@ -2377,13 +2404,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->features & RAVB_PTP) { > > Not transparent enough again... See above. Cheers, Biju