| Message ID | 20230428002009.2948020-7-vadfed@meta.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Create common DPLL configuration API | expand |
Fri, Apr 28, 2023 at 02:20:07AM CEST, vadfed@meta.com wrote: >From: Vadim Fedorenko <vadim.fedorenko@linux.dev> > >Implement basic DPLL operations in ptp_ocp driver as the >simplest example of using new subsystem. > >Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >--- > drivers/ptp/Kconfig | 1 + > drivers/ptp/ptp_ocp.c | 327 +++++++++++++++++++++++++++++++++++------- > 2 files changed, 276 insertions(+), 52 deletions(-) > >diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig >index b00201d81313..e3575c2e34dc 100644 >--- a/drivers/ptp/Kconfig >+++ b/drivers/ptp/Kconfig >@@ -177,6 +177,7 @@ config PTP_1588_CLOCK_OCP > depends on COMMON_CLK > select NET_DEVLINK > select CRC16 >+ 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 2b63f3487645..100e5da0aeb3 100644 >--- a/drivers/ptp/ptp_ocp.c >+++ b/drivers/ptp/ptp_ocp.c >@@ -23,6 +23,7 @@ > #include <linux/mtd/mtd.h> > #include <linux/nvmem-consumer.h> > #include <linux/crc16.h> >+#include <linux/dpll.h> > > #define PCI_VENDOR_ID_FACEBOOK 0x1d9b > #define PCI_DEVICE_ID_FACEBOOK_TIMECARD 0x0400 >@@ -261,12 +262,21 @@ enum ptp_ocp_sma_mode { > SMA_MODE_OUT, > }; > >+static struct dpll_pin_frequency ptp_ocp_sma_freq[] = { const >+ DPLL_PIN_FREQUENCY_1PPS, >+ DPLL_PIN_FREQUENCY_10MHZ, >+ DPLL_PIN_FREQUENCY_IRIG_B, >+ DPLL_PIN_FREQUENCY_DCF77, >+}; >+ > struct ptp_ocp_sma_connector { > enum ptp_ocp_sma_mode mode; > bool fixed_fcn; > bool fixed_dir; > bool disabled; > u8 default_fcn; >+ struct dpll_pin *dpll_pin; >+ struct dpll_pin_properties dpll_prop; > }; > > struct ocp_attr_group { >@@ -295,6 +305,7 @@ struct ptp_ocp_serial_port { > > #define OCP_BOARD_ID_LEN 13 > #define OCP_SERIAL_LEN 6 >+#define OCP_SMA_NUM 4 > > struct ptp_ocp { > struct pci_dev *pdev; >@@ -351,8 +362,9 @@ struct ptp_ocp { > u32 ts_window_adjust; > u64 fw_cap; > struct ptp_ocp_signal signal[4]; >- struct ptp_ocp_sma_connector sma[4]; >+ struct ptp_ocp_sma_connector sma[OCP_SMA_NUM]; > const struct ocp_sma_op *sma_op; >+ struct dpll_device *dpll; > }; > > #define OCP_REQ_TIMESTAMP BIT(0) >@@ -836,6 +848,7 @@ static DEFINE_IDR(ptp_ocp_idr); > struct ocp_selector { > const char *name; > int value; >+ u64 frequency; > }; > > static const struct ocp_selector ptp_ocp_clock[] = { >@@ -856,31 +869,31 @@ 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, .frequency = 10000000 }, >+ { .name = "PPS1", .value = 0x0001, .frequency = 1 }, >+ { .name = "PPS2", .value = 0x0002, .frequency = 1 }, >+ { .name = "TS1", .value = 0x0004, .frequency = 0 }, >+ { .name = "TS2", .value = 0x0008, .frequency = 0 }, >+ { .name = "IRIG", .value = 0x0010, .frequency = 10000 }, >+ { .name = "DCF", .value = 0x0020, .frequency = 77500 }, >+ { .name = "TS3", .value = 0x0040, .frequency = 0 }, >+ { .name = "TS4", .value = 0x0080, .frequency = 0 }, >+ { .name = "FREQ1", .value = 0x0100, .frequency = 0 }, >+ { .name = "FREQ2", .value = 0x0200, .frequency = 0 }, >+ { .name = "FREQ3", .value = 0x0400, .frequency = 0 }, >+ { .name = "FREQ4", .value = 0x0800, .frequency = 0 }, >+ { .name = "None", .value = SMA_DISABLE, .frequency = 0 }, > { } > }; > > 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 = "10Mhz", .value = 0x0000, .frequency = 10000000 }, >+ { .name = "PHC", .value = 0x0001, .frequency = 1 }, >+ { .name = "MAC", .value = 0x0002, .frequency = 1 }, >+ { .name = "GNSS1", .value = 0x0004, .frequency = 1 }, >+ { .name = "GNSS2", .value = 0x0008, .frequency = 1 }, >+ { .name = "IRIG", .value = 0x0010, .frequency = 10000 }, >+ { .name = "DCF", .value = 0x0020, .frequency = 77000 }, > { .name = "GEN1", .value = 0x0040 }, > { .name = "GEN2", .value = 0x0080 }, > { .name = "GEN3", .value = 0x0100 }, >@@ -891,15 +904,15 @@ static const struct ocp_selector ptp_ocp_sma_out[] = { > }; > > static const struct ocp_selector ptp_ocp_art_sma_in[] = { >- { .name = "PPS1", .value = 0x0001 }, >- { .name = "10Mhz", .value = 0x0008 }, >+ { .name = "PPS1", .value = 0x0001, .frequency = 1 }, >+ { .name = "10Mhz", .value = 0x0008, .frequency = 1000000 }, > { } > }; > > static const struct ocp_selector ptp_ocp_art_sma_out[] = { >- { .name = "PHC", .value = 0x0002 }, >- { .name = "GNSS", .value = 0x0004 }, >- { .name = "10Mhz", .value = 0x0010 }, >+ { .name = "PHC", .value = 0x0002, .frequency = 1 }, >+ { .name = "GNSS", .value = 0x0004, .frequency = 1 }, >+ { .name = "10Mhz", .value = 0x0010, .frequency = 10000000 }, > { } > }; > >@@ -2283,22 +2296,34 @@ ptp_ocp_sma_fb_set_inputs(struct ptp_ocp *bp, int sma_nr, u32 val) > static void > ptp_ocp_sma_fb_init(struct ptp_ocp *bp) > { >+ struct dpll_pin_properties prop = { Why don't you have this as static const outside the function? >+ .label = NULL, Pointless init. >+ .type = DPLL_PIN_TYPE_EXT, >+ .capabilities = DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE, >+ .freq_supported_num = ARRAY_SIZE(ptp_ocp_sma_freq), >+ .freq_supported = ptp_ocp_sma_freq, >+ >+ }; > u32 reg; > int i; > > /* defaults */ >+ for (i = 0; i < OCP_SMA_NUM; i++) { >+ bp->sma[i].default_fcn = i & 1; >+ bp->sma[i].dpll_prop = prop; >+ bp->sma[i].dpll_prop.label = bp->ptp_info.pin_config[i].name; >+ } > bp->sma[0].mode = SMA_MODE_IN; > 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) { >- for (i = 0; i < 4; i++) { >+ for (i = 0; i < OCP_SMA_NUM; i++) { > bp->sma[i].fixed_fcn = true; > bp->sma[i].fixed_dir = true; >+ bp->sma[1].dpll_prop.capabilities &= >+ ~DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE; > } > return; > } >@@ -2308,7 +2333,7 @@ ptp_ocp_sma_fb_init(struct ptp_ocp *bp) > */ > reg = ioread32(&bp->sma_map2->gpio2); > if (reg == 0xffffffff) { >- for (i = 0; i < 4; i++) >+ for (i = 0; i < OCP_SMA_NUM; i++) > bp->sma[i].fixed_dir = true; > } else { > reg = ioread32(&bp->sma_map1->gpio1); >@@ -2330,7 +2355,7 @@ static const struct ocp_sma_op ocp_fb_sma_op = { > }; > > static int >-ptp_ocp_fb_set_pins(struct ptp_ocp *bp) >+ptp_ocp_set_pins(struct ptp_ocp *bp) > { > struct ptp_pin_desc *config; > int i; >@@ -2397,16 +2422,16 @@ ptp_ocp_fb_board_init(struct ptp_ocp *bp, struct ocp_resource *r) > > ptp_ocp_tod_init(bp); > ptp_ocp_nmea_out_init(bp); >- ptp_ocp_sma_init(bp); > ptp_ocp_signal_init(bp); > > err = ptp_ocp_attr_group_add(bp, fb_timecard_groups); > if (err) > return err; > >- err = ptp_ocp_fb_set_pins(bp); >+ err = ptp_ocp_set_pins(bp); > if (err) > return err; >+ ptp_ocp_sma_init(bp); > > return ptp_ocp_init_clock(bp); > } >@@ -2446,6 +2471,14 @@ ptp_ocp_register_resources(struct ptp_ocp *bp, kernel_ulong_t driver_data) > static void > ptp_ocp_art_sma_init(struct ptp_ocp *bp) > { >+ struct dpll_pin_properties prop = { >+ .label = NULL, >+ .type = DPLL_PIN_TYPE_EXT, >+ .capabilities = 0, Same comment as to the similar prop struct above. Plus another pointless init here. >+ .freq_supported_num = ARRAY_SIZE(ptp_ocp_sma_freq), >+ .freq_supported = ptp_ocp_sma_freq, >+ >+ }; > u32 reg; > int i; > >@@ -2460,16 +2493,16 @@ ptp_ocp_art_sma_init(struct ptp_ocp *bp) > bp->sma[2].default_fcn = 0x10; /* OUT: 10Mhz */ > bp->sma[3].default_fcn = 0x02; /* OUT: PHC */ > >- /* If no SMA map, the pin functions and directions are fixed. */ >- if (!bp->art_sma) { >- for (i = 0; i < 4; i++) { >+ >+ for (i = 0; i < OCP_SMA_NUM; i++) { >+ /* If no SMA map, the pin functions and directions are fixed. */ >+ bp->sma[i].dpll_prop = prop; >+ bp->sma[i].dpll_prop.label = bp->ptp_info.pin_config[i].name; >+ if (!bp->art_sma) { > bp->sma[i].fixed_fcn = true; > bp->sma[i].fixed_dir = true; >+ continue; > } >- return; >- } >- >- for (i = 0; i < 4; i++) { > reg = ioread32(&bp->art_sma->map[i].gpio); > > switch (reg & 0xff) { >@@ -2480,9 +2513,13 @@ ptp_ocp_art_sma_init(struct ptp_ocp *bp) > case 1: > case 8: > bp->sma[i].mode = SMA_MODE_IN; >+ bp->sma[i].dpll_prop.capabilities = >+ DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE; > break; > default: > bp->sma[i].mode = SMA_MODE_OUT; >+ bp->sma[i].dpll_prop.capabilities = >+ DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE; > break; > } > } >@@ -2549,6 +2586,9 @@ ptp_ocp_art_board_init(struct ptp_ocp *bp, struct ocp_resource *r) > /* Enable MAC serial port during initialisation */ > iowrite32(1, &bp->board_config->mro50_serial_activate); > >+ err = ptp_ocp_set_pins(bp); >+ if (err) >+ return err; > ptp_ocp_sma_init(bp); > > err = ptp_ocp_attr_group_add(bp, art_timecard_groups); >@@ -2690,16 +2730,9 @@ sma4_show(struct device *dev, struct device_attribute *attr, char *buf) > } > > static int >-ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) >+ptp_ocp_sma_store_val(struct ptp_ocp *bp, int val, enum ptp_ocp_sma_mode mode, int sma_nr) > { > struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1]; >- enum ptp_ocp_sma_mode mode; >- int val; >- >- mode = sma->mode; >- val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode); >- if (val < 0) >- return val; > > if (sma->fixed_dir && (mode != sma->mode || val & SMA_DISABLE)) > return -EOPNOTSUPP; >@@ -2734,6 +2767,20 @@ ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) > return val; > } > >+static int >+ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) >+{ >+ struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1]; >+ enum ptp_ocp_sma_mode mode; >+ int val; >+ >+ mode = sma->mode; >+ val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode); >+ if (val < 0) >+ return val; >+ return ptp_ocp_sma_store_val(bp, val, mode, sma_nr); >+} >+ > static ssize_t > sma1_store(struct device *dev, struct device_attribute *attr, > const char *buf, size_t count) >@@ -4172,12 +4219,148 @@ ptp_ocp_detach(struct ptp_ocp *bp) > device_unregister(&bp->dev); > } > >+static int ptp_ocp_dpll_lock_status_get(const struct dpll_device *dpll, >+ void *priv, >+ enum dpll_lock_status *status, >+ struct netlink_ext_ack *extack) >+{ >+ struct ptp_ocp *bp = priv; >+ int sync; >+ >+ sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; >+ *status = sync ? DPLL_LOCK_STATUS_LOCKED : DPLL_LOCK_STATUS_UNLOCKED; Does your device support event delivery in case of the status change? ice and mlx5 drivers do poll for changes in this area anyway. It's a part of this patchset. You should do the same if your device does not support events. Could you please implement notifications using dpll_device_notify() for status change and dpll_pin_notify() for pin state change? >+ >+ return 0; >+} >+ >+static int ptp_ocp_dpll_source_idx_get(const struct dpll_device *dpll, >+ void *priv, u32 *idx, >+ struct netlink_ext_ack *extack) >+{ >+ struct ptp_ocp *bp = priv; >+ >+ if (bp->pps_select) { >+ *idx = ioread32(&bp->pps_select->gpio1); >+ return 0; >+ } >+ return -EINVAL; >+} >+ >+static int ptp_ocp_dpll_mode_get(const struct dpll_device *dpll, void *priv, >+ u32 *mode, struct netlink_ext_ack *extack) >+{ >+ *mode = DPLL_MODE_AUTOMATIC; >+ return 0; >+} >+ >+static bool ptp_ocp_dpll_mode_supported(const struct dpll_device *dpll, >+ void *priv, const enum dpll_mode mode, >+ struct netlink_ext_ack *extack) >+{ >+ return mode == DPLL_MODE_AUTOMATIC; >+} >+ >+static int ptp_ocp_dpll_direction_get(const struct dpll_pin *pin, >+ void *pin_priv, >+ const struct dpll_device *dpll, >+ void *priv, >+ enum dpll_pin_direction *direction, >+ struct netlink_ext_ack *extack) >+{ >+ struct ptp_ocp_sma_connector *sma = pin_priv; >+ >+ *direction = sma->mode == SMA_MODE_IN ? >+ DPLL_PIN_DIRECTION_SOURCE : >+ DPLL_PIN_DIRECTION_OUTPUT; >+ return 0; >+} >+ >+static int ptp_ocp_dpll_direction_set(const struct dpll_pin *pin, >+ void *pin_priv, >+ const struct dpll_device *dpll, >+ void *dpll_priv, >+ enum dpll_pin_direction direction, >+ struct netlink_ext_ack *extack) >+{ >+ struct ptp_ocp_sma_connector *sma = pin_priv; >+ struct ptp_ocp *bp = dpll_priv; >+ enum ptp_ocp_sma_mode mode; >+ int sma_nr = (sma - bp->sma); >+ >+ if (sma->fixed_dir) I believe that this is a pointless check as DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE is not set and therefore the check in dpll_pin_direction_set() will be true and -EOPNOTSUPP will be returned from there. Remove this. >+ return -EOPNOTSUPP; >+ mode = direction == DPLL_PIN_DIRECTION_SOURCE ? >+ SMA_MODE_IN : SMA_MODE_OUT; >+ return ptp_ocp_sma_store_val(bp, 0, mode, sma_nr); You need sma_nr just here. Why can't you change ptp_ocp_sma_store_val() to accept struct ptp_ocp_sma_connector * instead avoiding the need for tne sma_nr completely? >+} >+ >+static int ptp_ocp_dpll_frequency_set(const struct dpll_pin *pin, >+ void *pin_priv, >+ const struct dpll_device *dpll, >+ void *dpll_priv, u64 frequency, >+ struct netlink_ext_ack *extack) >+{ >+ struct ptp_ocp_sma_connector *sma = pin_priv; >+ struct ptp_ocp *bp = dpll_priv; >+ const struct ocp_selector *tbl; >+ int sma_nr = (sma - bp->sma); >+ int val, i; >+ >+ if (sma->fixed_fcn) In that case, just fill up a single frequency in the properties, avoid this check-fail and let the dpll core handle it. >+ return -EOPNOTSUPP; >+ >+ tbl = bp->sma_op->tbl[sma->mode]; >+ for (i = 0; tbl[i].name; i++) >+ if (tbl[i].frequency == frequency) >+ return ptp_ocp_sma_store_val(bp, val, sma->mode, sma_nr); >+ return -EINVAL; >+} >+ >+static int ptp_ocp_dpll_frequency_get(const struct dpll_pin *pin, >+ void *pin_priv, >+ const struct dpll_device *dpll, >+ void *dpll_priv, u64 *frequency, >+ struct netlink_ext_ack *extack) >+{ >+ struct ptp_ocp_sma_connector *sma = pin_priv; >+ struct ptp_ocp *bp = dpll_priv; >+ const struct ocp_selector *tbl; >+ int sma_nr = (sma - bp->sma); 1) void "()"s here. 2) why don't you fill the sma_nr in struct ptp_ocp_sma_connector to make this easier to follow? IDK, just a suggestion, take or leave. Same applies to the the rest of similar occurances above. >+ u32 val; >+ int i; >+ >+ val = bp->sma_op->get(bp, sma_nr); >+ tbl = bp->sma_op->tbl[sma->mode]; >+ for (i = 0; tbl[i].name; i++) >+ if (val == tbl[i].value) { >+ *frequency = tbl[i].frequency; >+ return 0; >+ } >+ >+ return -EINVAL; >+} >+ >+static const struct dpll_device_ops dpll_ops = { >+ .lock_status_get = ptp_ocp_dpll_lock_status_get, >+ .source_pin_idx_get = ptp_ocp_dpll_source_idx_get, This op is a leftover, in dpll core it is not called. This was removed and agreed that drivers should implement state_on_dpll_get() op for pins to see which one is connected. Please fix here and remove the leftover from DPLL patch #2 as well. >+ .mode_get = ptp_ocp_dpll_mode_get, >+ .mode_supported = ptp_ocp_dpll_mode_supported, >+}; >+ >+static const struct dpll_pin_ops dpll_pins_ops = { >+ .frequency_get = ptp_ocp_dpll_frequency_get, >+ .frequency_set = ptp_ocp_dpll_frequency_set, >+ .direction_get = ptp_ocp_dpll_direction_get, >+ .direction_set = ptp_ocp_dpll_direction_set, >+}; >+ > static int > ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { > struct devlink *devlink; > struct ptp_ocp *bp; >- int err; >+ int err, i; >+ u64 clkid; > > devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev->dev); > if (!devlink) { >@@ -4227,8 +4410,39 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > ptp_ocp_info(bp); > devlink_register(devlink); >- return 0; > >+ clkid = pci_get_dsn(pdev); >+ bp->dpll = dpll_device_get(clkid, 0, THIS_MODULE); I suggested this the last time, but again: Could you please: 1) rename dpll_device_get to __dpll_device_get 2) introduce dpll_device_get as a macro filling up THIS_MODULE Then drivers will just call always: bp->dpll = dpll_device_get(clkid, 0); and the macro will fillup the module automatically. Please do the same for dpll_pin_get() >+ if (IS_ERR(bp->dpll)) { >+ dev_err(&pdev->dev, "dpll_device_alloc failed\n"); You need to fix your error path to call devlink_unregister() in this case. >+ goto out; >+ } >+ >+ err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp, &pdev->dev); >+ if (err) You need to fix your error path to call dpll_device_put() in this case. >+ goto out; >+ >+ for (i = 0; i < OCP_SMA_NUM; i++) { >+ bp->sma[i].dpll_pin = dpll_pin_get(clkid, i, THIS_MODULE, &bp->sma[i].dpll_prop); >+ if (IS_ERR(bp->sma[i].dpll_pin)) >+ goto out_dpll; >+ >+ err = dpll_pin_register(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, >+ &bp->sma[i], NULL); >+ if (err) { >+ dpll_pin_put(bp->sma[i].dpll_pin); >+ goto out_dpll; >+ } >+ } >+ >+ return 0; >+out_dpll: >+ while (i) { >+ --i; while (i--) { ? >+ dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, &bp->sma[i]); >+ dpll_pin_put(bp->sma[i].dpll_pin); >+ } >+ dpll_device_put(bp->dpll); > out: > ptp_ocp_detach(bp); > out_disable: >@@ -4243,7 +4457,16 @@ ptp_ocp_remove(struct pci_dev *pdev) > { > struct ptp_ocp *bp = pci_get_drvdata(pdev); > struct devlink *devlink = priv_to_devlink(bp); >+ int i; > >+ for (i = 0; i < OCP_SMA_NUM; i++) { >+ if (bp->sma[i].dpll_pin) { Remove this pointless check. It is always true. >+ dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, bp); >+ dpll_pin_put(bp->sma[i].dpll_pin); >+ } >+ } >+ dpll_device_unregister(bp->dpll, &dpll_ops, bp); >+ dpll_device_put(bp->dpll); > devlink_unregister(devlink); > ptp_ocp_detach(bp); > pci_disable_device(pdev); >-- >2.34.1 >
On 04/05/2023 10:27, Jiri Pirko wrote: > Fri, Apr 28, 2023 at 02:20:07AM CEST, vadfed@meta.com wrote: >> From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >> >> Implement basic DPLL operations in ptp_ocp driver as the >> simplest example of using new subsystem. >> >> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >> --- >> drivers/ptp/Kconfig | 1 + >> drivers/ptp/ptp_ocp.c | 327 +++++++++++++++++++++++++++++++++++------- >> 2 files changed, 276 insertions(+), 52 deletions(-) >> >> diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig >> index b00201d81313..e3575c2e34dc 100644 >> --- a/drivers/ptp/Kconfig >> +++ b/drivers/ptp/Kconfig >> @@ -177,6 +177,7 @@ config PTP_1588_CLOCK_OCP >> depends on COMMON_CLK >> select NET_DEVLINK >> select CRC16 >> + 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 2b63f3487645..100e5da0aeb3 100644 >> --- a/drivers/ptp/ptp_ocp.c >> +++ b/drivers/ptp/ptp_ocp.c >> @@ -23,6 +23,7 @@ >> #include <linux/mtd/mtd.h> >> #include <linux/nvmem-consumer.h> >> #include <linux/crc16.h> >> +#include <linux/dpll.h> >> >> #define PCI_VENDOR_ID_FACEBOOK 0x1d9b >> #define PCI_DEVICE_ID_FACEBOOK_TIMECARD 0x0400 >> @@ -261,12 +262,21 @@ enum ptp_ocp_sma_mode { >> SMA_MODE_OUT, >> }; >> >> +static struct dpll_pin_frequency ptp_ocp_sma_freq[] = { > > const Forgot about this one, will change it. > >> + DPLL_PIN_FREQUENCY_1PPS, >> + DPLL_PIN_FREQUENCY_10MHZ, >> + DPLL_PIN_FREQUENCY_IRIG_B, >> + DPLL_PIN_FREQUENCY_DCF77, >> +}; >> + >> struct ptp_ocp_sma_connector { >> enum ptp_ocp_sma_mode mode; >> bool fixed_fcn; >> bool fixed_dir; >> bool disabled; >> u8 default_fcn; >> + struct dpll_pin *dpll_pin; >> + struct dpll_pin_properties dpll_prop; >> }; >> >> struct ocp_attr_group { >> @@ -295,6 +305,7 @@ struct ptp_ocp_serial_port { >> >> #define OCP_BOARD_ID_LEN 13 >> #define OCP_SERIAL_LEN 6 >> +#define OCP_SMA_NUM 4 >> >> struct ptp_ocp { >> struct pci_dev *pdev; >> @@ -351,8 +362,9 @@ struct ptp_ocp { >> u32 ts_window_adjust; >> u64 fw_cap; >> struct ptp_ocp_signal signal[4]; >> - struct ptp_ocp_sma_connector sma[4]; >> + struct ptp_ocp_sma_connector sma[OCP_SMA_NUM]; >> const struct ocp_sma_op *sma_op; >> + struct dpll_device *dpll; >> }; >> >> #define OCP_REQ_TIMESTAMP BIT(0) >> @@ -836,6 +848,7 @@ static DEFINE_IDR(ptp_ocp_idr); >> struct ocp_selector { >> const char *name; >> int value; >> + u64 frequency; >> }; >> >> static const struct ocp_selector ptp_ocp_clock[] = { >> @@ -856,31 +869,31 @@ 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, .frequency = 10000000 }, >> + { .name = "PPS1", .value = 0x0001, .frequency = 1 }, >> + { .name = "PPS2", .value = 0x0002, .frequency = 1 }, >> + { .name = "TS1", .value = 0x0004, .frequency = 0 }, >> + { .name = "TS2", .value = 0x0008, .frequency = 0 }, >> + { .name = "IRIG", .value = 0x0010, .frequency = 10000 }, >> + { .name = "DCF", .value = 0x0020, .frequency = 77500 }, >> + { .name = "TS3", .value = 0x0040, .frequency = 0 }, >> + { .name = "TS4", .value = 0x0080, .frequency = 0 }, >> + { .name = "FREQ1", .value = 0x0100, .frequency = 0 }, >> + { .name = "FREQ2", .value = 0x0200, .frequency = 0 }, >> + { .name = "FREQ3", .value = 0x0400, .frequency = 0 }, >> + { .name = "FREQ4", .value = 0x0800, .frequency = 0 }, >> + { .name = "None", .value = SMA_DISABLE, .frequency = 0 }, >> { } >> }; >> >> 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 = "10Mhz", .value = 0x0000, .frequency = 10000000 }, >> + { .name = "PHC", .value = 0x0001, .frequency = 1 }, >> + { .name = "MAC", .value = 0x0002, .frequency = 1 }, >> + { .name = "GNSS1", .value = 0x0004, .frequency = 1 }, >> + { .name = "GNSS2", .value = 0x0008, .frequency = 1 }, >> + { .name = "IRIG", .value = 0x0010, .frequency = 10000 }, >> + { .name = "DCF", .value = 0x0020, .frequency = 77000 }, >> { .name = "GEN1", .value = 0x0040 }, >> { .name = "GEN2", .value = 0x0080 }, >> { .name = "GEN3", .value = 0x0100 }, >> @@ -891,15 +904,15 @@ static const struct ocp_selector ptp_ocp_sma_out[] = { >> }; >> >> static const struct ocp_selector ptp_ocp_art_sma_in[] = { >> - { .name = "PPS1", .value = 0x0001 }, >> - { .name = "10Mhz", .value = 0x0008 }, >> + { .name = "PPS1", .value = 0x0001, .frequency = 1 }, >> + { .name = "10Mhz", .value = 0x0008, .frequency = 1000000 }, >> { } >> }; >> >> static const struct ocp_selector ptp_ocp_art_sma_out[] = { >> - { .name = "PHC", .value = 0x0002 }, >> - { .name = "GNSS", .value = 0x0004 }, >> - { .name = "10Mhz", .value = 0x0010 }, >> + { .name = "PHC", .value = 0x0002, .frequency = 1 }, >> + { .name = "GNSS", .value = 0x0004, .frequency = 1 }, >> + { .name = "10Mhz", .value = 0x0010, .frequency = 10000000 }, >> { } >> }; >> >> @@ -2283,22 +2296,34 @@ ptp_ocp_sma_fb_set_inputs(struct ptp_ocp *bp, int sma_nr, u32 val) >> static void >> ptp_ocp_sma_fb_init(struct ptp_ocp *bp) >> { >> + struct dpll_pin_properties prop = { > > Why don't you have this as static const outside the function? > Because I'm changing label string for every pin. I cannot change it in the const object. > >> + .label = NULL, > > Pointless init. Agree >> + .type = DPLL_PIN_TYPE_EXT, >> + .capabilities = DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE, >> + .freq_supported_num = ARRAY_SIZE(ptp_ocp_sma_freq), >> + .freq_supported = ptp_ocp_sma_freq, >> + >> + }; >> u32 reg; >> int i; >> >> /* defaults */ >> + for (i = 0; i < OCP_SMA_NUM; i++) { >> + bp->sma[i].default_fcn = i & 1; >> + bp->sma[i].dpll_prop = prop; >> + bp->sma[i].dpll_prop.label = bp->ptp_info.pin_config[i].name; >> + } >> bp->sma[0].mode = SMA_MODE_IN; >> 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) { >> - for (i = 0; i < 4; i++) { >> + for (i = 0; i < OCP_SMA_NUM; i++) { >> bp->sma[i].fixed_fcn = true; >> bp->sma[i].fixed_dir = true; >> + bp->sma[1].dpll_prop.capabilities &= >> + ~DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE; >> } >> return; >> } >> @@ -2308,7 +2333,7 @@ ptp_ocp_sma_fb_init(struct ptp_ocp *bp) >> */ >> reg = ioread32(&bp->sma_map2->gpio2); >> if (reg == 0xffffffff) { >> - for (i = 0; i < 4; i++) >> + for (i = 0; i < OCP_SMA_NUM; i++) >> bp->sma[i].fixed_dir = true; >> } else { >> reg = ioread32(&bp->sma_map1->gpio1); >> @@ -2330,7 +2355,7 @@ static const struct ocp_sma_op ocp_fb_sma_op = { >> }; >> >> static int >> -ptp_ocp_fb_set_pins(struct ptp_ocp *bp) >> +ptp_ocp_set_pins(struct ptp_ocp *bp) >> { >> struct ptp_pin_desc *config; >> int i; >> @@ -2397,16 +2422,16 @@ ptp_ocp_fb_board_init(struct ptp_ocp *bp, struct ocp_resource *r) >> >> ptp_ocp_tod_init(bp); >> ptp_ocp_nmea_out_init(bp); >> - ptp_ocp_sma_init(bp); >> ptp_ocp_signal_init(bp); >> >> err = ptp_ocp_attr_group_add(bp, fb_timecard_groups); >> if (err) >> return err; >> >> - err = ptp_ocp_fb_set_pins(bp); >> + err = ptp_ocp_set_pins(bp); >> if (err) >> return err; >> + ptp_ocp_sma_init(bp); >> >> return ptp_ocp_init_clock(bp); >> } >> @@ -2446,6 +2471,14 @@ ptp_ocp_register_resources(struct ptp_ocp *bp, kernel_ulong_t driver_data) >> static void >> ptp_ocp_art_sma_init(struct ptp_ocp *bp) >> { >> + struct dpll_pin_properties prop = { >> + .label = NULL, >> + .type = DPLL_PIN_TYPE_EXT, >> + .capabilities = 0, > > Same comment as to the similar prop struct above. Plus another pointless > init here. > Will remove pointless init. > >> + .freq_supported_num = ARRAY_SIZE(ptp_ocp_sma_freq), >> + .freq_supported = ptp_ocp_sma_freq, >> + >> + }; >> u32 reg; >> int i; >> >> @@ -2460,16 +2493,16 @@ ptp_ocp_art_sma_init(struct ptp_ocp *bp) >> bp->sma[2].default_fcn = 0x10; /* OUT: 10Mhz */ >> bp->sma[3].default_fcn = 0x02; /* OUT: PHC */ >> >> - /* If no SMA map, the pin functions and directions are fixed. */ >> - if (!bp->art_sma) { >> - for (i = 0; i < 4; i++) { >> + >> + for (i = 0; i < OCP_SMA_NUM; i++) { >> + /* If no SMA map, the pin functions and directions are fixed. */ >> + bp->sma[i].dpll_prop = prop; >> + bp->sma[i].dpll_prop.label = bp->ptp_info.pin_config[i].name; >> + if (!bp->art_sma) { >> bp->sma[i].fixed_fcn = true; >> bp->sma[i].fixed_dir = true; >> + continue; >> } >> - return; >> - } >> - >> - for (i = 0; i < 4; i++) { >> reg = ioread32(&bp->art_sma->map[i].gpio); >> >> switch (reg & 0xff) { >> @@ -2480,9 +2513,13 @@ ptp_ocp_art_sma_init(struct ptp_ocp *bp) >> case 1: >> case 8: >> bp->sma[i].mode = SMA_MODE_IN; >> + bp->sma[i].dpll_prop.capabilities = >> + DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE; >> break; >> default: >> bp->sma[i].mode = SMA_MODE_OUT; >> + bp->sma[i].dpll_prop.capabilities = >> + DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE; >> break; >> } >> } >> @@ -2549,6 +2586,9 @@ ptp_ocp_art_board_init(struct ptp_ocp *bp, struct ocp_resource *r) >> /* Enable MAC serial port during initialisation */ >> iowrite32(1, &bp->board_config->mro50_serial_activate); >> >> + err = ptp_ocp_set_pins(bp); >> + if (err) >> + return err; >> ptp_ocp_sma_init(bp); >> >> err = ptp_ocp_attr_group_add(bp, art_timecard_groups); >> @@ -2690,16 +2730,9 @@ sma4_show(struct device *dev, struct device_attribute *attr, char *buf) >> } >> >> static int >> -ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) >> +ptp_ocp_sma_store_val(struct ptp_ocp *bp, int val, enum ptp_ocp_sma_mode mode, int sma_nr) >> { >> struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1]; >> - enum ptp_ocp_sma_mode mode; >> - int val; >> - >> - mode = sma->mode; >> - val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode); >> - if (val < 0) >> - return val; >> >> if (sma->fixed_dir && (mode != sma->mode || val & SMA_DISABLE)) >> return -EOPNOTSUPP; >> @@ -2734,6 +2767,20 @@ ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) >> return val; >> } >> >> +static int >> +ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) >> +{ >> + struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1]; >> + enum ptp_ocp_sma_mode mode; >> + int val; >> + >> + mode = sma->mode; >> + val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode); >> + if (val < 0) >> + return val; >> + return ptp_ocp_sma_store_val(bp, val, mode, sma_nr); >> +} >> + >> static ssize_t >> sma1_store(struct device *dev, struct device_attribute *attr, >> const char *buf, size_t count) >> @@ -4172,12 +4219,148 @@ ptp_ocp_detach(struct ptp_ocp *bp) >> device_unregister(&bp->dev); >> } >> >> +static int ptp_ocp_dpll_lock_status_get(const struct dpll_device *dpll, >> + void *priv, >> + enum dpll_lock_status *status, >> + struct netlink_ext_ack *extack) >> +{ >> + struct ptp_ocp *bp = priv; >> + int sync; >> + >> + sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; >> + *status = sync ? DPLL_LOCK_STATUS_LOCKED : DPLL_LOCK_STATUS_UNLOCKED; > > Does your device support event delivery in case of the status change? > ice and mlx5 drivers do poll for changes in this area anyway. It's a > part of this patchset. You should do the same if your device does > not support events. > > Could you please implement notifications using > dpll_device_notify() for status change and dpll_pin_notify() for pin > state change? > We are working on implementation of interrupt-based notifications, that's why I didn't implement polling code. Hopefully it will be ready before the non-RFC patchset submission. > >> + >> + return 0; >> +} >> + >> +static int ptp_ocp_dpll_source_idx_get(const struct dpll_device *dpll, >> + void *priv, u32 *idx, >> + struct netlink_ext_ack *extack) >> +{ >> + struct ptp_ocp *bp = priv; >> + >> + if (bp->pps_select) { >> + *idx = ioread32(&bp->pps_select->gpio1); >> + return 0; >> + } >> + return -EINVAL; >> +} >> + >> +static int ptp_ocp_dpll_mode_get(const struct dpll_device *dpll, void *priv, >> + u32 *mode, struct netlink_ext_ack *extack) >> +{ >> + *mode = DPLL_MODE_AUTOMATIC; >> + return 0; >> +} >> + >> +static bool ptp_ocp_dpll_mode_supported(const struct dpll_device *dpll, >> + void *priv, const enum dpll_mode mode, >> + struct netlink_ext_ack *extack) >> +{ >> + return mode == DPLL_MODE_AUTOMATIC; >> +} >> + >> +static int ptp_ocp_dpll_direction_get(const struct dpll_pin *pin, >> + void *pin_priv, >> + const struct dpll_device *dpll, >> + void *priv, >> + enum dpll_pin_direction *direction, >> + struct netlink_ext_ack *extack) >> +{ >> + struct ptp_ocp_sma_connector *sma = pin_priv; >> + >> + *direction = sma->mode == SMA_MODE_IN ? >> + DPLL_PIN_DIRECTION_SOURCE : >> + DPLL_PIN_DIRECTION_OUTPUT; >> + return 0; >> +} >> + >> +static int ptp_ocp_dpll_direction_set(const struct dpll_pin *pin, >> + void *pin_priv, >> + const struct dpll_device *dpll, >> + void *dpll_priv, >> + enum dpll_pin_direction direction, >> + struct netlink_ext_ack *extack) >> +{ >> + struct ptp_ocp_sma_connector *sma = pin_priv; >> + struct ptp_ocp *bp = dpll_priv; >> + enum ptp_ocp_sma_mode mode; >> + int sma_nr = (sma - bp->sma); >> + >> + if (sma->fixed_dir) > > I believe that this is a pointless check as DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE > is not set and therefore the check in dpll_pin_direction_set() will be > true and -EOPNOTSUPP will be returned from there. > Remove this. Yep, will do it. > >> + return -EOPNOTSUPP; >> + mode = direction == DPLL_PIN_DIRECTION_SOURCE ? >> + SMA_MODE_IN : SMA_MODE_OUT; >> + return ptp_ocp_sma_store_val(bp, 0, mode, sma_nr); > > You need sma_nr just here. Why can't you change ptp_ocp_sma_store_val() > to accept struct ptp_ocp_sma_connector * instead avoiding the need for > tne sma_nr completely? > I wanted to add minimal changes to the driver, I will consider changing this as a separate net-next patch. > >> +} >> + >> +static int ptp_ocp_dpll_frequency_set(const struct dpll_pin *pin, >> + void *pin_priv, >> + const struct dpll_device *dpll, >> + void *dpll_priv, u64 frequency, >> + struct netlink_ext_ack *extack) >> +{ >> + struct ptp_ocp_sma_connector *sma = pin_priv; >> + struct ptp_ocp *bp = dpll_priv; >> + const struct ocp_selector *tbl; >> + int sma_nr = (sma - bp->sma); >> + int val, i; >> + >> + if (sma->fixed_fcn) > > In that case, just fill up a single frequency in the properties, > avoid this check-fail and let the dpll core handle it. > Makes sense. > >> + return -EOPNOTSUPP; >> + >> + tbl = bp->sma_op->tbl[sma->mode]; >> + for (i = 0; tbl[i].name; i++) >> + if (tbl[i].frequency == frequency) >> + return ptp_ocp_sma_store_val(bp, val, sma->mode, sma_nr); >> + return -EINVAL; >> +} >> + >> +static int ptp_ocp_dpll_frequency_get(const struct dpll_pin *pin, >> + void *pin_priv, >> + const struct dpll_device *dpll, >> + void *dpll_priv, u64 *frequency, >> + struct netlink_ext_ack *extack) >> +{ >> + struct ptp_ocp_sma_connector *sma = pin_priv; >> + struct ptp_ocp *bp = dpll_priv; >> + const struct ocp_selector *tbl; >> + int sma_nr = (sma - bp->sma); > > 1) void "()"s here. > 2) why don't you fill the sma_nr in struct ptp_ocp_sma_connector to make > this easier to follow? IDK, just a suggestion, take or leave. > > Same applies to the the rest of similar occurances above. > Will do it, yes. > >> + u32 val; >> + int i; >> + >> + val = bp->sma_op->get(bp, sma_nr); >> + tbl = bp->sma_op->tbl[sma->mode]; >> + for (i = 0; tbl[i].name; i++) >> + if (val == tbl[i].value) { >> + *frequency = tbl[i].frequency; >> + return 0; >> + } >> + >> + return -EINVAL; >> +} >> + >> +static const struct dpll_device_ops dpll_ops = { >> + .lock_status_get = ptp_ocp_dpll_lock_status_get, >> + .source_pin_idx_get = ptp_ocp_dpll_source_idx_get, > > This op is a leftover, in dpll core it is not called. This was removed > and agreed that drivers should implement state_on_dpll_get() op for pins > to see which one is connected. > > Please fix here and remove the leftover from DPLL patch #2 as well. > > >> + .mode_get = ptp_ocp_dpll_mode_get, >> + .mode_supported = ptp_ocp_dpll_mode_supported, >> +}; >> + >> +static const struct dpll_pin_ops dpll_pins_ops = { >> + .frequency_get = ptp_ocp_dpll_frequency_get, >> + .frequency_set = ptp_ocp_dpll_frequency_set, >> + .direction_get = ptp_ocp_dpll_direction_get, >> + .direction_set = ptp_ocp_dpll_direction_set, >> +}; >> + >> static int >> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> { >> struct devlink *devlink; >> struct ptp_ocp *bp; >> - int err; >> + int err, i; >> + u64 clkid; >> >> devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev->dev); >> if (!devlink) { >> @@ -4227,8 +4410,39 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> >> ptp_ocp_info(bp); >> devlink_register(devlink); >> - return 0; >> >> + clkid = pci_get_dsn(pdev); >> + bp->dpll = dpll_device_get(clkid, 0, THIS_MODULE); > > I suggested this the last time, but again: Could you please: > 1) rename dpll_device_get to __dpll_device_get > 2) introduce dpll_device_get as a macro filling up THIS_MODULE > > Then drivers will just call always: > bp->dpll = dpll_device_get(clkid, 0); > and the macro will fillup the module automatically. > > Please do the same for dpll_pin_get() > Not sure why I have missed this part, but sure will do it. > >> + if (IS_ERR(bp->dpll)) { >> + dev_err(&pdev->dev, "dpll_device_alloc failed\n"); > > You need to fix your error path to call devlink_unregister() in this > case. > > >> + goto out; >> + } >> + >> + err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp, &pdev->dev); >> + if (err) > > You need to fix your error path to call dpll_device_put() in this > case. > Ok, I'll re-check and update the error path for the next version. > >> + goto out; >> + >> + for (i = 0; i < OCP_SMA_NUM; i++) { >> + bp->sma[i].dpll_pin = dpll_pin_get(clkid, i, THIS_MODULE, &bp->sma[i].dpll_prop); >> + if (IS_ERR(bp->sma[i].dpll_pin)) >> + goto out_dpll; >> + >> + err = dpll_pin_register(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, >> + &bp->sma[i], NULL); >> + if (err) { >> + dpll_pin_put(bp->sma[i].dpll_pin); >> + goto out_dpll; >> + } >> + } >> + >> + return 0; >> +out_dpll: >> + while (i) { >> + --i; > > while (i--) { > ? > >> + dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, &bp->sma[i]); >> + dpll_pin_put(bp->sma[i].dpll_pin); >> + } >> + dpll_device_put(bp->dpll); >> out: >> ptp_ocp_detach(bp); >> out_disable: >> @@ -4243,7 +4457,16 @@ ptp_ocp_remove(struct pci_dev *pdev) >> { >> struct ptp_ocp *bp = pci_get_drvdata(pdev); >> struct devlink *devlink = priv_to_devlink(bp); >> + int i; >> >> + for (i = 0; i < OCP_SMA_NUM; i++) { >> + if (bp->sma[i].dpll_pin) { > > Remove this pointless check. It is always true. Agree. > > >> + dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, bp); >> + dpll_pin_put(bp->sma[i].dpll_pin); >> + } >> + } >> + dpll_device_unregister(bp->dpll, &dpll_ops, bp); >> + dpll_device_put(bp->dpll); >> devlink_unregister(devlink); >> ptp_ocp_detach(bp); >> pci_disable_device(pdev); >> -- >> 2.34.1 >>
Fri, May 05, 2023 at 03:43:08PM CEST, vadim.fedorenko@linux.dev wrote: >On 04/05/2023 10:27, Jiri Pirko wrote: >> Fri, Apr 28, 2023 at 02:20:07AM CEST, vadfed@meta.com wrote: >> > From: Vadim Fedorenko <vadim.fedorenko@linux.dev> >> > >> > Implement basic DPLL operations in ptp_ocp driver as the >> > simplest example of using new subsystem. >> > >> > Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev> >> > --- >> > drivers/ptp/Kconfig | 1 + >> > drivers/ptp/ptp_ocp.c | 327 +++++++++++++++++++++++++++++++++++------- >> > 2 files changed, 276 insertions(+), 52 deletions(-) >> > >> > diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig >> > index b00201d81313..e3575c2e34dc 100644 >> > --- a/drivers/ptp/Kconfig >> > +++ b/drivers/ptp/Kconfig >> > @@ -177,6 +177,7 @@ config PTP_1588_CLOCK_OCP >> > depends on COMMON_CLK >> > select NET_DEVLINK >> > select CRC16 >> > + 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 2b63f3487645..100e5da0aeb3 100644 >> > --- a/drivers/ptp/ptp_ocp.c >> > +++ b/drivers/ptp/ptp_ocp.c >> > @@ -23,6 +23,7 @@ >> > #include <linux/mtd/mtd.h> >> > #include <linux/nvmem-consumer.h> >> > #include <linux/crc16.h> >> > +#include <linux/dpll.h> >> > >> > #define PCI_VENDOR_ID_FACEBOOK 0x1d9b >> > #define PCI_DEVICE_ID_FACEBOOK_TIMECARD 0x0400 >> > @@ -261,12 +262,21 @@ enum ptp_ocp_sma_mode { >> > SMA_MODE_OUT, >> > }; >> > >> > +static struct dpll_pin_frequency ptp_ocp_sma_freq[] = { >> >> const > >Forgot about this one, will change it. > >> >> > + DPLL_PIN_FREQUENCY_1PPS, >> > + DPLL_PIN_FREQUENCY_10MHZ, >> > + DPLL_PIN_FREQUENCY_IRIG_B, >> > + DPLL_PIN_FREQUENCY_DCF77, >> > +}; >> > + >> > struct ptp_ocp_sma_connector { >> > enum ptp_ocp_sma_mode mode; >> > bool fixed_fcn; >> > bool fixed_dir; >> > bool disabled; >> > u8 default_fcn; >> > + struct dpll_pin *dpll_pin; >> > + struct dpll_pin_properties dpll_prop; >> > }; >> > >> > struct ocp_attr_group { >> > @@ -295,6 +305,7 @@ struct ptp_ocp_serial_port { >> > >> > #define OCP_BOARD_ID_LEN 13 >> > #define OCP_SERIAL_LEN 6 >> > +#define OCP_SMA_NUM 4 >> > >> > struct ptp_ocp { >> > struct pci_dev *pdev; >> > @@ -351,8 +362,9 @@ struct ptp_ocp { >> > u32 ts_window_adjust; >> > u64 fw_cap; >> > struct ptp_ocp_signal signal[4]; >> > - struct ptp_ocp_sma_connector sma[4]; >> > + struct ptp_ocp_sma_connector sma[OCP_SMA_NUM]; >> > const struct ocp_sma_op *sma_op; >> > + struct dpll_device *dpll; >> > }; >> > >> > #define OCP_REQ_TIMESTAMP BIT(0) >> > @@ -836,6 +848,7 @@ static DEFINE_IDR(ptp_ocp_idr); >> > struct ocp_selector { >> > const char *name; >> > int value; >> > + u64 frequency; >> > }; >> > >> > static const struct ocp_selector ptp_ocp_clock[] = { >> > @@ -856,31 +869,31 @@ 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, .frequency = 10000000 }, >> > + { .name = "PPS1", .value = 0x0001, .frequency = 1 }, >> > + { .name = "PPS2", .value = 0x0002, .frequency = 1 }, >> > + { .name = "TS1", .value = 0x0004, .frequency = 0 }, >> > + { .name = "TS2", .value = 0x0008, .frequency = 0 }, >> > + { .name = "IRIG", .value = 0x0010, .frequency = 10000 }, >> > + { .name = "DCF", .value = 0x0020, .frequency = 77500 }, >> > + { .name = "TS3", .value = 0x0040, .frequency = 0 }, >> > + { .name = "TS4", .value = 0x0080, .frequency = 0 }, >> > + { .name = "FREQ1", .value = 0x0100, .frequency = 0 }, >> > + { .name = "FREQ2", .value = 0x0200, .frequency = 0 }, >> > + { .name = "FREQ3", .value = 0x0400, .frequency = 0 }, >> > + { .name = "FREQ4", .value = 0x0800, .frequency = 0 }, >> > + { .name = "None", .value = SMA_DISABLE, .frequency = 0 }, >> > { } >> > }; >> > >> > 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 = "10Mhz", .value = 0x0000, .frequency = 10000000 }, >> > + { .name = "PHC", .value = 0x0001, .frequency = 1 }, >> > + { .name = "MAC", .value = 0x0002, .frequency = 1 }, >> > + { .name = "GNSS1", .value = 0x0004, .frequency = 1 }, >> > + { .name = "GNSS2", .value = 0x0008, .frequency = 1 }, >> > + { .name = "IRIG", .value = 0x0010, .frequency = 10000 }, >> > + { .name = "DCF", .value = 0x0020, .frequency = 77000 }, >> > { .name = "GEN1", .value = 0x0040 }, >> > { .name = "GEN2", .value = 0x0080 }, >> > { .name = "GEN3", .value = 0x0100 }, >> > @@ -891,15 +904,15 @@ static const struct ocp_selector ptp_ocp_sma_out[] = { >> > }; >> > >> > static const struct ocp_selector ptp_ocp_art_sma_in[] = { >> > - { .name = "PPS1", .value = 0x0001 }, >> > - { .name = "10Mhz", .value = 0x0008 }, >> > + { .name = "PPS1", .value = 0x0001, .frequency = 1 }, >> > + { .name = "10Mhz", .value = 0x0008, .frequency = 1000000 }, >> > { } >> > }; >> > >> > static const struct ocp_selector ptp_ocp_art_sma_out[] = { >> > - { .name = "PHC", .value = 0x0002 }, >> > - { .name = "GNSS", .value = 0x0004 }, >> > - { .name = "10Mhz", .value = 0x0010 }, >> > + { .name = "PHC", .value = 0x0002, .frequency = 1 }, >> > + { .name = "GNSS", .value = 0x0004, .frequency = 1 }, >> > + { .name = "10Mhz", .value = 0x0010, .frequency = 10000000 }, >> > { } >> > }; >> > >> > @@ -2283,22 +2296,34 @@ ptp_ocp_sma_fb_set_inputs(struct ptp_ocp *bp, int sma_nr, u32 val) >> > static void >> > ptp_ocp_sma_fb_init(struct ptp_ocp *bp) >> > { >> > + struct dpll_pin_properties prop = { >> >> Why don't you have this as static const outside the function? >> > >Because I'm changing label string for every pin. I cannot change it in >the const object. No you don't. You just copy prop to bp->sma[i].dpll_prop. Prop does not change. > >> >> > + .label = NULL, >> >> Pointless init. > >Agree > >> > + .type = DPLL_PIN_TYPE_EXT, >> > + .capabilities = DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE, >> > + .freq_supported_num = ARRAY_SIZE(ptp_ocp_sma_freq), >> > + .freq_supported = ptp_ocp_sma_freq, >> > + >> > + }; >> > u32 reg; >> > int i; >> > >> > /* defaults */ >> > + for (i = 0; i < OCP_SMA_NUM; i++) { >> > + bp->sma[i].default_fcn = i & 1; >> > + bp->sma[i].dpll_prop = prop; >> > + bp->sma[i].dpll_prop.label = bp->ptp_info.pin_config[i].name; >> > + } >> > bp->sma[0].mode = SMA_MODE_IN; >> > 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) { >> > - for (i = 0; i < 4; i++) { >> > + for (i = 0; i < OCP_SMA_NUM; i++) { >> > bp->sma[i].fixed_fcn = true; >> > bp->sma[i].fixed_dir = true; >> > + bp->sma[1].dpll_prop.capabilities &= >> > + ~DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE; >> > } >> > return; >> > } >> > @@ -2308,7 +2333,7 @@ ptp_ocp_sma_fb_init(struct ptp_ocp *bp) >> > */ >> > reg = ioread32(&bp->sma_map2->gpio2); >> > if (reg == 0xffffffff) { >> > - for (i = 0; i < 4; i++) >> > + for (i = 0; i < OCP_SMA_NUM; i++) >> > bp->sma[i].fixed_dir = true; >> > } else { >> > reg = ioread32(&bp->sma_map1->gpio1); >> > @@ -2330,7 +2355,7 @@ static const struct ocp_sma_op ocp_fb_sma_op = { >> > }; >> > >> > static int >> > -ptp_ocp_fb_set_pins(struct ptp_ocp *bp) >> > +ptp_ocp_set_pins(struct ptp_ocp *bp) >> > { >> > struct ptp_pin_desc *config; >> > int i; >> > @@ -2397,16 +2422,16 @@ ptp_ocp_fb_board_init(struct ptp_ocp *bp, struct ocp_resource *r) >> > >> > ptp_ocp_tod_init(bp); >> > ptp_ocp_nmea_out_init(bp); >> > - ptp_ocp_sma_init(bp); >> > ptp_ocp_signal_init(bp); >> > >> > err = ptp_ocp_attr_group_add(bp, fb_timecard_groups); >> > if (err) >> > return err; >> > >> > - err = ptp_ocp_fb_set_pins(bp); >> > + err = ptp_ocp_set_pins(bp); >> > if (err) >> > return err; >> > + ptp_ocp_sma_init(bp); >> > >> > return ptp_ocp_init_clock(bp); >> > } >> > @@ -2446,6 +2471,14 @@ ptp_ocp_register_resources(struct ptp_ocp *bp, kernel_ulong_t driver_data) >> > static void >> > ptp_ocp_art_sma_init(struct ptp_ocp *bp) >> > { >> > + struct dpll_pin_properties prop = { >> > + .label = NULL, >> > + .type = DPLL_PIN_TYPE_EXT, >> > + .capabilities = 0, >> >> Same comment as to the similar prop struct above. Plus another pointless >> init here. >> > >Will remove pointless init. > >> >> > + .freq_supported_num = ARRAY_SIZE(ptp_ocp_sma_freq), >> > + .freq_supported = ptp_ocp_sma_freq, >> > + >> > + }; >> > u32 reg; >> > int i; >> > >> > @@ -2460,16 +2493,16 @@ ptp_ocp_art_sma_init(struct ptp_ocp *bp) >> > bp->sma[2].default_fcn = 0x10; /* OUT: 10Mhz */ >> > bp->sma[3].default_fcn = 0x02; /* OUT: PHC */ >> > >> > - /* If no SMA map, the pin functions and directions are fixed. */ >> > - if (!bp->art_sma) { >> > - for (i = 0; i < 4; i++) { >> > + >> > + for (i = 0; i < OCP_SMA_NUM; i++) { >> > + /* If no SMA map, the pin functions and directions are fixed. */ >> > + bp->sma[i].dpll_prop = prop; >> > + bp->sma[i].dpll_prop.label = bp->ptp_info.pin_config[i].name; >> > + if (!bp->art_sma) { >> > bp->sma[i].fixed_fcn = true; >> > bp->sma[i].fixed_dir = true; >> > + continue; >> > } >> > - return; >> > - } >> > - >> > - for (i = 0; i < 4; i++) { >> > reg = ioread32(&bp->art_sma->map[i].gpio); >> > >> > switch (reg & 0xff) { >> > @@ -2480,9 +2513,13 @@ ptp_ocp_art_sma_init(struct ptp_ocp *bp) >> > case 1: >> > case 8: >> > bp->sma[i].mode = SMA_MODE_IN; >> > + bp->sma[i].dpll_prop.capabilities = >> > + DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE; >> > break; >> > default: >> > bp->sma[i].mode = SMA_MODE_OUT; >> > + bp->sma[i].dpll_prop.capabilities = >> > + DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE; >> > break; >> > } >> > } >> > @@ -2549,6 +2586,9 @@ ptp_ocp_art_board_init(struct ptp_ocp *bp, struct ocp_resource *r) >> > /* Enable MAC serial port during initialisation */ >> > iowrite32(1, &bp->board_config->mro50_serial_activate); >> > >> > + err = ptp_ocp_set_pins(bp); >> > + if (err) >> > + return err; >> > ptp_ocp_sma_init(bp); >> > >> > err = ptp_ocp_attr_group_add(bp, art_timecard_groups); >> > @@ -2690,16 +2730,9 @@ sma4_show(struct device *dev, struct device_attribute *attr, char *buf) >> > } >> > >> > static int >> > -ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) >> > +ptp_ocp_sma_store_val(struct ptp_ocp *bp, int val, enum ptp_ocp_sma_mode mode, int sma_nr) >> > { >> > struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1]; >> > - enum ptp_ocp_sma_mode mode; >> > - int val; >> > - >> > - mode = sma->mode; >> > - val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode); >> > - if (val < 0) >> > - return val; >> > >> > if (sma->fixed_dir && (mode != sma->mode || val & SMA_DISABLE)) >> > return -EOPNOTSUPP; >> > @@ -2734,6 +2767,20 @@ ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) >> > return val; >> > } >> > >> > +static int >> > +ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) >> > +{ >> > + struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1]; >> > + enum ptp_ocp_sma_mode mode; >> > + int val; >> > + >> > + mode = sma->mode; >> > + val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode); >> > + if (val < 0) >> > + return val; >> > + return ptp_ocp_sma_store_val(bp, val, mode, sma_nr); >> > +} >> > + >> > static ssize_t >> > sma1_store(struct device *dev, struct device_attribute *attr, >> > const char *buf, size_t count) >> > @@ -4172,12 +4219,148 @@ ptp_ocp_detach(struct ptp_ocp *bp) >> > device_unregister(&bp->dev); >> > } >> > >> > +static int ptp_ocp_dpll_lock_status_get(const struct dpll_device *dpll, >> > + void *priv, >> > + enum dpll_lock_status *status, >> > + struct netlink_ext_ack *extack) >> > +{ >> > + struct ptp_ocp *bp = priv; >> > + int sync; >> > + >> > + sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; >> > + *status = sync ? DPLL_LOCK_STATUS_LOCKED : DPLL_LOCK_STATUS_UNLOCKED; >> >> Does your device support event delivery in case of the status change? >> ice and mlx5 drivers do poll for changes in this area anyway. It's a >> part of this patchset. You should do the same if your device does >> not support events. >> >> Could you please implement notifications using >> dpll_device_notify() for status change and dpll_pin_notify() for pin >> state change? >> > >We are working on implementation of interrupt-based notifications, that's why >I didn't implement polling code. Hopefully it will be ready before the >non-RFC patchset submission. Do polling implementation if not and replace by interrupt driver later. Point is, there have to be notifications from day 1. > >> >> > + >> > + return 0; >> > +} >> > + >> > +static int ptp_ocp_dpll_source_idx_get(const struct dpll_device *dpll, >> > + void *priv, u32 *idx, >> > + struct netlink_ext_ack *extack) >> > +{ >> > + struct ptp_ocp *bp = priv; >> > + >> > + if (bp->pps_select) { >> > + *idx = ioread32(&bp->pps_select->gpio1); >> > + return 0; >> > + } >> > + return -EINVAL; >> > +} >> > + >> > +static int ptp_ocp_dpll_mode_get(const struct dpll_device *dpll, void *priv, >> > + u32 *mode, struct netlink_ext_ack *extack) >> > +{ >> > + *mode = DPLL_MODE_AUTOMATIC; >> > + return 0; >> > +} >> > + >> > +static bool ptp_ocp_dpll_mode_supported(const struct dpll_device *dpll, >> > + void *priv, const enum dpll_mode mode, >> > + struct netlink_ext_ack *extack) >> > +{ >> > + return mode == DPLL_MODE_AUTOMATIC; >> > +} >> > + >> > +static int ptp_ocp_dpll_direction_get(const struct dpll_pin *pin, >> > + void *pin_priv, >> > + const struct dpll_device *dpll, >> > + void *priv, >> > + enum dpll_pin_direction *direction, >> > + struct netlink_ext_ack *extack) >> > +{ >> > + struct ptp_ocp_sma_connector *sma = pin_priv; >> > + >> > + *direction = sma->mode == SMA_MODE_IN ? >> > + DPLL_PIN_DIRECTION_SOURCE : >> > + DPLL_PIN_DIRECTION_OUTPUT; >> > + return 0; >> > +} >> > + >> > +static int ptp_ocp_dpll_direction_set(const struct dpll_pin *pin, >> > + void *pin_priv, >> > + const struct dpll_device *dpll, >> > + void *dpll_priv, >> > + enum dpll_pin_direction direction, >> > + struct netlink_ext_ack *extack) >> > +{ >> > + struct ptp_ocp_sma_connector *sma = pin_priv; >> > + struct ptp_ocp *bp = dpll_priv; >> > + enum ptp_ocp_sma_mode mode; >> > + int sma_nr = (sma - bp->sma); >> > + >> > + if (sma->fixed_dir) >> >> I believe that this is a pointless check as DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE >> is not set and therefore the check in dpll_pin_direction_set() will be >> true and -EOPNOTSUPP will be returned from there. >> Remove this. > >Yep, will do it. > >> >> > + return -EOPNOTSUPP; >> > + mode = direction == DPLL_PIN_DIRECTION_SOURCE ? >> > + SMA_MODE_IN : SMA_MODE_OUT; >> > + return ptp_ocp_sma_store_val(bp, 0, mode, sma_nr); >> >> You need sma_nr just here. Why can't you change ptp_ocp_sma_store_val() >> to accept struct ptp_ocp_sma_connector * instead avoiding the need for >> tne sma_nr completely? >> > >I wanted to add minimal changes to the driver, I will consider changing this >as a separate net-next patch. Hmm, you do change ptp_ocp_sma_store_val() here anyway. Could be an extra patch in this patchset before this one if you want to have it separate. > >> >> > +} >> > + >> > +static int ptp_ocp_dpll_frequency_set(const struct dpll_pin *pin, >> > + void *pin_priv, >> > + const struct dpll_device *dpll, >> > + void *dpll_priv, u64 frequency, >> > + struct netlink_ext_ack *extack) >> > +{ >> > + struct ptp_ocp_sma_connector *sma = pin_priv; >> > + struct ptp_ocp *bp = dpll_priv; >> > + const struct ocp_selector *tbl; >> > + int sma_nr = (sma - bp->sma); >> > + int val, i; >> > + >> > + if (sma->fixed_fcn) >> >> In that case, just fill up a single frequency in the properties, >> avoid this check-fail and let the dpll core handle it. >> > >Makes sense. > >> >> > + return -EOPNOTSUPP; >> > + >> > + tbl = bp->sma_op->tbl[sma->mode]; >> > + for (i = 0; tbl[i].name; i++) >> > + if (tbl[i].frequency == frequency) >> > + return ptp_ocp_sma_store_val(bp, val, sma->mode, sma_nr); >> > + return -EINVAL; >> > +} >> > + >> > +static int ptp_ocp_dpll_frequency_get(const struct dpll_pin *pin, >> > + void *pin_priv, >> > + const struct dpll_device *dpll, >> > + void *dpll_priv, u64 *frequency, >> > + struct netlink_ext_ack *extack) >> > +{ >> > + struct ptp_ocp_sma_connector *sma = pin_priv; >> > + struct ptp_ocp *bp = dpll_priv; >> > + const struct ocp_selector *tbl; >> > + int sma_nr = (sma - bp->sma); >> >> 1) void "()"s here. >> 2) why don't you fill the sma_nr in struct ptp_ocp_sma_connector to make >> this easier to follow? IDK, just a suggestion, take or leave. >> >> Same applies to the the rest of similar occurances above. >> > >Will do it, yes. > >> >> > + u32 val; >> > + int i; >> > + >> > + val = bp->sma_op->get(bp, sma_nr); >> > + tbl = bp->sma_op->tbl[sma->mode]; >> > + for (i = 0; tbl[i].name; i++) >> > + if (val == tbl[i].value) { >> > + *frequency = tbl[i].frequency; >> > + return 0; >> > + } >> > + >> > + return -EINVAL; >> > +} >> > + >> > +static const struct dpll_device_ops dpll_ops = { >> > + .lock_status_get = ptp_ocp_dpll_lock_status_get, >> > + .source_pin_idx_get = ptp_ocp_dpll_source_idx_get, >> >> This op is a leftover, in dpll core it is not called. This was removed >> and agreed that drivers should implement state_on_dpll_get() op for pins >> to see which one is connected. >> >> Please fix here and remove the leftover from DPLL patch #2 as well. Did you miss this by any chance? >> >> >> > + .mode_get = ptp_ocp_dpll_mode_get, >> > + .mode_supported = ptp_ocp_dpll_mode_supported, >> > +}; >> > + >> > +static const struct dpll_pin_ops dpll_pins_ops = { >> > + .frequency_get = ptp_ocp_dpll_frequency_get, >> > + .frequency_set = ptp_ocp_dpll_frequency_set, >> > + .direction_get = ptp_ocp_dpll_direction_get, >> > + .direction_set = ptp_ocp_dpll_direction_set, >> > +}; >> > + >> > static int >> > ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> > { >> > struct devlink *devlink; >> > struct ptp_ocp *bp; >> > - int err; >> > + int err, i; >> > + u64 clkid; >> > >> > devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev->dev); >> > if (!devlink) { >> > @@ -4227,8 +4410,39 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> > >> > ptp_ocp_info(bp); >> > devlink_register(devlink); >> > - return 0; >> > >> > + clkid = pci_get_dsn(pdev); >> > + bp->dpll = dpll_device_get(clkid, 0, THIS_MODULE); >> >> I suggested this the last time, but again: Could you please: >> 1) rename dpll_device_get to __dpll_device_get >> 2) introduce dpll_device_get as a macro filling up THIS_MODULE >> >> Then drivers will just call always: >> bp->dpll = dpll_device_get(clkid, 0); >> and the macro will fillup the module automatically. >> >> Please do the same for dpll_pin_get() >> > >Not sure why I have missed this part, but sure will do it. Awesome. > >> >> > + if (IS_ERR(bp->dpll)) { >> > + dev_err(&pdev->dev, "dpll_device_alloc failed\n"); >> >> You need to fix your error path to call devlink_unregister() in this >> case. >> >> >> > + goto out; >> > + } >> > + >> > + err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp, &pdev->dev); >> > + if (err) >> >> You need to fix your error path to call dpll_device_put() in this >> case. >> > >Ok, I'll re-check and update the error path for the next version. > >> >> > + goto out; >> > + >> > + for (i = 0; i < OCP_SMA_NUM; i++) { >> > + bp->sma[i].dpll_pin = dpll_pin_get(clkid, i, THIS_MODULE, &bp->sma[i].dpll_prop); >> > + if (IS_ERR(bp->sma[i].dpll_pin)) >> > + goto out_dpll; >> > + >> > + err = dpll_pin_register(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, >> > + &bp->sma[i], NULL); >> > + if (err) { >> > + dpll_pin_put(bp->sma[i].dpll_pin); >> > + goto out_dpll; >> > + } >> > + } >> > + >> > + return 0; >> > +out_dpll: >> > + while (i) { >> > + --i; >> >> while (i--) { >> ? >> >> > + dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, &bp->sma[i]); >> > + dpll_pin_put(bp->sma[i].dpll_pin); >> > + } >> > + dpll_device_put(bp->dpll); >> > out: >> > ptp_ocp_detach(bp); >> > out_disable: >> > @@ -4243,7 +4457,16 @@ ptp_ocp_remove(struct pci_dev *pdev) >> > { >> > struct ptp_ocp *bp = pci_get_drvdata(pdev); >> > struct devlink *devlink = priv_to_devlink(bp); >> > + int i; >> > >> > + for (i = 0; i < OCP_SMA_NUM; i++) { >> > + if (bp->sma[i].dpll_pin) { >> >> Remove this pointless check. It is always true. > >Agree. > >> >> >> > + dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, bp); >> > + dpll_pin_put(bp->sma[i].dpll_pin); >> > + } >> > + } >> > + dpll_device_unregister(bp->dpll, &dpll_ops, bp); >> > + dpll_device_put(bp->dpll); >> > devlink_unregister(devlink); >> > ptp_ocp_detach(bp); >> > pci_disable_device(pdev); >> > -- >> > 2.34.1 >> > >
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index b00201d81313..e3575c2e34dc 100644 --- a/drivers/ptp/Kconfig +++ b/drivers/ptp/Kconfig @@ -177,6 +177,7 @@ config PTP_1588_CLOCK_OCP depends on COMMON_CLK select NET_DEVLINK select CRC16 + 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 2b63f3487645..100e5da0aeb3 100644 --- a/drivers/ptp/ptp_ocp.c +++ b/drivers/ptp/ptp_ocp.c @@ -23,6 +23,7 @@ #include <linux/mtd/mtd.h> #include <linux/nvmem-consumer.h> #include <linux/crc16.h> +#include <linux/dpll.h> #define PCI_VENDOR_ID_FACEBOOK 0x1d9b #define PCI_DEVICE_ID_FACEBOOK_TIMECARD 0x0400 @@ -261,12 +262,21 @@ enum ptp_ocp_sma_mode { SMA_MODE_OUT, }; +static struct dpll_pin_frequency ptp_ocp_sma_freq[] = { + DPLL_PIN_FREQUENCY_1PPS, + DPLL_PIN_FREQUENCY_10MHZ, + DPLL_PIN_FREQUENCY_IRIG_B, + DPLL_PIN_FREQUENCY_DCF77, +}; + struct ptp_ocp_sma_connector { enum ptp_ocp_sma_mode mode; bool fixed_fcn; bool fixed_dir; bool disabled; u8 default_fcn; + struct dpll_pin *dpll_pin; + struct dpll_pin_properties dpll_prop; }; struct ocp_attr_group { @@ -295,6 +305,7 @@ struct ptp_ocp_serial_port { #define OCP_BOARD_ID_LEN 13 #define OCP_SERIAL_LEN 6 +#define OCP_SMA_NUM 4 struct ptp_ocp { struct pci_dev *pdev; @@ -351,8 +362,9 @@ struct ptp_ocp { u32 ts_window_adjust; u64 fw_cap; struct ptp_ocp_signal signal[4]; - struct ptp_ocp_sma_connector sma[4]; + struct ptp_ocp_sma_connector sma[OCP_SMA_NUM]; const struct ocp_sma_op *sma_op; + struct dpll_device *dpll; }; #define OCP_REQ_TIMESTAMP BIT(0) @@ -836,6 +848,7 @@ static DEFINE_IDR(ptp_ocp_idr); struct ocp_selector { const char *name; int value; + u64 frequency; }; static const struct ocp_selector ptp_ocp_clock[] = { @@ -856,31 +869,31 @@ 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, .frequency = 10000000 }, + { .name = "PPS1", .value = 0x0001, .frequency = 1 }, + { .name = "PPS2", .value = 0x0002, .frequency = 1 }, + { .name = "TS1", .value = 0x0004, .frequency = 0 }, + { .name = "TS2", .value = 0x0008, .frequency = 0 }, + { .name = "IRIG", .value = 0x0010, .frequency = 10000 }, + { .name = "DCF", .value = 0x0020, .frequency = 77500 }, + { .name = "TS3", .value = 0x0040, .frequency = 0 }, + { .name = "TS4", .value = 0x0080, .frequency = 0 }, + { .name = "FREQ1", .value = 0x0100, .frequency = 0 }, + { .name = "FREQ2", .value = 0x0200, .frequency = 0 }, + { .name = "FREQ3", .value = 0x0400, .frequency = 0 }, + { .name = "FREQ4", .value = 0x0800, .frequency = 0 }, + { .name = "None", .value = SMA_DISABLE, .frequency = 0 }, { } }; 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 = "10Mhz", .value = 0x0000, .frequency = 10000000 }, + { .name = "PHC", .value = 0x0001, .frequency = 1 }, + { .name = "MAC", .value = 0x0002, .frequency = 1 }, + { .name = "GNSS1", .value = 0x0004, .frequency = 1 }, + { .name = "GNSS2", .value = 0x0008, .frequency = 1 }, + { .name = "IRIG", .value = 0x0010, .frequency = 10000 }, + { .name = "DCF", .value = 0x0020, .frequency = 77000 }, { .name = "GEN1", .value = 0x0040 }, { .name = "GEN2", .value = 0x0080 }, { .name = "GEN3", .value = 0x0100 }, @@ -891,15 +904,15 @@ static const struct ocp_selector ptp_ocp_sma_out[] = { }; static const struct ocp_selector ptp_ocp_art_sma_in[] = { - { .name = "PPS1", .value = 0x0001 }, - { .name = "10Mhz", .value = 0x0008 }, + { .name = "PPS1", .value = 0x0001, .frequency = 1 }, + { .name = "10Mhz", .value = 0x0008, .frequency = 1000000 }, { } }; static const struct ocp_selector ptp_ocp_art_sma_out[] = { - { .name = "PHC", .value = 0x0002 }, - { .name = "GNSS", .value = 0x0004 }, - { .name = "10Mhz", .value = 0x0010 }, + { .name = "PHC", .value = 0x0002, .frequency = 1 }, + { .name = "GNSS", .value = 0x0004, .frequency = 1 }, + { .name = "10Mhz", .value = 0x0010, .frequency = 10000000 }, { } }; @@ -2283,22 +2296,34 @@ ptp_ocp_sma_fb_set_inputs(struct ptp_ocp *bp, int sma_nr, u32 val) static void ptp_ocp_sma_fb_init(struct ptp_ocp *bp) { + struct dpll_pin_properties prop = { + .label = NULL, + .type = DPLL_PIN_TYPE_EXT, + .capabilities = DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE, + .freq_supported_num = ARRAY_SIZE(ptp_ocp_sma_freq), + .freq_supported = ptp_ocp_sma_freq, + + }; u32 reg; int i; /* defaults */ + for (i = 0; i < OCP_SMA_NUM; i++) { + bp->sma[i].default_fcn = i & 1; + bp->sma[i].dpll_prop = prop; + bp->sma[i].dpll_prop.label = bp->ptp_info.pin_config[i].name; + } bp->sma[0].mode = SMA_MODE_IN; 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) { - for (i = 0; i < 4; i++) { + for (i = 0; i < OCP_SMA_NUM; i++) { bp->sma[i].fixed_fcn = true; bp->sma[i].fixed_dir = true; + bp->sma[1].dpll_prop.capabilities &= + ~DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE; } return; } @@ -2308,7 +2333,7 @@ ptp_ocp_sma_fb_init(struct ptp_ocp *bp) */ reg = ioread32(&bp->sma_map2->gpio2); if (reg == 0xffffffff) { - for (i = 0; i < 4; i++) + for (i = 0; i < OCP_SMA_NUM; i++) bp->sma[i].fixed_dir = true; } else { reg = ioread32(&bp->sma_map1->gpio1); @@ -2330,7 +2355,7 @@ static const struct ocp_sma_op ocp_fb_sma_op = { }; static int -ptp_ocp_fb_set_pins(struct ptp_ocp *bp) +ptp_ocp_set_pins(struct ptp_ocp *bp) { struct ptp_pin_desc *config; int i; @@ -2397,16 +2422,16 @@ ptp_ocp_fb_board_init(struct ptp_ocp *bp, struct ocp_resource *r) ptp_ocp_tod_init(bp); ptp_ocp_nmea_out_init(bp); - ptp_ocp_sma_init(bp); ptp_ocp_signal_init(bp); err = ptp_ocp_attr_group_add(bp, fb_timecard_groups); if (err) return err; - err = ptp_ocp_fb_set_pins(bp); + err = ptp_ocp_set_pins(bp); if (err) return err; + ptp_ocp_sma_init(bp); return ptp_ocp_init_clock(bp); } @@ -2446,6 +2471,14 @@ ptp_ocp_register_resources(struct ptp_ocp *bp, kernel_ulong_t driver_data) static void ptp_ocp_art_sma_init(struct ptp_ocp *bp) { + struct dpll_pin_properties prop = { + .label = NULL, + .type = DPLL_PIN_TYPE_EXT, + .capabilities = 0, + .freq_supported_num = ARRAY_SIZE(ptp_ocp_sma_freq), + .freq_supported = ptp_ocp_sma_freq, + + }; u32 reg; int i; @@ -2460,16 +2493,16 @@ ptp_ocp_art_sma_init(struct ptp_ocp *bp) bp->sma[2].default_fcn = 0x10; /* OUT: 10Mhz */ bp->sma[3].default_fcn = 0x02; /* OUT: PHC */ - /* If no SMA map, the pin functions and directions are fixed. */ - if (!bp->art_sma) { - for (i = 0; i < 4; i++) { + + for (i = 0; i < OCP_SMA_NUM; i++) { + /* If no SMA map, the pin functions and directions are fixed. */ + bp->sma[i].dpll_prop = prop; + bp->sma[i].dpll_prop.label = bp->ptp_info.pin_config[i].name; + if (!bp->art_sma) { bp->sma[i].fixed_fcn = true; bp->sma[i].fixed_dir = true; + continue; } - return; - } - - for (i = 0; i < 4; i++) { reg = ioread32(&bp->art_sma->map[i].gpio); switch (reg & 0xff) { @@ -2480,9 +2513,13 @@ ptp_ocp_art_sma_init(struct ptp_ocp *bp) case 1: case 8: bp->sma[i].mode = SMA_MODE_IN; + bp->sma[i].dpll_prop.capabilities = + DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE; break; default: bp->sma[i].mode = SMA_MODE_OUT; + bp->sma[i].dpll_prop.capabilities = + DPLL_PIN_CAPS_DIRECTION_CAN_CHANGE; break; } } @@ -2549,6 +2586,9 @@ ptp_ocp_art_board_init(struct ptp_ocp *bp, struct ocp_resource *r) /* Enable MAC serial port during initialisation */ iowrite32(1, &bp->board_config->mro50_serial_activate); + err = ptp_ocp_set_pins(bp); + if (err) + return err; ptp_ocp_sma_init(bp); err = ptp_ocp_attr_group_add(bp, art_timecard_groups); @@ -2690,16 +2730,9 @@ sma4_show(struct device *dev, struct device_attribute *attr, char *buf) } static int -ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) +ptp_ocp_sma_store_val(struct ptp_ocp *bp, int val, enum ptp_ocp_sma_mode mode, int sma_nr) { struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1]; - enum ptp_ocp_sma_mode mode; - int val; - - mode = sma->mode; - val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode); - if (val < 0) - return val; if (sma->fixed_dir && (mode != sma->mode || val & SMA_DISABLE)) return -EOPNOTSUPP; @@ -2734,6 +2767,20 @@ ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) return val; } +static int +ptp_ocp_sma_store(struct ptp_ocp *bp, const char *buf, int sma_nr) +{ + struct ptp_ocp_sma_connector *sma = &bp->sma[sma_nr - 1]; + enum ptp_ocp_sma_mode mode; + int val; + + mode = sma->mode; + val = sma_parse_inputs(bp->sma_op->tbl, buf, &mode); + if (val < 0) + return val; + return ptp_ocp_sma_store_val(bp, val, mode, sma_nr); +} + static ssize_t sma1_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) @@ -4172,12 +4219,148 @@ ptp_ocp_detach(struct ptp_ocp *bp) device_unregister(&bp->dev); } +static int ptp_ocp_dpll_lock_status_get(const struct dpll_device *dpll, + void *priv, + enum dpll_lock_status *status, + struct netlink_ext_ack *extack) +{ + struct ptp_ocp *bp = priv; + int sync; + + sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; + *status = sync ? DPLL_LOCK_STATUS_LOCKED : DPLL_LOCK_STATUS_UNLOCKED; + + return 0; +} + +static int ptp_ocp_dpll_source_idx_get(const struct dpll_device *dpll, + void *priv, u32 *idx, + struct netlink_ext_ack *extack) +{ + struct ptp_ocp *bp = priv; + + if (bp->pps_select) { + *idx = ioread32(&bp->pps_select->gpio1); + return 0; + } + return -EINVAL; +} + +static int ptp_ocp_dpll_mode_get(const struct dpll_device *dpll, void *priv, + u32 *mode, struct netlink_ext_ack *extack) +{ + *mode = DPLL_MODE_AUTOMATIC; + return 0; +} + +static bool ptp_ocp_dpll_mode_supported(const struct dpll_device *dpll, + void *priv, const enum dpll_mode mode, + struct netlink_ext_ack *extack) +{ + return mode == DPLL_MODE_AUTOMATIC; +} + +static int ptp_ocp_dpll_direction_get(const struct dpll_pin *pin, + void *pin_priv, + const struct dpll_device *dpll, + void *priv, + enum dpll_pin_direction *direction, + struct netlink_ext_ack *extack) +{ + struct ptp_ocp_sma_connector *sma = pin_priv; + + *direction = sma->mode == SMA_MODE_IN ? + DPLL_PIN_DIRECTION_SOURCE : + DPLL_PIN_DIRECTION_OUTPUT; + return 0; +} + +static int ptp_ocp_dpll_direction_set(const struct dpll_pin *pin, + void *pin_priv, + const struct dpll_device *dpll, + void *dpll_priv, + enum dpll_pin_direction direction, + struct netlink_ext_ack *extack) +{ + struct ptp_ocp_sma_connector *sma = pin_priv; + struct ptp_ocp *bp = dpll_priv; + enum ptp_ocp_sma_mode mode; + int sma_nr = (sma - bp->sma); + + if (sma->fixed_dir) + return -EOPNOTSUPP; + mode = direction == DPLL_PIN_DIRECTION_SOURCE ? + SMA_MODE_IN : SMA_MODE_OUT; + return ptp_ocp_sma_store_val(bp, 0, mode, sma_nr); +} + +static int ptp_ocp_dpll_frequency_set(const struct dpll_pin *pin, + void *pin_priv, + const struct dpll_device *dpll, + void *dpll_priv, u64 frequency, + struct netlink_ext_ack *extack) +{ + struct ptp_ocp_sma_connector *sma = pin_priv; + struct ptp_ocp *bp = dpll_priv; + const struct ocp_selector *tbl; + int sma_nr = (sma - bp->sma); + int val, i; + + if (sma->fixed_fcn) + return -EOPNOTSUPP; + + tbl = bp->sma_op->tbl[sma->mode]; + for (i = 0; tbl[i].name; i++) + if (tbl[i].frequency == frequency) + return ptp_ocp_sma_store_val(bp, val, sma->mode, sma_nr); + return -EINVAL; +} + +static int ptp_ocp_dpll_frequency_get(const struct dpll_pin *pin, + void *pin_priv, + const struct dpll_device *dpll, + void *dpll_priv, u64 *frequency, + struct netlink_ext_ack *extack) +{ + struct ptp_ocp_sma_connector *sma = pin_priv; + struct ptp_ocp *bp = dpll_priv; + const struct ocp_selector *tbl; + int sma_nr = (sma - bp->sma); + u32 val; + int i; + + val = bp->sma_op->get(bp, sma_nr); + tbl = bp->sma_op->tbl[sma->mode]; + for (i = 0; tbl[i].name; i++) + if (val == tbl[i].value) { + *frequency = tbl[i].frequency; + return 0; + } + + return -EINVAL; +} + +static const struct dpll_device_ops dpll_ops = { + .lock_status_get = ptp_ocp_dpll_lock_status_get, + .source_pin_idx_get = ptp_ocp_dpll_source_idx_get, + .mode_get = ptp_ocp_dpll_mode_get, + .mode_supported = ptp_ocp_dpll_mode_supported, +}; + +static const struct dpll_pin_ops dpll_pins_ops = { + .frequency_get = ptp_ocp_dpll_frequency_get, + .frequency_set = ptp_ocp_dpll_frequency_set, + .direction_get = ptp_ocp_dpll_direction_get, + .direction_set = ptp_ocp_dpll_direction_set, +}; + static int ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) { struct devlink *devlink; struct ptp_ocp *bp; - int err; + int err, i; + u64 clkid; devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev->dev); if (!devlink) { @@ -4227,8 +4410,39 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) ptp_ocp_info(bp); devlink_register(devlink); - return 0; + clkid = pci_get_dsn(pdev); + bp->dpll = dpll_device_get(clkid, 0, THIS_MODULE); + if (IS_ERR(bp->dpll)) { + dev_err(&pdev->dev, "dpll_device_alloc failed\n"); + goto out; + } + + err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp, &pdev->dev); + if (err) + goto out; + + for (i = 0; i < OCP_SMA_NUM; i++) { + bp->sma[i].dpll_pin = dpll_pin_get(clkid, i, THIS_MODULE, &bp->sma[i].dpll_prop); + if (IS_ERR(bp->sma[i].dpll_pin)) + goto out_dpll; + + err = dpll_pin_register(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, + &bp->sma[i], NULL); + if (err) { + dpll_pin_put(bp->sma[i].dpll_pin); + goto out_dpll; + } + } + + return 0; +out_dpll: + while (i) { + --i; + dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, &bp->sma[i]); + dpll_pin_put(bp->sma[i].dpll_pin); + } + dpll_device_put(bp->dpll); out: ptp_ocp_detach(bp); out_disable: @@ -4243,7 +4457,16 @@ ptp_ocp_remove(struct pci_dev *pdev) { struct ptp_ocp *bp = pci_get_drvdata(pdev); struct devlink *devlink = priv_to_devlink(bp); + int i; + for (i = 0; i < OCP_SMA_NUM; i++) { + if (bp->sma[i].dpll_pin) { + dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, bp); + dpll_pin_put(bp->sma[i].dpll_pin); + } + } + dpll_device_unregister(bp->dpll, &dpll_ops, bp); + dpll_device_put(bp->dpll); devlink_unregister(devlink); ptp_ocp_detach(bp); pci_disable_device(pdev);