Message-ID: <a3bb90c4780d491a7194bbf9cea775be5011c45e.camel@microchip.com>
Date:   Thu, 1 Dec 2022 10:01:48 +0000
From:   <Arun.Ramadoss@...rochip.com>
To:     <olteanv@...il.com>
CC:     <andrew@...n.ch>, <linux-kernel@...r.kernel.org>,
        <UNGLinuxDriver@...rochip.com>, <vivien.didelot@...il.com>,
        <linux@...linux.org.uk>, <ceggers@...i.de>,
        <Tristram.Ha@...rochip.com>, <f.fainelli@...il.com>,
        <kuba@...nel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>,
        <richardcochran@...il.com>, <netdev@...r.kernel.org>,
        <Woojung.Huh@...rochip.com>, <davem@...emloft.net>
Subject: Re: [Patch net-next v1 01/12] net: dsa: microchip: ptp: add the posix
 clock support

Hi Vladimir,

On Thu, 2022-12-01 at 02:17 +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:16PM +0530, Arun Ramadoss wrote:
> > From: Christian Eggers <ceggers@...i.de>
> > 
> > This patch implement routines (adjfine, adjtime, gettime and
> > settime)
> > for manipulating the chip's PTP clock. It registers the ptp caps
> > to posix clock register.
> > 
> > Signed-off-by: Christian Eggers <ceggers@...i.de>
> > Co-developed-by: Arun Ramadoss <arun.ramadoss@...rochip.com>
> > Signed-off-by: Arun Ramadoss <arun.ramadoss@...rochip.com>
> > 
> > ---
> > RFC v2 -> Patch v1
> > - Repharsed the Kconfig help text
> > - Removed IS_ERR_OR_NULL check in ptp_clock_unregister
> > - Add the check for ptp_data->clock in ksz_ptp_ts_info
> > - Renamed MAX_DRIFT_CORR to KSZ_MAX_DRIFT_CORR
> > - Removed the comments
> > - Variables declaration in reverse christmas tree
> > - Added the ptp_clock_optional
> > ---
> > diff --git a/drivers/net/dsa/microchip/ksz_common.h
> > b/drivers/net/dsa/microchip/ksz_common.h
> > index c6726cbd5465..5a6bfd42c6f9 100644
> > --- a/drivers/net/dsa/microchip/ksz_common.h
> > +++ b/drivers/net/dsa/microchip/ksz_common.h
> > @@ -444,6 +447,19 @@ static inline int ksz_write32(struct
> > ksz_device *dev, u32 reg, u32 value)
> >       return ret;
> >  }
> > 
> > +static inline int ksz_rmw16(struct ksz_device *dev, u32 reg, u16
> > mask,
> > +                         u16 value)
> > +{
> > +     int ret;
> > +
> > +     ret = regmap_update_bits(dev->regmap[1], reg, mask, value);
> > +     if (ret)
> > +             dev_err(dev->dev, "can't rmw 16bit reg: 0x%x %pe\n",
> > reg,
> > +                     ERR_PTR(ret));
> 
> Is the colon misplaced? What do you want to say, "can't rmw 16bit
> reg: 0x0 -EIO",
> or "can't rmw 16bit reg 0x0: -EIO"?
> 
> Reminds me of a joke:
> "The inventor of the Oxford comma has died. Tributes have been led by
> J.K. Rowling, his wife and the Queen of England".

Its a copy paste problem. I reused the exisiting inline functions based
on patch *net: dsa: microchip: add support for regmap_access_tables*. 
I will move the semicolon after 0x%x:

> 
> > +
> > +     return ret;
> > +}
> > +
> >  static inline int ksz_write64(struct ksz_device *dev, u32 reg, u64
> > value)
> >  {
> >       u32 val[2];
> > +static int ksz_ptp_settime(struct ptp_clock_info *ptp,
> > +                        const struct timespec64 *ts)
> > +{
> > +     struct ksz_ptp_data *ptp_data = ptp_caps_to_data(ptp);
> > +     struct ksz_device *dev = ptp_data_to_ksz_dev(ptp_data);
> > +     int ret;
> > +
> > +     mutex_lock(&ptp_data->lock);
> > +
> > +     /* Write to shadow registers and Load PTP clock */
> > +     ret = ksz_write16(dev, REG_PTP_RTC_SUB_NANOSEC__2,
> > PTP_RTC_0NS);
> > +     if (ret)
> > +             goto error_return;
> > +
> > +     ret = ksz_write32(dev, REG_PTP_RTC_NANOSEC, ts->tv_nsec);
> > +     if (ret)
> > +             goto error_return;
> > +
> +     ret = ksz_write32(dev, REG_PTP_RTC_SEC, ts->tv_sec);
> > +     if (ret)
> > +             goto error_return;
> > +
> > +     ret = ksz_rmw16(dev, REG_PTP_CLK_CTRL, PTP_LOAD_TIME,
> > PTP_LOAD_TIME);
> > +
> > +error_return:
> 
> I would avoid naming labels with "error_", if the success code path
> is
> also going to run through the code they point to. "goto unlock"
> sounds
> about right.

Ok. I will rename the goto block.

> 
> > +     mutex_unlock(&ptp_data->lock);
> > +
> > +     return ret;
> > +}
> > +
> > +static const struct ptp_clock_info ksz_ptp_caps = {
> > +     .owner          = THIS_MODULE,
> > +     .name           = "Microchip Clock",
> > +     .max_adj        = KSZ_MAX_DRIFT_CORR,
> > +     .gettime64      = ksz_ptp_gettime,
> > +     .settime64      = ksz_ptp_settime,
> > +     .adjfine        = ksz_ptp_adjfine,
> > +     .adjtime        = ksz_ptp_adjtime,
> > +};
> 
> Is it a conscious decision to have this structure declared here in
> the
> .rodata section (I think that's where this goes?), when it will only
> be
> used as a blueprint for the implicit memcpy (struct assignment) in
> ksz_ptp_clock_register()?

To reduce number of line in the ksz_ptp_clock_register(), I moved the
structure intialization outside of function. Referred other dsa
implementation found this type in
drivers/net/dsa/ocelot/felix_vsc9959.c, just reused it.
I didn't thought of .rodata section and memcpy overhead.

I will move this initialization within ksz_ptp_clock_register.

> 
> Just saying that it would be possible to initialize the fields in
> ptp_data->caps even without resorting to declaring one extra
> structure,
> which consumes space. I'll leave you alone if you ACK that you know
> your
> assignment below is a struct copy and not a pointer assignment.
> 
> > +
> > +int ksz_ptp_clock_register(struct dsa_switch *ds)
> > +{
> > +     struct ksz_device *dev = ds->priv;
> > +     struct ksz_ptp_data *ptp_data;
> > +     int ret;
> > +
> > +     ptp_data = &dev->ptp_data;
> > +     mutex_init(&ptp_data->lock);
> > +
> > +     ptp_data->caps = ksz_ptp_caps;
> > +
> > +     ret = ksz_ptp_start_clock(dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ptp_data->clock = ptp_clock_register(&ptp_data->caps, dev-
> > >dev);
> > +     if (IS_ERR_OR_NULL(ptp_data->clock))
> > +             return PTR_ERR(ptp_data->clock);
> > +
> > +     ret = ksz_rmw16(dev, REG_PTP_MSG_CONF1, PTP_802_1AS,
> > PTP_802_1AS);
> > +     if (ret)
> > +             goto error_unregister_clock;
> 
> Registering a structure with a subsystem generally means that it
> becomes
> immediately accessible to user space, and its (POSIX clock) ops are
> callable.
> 
> You haven't explained what PTP_802_1AS does, concretely, even though
> I asked for a comment in the previous patch set.

I overlooked the comment in the previous patch set. Christian also gave
offline comment that, this bit is reserved in KSZ9563 datasheet. 
This bit should be set whenever we operate in 802.1AS(gPTP). 
When this bit, then all the PTP packets will be forwared to host port
and none to other ports.
After changing my patch to include 1 step timestamping, I think it
should be set only for LAN937x 2 step mode.

>  Is it okay for the PTP
> clock to be registered while the PTP_802_1AS bit hasn't been yet
> written?
> The first few operations might take place with it still unset.
> 
> I know what 802.1AS is, I just don't know what the register field
> does.



> 
> > +
> > +     return 0;
> > +
> > +error_unregister_clock:
> > +     ptp_clock_unregister(ptp_data->clock);
> > +     return ret;
> > +}

Powered by blists - more mailing lists