diff mbox series

[v3,7/9] uio: add Xilinx user clock monitor support

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

Commit Message

Harry Austen Aug. 26, 2024, 12:38 p.m. UTC
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

Comments

Greg KH Aug. 26, 2024, 1:11 p.m. UTC | #1
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
Harry Austen Aug. 27, 2024, 7:08 p.m. UTC | #2
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
Stephen Boyd Aug. 27, 2024, 11:40 p.m. UTC | #3
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?
Greg KH Aug. 28, 2024, 7:10 a.m. UTC | #4
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
Stephen Boyd Aug. 29, 2024, 6:17 p.m. UTC | #5
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?
Harry Austen Aug. 30, 2024, 7:31 a.m. UTC | #6
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 mbox series

Patch

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");