netdev - Re: [PATCH net-next] 1588 support on bcm54210pe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YmLC98NMfHUxwPF6@lunn.ch>
Date:   Fri, 22 Apr 2022 17:00:07 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Lasse Johnsen <lasse@...ebeat.app>
Cc:     netdev@...r.kernel.org, richardcochran@...il.com,
        Gordon Hollingworth <gordon@...pberrypi.com>,
        Ahmad Byagowi <clk@...com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        bcm-kernel-feedback-list@...adcom.com,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net-next] 1588 support on bcm54210pe

On Fri, Apr 22, 2022 at 03:21:16PM +0100, Lasse Johnsen wrote:
> Hi Andrew,
> 
> > On 20 Apr 2022, at 20:19, Andrew Lunn <andrew@...n.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.

Still not wrapped. Kernel developers tend to be old school, still
believe a terminal is 80 characters wide, and has 25 lines, just like
the VT100 they grew up with. It can be hard to get some email clients
to do this correctly, which is why most use mutt.

> > 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 :-)

As i said, it is a learning curve, but we are here to help.

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

I step in the right direction.

git log --oneline drivers/net/phy/broadcom.c
bf8bfc4336f7 net: phy: broadcom: Fix brcm_fet_config_init()
d15c7e875d44 net: phy: broadcom: hook up soft_reset for BCM54616S
72e78d22e152 net: phy: broadcom: Utilize appropriate suspend for BCM54810/11
38b6a9073007 net: phy: broadcom: Wire suspend/resume for BCM50610 and BCM50610M
d6da08ed1425 net: phy: broadcom: Add IDDQ-SR mode
8dc84dcd7f74 net: phy: broadcom: Enable 10BaseT DAC early wake
ad4e1e48a629 net: phy: broadcom: re-add check for PHY_BRCM_DIS_TXCRXC_NOENRGY on the BCM54811 PHY
5a32fcdb1e68 net: phy: broadcom: Add statistics for all Gigabit PHYs
b1dd9bf688b0 net: phy: broadcom: Fix RGMII delays for BCM50160 and BCM50610M

The prefix "net: phy: broadcom" helps get the right people to review
your patch. Florian will be looking for anything "broadcom". I look
for anything "phy".

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

There is a tool to help you get the correct people to send patches to:

./scripts/get_maintainer.pl <FILENAME>.patch

and it will give you something like:

Florian Fainelli <f.fainelli@...il.com> (supporter:BROADCOM ETHERNET PHY DRIVERS)
Andrew Lunn <andrew@...n.ch> (maintainer:ETHERNET PHY LIBRARY)
Heiner Kallweit <hkallweit1@...il.com> (maintainer:ETHERNET PHY LIBRARY)
Russell King <linux@...linux.org.uk> (reviewer:ETHERNET PHY LIBRARY)
"David S. Miller" <davem@...emloft.net> (maintainer:NETWORKING DRIVERS)
Jakub Kicinski <kuba@...nel.org> (maintainer:NETWORKING DRIVERS)
Paolo Abeni <pabeni@...hat.com> (maintainer:NETWORKING DRIVERS)
bcm-kernel-feedback-list@...adcom.com (open list:BROADCOM ETHERNET PHY DRIVERS)
netdev@...r.kernel.org (open list:BROADCOM ETHERNET PHY DRIVERS)
linux-kernel@...r.kernel.org (open list)

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

Florian can probably tell us more, but often hardware like this is
shared by multiple devices. If it is, you might want to use a more
generic prefix.

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

Signed-off-by is important. Without it, your patch will not get
accepted. Did a number of people write the code? You might need
Signed-off-by: from each of them, or you need to use
Co-Developed-by.

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Please try to use

git format-patch

and

git send-email

when sending your updated patches. It is a good idea to pass
--cover-letter to git format-patch, and give a 'big picture'
explanation in patch 0/X, along with a list of what you have changed
since the last version. Please also remember to put "v2: in the
subject, "patch net-next v2", so we can keep track of the different
versions.

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

Ah, they should not do that. There are solutions to this, but lets
leave this as is for the moment. Lets get other issues solved first.

      Andrew