Re: [PATCH net-next v2 10/10] net: ethernet: ravb: Add gPTP support for Gen4

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Sergey,

Thanks for your feedback.

On 2026-08-15 22:12:03 +0300, Sergey Shtylyov wrote:
> On 8/11/26 7:02 PM, Niklas Söderlund wrote:
> 
> > While driver advertises gPTP support on Gen4 platforms it is in fact
> > completely broken. On R-Car Gen4 devices the RAVB module have no
> > internal gPTP clock as generations before it. Instead it utilizes a
> > system-wide gPTP clock.
> > 
> > This change utilizes the refactoring of the RAVB gPTP code to add
> > support for a system-wide clock and stops the Gen4 devices trying to use
> > the non-existing internal gPTP clock.
> > 
> > To remain backward compatible the ptp-timer property is optional.If not
> > present, or not enabled, the RAVB driver will no longer advertise gPTP
> > support to user-space.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Sergey Shtylyov <sergei.shtylyov@gmail.com>
> [...]
> 
> > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> > index b4df5cab0e5c..4c8dc02022e2 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -249,6 +249,8 @@ enum APSR_BIT {
> >  	APSR_RDM	= 0x00002000,
> >  	APSR_TDM	= 0x00004000,
> >  	APSR_MIISELECT	= 0x01000000,	/* R-Car V4M only */
> > +	APSR_GPTPTIMER_SOURCE = BIT(25), /* Gen4 */
> > +	APSR_GPTPCLOCK	= BIT(29),	/* Gen4 */
> 
>    Again, I don't quite like that you switched to using BIT() all of a sudden...

Indeed, the while enum should really be converted to avoid LLM warnings.  
But doing this in this already large and way overdue series feels a bit 
too much. Once this is picked up I plan to convert them all to BIT macro 
to make life easier for the future.

> 
> [...]> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index 12877a8488ed..30170e92d3d7 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -2752,6 +2752,59 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
> >  	.magic_pkt = 1,
> >  };
> >  
> > +static int ravb_gen4_ptp_probe(struct net_device *ndev)
> > +{
> > +	struct ravb_private *priv = netdev_priv(ndev);
> > +
> > +	priv->of_gptp =
> > +		of_parse_phandle(priv->pdev->dev.of_node, "ptp-timer", 0);
> 
>    You now have full 100 columns, no need to break that line... :-)

Fixed in next version.

> 
> > +	if (!priv->of_gptp)
> > +		return 0;
> > +
> > +	if (!of_device_is_available(priv->of_gptp)) {
> > +		of_node_put(priv->of_gptp);
> > +		priv->of_gptp = NULL;
> > +		return 0;
> 
>    This *return* seems superfluous...

Thanks, fixed.

> 
> > +	}
> > +
> > +	return 0;
> > +}[...]
> 
> MBR, Sergey
> 

-- 
Kind Regards,
Niklas Söderlund