From: Andrew Lunn <andrew@lunn•ch>
To: Lasse Johnsen <lasse@timebeat•app>
Cc: Richard Cochran <richardcochran@gmail•com>,
netdev@vger•kernel.org,
Gordon Hollingworth <gordon@raspberrypi•com>,
Ahmad Byagowi <clk@fb•com>,
Florian Fainelli <f.fainelli@gmail•com>,
Heiner Kallweit <hkallweit1@gmail•com>,
Russell King <linux@armlinux•org.uk>,
bcm-kernel-feedback-list@broadcom•com,
"David S. Miller" <davem@davemloft•net>,
Jakub Kicinski <kuba@kernel•org>, Paolo Abeni <pabeni@redhat•com>
Subject: Re: [PATCH net-next v2] net: phy: broadcom: 1588 support on bcm54210pe
Date: Fri, 22 Apr 2022 17:42:42 +0200 [thread overview]
Message-ID: <YmLM8pFMVifHj7GG@lunn.ch> (raw)
In-Reply-To: <208820C3-E4C8-4B75-B926-15BCD844CE96@timebeat.app>
On Fri, Apr 22, 2022 at 04:08:18PM +0100, Lasse Johnsen wrote:
Hi Lasse
Don't attach a patch to the end of a discussion. What you email is
what comes out of git-format patch. Nothing added.
If you want to discuss review comments, just reply to the email with
the comments.
> From 3fcbbac9fe85909877f15d95f7a1e64dd6d06ab7 Mon Sep 17 00:00:00 2001
> From: Lasse Johnsen <l@ssejohnsen•me>
> Date: Sat, 5 Feb 2022 09:34:19 -0500
> Subject: [PATCH] Added support for IEEE1588 timestamping for the BCM54210PE
> PHY using the kernel mii_timestamper interface
>
> ---
> arch/arm/configs/bcm2711_defconfig | 1 +
> arch/arm64/configs/bcm2711_defconfig | 1 +
> .../net/ethernet/broadcom/genet/bcmgenet.c | 8 +-
> drivers/net/phy/Makefile | 1 +
> drivers/net/phy/bcm54210pe_ptp.c | 1382 +++++++++++++++++
> drivers/net/phy/bcm54210pe_ptp.h | 85 +
> drivers/net/phy/broadcom.c | 21 +-
> drivers/ptp/Kconfig | 17 +
You need to break this up into a patch series. You probably want the
following patches:
defconfig changes
The core ptp code, in library form
Extensions to drivers/net/phy/broadcom.c to use the new code
bcmgenet.c change.
> +static u64 four_u16_to_ns(u16 *four_u16)
> +{
> + u32 seconds;
> + u32 nanoseconds;
> + struct timespec64 ts;
> + u16 *ptr;
Now it has been through checkpatch, it is starting to look like Linux
code :-)
Network drivers have a few extra code style hoops to jump
through. Variables should be sorted longest to shortest. So you want:
> + struct timespec64 ts;
> + u32 nanoseconds;
> + u32 seconds;
> + u16 *ptr;
This is known as reverse Christmas tree.
> +static int bcm54210pe_interrupts_enable(struct phy_device *phydev, bool fsync_en, bool sop_en)
Although Linux as a whole allows 100 character lines, networking
mostly stays with 80. I'm not sure it is strictly enforced, but it is
a good idea to try to keep with it.
> +{
> + u16 interrupt_mask;
> +
> + interrupt_mask = 0;
You can combine these into one line.
> +static int bcm54210pe_get80bittime(struct bcm54210pe_private *private,
> + struct timespec64 *ts,
> + struct ptp_system_timestamp *sts)
> +{
> + struct phy_device *phydev;
> + u16 nco_6_register_value;
> + int i;
> + u64 time_stamp_48, time_stamp_80, control_ts;
> +
> + phydev = private->phydev;
> +
> + // Capture timestamp on next framesync
> + nco_6_register_value = 0x2000;
You should not have magic numbers. Please add a #define. If the
#define has a sensible name, it should then be obvious you are
capturing a timestamp on the next frame sync and so you don't need the
comment.
> +
> + // Lock
> + mutex_lock(&private->clock_lock);
Comments like this don't add any value. I can see it is a lock because
you are calling mutex_lock.
> +static int bcm54210pe_perout_enable(struct bcm54210pe_private *private, s64 period,
> + s64 pulsewidth, int enable)
> +{
> + struct phy_device *phydev;
> + u16 nco_6_register_value, frequency_hi, frequency_lo,
> + pulsewidth_reg, pulse_start_hi, pulse_start_lo;
> + int err;
> +
> + phydev = private->phydev;
> +
> + if (enable) {
> + frequency_hi = 0;
> + frequency_lo = 0;
> + pulsewidth_reg = 0;
> + pulse_start_hi = 0;
> + pulse_start_lo = 0;
> +
> + // Convert interval pulse spacing (period) and pulsewidth to 8 ns units
> + period /= 8;
> + pulsewidth /= 8;
> +
> + // Mode 2 only: If pulsewidth is not explicitly set with PTP_PEROUT_DUTY_CYCLE
> + if (pulsewidth == 0) {
> + if (period < 2500) {
> + // At a frequency at less than 20us (2500 x 8ns) set
> + // pulse length to 1/10th of the interval pulse spacing
> + pulsewidth = period / 10;
> +
> + // Where the interval pulse spacing is short,
> + // ensure we set a pulse length of 8ns
> + if (pulsewidth == 0)
> + pulsewidth = 1;
> +
> + } else {
> + // Otherwise set pulse with to 4us (8ns x 500 = 4us)
> + pulsewidth = 500;
> + }
> + }
> +
> + if (private->perout_mode == SYNC_OUT_MODE_1) {
> + // Set period
> + private->perout_period = period;
> +
> + if (!private->perout_en) {
> + // Set enable per_out
> + private->perout_en = true;
> + schedule_delayed_work(&private->perout_ws, msecs_to_jiffies(1));
> + }
> +
> + err = 0;
> +
> + } else if (private->perout_mode == SYNC_OUT_MODE_2) {
> + // Set enable per_out
> + private->perout_en = true;
> +
> + // Calculate registers
> +
> + // Lowest 16 bits of 8ns interval pulse spacing [15:0]
> + frequency_lo = (u16)period;
> +
> + // Highest 14 bits of 8ns interval pulse spacing [29:16]
> + frequency_hi = (u16)(0x3FFF & (period >> 16));
> +
> + // 2 lowest bits of 8ns pulse length [1:0]
> + frequency_hi |= (u16)pulsewidth << 14;
> +
> + // 7 highest bit of 8 ns pulse length [8:2]
> + pulsewidth_reg = (u16)(0x7F & (pulsewidth >> 2));
> +
> + // Get base value
> + nco_6_register_value = bcm54210pe_get_base_nco6_reg(private,
> + nco_6_register_value,
> + true);
> +
> + mutex_lock(&private->clock_lock);
> +
> + // Write to register
> + err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG,
> + nco_6_register_value);
> +
> + // Set sync out pulse interval spacing and pulse length
> + err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_3_0_REG, frequency_lo);
> + err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_3_1_REG, frequency_hi);
> + err |= bcm_phy_write_exp(phydev, NSE_DPPL_NCO_3_2_REG, pulsewidth_reg);
> +
> + // On next framesync load sync out frequency
> + err |= bcm_phy_write_exp(phydev, SHADOW_REG_LOAD, 0x0200);
> +
> + // Trigger immediate framesync
> + err |= bcm_phy_modify_exp(phydev, NSE_DPPL_NCO_6_REG, 0x003C, 0x0020);
> +
> + mutex_unlock(&private->clock_lock);
> + }
> + } else {
> + // Set disable pps
> + private->perout_en = false;
> +
> + // Get base value
> + nco_6_register_value = bcm54210pe_get_base_nco6_reg(private,
> + nco_6_register_value,
> + false);
> +
> + mutex_lock(&private->clock_lock);
> +
> + // Write to register
> + err = bcm_phy_write_exp(phydev, NSE_DPPL_NCO_6_REG, nco_6_register_value);
> +
> + mutex_unlock(&private->clock_lock);
> + }
> +
> + return err;
This is a pretty big function, and its indentation gets pretty deep. The coding style:
https://www.kernel.org/doc/html/latest/process/coding-style.html#functions
says:
Functions should be short and sweet, and do just one thing. They
should fit on one or two screenfuls of text (the ISO/ANSI screen size
is 80x24, as we all know), and do one thing and do that well.
Maybe you can break this up into helpers.
> +void bcm54210pe_txtstamp(struct mii_timestamper *mii_ts, struct sk_buff *skb, int type)
> +{
> + struct bcm54210pe_private *private = container_of(mii_ts, struct bcm54210pe_private,
> + mii_ts);
> +
> + switch (private->hwts_tx_en) {
> + case HWTSTAMP_TX_ON:
> + {
> + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> + skb_queue_tail(&private->tx_skb_queue, skb);
> + schedule_work(&private->txts_work);
> + break;
> + }
> +
> + case HWTSTAMP_TX_OFF:
> + {
> + }
That just looks odd.
Should there be a break? Do you want to fall through? If you do want
to fall through, you need to mark it so. But since it is empty, i
guess you don't want to fall through?
> +
> + default:
> + {
> + kfree_skb(skb);
> + break;
> + }
> + }
> +}
prev parent reply other threads:[~2022-04-22 15:43 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-20 14:03 Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface Lasse Johnsen
2022-04-20 19:19 ` Andrew Lunn
2022-04-21 17:32 ` Florian Fainelli
2022-04-22 14:21 ` [PATCH net-next] 1588 support on bcm54210pe Lasse Johnsen
2022-04-22 15:00 ` Andrew Lunn
2022-04-22 19:48 ` Richard Cochran
2022-04-23 14:40 ` Florian Fainelli
2022-04-23 18:16 ` Richard Cochran
2022-04-21 14:48 ` Support for IEEE1588 timestamping in the BCM54210PE PHY using the kernel mii_timestamper interface Richard Cochran
2022-04-22 15:08 ` [PATCH net-next v2] net: phy: broadcom: 1588 support on bcm54210pe Lasse Johnsen
2022-04-22 15:22 ` Jonathan Lemon
2022-04-22 18:11 ` Lasse Johnsen
2022-04-22 18:20 ` Jonathan Lemon
2022-04-23 10:26 ` Lasse Johnsen
2022-04-22 15:42 ` Andrew Lunn [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YmLM8pFMVifHj7GG@lunn.ch \
--to=andrew@lunn$(echo .)ch \
--cc=bcm-kernel-feedback-list@broadcom$(echo .)com \
--cc=clk@fb$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=f.fainelli@gmail$(echo .)com \
--cc=gordon@raspberrypi$(echo .)com \
--cc=hkallweit1@gmail$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=lasse@timebeat$(echo .)app \
--cc=linux@armlinux$(echo .)org.uk \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=richardcochran@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox