Date:   Fri, 22 Apr 2022 16:08:18 +0100
From:   Lasse Johnsen <lasse@...ebeat.app>
To:     Richard Cochran <richardcochran@...il.com>
Cc:     netdev@...r.kernel.org,
        Gordon Hollingworth <gordon@...pberrypi.com>,
        Ahmad Byagowi <clk@...com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        bcm-kernel-feedback-list@...adcom.com,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: [PATCH net-next v2] net: phy: broadcom: 1588 support on bcm54210pe

Hi Richard,

Firstly, off topic, please allow me the indulgence this once. If it was not for your work, then I’m pretty sure my career would have looked significantly different. It’s a real privilege to interact with you. Respect… :-)

(To anyone not too familiar with the real world consequences of what accurate clock sync has provided, in finance as an example, Richard’s work prevents brokerages from front-running their clients, it’s a bulwark against fraud and is the foundation for numerous pieces of financial regulation protecting the average Joe all over the world).


> On 21 Apr 2022, at 15:48, Richard Cochran <richardcochran@...il.com> wrote:
> 
> On Wed, Apr 20, 2022 at 03:03:26PM +0100, Lasse Johnsen wrote:
> 
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> index bd1f419bc47ae..2fa6258103025 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> 
> This change is unrelated to the PHY driver and should be in its own
> patch series.
> 

Noted, I have everything in a single commit after I flattened 86 previous commits - what is a good way to do what you propose?

>> @@ -39,8 +39,11 @@
>> 
>> #include <asm/unaligned.h>
>> 
>> +#include <linux/ptp_classify.h>
>> +
>> #include "bcmgenet.h"
>> 
>> +
> 
> Don't add extra blank lines.  As Andrew commented, run your code
> through checkpatch.pl and fix the coding style.

I have done so in the new patches below.

> 
>> /* Maximum number of hardware queues, downsized if needed */
>> #define GENET_MAX_MQ_CNT	4
>> 
>> @@ -2096,7 +2099,18 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
>> 	}
>> 
>> 	GENET_CB(skb)->last_cb = tx_cb_ptr;
>> -	skb_tx_timestamp(skb);
>> +
>> +	// Timestamping
>> +	if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
>> +	{
>> +		//skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>> +		skb_pull(skb, skb_mac_offset(skb)); // Feels like this pull should really be part of ptp_classify_raw...
>> +		skb_clone_tx_timestamp(skb);
>> +	}
>> +	else if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_SW_TSTAMP))
>> +	{
>> +		skb_tstamp_tx(skb, NULL);
>> +	}
> 
> This is totally wrong.  skb_tx_timestamp() is the correct MAC driver API.

Thank you. What is a good if condition in the instance then please? In the new patches I have gone with:

(unlikely(skb_shinfo(skb)->tx_flags & (SKBTX_HW_TSTAMP | SKBTX_SW_TSTAMP)) > 0)


> 
>> 
>> 	/* Decrement total BD count and advance our write pointer */
>> 	ring->free_bds -= nr_frags + 1;
>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>> index a13e402074cf8..528192d59d793 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -46,6 +46,7 @@ obj-$(CONFIG_BCM84881_PHY)	+= bcm84881.o
>> obj-$(CONFIG_BCM87XX_PHY)	+= bcm87xx.o
>> obj-$(CONFIG_BCM_CYGNUS_PHY)	+= bcm-cygnus.o
>> obj-$(CONFIG_BCM_NET_PHYLIB)	+= bcm-phy-lib.o
>> +obj-$(CONFIG_BCM54120PE_PHY)	+= bcm54210pe_ptp.o
>> obj-$(CONFIG_BROADCOM_PHY)	+= broadcom.o
>> obj-$(CONFIG_CICADA_PHY)	+= cicada.o
>> obj-$(CONFIG_CORTINA_PHY)	+= cortina.o
> 
>> diff --git a/drivers/net/phy/bcm54210pe_ptp.c b/drivers/net/phy/bcm54210pe_ptp.c
>> new file mode 100755
>> index 0000000000000..c4882c84229f9
>> --- /dev/null
>> +++ b/drivers/net/phy/bcm54210pe_ptp.c
>> @@ -0,0 +1,1406 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + *  drivers/net/phy/bcm54210pe_ptp.c
>> + *
>> + * IEEE1588 (PTP), perout and extts for BCM54210PE PHY
>> + *
>> + * Authors: Carlos Fernandez, Kyle Judd, Lasse Johnsen
>> + * License: GPL
> 
> Use the proper MODULE macros for this info.

I have added the module macros in the new patches.

> 
>> + */
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/ip.h>                                                                                
>> +#include <linux/net_tstamp.h>
>> +#include <linux/mii.h>
>> +#include <linux/phy.h>                                                                               
>> +#include <linux/ptp_classify.h>
>> +#include <linux/ptp_clock_kernel.h>                                                                  
>> +#include <linux/udp.h>
>> +#include <asm/unaligned.h> 
>> +#include <linux/brcmphy.h>
>> +#include <linux/irq.h>
>> +#include <linux/workqueue.h>
>> +#include <linux/gpio.h>
>> +#include <linux/if_ether.h>
>> +#include <linux/delay.h>
>> +#include <linux/sched.h>
> 
> Suggest keeping include directives in alphabetical order.

Noted, and included in new version of patches.

> 
>> +
>> +#include "bcm54210pe_ptp.h"
>> +#include "bcm-phy-lib.h"
>> +
>> +#define PTP_CONTROL_OFFSET		32
>> +#define PTP_TSMT_OFFSET 		0
>> +#define PTP_SEQUENCE_ID_OFFSET		30
>> +#define PTP_CLOCK_ID_OFFSET		20
>> +#define PTP_CLOCK_ID_SIZE		8
>> +#define PTP_SEQUENCE_PORT_NUMER_OFFSET  (PTP_CLOCK_ID_OFFSET + PTP_CLOCK_ID_SIZE)
>> +
>> +#define EXT_SELECT_REG			0x17
>> +#define EXT_DATA_REG			0x15
>> +
>> +#define EXT_ENABLE_REG1			0x17
>> +#define EXT_ENABLE_DATA1		0x0F7E
>> +#define EXT_ENABLE_REG2			0x15
>> +#define EXT_ENABLE_DATA2		0x0000
>> +
>> +#define EXT_1588_SLICE_REG		0x0810
>> +#define EXT_1588_SLICE_DATA		0x0101
> 
> EXT_1588_SLICE_DATA is not used anywhere.

Noted and removed. EXT_SELECT_REG removed for same reason.

>> +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;
>> +	}
>> +
>> +	list_for_each_safe(this, next, &private->circular_buffers[index])
>> +	{
>> +		item = list_entry(this, struct bcm54210pe_circular_buffer_item, list);
>> +
>> +		if(item->sequence_id == seq_id && item->is_valid)
>> +		{
>> +			item->is_valid = false;
> 
> Instead of using this flag, just remove matched items from list.

It’s a circular buffer of a predefined size. I could be wrong, but does this not preclude what you suggest? If not, I would humbly ask for guidance.

>> +static void bcm54210pe_read_sop_time_register(struct bcm54210pe_private *private)
>> +{
>> +	struct phy_device *phydev = private->phydev;
>> +	struct bcm54210pe_circular_buffer_item *item;
>> +	u16 fifo_info_1, fifo_info_2;
>> +	u8 tx_or_rx, msg_type, index;
>> +	u16 sequence_id;
>> +	u64 timestamp;
>> +	u16 Time[4];
>> +
>> +	mutex_lock(&private->timestamp_buffer_lock);
>> +
>> +	while(bcm_phy_read_exp(phydev, INTERRUPT_STATUS_REG) & 2)
> 
> Replace magic number 2 proper macro.

The magic is gone and replaced with INTC_SOP. INTC_FSYNC has also been defined in the header file in preparation for future interrupt driven behaviour. It is currently unused. If this is not agreeable I can remove. 

> 
> Also, this loop is potentially infinite.  Add a sanity check to break out.

I have added an iteration check to prevent this.

>> +	while (skb != NULL) {
>> +
>> +		// Yes....  skb_defer_rx_timestamp just did this but <ZZZzzz>....
>> +		skb_push(skb, ETH_HLEN);
>> +		type = ptp_classify_raw(skb);
>> +		skb_pull(skb, ETH_HLEN);
>> +
>> +		hdr = ptp_parse_header(skb, type);
>> +
>> +		if (hdr == NULL) {
>> +			goto dequeue;
>> +		}
>> +
>> +		msg_type = ptp_get_msgtype(hdr, type);
>> +		sequence_id = be16_to_cpu(hdr->sequence_id);
>> +
>> +		timestamp = 0;
>> +
>> +		for (x = 0; x < 10; x++) {
> 
> Ten times? and ...

If we have not matched the timestamp in the PHY fifo by then then a timestamp of 0 is returned. This strikes me as reasonable behaviour, but I can omit returning the packet in the err queue completely if that is the desired behaviour. I will rely on your guidance for the desired behaviour.

> 
>> +			bcm54210pe_read_sop_time_register(private);
>> +			if (bcm54210pe_fetch_timestamp(0, msg_type, sequence_id,
>> +						       private, &timestamp)) {
>> +				break;
>> +			}
>> +
>> +			udelay(private->fib_sequence[x] *
>> +			       private->fib_factor_rx);
> 
> with a cute udelay?  No, use a proper deferral mechanism.

Hehe… The proposed method strikes me as cheap and reasonable in the instance. I am happy to change, but will you propose a satisfactory deferral mechanism in the instance? I just don’t have sufficient kernel experience to identify the best approach and your input would be much appreciated. 

I have read this: https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

I think on the rx side it is unlikely that we get to the udelay as that timestamp “should be”(tm) available on the first iteration. On the corresponding tx side the amount of udelaying is also minimal, but again, I will take the direction you give me.  

>> +static int bcm54210pe_config_1588(struct phy_device *phydev)
>> +{
>> +	int err;
>> +
>> +	err = bcm_phy_write_exp(phydev, PTP_INTERRUPT_REG, 0x3c02 );
>> +
>> +	err |=  bcm_phy_write_exp(phydev, GLOBAL_TIMESYNC_REG, 0x0001); //Enable global timesync register.
>> +	err |=  bcm_phy_write_exp(phydev, EXT_1588_SLICE_REG, 0x0101); //ENABLE TX and RX slice 1588
> 
> Does this device support multiple ports?  If so, the driver should
> support that.

It does not no. It is a single port PHY.

>> 
>> +bool bcm54210pe_rxtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type)
>> +{
>> +	struct bcm54210pe_private *private = container_of(mii_ts, struct bcm54210pe_private, mii_ts);
>> +
>> +	if (private->hwts_rx_en) {
>> +		skb_queue_tail(&private->rx_skb_queue, skb);
> 
> This is clunky.  The time stamp item may already be in the list.  Code
> should check the list and deliver the skb immediately on match.  Queue
> the skb only when time stamp not ready yet.
> 

No, that strikes me as unlikely both when acting as a source or consumer of PTP. The updating of the list is triggered by an tx or rx frame event. So in order for an rx timestamp to be already available, a tx event must have occurred at more or less the exact same time. This is not impossible, but probably sufficiently improbable to warrant the check you suggest. However, your experience far outweighs mine and I can modify to implement the behaviour you suggest if you insist.

> It appears that time stamps generate an interrupt, no?  If so, then
> why not use the ISR to trigger reading of time stamps?
> 

No, as I indicated in my email we trigger poll style behaviour on the receipt or transmit of 1588 frames. This is not ideal, and I hope to change in the future time permitting, but in the current iteration, that is the behaviour and it performs well.

> See dp83640.c for an example of how to handle asynchronous Rx time stamps.
> 
> Moreover: Does this device provide in-band Rx time stamps?  If so, why
> not use them?

This is the first generation PHY and it does not do in-band RX. I asked BCM and studied the documentation. I’m sure I’m allowed to say, that the second generation 40nm BCM PHY (which - "I am not making this up" is available in 3 versions: BCM54210, BCM54210S and BCM54210SE - not “PE”) - supports in-band rx timestamps. However, as a matter of curiosity, BCM utilise the field in the header now used for minor versioning in 1588-2019, so in due course using this silicon feature will be a significant challenge.

>> +	// Initialisation of work_structs and similar
>> +	INIT_WORK(&bcm54210pe->txts_work, bcm54210pe_run_tx_timestamp_match_thread);
>> +	INIT_WORK(&bcm54210pe->rxts_work, bcm54210pe_run_rx_timestamp_match_thread);
>> +	INIT_DELAYED_WORK(&bcm54210pe->perout_ws, bcm54210pe_run_perout_mode_one_thread);
>> +	INIT_DELAYED_WORK(&bcm54210pe->extts_ws, bcm54210pe_run_extts_thread);
> 
> Don't use generic work.  Instead, implement ptp_clock_info::do_aux_work()

No periodic work is undertaken. Rather rx and tx frames schedule work. With this in mind are you confident your recommendation is sound? If so I might need additional instruction on how to implement.

> 
> That way, you get a kthread that may be given appropriate scheduling priority.

Given the currently scheme if you do not propose to change it, can I give the theads an alternative priority such as it is?

>> 
>> +static u64 ts_to_ns(struct timespec64 *ts)
>> +{
>> +	return ((u64)ts->tv_sec * (u64)1000000000) + ts->tv_nsec;
>> +}
> 
> Use timespec64_to_ns()

Thank you. Noted with thanks and updated.

> 
>> +static void ns_to_ts(u64 time_stamp, struct timespec64 *ts)
>> +{
>> +	ts->tv_sec  = ( (u64)time_stamp / (u64)1000000000 );
>> +	ts->tv_nsec = ( (u64)time_stamp % (u64)1000000000 );
>> +}
> 
> Use ns_to_timespec64()

Also noted with thanks and updated.

>> 
>> +static bool bcm54210pe_rxtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type);
>> +static void bcm54210pe_txtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type);
>> +static void bcm54210pe_run_rx_timestamp_match_thread(struct work_struct *w);
>> +static void bcm54210pe_run_tx_timestamp_match_thread(struct work_struct *w);
>> +static void bcm54210pe_read_sop_time_register(struct bcm54210pe_private *private);
>> +static bool bcm54210pe_fetch_timestamp(u8 txrx, u8 message_type, u16 seq_id, struct bcm54210pe_private *private, u64 *timestamp);
>> +
>> +static u16  bcm54210pe_get_base_nco6_reg(struct bcm54210pe_private *private, u16 val, bool do_nse_init);
>> +static int  bcm54210pe_interrupts_enable(struct phy_device *phydev, bool fsync_en, bool sop_en);
>> +static int  bcm54210pe_gettimex(struct ptp_clock_info *info, struct timespec64 *ts, struct ptp_system_timestamp *sts);
>> +static int  bcm54210pe_get80bittime(struct bcm54210pe_private *private, struct timespec64 *ts, struct ptp_system_timestamp *sts);
>> +static int  bcm54210pe_get48bittime(struct bcm54210pe_private *private, u64 *time_stamp);
>> +static void bcm54210pe_read80bittime_register(struct phy_device *phydev, u64 *time_stamp_80, u64 *time_stamp_48);
>> +static void bcm54210pe_read48bittime_register(struct phy_device *phydev, u64 *time_stamp);
>> +
>> +static int  bcm54210pe_perout_enable(struct bcm54210pe_private *private, s64 period, s64 pulsewidth, int on);
>> +static void bcm54210pe_run_perout_mode_one_thread(struct work_struct *perout_ws);
>> +
>> +static int  bcm54210pe_extts_enable(struct bcm54210pe_private *private, int enable);
>> +static void bcm54210pe_run_extts_thread(struct work_struct *extts_ws);
>> +static void bcm54210pe_trigger_extts_event(struct bcm54210pe_private *private, u64 timestamp);
>> +
>> +static u64  convert_48bit_to_80bit(u64 second_on_set, u64 ts);
>> +static u64  four_u16_to_ns(u16 *four_u16);
>> +static u64  ts_to_ns(struct timespec64 *ts);
>> +static void ns_to_ts(u64 time_stamp, struct timespec64 *ts);
> 
> Never put "static" prototypes into a header file.
> 
> Avoid static prototypes altogether.  Instead, order the functions
> within the source file so that sub-functions appear before their call
> sites.

Noted with thanks and updated.




View attachment "bcm54210pe-1588.v2.patch.txt" of type "text/plain" (47034 bytes)

Powered by blists - more mailing lists