diff mbox series

[RFC,v2,3/3] ptp_ocp: implement DPLL ops

Message ID 20220626192444.29321-4-vfedorenko@novek.ru (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series Create common DPLL/clock configuration API | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 0 this patch: 30
netdev/cc_maintainers warning 1 maintainers not CCed: richardcochran@gmail.com
netdev/build_clang fail Errors and warnings before: 0 this patch: 25
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 0 this patch: 35
netdev/checkpatch warning WARNING: line length of 86 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns WARNING: line length of 89 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns WARNING: line length of 94 exceeds 80 columns WARNING: line length of 96 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Vadim Fedorenko June 26, 2022, 7:24 p.m. UTC 
From: Vadim Fedorenko <vadfed@fb.com>

Implement DPLL operations in ptp_ocp driver.

Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
---
 drivers/ptp/Kconfig       |   1 +
 drivers/ptp/ptp_ocp.c     | 169 ++++++++++++++++++++++++++++++--------
 include/uapi/linux/dpll.h |   2 +
 3 files changed, 136 insertions(+), 36 deletions(-)

Comments

Jonathan Lemon June 27, 2022, 7:34 p.m. UTC | #1 
On Sun, Jun 26, 2022 at 10:24:44PM +0300, Vadim Fedorenko wrote:
> From: Vadim Fedorenko <vadfed@fb.com>
> 
> Implement DPLL operations in ptp_ocp driver.
> 
> Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
> ---
>  drivers/ptp/Kconfig       |   1 +
>  drivers/ptp/ptp_ocp.c     | 169 ++++++++++++++++++++++++++++++--------
>  include/uapi/linux/dpll.h |   2 +
>  3 files changed, 136 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> index 458218f88c5e..f74846ebc177 100644
> --- a/drivers/ptp/Kconfig
> +++ b/drivers/ptp/Kconfig
> @@ -176,6 +176,7 @@ config PTP_1588_CLOCK_OCP
>  	depends on !S390
>  	depends on COMMON_CLK
>  	select NET_DEVLINK
> +	select DPLL
>  	help
>  	  This driver adds support for an OpenCompute time card.
>  
> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index e59ea2173aac..f830625a63a3 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -21,6 +21,7 @@
>  #include <linux/mtd/mtd.h>
>  #include <linux/nvmem-consumer.h>
>  #include <linux/crc16.h>
> +#include <uapi/linux/dpll.h>
>  
>  #define PCI_VENDOR_ID_FACEBOOK			0x1d9b
>  #define PCI_DEVICE_ID_FACEBOOK_TIMECARD		0x0400
> @@ -336,6 +337,7 @@ struct ptp_ocp {
>  	struct ptp_ocp_signal	signal[4];
>  	struct ptp_ocp_sma_connector sma[4];
>  	const struct ocp_sma_op *sma_op;
> +	struct dpll_device *dpll;
>  };
>  
>  #define OCP_REQ_TIMESTAMP	BIT(0)
> @@ -660,18 +662,19 @@ static DEFINE_IDR(ptp_ocp_idr);
>  struct ocp_selector {
>  	const char *name;
>  	int value;
> +	int dpll_type;
>  };
>  
>  static const struct ocp_selector ptp_ocp_clock[] = {
> -	{ .name = "NONE",	.value = 0 },
> -	{ .name = "TOD",	.value = 1 },
> -	{ .name = "IRIG",	.value = 2 },
> -	{ .name = "PPS",	.value = 3 },
> -	{ .name = "PTP",	.value = 4 },
> -	{ .name = "RTC",	.value = 5 },
> -	{ .name = "DCF",	.value = 6 },
> -	{ .name = "REGS",	.value = 0xfe },
> -	{ .name = "EXT",	.value = 0xff },
> +	{ .name = "NONE",	.value = 0,		.dpll_type = 0 },
> +	{ .name = "TOD",	.value = 1,		.dpll_type = 0 },
> +	{ .name = "IRIG",	.value = 2,		.dpll_type = 0 },
> +	{ .name = "PPS",	.value = 3,		.dpll_type = 0 },
> +	{ .name = "PTP",	.value = 4,		.dpll_type = 0 },
> +	{ .name = "RTC",	.value = 5,		.dpll_type = 0 },
> +	{ .name = "DCF",	.value = 6,		.dpll_type = 0 },
> +	{ .name = "REGS",	.value = 0xfe,		.dpll_type = 0 },
> +	{ .name = "EXT",	.value = 0xff,		.dpll_type = 0 },

No need for zeros, or are they just temp stubs?

> @@ -680,37 +683,37 @@ static const struct ocp_selector ptp_ocp_clock[] = {
>  #define SMA_SELECT_MASK		GENMASK(14, 0)
>  
>  static const struct ocp_selector ptp_ocp_sma_in[] = {
> -	{ .name = "10Mhz",	.value = 0x0000 },
> -	{ .name = "PPS1",	.value = 0x0001 },
> -	{ .name = "PPS2",	.value = 0x0002 },
> -	{ .name = "TS1",	.value = 0x0004 },
> -	{ .name = "TS2",	.value = 0x0008 },
> -	{ .name = "IRIG",	.value = 0x0010 },
> -	{ .name = "DCF",	.value = 0x0020 },
> -	{ .name = "TS3",	.value = 0x0040 },
> -	{ .name = "TS4",	.value = 0x0080 },
> -	{ .name = "FREQ1",	.value = 0x0100 },
> -	{ .name = "FREQ2",	.value = 0x0200 },
> -	{ .name = "FREQ3",	.value = 0x0400 },
> -	{ .name = "FREQ4",	.value = 0x0800 },
> -	{ .name = "None",	.value = SMA_DISABLE },
> +	{ .name = "10Mhz",	.value = 0x0000,	.dpll_type = DPLL_TYPE_EXT_10MHZ },
> +	{ .name = "PPS1",	.value = 0x0001,	.dpll_type = DPLL_TYPE_EXT_1PPS },
> +	{ .name = "PPS2",	.value = 0x0002,	.dpll_type = DPLL_TYPE_EXT_1PPS },
> +	{ .name = "TS1",	.value = 0x0004,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "TS2",	.value = 0x0008,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "IRIG",	.value = 0x0010,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "DCF",	.value = 0x0020,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "TS3",	.value = 0x0040,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "TS4",	.value = 0x0080,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "FREQ1",	.value = 0x0100,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "FREQ2",	.value = 0x0200,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "FREQ3",	.value = 0x0400,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "FREQ4",	.value = 0x0800,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "None",	.value = SMA_DISABLE,	.dpll_type = DPLL_TYPE_NONE },

80-column limit (here and throughout the file)


>  static const struct ocp_selector ptp_ocp_sma_out[] = {
> -	{ .name = "10Mhz",	.value = 0x0000 },
> -	{ .name = "PHC",	.value = 0x0001 },
> -	{ .name = "MAC",	.value = 0x0002 },
> -	{ .name = "GNSS1",	.value = 0x0004 },
> -	{ .name = "GNSS2",	.value = 0x0008 },
> -	{ .name = "IRIG",	.value = 0x0010 },
> -	{ .name = "DCF",	.value = 0x0020 },
> -	{ .name = "GEN1",	.value = 0x0040 },
> -	{ .name = "GEN2",	.value = 0x0080 },
> -	{ .name = "GEN3",	.value = 0x0100 },
> -	{ .name = "GEN4",	.value = 0x0200 },
> -	{ .name = "GND",	.value = 0x2000 },
> -	{ .name = "VCC",	.value = 0x4000 },
> +	{ .name = "10Mhz",	.value = 0x0000,	.dpll_type = DPLL_TYPE_EXT_10MHZ },
> +	{ .name = "PHC",	.value = 0x0001,	.dpll_type = DPLL_TYPE_INT_OSCILLATOR },
> +	{ .name = "MAC",	.value = 0x0002,	.dpll_type = DPLL_TYPE_INT_OSCILLATOR },

How does this work?  The DPLL types seem somewhat restrictive here.


> +	{ .name = "GNSS1",	.value = 0x0004,	.dpll_type = DPLL_TYPE_GNSS },
> +	{ .name = "GNSS2",	.value = 0x0008,	.dpll_type = DPLL_TYPE_GNSS },
> +	{ .name = "IRIG",	.value = 0x0010,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "DCF",	.value = 0x0020,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "GEN1",	.value = 0x0040,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "GEN2",	.value = 0x0080,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "GEN3",	.value = 0x0100,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "GEN4",	.value = 0x0200,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "GND",	.value = 0x2000,	.dpll_type = DPLL_TYPE_CUSTOM },
> +	{ .name = "VCC",	.value = 0x4000,	.dpll_type = DPLL_TYPE_CUSTOM },

80-columns


>  
> @@ -3713,6 +3716,90 @@ ptp_ocp_detach(struct ptp_ocp *bp)
>  	device_unregister(&bp->dev);
>  }
>  
> +static int ptp_ocp_dpll_get_status(struct dpll_device *dpll)
> +{
> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> +	int sync;
> +
> +	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
> +	return sync;
> +}
> +
> +static int ptp_ocp_dpll_get_lock_status(struct dpll_device *dpll)
> +{
> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> +	int sync;
> +
> +	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
> +	return sync;
> +}
> +
> +static int ptp_ocp_sma_get_dpll_type(struct ptp_ocp *bp, int sma_nr)
> +{
> +	struct ocp_selector *tbl;
> +	u32 val;
> +
> +	if (bp->sma[sma_nr].mode == SMA_MODE_IN)
> +		tbl = bp->sma_op->tbl[0];
> +	else
> +		tbl = bp->sma_op->tbl[1];
> +
> +	val = ptp_ocp_sma_get(bp, sma_nr);
> +	return tbl[val].dpll_type;
> +}
> +
> +static int ptp_ocp_dpll_type_supported(struct dpll_device *dpll, int sma, int type, int dir)
> +{
> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> +	struct ocp_selector *tbl = bp->sma_op->tbl[dir];
> +	int i;
> +
> +	for (i = 0; i < sizeof(*tbl); i++) {
> +		if (tbl[i].dpll_type == type)
> +			return 1;
> +	}
> +	return 0;
> +}
> +
> +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
> +{
> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> +
> +	if (bp->sma[sma].mode != SMA_MODE_IN)
> +		return -1;
> +
> +	return ptp_ocp_sma_get_dpll_type(bp, sma);
> +}
> +
> +static int ptp_ocp_dpll_get_source_supported(struct dpll_device *dpll, int sma, int type)
> +{
> +	return ptp_ocp_dpll_type_supported(dpll, sma, type, 0);
> +}
> +
> +static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
> +{
> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> +
> +	if (bp->sma[sma].mode != SMA_MODE_OUT)
> +		return -1;
> +
> +	return ptp_ocp_sma_get_dpll_type(bp, sma);
> +}
> +
> +static int ptp_ocp_dpll_get_output_supported(struct dpll_device *dpll, int sma, int type)
> +{
> +	return ptp_ocp_dpll_type_supported(dpll, sma, type, 1);
> +}
> +
> +static struct dpll_device_ops dpll_ops = {
> +	.get_status		= ptp_ocp_dpll_get_status,
> +	.get_lock_status	= ptp_ocp_dpll_get_lock_status,
> +	.get_source_type	= ptp_ocp_dpll_get_source_type,
> +	.get_source_supported	= ptp_ocp_dpll_get_source_supported,
> +	.get_output_type	= ptp_ocp_dpll_get_output_type,
> +	.get_output_supported	= ptp_ocp_dpll_get_output_supported,
> +};
> +
>  static int
>  ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
> @@ -3768,6 +3855,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  	ptp_ocp_info(bp);
>  	devlink_register(devlink);
> +
> +	bp->dpll = dpll_device_alloc(&dpll_ops, ARRAY_SIZE(bp->sma), ARRAY_SIZE(bp->sma), bp);
> +	if (!bp->dpll) {
> +		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
> +		return 0;
> +	}
> +	dpll_device_register(bp->dpll);
> +
>  	return 0;

80 cols, and this should be done before ptp_ocp_complete()
Also, should 'goto out', not return 0 and leak resources.


>  
>  out:
> @@ -3785,6 +3880,8 @@ ptp_ocp_remove(struct pci_dev *pdev)
>  	struct ptp_ocp *bp = pci_get_drvdata(pdev);
>  	struct devlink *devlink = priv_to_devlink(bp);
>  
> +	dpll_device_unregister(bp->dpll);
> +	dpll_device_free(bp->dpll);
>  	devlink_unregister(devlink);
>  	ptp_ocp_detach(bp);
>  	pci_disable_device(pdev);

This should be in ptp_ocp_detach() to handle probe failures.


> diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
> index 7ce45c6b4fd4..5e8c46712d18 100644
> --- a/include/uapi/linux/dpll.h
> +++ b/include/uapi/linux/dpll.h
> @@ -41,11 +41,13 @@ enum dpll_genl_attr {
>  
>  /* DPLL signal types used as source or as output */
>  enum dpll_genl_signal_type {
> +	DPLL_TYPE_NONE,
>  	DPLL_TYPE_EXT_1PPS,
>  	DPLL_TYPE_EXT_10MHZ,
>  	DPLL_TYPE_SYNCE_ETH_PORT,
>  	DPLL_TYPE_INT_OSCILLATOR,
>  	DPLL_TYPE_GNSS,
> +	DPLL_TYPE_CUSTOM,
>  
>  	__DPLL_TYPE_MAX,
>  };
> -- 
> 2.27.0
>
Vadim Fedorenko June 27, 2022, 10:13 p.m. UTC | #2 
On 27.06.2022 20:34, Jonathan Lemon wrote:
> On Sun, Jun 26, 2022 at 10:24:44PM +0300, Vadim Fedorenko wrote:
>> From: Vadim Fedorenko <vadfed@fb.com>
>>
>> Implement DPLL operations in ptp_ocp driver.
>>
>> Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
>> ---
>>   drivers/ptp/Kconfig       |   1 +
>>   drivers/ptp/ptp_ocp.c     | 169 ++++++++++++++++++++++++++++++--------
>>   include/uapi/linux/dpll.h |   2 +
>>   3 files changed, 136 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
>> index 458218f88c5e..f74846ebc177 100644
>> --- a/drivers/ptp/Kconfig
>> +++ b/drivers/ptp/Kconfig
>> @@ -176,6 +176,7 @@ config PTP_1588_CLOCK_OCP
>>   	depends on !S390
>>   	depends on COMMON_CLK
>>   	select NET_DEVLINK
>> +	select DPLL
>>   	help
>>   	  This driver adds support for an OpenCompute time card.
>>   
>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>> index e59ea2173aac..f830625a63a3 100644
>> --- a/drivers/ptp/ptp_ocp.c
>> +++ b/drivers/ptp/ptp_ocp.c
>> @@ -21,6 +21,7 @@
>>   #include <linux/mtd/mtd.h>
>>   #include <linux/nvmem-consumer.h>
>>   #include <linux/crc16.h>
>> +#include <uapi/linux/dpll.h>
>>   
>>   #define PCI_VENDOR_ID_FACEBOOK			0x1d9b
>>   #define PCI_DEVICE_ID_FACEBOOK_TIMECARD		0x0400
>> @@ -336,6 +337,7 @@ struct ptp_ocp {
>>   	struct ptp_ocp_signal	signal[4];
>>   	struct ptp_ocp_sma_connector sma[4];
>>   	const struct ocp_sma_op *sma_op;
>> +	struct dpll_device *dpll;
>>   };
>>   
>>   #define OCP_REQ_TIMESTAMP	BIT(0)
>> @@ -660,18 +662,19 @@ static DEFINE_IDR(ptp_ocp_idr);
>>   struct ocp_selector {
>>   	const char *name;
>>   	int value;
>> +	int dpll_type;
>>   };
>>   
>>   static const struct ocp_selector ptp_ocp_clock[] = {
>> -	{ .name = "NONE",	.value = 0 },
>> -	{ .name = "TOD",	.value = 1 },
>> -	{ .name = "IRIG",	.value = 2 },
>> -	{ .name = "PPS",	.value = 3 },
>> -	{ .name = "PTP",	.value = 4 },
>> -	{ .name = "RTC",	.value = 5 },
>> -	{ .name = "DCF",	.value = 6 },
>> -	{ .name = "REGS",	.value = 0xfe },
>> -	{ .name = "EXT",	.value = 0xff },
>> +	{ .name = "NONE",	.value = 0,		.dpll_type = 0 },
>> +	{ .name = "TOD",	.value = 1,		.dpll_type = 0 },
>> +	{ .name = "IRIG",	.value = 2,		.dpll_type = 0 },
>> +	{ .name = "PPS",	.value = 3,		.dpll_type = 0 },
>> +	{ .name = "PTP",	.value = 4,		.dpll_type = 0 },
>> +	{ .name = "RTC",	.value = 5,		.dpll_type = 0 },
>> +	{ .name = "DCF",	.value = 6,		.dpll_type = 0 },
>> +	{ .name = "REGS",	.value = 0xfe,		.dpll_type = 0 },
>> +	{ .name = "EXT",	.value = 0xff,		.dpll_type = 0 },
> 
> No need for zeros, or are they just temp stubs?

These are just temp stubs for now.

> 
>> @@ -680,37 +683,37 @@ static const struct ocp_selector ptp_ocp_clock[] = {
>>   #define SMA_SELECT_MASK		GENMASK(14, 0)
>>   
>>   static const struct ocp_selector ptp_ocp_sma_in[] = {
>> -	{ .name = "10Mhz",	.value = 0x0000 },
>> -	{ .name = "PPS1",	.value = 0x0001 },
>> -	{ .name = "PPS2",	.value = 0x0002 },
>> -	{ .name = "TS1",	.value = 0x0004 },
>> -	{ .name = "TS2",	.value = 0x0008 },
>> -	{ .name = "IRIG",	.value = 0x0010 },
>> -	{ .name = "DCF",	.value = 0x0020 },
>> -	{ .name = "TS3",	.value = 0x0040 },
>> -	{ .name = "TS4",	.value = 0x0080 },
>> -	{ .name = "FREQ1",	.value = 0x0100 },
>> -	{ .name = "FREQ2",	.value = 0x0200 },
>> -	{ .name = "FREQ3",	.value = 0x0400 },
>> -	{ .name = "FREQ4",	.value = 0x0800 },
>> -	{ .name = "None",	.value = SMA_DISABLE },
>> +	{ .name = "10Mhz",	.value = 0x0000,	.dpll_type = DPLL_TYPE_EXT_10MHZ },
>> +	{ .name = "PPS1",	.value = 0x0001,	.dpll_type = DPLL_TYPE_EXT_1PPS },
>> +	{ .name = "PPS2",	.value = 0x0002,	.dpll_type = DPLL_TYPE_EXT_1PPS },
>> +	{ .name = "TS1",	.value = 0x0004,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "TS2",	.value = 0x0008,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "IRIG",	.value = 0x0010,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "DCF",	.value = 0x0020,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "TS3",	.value = 0x0040,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "TS4",	.value = 0x0080,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "FREQ1",	.value = 0x0100,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "FREQ2",	.value = 0x0200,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "FREQ3",	.value = 0x0400,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "FREQ4",	.value = 0x0800,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "None",	.value = SMA_DISABLE,	.dpll_type = DPLL_TYPE_NONE },
> 
> 80-column limit (here and throughout the file)

I thought this rule was relaxed up to 100-columns?

> 
> 
>>   static const struct ocp_selector ptp_ocp_sma_out[] = {
>> -	{ .name = "10Mhz",	.value = 0x0000 },
>> -	{ .name = "PHC",	.value = 0x0001 },
>> -	{ .name = "MAC",	.value = 0x0002 },
>> -	{ .name = "GNSS1",	.value = 0x0004 },
>> -	{ .name = "GNSS2",	.value = 0x0008 },
>> -	{ .name = "IRIG",	.value = 0x0010 },
>> -	{ .name = "DCF",	.value = 0x0020 },
>> -	{ .name = "GEN1",	.value = 0x0040 },
>> -	{ .name = "GEN2",	.value = 0x0080 },
>> -	{ .name = "GEN3",	.value = 0x0100 },
>> -	{ .name = "GEN4",	.value = 0x0200 },
>> -	{ .name = "GND",	.value = 0x2000 },
>> -	{ .name = "VCC",	.value = 0x4000 },
>> +	{ .name = "10Mhz",	.value = 0x0000,	.dpll_type = DPLL_TYPE_EXT_10MHZ },
>> +	{ .name = "PHC",	.value = 0x0001,	.dpll_type = DPLL_TYPE_INT_OSCILLATOR },
>> +	{ .name = "MAC",	.value = 0x0002,	.dpll_type = DPLL_TYPE_INT_OSCILLATOR },
> 
> How does this work?  The DPLL types seem somewhat restrictive here.

Well, this is more like stubs, but a bit informative. I think I have to 
implement additional attribute to address specifics like second GNSS receiver 
and different oscillators (PHC/MAC) as well as other configurable values.

> 
> 
>> +	{ .name = "GNSS1",	.value = 0x0004,	.dpll_type = DPLL_TYPE_GNSS },
>> +	{ .name = "GNSS2",	.value = 0x0008,	.dpll_type = DPLL_TYPE_GNSS },
>> +	{ .name = "IRIG",	.value = 0x0010,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "DCF",	.value = 0x0020,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "GEN1",	.value = 0x0040,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "GEN2",	.value = 0x0080,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "GEN3",	.value = 0x0100,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "GEN4",	.value = 0x0200,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "GND",	.value = 0x2000,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "VCC",	.value = 0x4000,	.dpll_type = DPLL_TYPE_CUSTOM },
> 
> 80-columns
> 
> 
>>   
>> @@ -3713,6 +3716,90 @@ ptp_ocp_detach(struct ptp_ocp *bp)
>>   	device_unregister(&bp->dev);
>>   }
>>   
>> +static int ptp_ocp_dpll_get_status(struct dpll_device *dpll)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +	int sync;
>> +
>> +	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
>> +	return sync;
>> +}
>> +
>> +static int ptp_ocp_dpll_get_lock_status(struct dpll_device *dpll)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +	int sync;
>> +
>> +	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
>> +	return sync;
>> +}
>> +
>> +static int ptp_ocp_sma_get_dpll_type(struct ptp_ocp *bp, int sma_nr)
>> +{
>> +	struct ocp_selector *tbl;
>> +	u32 val;
>> +
>> +	if (bp->sma[sma_nr].mode == SMA_MODE_IN)
>> +		tbl = bp->sma_op->tbl[0];
>> +	else
>> +		tbl = bp->sma_op->tbl[1];
>> +
>> +	val = ptp_ocp_sma_get(bp, sma_nr);
>> +	return tbl[val].dpll_type;
>> +}
>> +
>> +static int ptp_ocp_dpll_type_supported(struct dpll_device *dpll, int sma, int type, int dir)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +	struct ocp_selector *tbl = bp->sma_op->tbl[dir];
>> +	int i;
>> +
>> +	for (i = 0; i < sizeof(*tbl); i++) {
>> +		if (tbl[i].dpll_type == type)
>> +			return 1;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +
>> +	if (bp->sma[sma].mode != SMA_MODE_IN)
>> +		return -1;
>> +
>> +	return ptp_ocp_sma_get_dpll_type(bp, sma);
>> +}
>> +
>> +static int ptp_ocp_dpll_get_source_supported(struct dpll_device *dpll, int sma, int type)
>> +{
>> +	return ptp_ocp_dpll_type_supported(dpll, sma, type, 0);
>> +}
>> +
>> +static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +
>> +	if (bp->sma[sma].mode != SMA_MODE_OUT)
>> +		return -1;
>> +
>> +	return ptp_ocp_sma_get_dpll_type(bp, sma);
>> +}
>> +
>> +static int ptp_ocp_dpll_get_output_supported(struct dpll_device *dpll, int sma, int type)
>> +{
>> +	return ptp_ocp_dpll_type_supported(dpll, sma, type, 1);
>> +}
>> +
>> +static struct dpll_device_ops dpll_ops = {
>> +	.get_status		= ptp_ocp_dpll_get_status,
>> +	.get_lock_status	= ptp_ocp_dpll_get_lock_status,
>> +	.get_source_type	= ptp_ocp_dpll_get_source_type,
>> +	.get_source_supported	= ptp_ocp_dpll_get_source_supported,
>> +	.get_output_type	= ptp_ocp_dpll_get_output_type,
>> +	.get_output_supported	= ptp_ocp_dpll_get_output_supported,
>> +};
>> +
>>   static int
>>   ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   {
>> @@ -3768,6 +3855,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>   
>>   	ptp_ocp_info(bp);
>>   	devlink_register(devlink);
>> +
>> +	bp->dpll = dpll_device_alloc(&dpll_ops, ARRAY_SIZE(bp->sma), ARRAY_SIZE(bp->sma), bp);
>> +	if (!bp->dpll) {
>> +		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
>> +		return 0;
>> +	}
>> +	dpll_device_register(bp->dpll);
>> +
>>   	return 0;
> 
> 80 cols, and this should be done before ptp_ocp_complete()
> Also, should 'goto out', not return 0 and leak resources.

I don't think we have to go with error path. Driver itself can work without DPLL 
device registered, there is no hard dependency. The DPLL device will not be 
registered and HW could not be configured/monitored via netlink, but could still 
be usable.

> 
> 
>>   
>>   out:
>> @@ -3785,6 +3880,8 @@ ptp_ocp_remove(struct pci_dev *pdev)
>>   	struct ptp_ocp *bp = pci_get_drvdata(pdev);
>>   	struct devlink *devlink = priv_to_devlink(bp);
>>   
>> +	dpll_device_unregister(bp->dpll);
>> +	dpll_device_free(bp->dpll);
>>   	devlink_unregister(devlink);
>>   	ptp_ocp_detach(bp);
>>   	pci_disable_device(pdev);
> 
> This should be in ptp_ocp_detach() to handle probe failures.

Will move it in the next version, thanks!
> 
> 
>> diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>> index 7ce45c6b4fd4..5e8c46712d18 100644
>> --- a/include/uapi/linux/dpll.h
>> +++ b/include/uapi/linux/dpll.h
>> @@ -41,11 +41,13 @@ enum dpll_genl_attr {
>>   
>>   /* DPLL signal types used as source or as output */
>>   enum dpll_genl_signal_type {
>> +	DPLL_TYPE_NONE,
>>   	DPLL_TYPE_EXT_1PPS,
>>   	DPLL_TYPE_EXT_10MHZ,
>>   	DPLL_TYPE_SYNCE_ETH_PORT,
>>   	DPLL_TYPE_INT_OSCILLATOR,
>>   	DPLL_TYPE_GNSS,
>> +	DPLL_TYPE_CUSTOM,
>>   
>>   	__DPLL_TYPE_MAX,
>>   };
>> -- 
>> 2.27.0
>>
Jonathan Lemon June 28, 2022, 7:11 p.m. UTC | #3 
On Mon, Jun 27, 2022 at 11:13:22PM +0100, Vadim Fedorenko wrote:
> On 27.06.2022 20:34, Jonathan Lemon wrote:
> > On Sun, Jun 26, 2022 at 10:24:44PM +0300, Vadim Fedorenko wrote:
> > > From: Vadim Fedorenko <vadfed@fb.com>
> > > 
> > > Implement DPLL operations in ptp_ocp driver.
> > > 
> > > Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
> > > ---
> > >   drivers/ptp/Kconfig       |   1 +
> > >   drivers/ptp/ptp_ocp.c     | 169 ++++++++++++++++++++++++++++++--------
> > >   include/uapi/linux/dpll.h |   2 +
> > >   3 files changed, 136 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
> > > index 458218f88c5e..f74846ebc177 100644
> > > --- a/drivers/ptp/Kconfig
> > > +++ b/drivers/ptp/Kconfig
> > > @@ -176,6 +176,7 @@ config PTP_1588_CLOCK_OCP
> > >   	depends on !S390
> > >   	depends on COMMON_CLK
> > >   	select NET_DEVLINK
> > > +	select DPLL
> > >   	help
> > >   	  This driver adds support for an OpenCompute time card.
> > > diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> > > index e59ea2173aac..f830625a63a3 100644
> > > --- a/drivers/ptp/ptp_ocp.c
> > > +++ b/drivers/ptp/ptp_ocp.c
> > > @@ -21,6 +21,7 @@
> > >   #include <linux/mtd/mtd.h>
> > >   #include <linux/nvmem-consumer.h>
> > >   #include <linux/crc16.h>
> > > +#include <uapi/linux/dpll.h>
> > >   #define PCI_VENDOR_ID_FACEBOOK			0x1d9b
> > >   #define PCI_DEVICE_ID_FACEBOOK_TIMECARD		0x0400
> > > @@ -336,6 +337,7 @@ struct ptp_ocp {
> > >   	struct ptp_ocp_signal	signal[4];
> > >   	struct ptp_ocp_sma_connector sma[4];
> > >   	const struct ocp_sma_op *sma_op;
> > > +	struct dpll_device *dpll;
> > >   };
> > >   #define OCP_REQ_TIMESTAMP	BIT(0)
> > > @@ -660,18 +662,19 @@ static DEFINE_IDR(ptp_ocp_idr);
> > >   struct ocp_selector {
> > >   	const char *name;
> > >   	int value;
> > > +	int dpll_type;
> > >   };
> > >   static const struct ocp_selector ptp_ocp_clock[] = {
> > > -	{ .name = "NONE",	.value = 0 },
> > > -	{ .name = "TOD",	.value = 1 },
> > > -	{ .name = "IRIG",	.value = 2 },
> > > -	{ .name = "PPS",	.value = 3 },
> > > -	{ .name = "PTP",	.value = 4 },
> > > -	{ .name = "RTC",	.value = 5 },
> > > -	{ .name = "DCF",	.value = 6 },
> > > -	{ .name = "REGS",	.value = 0xfe },
> > > -	{ .name = "EXT",	.value = 0xff },
> > > +	{ .name = "NONE",	.value = 0,		.dpll_type = 0 },
> > > +	{ .name = "TOD",	.value = 1,		.dpll_type = 0 },
> > > +	{ .name = "IRIG",	.value = 2,		.dpll_type = 0 },
> > > +	{ .name = "PPS",	.value = 3,		.dpll_type = 0 },
> > > +	{ .name = "PTP",	.value = 4,		.dpll_type = 0 },
> > > +	{ .name = "RTC",	.value = 5,		.dpll_type = 0 },
> > > +	{ .name = "DCF",	.value = 6,		.dpll_type = 0 },
> > > +	{ .name = "REGS",	.value = 0xfe,		.dpll_type = 0 },
> > > +	{ .name = "EXT",	.value = 0xff,		.dpll_type = 0 },
> > 
> > No need for zeros, or are they just temp stubs?
> 
> These are just temp stubs for now.
> 
> > 
> > > @@ -680,37 +683,37 @@ static const struct ocp_selector ptp_ocp_clock[] = {
> > >   #define SMA_SELECT_MASK		GENMASK(14, 0)
> > >   static const struct ocp_selector ptp_ocp_sma_in[] = {
> > > -	{ .name = "10Mhz",	.value = 0x0000 },
> > > -	{ .name = "PPS1",	.value = 0x0001 },
> > > -	{ .name = "PPS2",	.value = 0x0002 },
> > > -	{ .name = "TS1",	.value = 0x0004 },
> > > -	{ .name = "TS2",	.value = 0x0008 },
> > > -	{ .name = "IRIG",	.value = 0x0010 },
> > > -	{ .name = "DCF",	.value = 0x0020 },
> > > -	{ .name = "TS3",	.value = 0x0040 },
> > > -	{ .name = "TS4",	.value = 0x0080 },
> > > -	{ .name = "FREQ1",	.value = 0x0100 },
> > > -	{ .name = "FREQ2",	.value = 0x0200 },
> > > -	{ .name = "FREQ3",	.value = 0x0400 },
> > > -	{ .name = "FREQ4",	.value = 0x0800 },
> > > -	{ .name = "None",	.value = SMA_DISABLE },
> > > +	{ .name = "10Mhz",	.value = 0x0000,	.dpll_type = DPLL_TYPE_EXT_10MHZ },
> > > +	{ .name = "PPS1",	.value = 0x0001,	.dpll_type = DPLL_TYPE_EXT_1PPS },
> > > +	{ .name = "PPS2",	.value = 0x0002,	.dpll_type = DPLL_TYPE_EXT_1PPS },
> > > +	{ .name = "TS1",	.value = 0x0004,	.dpll_type = DPLL_TYPE_CUSTOM },
> > > +	{ .name = "TS2",	.value = 0x0008,	.dpll_type = DPLL_TYPE_CUSTOM },
> > > +	{ .name = "IRIG",	.value = 0x0010,	.dpll_type = DPLL_TYPE_CUSTOM },
> > > +	{ .name = "DCF",	.value = 0x0020,	.dpll_type = DPLL_TYPE_CUSTOM },
> > > +	{ .name = "TS3",	.value = 0x0040,	.dpll_type = DPLL_TYPE_CUSTOM },
> > > +	{ .name = "TS4",	.value = 0x0080,	.dpll_type = DPLL_TYPE_CUSTOM },
> > > +	{ .name = "FREQ1",	.value = 0x0100,	.dpll_type = DPLL_TYPE_CUSTOM },
> > > +	{ .name = "FREQ2",	.value = 0x0200,	.dpll_type = DPLL_TYPE_CUSTOM },
> > > +	{ .name = "FREQ3",	.value = 0x0400,	.dpll_type = DPLL_TYPE_CUSTOM },
> > > +	{ .name = "FREQ4",	.value = 0x0800,	.dpll_type = DPLL_TYPE_CUSTOM },
> > > +	{ .name = "None",	.value = SMA_DISABLE,	.dpll_type = DPLL_TYPE_NONE },
> > 
> > 80-column limit (here and throughout the file)
> 
> I thought this rule was relaxed up to 100-columns?

Only in exceptional cases, IIRC.  checkpatch complains too.


> > >   static int
> > >   ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > >   {
> > > @@ -3768,6 +3855,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > >   	ptp_ocp_info(bp);
> > >   	devlink_register(devlink);
> > > +
> > > +	bp->dpll = dpll_device_alloc(&dpll_ops, ARRAY_SIZE(bp->sma), ARRAY_SIZE(bp->sma), bp);
> > > +	if (!bp->dpll) {
> > > +		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
> > > +		return 0;
> > > +	}
> > > +	dpll_device_register(bp->dpll);
> > > +
> > >   	return 0;
> > 
> > 80 cols, and this should be done before ptp_ocp_complete()
> > Also, should 'goto out', not return 0 and leak resources.
> 
> I don't think we have to go with error path. Driver itself can work without
> DPLL device registered, there is no hard dependency. The DPLL device will
> not be registered and HW could not be configured/monitored via netlink, but
> could still be usable.

Not sure I agree with that - the DPLL device is selected in Kconfig, so
users would expect to have it present.  I think it makes more sense to
fail if it cannot be allocated.
Jakub Kicinski June 29, 2022, 3:24 a.m. UTC | #4 
On Tue, 28 Jun 2022 12:11:24 -0700 Jonathan Lemon wrote:
> > > 80-column limit (here and throughout the file)  
> > 
> > I thought this rule was relaxed up to 100-columns?  
> 
> Only in exceptional cases, IIRC.  checkpatch complains too.

Yup, for networking I still prefer 80 chars. 
My field of vision is narrow.

> > > 80 cols, and this should be done before ptp_ocp_complete()
> > > Also, should 'goto out', not return 0 and leak resources.  
> > 
> > I don't think we have to go with error path. Driver itself can work without
> > DPLL device registered, there is no hard dependency. The DPLL device will
> > not be registered and HW could not be configured/monitored via netlink, but
> > could still be usable.  
> 
> Not sure I agree with that - the DPLL device is selected in Kconfig, so
> users would expect to have it present.  I think it makes more sense to
> fail if it cannot be allocated.

+1
Vadim Fedorenko June 29, 2022, 11:31 p.m. UTC | #5 
On 29.06.2022 04:24, Jakub Kicinski wrote:
> On Tue, 28 Jun 2022 12:11:24 -0700 Jonathan Lemon wrote:
>>>> 80-column limit (here and throughout the file)
>>>
>>> I thought this rule was relaxed up to 100-columns?
>>
>> Only in exceptional cases, IIRC.  checkpatch complains too.
> 
> Yup, for networking I still prefer 80 chars.
> My field of vision is narrow.
> 
Ok, no problem, will follow strict rules next time.


>>>> 80 cols, and this should be done before ptp_ocp_complete()
>>>> Also, should 'goto out', not return 0 and leak resources.
>>>
>>> I don't think we have to go with error path. Driver itself can work without
>>> DPLL device registered, there is no hard dependency. The DPLL device will
>>> not be registered and HW could not be configured/monitored via netlink, but
>>> could still be usable.
>>
>> Not sure I agree with that - the DPLL device is selected in Kconfig, so
>> users would expect to have it present.  I think it makes more sense to
>> fail if it cannot be allocated.
> 
> +1

Ok, it's not a big deal to make it fail in case of DPLL error, will do it in 
next iteration.
Jiri Pirko Sept. 29, 2022, 11:33 a.m. UTC | #6 
Sun, Jun 26, 2022 at 09:24:44PM CEST, vfedorenko@novek.ru wrote:
>From: Vadim Fedorenko <vadfed@fb.com>
>
>Implement DPLL operations in ptp_ocp driver.
>
>Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
>---
> drivers/ptp/Kconfig       |   1 +
> drivers/ptp/ptp_ocp.c     | 169 ++++++++++++++++++++++++++++++--------
> include/uapi/linux/dpll.h |   2 +
> 3 files changed, 136 insertions(+), 36 deletions(-)
>
>diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
>index 458218f88c5e..f74846ebc177 100644
>--- a/drivers/ptp/Kconfig
>+++ b/drivers/ptp/Kconfig
>@@ -176,6 +176,7 @@ config PTP_1588_CLOCK_OCP
> 	depends on !S390
> 	depends on COMMON_CLK
> 	select NET_DEVLINK
>+	select DPLL
> 	help
> 	  This driver adds support for an OpenCompute time card.
> 
>diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>index e59ea2173aac..f830625a63a3 100644
>--- a/drivers/ptp/ptp_ocp.c
>+++ b/drivers/ptp/ptp_ocp.c
>@@ -21,6 +21,7 @@
> #include <linux/mtd/mtd.h>
> #include <linux/nvmem-consumer.h>
> #include <linux/crc16.h>
>+#include <uapi/linux/dpll.h>
> 
> #define PCI_VENDOR_ID_FACEBOOK			0x1d9b
> #define PCI_DEVICE_ID_FACEBOOK_TIMECARD		0x0400
>@@ -336,6 +337,7 @@ struct ptp_ocp {
> 	struct ptp_ocp_signal	signal[4];
> 	struct ptp_ocp_sma_connector sma[4];
> 	const struct ocp_sma_op *sma_op;
>+	struct dpll_device *dpll;
> };
> 
> #define OCP_REQ_TIMESTAMP	BIT(0)
>@@ -660,18 +662,19 @@ static DEFINE_IDR(ptp_ocp_idr);
> struct ocp_selector {
> 	const char *name;
> 	int value;
>+	int dpll_type;
> };
> 
> static const struct ocp_selector ptp_ocp_clock[] = {
>-	{ .name = "NONE",	.value = 0 },
>-	{ .name = "TOD",	.value = 1 },
>-	{ .name = "IRIG",	.value = 2 },
>-	{ .name = "PPS",	.value = 3 },
>-	{ .name = "PTP",	.value = 4 },
>-	{ .name = "RTC",	.value = 5 },
>-	{ .name = "DCF",	.value = 6 },
>-	{ .name = "REGS",	.value = 0xfe },
>-	{ .name = "EXT",	.value = 0xff },
>+	{ .name = "NONE",	.value = 0,		.dpll_type = 0 },
>+	{ .name = "TOD",	.value = 1,		.dpll_type = 0 },
>+	{ .name = "IRIG",	.value = 2,		.dpll_type = 0 },
>+	{ .name = "PPS",	.value = 3,		.dpll_type = 0 },
>+	{ .name = "PTP",	.value = 4,		.dpll_type = 0 },
>+	{ .name = "RTC",	.value = 5,		.dpll_type = 0 },
>+	{ .name = "DCF",	.value = 6,		.dpll_type = 0 },
>+	{ .name = "REGS",	.value = 0xfe,		.dpll_type = 0 },
>+	{ .name = "EXT",	.value = 0xff,		.dpll_type = 0 },
> 	{ }
> };
> 
>@@ -680,37 +683,37 @@ static const struct ocp_selector ptp_ocp_clock[] = {
> #define SMA_SELECT_MASK		GENMASK(14, 0)
> 
> static const struct ocp_selector ptp_ocp_sma_in[] = {
>-	{ .name = "10Mhz",	.value = 0x0000 },
>-	{ .name = "PPS1",	.value = 0x0001 },
>-	{ .name = "PPS2",	.value = 0x0002 },
>-	{ .name = "TS1",	.value = 0x0004 },
>-	{ .name = "TS2",	.value = 0x0008 },
>-	{ .name = "IRIG",	.value = 0x0010 },
>-	{ .name = "DCF",	.value = 0x0020 },
>-	{ .name = "TS3",	.value = 0x0040 },
>-	{ .name = "TS4",	.value = 0x0080 },
>-	{ .name = "FREQ1",	.value = 0x0100 },
>-	{ .name = "FREQ2",	.value = 0x0200 },
>-	{ .name = "FREQ3",	.value = 0x0400 },
>-	{ .name = "FREQ4",	.value = 0x0800 },
>-	{ .name = "None",	.value = SMA_DISABLE },
>+	{ .name = "10Mhz",	.value = 0x0000,	.dpll_type = DPLL_TYPE_EXT_10MHZ },
>+	{ .name = "PPS1",	.value = 0x0001,	.dpll_type = DPLL_TYPE_EXT_1PPS },
>+	{ .name = "PPS2",	.value = 0x0002,	.dpll_type = DPLL_TYPE_EXT_1PPS },
>+	{ .name = "TS1",	.value = 0x0004,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "TS2",	.value = 0x0008,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "IRIG",	.value = 0x0010,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "DCF",	.value = 0x0020,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "TS3",	.value = 0x0040,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "TS4",	.value = 0x0080,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "FREQ1",	.value = 0x0100,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "FREQ2",	.value = 0x0200,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "FREQ3",	.value = 0x0400,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "FREQ4",	.value = 0x0800,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "None",	.value = SMA_DISABLE,	.dpll_type = DPLL_TYPE_NONE },
> 	{ }
> };
> 
> static const struct ocp_selector ptp_ocp_sma_out[] = {
>-	{ .name = "10Mhz",	.value = 0x0000 },
>-	{ .name = "PHC",	.value = 0x0001 },
>-	{ .name = "MAC",	.value = 0x0002 },
>-	{ .name = "GNSS1",	.value = 0x0004 },
>-	{ .name = "GNSS2",	.value = 0x0008 },
>-	{ .name = "IRIG",	.value = 0x0010 },
>-	{ .name = "DCF",	.value = 0x0020 },
>-	{ .name = "GEN1",	.value = 0x0040 },
>-	{ .name = "GEN2",	.value = 0x0080 },
>-	{ .name = "GEN3",	.value = 0x0100 },
>-	{ .name = "GEN4",	.value = 0x0200 },
>-	{ .name = "GND",	.value = 0x2000 },
>-	{ .name = "VCC",	.value = 0x4000 },
>+	{ .name = "10Mhz",	.value = 0x0000,	.dpll_type = DPLL_TYPE_EXT_10MHZ },
>+	{ .name = "PHC",	.value = 0x0001,	.dpll_type = DPLL_TYPE_INT_OSCILLATOR },
>+	{ .name = "MAC",	.value = 0x0002,	.dpll_type = DPLL_TYPE_INT_OSCILLATOR },
>+	{ .name = "GNSS1",	.value = 0x0004,	.dpll_type = DPLL_TYPE_GNSS },
>+	{ .name = "GNSS2",	.value = 0x0008,	.dpll_type = DPLL_TYPE_GNSS },
>+	{ .name = "IRIG",	.value = 0x0010,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "DCF",	.value = 0x0020,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "GEN1",	.value = 0x0040,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "GEN2",	.value = 0x0080,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "GEN3",	.value = 0x0100,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "GEN4",	.value = 0x0200,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "GND",	.value = 0x2000,	.dpll_type = DPLL_TYPE_CUSTOM },
>+	{ .name = "VCC",	.value = 0x4000,	.dpll_type = DPLL_TYPE_CUSTOM },
> 	{ }
> };
> 
>@@ -3713,6 +3716,90 @@ ptp_ocp_detach(struct ptp_ocp *bp)
> 	device_unregister(&bp->dev);
> }
> 
>+static int ptp_ocp_dpll_get_status(struct dpll_device *dpll)
>+{
>+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);

Unnecessary cast.


>+	int sync;
>+
>+	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;

Just return without having "sync" variable.

>+	return sync;
>+}
>+
>+static int ptp_ocp_dpll_get_lock_status(struct dpll_device *dpll)
>+{
>+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>+	int sync;
>+
>+	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
>+	return sync;

This is very odd. You return something you read from device directly
over what likes to be an abstract API. Is it supposed to be a bool? If
yes, make it so.


>+}
>+
>+static int ptp_ocp_sma_get_dpll_type(struct ptp_ocp *bp, int sma_nr)

Type should be most probably enum instead of int.


>+{
>+	struct ocp_selector *tbl;
>+	u32 val;
>+
>+	if (bp->sma[sma_nr].mode == SMA_MODE_IN)
>+		tbl = bp->sma_op->tbl[0];
>+	else
>+		tbl = bp->sma_op->tbl[1];
>+
>+	val = ptp_ocp_sma_get(bp, sma_nr);
>+	return tbl[val].dpll_type;
>+}
>+
>+static int ptp_ocp_dpll_type_supported(struct dpll_device *dpll, int sma, int type, int dir)
>+{
>+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>+	struct ocp_selector *tbl = bp->sma_op->tbl[dir];
>+	int i;
>+
>+	for (i = 0; i < sizeof(*tbl); i++) {
>+		if (tbl[i].dpll_type == type)
>+			return 1;
>+	}
>+	return 0;

1/0? Bool please.


>+}
>+
>+static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
>+{
>+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>+
>+	if (bp->sma[sma].mode != SMA_MODE_IN)
>+		return -1;
>+
>+	return ptp_ocp_sma_get_dpll_type(bp, sma);

enum.


>+}
>+
>+static int ptp_ocp_dpll_get_source_supported(struct dpll_device *dpll, int sma, int type)
>+{
>+	return ptp_ocp_dpll_type_supported(dpll, sma, type, 0);
>+}
>+
>+static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
>+{
>+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>+
>+	if (bp->sma[sma].mode != SMA_MODE_OUT)
>+		return -1;
>+
>+	return ptp_ocp_sma_get_dpll_type(bp, sma);
>+}
>+
>+static int ptp_ocp_dpll_get_output_supported(struct dpll_device *dpll, int sma, int type)
>+{
>+	return ptp_ocp_dpll_type_supported(dpll, sma, type, 1);

1? Have the "dir" to be enum please.


>+}
>+
>+static struct dpll_device_ops dpll_ops = {
>+	.get_status		= ptp_ocp_dpll_get_status,
>+	.get_lock_status	= ptp_ocp_dpll_get_lock_status,
>+	.get_source_type	= ptp_ocp_dpll_get_source_type,
>+	.get_source_supported	= ptp_ocp_dpll_get_source_supported,
>+	.get_output_type	= ptp_ocp_dpll_get_output_type,
>+	.get_output_supported	= ptp_ocp_dpll_get_output_supported,
>+};
>+
> static int
> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
>@@ -3768,6 +3855,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> 
> 	ptp_ocp_info(bp);
> 	devlink_register(devlink);
>+
>+	bp->dpll = dpll_device_alloc(&dpll_ops, ARRAY_SIZE(bp->sma), ARRAY_SIZE(bp->sma), bp);
>+	if (!bp->dpll) {
>+		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
>+		return 0;
>+	}
>+	dpll_device_register(bp->dpll);

I wonder, why you don't have alloc-register and unregister-free
squashed?


>+
> 	return 0;
> 
> out:
>@@ -3785,6 +3880,8 @@ ptp_ocp_remove(struct pci_dev *pdev)
> 	struct ptp_ocp *bp = pci_get_drvdata(pdev);
> 	struct devlink *devlink = priv_to_devlink(bp);
> 
>+	dpll_device_unregister(bp->dpll);
>+	dpll_device_free(bp->dpll);
> 	devlink_unregister(devlink);
> 	ptp_ocp_detach(bp);
> 	pci_disable_device(pdev);
>diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>index 7ce45c6b4fd4..5e8c46712d18 100644
>--- a/include/uapi/linux/dpll.h
>+++ b/include/uapi/linux/dpll.h
>@@ -41,11 +41,13 @@ enum dpll_genl_attr {
> 
> /* DPLL signal types used as source or as output */
> enum dpll_genl_signal_type {
>+	DPLL_TYPE_NONE,
> 	DPLL_TYPE_EXT_1PPS,
> 	DPLL_TYPE_EXT_10MHZ,
> 	DPLL_TYPE_SYNCE_ETH_PORT,
> 	DPLL_TYPE_INT_OSCILLATOR,
> 	DPLL_TYPE_GNSS,
>+	DPLL_TYPE_CUSTOM,

This hunk does not belong to this patch.


> 
> 	__DPLL_TYPE_MAX,
> };
>-- 
>2.27.0
>
Vadim Fedorenko Sept. 30, 2022, 12:56 a.m. UTC | #7 
On 29.09.2022 12:33, Jiri Pirko wrote:
> Sun, Jun 26, 2022 at 09:24:44PM CEST, vfedorenko@novek.ru wrote:
>> From: Vadim Fedorenko <vadfed@fb.com>
>>
>> Implement DPLL operations in ptp_ocp driver.
>>
>> Signed-off-by: Vadim Fedorenko <vadfed@fb.com>
>> ---
>> drivers/ptp/Kconfig       |   1 +
>> drivers/ptp/ptp_ocp.c     | 169 ++++++++++++++++++++++++++++++--------
>> include/uapi/linux/dpll.h |   2 +
>> 3 files changed, 136 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
>> index 458218f88c5e..f74846ebc177 100644
>> --- a/drivers/ptp/Kconfig
>> +++ b/drivers/ptp/Kconfig
>> @@ -176,6 +176,7 @@ config PTP_1588_CLOCK_OCP
>> 	depends on !S390
>> 	depends on COMMON_CLK
>> 	select NET_DEVLINK
>> +	select DPLL
>> 	help
>> 	  This driver adds support for an OpenCompute time card.
>>
>> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
>> index e59ea2173aac..f830625a63a3 100644
>> --- a/drivers/ptp/ptp_ocp.c
>> +++ b/drivers/ptp/ptp_ocp.c
>> @@ -21,6 +21,7 @@
>> #include <linux/mtd/mtd.h>
>> #include <linux/nvmem-consumer.h>
>> #include <linux/crc16.h>
>> +#include <uapi/linux/dpll.h>
>>
>> #define PCI_VENDOR_ID_FACEBOOK			0x1d9b
>> #define PCI_DEVICE_ID_FACEBOOK_TIMECARD		0x0400
>> @@ -336,6 +337,7 @@ struct ptp_ocp {
>> 	struct ptp_ocp_signal	signal[4];
>> 	struct ptp_ocp_sma_connector sma[4];
>> 	const struct ocp_sma_op *sma_op;
>> +	struct dpll_device *dpll;
>> };
>>
>> #define OCP_REQ_TIMESTAMP	BIT(0)
>> @@ -660,18 +662,19 @@ static DEFINE_IDR(ptp_ocp_idr);
>> struct ocp_selector {
>> 	const char *name;
>> 	int value;
>> +	int dpll_type;
>> };
>>
>> static const struct ocp_selector ptp_ocp_clock[] = {
>> -	{ .name = "NONE",	.value = 0 },
>> -	{ .name = "TOD",	.value = 1 },
>> -	{ .name = "IRIG",	.value = 2 },
>> -	{ .name = "PPS",	.value = 3 },
>> -	{ .name = "PTP",	.value = 4 },
>> -	{ .name = "RTC",	.value = 5 },
>> -	{ .name = "DCF",	.value = 6 },
>> -	{ .name = "REGS",	.value = 0xfe },
>> -	{ .name = "EXT",	.value = 0xff },
>> +	{ .name = "NONE",	.value = 0,		.dpll_type = 0 },
>> +	{ .name = "TOD",	.value = 1,		.dpll_type = 0 },
>> +	{ .name = "IRIG",	.value = 2,		.dpll_type = 0 },
>> +	{ .name = "PPS",	.value = 3,		.dpll_type = 0 },
>> +	{ .name = "PTP",	.value = 4,		.dpll_type = 0 },
>> +	{ .name = "RTC",	.value = 5,		.dpll_type = 0 },
>> +	{ .name = "DCF",	.value = 6,		.dpll_type = 0 },
>> +	{ .name = "REGS",	.value = 0xfe,		.dpll_type = 0 },
>> +	{ .name = "EXT",	.value = 0xff,		.dpll_type = 0 },
>> 	{ }
>> };
>>
>> @@ -680,37 +683,37 @@ static const struct ocp_selector ptp_ocp_clock[] = {
>> #define SMA_SELECT_MASK		GENMASK(14, 0)
>>
>> static const struct ocp_selector ptp_ocp_sma_in[] = {
>> -	{ .name = "10Mhz",	.value = 0x0000 },
>> -	{ .name = "PPS1",	.value = 0x0001 },
>> -	{ .name = "PPS2",	.value = 0x0002 },
>> -	{ .name = "TS1",	.value = 0x0004 },
>> -	{ .name = "TS2",	.value = 0x0008 },
>> -	{ .name = "IRIG",	.value = 0x0010 },
>> -	{ .name = "DCF",	.value = 0x0020 },
>> -	{ .name = "TS3",	.value = 0x0040 },
>> -	{ .name = "TS4",	.value = 0x0080 },
>> -	{ .name = "FREQ1",	.value = 0x0100 },
>> -	{ .name = "FREQ2",	.value = 0x0200 },
>> -	{ .name = "FREQ3",	.value = 0x0400 },
>> -	{ .name = "FREQ4",	.value = 0x0800 },
>> -	{ .name = "None",	.value = SMA_DISABLE },
>> +	{ .name = "10Mhz",	.value = 0x0000,	.dpll_type = DPLL_TYPE_EXT_10MHZ },
>> +	{ .name = "PPS1",	.value = 0x0001,	.dpll_type = DPLL_TYPE_EXT_1PPS },
>> +	{ .name = "PPS2",	.value = 0x0002,	.dpll_type = DPLL_TYPE_EXT_1PPS },
>> +	{ .name = "TS1",	.value = 0x0004,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "TS2",	.value = 0x0008,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "IRIG",	.value = 0x0010,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "DCF",	.value = 0x0020,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "TS3",	.value = 0x0040,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "TS4",	.value = 0x0080,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "FREQ1",	.value = 0x0100,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "FREQ2",	.value = 0x0200,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "FREQ3",	.value = 0x0400,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "FREQ4",	.value = 0x0800,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "None",	.value = SMA_DISABLE,	.dpll_type = DPLL_TYPE_NONE },
>> 	{ }
>> };
>>
>> static const struct ocp_selector ptp_ocp_sma_out[] = {
>> -	{ .name = "10Mhz",	.value = 0x0000 },
>> -	{ .name = "PHC",	.value = 0x0001 },
>> -	{ .name = "MAC",	.value = 0x0002 },
>> -	{ .name = "GNSS1",	.value = 0x0004 },
>> -	{ .name = "GNSS2",	.value = 0x0008 },
>> -	{ .name = "IRIG",	.value = 0x0010 },
>> -	{ .name = "DCF",	.value = 0x0020 },
>> -	{ .name = "GEN1",	.value = 0x0040 },
>> -	{ .name = "GEN2",	.value = 0x0080 },
>> -	{ .name = "GEN3",	.value = 0x0100 },
>> -	{ .name = "GEN4",	.value = 0x0200 },
>> -	{ .name = "GND",	.value = 0x2000 },
>> -	{ .name = "VCC",	.value = 0x4000 },
>> +	{ .name = "10Mhz",	.value = 0x0000,	.dpll_type = DPLL_TYPE_EXT_10MHZ },
>> +	{ .name = "PHC",	.value = 0x0001,	.dpll_type = DPLL_TYPE_INT_OSCILLATOR },
>> +	{ .name = "MAC",	.value = 0x0002,	.dpll_type = DPLL_TYPE_INT_OSCILLATOR },
>> +	{ .name = "GNSS1",	.value = 0x0004,	.dpll_type = DPLL_TYPE_GNSS },
>> +	{ .name = "GNSS2",	.value = 0x0008,	.dpll_type = DPLL_TYPE_GNSS },
>> +	{ .name = "IRIG",	.value = 0x0010,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "DCF",	.value = 0x0020,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "GEN1",	.value = 0x0040,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "GEN2",	.value = 0x0080,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "GEN3",	.value = 0x0100,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "GEN4",	.value = 0x0200,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "GND",	.value = 0x2000,	.dpll_type = DPLL_TYPE_CUSTOM },
>> +	{ .name = "VCC",	.value = 0x4000,	.dpll_type = DPLL_TYPE_CUSTOM },
>> 	{ }
>> };
>>
>> @@ -3713,6 +3716,90 @@ ptp_ocp_detach(struct ptp_ocp *bp)
>> 	device_unregister(&bp->dev);
>> }
>>
>> +static int ptp_ocp_dpll_get_status(struct dpll_device *dpll)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
> 
> Unnecessary cast.
> 
> 
>> +	int sync;
>> +
>> +	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
> 
> Just return without having "sync" variable.
> 
>> +	return sync;
>> +}
>> +
>> +static int ptp_ocp_dpll_get_lock_status(struct dpll_device *dpll)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +	int sync;
>> +
>> +	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
>> +	return sync;
> 
> This is very odd. You return something you read from device directly
> over what likes to be an abstract API. Is it supposed to be a bool? If
> yes, make it so.
> 

Thanks for pointing to the issue. I will make it boolean type, as well as other 
points.

> 
>> +}
>> +
>> +static int ptp_ocp_sma_get_dpll_type(struct ptp_ocp *bp, int sma_nr)
> 
> Type should be most probably enum instead of int.
> 

There is a process going on to reimplement the interface around inputs and 
outputs, and we are going to use enums for all such cases. But thanks for 
pointing it too, it feels like we are on the same wave.

> 
>> +{
>> +	struct ocp_selector *tbl;
>> +	u32 val;
>> +
>> +	if (bp->sma[sma_nr].mode == SMA_MODE_IN)
>> +		tbl = bp->sma_op->tbl[0];
>> +	else
>> +		tbl = bp->sma_op->tbl[1];
>> +
>> +	val = ptp_ocp_sma_get(bp, sma_nr);
>> +	return tbl[val].dpll_type;
>> +}
>> +
>> +static int ptp_ocp_dpll_type_supported(struct dpll_device *dpll, int sma, int type, int dir)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +	struct ocp_selector *tbl = bp->sma_op->tbl[dir];
>> +	int i;
>> +
>> +	for (i = 0; i < sizeof(*tbl); i++) {
>> +		if (tbl[i].dpll_type == type)
>> +			return 1;
>> +	}
>> +	return 0;
> 
> 1/0? Bool please.
> 
> 
>> +}
>> +
>> +static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +
>> +	if (bp->sma[sma].mode != SMA_MODE_IN)
>> +		return -1;
>> +
>> +	return ptp_ocp_sma_get_dpll_type(bp, sma);
> 
> enum.
> 
> 
>> +}
>> +
>> +static int ptp_ocp_dpll_get_source_supported(struct dpll_device *dpll, int sma, int type)
>> +{
>> +	return ptp_ocp_dpll_type_supported(dpll, sma, type, 0);
>> +}
>> +
>> +static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
>> +{
>> +	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
>> +
>> +	if (bp->sma[sma].mode != SMA_MODE_OUT)
>> +		return -1;
>> +
>> +	return ptp_ocp_sma_get_dpll_type(bp, sma);
>> +}
>> +
>> +static int ptp_ocp_dpll_get_output_supported(struct dpll_device *dpll, int sma, int type)
>> +{
>> +	return ptp_ocp_dpll_type_supported(dpll, sma, type, 1);
> 
> 1? Have the "dir" to be enum please.
> 
> 
>> +}
>> +
>> +static struct dpll_device_ops dpll_ops = {
>> +	.get_status		= ptp_ocp_dpll_get_status,
>> +	.get_lock_status	= ptp_ocp_dpll_get_lock_status,
>> +	.get_source_type	= ptp_ocp_dpll_get_source_type,
>> +	.get_source_supported	= ptp_ocp_dpll_get_source_supported,
>> +	.get_output_type	= ptp_ocp_dpll_get_output_type,
>> +	.get_output_supported	= ptp_ocp_dpll_get_output_supported,
>> +};
>> +
>> static int
>> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> {
>> @@ -3768,6 +3855,14 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>>
>> 	ptp_ocp_info(bp);
>> 	devlink_register(devlink);
>> +
>> +	bp->dpll = dpll_device_alloc(&dpll_ops, ARRAY_SIZE(bp->sma), ARRAY_SIZE(bp->sma), bp);
>> +	if (!bp->dpll) {
>> +		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
>> +		return 0;
>> +	}
>> +	dpll_device_register(bp->dpll);
> 
> I wonder, why you don't have alloc-register and unregister-free
> squashed?
> 
There were some concerns about how to deal with locking in case of squashing
these functions. But it looks like we will end up with special locks and then I 
will try to squash them.

> 
>> +
>> 	return 0;
>>
>> out:
>> @@ -3785,6 +3880,8 @@ ptp_ocp_remove(struct pci_dev *pdev)
>> 	struct ptp_ocp *bp = pci_get_drvdata(pdev);
>> 	struct devlink *devlink = priv_to_devlink(bp);
>>
>> +	dpll_device_unregister(bp->dpll);
>> +	dpll_device_free(bp->dpll);
>> 	devlink_unregister(devlink);
>> 	ptp_ocp_detach(bp);
>> 	pci_disable_device(pdev);
>> diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
>> index 7ce45c6b4fd4..5e8c46712d18 100644
>> --- a/include/uapi/linux/dpll.h
>> +++ b/include/uapi/linux/dpll.h
>> @@ -41,11 +41,13 @@ enum dpll_genl_attr {
>>
>> /* DPLL signal types used as source or as output */
>> enum dpll_genl_signal_type {
>> +	DPLL_TYPE_NONE,
>> 	DPLL_TYPE_EXT_1PPS,
>> 	DPLL_TYPE_EXT_10MHZ,
>> 	DPLL_TYPE_SYNCE_ETH_PORT,
>> 	DPLL_TYPE_INT_OSCILLATOR,
>> 	DPLL_TYPE_GNSS,
>> +	DPLL_TYPE_CUSTOM,
> 
> This hunk does not belong to this patch.

Oh yeah, thanks for catching!
> 
> 
>>
>> 	__DPLL_TYPE_MAX,
>> };
>> -- 
>> 2.27.0
>>
diff mbox series

Patch

diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 458218f88c5e..f74846ebc177 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -176,6 +176,7 @@  config PTP_1588_CLOCK_OCP
 	depends on !S390
 	depends on COMMON_CLK
 	select NET_DEVLINK
+	select DPLL
 	help
 	  This driver adds support for an OpenCompute time card.
 
diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
index e59ea2173aac..f830625a63a3 100644
--- a/drivers/ptp/ptp_ocp.c
+++ b/drivers/ptp/ptp_ocp.c
@@ -21,6 +21,7 @@ 
 #include <linux/mtd/mtd.h>
 #include <linux/nvmem-consumer.h>
 #include <linux/crc16.h>
+#include <uapi/linux/dpll.h>
 
 #define PCI_VENDOR_ID_FACEBOOK			0x1d9b
 #define PCI_DEVICE_ID_FACEBOOK_TIMECARD		0x0400
@@ -336,6 +337,7 @@  struct ptp_ocp {
 	struct ptp_ocp_signal	signal[4];
 	struct ptp_ocp_sma_connector sma[4];
 	const struct ocp_sma_op *sma_op;
+	struct dpll_device *dpll;
 };
 
 #define OCP_REQ_TIMESTAMP	BIT(0)
@@ -660,18 +662,19 @@  static DEFINE_IDR(ptp_ocp_idr);
 struct ocp_selector {
 	const char *name;
 	int value;
+	int dpll_type;
 };
 
 static const struct ocp_selector ptp_ocp_clock[] = {
-	{ .name = "NONE",	.value = 0 },
-	{ .name = "TOD",	.value = 1 },
-	{ .name = "IRIG",	.value = 2 },
-	{ .name = "PPS",	.value = 3 },
-	{ .name = "PTP",	.value = 4 },
-	{ .name = "RTC",	.value = 5 },
-	{ .name = "DCF",	.value = 6 },
-	{ .name = "REGS",	.value = 0xfe },
-	{ .name = "EXT",	.value = 0xff },
+	{ .name = "NONE",	.value = 0,		.dpll_type = 0 },
+	{ .name = "TOD",	.value = 1,		.dpll_type = 0 },
+	{ .name = "IRIG",	.value = 2,		.dpll_type = 0 },
+	{ .name = "PPS",	.value = 3,		.dpll_type = 0 },
+	{ .name = "PTP",	.value = 4,		.dpll_type = 0 },
+	{ .name = "RTC",	.value = 5,		.dpll_type = 0 },
+	{ .name = "DCF",	.value = 6,		.dpll_type = 0 },
+	{ .name = "REGS",	.value = 0xfe,		.dpll_type = 0 },
+	{ .name = "EXT",	.value = 0xff,		.dpll_type = 0 },
 	{ }
 };
 
@@ -680,37 +683,37 @@  static const struct ocp_selector ptp_ocp_clock[] = {
 #define SMA_SELECT_MASK		GENMASK(14, 0)
 
 static const struct ocp_selector ptp_ocp_sma_in[] = {
-	{ .name = "10Mhz",	.value = 0x0000 },
-	{ .name = "PPS1",	.value = 0x0001 },
-	{ .name = "PPS2",	.value = 0x0002 },
-	{ .name = "TS1",	.value = 0x0004 },
-	{ .name = "TS2",	.value = 0x0008 },
-	{ .name = "IRIG",	.value = 0x0010 },
-	{ .name = "DCF",	.value = 0x0020 },
-	{ .name = "TS3",	.value = 0x0040 },
-	{ .name = "TS4",	.value = 0x0080 },
-	{ .name = "FREQ1",	.value = 0x0100 },
-	{ .name = "FREQ2",	.value = 0x0200 },
-	{ .name = "FREQ3",	.value = 0x0400 },
-	{ .name = "FREQ4",	.value = 0x0800 },
-	{ .name = "None",	.value = SMA_DISABLE },
+	{ .name = "10Mhz",	.value = 0x0000,	.dpll_type = DPLL_TYPE_EXT_10MHZ },
+	{ .name = "PPS1",	.value = 0x0001,	.dpll_type = DPLL_TYPE_EXT_1PPS },
+	{ .name = "PPS2",	.value = 0x0002,	.dpll_type = DPLL_TYPE_EXT_1PPS },
+	{ .name = "TS1",	.value = 0x0004,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "TS2",	.value = 0x0008,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "IRIG",	.value = 0x0010,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "DCF",	.value = 0x0020,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "TS3",	.value = 0x0040,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "TS4",	.value = 0x0080,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "FREQ1",	.value = 0x0100,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "FREQ2",	.value = 0x0200,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "FREQ3",	.value = 0x0400,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "FREQ4",	.value = 0x0800,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "None",	.value = SMA_DISABLE,	.dpll_type = DPLL_TYPE_NONE },
 	{ }
 };
 
 static const struct ocp_selector ptp_ocp_sma_out[] = {
-	{ .name = "10Mhz",	.value = 0x0000 },
-	{ .name = "PHC",	.value = 0x0001 },
-	{ .name = "MAC",	.value = 0x0002 },
-	{ .name = "GNSS1",	.value = 0x0004 },
-	{ .name = "GNSS2",	.value = 0x0008 },
-	{ .name = "IRIG",	.value = 0x0010 },
-	{ .name = "DCF",	.value = 0x0020 },
-	{ .name = "GEN1",	.value = 0x0040 },
-	{ .name = "GEN2",	.value = 0x0080 },
-	{ .name = "GEN3",	.value = 0x0100 },
-	{ .name = "GEN4",	.value = 0x0200 },
-	{ .name = "GND",	.value = 0x2000 },
-	{ .name = "VCC",	.value = 0x4000 },
+	{ .name = "10Mhz",	.value = 0x0000,	.dpll_type = DPLL_TYPE_EXT_10MHZ },
+	{ .name = "PHC",	.value = 0x0001,	.dpll_type = DPLL_TYPE_INT_OSCILLATOR },
+	{ .name = "MAC",	.value = 0x0002,	.dpll_type = DPLL_TYPE_INT_OSCILLATOR },
+	{ .name = "GNSS1",	.value = 0x0004,	.dpll_type = DPLL_TYPE_GNSS },
+	{ .name = "GNSS2",	.value = 0x0008,	.dpll_type = DPLL_TYPE_GNSS },
+	{ .name = "IRIG",	.value = 0x0010,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "DCF",	.value = 0x0020,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "GEN1",	.value = 0x0040,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "GEN2",	.value = 0x0080,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "GEN3",	.value = 0x0100,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "GEN4",	.value = 0x0200,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "GND",	.value = 0x2000,	.dpll_type = DPLL_TYPE_CUSTOM },
+	{ .name = "VCC",	.value = 0x4000,	.dpll_type = DPLL_TYPE_CUSTOM },
 	{ }
 };
 
@@ -3713,6 +3716,90 @@  ptp_ocp_detach(struct ptp_ocp *bp)
 	device_unregister(&bp->dev);
 }
 
+static int ptp_ocp_dpll_get_status(struct dpll_device *dpll)
+{
+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+	int sync;
+
+	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
+	return sync;
+}
+
+static int ptp_ocp_dpll_get_lock_status(struct dpll_device *dpll)
+{
+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+	int sync;
+
+	sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC;
+	return sync;
+}
+
+static int ptp_ocp_sma_get_dpll_type(struct ptp_ocp *bp, int sma_nr)
+{
+	struct ocp_selector *tbl;
+	u32 val;
+
+	if (bp->sma[sma_nr].mode == SMA_MODE_IN)
+		tbl = bp->sma_op->tbl[0];
+	else
+		tbl = bp->sma_op->tbl[1];
+
+	val = ptp_ocp_sma_get(bp, sma_nr);
+	return tbl[val].dpll_type;
+}
+
+static int ptp_ocp_dpll_type_supported(struct dpll_device *dpll, int sma, int type, int dir)
+{
+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+	struct ocp_selector *tbl = bp->sma_op->tbl[dir];
+	int i;
+
+	for (i = 0; i < sizeof(*tbl); i++) {
+		if (tbl[i].dpll_type == type)
+			return 1;
+	}
+	return 0;
+}
+
+static int ptp_ocp_dpll_get_source_type(struct dpll_device *dpll, int sma)
+{
+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+
+	if (bp->sma[sma].mode != SMA_MODE_IN)
+		return -1;
+
+	return ptp_ocp_sma_get_dpll_type(bp, sma);
+}
+
+static int ptp_ocp_dpll_get_source_supported(struct dpll_device *dpll, int sma, int type)
+{
+	return ptp_ocp_dpll_type_supported(dpll, sma, type, 0);
+}
+
+static int ptp_ocp_dpll_get_output_type(struct dpll_device *dpll, int sma)
+{
+	struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll);
+
+	if (bp->sma[sma].mode != SMA_MODE_OUT)
+		return -1;
+
+	return ptp_ocp_sma_get_dpll_type(bp, sma);
+}
+
+static int ptp_ocp_dpll_get_output_supported(struct dpll_device *dpll, int sma, int type)
+{
+	return ptp_ocp_dpll_type_supported(dpll, sma, type, 1);
+}
+
+static struct dpll_device_ops dpll_ops = {
+	.get_status		= ptp_ocp_dpll_get_status,
+	.get_lock_status	= ptp_ocp_dpll_get_lock_status,
+	.get_source_type	= ptp_ocp_dpll_get_source_type,
+	.get_source_supported	= ptp_ocp_dpll_get_source_supported,
+	.get_output_type	= ptp_ocp_dpll_get_output_type,
+	.get_output_supported	= ptp_ocp_dpll_get_output_supported,
+};
+
 static int
 ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
@@ -3768,6 +3855,14 @@  ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	ptp_ocp_info(bp);
 	devlink_register(devlink);
+
+	bp->dpll = dpll_device_alloc(&dpll_ops, ARRAY_SIZE(bp->sma), ARRAY_SIZE(bp->sma), bp);
+	if (!bp->dpll) {
+		dev_err(&pdev->dev, "dpll_device_alloc failed\n");
+		return 0;
+	}
+	dpll_device_register(bp->dpll);
+
 	return 0;
 
 out:
@@ -3785,6 +3880,8 @@  ptp_ocp_remove(struct pci_dev *pdev)
 	struct ptp_ocp *bp = pci_get_drvdata(pdev);
 	struct devlink *devlink = priv_to_devlink(bp);
 
+	dpll_device_unregister(bp->dpll);
+	dpll_device_free(bp->dpll);
 	devlink_unregister(devlink);
 	ptp_ocp_detach(bp);
 	pci_disable_device(pdev);
diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h
index 7ce45c6b4fd4..5e8c46712d18 100644
--- a/include/uapi/linux/dpll.h
+++ b/include/uapi/linux/dpll.h
@@ -41,11 +41,13 @@  enum dpll_genl_attr {
 
 /* DPLL signal types used as source or as output */
 enum dpll_genl_signal_type {
+	DPLL_TYPE_NONE,
 	DPLL_TYPE_EXT_1PPS,
 	DPLL_TYPE_EXT_10MHZ,
 	DPLL_TYPE_SYNCE_ETH_PORT,
 	DPLL_TYPE_INT_OSCILLATOR,
 	DPLL_TYPE_GNSS,
+	DPLL_TYPE_CUSTOM,
 
 	__DPLL_TYPE_MAX,
 };