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

[Sergey Shtylyov <sergei.shtylyov@gmail.com>]

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...

[...]> 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... :-)

> +	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...

> +	}
> +
> +	return 0;
> +}[...]

MBR, Sergey