| Message ID | 20240826123602.1872-8-hpausten@protonmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | clk: clocking-wizard: add user clock monitor support | expand |
On Mon, Aug 26, 2024 at 12:38:36PM +0000, Harry Austen wrote: > Xilinx clocking wizard IP core supports monitoring of up to four > optional user clock inputs, with a corresponding interrupt for > notification in change of clock state (stop, underrun, overrun or > glitch). Give userspace access to this monitor logic through use of the > UIO framework. > > Implemented as an auxiliary_driver to avoid introducing UIO dependency > to the main clock driver. > > Signed-off-by: Harry Austen <hpausten@protonmail.com> > --- > drivers/uio/Kconfig | 8 ++++ > drivers/uio/Makefile | 1 + > drivers/uio/uio_xlnx_clk_mon.c | 71 ++++++++++++++++++++++++++++++++++ > 3 files changed, 80 insertions(+) > create mode 100644 drivers/uio/uio_xlnx_clk_mon.c > > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig > index b060dcd7c6350..ca8a53de26a67 100644 > --- a/drivers/uio/Kconfig > +++ b/drivers/uio/Kconfig > @@ -164,4 +164,12 @@ config UIO_DFL > opae-sdk/tools/libopaeuio/ > > If you compile this as a module, it will be called uio_dfl. > + > +config UIO_XLNX_CLK_MON > + tristate "Xilinx user clock monitor support" > + depends on COMMON_CLK_XLNX_CLKWZRD > + help > + Userspace I/O interface to the user clock monitor logic within the > + Xilinx Clocking Wizard IP core. Why do you want a UIO api for a clock device? What userspace code is going to access the hardware this way? Why not use the normal kernel/user apis instead? thanks, greg k-h
On Mon Aug 26, 2024 at 2:11 PM BST, Greg Kroah-Hartman wrote: > On Mon, Aug 26, 2024 at 12:38:36PM +0000, Harry Austen wrote: > > Xilinx clocking wizard IP core supports monitoring of up to four > > optional user clock inputs, with a corresponding interrupt for > > notification in change of clock state (stop, underrun, overrun or > > glitch). Give userspace access to this monitor logic through use of the > > UIO framework. > > > > Implemented as an auxiliary_driver to avoid introducing UIO dependency > > to the main clock driver. > > > > Signed-off-by: Harry Austen <hpausten@protonmail.com> > > --- > > drivers/uio/Kconfig | 8 ++++ > > drivers/uio/Makefile | 1 + > > drivers/uio/uio_xlnx_clk_mon.c | 71 ++++++++++++++++++++++++++++++++++ > > 3 files changed, 80 insertions(+) > > create mode 100644 drivers/uio/uio_xlnx_clk_mon.c > > > > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig > > index b060dcd7c6350..ca8a53de26a67 100644 > > --- a/drivers/uio/Kconfig > > +++ b/drivers/uio/Kconfig > > @@ -164,4 +164,12 @@ config UIO_DFL > > opae-sdk/tools/libopaeuio/ > > > > If you compile this as a module, it will be called uio_dfl. > > + > > +config UIO_XLNX_CLK_MON > > + tristate "Xilinx user clock monitor support" > > + depends on COMMON_CLK_XLNX_CLKWZRD > > + help > > + Userspace I/O interface to the user clock monitor logic within the > > + Xilinx Clocking Wizard IP core. > > Why do you want a UIO api for a clock device? What userspace code is > going to access the hardware this way? Why not use the normal > kernel/user apis instead? I was just trying to provide userspace access to these _unexpected_ clock status event indications (clock stopped, underrun, overrun or glitched) and UIO seemed like an easy way to do it and leave interrupt enablement and monitoring up to userspace. I'm not aware of any existing clock event notification framework. Are you suggesting that such a generic event notification mechanism should be added to the clk subsystem? e.g. additional clk_ops callbacks etc.? > > thanks, > > greg k-h Thanks for the review, Harry
Quoting Harry Austen (2024-08-27 12:08:52) > On Mon Aug 26, 2024 at 2:11 PM BST, Greg Kroah-Hartman wrote: > > On Mon, Aug 26, 2024 at 12:38:36PM +0000, Harry Austen wrote: > > > Xilinx clocking wizard IP core supports monitoring of up to four > > > optional user clock inputs, with a corresponding interrupt for > > > notification in change of clock state (stop, underrun, overrun or > > > glitch). Give userspace access to this monitor logic through use of the > > > UIO framework. > > > > > > Implemented as an auxiliary_driver to avoid introducing UIO dependency > > > to the main clock driver. > > > > > > Signed-off-by: Harry Austen <hpausten@protonmail.com> > > > --- > > > drivers/uio/Kconfig | 8 ++++ > > > drivers/uio/Makefile | 1 + > > > drivers/uio/uio_xlnx_clk_mon.c | 71 ++++++++++++++++++++++++++++++++++ > > > 3 files changed, 80 insertions(+) > > > create mode 100644 drivers/uio/uio_xlnx_clk_mon.c > > > > > > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig > > > index b060dcd7c6350..ca8a53de26a67 100644 > > > --- a/drivers/uio/Kconfig > > > +++ b/drivers/uio/Kconfig > > > @@ -164,4 +164,12 @@ config UIO_DFL > > > opae-sdk/tools/libopaeuio/ > > > > > > If you compile this as a module, it will be called uio_dfl. > > > + > > > +config UIO_XLNX_CLK_MON > > > + tristate "Xilinx user clock monitor support" > > > + depends on COMMON_CLK_XLNX_CLKWZRD > > > + help > > > + Userspace I/O interface to the user clock monitor logic within the > > > + Xilinx Clocking Wizard IP core. > > > > Why do you want a UIO api for a clock device? What userspace code is > > going to access the hardware this way? Why not use the normal > > kernel/user apis instead? > > I was just trying to provide userspace access to these _unexpected_ clock > status event indications (clock stopped, underrun, overrun or glitched) and UIO Maybe unexpected events can be indicated through the EDAC subsystem, except that is usually about memory or cache errors, not device driver issues. > seemed like an easy way to do it and leave interrupt enablement and monitoring > up to userspace. I'm not aware of any existing clock event notification > framework. Are you suggesting that such a generic event notification mechanism > should be added to the clk subsystem? e.g. additional clk_ops callbacks etc.? > I've been thinking of adding devcoredump support to clk drivers when they hit an error condition. The idea is it would be a coredump for the device register state when the clk driver detects an error. Maybe you can use devcoredump for this?
On Tue, Aug 27, 2024 at 04:40:52PM -0700, Stephen Boyd wrote: > Quoting Harry Austen (2024-08-27 12:08:52) > > On Mon Aug 26, 2024 at 2:11 PM BST, Greg Kroah-Hartman wrote: > > > On Mon, Aug 26, 2024 at 12:38:36PM +0000, Harry Austen wrote: > > > > Xilinx clocking wizard IP core supports monitoring of up to four > > > > optional user clock inputs, with a corresponding interrupt for > > > > notification in change of clock state (stop, underrun, overrun or > > > > glitch). Give userspace access to this monitor logic through use of the > > > > UIO framework. > > > > > > > > Implemented as an auxiliary_driver to avoid introducing UIO dependency > > > > to the main clock driver. > > > > > > > > Signed-off-by: Harry Austen <hpausten@protonmail.com> > > > > --- > > > > drivers/uio/Kconfig | 8 ++++ > > > > drivers/uio/Makefile | 1 + > > > > drivers/uio/uio_xlnx_clk_mon.c | 71 ++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 80 insertions(+) > > > > create mode 100644 drivers/uio/uio_xlnx_clk_mon.c > > > > > > > > diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig > > > > index b060dcd7c6350..ca8a53de26a67 100644 > > > > --- a/drivers/uio/Kconfig > > > > +++ b/drivers/uio/Kconfig > > > > @@ -164,4 +164,12 @@ config UIO_DFL > > > > opae-sdk/tools/libopaeuio/ > > > > > > > > If you compile this as a module, it will be called uio_dfl. > > > > + > > > > +config UIO_XLNX_CLK_MON > > > > + tristate "Xilinx user clock monitor support" > > > > + depends on COMMON_CLK_XLNX_CLKWZRD > > > > + help > > > > + Userspace I/O interface to the user clock monitor logic within the > > > > + Xilinx Clocking Wizard IP core. > > > > > > Why do you want a UIO api for a clock device? What userspace code is > > > going to access the hardware this way? Why not use the normal > > > kernel/user apis instead? > > > > I was just trying to provide userspace access to these _unexpected_ clock > > status event indications (clock stopped, underrun, overrun or glitched) and UIO That is going to be a brand-new user/kernel api that isn't documented anywhere and will be unique to this one device. Please don't do that. > Maybe unexpected events can be indicated through the EDAC subsystem, > except that is usually about memory or cache errors, not device driver > issues. If you all need a new way to report issues like this to userspace, then let's create the correct api for it, don't require userspace to mmap a random device and expect to poke around in it safely to get the information. Odds are that mmap will change with the next version of this device, right? thanks, greg k-h
Quoting Greg Kroah-Hartman (2024-08-28 00:10:46) > On Tue, Aug 27, 2024 at 04:40:52PM -0700, Stephen Boyd wrote: > > Quoting Harry Austen (2024-08-27 12:08:52) > > > On Mon Aug 26, 2024 at 2:11 PM BST, Greg Kroah-Hartman wrote: > > > > Why do you want a UIO api for a clock device? What userspace code is > > > > going to access the hardware this way? Why not use the normal > > > > kernel/user apis instead? > > > > > > I was just trying to provide userspace access to these _unexpected_ clock > > > status event indications (clock stopped, underrun, overrun or glitched) and UIO > > That is going to be a brand-new user/kernel api that isn't documented > anywhere and will be unique to this one device. Please don't do that. > > > Maybe unexpected events can be indicated through the EDAC subsystem, > > except that is usually about memory or cache errors, not device driver > > issues. > > If you all need a new way to report issues like this to userspace, then > let's create the correct api for it, don't require userspace to mmap a > random device and expect to poke around in it safely to get the > information. > > Odds are that mmap will change with the next version of this device, > right? Agreed. I'm wondering if we don't even need to invent anything new though and can simply use devcoredump. Harry?
On Thu Aug 29, 2024 at 7:17 PM BST, Stephen Boyd wrote: > Quoting Greg Kroah-Hartman (2024-08-28 00:10:46) > > On Tue, Aug 27, 2024 at 04:40:52PM -0700, Stephen Boyd wrote: > > > Quoting Harry Austen (2024-08-27 12:08:52) > > > > On Mon Aug 26, 2024 at 2:11 PM BST, Greg Kroah-Hartman wrote: > > > > > Why do you want a UIO api for a clock device? What userspace code is > > > > > going to access the hardware this way? Why not use the normal > > > > > kernel/user apis instead? > > > > > > > > I was just trying to provide userspace access to these _unexpected_ clock > > > > status event indications (clock stopped, underrun, overrun or glitched) and UIO > > > > That is going to be a brand-new user/kernel api that isn't documented > > anywhere and will be unique to this one device. Please don't do that. > > > > > Maybe unexpected events can be indicated through the EDAC subsystem, > > > except that is usually about memory or cache errors, not device driver > > > issues. > > > > If you all need a new way to report issues like this to userspace, then > > let's create the correct api for it, don't require userspace to mmap a > > random device and expect to poke around in it safely to get the > > information. > > > > Odds are that mmap will change with the next version of this device, > > right? > > Agreed. I'm wondering if we don't even need to invent anything new > though and can simply use devcoredump. Harry? I am not familiar with devcoredump, so will experiment with it for the user clock monitor support. Thanks for the suggestion! In the mean time, I think it might make more sense to split out the other _tidyup_ patches into a separate patchset, so that can be merged separately first. Will do that next. Thanks for the review! Harry
diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig index b060dcd7c6350..ca8a53de26a67 100644 --- a/drivers/uio/Kconfig +++ b/drivers/uio/Kconfig @@ -164,4 +164,12 @@ config UIO_DFL opae-sdk/tools/libopaeuio/ If you compile this as a module, it will be called uio_dfl. + +config UIO_XLNX_CLK_MON + tristate "Xilinx user clock monitor support" + depends on COMMON_CLK_XLNX_CLKWZRD + help + Userspace I/O interface to the user clock monitor logic within the + Xilinx Clocking Wizard IP core. + endif diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile index 1c5f3b5a95cf5..1e8c242265431 100644 --- a/drivers/uio/Makefile +++ b/drivers/uio/Makefile @@ -11,3 +11,4 @@ obj-$(CONFIG_UIO_MF624) += uio_mf624.o obj-$(CONFIG_UIO_FSL_ELBC_GPCM) += uio_fsl_elbc_gpcm.o obj-$(CONFIG_UIO_HV_GENERIC) += uio_hv_generic.o obj-$(CONFIG_UIO_DFL) += uio_dfl.o +obj-$(CONFIG_UIO_XLNX_CLK_MON) += uio_xlnx_clk_mon.o diff --git a/drivers/uio/uio_xlnx_clk_mon.c b/drivers/uio/uio_xlnx_clk_mon.c new file mode 100644 index 0000000000000..afcbeae98eaaf --- /dev/null +++ b/drivers/uio/uio_xlnx_clk_mon.c @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Driver for user clock monitor logic within Xilinx 'Clocking Wizard' IP core + * + * Copyright (C) 2024 Harry Austen <hpausten@protonmail.com> + */ + +#include <linux/auxiliary_bus.h> +#include <linux/bits.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/uio_driver.h> + +#define WZRD_INTR_ENABLE 0x10 + +static int clk_mon_irqcontrol(struct uio_info *info, s32 irq_on) +{ + if (irq_on) + iowrite32(GENMASK(15, 0), info->mem[0].internal_addr + WZRD_INTR_ENABLE); + else + iowrite32(0, info->mem[0].internal_addr + WZRD_INTR_ENABLE); + + return 0; +} + +static int probe(struct auxiliary_device *adev, const struct auxiliary_device_id *id) +{ + struct platform_device *pdev = to_platform_device(adev->dev.parent); + struct device *dev = &adev->dev; + struct uio_info *info; + int irq; + + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); + if (!info) + return -ENOMEM; + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return 0; + + info->name = KBUILD_MODNAME; + info->version = "0.0.1"; + + info->mem[0].name = "clock monitor"; + info->mem[0].memtype = UIO_MEM_PHYS; + info->mem[0].addr = platform_get_resource(pdev, IORESOURCE_IO, 0)->start; + info->mem[0].size = (WZRD_INTR_ENABLE + 4 + PAGE_SIZE - 1) & PAGE_MASK; + info->mem[0].internal_addr = (__force void __iomem *)dev->platform_data; + + info->irq = irq; + info->irqcontrol = clk_mon_irqcontrol; + return devm_uio_register_device(dev, info); +} + +static struct auxiliary_device_id ids[] = { + { .name = "clk_xlnx_clock_wizard.clk-mon" }, + {} +}; +MODULE_DEVICE_TABLE(auxiliary, ids); + +static struct auxiliary_driver xlnx_clk_mon_driver = { + .id_table = ids, + .probe = probe, +}; + +module_auxiliary_driver(xlnx_clk_mon_driver); + +MODULE_AUTHOR("Harry Austen <hpausten@protonmail.com>"); +MODULE_DESCRIPTION("Driver for Xilinx user clock monitor logic"); +MODULE_LICENSE("GPL");
Xilinx clocking wizard IP core supports monitoring of up to four optional user clock inputs, with a corresponding interrupt for notification in change of clock state (stop, underrun, overrun or glitch). Give userspace access to this monitor logic through use of the UIO framework. Implemented as an auxiliary_driver to avoid introducing UIO dependency to the main clock driver. Signed-off-by: Harry Austen <hpausten@protonmail.com> --- drivers/uio/Kconfig | 8 ++++ drivers/uio/Makefile | 1 + drivers/uio/uio_xlnx_clk_mon.c | 71 ++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+) create mode 100644 drivers/uio/uio_xlnx_clk_mon.c