Re: [PATCH net-next] ptp: add debugfs support for IDT family of timing devices
On Fri, Jul 10, 2020 at 11:41:25AM -0400, min.li...@renesas.com wrote:
> From: Min Li
>
> This patch is to add debugfs support for ptp_clockmatrix and ptp_idt82p33.
> It will create a debugfs directory called idtptp{x} and x is the ptp index.
> Three inerfaces are present, which are cmd, help and regs. help is read
> only and will display a brief help message. regs is read only amd will show
> all register values.
You can get register dumps for free in debugfs if you use regmap. And
i2c devices are well supported by regmap.
Andrew
Re: [PATCH net-next] ptp: add debugfs support for IDT family of timing devices
On Sat, Jul 11, 2020 at 11:38:39AM -0700, Richard Cochran wrote: > On Sat, Jul 11, 2020 at 06:38:06PM +0200, Andrew Lunn wrote: > > But configuration does not belong in debugfs. It would be good to > > explain what is being configured by these parameters, then we can > > maybe make a suggestion about the correct API to use. > > Can we at least enumerate the possibilities? > > - driver specific char device > - private ioctls > - debugfs Hi Richard Since nobody has explained what is actually being configured here, the list is long, and is very likely to contain all sorts of wrong ways of doing it: A new generic parameter added to the PTP API which other PTP clock providers could use. A device tree property. A device tree clock, regulator, ... An ACPI property? A sysfs file. A module parameter A new POSIX clock? An LED class device? A netlink attribute? Andrew
Re: [PATCH net-next] ptp: add debugfs support for IDT family of timing devices
On Sat, Jul 11, 2020 at 06:38:06PM +0200, Andrew Lunn wrote: > But configuration does not belong in debugfs. It would be good to > explain what is being configured by these parameters, then we can > maybe make a suggestion about the correct API to use. Can we at least enumerate the possibilities? - driver specific char device - private ioctls - debugfs What else? My understanding is that debugfs is the preferred place for this kind of thing. Of course nobody expects any kind of stable ABI for this, and so that is a non-issue IHMO. Thanks, Richard
Re: [PATCH net-next] ptp: add debugfs support for IDT family of timing devices
On Sat, Jul 11, 2020 at 06:46:01AM -0700, Richard Cochran wrote:
> On Fri, Jul 10, 2020 at 01:58:44PM -0700, Jakub Kicinski wrote:
> > On Fri, 10 Jul 2020 11:41:25 -0400 min.li...@renesas.com wrote:
> > > From: Min Li
> > >
> > > This patch is to add debugfs support for ptp_clockmatrix and ptp_idt82p33.
> > > It will create a debugfs directory called idtptp{x} and x is the ptp
> > > index.
> > > Three inerfaces are present, which are cmd, help and regs. help is read
> > > only and will display a brief help message. regs is read only amd will
> > > show
> > > all register values. cmd is write only and will accept certain commands.
> > > Currently, the accepted commands are combomode to set comobo mode and
> > > write
> > > to write up to 4 registers.
> > >
> > > Signed-off-by: Min Li
> >
> > No private configuration interfaces in debugfs, please.
>
> I suggested to Min to use debugfs for device-specific configuration
> that would be out of place in the generic PTP Hardware Clock
> interface.
>
> > If what you're exposing is a useful feature it deserves a proper
> > uAPI interface.
>
> Can you expand on what you mean by "proper uAPI interface" please?
Hi Richard
Well, one obvious issues is that debugfs it totally optional, and
often not built. You would not want the correct operation of a device
to depend something which is optional. debugfs is also unstable. There
are no ABI rules. So user space cannot rely on the API being the same
from version to version. Again, not something you want for the correct
operation of a device.
Allowing registers to be read, is a typical debug operation. So that
part seems reasonable. A kernel developer/debugger has the skills to
deal with unstable APIs, and rebuilding the kernel to actually have
debugfs in the image.
But configuration does not belong in debugfs. It would be good to
explain what is being configured by these parameters, then we can
maybe make a suggestion about the correct API to use.
Andrew
Re: [PATCH net-next] ptp: add debugfs support for IDT family of timing devices
On Fri, Jul 10, 2020 at 01:58:44PM -0700, Jakub Kicinski wrote:
> On Fri, 10 Jul 2020 11:41:25 -0400 min.li...@renesas.com wrote:
> > From: Min Li
> >
> > This patch is to add debugfs support for ptp_clockmatrix and ptp_idt82p33.
> > It will create a debugfs directory called idtptp{x} and x is the ptp index.
> > Three inerfaces are present, which are cmd, help and regs. help is read
> > only and will display a brief help message. regs is read only amd will show
> > all register values. cmd is write only and will accept certain commands.
> > Currently, the accepted commands are combomode to set comobo mode and write
> > to write up to 4 registers.
> >
> > Signed-off-by: Min Li
>
> No private configuration interfaces in debugfs, please.
I suggested to Min to use debugfs for device-specific configuration
that would be out of place in the generic PTP Hardware Clock
interface.
> If what you're exposing is a useful feature it deserves a proper
> uAPI interface.
Can you expand on what you mean by "proper uAPI interface" please?
Thanks,
Richard
Re: [PATCH net-next] ptp: add debugfs support for IDT family of timing devices
On Fri, 10 Jul 2020 11:41:25 -0400 min.li...@renesas.com wrote:
> From: Min Li
>
> This patch is to add debugfs support for ptp_clockmatrix and ptp_idt82p33.
> It will create a debugfs directory called idtptp{x} and x is the ptp index.
> Three inerfaces are present, which are cmd, help and regs. help is read
> only and will display a brief help message. regs is read only amd will show
> all register values. cmd is write only and will accept certain commands.
> Currently, the accepted commands are combomode to set comobo mode and write
> to write up to 4 registers.
>
> Signed-off-by: Min Li
No private configuration interfaces in debugfs, please.
If what you're exposing is a useful feature it deserves a proper
uAPI interface.
[PATCH net-next] ptp: add debugfs support for IDT family of timing devices
From: Min Li
This patch is to add debugfs support for ptp_clockmatrix and ptp_idt82p33.
It will create a debugfs directory called idtptp{x} and x is the ptp index.
Three inerfaces are present, which are cmd, help and regs. help is read
only and will display a brief help message. regs is read only amd will show
all register values. cmd is write only and will accept certain commands.
Currently, the accepted commands are combomode to set comobo mode and write
to write up to 4 registers.
Signed-off-by: Min Li
---
drivers/ptp/Kconfig | 2 +
drivers/ptp/Makefile | 4 +-
drivers/ptp/ptp_idt_debugfs.c | 354 ++
drivers/ptp/ptp_idt_debugfs.h | 83 ++
4 files changed, 442 insertions(+), 1 deletion(-)
create mode 100644 drivers/ptp/ptp_idt_debugfs.c
create mode 100644 drivers/ptp/ptp_idt_debugfs.h
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index 942f72d..25b74a2 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -118,6 +118,7 @@ config PTP_1588_CLOCK_KVM
config PTP_1588_CLOCK_IDT82P33
tristate "IDT 82P33xxx PTP clock"
depends on PTP_1588_CLOCK && I2C
+ select DEBUG_FS
default n
help
This driver adds support for using the IDT 82P33xxx as a PTP
@@ -130,6 +131,7 @@ config PTP_1588_CLOCK_IDT82P33
config PTP_1588_CLOCK_IDTCM
tristate "IDT CLOCKMATRIX as PTP clock"
depends on PTP_1588_CLOCK && I2C
+ select DEBUG_FS
default n
help
This driver adds support for using IDT CLOCKMATRIX(TM) as a PTP
diff --git a/drivers/ptp/Makefile b/drivers/ptp/Makefile
index 7aff75f..a4a9e35 100644
--- a/drivers/ptp/Makefile
+++ b/drivers/ptp/Makefile
@@ -12,6 +12,8 @@ obj-$(CONFIG_PTP_1588_CLOCK_KVM) += ptp_kvm.o
obj-$(CONFIG_PTP_1588_CLOCK_QORIQ) += ptp-qoriq.o
ptp-qoriq-y+= ptp_qoriq.o
ptp-qoriq-$(CONFIG_DEBUG_FS) += ptp_qoriq_debugfs.o
-obj-$(CONFIG_PTP_1588_CLOCK_IDTCM) += ptp_clockmatrix.o
+obj-$(CONFIG_PTP_1588_CLOCK_IDTCM) += ptp_idtcm.o
obj-$(CONFIG_PTP_1588_CLOCK_IDT82P33) += ptp_idt82p33.o
obj-$(CONFIG_PTP_1588_CLOCK_VMW) += ptp_vmw.o
+
+ptp_idtcm-objs += ptp_clockmatrix.o ptp_idt_debugfs.o
diff --git a/drivers/ptp/ptp_idt_debugfs.c b/drivers/ptp/ptp_idt_debugfs.c
new file mode 100644
index 000..0ae02f7
--- /dev/null
+++ b/drivers/ptp/ptp_idt_debugfs.c
@@ -0,0 +1,354 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * PTP debugfs interface for the IDT family of timing and
+ * synchronization devices.
+ *
+ * Copyright (C) 2019 Integrated Device Technology, Inc., a Renesas Company.
+ */
+#include
+#include
+#include
+#include
+#include
+
+#include "ptp_idt_debugfs.h"
+
+MODULE_LICENSE("GPL");
+
+#define DEBUGFS_CMD_MAX_WRITE_SIZE (4)
+#define DEBUGFS_CMD_MENU_SIZE (4)
+#define DEBUGFS_CMD_MAX_SIZE (64)
+#define DEBUGFS_CMD_HELP \
+ "1) set combomode:\n" \
+ "echo -n \"combomode:mode[current/fast/slow/hold]\" > cmd\n" \
+ "2) write up to 4 registers:\n" \
+ "echo -n \"write:addr[hex]:value[hex]:num of bytes[max 4]\" > cmd\n" \
+ "3) show all registers:\n" \
+ "cat regs"
+
+struct idtptp_debugfs {
+ idtptp_debugfs_handle handle;
+ struct ptp_clock_info *info;
+ debugfs_cmd_handler handler;
+ struct idtptp_debugfs_regs space;
+ struct dentry *root;
+ struct dentry *cmd;
+ struct dentry *regs;
+ struct dentry *help;
+};
+
+struct idtptp_debugfs_menu {
+ const char *input[DEBUGFS_CMD_MENU_SIZE];
+ const int output[DEBUGFS_CMD_MENU_SIZE];
+ const int size;
+};
+
+static struct idtptp_debugfs_menu cmdmenu = {
+ .input = {"combomode", "write", "read"},
+ .output = {IDTPTP_DEBUGFS_COMBOMODE,
+ IDTPTP_DEBUGFS_WRITE_REG,
+ IDTPTP_DEBUGFS_READ_REG},
+ .size = 3,
+};
+
+static struct idtptp_debugfs_menu combomodemenu = {
+ .input = {"current", "fast", "slow", "hold"},
+ .output = {E_COMBOMODE_CURRENT,
+ E_COMBOMODE_FASTAVG,
+ E_COMBOMODE_SLOWAVG,
+ E_COMBOMODE_HOLDOVER},
+ .size = 4,
+};
+
+static inline int
+idtptp_debugfs_parse_menu(char *input,
+ struct idtptp_debugfs_menu *menu)
+{
+ int i;
+
+ for (i = 0; i < menu->size; i++) {
+ if (strcmp(input, menu->input[i]) == 0)
+ return menu->output[i];
+ }
+
+ return -EINVAL;
+}
+
+static inline int
+idtptp_debugfs_parse_combomode(char *param,
+ struct idtptp_debugfs_data *data)
+{
+ char **strp =
+ char *modein;
+
+ modein = strsep(strp, ":");
+ if (modein == NULL)
+ return -EINVAL;
+
+ data->combomode = idtptp_debugfs_parse_menu(modein, );
+
+ return data->combomode;
+}
+
+static inline int
+idtptp_debugfs_parse_write(char *param,
+
