by Pavan Chebbi

[permalink] [raw]

Subject: Re: [Patch net-next v1 01/12] net: dsa: microchip: ptp: add the posix clock support

On Mon, Nov 28, 2022 at 4:03 PM Arun Ramadoss
<[email protected]> wrote:

> diff --git a/drivers/net/dsa/microchip/ksz_ptp_reg.h b/drivers/net/dsa/microchip/ksz_ptp_reg.h
> new file mode 100644
> index 000000000000..e578a0006ecf
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz_ptp_reg.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Microchip KSZ PTP register definitions
> + * Copyright (C) 2022 Microchip Technology Inc.
> + */
> +
> +#ifndef __KSZ_PTP_REGS_H
> +#define __KSZ_PTP_REGS_H
> +
> +/* 5 - PTP Clock */
> +#define REG_PTP_CLK_CTRL 0x0500
> +
> +#define PTP_STEP_ADJ BIT(6)
> +#define PTP_STEP_DIR BIT(5)
> +#define PTP_READ_TIME BIT(4)
> +#define PTP_LOAD_TIME BIT(3)

PTP_WRITE_TIME sounds more intuitive than PTP_LOAD_TIME?
Also I see that all the #defines are introduced in this patch, some of
which are used later. It is a good idea to introduce the #defines in
the same patches where they are being used for the first time.
I will be looking at the entire series but am responding to this now.

> +#define PTP_CLK_ADJ_ENABLE BIT(2)
> +#define PTP_CLK_ENABLE BIT(1)
> +#define PTP_CLK_RESET BIT(0)
> +
> +#define REG_PTP_RTC_SUB_NANOSEC__2 0x0502
> +
> +#define PTP_RTC_SUB_NANOSEC_M 0x0007
> +#define PTP_RTC_0NS 0x00
> +
> +#define REG_PTP_RTC_NANOSEC 0x0504
> +#define REG_PTP_RTC_NANOSEC_H 0x0504
> +#define REG_PTP_RTC_NANOSEC_L 0x0506
> +
> +#define REG_PTP_RTC_SEC 0x0508
> +#define REG_PTP_RTC_SEC_H 0x0508
> +#define REG_PTP_RTC_SEC_L 0x050A
> +
> +#define REG_PTP_SUBNANOSEC_RATE 0x050C
> +#define REG_PTP_SUBNANOSEC_RATE_H 0x050C
> +#define PTP_SUBNANOSEC_M 0x3FFFFFFF
> +
> +#define PTP_RATE_DIR BIT(31)
> +#define PTP_TMP_RATE_ENABLE BIT(30)
> +
> +#define REG_PTP_SUBNANOSEC_RATE_L 0x050E
> +
> +#define REG_PTP_RATE_DURATION 0x0510
> +#define REG_PTP_RATE_DURATION_H 0x0510
> +#define REG_PTP_RATE_DURATION_L 0x0512
> +
> +#define REG_PTP_MSG_CONF1 0x0514
> +
> +#define PTP_802_1AS BIT(7)
> +#define PTP_ENABLE BIT(6)
> +#define PTP_ETH_ENABLE BIT(5)
> +#define PTP_IPV4_UDP_ENABLE BIT(4)
> +#define PTP_IPV6_UDP_ENABLE BIT(3)
> +#define PTP_TC_P2P BIT(2)
> +#define PTP_MASTER BIT(1)
> +#define PTP_1STEP BIT(0)
> +
> +#endif
> --
> 2.36.1
>

Attachments:

smime.p7s (4.11 kB)
S/MIME Cryptographic Signature

2022-11-28 15:24:14

by Christian Eggers

Hi Pavan,

On Tue, 2022-11-29 at 14:23 +0530, Pavan Chebbi wrote:
> On Mon, Nov 28, 2022 at 4:05 PM Arun Ramadoss
> <[email protected]> wrote:
>
> > +static int ksz_ptp_enable(struct ptp_clock_info *ptp,
> > + struct ptp_clock_request *req, int on)
> > +{
> > + struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
> > + struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
> > + struct ptp_perout_request *request = &req->perout;
> > + int ret;
> > +
> > + switch (req->type) {
> > + case PTP_CLK_REQ_PEROUT:
> > + if (request->index > ptp->n_per_out)
> > + return -EINVAL;
>
> Should be -EOPNOTSUPP ? I see some other places where -EOPNOTSUPP is
> more appropriate.

I got a offline comment like This check is probably redundant (already
checked in period_store() and ptp_ioctl()). I am looking into whether
this check is required or already handled in upper layers.
If the check is required, then I feel -EINVAL/-ERANGE should be
reasonable. Because we are supporting periodic output only thing is
index is out of bound. If we return -EOPNOTSUPP, it indicates we are
not supporting periodic output.

>
> > +
> > + mutex_lock(&ptp_data->lock);
> > + ret = ksz_ptp_enable_perout(dev, request, on);
> > + mutex_unlock(&ptp_data->lock);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > /* Function is pointer to the do_aux_work in the ptp_clock
> > capability */
> > static long ksz_ptp_do_aux_work(struct ptp_clock_info *ptp)
> > {
> > @@ -508,6 +823,8 @@ static const struct ptp_clock_info ksz_ptp_caps
> > = {
> > .adjfine = ksz_ptp_adjfine,
> > .adjtime = ksz_ptp_adjtime,
> > .do_aux_work = ksz_ptp_do_aux_work,
> > + .enable = ksz_ptp_enable,
> > + .n_per_out = 3,
> > };
> >

2022-11-30 05:14:36

by Arun Ramadoss

[permalink] [raw]

Subject: Re: [Patch net-next v1 02/12] net: dsa: microchip: ptp: Initial hardware time stamping support

Hi Pavan,
Thanks for the comment.

On Tue, 2022-11-29 at 14:19 +0530, Pavan Chebbi wrote:
> On Mon, Nov 28, 2022 at 4:03 PM Arun Ramadoss
> <[email protected]> wrote:
> >
> > From: Christian Eggers <[email protected]>
> >
> > This patch adds the routine for get_ts_info, hwstamp_get, set. This
> > enables
> > the PTP support towards userspace applications such as linuxptp.
> > Tx timestamping can be enabled per port and Rx timestamping enabled
> > globally.
> >
> > Signed-off-by: Christian Eggers <[email protected]>
> > Co-developed-by: Arun Ramadoss <[email protected]>
> > Signed-off-by: Arun Ramadoss <[email protected]>
> >
> > ---
> > RFC v2 -> Patch v1
> > - moved tagger set and get function to separate patch
> > - Removed unnecessary comments
> > ---
> > drivers/net/dsa/microchip/ksz_common.c | 2 +
> > drivers/net/dsa/microchip/ksz_common.h | 4 ++
> > drivers/net/dsa/microchip/ksz_ptp.c | 77
> > +++++++++++++++++++++++++-
> > drivers/net/dsa/microchip/ksz_ptp.h | 14 +++++
> > 4 files changed, 95 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > b/drivers/net/dsa/microchip/ksz_common.c
> > index 2d09cd141db6..7b85b258270c 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.c
> > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > @@ -2873,6 +2873,8 @@ static const struct dsa_switch_ops
> > ksz_switch_ops = {
> > .port_change_mtu = ksz_change_mtu,
> > .port_max_mtu = ksz_max_mtu,
> > .get_ts_info = ksz_get_ts_info,
> > + .port_hwtstamp_get = ksz_hwtstamp_get,
> > + .port_hwtstamp_set = ksz_hwtstamp_set,
> > };
> >
> > struct ksz_device *ksz_switch_alloc(struct device *base, void
> > *priv)
> > diff --git a/drivers/net/dsa/microchip/ksz_common.h
> > b/drivers/net/dsa/microchip/ksz_common.h
> > index 5a6bfd42c6f9..cd20f39a565f 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.h
> > +++ b/drivers/net/dsa/microchip/ksz_common.h
> > @@ -103,6 +103,10 @@ struct ksz_port {
> > struct ksz_device *ksz_dev;
> > struct ksz_irq pirq;
> > u8 num;
> > +#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ_PTP)
> > + u8 hwts_tx_en;
> > + bool hwts_rx_en;
>
> I see that the hwts_rx_en gets removed in the later patch. Instead
> you
> could add rx filters support only later when you have the final code
> in place.

In RFC v2, this patch 2 and next patch 3 are in single patch. There are
two simple reasons, I had splitted them two. One is to have logical
commit and ease of code review. Patch 2 will add the hwtstamp_set and
get dsa hooks and patch 3 to mechanism to coordinate with the tag_ksz.c
to add additional 4 bytes in tail tag when PTP is enabled based on
tagger_data.
Second reason to split is to have patch authorship. I had used the
Christian Eggers patch and extending support for LAN937x. In the
initial series, it has dsa_port->priv variable to store the
ptp_shared_data. Now priv variable is removed from dsa_port so I used
tagger_data based on sja1105 implementation. As it is different from
intial patch series, moved to separate patch.

>
> > +#endif
> > };
> >
> >
> >
> >
> >
> > #endif
> > --
> > 2.36.1
> >

2022-11-30 05:15:05

by Arun Ramadoss

[permalink] [raw]

Subject: Re: [Patch net-next v1 11/12] net: dsa: microchip: ptp: add periodic output signal

On Tue, 2022-11-29 at 15:27 +0530, Pavan Chebbi wrote:
> On Tue, Nov 29, 2022 at 2:23 PM Pavan Chebbi <
> [email protected]> wrote:
> >
> > On Mon, Nov 28, 2022 at 4:05 PM Arun Ramadoss
> > <[email protected]> wrote:
> >
> > > +static int ksz_ptp_enable(struct ptp_clock_info *ptp,
> > > + struct ptp_clock_request *req, int on)
> > > +{
> > > + struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
> > > + struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
> > > + struct ptp_perout_request *request = &req->perout;
> > > + int ret;
> > > +
> > > + switch (req->type) {
> > > + case PTP_CLK_REQ_PEROUT:
> > > + if (request->index > ptp->n_per_out)
> > > + return -EINVAL;
> >
> > Should be -EOPNOTSUPP ? I see some other places where -EOPNOTSUPP
> > is
> > more appropriate.
> >
> > > +
> > > + mutex_lock(&ptp_data->lock);
> > > + ret = ksz_ptp_enable_perout(dev, request, on);
> > > + mutex_unlock(&ptp_data->lock);
> > > + break;
> > > + default:
> > > + return -EINVAL;
>
> OK I really meant here.

Ok. I will update -EINVAL to -EOPNOTSUPP.

>
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > /* Function is pointer to the do_aux_work in the ptp_clock
> > > capability */
> > > static long ksz_ptp_do_aux_work(struct ptp_clock_info *ptp)
> > > {
> > > @@ -508,6 +823,8 @@ static const struct ptp_clock_info
> > > ksz_ptp_caps = {
> > > .adjfine = ksz_ptp_adjfine,
> > > .adjtime = ksz_ptp_adjtime,
> > > .do_aux_work = ksz_ptp_do_aux_work,
> > > + .enable = ksz_ptp_enable,
> > > + .n_per_out = 3,
> > > };
> > >

2022-11-30 05:33:34

by Arun Ramadoss

[permalink] [raw]

Subject: Re: [Patch net-next v1 01/12] net: dsa: microchip: ptp: add the posix clock support

Hi Pavan,

On Mon, 2022-11-28 at 20:19 +0530, Pavan Chebbi wrote:
> On Mon, Nov 28, 2022 at 4:03 PM Arun Ramadoss
> <[email protected]> wrote:
>
> > diff --git a/drivers/net/dsa/microchip/ksz_ptp_reg.h
> > b/drivers/net/dsa/microchip/ksz_ptp_reg.h
> > new file mode 100644
> > index 000000000000..e578a0006ecf
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/ksz_ptp_reg.h
> > @@ -0,0 +1,57 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Microchip KSZ PTP register definitions
> > + * Copyright (C) 2022 Microchip Technology Inc.
> > + */
> > +
> > +#ifndef __KSZ_PTP_REGS_H
> > +#define __KSZ_PTP_REGS_H
> > +
> > +/* 5 - PTP Clock */
> > +#define REG_PTP_CLK_CTRL 0x0500
> > +
> > +#define PTP_STEP_ADJ BIT(6)
> > +#define PTP_STEP_DIR BIT(5)
> > +#define PTP_READ_TIME BIT(4)
> > +#define PTP_LOAD_TIME BIT(3)
>
> PTP_WRITE_TIME sounds more intuitive than PTP_LOAD_TIME?

To have similar naming convention between code and Datasheet it is
named as Load time.

> Also I see that all the #defines are introduced in this patch, some
> of
> which are used later. It is a good idea to introduce the #defines in
> the same patches where they are being used for the first time.
> I will be looking at the entire series but am responding to this now.

The patches are splitted into multiple smaller patches for review. I
missed to move header file defines to appropriate patches. Sure I will
move it.

>
> > +#define PTP_CLK_ADJ_ENABLE BIT(2)
> > +#define PTP_CLK_ENABLE BIT(1)
> > +#define PTP_CLK_RESET BIT(0)
> > +
> > +#define REG_PTP_RTC_SUB_NANOSEC__2 0x0502
> > +
> > +#define PTP_RTC_SUB_NANOSEC_M 0x0007
> > +#define PTP_RTC_0NS 0x00
> > +
> > +#define REG_PTP_RTC_NANOSEC 0x0504
> > +#define REG_PTP_RTC_NANOSEC_H 0x0504
> > +#define REG_PTP_RTC_NANOSEC_L 0x0506
> > +
> > +#define REG_PTP_RTC_SEC 0x0508
> > +#define REG_PTP_RTC_SEC_H 0x0508
> > +#define REG_PTP_RTC_SEC_L 0x050A
> > +
> >
> >
> > +#endif
> > --
> > 2.36.1
> >

2022-11-30 05:38:38

by Arun Ramadoss

[permalink] [raw]

Subject: Re: [Patch net-next v1 04/12] net: dsa: microchip: ptp: Manipulating absolute time using ptp hw clock

Hi Pavan,
Thanks for the review comment.

On Tue, 2022-11-29 at 14:13 +0530, Pavan Chebbi wrote:
> On Mon, Nov 28, 2022 at 4:04 PM Arun Ramadoss
> <[email protected]> wrote:
> > +/* Function is pointer to the do_aux_work in the ptp_clock
> > capability */
> > +static long ksz_ptp_do_aux_work(struct ptp_clock_info *ptp)
> > +{
> > + struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
> > + struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
> > + struct timespec64 ts;
> > +
> > + mutex_lock(&ptp_data->lock);
> > + _ksz_ptp_gettime(dev, &ts);
> > + mutex_unlock(&ptp_data->lock);
> > +
> > + spin_lock_bh(&ptp_data->clock_lock);
> > + ptp_data->clock_time = ts;
> > + spin_unlock_bh(&ptp_data->clock_lock);
>
> If I understand this correctly, the software clock is updated with
> full 64b every 1s. However only 32b timestamp registers are read
> while
> processing packets and higher bits from this clock are used.
> How do you ensure these higher order bits are in sync with the higher
> order bits in the HW? IOW, what if lower 32b have wrapped around and
> you are required to stamp a packet but you still don't have aux
> worker
> updated.

The Ptp Hardware Clock (PHC) seconds register is 32 bit wide. To have
register overflow it takes 4,294,967,296 seconds which is approximately
around 136 Years. So, it is bigger value and assume that we don't need
to care of PHC second register overflow.
For the packet timestamping, value is read from 32 bit register. This
register is splited into 2 bits seconds + 30 bits nanoseconds register.
In the ksz_tstamp_reconstruct function, lower 2 bits in the ptp_data-
>clock_time is cleared and 2 bits from the timestamp register are
added.

spin_lock_bh(&ptp_data->clock_lock);
ptp_clock_time = ptp_data->clock_time;
spin_unlock_bh(&ptp_data->clock_lock);

/* calculate full time from partial time stamp */
ts.tv_sec = (ptp_clock_time.tv_sec & ~3) | ts.tv_sec;

>
> > +
> > + return HZ; /* reschedule in 1 second */
> > +}
> > +
> > static int ksz_ptp_start_clock(struct ksz_device *dev)
> > {
> > - return ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE,
> > PTP_CLK_ENABLE);
> > + struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> > + int ret;
> > +
> > + ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE,
> > PTP_CLK_ENABLE);
> > + if (ret)
> > + return ret;
> > +
> > + spin_lock_bh(&ptp_data->clock_lock);
> > + ptp_data->clock_time.tv_sec = 0;
> > + ptp_data->clock_time.tv_nsec = 0;
> > + spin_unlock_bh(&ptp_data->clock_lock);
> > +
> > + return 0;
> > }
> >
> >
> > --
> > 2.36.1
> >

2022-11-30 06:33:38

Hi Vladimir,
Thanks for the review comment.

On Thu, 2022-12-01 at 03:04 +0200, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> On Mon, Nov 28, 2022 at 04:02:19PM +0530, Arun Ramadoss wrote:
> > diff --git a/drivers/net/dsa/microchip/ksz_ptp.c
> > b/drivers/net/dsa/microchip/ksz_ptp.c
> > index 184aa57a8489..415522ef4ce9 100644
> > --- a/drivers/net/dsa/microchip/ksz_ptp.c
> > +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> > @@ -200,6 +209,12 @@ static int ksz_ptp_settime(struct
> > ptp_clock_info *ptp,
> > goto error_return;
> >
> > ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_LOAD_TIME,
> > PTP_LOAD_TIME);
> > + if (ret)
> > + goto error_return;
> > +
> > + spin_lock_bh(&ptp_data->clock_lock);
>
> Why disable bottom halves? Where is the bottom half that this races
> with?

The interrupts are added in the following patches in the series. During
the deferred packet timestamping, partial timestamps are reconstructed
to absolute time using the ptp_data->clock_time in the bottom halves.
So we need this spin_lock_bh.

>
> > + ptp_data->clock_time = *ts;
> > + spin_unlock_bh(&ptp_data->clock_lock);
> >
> > error_return:
> > mutex_unlock(&ptp_data->lock);
> > }
> >
> > +/* Function is pointer to the do_aux_work in the ptp_clock
> > capability */
> > +static long ksz_ptp_do_aux_work(struct ptp_clock_info *ptp)
> > +{
> > + struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
> > + struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
> > + struct timespec64 ts;
> > +
> > + mutex_lock(&ptp_data->lock);
> > + _ksz_ptp_gettime(dev, &ts);
> > + mutex_unlock(&ptp_data->lock);
>
> Why don't you call ksz_ptp_gettime(ptp, &ts) directly?

I will use ksz_ptp_gettime directly.

>
> > +
> > + spin_lock_bh(&ptp_data->clock_lock);
> > + ptp_data->clock_time = ts;
> > + spin_unlock_bh(&ptp_data->clock_lock);
> > +
> > + return HZ; /* reschedule in 1 second */
> > +}
> > +
> > static int ksz_ptp_start_clock(struct ksz_device *dev)
> > {
> > - return ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE,
> > PTP_CLK_ENABLE);
> > + struct ksz_ptp_data *ptp_data = &dev->ptp_data;
> > + int ret;
> > +
> > + ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_CLK_ENABLE,
> > PTP_CLK_ENABLE);
> > + if (ret)
> > + return ret;
> > +
> > + spin_lock_bh(&ptp_data->clock_lock);
> > + ptp_data->clock_time.tv_sec = 0;
> > + ptp_data->clock_time.tv_nsec = 0;
> > + spin_unlock_bh(&ptp_data->clock_lock);
>
> Does ksz_ptp_start_clock() race with anything? The PTP clock has not
> even been registered by the time this has been called. This is
> literally
> an example of the "spin_lock_init(); spin_lock();" antipattern.

Yes, this function is called before PTP clock registeration. I will
remove the spin_lock for here.

>
> > +
> > + return 0;
> > }