[PATCH 2/3] hwmon: Support ADI Fan Control IP
The purpose of this IP Core is to control the fan used for the cooling of a
Xilinx Zynq Ultrascale+ MPSoC without the need of any external temperature
sensors. To achieve this, the IP core uses the PL SYSMONE4 primitive to
obtain the PL temperature and, based on those readings, it then outputs
a PWM signal to control the fan rotation accordingly.
Signed-off-by: Nuno Sá
---
MAINTAINERS | 7 +
drivers/hwmon/Kconfig | 9 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/axi-fan-control.c | 452
4 files changed, 469 insertions(+)
create mode 100644 drivers/hwmon/axi-fan-control.c
diff --git a/MAINTAINERS b/MAINTAINERS
index c7035ce2460b..d775c923d23b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2853,6 +2853,13 @@ S: Maintained
F: Documentation/devicetree/bindings/sound/axentia,*
F: sound/soc/atmel/tse850-pcm5142.c
+AXI-FAN-CONTROL HARDWARE MONITOR DRIVER
+M: Nuno Sá
+W: http://ez.analog.com/community/linux-device-drivers
+L: linux-hwmon@vger.kernel.org
+S: Supported
+F: drivers/hwmon/axi-fan-control.c
+
AXXIA I2C CONTROLLER
M: Krzysztof Adamski
L: linux-...@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 2ca5668bdb62..2396cc347c47 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -269,6 +269,15 @@ config SENSORS_ASC7621
This driver can also be built as a module. If so, the module
will be called asc7621.
+config SENSORS_AXI_FAN_CONTROL
+ tristate "Analog Devices FAN Control HDL Core driver"
+ help
+ If you say yes here you get support for the Analog Devices
+ AXI HDL FAN monitoring core.
+
+ This driver can also be built as a module. If so, the module
+ will be called axi-fan-control
+
config SENSORS_K8TEMP
tristate "AMD Athlon64/FX or Opteron temperature sensor"
depends on X86 && PCI
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index c86ce4d3d36b..ebb1fd2ea82b 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_SENSORS_AS370) += as370-hwmon.o
obj-$(CONFIG_SENSORS_ASC7621) += asc7621.o
obj-$(CONFIG_SENSORS_ASPEED) += aspeed-pwm-tacho.o
obj-$(CONFIG_SENSORS_ATXP1)+= atxp1.o
+obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o
obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi-fan-control.c
new file mode 100644
index ..f86efc444753
--- /dev/null
+++ b/drivers/hwmon/axi-fan-control.c
@@ -0,0 +1,452 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Fan Control HDL CORE driver
+ *
+ * Copyright 2019 Analog Devices Inc.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* register map */
+#define ADI_REG_RSTN 0x0080
+#define ADI_REG_PWM_WIDTH 0x0084
+#define ADI_REG_TACH_PERIOD0x0088
+#define ADI_REG_TACH_TOLERANCE 0x008c
+#define ADI_REG_PWM_PERIOD 0x00c0
+#define ADI_REG_TACH_MEASUR0x00c4
+#define ADI_REG_TEMPERATURE0x00c8
+
+#define ADI_REG_IRQ_MASK 0x0040
+#define ADI_REG_IRQ_PENDING0x0044
+#define ADI_REG_IRQ_SRC0x0048
+
+/* IRQ sources */
+#define ADI_IRQ_SRC_PWM_CHANGEDBIT(0)
+#define ADI_IRQ_SRC_TACH_ERR BIT(1)
+#define ADI_IRQ_SRC_TEMP_INCREASE BIT(2)
+#define ADI_IRQ_SRC_NEW_MEASUR BIT(3)
+#define ADI_IRQ_SRC_MASK GENMASK(3, 0)
+#define ADI_IRQ_MASK_OUT_ALL 0xU
+
+#define SYSFS_PWM_MAX 255
+
+struct axi_fan_control_data {
+ struct clk *clk;
+ void __iomem *base;
+ /* tacho period */
+ atomic_t tach;
+ int irq;
+ /* pulses per revolution */
+ u32 ppr;
+ bool hw_pwm_req;
+ bool update_tacho_params;
+ u8 fan_fault;
+};
+
+static inline void axi_fan_control_iowrite(const u32 val, const u32 reg,
+ const struct axi_fan_control_data *ctl)
+{
+ iowrite32(val, ctl->base + reg);
+}
+
+static inline u32 axi_fan_control_ioread(const u32 reg,
+const struct axi_fan_control_data *ctl)
+{
+ return ioread32(ctl->base + reg);
+}
+
+static long axi_fan_control_get_pwm_duty(const struct axi_fan_control_data
*ctl)
+{
+ u32 pwm_width = axi_fan_control_ioread(ADI_REG_PWM_WIDTH, ctl);
+ u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD, ctl);
+
+ return DIV_ROUND_CLOSEST(pwm_width * SYSFS_PWM_MAX, pwm_period);
+}
+
+static int axi_fan_control_set_pwm_duty(const long val,
+ struct axi_fan_control_data *ctl)
+{
+ u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD, ctl);
+ u32 new_width;
+ long __val = cl
Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP
On Thu, Sep 26, 2019 at 12:39:24PM +0200, Nuno Sá wrote:
> The purpose of this IP Core is to control the fan used for the cooling of a
> Xilinx Zynq Ultrascale+ MPSoC without the need of any external temperature
> sensors. To achieve this, the IP core uses the PL SYSMONE4 primitive to
> obtain the PL temperature and, based on those readings, it then outputs
> a PWM signal to control the fan rotation accordingly.
>
> Signed-off-by: Nuno Sá
> ---
> MAINTAINERS | 7 +
> drivers/hwmon/Kconfig | 9 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/axi-fan-control.c | 452
> 4 files changed, 469 insertions(+)
> create mode 100644 drivers/hwmon/axi-fan-control.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c7035ce2460b..d775c923d23b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2853,6 +2853,13 @@ S: Maintained
> F: Documentation/devicetree/bindings/sound/axentia,*
> F: sound/soc/atmel/tse850-pcm5142.c
>
> +AXI-FAN-CONTROL HARDWARE MONITOR DRIVER
> +M: Nuno Sá
> +W: http://ez.analog.com/community/linux-device-drivers
> +L: linux-hwmon@vger.kernel.org
> +S: Supported
> +F: drivers/hwmon/axi-fan-control.c
> +
> AXXIA I2C CONTROLLER
> M: Krzysztof Adamski
> L: linux-...@vger.kernel.org
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 2ca5668bdb62..2396cc347c47 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -269,6 +269,15 @@ config SENSORS_ASC7621
> This driver can also be built as a module. If so, the module
> will be called asc7621.
>
> +config SENSORS_AXI_FAN_CONTROL
> + tristate "Analog Devices FAN Control HDL Core driver"
> + help
> + If you say yes here you get support for the Analog Devices
> + AXI HDL FAN monitoring core.
> +
> + This driver can also be built as a module. If so, the module
> + will be called axi-fan-control
> +
> config SENSORS_K8TEMP
> tristate "AMD Athlon64/FX or Opteron temperature sensor"
> depends on X86 && PCI
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index c86ce4d3d36b..ebb1fd2ea82b 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_SENSORS_AS370) += as370-hwmon.o
> obj-$(CONFIG_SENSORS_ASC7621)+= asc7621.o
> obj-$(CONFIG_SENSORS_ASPEED) += aspeed-pwm-tacho.o
> obj-$(CONFIG_SENSORS_ATXP1) += atxp1.o
> +obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
> obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o
> obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
> diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi-fan-control.c
> new file mode 100644
> index ..f86efc444753
> --- /dev/null
> +++ b/drivers/hwmon/axi-fan-control.c
> @@ -0,0 +1,452 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Fan Control HDL CORE driver
> + *
> + * Copyright 2019 Analog Devices Inc.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* register map */
> +#define ADI_REG_RSTN 0x0080
> +#define ADI_REG_PWM_WIDTH0x0084
> +#define ADI_REG_TACH_PERIOD 0x0088
> +#define ADI_REG_TACH_TOLERANCE 0x008c
> +#define ADI_REG_PWM_PERIOD 0x00c0
> +#define ADI_REG_TACH_MEASUR 0x00c4
> +#define ADI_REG_TEMPERATURE 0x00c8
> +
> +#define ADI_REG_IRQ_MASK 0x0040
> +#define ADI_REG_IRQ_PENDING 0x0044
> +#define ADI_REG_IRQ_SRC 0x0048
> +
> +/* IRQ sources */
> +#define ADI_IRQ_SRC_PWM_CHANGED BIT(0)
include linux/bits.h
> +#define ADI_IRQ_SRC_TACH_ERR BIT(1)
> +#define ADI_IRQ_SRC_TEMP_INCREASEBIT(2)
> +#define ADI_IRQ_SRC_NEW_MEASUR BIT(3)
> +#define ADI_IRQ_SRC_MASK GENMASK(3, 0)
> +#define ADI_IRQ_MASK_OUT_ALL 0xU
> +
> +#define SYSFS_PWM_MAX255
> +
> +struct axi_fan_control_data {
> + struct clk *clk;
> + void __iomem *base;
> + /* tacho period */
> + atomic_t tach;
Why exactly is this an atomic ? The value is only set, and it is set in
a single operation. Why does it matter if reqading it catches the old or
the new value ?
> + int irq;
> + /* pulses per revolution */
> + u32 ppr;
> + bool hw_pwm_req;
> + bool update_tacho_params;
> + u8 fan_fault;
> +};
> +
> +static inline void axi_fan_control_iowrite(const u32 val, const u32 reg,
> + const struct axi_fan_control_data *ctl)
Multi-line alignment. Also, please consider shorter function names.
An "axi_fan_control_" prefix for static functions is really a bit excessive
and doesn't add value. "axi_" would do it just as well.
> +{
> + iowrite32(val, ctl->base + reg);
> +}
> +
> +static inline u32 axi_fan_control_ioread(const u32 reg,
> + const struct axi_fa
Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP
On Sun, 2019-10-06 at 08:32 -0700, Guenter Roeck wrote:
>
> On Thu, Sep 26, 2019 at 12:39:24PM +0200, Nuno Sá wrote:
> > The purpose of this IP Core is to control the fan used for the
> > cooling of a
> > Xilinx Zynq Ultrascale+ MPSoC without the need of any external
> > temperature
> > sensors. To achieve this, the IP core uses the PL SYSMONE4
> > primitive to
> > obtain the PL temperature and, based on those readings, it then
> > outputs
> > a PWM signal to control the fan rotation accordingly.
> >
> > Signed-off-by: Nuno Sá
> > ---
> > MAINTAINERS | 7 +
> > drivers/hwmon/Kconfig | 9 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/axi-fan-control.c | 452
> >
> > 4 files changed, 469 insertions(+)
> > create mode 100644 drivers/hwmon/axi-fan-control.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c7035ce2460b..d775c923d23b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2853,6 +2853,13 @@ S: Maintained
> > F: Documentation/devicetree/bindings/sound/axentia,*
> > F: sound/soc/atmel/tse850-pcm5142.c
> >
> > +AXI-FAN-CONTROL HARDWARE MONITOR DRIVER
> > +M: Nuno Sá
> > +W: http://ez.analog.com/community/linux-device-drivers
> > +L: linux-hwmon@vger.kernel.org
> > +S: Supported
> > +F: drivers/hwmon/axi-fan-control.c
> > +
> > AXXIA I2C CONTROLLER
> > M: Krzysztof Adamski
> > L: linux-...@vger.kernel.org
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 2ca5668bdb62..2396cc347c47 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -269,6 +269,15 @@ config SENSORS_ASC7621
> > This driver can also be built as a module. If so, the module
> > will be called asc7621.
> >
> > +config SENSORS_AXI_FAN_CONTROL
> > + tristate "Analog Devices FAN Control HDL Core driver"
> > + help
> > + If you say yes here you get support for the Analog Devices
> > + AXI HDL FAN monitoring core.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called axi-fan-control
> > +
> > config SENSORS_K8TEMP
> > tristate "AMD Athlon64/FX or Opteron temperature sensor"
> > depends on X86 && PCI
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index c86ce4d3d36b..ebb1fd2ea82b 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -51,6 +51,7 @@ obj-$(CONFIG_SENSORS_AS370) += as370-
> > hwmon.o
> > obj-$(CONFIG_SENSORS_ASC7621) += asc7621.o
> > obj-$(CONFIG_SENSORS_ASPEED) += aspeed-pwm-tacho.o
> > obj-$(CONFIG_SENSORS_ATXP1)+= atxp1.o
> > +obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
> > obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o
> > obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> > obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
> > diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi-
> > fan-control.c
> > new file mode 100644
> > index ..f86efc444753
> > --- /dev/null
> > +++ b/drivers/hwmon/axi-fan-control.c
> > @@ -0,0 +1,452 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Fan Control HDL CORE driver
> > + *
> > + * Copyright 2019 Analog Devices Inc.
> > + */
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +/* register map */
> > +#define ADI_REG_RSTN 0x0080
> > +#define ADI_REG_PWM_WIDTH 0x0084
> > +#define ADI_REG_TACH_PERIOD0x0088
> > +#define ADI_REG_TACH_TOLERANCE 0x008c
> > +#define ADI_REG_PWM_PERIOD 0x00c0
> > +#define ADI_REG_TACH_MEASUR0x00c4
> > +#define ADI_REG_TEMPERATURE0x00c8
> > +
> > +#define ADI_REG_IRQ_MASK 0x0040
> > +#define ADI_REG_IRQ_PENDING0x0044
> > +#define ADI_REG_IRQ_SRC0x0048
> > +
> > +/* IRQ sources */
> > +#define ADI_IRQ_SRC_PWM_CHANGEDBIT(0)
>
> include linux/bits.h
ack.
> > +#define ADI_IRQ_SRC_TACH_ERR BIT(1)
> > +#define ADI_IRQ_SRC_TEMP_INCREASE BIT(2)
> > +#define ADI_IRQ_SRC_NEW_MEASUR BIT(3)
> > +#define ADI_IRQ_SRC_MASK GENMASK(3, 0)
> > +#define ADI_IRQ_MASK_OUT_ALL 0xU
> > +
> > +#define SYSFS_PWM_MAX 255
> > +
> > +struct axi_fan_control_data {
> > + struct clk *clk;
> > + void __iomem *base;
> > + /* tacho period */
> > + atomic_t tach;
>
> Why exactly is this an atomic ? The value is only set, and it is set
> in
> a single operation. Why does it matter if reqading it catches the old
> or
> the new value ?
Hmm, I think that I don't even need this variable. I can just ioread
the value when getting the fan rpm and get whatever it is on the
register at that moment.
> > + int irq;
> > + /* pulses per revolution */
> > + u32 ppr;
> > + bool hw_pwm_req;
> > + bool update_tacho_params;
> > + u8 fan_fault;
> > +};
> > +
> > +static inline void axi
Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP
On 10/7/19 6:52 AM, Sa, Nuno wrote:
[ ... ]
+static long axi_fan_control_get_pwm_duty(const struct
axi_fan_control_data *ctl)
+{
+ u32 pwm_width = axi_fan_control_ioread(ADI_REG_PWM_WIDTH, ctl);
+ u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD,
ctl);
+
+ return DIV_ROUND_CLOSEST(pwm_width * SYSFS_PWM_MAX,
pwm_period);
Is pwm_period guaranteed to be != 0 ?
Yes, It is a RO register and it is set by the core with the default of
0x4e20.
Trusting the hardware doesn't make me too comfortable. Are we sure at all
times that the HW isn't messed up ? If so, please at least add a comment
stating that the HW will never return 0. We can then fix it after we get
the first crash report from the field ;-).
[ ... ]
+ if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE)
+ /* hardware requested a new pwm */
+ ctl->hw_pwm_req = true;
+
I don't really understand the logic here. If
ADI_IRQ_SRC_TEMP_INCREASE means
that hardware wants a new pwm, how is userspace informed about that
request ?
It isn't. Userspace would have to read the pwm attribute and figure
that changed. Should I use something like sysfs_notify() on the pwm
attribute?
That might make sense.
And why are the tacho paramaters _not_ updated in this case later on
(unless
ADI_IRQ_SRC_PWM_CHANGED and ADI_IRQ_SRC_TEMP_INCREASE are both set) ?
It might be useful to describe the expected sequence of events.
The core can change the PWM by itself (which is when we receive
ADI_IRQ_SRC_TEMP_INCREASE) and in that case it will use predefined
values to evaluate the tacho signal (so it won't use the values on
TACH_PERIOD and TACH_TOLERANCE). Alternatively, the user can request a
new PWM by writing the pwm attribute. In this case the CORE is
expecting that TACH_PERIOD and TACH_TOLERANCE are given otherwise it
won't evaluate the tacho signal. Note that when is the user which
requests a new pwm we only get ADI_IRQ_SRC_PWM_CHANGED (and not +
if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE), so I use that to know
when do I have to update the tacho parameters.
Wondering ... if setting the pwm requires an update of period and tolerance,
why not set update_tacho_params to true when the pwm value is written, or
update the registers directly instead of waiting for an interrupt ?
Either case, I think the above sequence of events should be explained
in the driver for future developers to understand why the code is written
the way it is.
)
+ if (irq_pending & ADI_IRQ_SRC_TACH_ERR)
+ ctl->fan_fault = 1;
Is it on purpose that this bit is never reset ?
Yes, and it is wrong. I though that the core would never clear this
alarm but it does clear it in the next temperature reading cycle (and
set it again if needed). Then, would a clear on read be a correct
approach?
Not sure if there is a "correct", but I think it would make sense.
+
+ /* clear all interrupts */
+ clear_mask = irq_pending & ADI_IRQ_SRC_MASK;
+ axi_fan_control_iowrite(clear_mask, ADI_REG_IRQ_PENDING, ctl);
+
+ return IRQ_HANDLED;
+}
+
+static int axi_fan_control_init(struct axi_fan_control_data *ctl,
+ const struct device_node *np)
+{
+ int ret;
+
+ /* get fan pulses per revolution */
+ ret = of_property_read_u32(np, "adi,pulses-per-revolution",
&ctl->ppr);
+ if (ret)
+ return ret;
So all random values are acceptable, including 0 and 0x ?
Yes, I'm aware that 1 and 2 are typical values but I'm not sure what is
the maximum that typically exists so I didn't want to put limits here
without knowing. Though at least 0 must not be accepted since then we
are always dividing by 0 when reading the FAN rpm.
The only values I am aware of are 2 and 4. I don't recall seeing any fans
with 1 pulse per revolution. Overall, I don't think values other than 1, 2,
and 4 make sense.
Guenter
Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP
On Mon, 2019-10-07 at 07:18 -0700, Guenter Roeck wrote:
>
> On 10/7/19 6:52 AM, Sa, Nuno wrote:
> [ ... ]
> > > > +static long axi_fan_control_get_pwm_duty(const struct
> > > > axi_fan_control_data *ctl)
> > > > +{
> > > > + u32 pwm_width =
> > > > axi_fan_control_ioread(ADI_REG_PWM_WIDTH, ctl);
> > > > + u32 pwm_period =
> > > > axi_fan_control_ioread(ADI_REG_PWM_PERIOD,
> > > > ctl);
> > > > +
> > > > + return DIV_ROUND_CLOSEST(pwm_width * SYSFS_PWM_MAX,
> > > > pwm_period);
> > >
> > > Is pwm_period guaranteed to be != 0 ?
> >
> > Yes, It is a RO register and it is set by the core with the default
> > of
> > 0x4e20.
>
> Trusting the hardware doesn't make me too comfortable. Are we sure at
> all
> times that the HW isn't messed up ? If so, please at least add a
> comment
> stating that the HW will never return 0. We can then fix it after we
> get
> the first crash report from the field ;-).
Will do that.
> [ ... ]
>
> > > > + if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE)
> > > > + /* hardware requested a new pwm */
> > > > + ctl->hw_pwm_req = true;
> > > > +
> > > I don't really understand the logic here. If
> > > ADI_IRQ_SRC_TEMP_INCREASE means
> > > that hardware wants a new pwm, how is userspace informed about
> > > that
> > > request ?
> >
> > It isn't. Userspace would have to read the pwm attribute and figure
> > that changed. Should I use something like sysfs_notify() on the pwm
> > attribute?
> >
> That might make sense.
>
> > > And why are the tacho paramaters _not_ updated in this case later
> > > on
> > > (unless
> > > ADI_IRQ_SRC_PWM_CHANGED and ADI_IRQ_SRC_TEMP_INCREASE are both
> > > set) ?
> > > It might be useful to describe the expected sequence of events.
> >
> > The core can change the PWM by itself (which is when we receive
> > ADI_IRQ_SRC_TEMP_INCREASE) and in that case it will use predefined
> > values to evaluate the tacho signal (so it won't use the values on
> > TACH_PERIOD and TACH_TOLERANCE). Alternatively, the user can
> > request a
> > new PWM by writing the pwm attribute. In this case the CORE is
> > expecting that TACH_PERIOD and TACH_TOLERANCE are given otherwise
> > it
> > won't evaluate the tacho signal. Note that when is the user which
> > requests a new pwm we only get ADI_IRQ_SRC_PWM_CHANGED (and not +
> > if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE), so I use that to know
> > when do I have to update the tacho parameters.
> >
> Wondering ... if setting the pwm requires an update of period and
> tolerance,
> why not set update_tacho_params to true when the pwm value is
> written, or
> update the registers directly instead of waiting for an interrupt ?
After requesting a new duty-cycle there is 5s delay on which the core
waits for the fan rotation speed to stabilize. Only after that, we have
to provide the tach parameters. Also note that we do need to use the
updated tach measurement value to derive this parameters. So, we need
to wait for the ADI_IRQ_SRC_NEW_MEASUR interrupt.
> Either case, I think the above sequence of events should be explained
> in the driver for future developers to understand why the code is
> written
> the way it is.
will do that.
> > )
> >
> > > > + if (irq_pending & ADI_IRQ_SRC_TACH_ERR)
> > > > + ctl->fan_fault = 1;
> > >
> > > Is it on purpose that this bit is never reset ?
> >
> > Yes, and it is wrong. I though that the core would never clear this
> > alarm but it does clear it in the next temperature reading cycle
> > (and
> > set it again if needed). Then, would a clear on read be a correct
> > approach?
>
> Not sure if there is a "correct", but I think it would make sense.
>
> > > > +
> > > > + /* clear all interrupts */
> > > > + clear_mask = irq_pending & ADI_IRQ_SRC_MASK;
> > > > + axi_fan_control_iowrite(clear_mask,
> > > > ADI_REG_IRQ_PENDING, ctl);
> > > > +
> > > > + return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static int axi_fan_control_init(struct axi_fan_control_data
> > > > *ctl,
> > > > + const struct device_node *np)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + /* get fan pulses per revolution */
> > > > + ret = of_property_read_u32(np, "adi,pulses-per-
> > > > revolution",
> > > > &ctl->ppr);
> > > > + if (ret)
> > > > + return ret;
> > >
> > > So all random values are acceptable, including 0 and 0x ?
> >
> > Yes, I'm aware that 1 and 2 are typical values but I'm not sure
> > what is
> > the maximum that typically exists so I didn't want to put limits
> > here
> > without knowing. Though at least 0 must not be accepted since then
> > we
> > are always dividing by 0 when reading the FAN rpm.
> >
> The only values I am aware of are 2 and 4. I don't recall seeing any
> fans
> with 1 pulse per revolution. Overall, I don't think values other than
> 1, 2,
> and 4 make sense.
Will use 1,2 and 4 and update dt bindings accor
Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP
Hi Guenter,
One question/doubt that I just noticed now...
On Sun, 2019-10-06 at 08:32 -0700, Guenter Roeck wrote:
>
> On Thu, Sep 26, 2019 at 12:39:24PM +0200, Nuno Sá wrote:
> > The purpose of this IP Core is to control the fan used for the
> > cooling of a
> > Xilinx Zynq Ultrascale+ MPSoC without the need of any external
> > temperature
> > sensors. To achieve this, the IP core uses the PL SYSMONE4
> > primitive to
> > obtain the PL temperature and, based on those readings, it then
> > outputs
> > a PWM signal to control the fan rotation accordingly.
> >
> > Signed-off-by: Nuno Sá
> > ---
> > MAINTAINERS | 7 +
> > drivers/hwmon/Kconfig | 9 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/axi-fan-control.c | 452
> >
> > 4 files changed, 469 insertions(+)
> > create mode 100644 drivers/hwmon/axi-fan-control.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c7035ce2460b..d775c923d23b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2853,6 +2853,13 @@ S: Maintained
> > F: Documentation/devicetree/bindings/sound/axentia,*
> > F: sound/soc/atmel/tse850-pcm5142.c
> >
> > +AXI-FAN-CONTROL HARDWARE MONITOR DRIVER
> > +M: Nuno Sá
> > +W: http://ez.analog.com/community/linux-device-drivers
> > +L: linux-hwmon@vger.kernel.org
> > +S: Supported
> > +F: drivers/hwmon/axi-fan-control.c
> > +
> > AXXIA I2C CONTROLLER
> > M: Krzysztof Adamski
> > L: linux-...@vger.kernel.org
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 2ca5668bdb62..2396cc347c47 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -269,6 +269,15 @@ config SENSORS_ASC7621
> > This driver can also be built as a module. If so, the module
> > will be called asc7621.
> >
> > +config SENSORS_AXI_FAN_CONTROL
> > + tristate "Analog Devices FAN Control HDL Core driver"
> > + help
> > + If you say yes here you get support for the Analog Devices
> > + AXI HDL FAN monitoring core.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called axi-fan-control
> > +
> > config SENSORS_K8TEMP
> > tristate "AMD Athlon64/FX or Opteron temperature sensor"
> > depends on X86 && PCI
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index c86ce4d3d36b..ebb1fd2ea82b 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -51,6 +51,7 @@ obj-$(CONFIG_SENSORS_AS370) += as370-
> > hwmon.o
> > obj-$(CONFIG_SENSORS_ASC7621) += asc7621.o
> > obj-$(CONFIG_SENSORS_ASPEED) += aspeed-pwm-tacho.o
> > obj-$(CONFIG_SENSORS_ATXP1)+= atxp1.o
> > +obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
> > obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o
> > obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> > obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
> > diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi-
> > fan-control.c
> > new file mode 100644
> > index ..f86efc444753
> > --- /dev/null
> > +++ b/drivers/hwmon/axi-fan-control.c
> > @@ -0,0 +1,452 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Fan Control HDL CORE driver
> > + *
> > + * Copyright 2019 Analog Devices Inc.
> > + */
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +/* register map */
> > +#define ADI_REG_RSTN 0x0080
> > +#define ADI_REG_PWM_WIDTH 0x0084
> > +#define ADI_REG_TACH_PERIOD0x0088
> > +#define ADI_REG_TACH_TOLERANCE 0x008c
> > +#define ADI_REG_PWM_PERIOD 0x00c0
> > +#define ADI_REG_TACH_MEASUR0x00c4
> > +#define ADI_REG_TEMPERATURE0x00c8
> > +
> > +#define ADI_REG_IRQ_MASK 0x0040
> > +#define ADI_REG_IRQ_PENDING0x0044
> > +#define ADI_REG_IRQ_SRC0x0048
> > +
> > +/* IRQ sources */
> > +#define ADI_IRQ_SRC_PWM_CHANGEDBIT(0)
>
> include linux/bits.h
>
> > +#define ADI_IRQ_SRC_TACH_ERR BIT(1)
> > +#define ADI_IRQ_SRC_TEMP_INCREASE BIT(2)
> > +#define ADI_IRQ_SRC_NEW_MEASUR BIT(3)
> > +#define ADI_IRQ_SRC_MASK GENMASK(3, 0)
> > +#define ADI_IRQ_MASK_OUT_ALL 0xU
> > +
> > +#define SYSFS_PWM_MAX 255
> > +
> > +struct axi_fan_control_data {
> > + struct clk *clk;
> > + void __iomem *base;
> > + /* tacho period */
> > + atomic_t tach;
>
> Why exactly is this an atomic ? The value is only set, and it is set
> in
> a single operation. Why does it matter if reqading it catches the old
> or
> the new value ?
>
> > + int irq;
> > + /* pulses per revolution */
> > + u32 ppr;
> > + bool hw_pwm_req;
> > + bool update_tacho_params;
> > + u8 fan_fault;
> > +};
> > +
> > +static inline void axi_fan_control_iowrite(const u32 val, const
> > u32 reg,
> > + const struct axi_
Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP
On Tue, Oct 08, 2019 at 03:59:49PM +, Sa, Nuno wrote:
[ ... ]
> > > +
> > > + ctl->irq = platform_get_irq(pdev, 0);
> > > + if (ctl->irq < 0) {
> >
> > This can return -EPROBE_DEFER. On top of that, it already generates
> > an error
> > message, meaning another one here is unnecessary.
>
> Why returning -EPROBE_DEFER? platform_get_irq() already seems to handle
> EPROBE_DEFER...
>
... which is exactly why I am saying that you don't need another error
message, and that an unconditional error message is a bad idea.
Guenter
Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP
On Tue, 2019-10-08 at 13:11 -0700, Guenter Roeck wrote:
>
> On Tue, Oct 08, 2019 at 03:59:49PM +, Sa, Nuno wrote:
> [ ... ]
> > > > +
> > > > + ctl->irq = platform_get_irq(pdev, 0);
> > > > + if (ctl->irq < 0) {
> > >
> > > This can return -EPROBE_DEFER. On top of that, it already
> > > generates
> > > an error
> > > message, meaning another one here is unnecessary.
> >
> > Why returning -EPROBE_DEFER? platform_get_irq() already seems to
> > handle
> > EPROBE_DEFER...
> >
> ... which is exactly why I am saying that you don't need another
> error
> message, and that an unconditional error message is a bad idea.
>
Ahhh, sorry. I completely misunderstood you :(
Nuno Sá
> Guenter
