[PATCH net-next v3 00/10] ptp: ocp: various updates

[PATCH net-next v3 00/10] ptp: ocp: various updates

[Jonathan Lemon]

Collection of cleanups and updates to the timecard.

v2->v3
  Remove inline keyword from function wrappers
  checkpatch style changes

v1->v2
 Fix clang compilation error

Jonathan Lemon (8):
  ptp: ocp: 32-bit fixups for pci start address
  ptp: ocp: revise firmware display
  ptp: ocp: parameterize input/output sma selectors
  ptp: ocp: constify selectors
  ptp: ocp: vectorize the sma accessor functions
  ptp: ocp: add .init function for sma_op vector
  ptp: ocp: fix PPS source selector reporting
  ptp: ocp: change sysfs attr group handling

Vadim Fedorenko (2):
  ptp: ocp: add Celestica timecard PCI ids
  ptp: ocp: Add firmware header checks

 drivers/ptp/ptp_ocp.c | 716 ++++++++++++++++++++++++++----------------
 1 file changed, 451 insertions(+), 265 deletions(-)

-- 
2.31.1

[PATCH net-next v3 01/10] ptp: ocp: 32-bit fixups for pci start address

[Jonathan Lemon]

Use 'resource_size_t' instead of 'unsigned long' when computing the
pci start address, for the benefit of 32-bit platforms.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 36c0e188216b..e02a0fd70d3d 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -1403,7 +1403,7 @@ static const struct devlink_ops ptp_ocp_devlink_ops = {
 };
 
 static void __iomem *
-__ptp_ocp_get_mem(struct ptp_ocp *bp, unsigned long start, int size)
+__ptp_ocp_get_mem(struct ptp_ocp *bp, resource_size_t start, int size)
 {
 	struct resource res = DEFINE_RES_MEM_NAMED(start, size, "ptp_ocp");
 
@@ -1413,7 +1413,7 @@ __ptp_ocp_get_mem(struct ptp_ocp *bp, unsigned long start, int size)
 static void __iomem *
 ptp_ocp_get_mem(struct ptp_ocp *bp, struct ocp_resource *r)
 {
-	unsigned long start;
+	resource_size_t start;
 
 	start = pci_resource_start(bp->pdev, 0) + r->offset;
 	return __ptp_ocp_get_mem(bp, start, r->size);
@@ -1427,7 +1427,7 @@ ptp_ocp_set_irq_resource(struct resource *res, int irq)
 }
 
 static void
-ptp_ocp_set_mem_resource(struct resource *res, unsigned long start, int size)
+ptp_ocp_set_mem_resource(struct resource *res, resource_size_t start, int size)
 {
 	struct resource r = DEFINE_RES_MEM(start, size);
 	*res = r;
@@ -1440,7 +1440,7 @@ ptp_ocp_register_spi(struct ptp_ocp *bp, struct ocp_resource *r)
 	struct pci_dev *pdev = bp->pdev;
 	struct platform_device *p;
 	struct resource res[2];
-	unsigned long start;
+	resource_size_t start;
 	int id;
 
 	start = pci_resource_start(pdev, 0) + r->offset;
@@ -1467,7 +1467,7 @@ ptp_ocp_i2c_bus(struct pci_dev *pdev, struct ocp_resource *r, int id)
 {
 	struct ptp_ocp_i2c_info *info;
 	struct resource res[2];
-	unsigned long start;
+	resource_size_t start;
 
 	info = r->extra;
 	start = pci_resource_start(pdev, 0) + r->offset;
-- 
2.31.1

[PATCH net-next v3 02/10] ptp: ocp: add Celestica timecard PCI ids

[Jonathan Lemon]

From: Vadim Fedorenko <vadfed@fb.com>

Celestica is producing card with their own vendor id and device id.
Add these ids to driver to support this card.

Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index e02a0fd70d3d..d8d80138c629 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -28,6 +28,14 @@
 #define PCI_DEVICE_ID_FACEBOOK_TIMECARD 0x0400
 #endif
 
+#ifndef PCI_VENDOR_ID_CELESTICA
+#define PCI_VENDOR_ID_CELESTICA 0x18d4
+#endif
+
+#ifndef PCI_DEVICE_ID_CELESTICA_TIMECARD
+#define PCI_DEVICE_ID_CELESTICA_TIMECARD 0x1008
+#endif
+
 static struct class timecard_class = {
 	.owner		= THIS_MODULE,
 	.name		= "timecard",
@@ -634,7 +642,8 @@ static struct ocp_resource ocp_fb_resource[] = {
 
 static const struct pci_device_id ptp_ocp_pcidev_id[] = {
 	{ PCI_DEVICE_DATA(FACEBOOK, TIMECARD, &ocp_fb_resource) },
-	{ 0 }
+	{ PCI_DEVICE_DATA(CELESTICA, TIMECARD, &ocp_fb_resource) },
+	{ }
 };
 MODULE_DEVICE_TABLE(pci, ptp_ocp_pcidev_id);
 
-- 
2.31.1

Re: [PATCH net-next v3 02/10] ptp: ocp: add Celestica timecard PCI ids

[Jakub Kicinski]

On Fri, 13 May 2022 15:59:16 -0700 Jonathan Lemon wrote:
> +#ifndef PCI_VENDOR_ID_CELESTICA
> +#define PCI_VENDOR_ID_CELESTICA 0x18d4
> +#endif
> +
> +#ifndef PCI_DEVICE_ID_CELESTICA_TIMECARD
> +#define PCI_DEVICE_ID_CELESTICA_TIMECARD 0x1008
> +#endif

The ifdefs are unnecessary, these kind of constructs are often used out
of tree when one does not control the headers, but not sure what purpose
they'd serve upstream?

Re: [PATCH net-next v3 02/10] ptp: ocp: add Celestica timecard PCI ids

[Jonathan Lemon]

On Mon, May 16, 2022 at 05:43:03PM -0700, Jakub Kicinski wrote:
> On Fri, 13 May 2022 15:59:16 -0700 Jonathan Lemon wrote:
> > +#ifndef PCI_VENDOR_ID_CELESTICA
> > +#define PCI_VENDOR_ID_CELESTICA 0x18d4
> > +#endif
> > +
> > +#ifndef PCI_DEVICE_ID_CELESTICA_TIMECARD
> > +#define PCI_DEVICE_ID_CELESTICA_TIMECARD 0x1008
> > +#endif
> 
> The ifdefs are unnecessary, these kind of constructs are often used out
> of tree when one does not control the headers, but not sure what purpose
> they'd serve upstream?

include/linux/pci_ids.h says:

 *      Do not add new entries to this file unless the definitions
 *      are shared between multiple drivers.

Neither FACEBOOK (0x1d9b) nor CELESTICA (0x18d4) are present
in this file.  This seems to a common idiom in several other
drivers.  Picking one at random:

   gve.h:#define PCI_VENDOR_ID_GOOGLE 0x1ae0


So these #defines are needed.
-- 
Jonathan

Re: [PATCH net-next v3 02/10] ptp: ocp: add Celestica timecard PCI ids

[Jakub Kicinski]

On Mon, 16 May 2022 18:46:44 -0700 Jonathan Lemon wrote:
> On Mon, May 16, 2022 at 05:43:03PM -0700, Jakub Kicinski wrote:
> > On Fri, 13 May 2022 15:59:16 -0700 Jonathan Lemon wrote:  
> > > +#ifndef PCI_VENDOR_ID_CELESTICA
> > > +#define PCI_VENDOR_ID_CELESTICA 0x18d4
> > > +#endif
> > > +
> > > +#ifndef PCI_DEVICE_ID_CELESTICA_TIMECARD
> > > +#define PCI_DEVICE_ID_CELESTICA_TIMECARD 0x1008
> > > +#endif  
> > 
> > The ifdefs are unnecessary, these kind of constructs are often used out
> > of tree when one does not control the headers, but not sure what purpose
> > they'd serve upstream?  
> 
> include/linux/pci_ids.h says:
> 
>  *      Do not add new entries to this file unless the definitions
>  *      are shared between multiple drivers.
> 
> Neither FACEBOOK (0x1d9b) nor CELESTICA (0x18d4) are present
> in this file.  This seems to a common idiom in several other
> drivers.  Picking one at random:
> 
>    gve.h:#define PCI_VENDOR_ID_GOOGLE 0x1ae0
> 
> 
> So these #defines are needed.

Indeed, but also I'm not complaining about defines but the ifdefs 
in which they are wrapped :)

Re: [PATCH net-next v3 02/10] ptp: ocp: add Celestica timecard PCI ids

[Jonathan Lemon]

On Tue, May 17, 2022 at 08:25:58AM -0700, Jakub Kicinski wrote:
> On Mon, 16 May 2022 18:46:44 -0700 Jonathan Lemon wrote:
> > On Mon, May 16, 2022 at 05:43:03PM -0700, Jakub Kicinski wrote:
> > > On Fri, 13 May 2022 15:59:16 -0700 Jonathan Lemon wrote:  
> > > > +#ifndef PCI_VENDOR_ID_CELESTICA
> > > > +#define PCI_VENDOR_ID_CELESTICA 0x18d4
> > > > +#endif
> > > > +
> > > > +#ifndef PCI_DEVICE_ID_CELESTICA_TIMECARD
> > > > +#define PCI_DEVICE_ID_CELESTICA_TIMECARD 0x1008
> > > > +#endif  
> > > 
> > > The ifdefs are unnecessary, these kind of constructs are often used out
> > > of tree when one does not control the headers, but not sure what purpose
> > > they'd serve upstream?  
> > 
> > include/linux/pci_ids.h says:
> > 
> >  *      Do not add new entries to this file unless the definitions
> >  *      are shared between multiple drivers.
> > 
> > Neither FACEBOOK (0x1d9b) nor CELESTICA (0x18d4) are present
> > in this file.  This seems to a common idiom in several other
> > drivers.  Picking one at random:
> > 
> >    gve.h:#define PCI_VENDOR_ID_GOOGLE 0x1ae0
> > 
> > 
> > So these #defines are needed.
> 
> Indeed, but also I'm not complaining about defines but the ifdefs 
> in which they are wrapped :)

This is standard defensive coding practice.  I would vastly
prefer to leave them this way, and my hard-wired fingers 
would also not like to be retrained.

Next, you'll be telling me that all the kernel headers
should be using "#pragma once".
-- 
Jonathan

[PATCH net-next v3 03/10] ptp: ocp: revise firmware display

[Jonathan Lemon]

Preparse the firmware image information into loader/tag/version,
and set the fw capabilities based on the tag/version.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 64 +++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 23 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index d8d80138c629..aae1d9b0fb89 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -318,7 +318,9 @@ struct ptp_ocp {
 	int			gnss2_port;
 	int			mac_port;	/* miniature atomic clock */
 	int			nmea_port;
-	u32			fw_version;
+	bool			fw_loader;
+	u8			fw_tag;
+	u16			fw_version;
 	u8			board_id[OCP_BOARD_ID_LEN];
 	u8			serial[OCP_SERIAL_LEN];
 	bool			has_eeprom_data;
@@ -1369,6 +1371,7 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
 			 struct netlink_ext_ack *extack)
 {
 	struct ptp_ocp *bp = devlink_priv(devlink);
+	const char *fw_image;
 	char buf[32];
 	int err;
 
@@ -1376,13 +1379,9 @@ ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
 	if (err)
 		return err;
 
-	if (bp->fw_version & 0xffff) {
-		sprintf(buf, "%d", bp->fw_version);
-		err = devlink_info_version_running_put(req, "fw", buf);
-	} else {
-		sprintf(buf, "%d", bp->fw_version >> 16);
-		err = devlink_info_version_running_put(req, "loader", buf);
-	}
+	fw_image = bp->fw_loader ? "loader" : "fw";
+	sprintf(buf, "%d.%d", bp->fw_tag, bp->fw_version);
+	err = devlink_info_version_running_put(req, fw_image, buf);
 	if (err)
 		return err;
 
@@ -1905,23 +1904,50 @@ ptp_ocp_fb_set_pins(struct ptp_ocp *bp)
 	return 0;
 }
 
+static void
+ptp_ocp_fb_set_version(struct ptp_ocp *bp)
+{
+	u64 cap = OCP_CAP_BASIC;
+	u32 version;
+
+	version = ioread32(&bp->image->version);
+
+	/* if lower 16 bits are empty, this is the fw loader. */
+	if ((version & 0xffff) == 0) {
+		version = version >> 16;
+		bp->fw_loader = true;
+	}
+
+	bp->fw_tag = version >> 15;
+	bp->fw_version = version & 0x7fff;
+
+	if (bp->fw_tag) {
+		/* FPGA firmware */
+		if (version >= 5)
+			cap |= OCP_CAP_SIGNAL | OCP_CAP_FREQ;
+	} else {
+		/* SOM firmware */
+		if (version >= 19)
+			cap |= OCP_CAP_SIGNAL;
+		if (version >= 20)
+			cap |= OCP_CAP_FREQ;
+	}
+
+	bp->fw_cap = cap;
+}
+
 /* FB specific board initializers; last "resource" registered. */
 static int
 ptp_ocp_fb_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
 {
-	int ver, err;
+	int err;
 
 	bp->flash_start = 1024 * 4096;
 	bp->eeprom_map = fb_eeprom_map;
 	bp->fw_version = ioread32(&bp->image->version);
 	bp->attr_tbl = fb_timecard_groups;
-	bp->fw_cap = OCP_CAP_BASIC;
 
-	ver = bp->fw_version & 0xffff;
-	if (ver >= 19)
-		bp->fw_cap |= OCP_CAP_SIGNAL;
-	if (ver >= 20)
-		bp->fw_cap |= OCP_CAP_FREQ;
+	ptp_ocp_fb_set_version(bp);
 
 	ptp_ocp_tod_init(bp);
 	ptp_ocp_nmea_out_init(bp);
@@ -3477,14 +3503,6 @@ ptp_ocp_info(struct ptp_ocp *bp)
 
 	ptp_ocp_phc_info(bp);
 
-	dev_info(dev, "version %x\n", bp->fw_version);
-	if (bp->fw_version & 0xffff)
-		dev_info(dev, "regular image, version %d\n",
-			 bp->fw_version & 0xffff);
-	else
-		dev_info(dev, "golden image, version %d\n",
-			 bp->fw_version >> 16);
-
 	ptp_ocp_serial_info(dev, "GNSS", bp->gnss_port, 115200);
 	ptp_ocp_serial_info(dev, "GNSS2", bp->gnss2_port, 115200);
 	ptp_ocp_serial_info(dev, "MAC", bp->mac_port, 57600);
-- 
2.31.1

[PATCH net-next v3 04/10] ptp: ocp: parameterize input/output sma selectors

[Jonathan Lemon]

Group the sma input/output tables together and select the correct
group from the bp information.  This allows adding new groups with
different sma mappings.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index aae1d9b0fb89..d633bb53e021 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -331,6 +331,7 @@ struct ptp_ocp {
 	u64			fw_cap;
 	struct ptp_ocp_signal	signal[4];
 	struct ptp_ocp_sma_connector sma[4];
+	u8			sma_tbl;
 };
 
 #define OCP_REQ_TIMESTAMP	BIT(0)
@@ -709,6 +710,10 @@ static struct ocp_selector ptp_ocp_sma_out[] = {
 	{ }
 };
 
+static struct ocp_selector *ocp_sma_tbl[][2] = {
+	{ ptp_ocp_sma_in, ptp_ocp_sma_out },
+};
+
 static const char *
 ptp_ocp_select_name_from_val(struct ocp_selector *tbl, int val)
 {
@@ -2059,35 +2064,35 @@ __handle_signal_inputs(struct ptp_ocp *bp, u32 val)
  */
 
 static ssize_t
-ptp_ocp_show_output(u32 val, char *buf, int def_val)
+ptp_ocp_show_output(struct ocp_selector *tbl, u32 val, char *buf, int def_val)
 {
 	const char *name;
 	ssize_t count;
 
 	count = sysfs_emit(buf, "OUT: ");
-	name = ptp_ocp_select_name_from_val(ptp_ocp_sma_out, val);
+	name = ptp_ocp_select_name_from_val(tbl, val);
 	if (!name)
-		name = ptp_ocp_select_name_from_val(ptp_ocp_sma_out, def_val);
+		name = ptp_ocp_select_name_from_val(tbl, def_val);
 	count += sysfs_emit_at(buf, count, "%s\n", name);
 	return count;
 }
 
 static ssize_t
-ptp_ocp_show_inputs(u32 val, char *buf, int def_val)
+ptp_ocp_show_inputs(struct ocp_selector *tbl, u32 val, char *buf, int def_val)
 {
 	const char *name;
 	ssize_t count;
 	int i;
 
 	count = sysfs_emit(buf, "IN: ");
-	for (i = 0; i < ARRAY_SIZE(ptp_ocp_sma_in); i++) {
-		if (val & ptp_ocp_sma_in[i].value) {
-			name = ptp_ocp_sma_in[i].name;
+	for (i = 0; tbl[i].name; i++) {
+		if (val & tbl[i].value) {
+			name = tbl[i].name;
 			count += sysfs_emit_at(buf, count, "%s ", name);
 		}
 	}
 	if (!val && def_val >= 0) {
-		name = ptp_ocp_select_name_from_val(ptp_ocp_sma_in, def_val);
+		name = ptp_ocp_select_name_from_val(tbl, def_val);
 		count += sysfs_emit_at(buf, count, "%s ", name);
 	}
 	if (count)
@@ -2097,9 +2102,9 @@ ptp_ocp_show_inputs(u32 val, char *buf, int def_val)
 }
 
 static int
-sma_parse_inputs(const char *buf, enum ptp_ocp_sma_mode *mode)
+sma_parse_inputs(struct ocp_selector *tbl[], const char *buf,
+		 enum ptp_ocp_sma_mode *mode)
 {
-	struct ocp_selector *tbl[] = { ptp_ocp_sma_in, ptp_ocp_sma_out };
 	int idx, count, dir;
 	char **argv;
 	int ret;
@@ -2158,17 +2163,20 @@ ptp_ocp_sma_show(struct ptp_ocp *bp, int sma_nr, char *buf,
 		 int default_in_val, int default_out_val)
 {
 	struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1];
+	struct ocp_selector **tbl;
 	u32 val;
 
+	tbl = ocp_sma_tbl[bp->sma_tbl];
+
 	val = ptp_ocp_sma_get(bp, sma_nr, sma->mode) & SMA_SELECT_MASK;
 
 	if (sma->mode == SMA_MODE_IN) {
 		if (sma->disabled)
 			val = SMA_DISABLE;
-		return ptp_ocp_show_inputs(val, buf, default_in_val);
+		return ptp_ocp_show_inputs(tbl[0], val, buf, default_in_val);
 	}
 
-	return ptp_ocp_show_output(val, buf, default_out_val);
+	return ptp_ocp_show_output(tbl[1], val, buf, default_out_val);
 }
 
 static ssize_t
@@ -2259,7 +2267,7 @@ ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr)
 	int val;
 
 	mode = sma->mode;
-	val = sma_parse_inputs(buf, &mode);
+	val = sma_parse_inputs(ocp_sma_tbl[bp->sma_tbl], buf, &mode);
 	if (val < 0)
 		return val;
 
@@ -2348,7 +2356,9 @@ static ssize_t
 available_sma_inputs_show(struct device *dev,
 			  struct device_attribute *attr, char *buf)
 {
-	return ptp_ocp_select_table_show(ptp_ocp_sma_in, buf);
+	struct ptp_ocp *bp = dev_get_drvdata(dev);
+
+	return ptp_ocp_select_table_show(ocp_sma_tbl[bp->sma_tbl][0], buf);
 }
 static DEVICE_ATTR_RO(available_sma_inputs);
 
@@ -2356,7 +2366,9 @@ static ssize_t
 available_sma_outputs_show(struct device *dev,
 			   struct device_attribute *attr, char *buf)
 {
-	return ptp_ocp_select_table_show(ptp_ocp_sma_out, buf);
+	struct ptp_ocp *bp = dev_get_drvdata(dev);
+
+	return ptp_ocp_select_table_show(ocp_sma_tbl[bp->sma_tbl][1], buf);
 }
 static DEVICE_ATTR_RO(available_sma_outputs);
 
-- 
2.31.1

[PATCH net-next v3 05/10] ptp: ocp: constify selectors

[Jonathan Lemon]

The ocp selectors are all constant, so label them as such.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index d633bb53e021..a6686eca99bf 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -658,7 +658,7 @@ struct ocp_selector {
 	int value;
 };
 
-static struct ocp_selector ptp_ocp_clock[] = {
+static const struct ocp_selector ptp_ocp_clock[] = {
 	{ .name = "NONE",	.value = 0 },
 	{ .name = "TOD",	.value = 1 },
 	{ .name = "IRIG",	.value = 2 },
@@ -675,7 +675,7 @@ static struct ocp_selector ptp_ocp_clock[] = {
 #define SMA_SELECT_MASK		((1U << 15) - 1)
 #define SMA_DISABLE		0x10000
 
-static struct ocp_selector ptp_ocp_sma_in[] = {
+static const struct ocp_selector ptp_ocp_sma_in[] = {
 	{ .name = "10Mhz",	.value = 0x0000 },
 	{ .name = "PPS1",	.value = 0x0001 },
 	{ .name = "PPS2",	.value = 0x0002 },
@@ -693,7 +693,7 @@ static struct ocp_selector ptp_ocp_sma_in[] = {
 	{ }
 };
 
-static struct ocp_selector ptp_ocp_sma_out[] = {
+static const struct ocp_selector ptp_ocp_sma_out[] = {
 	{ .name = "10Mhz",	.value = 0x0000 },
 	{ .name = "PHC",	.value = 0x0001 },
 	{ .name = "MAC",	.value = 0x0002 },
@@ -710,12 +710,12 @@ static struct ocp_selector ptp_ocp_sma_out[] = {
 	{ }
 };
 
-static struct ocp_selector *ocp_sma_tbl[][2] = {
+static const struct ocp_selector *ocp_sma_tbl[][2] = {
 	{ ptp_ocp_sma_in, ptp_ocp_sma_out },
 };
 
 static const char *
-ptp_ocp_select_name_from_val(struct ocp_selector *tbl, int val)
+ptp_ocp_select_name_from_val(const struct ocp_selector *tbl, int val)
 {
 	int i;
 
@@ -726,7 +726,7 @@ ptp_ocp_select_name_from_val(struct ocp_selector *tbl, int val)
 }
 
 static int
-ptp_ocp_select_val_from_name(struct ocp_selector *tbl, const char *name)
+ptp_ocp_select_val_from_name(const struct ocp_selector *tbl, const char *name)
 {
 	const char *select;
 	int i;
@@ -740,7 +740,7 @@ ptp_ocp_select_val_from_name(struct ocp_selector *tbl, const char *name)
 }
 
 static ssize_t
-ptp_ocp_select_table_show(struct ocp_selector *tbl, char *buf)
+ptp_ocp_select_table_show(const struct ocp_selector *tbl, char *buf)
 {
 	ssize_t count;
 	int i;
@@ -2064,7 +2064,8 @@ __handle_signal_inputs(struct ptp_ocp *bp, u32 val)
  */
 
 static ssize_t
-ptp_ocp_show_output(struct ocp_selector *tbl, u32 val, char *buf, int def_val)
+ptp_ocp_show_output(const struct ocp_selector *tbl, u32 val, char *buf,
+		    int def_val)
 {
 	const char *name;
 	ssize_t count;
@@ -2078,7 +2079,8 @@ ptp_ocp_show_output(struct ocp_selector *tbl, u32 val, char *buf, int def_val)
 }
 
 static ssize_t
-ptp_ocp_show_inputs(struct ocp_selector *tbl, u32 val, char *buf, int def_val)
+ptp_ocp_show_inputs(const struct ocp_selector *tbl, u32 val, char *buf,
+		    int def_val)
 {
 	const char *name;
 	ssize_t count;
@@ -2102,7 +2104,7 @@ ptp_ocp_show_inputs(struct ocp_selector *tbl, u32 val, char *buf, int def_val)
 }
 
 static int
-sma_parse_inputs(struct ocp_selector *tbl[], const char *buf,
+sma_parse_inputs(const struct ocp_selector * const tbl[], const char *buf,
 		 enum ptp_ocp_sma_mode *mode)
 {
 	int idx, count, dir;
@@ -2163,7 +2165,7 @@ ptp_ocp_sma_show(struct ptp_ocp *bp, int sma_nr, char *buf,
 		 int default_in_val, int default_out_val)
 {
 	struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1];
-	struct ocp_selector **tbl;
+	const struct ocp_selector * const *tbl;
 	u32 val;
 
 	tbl = ocp_sma_tbl[bp->sma_tbl];
-- 
2.31.1

[PATCH net-next v3 06/10] ptp: ocp: vectorize the sma accessor functions

[Jonathan Lemon]

Move the SMA get and set functions into an operations vector for
different boards.

Create wrappers for the accessor functions.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 314 +++++++++++++++++++++++-------------------
 1 file changed, 169 insertions(+), 145 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index a6686eca99bf..b220ea59f84d 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -331,7 +331,7 @@ struct ptp_ocp {
 	u64			fw_cap;
 	struct ptp_ocp_signal	signal[4];
 	struct ptp_ocp_sma_connector sma[4];
-	u8			sma_tbl;
+	const struct ocp_sma_op *sma_op;
 };
 
 #define OCP_REQ_TIMESTAMP	BIT(0)
@@ -710,10 +710,31 @@ static const struct ocp_selector ptp_ocp_sma_out[] = {
 	{ }
 };
 
-static const struct ocp_selector *ocp_sma_tbl[][2] = {
-	{ ptp_ocp_sma_in, ptp_ocp_sma_out },
+struct ocp_sma_op {
+	const struct ocp_selector *tbl[2];
+	u32 (*get)(struct ptp_ocp *bp, int sma_nr);
+	int (*set_inputs)(struct ptp_ocp *bp, int sma_nr, u32 val);
+	int (*set_output)(struct ptp_ocp *bp, int sma_nr, u32 val);
 };
 
+static u32
+ptp_ocp_sma_get(struct ptp_ocp *bp, int sma_nr)
+{
+	return bp->sma_op->get(bp, sma_nr);
+}
+
+static int
+ptp_ocp_sma_set_inputs(struct ptp_ocp *bp, int sma_nr, u32 val)
+{
+	return bp->sma_op->set_inputs(bp, sma_nr, val);
+}
+
+static int
+ptp_ocp_sma_set_output(struct ptp_ocp *bp, int sma_nr, u32 val)
+{
+	return bp->sma_op->set_output(bp, sma_nr, val);
+}
+
 static const char *
 ptp_ocp_select_name_from_val(const struct ocp_selector *tbl, int val)
 {
@@ -1849,6 +1870,140 @@ ptp_ocp_signal_init(struct ptp_ocp *bp)
 					     bp->signal_out[i]->mem);
 }
 
+static void
+ptp_ocp_enable_fpga(u32 __iomem *reg, u32 bit, bool enable)
+{
+	u32 ctrl;
+	bool on;
+
+	ctrl = ioread32(reg);
+	on = ctrl & bit;
+	if (on ^ enable) {
+		ctrl &= ~bit;
+		ctrl |= enable ? bit : 0;
+		iowrite32(ctrl, reg);
+	}
+}
+
+static void
+ptp_ocp_irig_out(struct ptp_ocp *bp, bool enable)
+{
+	return ptp_ocp_enable_fpga(&bp->irig_out->ctrl,
+				   IRIG_M_CTRL_ENABLE, enable);
+}
+
+static void
+ptp_ocp_irig_in(struct ptp_ocp *bp, bool enable)
+{
+	return ptp_ocp_enable_fpga(&bp->irig_in->ctrl,
+				   IRIG_S_CTRL_ENABLE, enable);
+}
+
+static void
+ptp_ocp_dcf_out(struct ptp_ocp *bp, bool enable)
+{
+	return ptp_ocp_enable_fpga(&bp->dcf_out->ctrl,
+				   DCF_M_CTRL_ENABLE, enable);
+}
+
+static void
+ptp_ocp_dcf_in(struct ptp_ocp *bp, bool enable)
+{
+	return ptp_ocp_enable_fpga(&bp->dcf_in->ctrl,
+				   DCF_S_CTRL_ENABLE, enable);
+}
+
+static void
+__handle_signal_outputs(struct ptp_ocp *bp, u32 val)
+{
+	ptp_ocp_irig_out(bp, val & 0x00100010);
+	ptp_ocp_dcf_out(bp, val & 0x00200020);
+}
+
+static void
+__handle_signal_inputs(struct ptp_ocp *bp, u32 val)
+{
+	ptp_ocp_irig_in(bp, val & 0x00100010);
+	ptp_ocp_dcf_in(bp, val & 0x00200020);
+}
+
+static u32
+ptp_ocp_sma_fb_get(struct ptp_ocp *bp, int sma_nr)
+{
+	u32 __iomem *gpio;
+	u32 shift;
+
+	if (bp->sma[sma_nr - 1].fixed_fcn)
+		return (sma_nr - 1) & 1;
+
+	if (bp->sma[sma_nr - 1].mode == SMA_MODE_IN)
+		gpio = sma_nr > 2 ? &bp->sma_map2->gpio1 : &bp->sma_map1->gpio1;
+	else
+		gpio = sma_nr > 2 ? &bp->sma_map1->gpio2 : &bp->sma_map2->gpio2;
+	shift = sma_nr & 1 ? 0 : 16;
+
+	return (ioread32(gpio) >> shift) & 0xffff;
+}
+
+static int
+ptp_ocp_sma_fb_set_output(struct ptp_ocp *bp, int sma_nr, u32 val)
+{
+	u32 reg, mask, shift;
+	unsigned long flags;
+	u32 __iomem *gpio;
+
+	gpio = sma_nr > 2 ? &bp->sma_map1->gpio2 : &bp->sma_map2->gpio2;
+	shift = sma_nr & 1 ? 0 : 16;
+
+	mask = 0xffff << (16 - shift);
+
+	spin_lock_irqsave(&bp->lock, flags);
+
+	reg = ioread32(gpio);
+	reg = (reg & mask) | (val << shift);
+
+	__handle_signal_outputs(bp, reg);
+
+	iowrite32(reg, gpio);
+
+	spin_unlock_irqrestore(&bp->lock, flags);
+
+	return 0;
+}
+
+static int
+ptp_ocp_sma_fb_set_inputs(struct ptp_ocp *bp, int sma_nr, u32 val)
+{
+	u32 reg, mask, shift;
+	unsigned long flags;
+	u32 __iomem *gpio;
+
+	gpio = sma_nr > 2 ? &bp->sma_map2->gpio1 : &bp->sma_map1->gpio1;
+	shift = sma_nr & 1 ? 0 : 16;
+
+	mask = 0xffff << (16 - shift);
+
+	spin_lock_irqsave(&bp->lock, flags);
+
+	reg = ioread32(gpio);
+	reg = (reg & mask) | (val << shift);
+
+	__handle_signal_inputs(bp, reg);
+
+	iowrite32(reg, gpio);
+
+	spin_unlock_irqrestore(&bp->lock, flags);
+
+	return 0;
+}
+
+static const struct ocp_sma_op ocp_fb_sma_op = {
+	.tbl		= { ptp_ocp_sma_in, ptp_ocp_sma_out },
+	.get		= ptp_ocp_sma_fb_get,
+	.set_inputs	= ptp_ocp_sma_fb_set_inputs,
+	.set_output	= ptp_ocp_sma_fb_set_output,
+};
+
 static void
 ptp_ocp_sma_init(struct ptp_ocp *bp)
 {
@@ -1951,6 +2106,7 @@ ptp_ocp_fb_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
 	bp->eeprom_map = fb_eeprom_map;
 	bp->fw_version = ioread32(&bp->image->version);
 	bp->attr_tbl = fb_timecard_groups;
+	bp->sma_op = &ocp_fb_sma_op;
 
 	ptp_ocp_fb_set_version(bp);
 
@@ -1998,71 +2154,6 @@ ptp_ocp_register_resources(struct ptp_ocp *bp, kernel_ulong_t driver_data)
 	return err;
 }
 
-static void
-ptp_ocp_enable_fpga(u32 __iomem *reg, u32 bit, bool enable)
-{
-	u32 ctrl;
-	bool on;
-
-	ctrl = ioread32(reg);
-	on = ctrl & bit;
-	if (on ^ enable) {
-		ctrl &= ~bit;
-		ctrl |= enable ? bit : 0;
-		iowrite32(ctrl, reg);
-	}
-}
-
-static void
-ptp_ocp_irig_out(struct ptp_ocp *bp, bool enable)
-{
-	return ptp_ocp_enable_fpga(&bp->irig_out->ctrl,
-				   IRIG_M_CTRL_ENABLE, enable);
-}
-
-static void
-ptp_ocp_irig_in(struct ptp_ocp *bp, bool enable)
-{
-	return ptp_ocp_enable_fpga(&bp->irig_in->ctrl,
-				   IRIG_S_CTRL_ENABLE, enable);
-}
-
-static void
-ptp_ocp_dcf_out(struct ptp_ocp *bp, bool enable)
-{
-	return ptp_ocp_enable_fpga(&bp->dcf_out->ctrl,
-				   DCF_M_CTRL_ENABLE, enable);
-}
-
-static void
-ptp_ocp_dcf_in(struct ptp_ocp *bp, bool enable)
-{
-	return ptp_ocp_enable_fpga(&bp->dcf_in->ctrl,
-				   DCF_S_CTRL_ENABLE, enable);
-}
-
-static void
-__handle_signal_outputs(struct ptp_ocp *bp, u32 val)
-{
-	ptp_ocp_irig_out(bp, val & 0x00100010);
-	ptp_ocp_dcf_out(bp, val & 0x00200020);
-}
-
-static void
-__handle_signal_inputs(struct ptp_ocp *bp, u32 val)
-{
-	ptp_ocp_irig_in(bp, val & 0x00100010);
-	ptp_ocp_dcf_in(bp, val & 0x00200020);
-}
-
-/*
- * ANT0 == gps	(in)
- * ANT1 == sma1 (in)
- * ANT2 == sma2 (in)
- * ANT3 == sma3 (out)
- * ANT4 == sma4 (out)
- */
-
 static ssize_t
 ptp_ocp_show_output(const struct ocp_selector *tbl, u32 val, char *buf,
 		    int def_val)
@@ -2142,24 +2233,6 @@ sma_parse_inputs(const struct ocp_selector * const tbl[], const char *buf,
 	return ret;
 }
 
-static u32
-ptp_ocp_sma_get(struct ptp_ocp *bp, int sma_nr, enum ptp_ocp_sma_mode mode)
-{
-	u32 __iomem *gpio;
-	u32 shift;
-
-	if (bp->sma[sma_nr - 1].fixed_fcn)
-		return (sma_nr - 1) & 1;
-
-	if (mode == SMA_MODE_IN)
-		gpio = sma_nr > 2 ? &bp->sma_map2->gpio1 : &bp->sma_map1->gpio1;
-	else
-		gpio = sma_nr > 2 ? &bp->sma_map1->gpio2 : &bp->sma_map2->gpio2;
-	shift = sma_nr & 1 ? 0 : 16;
-
-	return (ioread32(gpio) >> shift) & 0xffff;
-}
-
 static ssize_t
 ptp_ocp_sma_show(struct ptp_ocp *bp, int sma_nr, char *buf,
 		 int default_in_val, int default_out_val)
@@ -2168,9 +2241,8 @@ ptp_ocp_sma_show(struct ptp_ocp *bp, int sma_nr, char *buf,
 	const struct ocp_selector * const *tbl;
 	u32 val;
 
-	tbl = ocp_sma_tbl[bp->sma_tbl];
-
-	val = ptp_ocp_sma_get(bp, sma_nr, sma->mode) & SMA_SELECT_MASK;
+	tbl = bp->sma_op->tbl;
+	val = ptp_ocp_sma_get(bp, sma_nr) & SMA_SELECT_MASK;
 
 	if (sma->mode == SMA_MODE_IN) {
 		if (sma->disabled)
@@ -2213,54 +2285,6 @@ sma4_show(struct device *dev, struct device_attribute *attr, char *buf)
 	return ptp_ocp_sma_show(bp, 4, buf, -1, 1);
 }
 
-static void
-ptp_ocp_sma_store_output(struct ptp_ocp *bp, int sma_nr, u32 val)
-{
-	u32 reg, mask, shift;
-	unsigned long flags;
-	u32 __iomem *gpio;
-
-	gpio = sma_nr > 2 ? &bp->sma_map1->gpio2 : &bp->sma_map2->gpio2;
-	shift = sma_nr & 1 ? 0 : 16;
-
-	mask = 0xffff << (16 - shift);
-
-	spin_lock_irqsave(&bp->lock, flags);
-
-	reg = ioread32(gpio);
-	reg = (reg & mask) | (val << shift);
-
-	__handle_signal_outputs(bp, reg);
-
-	iowrite32(reg, gpio);
-
-	spin_unlock_irqrestore(&bp->lock, flags);
-}
-
-static void
-ptp_ocp_sma_store_inputs(struct ptp_ocp *bp, int sma_nr, u32 val)
-{
-	u32 reg, mask, shift;
-	unsigned long flags;
-	u32 __iomem *gpio;
-
-	gpio = sma_nr > 2 ? &bp->sma_map2->gpio1 : &bp->sma_map1->gpio1;
-	shift = sma_nr & 1 ? 0 : 16;
-
-	mask = 0xffff << (16 - shift);
-
-	spin_lock_irqsave(&bp->lock, flags);
-
-	reg = ioread32(gpio);
-	reg = (reg & mask) | (val << shift);
-
-	__handle_signal_inputs(bp, reg);
-
-	iowrite32(reg, gpio);
-
-	spin_unlock_irqrestore(&bp->lock, flags);
-}
-
 static int
 ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr)
 {
@@ -2269,7 +2293,7 @@ ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr)
 	int val;
 
 	mode = sma->mode;
-	val = sma_parse_inputs(ocp_sma_tbl[bp->sma_tbl], buf, &mode);
+	val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode);
 	if (val < 0)
 		return val;
 
@@ -2286,9 +2310,9 @@ ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr)
 
 	if (mode != sma->mode) {
 		if (mode == SMA_MODE_IN)
-			ptp_ocp_sma_store_output(bp, sma_nr, 0);
+			ptp_ocp_sma_set_output(bp, sma_nr, 0);
 		else
-			ptp_ocp_sma_store_inputs(bp, sma_nr, 0);
+			ptp_ocp_sma_set_inputs(bp, sma_nr, 0);
 		sma->mode = mode;
 	}
 
@@ -2299,11 +2323,11 @@ ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr)
 		val = 0;
 
 	if (mode == SMA_MODE_IN)
-		ptp_ocp_sma_store_inputs(bp, sma_nr, val);
+		val = ptp_ocp_sma_set_inputs(bp, sma_nr, val);
 	else
-		ptp_ocp_sma_store_output(bp, sma_nr, val);
+		val = ptp_ocp_sma_set_output(bp, sma_nr, val);
 
-	return 0;
+	return val;
 }
 
 static ssize_t
@@ -2360,7 +2384,7 @@ available_sma_inputs_show(struct device *dev,
 {
 	struct ptp_ocp *bp = dev_get_drvdata(dev);
 
-	return ptp_ocp_select_table_show(ocp_sma_tbl[bp->sma_tbl][0], buf);
+	return ptp_ocp_select_table_show(bp->sma_op->tbl[0], buf);
 }
 static DEVICE_ATTR_RO(available_sma_inputs);
 
@@ -2370,7 +2394,7 @@ available_sma_outputs_show(struct device *dev,
 {
 	struct ptp_ocp *bp = dev_get_drvdata(dev);
 
-	return ptp_ocp_select_table_show(ocp_sma_tbl[bp->sma_tbl][1], buf);
+	return ptp_ocp_select_table_show(bp->sma_op->tbl[1], buf);
 }
 static DEVICE_ATTR_RO(available_sma_outputs);
 
-- 
2.31.1

[PATCH net-next v3 07/10] ptp: ocp: add .init function for sma_op vector

[Jonathan Lemon]

Create an .init function for the op vector, and a corresponding
wrapper function, for different sma mapping setups.

Add a default_fcn to the sma information, and use it when displaying
information for pins which have fixed functions.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index b220ea59f84d..af776c788379 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -253,6 +253,7 @@ struct ptp_ocp_sma_connector {
 	bool	fixed_fcn;
 	bool	fixed_dir;
 	bool	disabled;
+	u8	default_fcn;
 };
 
 struct ocp_attr_group {
@@ -712,11 +713,18 @@ static const struct ocp_selector ptp_ocp_sma_out[] = {
 
 struct ocp_sma_op {
 	const struct ocp_selector *tbl[2];
+	void (*init)(struct ptp_ocp *bp);
 	u32 (*get)(struct ptp_ocp *bp, int sma_nr);
 	int (*set_inputs)(struct ptp_ocp *bp, int sma_nr, u32 val);
 	int (*set_output)(struct ptp_ocp *bp, int sma_nr, u32 val);
 };
 
+static void
+ptp_ocp_sma_init(struct ptp_ocp *bp)
+{
+	return bp->sma_op->init(bp);
+}
+
 static u32
 ptp_ocp_sma_get(struct ptp_ocp *bp, int sma_nr)
 {
@@ -1997,15 +2005,8 @@ ptp_ocp_sma_fb_set_inputs(struct ptp_ocp *bp, int sma_nr, u32 val)
 	return 0;
 }
 
-static const struct ocp_sma_op ocp_fb_sma_op = {
-	.tbl		= { ptp_ocp_sma_in, ptp_ocp_sma_out },
-	.get		= ptp_ocp_sma_fb_get,
-	.set_inputs	= ptp_ocp_sma_fb_set_inputs,
-	.set_output	= ptp_ocp_sma_fb_set_output,
-};
-
 static void
-ptp_ocp_sma_init(struct ptp_ocp *bp)
+ptp_ocp_sma_fb_init(struct ptp_ocp *bp)
 {
 	u32 reg;
 	int i;
@@ -2015,6 +2016,8 @@ ptp_ocp_sma_init(struct ptp_ocp *bp)
 	bp->sma[1].mode = SMA_MODE_IN;
 	bp->sma[2].mode = SMA_MODE_OUT;
 	bp->sma[3].mode = SMA_MODE_OUT;
+	for (i = 0; i < 4; i++)
+		bp->sma[i].default_fcn = i & 1;
 
 	/* If no SMA1 map, the pin functions and directions are fixed. */
 	if (!bp->sma_map1) {
@@ -2043,6 +2046,14 @@ ptp_ocp_sma_init(struct ptp_ocp *bp)
 	}
 }
 
+static const struct ocp_sma_op ocp_fb_sma_op = {
+	.tbl		= { ptp_ocp_sma_in, ptp_ocp_sma_out },
+	.init		= ptp_ocp_sma_fb_init,
+	.get		= ptp_ocp_sma_fb_get,
+	.set_inputs	= ptp_ocp_sma_fb_set_inputs,
+	.set_output	= ptp_ocp_sma_fb_set_output,
+};
+
 static int
 ptp_ocp_fb_set_pins(struct ptp_ocp *bp)
 {
@@ -2301,7 +2312,7 @@ ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr)
 		return -EOPNOTSUPP;
 
 	if (sma->fixed_fcn) {
-		if (val != ((sma_nr - 1) & 1))
+		if (val != sma->default_fcn)
 			return -EOPNOTSUPP;
 		return 0;
 	}
-- 
2.31.1

[PATCH net-next v3 08/10] ptp: ocp: fix PPS source selector reporting

[Jonathan Lemon]

The NTL timecard design has a PPS1 selector which selects the
the PPS source automatically, according to Section 1.9 of the
documentation.

  If there is a SMA PPS input detected:
     - send signal to MAC and PPS slave selector.

  If there is a MAC PPS input detected:
     - send GNSS1 to the MAC
     - send MAC to the PPS slave

  If there is a GNSS1 input detected:
     - send GNSS1 to the MAC
     - send GNSS1 to the PPS slave.MAC

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index af776c788379..67733b319bdc 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -3070,10 +3070,10 @@ ptp_ocp_summary_show(struct seq_file *s, void *data)
 	struct device *dev = s->private;
 	struct ptp_system_timestamp sts;
 	struct ts_reg __iomem *ts_reg;
+	char *buf, *src, *mac_src;
 	struct timespec64 ts;
 	struct ptp_ocp *bp;
 	u16 sma_val[4][2];
-	char *src, *buf;
 	u32 ctrl, val;
 	bool on, map;
 	int i;
@@ -3236,17 +3236,26 @@ ptp_ocp_summary_show(struct seq_file *s, void *data)
 	if (bp->pps_select) {
 		val = ioread32(&bp->pps_select->gpio1);
 		src = &buf[80];
-		if (val & 0x01)
+		mac_src = "GNSS1";
+		if (val & 0x01) {
 			gpio_input_map(src, bp, sma_val, 0, NULL);
-		else if (val & 0x02)
+			mac_src = src;
+		} else if (val & 0x02) {
 			src = "MAC";
-		else if (val & 0x04)
+		} else if (val & 0x04) {
 			src = "GNSS1";
-		else
+		} else {
 			src = "----";
+			mac_src = src;
+		}
 	} else {
 		src = "?";
+		mac_src = src;
 	}
+	seq_printf(s, "MAC PPS1 src: %s\n", mac_src);
+
+	gpio_input_map(buf, bp, sma_val, 1, "GNSS2");
+	seq_printf(s, "MAC PPS2 src: %s\n", buf);
 
 	/* assumes automatic switchover/selection */
 	val = ioread32(&bp->reg->select);
@@ -3271,12 +3280,6 @@ ptp_ocp_summary_show(struct seq_file *s, void *data)
 	seq_printf(s, "%7s: %s, state: %s\n", "PHC src", buf,
 		   val & OCP_STATUS_IN_SYNC ? "sync" : "unsynced");
 
-	/* reuses PPS1 src from earlier */
-	seq_printf(s, "MAC PPS1 src: %s\n", src);
-
-	gpio_input_map(buf, bp, sma_val, 1, "GNSS2");
-	seq_printf(s, "MAC PPS2 src: %s\n", buf);
-
 	if (!ptp_ocp_gettimex(&bp->ptp_info, &ts, &sts)) {
 		struct timespec64 sys_ts;
 		s64 pre_ns, post_ns, ns;
-- 
2.31.1

Re: [PATCH net-next v3 08/10] ptp: ocp: fix PPS source selector reporting

[Jakub Kicinski]

On Fri, 13 May 2022 15:59:22 -0700 Jonathan Lemon wrote:
> The NTL timecard design has a PPS1 selector which selects the
> the PPS source automatically, according to Section 1.9 of the
> documentation.
> 
>   If there is a SMA PPS input detected:
>      - send signal to MAC and PPS slave selector.
> 
>   If there is a MAC PPS input detected:
>      - send GNSS1 to the MAC
>      - send MAC to the PPS slave
> 
>   If there is a GNSS1 input detected:
>      - send GNSS1 to the MAC
>      - send GNSS1 to the PPS slave.MAC
> 
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>

This one and patch 10 need Fixes tags

Re: [PATCH net-next v3 08/10] ptp: ocp: fix PPS source selector reporting

[Jonathan Lemon]

On Mon, May 16, 2022 at 05:43:17PM -0700, Jakub Kicinski wrote:
> On Fri, 13 May 2022 15:59:22 -0700 Jonathan Lemon wrote:
> > The NTL timecard design has a PPS1 selector which selects the
> > the PPS source automatically, according to Section 1.9 of the
> > documentation.
> > 
> >   If there is a SMA PPS input detected:
> >      - send signal to MAC and PPS slave selector.
> > 
> >   If there is a MAC PPS input detected:
> >      - send GNSS1 to the MAC
> >      - send MAC to the PPS slave
> > 
> >   If there is a GNSS1 input detected:
> >      - send GNSS1 to the MAC
> >      - send GNSS1 to the PPS slave.MAC
> > 
> > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> 
> This one and patch 10 need Fixes tags

This is for a debugfs entry.  I disagree that a Fixes: tag
is needed here.

I'll do it for patch 10 if you insist, but this only happens
if ptp_clock_register() fails, which not normally seen.
-- 
Jonathan

Re: [PATCH net-next v3 08/10] ptp: ocp: fix PPS source selector reporting

[Jakub Kicinski]

On Mon, 16 May 2022 18:54:28 -0700 Jonathan Lemon wrote:
> On Mon, May 16, 2022 at 05:43:17PM -0700, Jakub Kicinski wrote:
> > On Fri, 13 May 2022 15:59:22 -0700 Jonathan Lemon wrote:  
> > > The NTL timecard design has a PPS1 selector which selects the
> > > the PPS source automatically, according to Section 1.9 of the
> > > documentation.
> > > 
> > >   If there is a SMA PPS input detected:
> > >      - send signal to MAC and PPS slave selector.
> > > 
> > >   If there is a MAC PPS input detected:
> > >      - send GNSS1 to the MAC
> > >      - send MAC to the PPS slave
> > > 
> > >   If there is a GNSS1 input detected:
> > >      - send GNSS1 to the MAC
> > >      - send GNSS1 to the PPS slave.MAC
> > > 
> > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>  
> > 
> > This one and patch 10 need Fixes tags  
> 
> This is for a debugfs entry.  I disagree that a Fixes: tag
> is needed here.
> 
> I'll do it for patch 10 if you insist, but this only happens
> if ptp_clock_register() fails, which not normally seen.

Fixes need a fixes tag and need to target the right tree.

Re: [PATCH net-next v3 08/10] ptp: ocp: fix PPS source selector reporting

[Jonathan Lemon]

On Mon, May 16, 2022 at 06:54:28PM -0700, Jonathan Lemon wrote:
> On Mon, May 16, 2022 at 05:43:17PM -0700, Jakub Kicinski wrote:
> > On Fri, 13 May 2022 15:59:22 -0700 Jonathan Lemon wrote:
> > > The NTL timecard design has a PPS1 selector which selects the
> > > the PPS source automatically, according to Section 1.9 of the
> > > documentation.
> > > 
> > >   If there is a SMA PPS input detected:
> > >      - send signal to MAC and PPS slave selector.
> > > 
> > >   If there is a MAC PPS input detected:
> > >      - send GNSS1 to the MAC
> > >      - send MAC to the PPS slave
> > > 
> > >   If there is a GNSS1 input detected:
> > >      - send GNSS1 to the MAC
> > >      - send GNSS1 to the PPS slave.MAC
> > > 
> > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> > 
> > This one and patch 10 need Fixes tags
> 
> This is for a debugfs entry.  I disagree that a Fixes: tag
> is needed here.
> 
> I'll do it for patch 10 if you insist, but this only happens
> if ptp_clock_register() fails, which not normally seen.

Actually, patch 10 would be:

Fixes: c205d53c4923 ("ptp: ocp: Add firmware capability bits for feature gating")

Which is only in 5.18-rcX at this point.

Do we need a fixes tags for code which hasn't made it into a
full release release yet?
-- 
Jonathan

Re: [PATCH net-next v3 08/10] ptp: ocp: fix PPS source selector reporting

[Jakub Kicinski]

On Tue, 17 May 2022 08:39:42 -0700 Jonathan Lemon wrote:
> On Mon, May 16, 2022 at 06:54:28PM -0700, Jonathan Lemon wrote:
> > On Mon, May 16, 2022 at 05:43:17PM -0700, Jakub Kicinski wrote:  
> > > This one and patch 10 need Fixes tags  
> > 
> > This is for a debugfs entry.  I disagree that a Fixes: tag
> > is needed here.
> > 
> > I'll do it for patch 10 if you insist, but this only happens
> > if ptp_clock_register() fails, which not normally seen.  
> 
> Actually, patch 10 would be:
> 
> Fixes: c205d53c4923 ("ptp: ocp: Add firmware capability bits for feature gating")
> 
> Which is only in 5.18-rcX at this point.
> 
> Do we need a fixes tags for code which hasn't made it into a
> full release release yet?

Yup, having the Fixes tag makes it obvious to the maintainer that 
the tree selection is correct and helps backporters figure out if 
they need to worry that the patch didn't apply.

[PATCH net-next v3 09/10] ptp: ocp: Add firmware header checks

[Jonathan Lemon]

From: Vadim Fedorenko <vadfed@fb.com>

Right now it's possible to flash any kind of binary via devlink and
break the card easily. This diff adds an optional header check when
installing the firmware.

Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 78 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 73 insertions(+), 5 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 67733b319bdc..4ff7f16242cf 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -19,6 +19,7 @@
 #include <linux/i2c.h>
 #include <linux/mtd/mtd.h>
 #include <linux/nvmem-consumer.h>
+#include <linux/crc16.h>
 
 #ifndef PCI_VENDOR_ID_FACEBOOK
 #define PCI_VENDOR_ID_FACEBOOK 0x1d9b
@@ -223,6 +224,17 @@ struct ptp_ocp_flash_info {
 	void *data;
 };
 
+struct ptp_ocp_firmware_header {
+	char magic[4];
+	__be16 pci_vendor_id;
+	__be16 pci_device_id;
+	__be32 image_size;
+	__be16 hw_revision;
+	__be16 crc;
+};
+
+#define OCP_FIRMWARE_MAGIC_HEADER "OCPC"
+
 struct ptp_ocp_i2c_info {
 	const char *name;
 	unsigned long fixed_rate;
@@ -1333,25 +1345,81 @@ ptp_ocp_find_flash(struct ptp_ocp *bp)
 	return dev;
 }
 
+static int
+ptp_ocp_devlink_fw_image(struct devlink *devlink, const struct firmware *fw,
+			 const u8 **data, size_t *size)
+{
+	struct ptp_ocp *bp = devlink_priv(devlink);
+	const struct ptp_ocp_firmware_header *hdr;
+	size_t offset, length;
+	u16 crc;
+
+	hdr = (const struct ptp_ocp_firmware_header *)fw->data;
+	if (memcmp(hdr->magic, OCP_FIRMWARE_MAGIC_HEADER, 4)) {
+		devlink_flash_update_status_notify(devlink,
+			"No firmware header found, flashing raw image",
+			NULL, 0, 0);
+		offset = 0;
+		length = fw->size;
+		goto out;
+	}
+
+	if (be16_to_cpu(hdr->pci_vendor_id) != bp->pdev->vendor ||
+	    be16_to_cpu(hdr->pci_device_id) != bp->pdev->device) {
+		devlink_flash_update_status_notify(devlink,
+			"Firmware image compatibility check failed",
+			NULL, 0, 0);
+		return -EINVAL;
+	}
+
+	offset = sizeof(*hdr);
+	length = be32_to_cpu(hdr->image_size);
+	if (length != (fw->size - offset)) {
+		devlink_flash_update_status_notify(devlink,
+			"Firmware image size check failed",
+			NULL, 0, 0);
+		return -EINVAL;
+	}
+
+	crc = crc16(0xffff, &fw->data[offset], length);
+	if (be16_to_cpu(hdr->crc) != crc) {
+		devlink_flash_update_status_notify(devlink,
+			"Firmware image CRC check failed",
+			NULL, 0, 0);
+		return -EINVAL;
+	}
+
+out:
+	*data = &fw->data[offset];
+	*size = length;
+
+	return 0;
+}
+
 static int
 ptp_ocp_devlink_flash(struct devlink *devlink, struct device *dev,
 		      const struct firmware *fw)
 {
 	struct mtd_info *mtd = dev_get_drvdata(dev);
 	struct ptp_ocp *bp = devlink_priv(devlink);
-	size_t off, len, resid, wrote;
+	size_t off, len, size, resid, wrote;
 	struct erase_info erase;
 	size_t base, blksz;
-	int err = 0;
+	const u8 *data;
+	int err;
+
+	err = ptp_ocp_devlink_fw_image(devlink, fw, &data, &size);
+	if (err)
+		goto out;
 
 	off = 0;
 	base = bp->flash_start;
 	blksz = 4096;
-	resid = fw->size;
+	resid = size;
 
 	while (resid) {
 		devlink_flash_update_status_notify(devlink, "Flashing",
-						   NULL, off, fw->size);
+						   NULL, off, size);
 
 		len = min_t(size_t, resid, blksz);
 		erase.addr = base + off;
@@ -1361,7 +1429,7 @@ ptp_ocp_devlink_flash(struct devlink *devlink, struct device *dev,
 		if (err)
 			goto out;
 
-		err = mtd_write(mtd, base + off, len, &wrote, &fw->data[off]);
+		err = mtd_write(mtd, base + off, len, &wrote, data + off);
 		if (err)
 			goto out;
 
-- 
2.31.1

[PATCH net-next v3 10/10] ptp: ocp: change sysfs attr group handling

[Jonathan Lemon]

In the detach path, the driver calls sysfs_remove_group() for the
groups it believes has been registered.  However, if the group was
never previously registered, then this causes a splat.

Instead, compute the groups that should be registered in advance,
and then use sysfs_create_groups(), which registers them all at once.
Update the error handling appropriately.

Reported-by: Zheyu Ma <zheyuma97@gmail.com>
Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/ptp/ptp_ocp.c | 67 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 53 insertions(+), 14 deletions(-)

diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index 4ff7f16242cf..6772b1e7e6d2 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -321,7 +321,7 @@ struct ptp_ocp {
 	struct platform_device	*spi_flash;
 	struct clk_hw		*i2c_clk;
 	struct timer_list	watchdog;
-	const struct ocp_attr_group *attr_tbl;
+	const struct attribute_group **attr_group;
 	const struct ptp_ocp_eeprom_map *eeprom_map;
 	struct dentry		*debug_root;
 	time64_t		gnss_lost;
@@ -1946,6 +1946,30 @@ ptp_ocp_signal_init(struct ptp_ocp *bp)
 					     bp->signal_out[i]->mem);
 }
 
+static int
+ptp_ocp_build_attr_group(struct ptp_ocp *bp,
+			 const struct ocp_attr_group *attr_tbl)
+{
+	int count, i;
+
+	count = 0;
+	for (i = 0; attr_tbl[i].cap; i++)
+		if (attr_tbl[i].cap & bp->fw_cap)
+			count++;
+
+	bp->attr_group = kcalloc(count + 1, sizeof(struct attribute_group *),
+				 GFP_KERNEL);
+	if (!bp->attr_group)
+		return -ENOMEM;
+
+	count = 0;
+	for (i = 0; attr_tbl[i].cap; i++)
+		if (attr_tbl[i].cap & bp->fw_cap)
+			bp->attr_group[count++] = attr_tbl[i].group;
+
+	return 0;
+}
+
 static void
 ptp_ocp_enable_fpga(u32 __iomem *reg, u32 bit, bool enable)
 {
@@ -2184,7 +2208,6 @@ ptp_ocp_fb_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
 	bp->flash_start = 1024 * 4096;
 	bp->eeprom_map = fb_eeprom_map;
 	bp->fw_version = ioread32(&bp->image->version);
-	bp->attr_tbl = fb_timecard_groups;
 	bp->sma_op = &ocp_fb_sma_op;
 
 	ptp_ocp_fb_set_version(bp);
@@ -2194,6 +2217,10 @@ ptp_ocp_fb_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
 	ptp_ocp_sma_init(bp);
 	ptp_ocp_signal_init(bp);
 
+	err = ptp_ocp_build_attr_group(bp, fb_timecard_groups);
+	if (err)
+		return err;
+
 	err = ptp_ocp_fb_set_pins(bp);
 	if (err)
 		return err;
@@ -3539,12 +3566,31 @@ ptp_ocp_link_child(struct ptp_ocp *bp, const char *name, const char *link)
 	put_device(child);
 }
 
+static void
+ptp_ocp_attr_group_del(struct ptp_ocp *bp)
+{
+	sysfs_remove_groups(&bp->dev.kobj, bp->attr_group);
+	kfree(bp->attr_group);
+}
+
+static int
+ptp_ocp_attr_group_add(struct ptp_ocp *bp)
+{
+	int err;
+
+	err = sysfs_create_groups(&bp->dev.kobj, bp->attr_group);
+	if (err)
+		bp->attr_group[0] = NULL;
+
+	return err;
+}
+
 static int
 ptp_ocp_complete(struct ptp_ocp *bp)
 {
 	struct pps_device *pps;
 	char buf[32];
-	int i, err;
+	int err;
 
 	if (bp->gnss_port != -1) {
 		sprintf(buf, "ttyS%d", bp->gnss_port);
@@ -3569,13 +3615,9 @@ ptp_ocp_complete(struct ptp_ocp *bp)
 	if (pps)
 		ptp_ocp_symlink(bp, pps->dev, "pps");
 
-	for (i = 0; bp->attr_tbl[i].cap; i++) {
-		if (!(bp->attr_tbl[i].cap & bp->fw_cap))
-			continue;
-		err = sysfs_create_group(&bp->dev.kobj, bp->attr_tbl[i].group);
-		if (err)
-			return err;
-	}
+	err = ptp_ocp_attr_group_add(bp);
+	if (err)
+		return err;
 
 	ptp_ocp_debugfs_add_device(bp);
 
@@ -3640,15 +3682,11 @@ static void
 ptp_ocp_detach_sysfs(struct ptp_ocp *bp)
 {
 	struct device *dev = &bp->dev;
-	int i;
 
 	sysfs_remove_link(&dev->kobj, "ttyGNSS");
 	sysfs_remove_link(&dev->kobj, "ttyMAC");
 	sysfs_remove_link(&dev->kobj, "ptp");
 	sysfs_remove_link(&dev->kobj, "pps");
-	if (bp->attr_tbl)
-		for (i = 0; bp->attr_tbl[i].cap; i++)
-			sysfs_remove_group(&dev->kobj, bp->attr_tbl[i].group);
 }
 
 static void
@@ -3658,6 +3696,7 @@ ptp_ocp_detach(struct ptp_ocp *bp)
 
 	ptp_ocp_debugfs_remove_device(bp);
 	ptp_ocp_detach_sysfs(bp);
+	ptp_ocp_attr_group_del(bp);
 	if (timer_pending(&bp->watchdog))
 		del_timer_sync(&bp->watchdog);
 	if (bp->ts0)
-- 
2.31.1