Re: Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface

[Andrew Lunn <andrew@lunn.ch>]

On Wed, Apr 20, 2022 at 03:03:26PM +0100, Lasse Johnsen wrote:
> Hello,
> 
> 
> The attached set of patches adds support for the IEEE1588 functionality on the BCM54210PE PHY using the Linux Kernel mii_timestamper interface. The BCM54210PE PHY can be found in the Raspberry PI Compute Module 4 and the work has been undertaken by Timebeat.app on behalf of Raspberry PI with help and support from the nice engineers at Broadcom.

Hi Lasse

There are a few process issues you should address.

Please wrap your email at about 80 characters.

Please take a read of

https://www.kernel.org/doc/html/latest/process/submitting-patches.html

and

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq

It is a bit of a learning curve getting patches accepted, and you have
to follow the processes defined in these documents.

>  arch/arm/configs/bcm2711_defconfig            |    1 +
>  arch/arm64/configs/bcm2711_defconfig          |    1 +

You will need to split these changes up. defconfg changes go via the
Broadcom maintainers. PHY driver changes go via netdev. You can
initially post them as a series, but in the end you might need to send
them to different people/lists.

> +obj-$(CONFIG_BCM54120PE_PHY)	+= bcm54210pe_ptp.o

How specific is this code to the bcm54210pe? Should it work for any
bcm54xxx PHY? You might want to name this file broadcom_ptp.c if it
will work with any PHY supported by broadcom.c.

> +static bool bcm54210pe_fetch_timestamp(u8 txrx, u8 message_type, u16 seq_id, struct bcm54210pe_private *private, u64 *timestamp)
> +{
> +	struct bcm54210pe_circular_buffer_item *item; 
> +	struct list_head *this, *next;
> +
> +	u8 index = (txrx * 4) + message_type;
> +
> +	if(index >= CIRCULAR_BUFFER_COUNT)
> +	{
> +		return false;
> +	}

Please run your code through ./scripts/checkpatch.pl. You will find
the code has a number of code style issues which need cleaning up.

> +#if IS_ENABLED (CONFIG_BCM54120PE_PHY)
> +{
> +	.phy_id		= PHY_ID_BCM54213PE,
> +	.phy_id_mask	= 0xffffffff,
> +        .name           = "Broadcom BCM54210PE",
> +        /* PHY_GBIT_FEATURES */
> +        .config_init    = bcm54xx_config_init,
> +        .ack_interrupt  = bcm_phy_ack_intr,
> +        .config_intr    = bcm_phy_config_intr,
> +	.probe		= bcm54210pe_probe,
> +#elif
> +{ 
>  	.phy_id		= PHY_ID_BCM54213PE,
>  	.phy_id_mask	= 0xffffffff,
>  	.name		= "Broadcom BCM54213PE",
> @@ -786,6 +804,7 @@ static struct phy_driver broadcom_drivers[] = {
>  	.config_init	= bcm54xx_config_init,
>  	.ack_interrupt	= bcm_phy_ack_intr,
>  	.config_intr	= bcm_phy_config_intr,
> +#endif

Don't replace the existing entry, extend it with your new
functionality.

	Andrew