Re: [PATCH v6 net-next 2/3] octeontx2-af: Add support for Marvell PTP coprocessor

[sundeep subbaraya <sundeep.lkml@gmail.com>]

Hi,

On Wed, Aug 19, 2020 at 9:30 PM Jesse Brandeburg
<jesse.brandeburg@intel.com> wrote:
>
> sundeep.lkml@gmail.com wrote:
>
> > From: Aleksey Makarov <amakarov@marvell.com>
> >
> > This patch adds driver for Precision Time
> > Protocol Clock and Timestamping block found on
> > Octeontx2 platform. The driver does initial
> > configuration and exposes a function to adjust
> > PTP hardware clock.
>
> Please explain in the commit message why you have two methods of
> handling the clocks PCI space, as without that it seems like some of
> the code is either un-necessary or not clear why it's there.
>
Sure will elaborate it.
> >
> > Co-developed-by: Subbaraya Sundeep <sbhatta@marvell.com>
> > Signed-off-by: Subbaraya Sundeep <sbhatta@marvell.com>
> > Signed-off-by: Aleksey Makarov <amakarov@marvell.com>
> > Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
> > ---
> >  drivers/net/ethernet/marvell/octeontx2/af/Makefile |   2 +-
> >  drivers/net/ethernet/marvell/octeontx2/af/mbox.h   |  17 ++
> >  drivers/net/ethernet/marvell/octeontx2/af/ptp.c    | 248 +++++++++++++++++++++
> >  drivers/net/ethernet/marvell/octeontx2/af/ptp.h    |  22 ++
> >  drivers/net/ethernet/marvell/octeontx2/af/rvu.c    |  29 ++-
> >  drivers/net/ethernet/marvell/octeontx2/af/rvu.h    |   4 +
> >  6 files changed, 318 insertions(+), 4 deletions(-)
> >  create mode 100644 drivers/net/ethernet/marvell/octeontx2/af/ptp.c
> >  create mode 100644 drivers/net/ethernet/marvell/octeontx2/af/ptp.h
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/Makefile b/drivers/net/ethernet/marvell/octeontx2/af/Makefile
> > index 1b25948..0bc2410 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/Makefile
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/Makefile
> > @@ -8,4 +8,4 @@ obj-$(CONFIG_OCTEONTX2_AF) += octeontx2_af.o
> >
> >  octeontx2_mbox-y := mbox.o
> >  octeontx2_af-y := cgx.o rvu.o rvu_cgx.o rvu_npa.o rvu_nix.o \
> > -               rvu_reg.o rvu_npc.o rvu_debugfs.o
> > +               rvu_reg.o rvu_npc.o rvu_debugfs.o ptp.o
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> > index c89b098..4aaef0a 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> > @@ -127,6 +127,7 @@ M(ATTACH_RESOURCES,       0x002, attach_resources, rsrc_attach, msg_rsp)  \
> >  M(DETACH_RESOURCES,  0x003, detach_resources, rsrc_detach, msg_rsp)  \
> >  M(MSIX_OFFSET,               0x005, msix_offset, msg_req, msix_offset_rsp)   \
> >  M(VF_FLR,            0x006, vf_flr, msg_req, msg_rsp)                \
> > +M(PTP_OP,            0x007, ptp_op, ptp_req, ptp_rsp)                \
> >  M(GET_HW_CAP,                0x008, get_hw_cap, msg_req, get_hw_cap_rsp)     \
> >  /* CGX mbox IDs (range 0x200 - 0x3FF) */                             \
> >  M(CGX_START_RXTX,    0x200, cgx_start_rxtx, msg_req, msg_rsp)        \
> > @@ -862,4 +863,20 @@ struct npc_get_kex_cfg_rsp {
> >       u8 mkex_pfl_name[MKEX_NAME_LEN];
> >  };
> >
> > +enum ptp_op {
> > +     PTP_OP_ADJFINE = 0,
> > +     PTP_OP_GET_CLOCK = 1,
> > +};
> > +
> > +struct ptp_req {
> > +     struct mbox_msghdr hdr;
> > +     u8 op;
> > +     s64 scaled_ppm;
> > +};
> > +
> > +struct ptp_rsp {
> > +     struct mbox_msghdr hdr;
> > +     u64 clk;
> > +};
> > +
> >  #endif /* MBOX_H */
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/ptp.c b/drivers/net/ethernet/marvell/octeontx2/af/ptp.c
> > new file mode 100644
> > index 0000000..e9e131d
> > --- /dev/null
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/ptp.c
> > @@ -0,0 +1,248 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Marvell PTP driver */
>
> Your file is missing Copyrights, is that your intent?
>
From the discussion during VF driver submission @
https://patchwork.ozlabs.org/project/netdev/patch/1584092566-4793-4-git-send-email-sunil.kovvuri@gmail.com/#2384778
we are putting only the two lines SPDX and short driver description

> I didn't have any comments for the rest of the patch, except that there
> is a lack of comments and communication of intent of the code. I can
> see what it does, but think of the point of view of a kernel consumer
> getting this code in the future and wanting to extend it or debug it,
> and the code being able to talk to "future you" who has no idea why the
> code was there or what it was trying to do.
>
Okay I will add comments where seems necessary.

Thanks,
Sundeep
> <snip>