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

[Jesse Brandeburg <jesse.brandeburg@intel.com>]

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.

> 
> 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?

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.

<snip>