Hi,
On Wed, Aug 19, 2020 at 9:30 PM Jesse Brandeburg
<jesse.brandeb...@intel.com> wrote:
>
> sundeep.l...@gmail.com wrote:
>
> > From: Aleksey Makarov <amaka...@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 <sbha...@marvell.com>
> > Signed-off-by: Subbaraya Sundeep <sbha...@marvell.com>
> > Signed-off-by: Aleksey Makarov <amaka...@marvell.com>
> > Signed-off-by: Sunil Goutham <sgout...@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.kovv...@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>
