[PATCH net-next] 1588 support on bcm54210pe

[Lasse Johnsen <lasse@timebeat.app>]

Hi Andrew,

> On 20 Apr 2022, at 20:19, Andrew Lunn <andrew@lunn.ch> wrote:
> 
> 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.

I have read the documents, I understand about 10% of them and I am considering jumping off a tall building :-)

I’ve changed the subject of the email. How did I do?

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

Ok. I was asked by Florian to put the Broadcom maintainers in Cc so I will do this to begin with.

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

I am confident that this code is relevant exclusively to the BCM54210PE. It will not even work with the BCM54210, BCM54210S and BCM54210SE PHYs.

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

I am about to respond to Richard's mail with an amended set of patches which is much cleaner. checkpatch now complains only about a Signed-off line and asks if Maintainers needs updating because I’ve added a file (I guess it probably does). 

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

Is what you propose possible? Isn’t the issue here that the two PHYs (54213PE and 54210PE) present themselves with the same phy ID? If there is a way to seperate them then I will need your instruction on how to do it. 

> 
> 	Andrew

All the best,

Lasse