| Message ID | 20221129213724.10119-5-vfedorenko@novek.ru (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Create common DPLL/clock configuration API | expand |
Tue, Nov 29, 2022 at 10:37:24PM CET, vfedorenko@novek.ru wrote: >From: Vadim Fedorenko <vadfed@fb.com> > >Implement basic DPLL operations in ptp_ocp driver as the >simplest example of using new subsystem. > >Signed-off-by: Vadim Fedorenko <vadfed@fb.com> >--- > drivers/ptp/Kconfig | 1 + > drivers/ptp/ptp_ocp.c | 123 +++++++++++++++++++++++++++++------------- > 2 files changed, 87 insertions(+), 37 deletions(-) > >diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig >index fe4971b65c64..8c4cfabc1bfa 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 154d58cbd9ce..605853ac4a12 100644 >--- a/drivers/ptp/ptp_ocp.c >+++ b/drivers/ptp/ptp_ocp.c >@@ -23,6 +23,8 @@ > #include <linux/mtd/mtd.h> > #include <linux/nvmem-consumer.h> > #include <linux/crc16.h> >+#include <linux/dpll.h> >+#include <uapi/linux/dpll.h> > > #define PCI_VENDOR_ID_FACEBOOK 0x1d9b > #define PCI_DEVICE_ID_FACEBOOK_TIMECARD 0x0400 >@@ -353,6 +355,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) >@@ -835,18 +838,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 }, > { } > }; > >@@ -855,37 +859,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_PIN_SIGNAL_TYPE_10_MHZ }, >+ { .name = "PPS1", .value = 0x0001, .dpll_type = DPLL_PIN_SIGNAL_TYPE_1_PPS }, >+ { .name = "PPS2", .value = 0x0002, .dpll_type = DPLL_PIN_SIGNAL_TYPE_1_PPS }, >+ { .name = "TS1", .value = 0x0004, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "TS2", .value = 0x0008, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "TS3", .value = 0x0040, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "TS4", .value = 0x0080, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "FREQ1", .value = 0x0100, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "FREQ2", .value = 0x0200, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "FREQ3", .value = 0x0400, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "FREQ4", .value = 0x0800, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "None", .value = SMA_DISABLE, .dpll_type = 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 = "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_PIN_SIGNAL_TYPE_10_MHZ }, >+ { .name = "PHC", .value = 0x0001, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "MAC", .value = 0x0002, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "GNSS1", .value = 0x0004, .dpll_type = DPLL_PIN_SIGNAL_TYPE_1_PPS }, >+ { .name = "GNSS2", .value = 0x0008, .dpll_type = DPLL_PIN_SIGNAL_TYPE_1_PPS }, >+ { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "GEN1", .value = 0x0040, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "GEN2", .value = 0x0080, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "GEN3", .value = 0x0100, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "GEN4", .value = 0x0200, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >+ { .name = "GND", .value = 0x2000, .dpll_type = 0 }, >+ { .name = "VCC", .value = 0x4000, .dpll_type = 0 }, > { } > }; > >@@ -4175,12 +4179,41 @@ ptp_ocp_detach(struct ptp_ocp *bp) > device_unregister(&bp->dev); > } > >+static int ptp_ocp_dpll_get_attr(struct dpll_device *dpll, struct dpll_attr *attr) >+{ >+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >+ int sync; >+ >+ sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; >+ dpll_attr_lock_status_set(attr, sync ? DPLL_LOCK_STATUS_LOCKED : DPLL_LOCK_STATUS_UNLOCKED); get,set,confuse. This attr thing sucks, sorry :/ >+ >+ return 0; >+} >+ >+static int ptp_ocp_dpll_pin_get_attr(struct dpll_device *dpll, struct dpll_pin *pin, >+ struct dpll_pin_attr *attr) >+{ >+ dpll_pin_attr_type_set(attr, DPLL_PIN_TYPE_EXT); This is exactly what I was talking about in the cover letter. This is const, should be put into static struct and passed to dpll_device_alloc(). >+ return 0; >+} >+ >+static struct dpll_device_ops dpll_ops = { >+ .get = ptp_ocp_dpll_get_attr, >+}; >+ >+static struct dpll_pin_ops dpll_pin_ops = { >+ .get = ptp_ocp_dpll_pin_get_attr, >+}; >+ > static int > ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) > { >+ const u8 dpll_cookie[DPLL_COOKIE_LEN] = { "OCP" }; >+ char pin_desc[PIN_DESC_LEN]; > struct devlink *devlink; >+ struct dpll_pin *pin; > struct ptp_ocp *bp; >- int err; >+ int err, i; > > devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev->dev); > if (!devlink) { >@@ -4230,6 +4263,20 @@ 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, DPLL_TYPE_PPS, dpll_cookie, pdev->bus->number, bp, &pdev->dev); >+ if (!bp->dpll) { >+ dev_err(&pdev->dev, "dpll_device_alloc failed\n"); >+ goto out; >+ } >+ dpll_device_register(bp->dpll); You still have the 2 step init process. I believe it would be better to just have dpll_device_create/destroy() to do it in one shot. >+ >+ for (i = 0; i < 4; i++) { >+ snprintf(pin_desc, PIN_DESC_LEN, "sma%d", i + 1); >+ pin = dpll_pin_alloc(pin_desc, PIN_DESC_LEN); >+ dpll_pin_register(bp->dpll, pin, &dpll_pin_ops, bp); Same here, no point of having 2 step init. >+ } >+ > return 0; Btw, did you consider having dpll instance here as and auxdev? It would be suitable I believe. It is quite simple to do it. See following patch as an example: commit bd02fd76d1909637c95e8ef13e7fd1e748af910d Author: Jiri Pirko <jiri@nvidia.com> Date: Mon Jul 25 10:29:17 2022 +0200 mlxsw: core_linecards: Introduce per line card auxiliary device > > out: >@@ -4247,6 +4294,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); >-- >2.27.0 >
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Wednesday, November 30, 2022 1:41 PM > >Tue, Nov 29, 2022 at 10:37:24PM CET, vfedorenko@novek.ru wrote: >>From: Vadim Fedorenko <vadfed@fb.com> >> >>Implement basic DPLL operations in ptp_ocp driver as the >>simplest example of using new subsystem. >> >>Signed-off-by: Vadim Fedorenko <vadfed@fb.com> >>--- >> drivers/ptp/Kconfig | 1 + >> drivers/ptp/ptp_ocp.c | 123 +++++++++++++++++++++++++++++------------- >> 2 files changed, 87 insertions(+), 37 deletions(-) >> >>diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig >>index fe4971b65c64..8c4cfabc1bfa 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 154d58cbd9ce..605853ac4a12 100644 >>--- a/drivers/ptp/ptp_ocp.c >>+++ b/drivers/ptp/ptp_ocp.c >>@@ -23,6 +23,8 @@ >> #include <linux/mtd/mtd.h> >> #include <linux/nvmem-consumer.h> >> #include <linux/crc16.h> >>+#include <linux/dpll.h> >>+#include <uapi/linux/dpll.h> >> >> #define PCI_VENDOR_ID_FACEBOOK 0x1d9b >> #define PCI_DEVICE_ID_FACEBOOK_TIMECARD 0x0400 >>@@ -353,6 +355,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) >>@@ -835,18 +838,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 }, >> { } >> }; >> >>@@ -855,37 +859,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_PIN_SIGNAL_TYPE_10_MHZ }, >>+ { .name = "PPS1", .value = 0x0001, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_1_PPS }, >>+ { .name = "PPS2", .value = 0x0002, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_1_PPS }, >>+ { .name = "TS1", .value = 0x0004, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "TS2", .value = 0x0008, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "IRIG", .value = 0x0010, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "DCF", .value = 0x0020, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "TS3", .value = 0x0040, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "TS4", .value = 0x0080, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "FREQ1", .value = 0x0100, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "FREQ2", .value = 0x0200, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "FREQ3", .value = 0x0400, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "FREQ4", .value = 0x0800, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "None", .value = SMA_DISABLE, .dpll_type = 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 = "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_PIN_SIGNAL_TYPE_10_MHZ }, >>+ { .name = "PHC", .value = 0x0001, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "MAC", .value = 0x0002, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "GNSS1", .value = 0x0004, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_1_PPS }, >>+ { .name = "GNSS2", .value = 0x0008, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_1_PPS }, >>+ { .name = "IRIG", .value = 0x0010, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "DCF", .value = 0x0020, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "GEN1", .value = 0x0040, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "GEN2", .value = 0x0080, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "GEN3", .value = 0x0100, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "GEN4", .value = 0x0200, .dpll_type = >DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, >>+ { .name = "GND", .value = 0x2000, .dpll_type = 0 }, >>+ { .name = "VCC", .value = 0x4000, .dpll_type = 0 }, >> { } >> }; >> >>@@ -4175,12 +4179,41 @@ ptp_ocp_detach(struct ptp_ocp *bp) >> device_unregister(&bp->dev); >> } >> >>+static int ptp_ocp_dpll_get_attr(struct dpll_device *dpll, struct >dpll_attr *attr) >>+{ >>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >>+ int sync; >>+ >>+ sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; >>+ dpll_attr_lock_status_set(attr, sync ? DPLL_LOCK_STATUS_LOCKED : >DPLL_LOCK_STATUS_UNLOCKED); > >get,set,confuse. This attr thing sucks, sorry :/ Once again, I feel obligated to add some explanations :) getter is ops called by dpll subsystem, it requires data, so here value shall be set for the caller, right? Also have explained the reason why this attr struct and functions are done this way in the response to cover letter concerns. > > >>+ >>+ return 0; >>+} >>+ >>+static int ptp_ocp_dpll_pin_get_attr(struct dpll_device *dpll, struct >dpll_pin *pin, >>+ struct dpll_pin_attr *attr) >>+{ >>+ dpll_pin_attr_type_set(attr, DPLL_PIN_TYPE_EXT); > >This is exactly what I was talking about in the cover letter. This is >const, should be put into static struct and passed to >dpll_device_alloc(). Actually this type or some other parameters might change in the run-time, depends on the device, it is up to the driver how it will handle any getter, if driver knows it won't change it could also have some static member and copy the data with: dpll_pin_attr_copy(...); > > >>+ return 0; >>+} >>+ >>+static struct dpll_device_ops dpll_ops = { >>+ .get = ptp_ocp_dpll_get_attr, >>+}; >>+ >>+static struct dpll_pin_ops dpll_pin_ops = { >>+ .get = ptp_ocp_dpll_pin_get_attr, >>+}; >>+ >> static int >> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) >> { >>+ const u8 dpll_cookie[DPLL_COOKIE_LEN] = { "OCP" }; >>+ char pin_desc[PIN_DESC_LEN]; >> struct devlink *devlink; >>+ struct dpll_pin *pin; >> struct ptp_ocp *bp; >>- int err; >>+ int err, i; >> >> devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev- >>dev); >> if (!devlink) { >>@@ -4230,6 +4263,20 @@ 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, DPLL_TYPE_PPS, dpll_cookie, >pdev->bus->number, bp, &pdev->dev); >>+ if (!bp->dpll) { >>+ dev_err(&pdev->dev, "dpll_device_alloc failed\n"); >>+ goto out; >>+ } >>+ dpll_device_register(bp->dpll); > >You still have the 2 step init process. I believe it would be better to >just have dpll_device_create/destroy() to do it in one shot. For me either is ok, but due to pins alloc/register as explained below I would leave it as it is. > > >>+ >>+ for (i = 0; i < 4; i++) { >>+ snprintf(pin_desc, PIN_DESC_LEN, "sma%d", i + 1); >>+ pin = dpll_pin_alloc(pin_desc, PIN_DESC_LEN); >>+ dpll_pin_register(bp->dpll, pin, &dpll_pin_ops, bp); > >Same here, no point of having 2 step init. The alloc of a pin is not required if the pin already exist and would be just registered with another dpll. Once we decide to entirely drop shared pins idea this could be probably done, although other kernel code usually use this twostep approach? > > >>+ } >>+ >> return 0; > > >Btw, did you consider having dpll instance here as and auxdev? It would >be suitable I believe. It is quite simple to do it. See following patch >as an example: I haven't think about it, definetly gonna take a look to see if there any benefits in ice. Thanks, Arkadiusz > >commit bd02fd76d1909637c95e8ef13e7fd1e748af910d >Author: Jiri Pirko <jiri@nvidia.com> >Date: Mon Jul 25 10:29:17 2022 +0200 > > mlxsw: core_linecards: Introduce per line card auxiliary device > > > > >> >> out: >>@@ -4247,6 +4294,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); >>-- >>2.27.0 >>
Fri, Dec 02, 2022 at 12:27:32PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Wednesday, November 30, 2022 1:41 PM >> >>Tue, Nov 29, 2022 at 10:37:24PM CET, vfedorenko@novek.ru wrote: >>>From: Vadim Fedorenko <vadfed@fb.com> [...] >>>+static int ptp_ocp_dpll_get_attr(struct dpll_device *dpll, struct >>dpll_attr *attr) >>>+{ >>>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >>>+ int sync; >>>+ >>>+ sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; >>>+ dpll_attr_lock_status_set(attr, sync ? DPLL_LOCK_STATUS_LOCKED : >>DPLL_LOCK_STATUS_UNLOCKED); >> >>get,set,confuse. This attr thing sucks, sorry :/ > >Once again, I feel obligated to add some explanations :) > >getter is ops called by dpll subsystem, it requires data, so here value shall >be set for the caller, right? >Also have explained the reason why this attr struct and functions are done this >way in the response to cover letter concerns. Okay, I will react there. > >> >> >>>+ >>>+ return 0; >>>+} >>>+ >>>+static int ptp_ocp_dpll_pin_get_attr(struct dpll_device *dpll, struct >>dpll_pin *pin, >>>+ struct dpll_pin_attr *attr) >>>+{ >>>+ dpll_pin_attr_type_set(attr, DPLL_PIN_TYPE_EXT); >> >>This is exactly what I was talking about in the cover letter. This is >>const, should be put into static struct and passed to >>dpll_device_alloc(). > >Actually this type or some other parameters might change in the run-time, No. This should not change. If the pin is SyncE port, it's that for all lifetime of pin. It cannot turn to be a EXT/SMA connector all of the sudden. This should be definitelly fixed, it's a device topology. Can you explain the exact scenario when the change of personality of pin can happen? Perhaps I'm missing something. >depends on the device, it is up to the driver how it will handle any getter, >if driver knows it won't change it could also have some static member and copy >the data with: dpll_pin_attr_copy(...); > >> >> >>>+ return 0; >>>+} >>>+ >>>+static struct dpll_device_ops dpll_ops = { >>>+ .get = ptp_ocp_dpll_get_attr, >>>+}; >>>+ >>>+static struct dpll_pin_ops dpll_pin_ops = { >>>+ .get = ptp_ocp_dpll_pin_get_attr, >>>+}; >>>+ >>> static int >>> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>> { >>>+ const u8 dpll_cookie[DPLL_COOKIE_LEN] = { "OCP" }; >>>+ char pin_desc[PIN_DESC_LEN]; >>> struct devlink *devlink; >>>+ struct dpll_pin *pin; >>> struct ptp_ocp *bp; >>>- int err; >>>+ int err, i; >>> >>> devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev- >>>dev); >>> if (!devlink) { >>>@@ -4230,6 +4263,20 @@ 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, DPLL_TYPE_PPS, dpll_cookie, >>pdev->bus->number, bp, &pdev->dev); >>>+ if (!bp->dpll) { >>>+ dev_err(&pdev->dev, "dpll_device_alloc failed\n"); >>>+ goto out; >>>+ } >>>+ dpll_device_register(bp->dpll); >> >>You still have the 2 step init process. I believe it would be better to >>just have dpll_device_create/destroy() to do it in one shot. > >For me either is ok, but due to pins alloc/register as explained below I would >leave it as it is. Please don't, it has no value. Just adds unnecesary code. Have it nice and simple. > >> >> >>>+ >>>+ for (i = 0; i < 4; i++) { >>>+ snprintf(pin_desc, PIN_DESC_LEN, "sma%d", i + 1); >>>+ pin = dpll_pin_alloc(pin_desc, PIN_DESC_LEN); >>>+ dpll_pin_register(bp->dpll, pin, &dpll_pin_ops, bp); >> >>Same here, no point of having 2 step init. > >The alloc of a pin is not required if the pin already exist and would be just >registered with another dpll. Please don't. Have a pin created on a single DPLL. Why you make things compitated here? I don't follow. >Once we decide to entirely drop shared pins idea this could be probably done, >although other kernel code usually use this twostep approach? No, it does not. It's is used whatever fits on the individual usecase. > >> >> >>>+ } >>>+ >>> return 0; >> >> >>Btw, did you consider having dpll instance here as and auxdev? It would >>be suitable I believe. It is quite simple to do it. See following patch >>as an example: > >I haven't think about it, definetly gonna take a look to see if there any >benefits in ice. Please do. The proper separation and bus/device modelling is at least one of the benefits. The other one is that all dpll drivers would happily live in drivers/dpll/ side by side. > >Thanks, >Arkadiusz > >> >>commit bd02fd76d1909637c95e8ef13e7fd1e748af910d >>Author: Jiri Pirko <jiri@nvidia.com> >>Date: Mon Jul 25 10:29:17 2022 +0200 >> >> mlxsw: core_linecards: Introduce per line card auxiliary device >> >> >> >> >>> >>> out: >>>@@ -4247,6 +4294,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); >>>-- >>>2.27.0 >>>
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Friday, December 2, 2022 1:49 PM > >Fri, Dec 02, 2022 at 12:27:32PM CET, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Wednesday, November 30, 2022 1:41 PM >>> >>>Tue, Nov 29, 2022 at 10:37:24PM CET, vfedorenko@novek.ru wrote: >>>>From: Vadim Fedorenko <vadfed@fb.com> > >[...] > > >>>>+static int ptp_ocp_dpll_get_attr(struct dpll_device *dpll, struct >>>dpll_attr *attr) >>>>+{ >>>>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >>>>+ int sync; >>>>+ >>>>+ sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; >>>>+ dpll_attr_lock_status_set(attr, sync ? DPLL_LOCK_STATUS_LOCKED : >>>DPLL_LOCK_STATUS_UNLOCKED); >>> >>>get,set,confuse. This attr thing sucks, sorry :/ >> >>Once again, I feel obligated to add some explanations :) >> >>getter is ops called by dpll subsystem, it requires data, so here value >>shall be set for the caller, right? >>Also have explained the reason why this attr struct and functions are >>done this way in the response to cover letter concerns. > >Okay, I will react there. Thanks! > > >> >>> >>> >>>>+ >>>>+ return 0; >>>>+} >>>>+ >>>>+static int ptp_ocp_dpll_pin_get_attr(struct dpll_device *dpll, >>>>+struct >>>dpll_pin *pin, >>>>+ struct dpll_pin_attr *attr) { >>>>+ dpll_pin_attr_type_set(attr, DPLL_PIN_TYPE_EXT); >>> >>>This is exactly what I was talking about in the cover letter. This is >>>const, should be put into static struct and passed to >>>dpll_device_alloc(). >> >>Actually this type or some other parameters might change in the >>run-time, > >No. This should not change. >If the pin is SyncE port, it's that for all lifetime of pin. It cannot turn >to be a EXT/SMA connector all of the sudden. This should be definitelly >fixed, it's a device topology. > >Can you explain the exact scenario when the change of personality of pin >can happen? Perhaps I'm missing something. > Our device is not capable of doing this type of switch, but why to assume that some other HW would not? As I understand generic dpll subsystem must not be tied to any HW, and you proposal makes it exactly tied to our approaches. As Vadim requested to have possibility to change pin between source/output "states" this seems also possible that some HW might have multiple types possible. I don't get why "all of the sudden", DPLLA_PIN_TYPE_SUPPORTED can have multiple values, which means that the user can pick one of those with set command. Then if HW supports it could redirect signals/setup things accordingly. > > >>depends on the device, it is up to the driver how it will handle any >>getter, if driver knows it won't change it could also have some static >>member and copy the data with: dpll_pin_attr_copy(...); >> >>> >>> >>>>+ return 0; >>>>+} >>>>+ >>>>+static struct dpll_device_ops dpll_ops = { >>>>+ .get = ptp_ocp_dpll_get_attr, >>>>+}; >>>>+ >>>>+static struct dpll_pin_ops dpll_pin_ops = { >>>>+ .get = ptp_ocp_dpll_pin_get_attr, >>>>+}; >>>>+ >>>> static int >>>> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>>> { >>>>+ const u8 dpll_cookie[DPLL_COOKIE_LEN] = { "OCP" }; >>>>+ char pin_desc[PIN_DESC_LEN]; >>>> struct devlink *devlink; >>>>+ struct dpll_pin *pin; >>>> struct ptp_ocp *bp; >>>>- int err; >>>>+ int err, i; >>>> >>>> devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev- >>>>dev); >>>> if (!devlink) { >>>>@@ -4230,6 +4263,20 @@ 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, DPLL_TYPE_PPS, dpll_cookie, >>>pdev->bus->number, bp, &pdev->dev); >>>>+ if (!bp->dpll) { >>>>+ dev_err(&pdev->dev, "dpll_device_alloc failed\n"); >>>>+ goto out; >>>>+ } >>>>+ dpll_device_register(bp->dpll); >>> >>>You still have the 2 step init process. I believe it would be better >>>to just have dpll_device_create/destroy() to do it in one shot. >> >>For me either is ok, but due to pins alloc/register as explained below >>I would leave it as it is. > >Please don't, it has no value. Just adds unnecesary code. Have it nice and >simple. > Actually this comment relates to the other commit, could we keep comments in the threads they belong to please, this would be much easier to track. But yeah sure, if there is no strong opinion on that we could change it. > >> >>> >>> >>>>+ >>>>+ for (i = 0; i < 4; i++) { >>>>+ snprintf(pin_desc, PIN_DESC_LEN, "sma%d", i + 1); >>>>+ pin = dpll_pin_alloc(pin_desc, PIN_DESC_LEN); >>>>+ dpll_pin_register(bp->dpll, pin, &dpll_pin_ops, bp); >>> >>>Same here, no point of having 2 step init. >> >>The alloc of a pin is not required if the pin already exist and would >>be just registered with another dpll. > >Please don't. Have a pin created on a single DPLL. Why you make things >compitated here? I don't follow. Tried to explain on the cover-letter thread, let's discuss there please. > > >>Once we decide to entirely drop shared pins idea this could be probably >>done, although other kernel code usually use this twostep approach? > >No, it does not. It's is used whatever fits on the individual usecase. Similar to above, no strong opinion here from me, for shared pin it is certainly useful. > > >> >>> >>> >>>>+ } >>>>+ >>>> return 0; >>> >>> >>>Btw, did you consider having dpll instance here as and auxdev? It >>>would be suitable I believe. It is quite simple to do it. See >>>following patch as an example: >> >>I haven't think about it, definetly gonna take a look to see if there >>any benefits in ice. > >Please do. The proper separation and bus/device modelling is at least one >of the benefits. The other one is that all dpll drivers would happily live >in drivers/dpll/ side by side. > Well, makes sense, but still need to take a closer look on that. I could do that on ice-driver part, don't feel strong enough yet to introduce Changes here in ptp_ocp. Thank you, Arkadiusz > > >> >>Thanks, >>Arkadiusz >> >>> >>>commit bd02fd76d1909637c95e8ef13e7fd1e748af910d >>>Author: Jiri Pirko <jiri@nvidia.com> >>>Date: Mon Jul 25 10:29:17 2022 +0200 >>> >>> mlxsw: core_linecards: Introduce per line card auxiliary device >>> >>> >>> >>> >>>> >>>> out: >>>>@@ -4247,6 +4294,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); >>>>-- >>>>2.27.0 >>>>
Fri, Dec 02, 2022 at 03:39:17PM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Friday, December 2, 2022 1:49 PM >> >>Fri, Dec 02, 2022 at 12:27:32PM CET, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jiri Pirko <jiri@resnulli.us> >>>>Sent: Wednesday, November 30, 2022 1:41 PM >>>> >>>>Tue, Nov 29, 2022 at 10:37:24PM CET, vfedorenko@novek.ru wrote: >>>>>From: Vadim Fedorenko <vadfed@fb.com> >> >>[...] >> >> >>>>>+static int ptp_ocp_dpll_get_attr(struct dpll_device *dpll, struct >>>>dpll_attr *attr) >>>>>+{ >>>>>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >>>>>+ int sync; >>>>>+ >>>>>+ sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; >>>>>+ dpll_attr_lock_status_set(attr, sync ? DPLL_LOCK_STATUS_LOCKED : >>>>DPLL_LOCK_STATUS_UNLOCKED); >>>> >>>>get,set,confuse. This attr thing sucks, sorry :/ >>> >>>Once again, I feel obligated to add some explanations :) >>> >>>getter is ops called by dpll subsystem, it requires data, so here value >>>shall be set for the caller, right? >>>Also have explained the reason why this attr struct and functions are >>>done this way in the response to cover letter concerns. >> >>Okay, I will react there. > >Thanks! > >> >> >>> >>>> >>>> >>>>>+ >>>>>+ return 0; >>>>>+} >>>>>+ >>>>>+static int ptp_ocp_dpll_pin_get_attr(struct dpll_device *dpll, >>>>>+struct >>>>dpll_pin *pin, >>>>>+ struct dpll_pin_attr *attr) { >>>>>+ dpll_pin_attr_type_set(attr, DPLL_PIN_TYPE_EXT); >>>> >>>>This is exactly what I was talking about in the cover letter. This is >>>>const, should be put into static struct and passed to >>>>dpll_device_alloc(). >>> >>>Actually this type or some other parameters might change in the >>>run-time, >> >>No. This should not change. >>If the pin is SyncE port, it's that for all lifetime of pin. It cannot turn >>to be a EXT/SMA connector all of the sudden. This should be definitelly >>fixed, it's a device topology. >> >>Can you explain the exact scenario when the change of personality of pin >>can happen? Perhaps I'm missing something. >> > >Our device is not capable of doing this type of switch, but why to assume >that some other HW would not? As I understand generic dpll subsystem must not >be tied to any HW, and you proposal makes it exactly tied to our approaches. >As Vadim requested to have possibility to change pin between source/output >"states" this seems also possible that some HW might have multiple types >possible. How? How do you physically change from EXT connector to SyncE port? That does not make sense. Topology is given. Let's go back to Earth here. >I don't get why "all of the sudden", DPLLA_PIN_TYPE_SUPPORTED can have multiple >values, which means that the user can pick one of those with set command. >Then if HW supports it could redirect signals/setup things accordingly. We have to stritly distinguis between things that are given, wired-up, static and things that could be configured. > >> >> >>>depends on the device, it is up to the driver how it will handle any >>>getter, if driver knows it won't change it could also have some static >>>member and copy the data with: dpll_pin_attr_copy(...); >>> >>>> >>>> >>>>>+ return 0; >>>>>+} >>>>>+ >>>>>+static struct dpll_device_ops dpll_ops = { >>>>>+ .get = ptp_ocp_dpll_get_attr, >>>>>+}; >>>>>+ >>>>>+static struct dpll_pin_ops dpll_pin_ops = { >>>>>+ .get = ptp_ocp_dpll_pin_get_attr, >>>>>+}; >>>>>+ >>>>> static int >>>>> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>>>> { >>>>>+ const u8 dpll_cookie[DPLL_COOKIE_LEN] = { "OCP" }; >>>>>+ char pin_desc[PIN_DESC_LEN]; >>>>> struct devlink *devlink; >>>>>+ struct dpll_pin *pin; >>>>> struct ptp_ocp *bp; >>>>>- int err; >>>>>+ int err, i; >>>>> >>>>> devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev- >>>>>dev); >>>>> if (!devlink) { >>>>>@@ -4230,6 +4263,20 @@ 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, DPLL_TYPE_PPS, dpll_cookie, >>>>pdev->bus->number, bp, &pdev->dev); >>>>>+ if (!bp->dpll) { >>>>>+ dev_err(&pdev->dev, "dpll_device_alloc failed\n"); >>>>>+ goto out; >>>>>+ } >>>>>+ dpll_device_register(bp->dpll); >>>> >>>>You still have the 2 step init process. I believe it would be better >>>>to just have dpll_device_create/destroy() to do it in one shot. >>> >>>For me either is ok, but due to pins alloc/register as explained below >>>I would leave it as it is. >> >>Please don't, it has no value. Just adds unnecesary code. Have it nice and >>simple. >> > >Actually this comment relates to the other commit, could we keep comments >in the threads they belong to please, this would be much easier to track. >But yeah sure, if there is no strong opinion on that we could change it. Ok. > >> >>> >>>> >>>> >>>>>+ >>>>>+ for (i = 0; i < 4; i++) { >>>>>+ snprintf(pin_desc, PIN_DESC_LEN, "sma%d", i + 1); >>>>>+ pin = dpll_pin_alloc(pin_desc, PIN_DESC_LEN); >>>>>+ dpll_pin_register(bp->dpll, pin, &dpll_pin_ops, bp); >>>> >>>>Same here, no point of having 2 step init. >>> >>>The alloc of a pin is not required if the pin already exist and would >>>be just registered with another dpll. >> >>Please don't. Have a pin created on a single DPLL. Why you make things >>compitated here? I don't follow. > >Tried to explain on the cover-letter thread, let's discuss there please. Ok. > >> >> >>>Once we decide to entirely drop shared pins idea this could be probably >>>done, although other kernel code usually use this twostep approach? >> >>No, it does not. It's is used whatever fits on the individual usecase. > >Similar to above, no strong opinion here from me, for shared pin it is >certainly useful. > >> >> >>> >>>> >>>> >>>>>+ } >>>>>+ >>>>> return 0; >>>> >>>> >>>>Btw, did you consider having dpll instance here as and auxdev? It >>>>would be suitable I believe. It is quite simple to do it. See >>>>following patch as an example: >>> >>>I haven't think about it, definetly gonna take a look to see if there >>>any benefits in ice. >> >>Please do. The proper separation and bus/device modelling is at least one >>of the benefits. The other one is that all dpll drivers would happily live >>in drivers/dpll/ side by side. >> > >Well, makes sense, but still need to take a closer look on that. >I could do that on ice-driver part, don't feel strong enough yet to introduce Sure Ice should be ready. >Changes here in ptp_ocp. I think that Vadim said he is going to look at that during the call. My commit introducing this to mlxsw is a nice and simple example how this could be done in ptp_ocp. > >Thank you, >Arkadiusz > >> >> >>> >>>Thanks, >>>Arkadiusz >>> >>>> >>>>commit bd02fd76d1909637c95e8ef13e7fd1e748af910d >>>>Author: Jiri Pirko <jiri@nvidia.com> >>>>Date: Mon Jul 25 10:29:17 2022 +0200 >>>> >>>> mlxsw: core_linecards: Introduce per line card auxiliary device >>>> >>>> >>>> >>>> >>>>> >>>>> out: >>>>>@@ -4247,6 +4294,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); >>>>>-- >>>>>2.27.0 >>>>>
On Fri, 2 Dec 2022 14:39:17 +0000 Kubalewski, Arkadiusz wrote: > >>>Btw, did you consider having dpll instance here as and auxdev? It > >>>would be suitable I believe. It is quite simple to do it. See > >>>following patch as an example: > >> > >>I haven't think about it, definetly gonna take a look to see if there > >>any benefits in ice. > > > >Please do. The proper separation and bus/device modelling is at least one > >of the benefits. The other one is that all dpll drivers would happily live > >in drivers/dpll/ side by side. > > Well, makes sense, but still need to take a closer look on that. > I could do that on ice-driver part, don't feel strong enough yet to introduce > Changes here in ptp_ocp. FWIW auxdev makes absolutely no sense to me for DPLL :/ So Jiri, please say why.
Wed, Dec 07, 2022 at 03:33:13AM CET, kuba@kernel.org wrote: >On Fri, 2 Dec 2022 14:39:17 +0000 Kubalewski, Arkadiusz wrote: >> >>>Btw, did you consider having dpll instance here as and auxdev? It >> >>>would be suitable I believe. It is quite simple to do it. See >> >>>following patch as an example: >> >> >> >>I haven't think about it, definetly gonna take a look to see if there >> >>any benefits in ice. >> > >> >Please do. The proper separation and bus/device modelling is at least one >> >of the benefits. The other one is that all dpll drivers would happily live >> >in drivers/dpll/ side by side. >> >> Well, makes sense, but still need to take a closer look on that. >> I could do that on ice-driver part, don't feel strong enough yet to introduce >> Changes here in ptp_ocp. > >FWIW auxdev makes absolutely no sense to me for DPLL :/ >So Jiri, please say why. Why not? It's a subdev of a device. In mlx5, we have separate auxdevs for eth, rdma, vnet, representors. DPLL is also a separate entity which could be instantiated independently (as it is not really dependent on eth/rdma/etc)). Auxdev looks like an awesome fit. Why do you think it is not? Also, what's good about auxdev is that you can maintain them quite independetly. So there is going to be driver/dpll/ directory which would contain all dpll drivers.
>From: Jiri Pirko <jiri@resnulli.us> >Sent: Friday, December 2, 2022 5:21 PM > >Fri, Dec 02, 2022 at 03:39:17PM CET, arkadiusz.kubalewski@intel.com wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Sent: Friday, December 2, 2022 1:49 PM >>> >>>Fri, Dec 02, 2022 at 12:27:32PM CET, arkadiusz.kubalewski@intel.com >wrote: >>>>>From: Jiri Pirko <jiri@resnulli.us> >>>>>Sent: Wednesday, November 30, 2022 1:41 PM >>>>> >>>>>Tue, Nov 29, 2022 at 10:37:24PM CET, vfedorenko@novek.ru wrote: >>>>>>From: Vadim Fedorenko <vadfed@fb.com> >>> >>>[...] >>> >>> >>>>>>+static int ptp_ocp_dpll_get_attr(struct dpll_device *dpll, struct >>>>>dpll_attr *attr) >>>>>>+{ >>>>>>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >>>>>>+ int sync; >>>>>>+ >>>>>>+ sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; >>>>>>+ dpll_attr_lock_status_set(attr, sync ? DPLL_LOCK_STATUS_LOCKED >: >>>>>DPLL_LOCK_STATUS_UNLOCKED); >>>>> >>>>>get,set,confuse. This attr thing sucks, sorry :/ >>>> >>>>Once again, I feel obligated to add some explanations :) >>>> >>>>getter is ops called by dpll subsystem, it requires data, so here value >>>>shall be set for the caller, right? >>>>Also have explained the reason why this attr struct and functions are >>>>done this way in the response to cover letter concerns. >>> >>>Okay, I will react there. >> >>Thanks! >> >>> >>> >>>> >>>>> >>>>> >>>>>>+ >>>>>>+ return 0; >>>>>>+} >>>>>>+ >>>>>>+static int ptp_ocp_dpll_pin_get_attr(struct dpll_device *dpll, >>>>>>+struct >>>>>dpll_pin *pin, >>>>>>+ struct dpll_pin_attr *attr) { >>>>>>+ dpll_pin_attr_type_set(attr, DPLL_PIN_TYPE_EXT); >>>>> >>>>>This is exactly what I was talking about in the cover letter. This is >>>>>const, should be put into static struct and passed to >>>>>dpll_device_alloc(). >>>> >>>>Actually this type or some other parameters might change in the >>>>run-time, >>> >>>No. This should not change. >>>If the pin is SyncE port, it's that for all lifetime of pin. It cannot >turn >>>to be a EXT/SMA connector all of the sudden. This should be definitelly >>>fixed, it's a device topology. >>> >>>Can you explain the exact scenario when the change of personality of pin >>>can happen? Perhaps I'm missing something. >>> >> >>Our device is not capable of doing this type of switch, but why to assume >>that some other HW would not? As I understand generic dpll subsystem must >not >>be tied to any HW, and you proposal makes it exactly tied to our >approaches. >>As Vadim requested to have possibility to change pin between source/output >>"states" this seems also possible that some HW might have multiple types >>possible. > >How? How do you physically change from EXT connector to SyncE port? That >does not make sense. Topology is given. Let's go back to Earth here. > I suppose by using some kind of hardware fuse/signal selector controlled by firmware/driver. Don't think it is out of space, just depends on hardware. > >>I don't get why "all of the sudden", DPLLA_PIN_TYPE_SUPPORTED can have >multiple >>values, which means that the user can pick one of those with set command. >>Then if HW supports it could redirect signals/setup things accordingly. > >We have to stritly distinguis between things that are given, wired-up, >static and things that could be configured. > This is supposed to be generic interface, right? What you insist on, is to hardcode most of it in software, which means that hardware designs would have to follow possibilities given by the software. > >> >>> >>> >>>>depends on the device, it is up to the driver how it will handle any >>>>getter, if driver knows it won't change it could also have some static >>>>member and copy the data with: dpll_pin_attr_copy(...); >>>> >>>>> >>>>> >>>>>>+ return 0; >>>>>>+} >>>>>>+ >>>>>>+static struct dpll_device_ops dpll_ops = { >>>>>>+ .get = ptp_ocp_dpll_get_attr, >>>>>>+}; >>>>>>+ >>>>>>+static struct dpll_pin_ops dpll_pin_ops = { >>>>>>+ .get = ptp_ocp_dpll_pin_get_attr, >>>>>>+}; >>>>>>+ >>>>>> static int >>>>>> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>>>>> { >>>>>>+ const u8 dpll_cookie[DPLL_COOKIE_LEN] = { "OCP" }; >>>>>>+ char pin_desc[PIN_DESC_LEN]; >>>>>> struct devlink *devlink; >>>>>>+ struct dpll_pin *pin; >>>>>> struct ptp_ocp *bp; >>>>>>- int err; >>>>>>+ int err, i; >>>>>> >>>>>> devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), >&pdev- >>>>>>dev); >>>>>> if (!devlink) { >>>>>>@@ -4230,6 +4263,20 @@ 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, DPLL_TYPE_PPS, >dpll_cookie, >>>>>pdev->bus->number, bp, &pdev->dev); >>>>>>+ if (!bp->dpll) { >>>>>>+ dev_err(&pdev->dev, "dpll_device_alloc failed\n"); >>>>>>+ goto out; >>>>>>+ } >>>>>>+ dpll_device_register(bp->dpll); >>>>> >>>>>You still have the 2 step init process. I believe it would be better >>>>>to just have dpll_device_create/destroy() to do it in one shot. >>>> >>>>For me either is ok, but due to pins alloc/register as explained below >>>>I would leave it as it is. >>> >>>Please don't, it has no value. Just adds unnecesary code. Have it nice >and >>>simple. >>> >> >>Actually this comment relates to the other commit, could we keep comments >>in the threads they belong to please, this would be much easier to track. >>But yeah sure, if there is no strong opinion on that we could change it. > >Ok. > > >> >>> >>>> >>>>> >>>>> >>>>>>+ >>>>>>+ for (i = 0; i < 4; i++) { >>>>>>+ snprintf(pin_desc, PIN_DESC_LEN, "sma%d", i + 1); >>>>>>+ pin = dpll_pin_alloc(pin_desc, PIN_DESC_LEN); >>>>>>+ dpll_pin_register(bp->dpll, pin, &dpll_pin_ops, bp); >>>>> >>>>>Same here, no point of having 2 step init. >>>> >>>>The alloc of a pin is not required if the pin already exist and would >>>>be just registered with another dpll. >>> >>>Please don't. Have a pin created on a single DPLL. Why you make things >>>compitated here? I don't follow. >> >>Tried to explain on the cover-letter thread, let's discuss there please. > >Ok. > > >> >>> >>> >>>>Once we decide to entirely drop shared pins idea this could be probably >>>>done, although other kernel code usually use this twostep approach? >>> >>>No, it does not. It's is used whatever fits on the individual usecase. >> >>Similar to above, no strong opinion here from me, for shared pin it is >>certainly useful. >> >>> >>> >>>> >>>>> >>>>> >>>>>>+ } >>>>>>+ >>>>>> return 0; >>>>> >>>>> >>>>>Btw, did you consider having dpll instance here as and auxdev? It >>>>>would be suitable I believe. It is quite simple to do it. See >>>>>following patch as an example: >>>> >>>>I haven't think about it, definetly gonna take a look to see if there >>>>any benefits in ice. >>> >>>Please do. The proper separation and bus/device modelling is at least one >>>of the benefits. The other one is that all dpll drivers would happily >live >>>in drivers/dpll/ side by side. >>> >> >>Well, makes sense, but still need to take a closer look on that. >>I could do that on ice-driver part, don't feel strong enough yet to >introduce > >Sure Ice should be ready. > > >>Changes here in ptp_ocp. > >I think that Vadim said he is going to look at that during the call. My >commit introducing this to mlxsw is a nice and simple example how this >could be done in ptp_ocp. > Yes, though first need to find a bit of time for it :S Thank you, Arkadiusz > >> >>Thank you, >>Arkadiusz >> >>> >>> >>>> >>>>Thanks, >>>>Arkadiusz >>>> >>>>> >>>>>commit bd02fd76d1909637c95e8ef13e7fd1e748af910d >>>>>Author: Jiri Pirko <jiri@nvidia.com> >>>>>Date: Mon Jul 25 10:29:17 2022 +0200 >>>>> >>>>> mlxsw: core_linecards: Introduce per line card auxiliary device >>>>> >>>>> >>>>> >>>>> >>>>>> >>>>>> out: >>>>>>@@ -4247,6 +4294,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); >>>>>>-- >>>>>>2.27.0 >>>>>>
Thu, Dec 08, 2022 at 01:35:02AM CET, arkadiusz.kubalewski@intel.com wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Sent: Friday, December 2, 2022 5:21 PM >> >>Fri, Dec 02, 2022 at 03:39:17PM CET, arkadiusz.kubalewski@intel.com wrote: >>>>From: Jiri Pirko <jiri@resnulli.us> >>>>Sent: Friday, December 2, 2022 1:49 PM >>>> >>>>Fri, Dec 02, 2022 at 12:27:32PM CET, arkadiusz.kubalewski@intel.com >>wrote: >>>>>>From: Jiri Pirko <jiri@resnulli.us> >>>>>>Sent: Wednesday, November 30, 2022 1:41 PM >>>>>> >>>>>>Tue, Nov 29, 2022 at 10:37:24PM CET, vfedorenko@novek.ru wrote: >>>>>>>From: Vadim Fedorenko <vadfed@fb.com> >>>> >>>>[...] >>>> >>>> >>>>>>>+static int ptp_ocp_dpll_get_attr(struct dpll_device *dpll, struct >>>>>>dpll_attr *attr) >>>>>>>+{ >>>>>>>+ struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); >>>>>>>+ int sync; >>>>>>>+ >>>>>>>+ sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; >>>>>>>+ dpll_attr_lock_status_set(attr, sync ? DPLL_LOCK_STATUS_LOCKED >>: >>>>>>DPLL_LOCK_STATUS_UNLOCKED); >>>>>> >>>>>>get,set,confuse. This attr thing sucks, sorry :/ >>>>> >>>>>Once again, I feel obligated to add some explanations :) >>>>> >>>>>getter is ops called by dpll subsystem, it requires data, so here value >>>>>shall be set for the caller, right? >>>>>Also have explained the reason why this attr struct and functions are >>>>>done this way in the response to cover letter concerns. >>>> >>>>Okay, I will react there. >>> >>>Thanks! >>> >>>> >>>> >>>>> >>>>>> >>>>>> >>>>>>>+ >>>>>>>+ return 0; >>>>>>>+} >>>>>>>+ >>>>>>>+static int ptp_ocp_dpll_pin_get_attr(struct dpll_device *dpll, >>>>>>>+struct >>>>>>dpll_pin *pin, >>>>>>>+ struct dpll_pin_attr *attr) { >>>>>>>+ dpll_pin_attr_type_set(attr, DPLL_PIN_TYPE_EXT); >>>>>> >>>>>>This is exactly what I was talking about in the cover letter. This is >>>>>>const, should be put into static struct and passed to >>>>>>dpll_device_alloc(). >>>>> >>>>>Actually this type or some other parameters might change in the >>>>>run-time, >>>> >>>>No. This should not change. >>>>If the pin is SyncE port, it's that for all lifetime of pin. It cannot >>turn >>>>to be a EXT/SMA connector all of the sudden. This should be definitelly >>>>fixed, it's a device topology. >>>> >>>>Can you explain the exact scenario when the change of personality of pin >>>>can happen? Perhaps I'm missing something. >>>> >>> >>>Our device is not capable of doing this type of switch, but why to assume >>>that some other HW would not? As I understand generic dpll subsystem must >>not >>>be tied to any HW, and you proposal makes it exactly tied to our >>approaches. >>>As Vadim requested to have possibility to change pin between source/output >>>"states" this seems also possible that some HW might have multiple types >>>possible. >> >>How? How do you physically change from EXT connector to SyncE port? That >>does not make sense. Topology is given. Let's go back to Earth here. >> > >I suppose by using some kind of hardware fuse/signal selector controlled by >firmware/driver. Don't think it is out of space, just depends on hardware. Can you describe this in more details please? I still don't follow how it makes sense to allow user to for example change EXT connector to SyncE port. If the pins are muxed, we already have model for that. But the same pin physical type change? How? > >> >>>I don't get why "all of the sudden", DPLLA_PIN_TYPE_SUPPORTED can have >>multiple >>>values, which means that the user can pick one of those with set command. >>>Then if HW supports it could redirect signals/setup things accordingly. >> >>We have to stritly distinguis between things that are given, wired-up, >>static and things that could be configured. >> > >This is supposed to be generic interface, right? >What you insist on, is to hardcode most of it in software, which means that >hardware designs would have to follow possibilities given by the software. Sure it is generic. I don't want to hardcode anything. Just the driver exposes whatever is in HW. If something can change in HW using configuration, driver/UAPI can expose it. However, what's the point of exposing something in UAPI which is static in HW? It only introduces confusion for the UAPI consumer. > >> >>> >>>> >>>> >>>>>depends on the device, it is up to the driver how it will handle any >>>>>getter, if driver knows it won't change it could also have some static >>>>>member and copy the data with: dpll_pin_attr_copy(...); >>>>> >>>>>> >>>>>> >>>>>>>+ return 0; >>>>>>>+} >>>>>>>+ >>>>>>>+static struct dpll_device_ops dpll_ops = { >>>>>>>+ .get = ptp_ocp_dpll_get_attr, >>>>>>>+}; >>>>>>>+ >>>>>>>+static struct dpll_pin_ops dpll_pin_ops = { >>>>>>>+ .get = ptp_ocp_dpll_pin_get_attr, >>>>>>>+}; >>>>>>>+ >>>>>>> static int >>>>>>> ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) >>>>>>> { >>>>>>>+ const u8 dpll_cookie[DPLL_COOKIE_LEN] = { "OCP" }; >>>>>>>+ char pin_desc[PIN_DESC_LEN]; >>>>>>> struct devlink *devlink; >>>>>>>+ struct dpll_pin *pin; >>>>>>> struct ptp_ocp *bp; >>>>>>>- int err; >>>>>>>+ int err, i; >>>>>>> >>>>>>> devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), >>&pdev- >>>>>>>dev); >>>>>>> if (!devlink) { >>>>>>>@@ -4230,6 +4263,20 @@ 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, DPLL_TYPE_PPS, >>dpll_cookie, >>>>>>pdev->bus->number, bp, &pdev->dev); >>>>>>>+ if (!bp->dpll) { >>>>>>>+ dev_err(&pdev->dev, "dpll_device_alloc failed\n"); >>>>>>>+ goto out; >>>>>>>+ } >>>>>>>+ dpll_device_register(bp->dpll); >>>>>> >>>>>>You still have the 2 step init process. I believe it would be better >>>>>>to just have dpll_device_create/destroy() to do it in one shot. >>>>> >>>>>For me either is ok, but due to pins alloc/register as explained below >>>>>I would leave it as it is. >>>> >>>>Please don't, it has no value. Just adds unnecesary code. Have it nice >>and >>>>simple. >>>> >>> >>>Actually this comment relates to the other commit, could we keep comments >>>in the threads they belong to please, this would be much easier to track. >>>But yeah sure, if there is no strong opinion on that we could change it. >> >>Ok. >> >> >>> >>>> >>>>> >>>>>> >>>>>> >>>>>>>+ >>>>>>>+ for (i = 0; i < 4; i++) { >>>>>>>+ snprintf(pin_desc, PIN_DESC_LEN, "sma%d", i + 1); >>>>>>>+ pin = dpll_pin_alloc(pin_desc, PIN_DESC_LEN); >>>>>>>+ dpll_pin_register(bp->dpll, pin, &dpll_pin_ops, bp); >>>>>> >>>>>>Same here, no point of having 2 step init. >>>>> >>>>>The alloc of a pin is not required if the pin already exist and would >>>>>be just registered with another dpll. >>>> >>>>Please don't. Have a pin created on a single DPLL. Why you make things >>>>compitated here? I don't follow. >>> >>>Tried to explain on the cover-letter thread, let's discuss there please. >> >>Ok. >> >> >>> >>>> >>>> >>>>>Once we decide to entirely drop shared pins idea this could be probably >>>>>done, although other kernel code usually use this twostep approach? >>>> >>>>No, it does not. It's is used whatever fits on the individual usecase. >>> >>>Similar to above, no strong opinion here from me, for shared pin it is >>>certainly useful. >>> >>>> >>>> >>>>> >>>>>> >>>>>> >>>>>>>+ } >>>>>>>+ >>>>>>> return 0; >>>>>> >>>>>> >>>>>>Btw, did you consider having dpll instance here as and auxdev? It >>>>>>would be suitable I believe. It is quite simple to do it. See >>>>>>following patch as an example: >>>>> >>>>>I haven't think about it, definetly gonna take a look to see if there >>>>>any benefits in ice. >>>> >>>>Please do. The proper separation and bus/device modelling is at least one >>>>of the benefits. The other one is that all dpll drivers would happily >>live >>>>in drivers/dpll/ side by side. >>>> >>> >>>Well, makes sense, but still need to take a closer look on that. >>>I could do that on ice-driver part, don't feel strong enough yet to >>introduce >> >>Sure Ice should be ready. >> >> >>>Changes here in ptp_ocp. >> >>I think that Vadim said he is going to look at that during the call. My >>commit introducing this to mlxsw is a nice and simple example how this >>could be done in ptp_ocp. >> > >Yes, though first need to find a bit of time for it :S > >Thank you, >Arkadiusz > >> >>> >>>Thank you, >>>Arkadiusz >>> >>>> >>>> >>>>> >>>>>Thanks, >>>>>Arkadiusz >>>>> >>>>>> >>>>>>commit bd02fd76d1909637c95e8ef13e7fd1e748af910d >>>>>>Author: Jiri Pirko <jiri@nvidia.com> >>>>>>Date: Mon Jul 25 10:29:17 2022 +0200 >>>>>> >>>>>> mlxsw: core_linecards: Introduce per line card auxiliary device >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> out: >>>>>>>@@ -4247,6 +4294,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); >>>>>>>-- >>>>>>>2.27.0 >>>>>>>
Wed, Dec 07, 2022 at 06:05:24PM CET, kuba@kernel.org wrote: >On Wed, 7 Dec 2022 14:19:22 +0100 Jiri Pirko wrote: >>> FWIW auxdev makes absolutely no sense to me for DPLL :/ >>> So Jiri, please say why. >> >> Why not? It's a subdev of a device. In mlx5, we have separate auxdevs >> for eth, rdma, vnet, representors. DPLL is also a separate entity which >> could be instantiated independently (as it is not really dependent on >> eth/rdma/etc)). Auxdev looks like an awesome fit. Why do you think it is >> not? >> >> Also, what's good about auxdev is that you can maintain them quite >> independetly. So there is going to be driver/dpll/ directory which would >> contain all dpll drivers. > >To what practical benefit? Where do we draw the line? Do you want >PTP clocks to also be auxdevs? DPLL lives in netdev, we don't have >to complicate things. auxdev is a Conway's law solution. Auxdev infra is quite simple to implement, I'm not sure what do you mean by complicating thing here. > >mlx5 already looks like sausage meat, it's already minced so you can >fit it there quite easily, but don't force it on non-enterprise devices. Not forcing, just suggesting. It's a low-hanging fruit, why not reach it? > >There is non 1:1 relationship with a bus device and subsystem in Linux, >LMK when you convinced Greg otherwise. Sure there is not. But maybe that is due to the simple fact that auxdev was introduces, what, 2 years back? My point is, we are introducing new subsystem, wouldn't it be nice to start it clean?
On Thu, 8 Dec 2022 12:22:09 +0100 Jiri Pirko wrote: > >To what practical benefit? Where do we draw the line? Do you want > >PTP clocks to also be auxdevs? DPLL lives in netdev, we don't have > >to complicate things. auxdev is a Conway's law solution. > > Auxdev infra is quite simple to implement, I'm not sure what do you mean > by complicating thing here. You didn't answer my question - what's the benefit? We're not faced with A or B choice. We have a A or nothing choice. Doing nothing is easy. > >mlx5 already looks like sausage meat, it's already minced so you can > >fit it there quite easily, but don't force it on non-enterprise devices. > > Not forcing, just suggesting. It's a low-hanging fruit, why not reach > it? What is the fruit? > >There is non 1:1 relationship with a bus device and subsystem in Linux, > >LMK when you convinced Greg otherwise. > > Sure there is not. But maybe that is due to the simple fact that auxdev > was introduces, what, 2 years back? My point is, we are introducing new > subsystem, wouldn't it be nice to start it clean? Still not getting what you think is clean.. Making all driver-facing objects in the kernel be a fake bus-device?!
Fri, Dec 09, 2022 at 01:36:34AM CET, kuba@kernel.org wrote: >On Thu, 8 Dec 2022 12:22:09 +0100 Jiri Pirko wrote: >> >To what practical benefit? Where do we draw the line? Do you want >> >PTP clocks to also be auxdevs? DPLL lives in netdev, we don't have >> >to complicate things. auxdev is a Conway's law solution. >> >> Auxdev infra is quite simple to implement, I'm not sure what do you mean >> by complicating thing here. > >You didn't answer my question - what's the benefit? >We're not faced with A or B choice. We have a A or nothing choice. >Doing nothing is easy. > >> >mlx5 already looks like sausage meat, it's already minced so you can >> >fit it there quite easily, but don't force it on non-enterprise devices. >> >> Not forcing, just suggesting. It's a low-hanging fruit, why not reach >> it? > >What is the fruit? > >> >There is non 1:1 relationship with a bus device and subsystem in Linux, >> >LMK when you convinced Greg otherwise. >> >> Sure there is not. But maybe that is due to the simple fact that auxdev >> was introduces, what, 2 years back? My point is, we are introducing new >> subsystem, wouldn't it be nice to start it clean? > >Still not getting what you think is clean.. Making all driver-facing >objects in the kernel be a fake bus-device?! Nevermind.
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index fe4971b65c64..8c4cfabc1bfa 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 154d58cbd9ce..605853ac4a12 100644 --- a/drivers/ptp/ptp_ocp.c +++ b/drivers/ptp/ptp_ocp.c @@ -23,6 +23,8 @@ #include <linux/mtd/mtd.h> #include <linux/nvmem-consumer.h> #include <linux/crc16.h> +#include <linux/dpll.h> +#include <uapi/linux/dpll.h> #define PCI_VENDOR_ID_FACEBOOK 0x1d9b #define PCI_DEVICE_ID_FACEBOOK_TIMECARD 0x0400 @@ -353,6 +355,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) @@ -835,18 +838,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 }, { } }; @@ -855,37 +859,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_PIN_SIGNAL_TYPE_10_MHZ }, + { .name = "PPS1", .value = 0x0001, .dpll_type = DPLL_PIN_SIGNAL_TYPE_1_PPS }, + { .name = "PPS2", .value = 0x0002, .dpll_type = DPLL_PIN_SIGNAL_TYPE_1_PPS }, + { .name = "TS1", .value = 0x0004, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "TS2", .value = 0x0008, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "TS3", .value = 0x0040, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "TS4", .value = 0x0080, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "FREQ1", .value = 0x0100, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "FREQ2", .value = 0x0200, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "FREQ3", .value = 0x0400, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "FREQ4", .value = 0x0800, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "None", .value = SMA_DISABLE, .dpll_type = 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 = "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_PIN_SIGNAL_TYPE_10_MHZ }, + { .name = "PHC", .value = 0x0001, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "MAC", .value = 0x0002, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "GNSS1", .value = 0x0004, .dpll_type = DPLL_PIN_SIGNAL_TYPE_1_PPS }, + { .name = "GNSS2", .value = 0x0008, .dpll_type = DPLL_PIN_SIGNAL_TYPE_1_PPS }, + { .name = "IRIG", .value = 0x0010, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "DCF", .value = 0x0020, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "GEN1", .value = 0x0040, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "GEN2", .value = 0x0080, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "GEN3", .value = 0x0100, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "GEN4", .value = 0x0200, .dpll_type = DPLL_PIN_SIGNAL_TYPE_CUSTOM_FREQ }, + { .name = "GND", .value = 0x2000, .dpll_type = 0 }, + { .name = "VCC", .value = 0x4000, .dpll_type = 0 }, { } }; @@ -4175,12 +4179,41 @@ ptp_ocp_detach(struct ptp_ocp *bp) device_unregister(&bp->dev); } +static int ptp_ocp_dpll_get_attr(struct dpll_device *dpll, struct dpll_attr *attr) +{ + struct ptp_ocp *bp = (struct ptp_ocp *)dpll_priv(dpll); + int sync; + + sync = ioread32(&bp->reg->status) & OCP_STATUS_IN_SYNC; + dpll_attr_lock_status_set(attr, sync ? DPLL_LOCK_STATUS_LOCKED : DPLL_LOCK_STATUS_UNLOCKED); + + return 0; +} + +static int ptp_ocp_dpll_pin_get_attr(struct dpll_device *dpll, struct dpll_pin *pin, + struct dpll_pin_attr *attr) +{ + dpll_pin_attr_type_set(attr, DPLL_PIN_TYPE_EXT); + return 0; +} + +static struct dpll_device_ops dpll_ops = { + .get = ptp_ocp_dpll_get_attr, +}; + +static struct dpll_pin_ops dpll_pin_ops = { + .get = ptp_ocp_dpll_pin_get_attr, +}; + static int ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id) { + const u8 dpll_cookie[DPLL_COOKIE_LEN] = { "OCP" }; + char pin_desc[PIN_DESC_LEN]; struct devlink *devlink; + struct dpll_pin *pin; struct ptp_ocp *bp; - int err; + int err, i; devlink = devlink_alloc(&ptp_ocp_devlink_ops, sizeof(*bp), &pdev->dev); if (!devlink) { @@ -4230,6 +4263,20 @@ 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, DPLL_TYPE_PPS, dpll_cookie, pdev->bus->number, bp, &pdev->dev); + if (!bp->dpll) { + dev_err(&pdev->dev, "dpll_device_alloc failed\n"); + goto out; + } + dpll_device_register(bp->dpll); + + for (i = 0; i < 4; i++) { + snprintf(pin_desc, PIN_DESC_LEN, "sma%d", i + 1); + pin = dpll_pin_alloc(pin_desc, PIN_DESC_LEN); + dpll_pin_register(bp->dpll, pin, &dpll_pin_ops, bp); + } + return 0; out: @@ -4247,6 +4294,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);