| Message ID | 20190926103925.194973-3-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support AXI FAN Control IP core | expand |
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á <nuno.sa@analog.com> > --- > 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á <nuno.sa@analog.com> > +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 <krzysztof.adamski@nokia.com> > L: linux-i2c@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 000000000000..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 <linux/clk.h> > +#include <linux/fpga/adi-axi-common.h> > +#include <linux/hwmon.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > + > +/* register map */ > +#define ADI_REG_RSTN 0x0080 > +#define ADI_REG_PWM_WIDTH 0x0084 > +#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_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 0xFFFFFFFFU > + > +#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_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_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); Is pwm_period guaranteed to be != 0 ? > +} > + > +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 = clamp_val(val, 0, SYSFS_PWM_MAX); > + > + new_width = DIV_ROUND_CLOSEST(__val * pwm_period, SYSFS_PWM_MAX); > + > + axi_fan_control_iowrite(new_width, ADI_REG_PWM_WIDTH, ctl); > + > + return 0; > +} > + > +static long axi_fan_control_get_fan_rpm(const struct axi_fan_control_data *ctl) > +{ > + unsigned long clk_rate = clk_get_rate(ctl->clk); > + > + if (!clk_rate) > + return -EINVAL; Unless the clock rate changes dynamically, it might make sense to read it once in the probe function. > + /* > + * The tacho period should be: > + * TACH = 60/(ppr * rpm), where rpm is revolutions per second > + * and ppr is pulses per revolution. > + * Given the tacho period, we can multiply it by the input clock > + * so that we know how many clocks we need to have this period. > + * From this, we can derive the RPM value. > + */ > + return DIV_ROUND_CLOSEST(60 * clk_rate, > + ctl->ppr * atomic_read(&ctl->tach)); Are ppr and tach guaranteed to be != 0 ? I don't think so, since neither is ever evaluated. > +} > + > +static int axi_fan_control_read_temp(struct device *dev, u32 attr, long *val) > +{ > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > + long raw_temp; > + > + switch (attr) { > + case hwmon_temp_input: > + raw_temp = axi_fan_control_ioread(ADI_REG_TEMPERATURE, ctl); > + /* > + * The formula for the temperature is: > + * T = (ADC * 501.3743 / 2^bits) - 273.6777 > + * It's multiplied by 1000 to have milidegrees as s/milidegrees/millidegrees/ > + * specified by the hwmon sysfs interface. > + */ > + *val = ((raw_temp * 501374) >> 16) - 273677; > + return 0; > + default: > + return -ENOTSUPP; > + } > +} > + > +static int axi_fan_control_read_fan(struct device *dev, u32 attr, long *val) > +{ > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > + > + switch (attr) { > + case hwmon_fan_fault: > + *val = ctl->fan_fault; > + return 0; > + case hwmon_fan_input: > + *val = axi_fan_control_get_fan_rpm(ctl); > + return 0; > + default: > + return -ENOTSUPP; > + } > +} > + > +static int axi_fan_control_read_pwm(struct device *dev, u32 attr, long *val) > +{ > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > + > + switch (attr) { > + case hwmon_pwm_input: > + *val = axi_fan_control_get_pwm_duty(ctl); > + return 0; > + default: > + return -ENOTSUPP; > + } > +} > + > +static int axi_fan_control_write_pwm(struct device *dev, u32 attr, long val) > +{ > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > + > + switch (attr) { > + case hwmon_pwm_input: > + return axi_fan_control_set_pwm_duty(val, ctl); > + default: > + return -ENOTSUPP; > + } > +} > + > +static int axi_fan_control_read_labels(struct device *dev, > + enum hwmon_sensor_types type, > + u32 attr, int channel, const char **str) > +{ > + switch (type) { > + case hwmon_fan: > + *str = "FAN"; > + return 0; > + case hwmon_temp: > + *str = "SYSMON4"; > + return 0; > + default: > + return -ENOTSUPP; > + } > +} > + > +static int axi_fan_control_read(struct device *dev, > + enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + switch (type) { > + case hwmon_fan: > + return axi_fan_control_read_fan(dev, attr, val); > + case hwmon_pwm: > + return axi_fan_control_read_pwm(dev, attr, val); > + case hwmon_temp: > + return axi_fan_control_read_temp(dev, attr, val); > + default: > + return -ENOTSUPP; > + } > +} > + > +static int axi_fan_control_write(struct device *dev, > + enum hwmon_sensor_types type, > + u32 attr, int channel, long val) > +{ > + switch (type) { > + case hwmon_pwm: > + return axi_fan_control_write_pwm(dev, attr, val); > + default: > + return -ENOTSUPP; > + } > +} > + > +static umode_t axi_fan_control_fan_is_visible(const u32 attr) > +{ > + switch (attr) { > + case hwmon_fan_input: > + case hwmon_fan_fault: > + case hwmon_fan_label: > + return 0444; > + default: > + return 0; > + } > +} > + > +static umode_t axi_fan_control_pwm_is_visible(const u32 attr) > +{ > + switch (attr) { > + case hwmon_pwm_input: > + return 0644; > + default: > + return 0; > + } > +} > + > +static umode_t axi_fan_control_temp_is_visible(const u32 attr) > +{ > + switch (attr) { > + case hwmon_temp_input: > + case hwmon_temp_label: > + return 0444; > + default: > + return 0; > + } > +} > + > +static umode_t axi_fan_control_is_visible(const void *data, > + enum hwmon_sensor_types type, > + u32 attr, int channel) > +{ > + switch (type) { > + case hwmon_fan: > + return axi_fan_control_fan_is_visible(attr); > + case hwmon_pwm: > + return axi_fan_control_pwm_is_visible(attr); > + case hwmon_temp: > + return axi_fan_control_temp_is_visible(attr); > + default: > + return 0; > + } > +} > + > +static irqreturn_t axi_fan_control_irq_handler(int irq, void *data) > +{ > + struct axi_fan_control_data *ctl = (struct axi_fan_control_data *)data; > + u32 irq_pending = axi_fan_control_ioread(ADI_REG_IRQ_PENDING, ctl); > + u32 clear_mask; > + > + if (irq_pending & ADI_IRQ_SRC_NEW_MEASUR) { > + u32 new_tach = axi_fan_control_ioread(ADI_REG_TACH_MEASUR, > + ctl); > + > + if (ctl->update_tacho_params) { > + /* get 25% tolerance */ > + u32 tach_tol = DIV_ROUND_CLOSEST(new_tach * 25, 100); > + /* set new tacho parameters */ > + axi_fan_control_iowrite(new_tach, ADI_REG_TACH_PERIOD, > + ctl); > + axi_fan_control_iowrite(tach_tol, > + ADI_REG_TACH_TOLERANCE, ctl); > + ctl->update_tacho_params = false; > + } > + > + atomic_set(&ctl->tach, new_tach); > + } > + > + if (irq_pending & ADI_IRQ_SRC_PWM_CHANGED) { > + /* > + * if the pwm changes on behalf of software, > + * we need to provide new tacho parameters to the core. > + * Wait for the next measurement for that... > + */ > + if (!ctl->hw_pwm_req) > + ctl->update_tacho_params = true; > + > + /* just set this to false even if it is already... */ > + ctl->hw_pwm_req = false; > + } > + > + 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 ? 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. > + if (irq_pending & ADI_IRQ_SRC_TACH_ERR) > + ctl->fan_fault = 1; Is it on purpose that this bit is never reset ? > + > + /* 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 0xffffffff ? > + /* > + * Enable all IRQs > + */ > + axi_fan_control_iowrite((ADI_IRQ_MASK_OUT_ALL & > + ~(ADI_IRQ_SRC_NEW_MEASUR | ADI_IRQ_SRC_TACH_ERR | > + ADI_IRQ_SRC_PWM_CHANGED | ADI_IRQ_SRC_TEMP_INCREASE)), One set of ( ) is unnecessary. > + ADI_REG_IRQ_MASK, ctl); > + > + /* bring the device out of reset */ > + axi_fan_control_iowrite(0x01, ADI_REG_RSTN, ctl); > + > + return ret; > +} > + > +static const struct hwmon_channel_info *axi_fan_control_info[] = { > + HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT), > + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_LABEL), > + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL), > + NULL > +}; > + > +static const struct hwmon_ops axi_fan_control_hwmon_ops = { > + .is_visible = axi_fan_control_is_visible, > + .read = axi_fan_control_read, > + .write = axi_fan_control_write, > + .read_string = axi_fan_control_read_labels, > +}; > + > +static const struct hwmon_chip_info axi_fan_control_chip_info = { > + .ops = &axi_fan_control_hwmon_ops, > + .info = axi_fan_control_info, > +}; > + > +static const u32 version_1_0_0 = ADI_AXI_PCORE_VER(1, 0, 'a'); > + > +static const struct of_device_id axi_fan_control_of_match[] = { > + { .compatible = "adi,axi-fan-control-1.00.a", > + .data = (void *)&version_1_0_0}, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, axi_fan_control_of_match); > + > +static int axi_fan_control_probe(struct platform_device *pdev) > +{ > + struct device *hwmon_dev; > + struct axi_fan_control_data *ctl; > + const struct of_device_id *id; > + struct resource *res; > + u32 version; > + int ret; > + > + id = of_match_node(axi_fan_control_of_match, pdev->dev.of_node); > + if (!id) > + return -EINVAL; > + > + ctl = devm_kzalloc(&pdev->dev, sizeof(*ctl), GFP_KERNEL); > + if (!ctl) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + ctl->base = devm_ioremap_resource(&pdev->dev, res); devm_platform_ioremap_resource() > + if (IS_ERR(ctl->base)) { > + dev_err(&pdev->dev, "ioremap failed with %ld\n", > + PTR_ERR(ctl->base)); > + return PTR_ERR(ctl->base); > + } This does not require an extra error message. > + > + ctl->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(ctl->clk)) { > + dev_err(&pdev->dev, "clk_get failed with %ld\n", > + PTR_ERR(ctl->clk)); > + return PTR_ERR(ctl->clk); > + } > + > + dev_dbg(&pdev->dev, "Re-mapped from 0x%08llX to %p\n", > + (unsigned long long)res->start, ctl->base); > + > + version = axi_fan_control_ioread(ADI_AXI_REG_VERSION, ctl); > + if (ADI_AXI_PCORE_VER_MAJOR(version) != > + ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data))) { > + dev_err(&pdev->dev, "Major version mismatch. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n", > + ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data)), > + ADI_AXI_PCORE_VER_MINOR((*(u32 *)id->data)), > + ADI_AXI_PCORE_VER_PATCH((*(u32 *)id->data)), > + ADI_AXI_PCORE_VER_MAJOR(version), > + ADI_AXI_PCORE_VER_MINOR(version), > + ADI_AXI_PCORE_VER_PATCH(version)); > + return -ENODEV; > + } > + > + 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. > + dev_err(&pdev->dev, "platform_get_irq failed with %d\n", > + ctl->irq); > + return ctl->irq; > + } > + > + ret = devm_request_threaded_irq(&pdev->dev, ctl->irq, NULL, > + axi_fan_control_irq_handler, > + IRQF_ONESHOT | IRQF_TRIGGER_HIGH, > + pdev->driver_override, ctl); > + if (ret) { > + dev_err(&pdev->dev, "failed to request an irq, %d", ret); > + return ret; > + } > + > + ret = axi_fan_control_init(ctl, pdev->dev.of_node); > + if (ret) { > + dev_err(&pdev->dev, "Failed to initialize device\n"); > + return ret; > + } > + > + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, > + "axi_fan_control", > + ctl, > + &axi_fan_control_chip_info, > + NULL); Another alignment issue. Shorter function and global variable names would help. > + > + return PTR_ERR_OR_ZERO(hwmon_dev); > +} > + > +static struct platform_driver axi_fan_control_driver = { > + .driver = { > + .name = "axi_fan_control_driver", > + .of_match_table = axi_fan_control_of_match, > + }, > + .probe = axi_fan_control_probe, > +}; > +module_platform_driver(axi_fan_control_driver); > + > +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>"); > +MODULE_DESCRIPTION("Analog Devices Fan Control HDL CORE driver"); > +MODULE_LICENSE("GPL"); > -- > 2.23.0 >
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á <nuno.sa@analog.com> > > --- > > 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á <nuno.sa@analog.com> > > +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 <krzysztof.adamski@nokia.com> > > L: linux-i2c@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 000000000000..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 <linux/clk.h> > > +#include <linux/fpga/adi-axi-common.h> > > +#include <linux/hwmon.h> > > +#include <linux/interrupt.h> > > +#include <linux/io.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > + > > +/* register map */ > > +#define ADI_REG_RSTN 0x0080 > > +#define ADI_REG_PWM_WIDTH 0x0084 > > +#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 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 0xFFFFFFFFU > > + > > +#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_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. Will do that. > > +{ > > + 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); > > 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. > > +} > > + > > +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 = clamp_val(val, 0, SYSFS_PWM_MAX); > > + > > + new_width = DIV_ROUND_CLOSEST(__val * pwm_period, > > SYSFS_PWM_MAX); > > + > > + axi_fan_control_iowrite(new_width, ADI_REG_PWM_WIDTH, ctl); > > + > > + return 0; > > +} > > + > > +static long axi_fan_control_get_fan_rpm(const struct > > axi_fan_control_data *ctl) > > +{ > > + unsigned long clk_rate = clk_get_rate(ctl->clk); > > + > > + if (!clk_rate) > > + return -EINVAL; > > Unless the clock rate changes dynamically, it might make sense to > read it once > in the probe function. Hmm, It can't so I will do as you suggest. > > + /* > > + * The tacho period should be: > > + * TACH = 60/(ppr * rpm), where rpm is revolutions per > > second > > + * and ppr is pulses per revolution. > > + * Given the tacho period, we can multiply it by the input > > clock > > + * so that we know how many clocks we need to have this period. > > + * From this, we can derive the RPM value. > > + */ > > + return DIV_ROUND_CLOSEST(60 * clk_rate, > > + ctl->ppr * atomic_read(&ctl->tach)); > > Are ppr and tach guaranteed to be != 0 ? I don't think so, > since neither is ever evaluated. Nope. I should make sure ppr is never 0 and I shoud evaluate tach before doing the operation and return 0 > > +} > > + > > +static int axi_fan_control_read_temp(struct device *dev, u32 attr, > > long *val) > > +{ > > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > > + long raw_temp; > > + > > + switch (attr) { > > + case hwmon_temp_input: > > + raw_temp = axi_fan_control_ioread(ADI_REG_TEMPERATURE, > > ctl); > > + /* > > + * The formula for the temperature is: > > + * T = (ADC * 501.3743 / 2^bits) - 273.6777 > > + * It's multiplied by 1000 to have milidegrees as > > s/milidegrees/millidegrees/ ack. > > + * specified by the hwmon sysfs interface. > > + */ > > + *val = ((raw_temp * 501374) >> 16) - 273677; > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_read_fan(struct device *dev, u32 attr, > > long *val) > > +{ > > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_fan_fault: > > + *val = ctl->fan_fault; > > + return 0; > > + case hwmon_fan_input: > > + *val = axi_fan_control_get_fan_rpm(ctl); > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_read_pwm(struct device *dev, u32 attr, > > long *val) > > +{ > > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_pwm_input: > > + *val = axi_fan_control_get_pwm_duty(ctl); > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_write_pwm(struct device *dev, u32 attr, > > long val) > > +{ > > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_pwm_input: > > + return axi_fan_control_set_pwm_duty(val, ctl); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_read_labels(struct device *dev, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel, const > > char **str) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + *str = "FAN"; > > + return 0; > > + case hwmon_temp: > > + *str = "SYSMON4"; > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_read(struct device *dev, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel, long *val) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + return axi_fan_control_read_fan(dev, attr, val); > > + case hwmon_pwm: > > + return axi_fan_control_read_pwm(dev, attr, val); > > + case hwmon_temp: > > + return axi_fan_control_read_temp(dev, attr, val); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_write(struct device *dev, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel, long val) > > +{ > > + switch (type) { > > + case hwmon_pwm: > > + return axi_fan_control_write_pwm(dev, attr, val); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static umode_t axi_fan_control_fan_is_visible(const u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_fan_input: > > + case hwmon_fan_fault: > > + case hwmon_fan_label: > > + return 0444; > > + default: > > + return 0; > > + } > > +} > > + > > +static umode_t axi_fan_control_pwm_is_visible(const u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_pwm_input: > > + return 0644; > > + default: > > + return 0; > > + } > > +} > > + > > +static umode_t axi_fan_control_temp_is_visible(const u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_temp_input: > > + case hwmon_temp_label: > > + return 0444; > > + default: > > + return 0; > > + } > > +} > > + > > +static umode_t axi_fan_control_is_visible(const void *data, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + return axi_fan_control_fan_is_visible(attr); > > + case hwmon_pwm: > > + return axi_fan_control_pwm_is_visible(attr); > > + case hwmon_temp: > > + return axi_fan_control_temp_is_visible(attr); > > + default: > > + return 0; > > + } > > +} > > + > > +static irqreturn_t axi_fan_control_irq_handler(int irq, void > > *data) > > +{ > > + struct axi_fan_control_data *ctl = (struct axi_fan_control_data > > *)data; > > + u32 irq_pending = axi_fan_control_ioread(ADI_REG_IRQ_PENDING, > > ctl); > > + u32 clear_mask; > > + > > + if (irq_pending & ADI_IRQ_SRC_NEW_MEASUR) { > > + u32 new_tach = > > axi_fan_control_ioread(ADI_REG_TACH_MEASUR, > > + ctl); > > + > > + if (ctl->update_tacho_params) { > > + /* get 25% tolerance */ > > + u32 tach_tol = DIV_ROUND_CLOSEST(new_tach * 25, > > 100); > > + /* set new tacho parameters */ > > + axi_fan_control_iowrite(new_tach, > > ADI_REG_TACH_PERIOD, > > + ctl); > > + axi_fan_control_iowrite(tach_tol, > > + ADI_REG_TACH_TOLERANCE, > > ctl); > > + ctl->update_tacho_params = false; > > + } > > + > > + atomic_set(&ctl->tach, new_tach); > > + } > > + > > + if (irq_pending & ADI_IRQ_SRC_PWM_CHANGED) { > > + /* > > + * if the pwm changes on behalf of software, > > + * we need to provide new tacho parameters to the core. > > + * Wait for the next measurement for that... > > + */ > > + if (!ctl->hw_pwm_req) > > + ctl->update_tacho_params = true; > > + > > + /* just set this to false even if it is already... */ > > + ctl->hw_pwm_req = false; > > + } > > + > > + 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? > 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. > > ) > > + 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? > > > + > > + /* 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 0xffffffff ? 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. > > + /* > > + * Enable all IRQs > > + */ > > + axi_fan_control_iowrite((ADI_IRQ_MASK_OUT_ALL & > > + ~(ADI_IRQ_SRC_NEW_MEASUR | ADI_IRQ_SRC_TACH_ERR > > | > > + ADI_IRQ_SRC_PWM_CHANGED | > > ADI_IRQ_SRC_TEMP_INCREASE)), > > One set of ( ) is unnecessary. ack. > > + ADI_REG_IRQ_MASK, ctl); > > + > > + /* bring the device out of reset */ > > + axi_fan_control_iowrite(0x01, ADI_REG_RSTN, ctl); > > + > > + return ret; > > +} > > + > > +static const struct hwmon_channel_info *axi_fan_control_info[] = { > > + HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT), > > + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_FAULT | > > HWMON_F_LABEL), > > + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL), > > + NULL > > +}; > > + > > +static const struct hwmon_ops axi_fan_control_hwmon_ops = { > > + .is_visible = axi_fan_control_is_visible, > > + .read = axi_fan_control_read, > > + .write = axi_fan_control_write, > > + .read_string = axi_fan_control_read_labels, > > +}; > > + > > +static const struct hwmon_chip_info axi_fan_control_chip_info = { > > + .ops = &axi_fan_control_hwmon_ops, > > + .info = axi_fan_control_info, > > +}; > > + > > +static const u32 version_1_0_0 = ADI_AXI_PCORE_VER(1, 0, 'a'); > > + > > +static const struct of_device_id axi_fan_control_of_match[] = { > > + { .compatible = "adi,axi-fan-control-1.00.a", > > + .data = (void *)&version_1_0_0}, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, axi_fan_control_of_match); > > + > > +static int axi_fan_control_probe(struct platform_device *pdev) > > +{ > > + struct device *hwmon_dev; > > + struct axi_fan_control_data *ctl; > > + const struct of_device_id *id; > > + struct resource *res; > > + u32 version; > > + int ret; > > + > > + id = of_match_node(axi_fan_control_of_match, pdev- > > >dev.of_node); > > + if (!id) > > + return -EINVAL; > > + > > + ctl = devm_kzalloc(&pdev->dev, sizeof(*ctl), GFP_KERNEL); > > + if (!ctl) > > + return -ENOMEM; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + ctl->base = devm_ioremap_resource(&pdev->dev, res); > > devm_platform_ioremap_resource() ack. > > + if (IS_ERR(ctl->base)) { > > + dev_err(&pdev->dev, "ioremap failed with %ld\n", > > + PTR_ERR(ctl->base)); > > + return PTR_ERR(ctl->base); > > + } > > This does not require an extra error message. ack. > > + > > + ctl->clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(ctl->clk)) { > > + dev_err(&pdev->dev, "clk_get failed with %ld\n", > > + PTR_ERR(ctl->clk)); > > + return PTR_ERR(ctl->clk); > > + } > > + > > + dev_dbg(&pdev->dev, "Re-mapped from 0x%08llX to %p\n", > > + (unsigned long long)res->start, ctl->base); > > + > > + version = axi_fan_control_ioread(ADI_AXI_REG_VERSION, ctl); > > + if (ADI_AXI_PCORE_VER_MAJOR(version) != > > + ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data))) { > > + dev_err(&pdev->dev, "Major version mismatch. Expected > > %d.%.2d.%c, Reported %d.%.2d.%c\n", > > + ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data)), > > + ADI_AXI_PCORE_VER_MINOR((*(u32 *)id->data)), > > + ADI_AXI_PCORE_VER_PATCH((*(u32 *)id->data)), > > + ADI_AXI_PCORE_VER_MAJOR(version), > > + ADI_AXI_PCORE_VER_MINOR(version), > > + ADI_AXI_PCORE_VER_PATCH(version)); > > + return -ENODEV; > > + } > > + > > + 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. ack. > > + dev_err(&pdev->dev, "platform_get_irq failed with > > %d\n", > > + ctl->irq); > > + return ctl->irq; > > + } > > + > > + ret = devm_request_threaded_irq(&pdev->dev, ctl->irq, NULL, > > + axi_fan_control_irq_handler, > > + IRQF_ONESHOT | > > IRQF_TRIGGER_HIGH, > > + pdev->driver_override, ctl); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to request an irq, %d", > > ret); > > + return ret; > > + } > > + > > + ret = axi_fan_control_init(ctl, pdev->dev.of_node); > > + if (ret) { > > + dev_err(&pdev->dev, "Failed to initialize device\n"); > > + return ret; > > + } > > + > > + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, > > + "axi_fan_control", > > + ctl, > > + &axi_fan_control_chip_i > > nfo, > > + NULL); > > Another alignment issue. Shorter function and global variable names > would help. ack. > > + > > + return PTR_ERR_OR_ZERO(hwmon_dev); > > +} > > + > > +static struct platform_driver axi_fan_control_driver = { > > + .driver = { > > + .name = "axi_fan_control_driver", > > + .of_match_table = axi_fan_control_of_match, > > + }, > > + .probe = axi_fan_control_probe, > > +}; > > +module_platform_driver(axi_fan_control_driver); > > + > > +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>"); > > +MODULE_DESCRIPTION("Analog Devices Fan Control HDL CORE driver"); > > +MODULE_LICENSE("GPL"); > > -- > > 2.23.0 > >
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 0xffffffff ? > > 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
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 0xffffffff ? > > > > 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 accordingly. > Guenter
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á <nuno.sa@analog.com> > > --- > > 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á <nuno.sa@analog.com> > > +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 <krzysztof.adamski@nokia.com> > > L: linux-i2c@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 000000000000..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 <linux/clk.h> > > +#include <linux/fpga/adi-axi-common.h> > > +#include <linux/hwmon.h> > > +#include <linux/interrupt.h> > > +#include <linux/io.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > + > > +/* register map */ > > +#define ADI_REG_RSTN 0x0080 > > +#define ADI_REG_PWM_WIDTH 0x0084 > > +#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_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 0xFFFFFFFFU > > + > > +#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_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_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); > > Is pwm_period guaranteed to be != 0 ? > > > +} > > + > > +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 = clamp_val(val, 0, SYSFS_PWM_MAX); > > + > > + new_width = DIV_ROUND_CLOSEST(__val * pwm_period, > > SYSFS_PWM_MAX); > > + > > + axi_fan_control_iowrite(new_width, ADI_REG_PWM_WIDTH, ctl); > > + > > + return 0; > > +} > > + > > +static long axi_fan_control_get_fan_rpm(const struct > > axi_fan_control_data *ctl) > > +{ > > + unsigned long clk_rate = clk_get_rate(ctl->clk); > > + > > + if (!clk_rate) > > + return -EINVAL; > > Unless the clock rate changes dynamically, it might make sense to > read it once > in the probe function. > > > + /* > > + * The tacho period should be: > > + * TACH = 60/(ppr * rpm), where rpm is revolutions per > > second > > + * and ppr is pulses per revolution. > > + * Given the tacho period, we can multiply it by the input > > clock > > + * so that we know how many clocks we need to have this period. > > + * From this, we can derive the RPM value. > > + */ > > + return DIV_ROUND_CLOSEST(60 * clk_rate, > > + ctl->ppr * atomic_read(&ctl->tach)); > > Are ppr and tach guaranteed to be != 0 ? I don't think so, > since neither is ever evaluated. > > > +} > > + > > +static int axi_fan_control_read_temp(struct device *dev, u32 attr, > > long *val) > > +{ > > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > > + long raw_temp; > > + > > + switch (attr) { > > + case hwmon_temp_input: > > + raw_temp = axi_fan_control_ioread(ADI_REG_TEMPERATURE, > > ctl); > > + /* > > + * The formula for the temperature is: > > + * T = (ADC * 501.3743 / 2^bits) - 273.6777 > > + * It's multiplied by 1000 to have milidegrees as > > s/milidegrees/millidegrees/ > > > + * specified by the hwmon sysfs interface. > > + */ > > + *val = ((raw_temp * 501374) >> 16) - 273677; > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_read_fan(struct device *dev, u32 attr, > > long *val) > > +{ > > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_fan_fault: > > + *val = ctl->fan_fault; > > + return 0; > > + case hwmon_fan_input: > > + *val = axi_fan_control_get_fan_rpm(ctl); > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_read_pwm(struct device *dev, u32 attr, > > long *val) > > +{ > > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_pwm_input: > > + *val = axi_fan_control_get_pwm_duty(ctl); > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_write_pwm(struct device *dev, u32 attr, > > long val) > > +{ > > + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); > > + > > + switch (attr) { > > + case hwmon_pwm_input: > > + return axi_fan_control_set_pwm_duty(val, ctl); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_read_labels(struct device *dev, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel, const > > char **str) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + *str = "FAN"; > > + return 0; > > + case hwmon_temp: > > + *str = "SYSMON4"; > > + return 0; > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_read(struct device *dev, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel, long *val) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + return axi_fan_control_read_fan(dev, attr, val); > > + case hwmon_pwm: > > + return axi_fan_control_read_pwm(dev, attr, val); > > + case hwmon_temp: > > + return axi_fan_control_read_temp(dev, attr, val); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static int axi_fan_control_write(struct device *dev, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel, long val) > > +{ > > + switch (type) { > > + case hwmon_pwm: > > + return axi_fan_control_write_pwm(dev, attr, val); > > + default: > > + return -ENOTSUPP; > > + } > > +} > > + > > +static umode_t axi_fan_control_fan_is_visible(const u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_fan_input: > > + case hwmon_fan_fault: > > + case hwmon_fan_label: > > + return 0444; > > + default: > > + return 0; > > + } > > +} > > + > > +static umode_t axi_fan_control_pwm_is_visible(const u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_pwm_input: > > + return 0644; > > + default: > > + return 0; > > + } > > +} > > + > > +static umode_t axi_fan_control_temp_is_visible(const u32 attr) > > +{ > > + switch (attr) { > > + case hwmon_temp_input: > > + case hwmon_temp_label: > > + return 0444; > > + default: > > + return 0; > > + } > > +} > > + > > +static umode_t axi_fan_control_is_visible(const void *data, > > + enum hwmon_sensor_types type, > > + u32 attr, int channel) > > +{ > > + switch (type) { > > + case hwmon_fan: > > + return axi_fan_control_fan_is_visible(attr); > > + case hwmon_pwm: > > + return axi_fan_control_pwm_is_visible(attr); > > + case hwmon_temp: > > + return axi_fan_control_temp_is_visible(attr); > > + default: > > + return 0; > > + } > > +} > > + > > +static irqreturn_t axi_fan_control_irq_handler(int irq, void > > *data) > > +{ > > + struct axi_fan_control_data *ctl = (struct axi_fan_control_data > > *)data; > > + u32 irq_pending = axi_fan_control_ioread(ADI_REG_IRQ_PENDING, > > ctl); > > + u32 clear_mask; > > + > > + if (irq_pending & ADI_IRQ_SRC_NEW_MEASUR) { > > + u32 new_tach = > > axi_fan_control_ioread(ADI_REG_TACH_MEASUR, > > + ctl); > > + > > + if (ctl->update_tacho_params) { > > + /* get 25% tolerance */ > > + u32 tach_tol = DIV_ROUND_CLOSEST(new_tach * 25, > > 100); > > + /* set new tacho parameters */ > > + axi_fan_control_iowrite(new_tach, > > ADI_REG_TACH_PERIOD, > > + ctl); > > + axi_fan_control_iowrite(tach_tol, > > + ADI_REG_TACH_TOLERANCE, > > ctl); > > + ctl->update_tacho_params = false; > > + } > > + > > + atomic_set(&ctl->tach, new_tach); > > + } > > + > > + if (irq_pending & ADI_IRQ_SRC_PWM_CHANGED) { > > + /* > > + * if the pwm changes on behalf of software, > > + * we need to provide new tacho parameters to the core. > > + * Wait for the next measurement for that... > > + */ > > + if (!ctl->hw_pwm_req) > > + ctl->update_tacho_params = true; > > + > > + /* just set this to false even if it is already... */ > > + ctl->hw_pwm_req = false; > > + } > > + > > + 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 ? > 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. > > > + if (irq_pending & ADI_IRQ_SRC_TACH_ERR) > > + ctl->fan_fault = 1; > > Is it on purpose that this bit is never reset ? > > > + > > + /* 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 0xffffffff ? > > > + /* > > + * Enable all IRQs > > + */ > > + axi_fan_control_iowrite((ADI_IRQ_MASK_OUT_ALL & > > + ~(ADI_IRQ_SRC_NEW_MEASUR | ADI_IRQ_SRC_TACH_ERR > > | > > + ADI_IRQ_SRC_PWM_CHANGED | > > ADI_IRQ_SRC_TEMP_INCREASE)), > > One set of ( ) is unnecessary. > > > + ADI_REG_IRQ_MASK, ctl); > > + > > + /* bring the device out of reset */ > > + axi_fan_control_iowrite(0x01, ADI_REG_RSTN, ctl); > > + > > + return ret; > > +} > > + > > +static const struct hwmon_channel_info *axi_fan_control_info[] = { > > + HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT), > > + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_FAULT | > > HWMON_F_LABEL), > > + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL), > > + NULL > > +}; > > + > > +static const struct hwmon_ops axi_fan_control_hwmon_ops = { > > + .is_visible = axi_fan_control_is_visible, > > + .read = axi_fan_control_read, > > + .write = axi_fan_control_write, > > + .read_string = axi_fan_control_read_labels, > > +}; > > + > > +static const struct hwmon_chip_info axi_fan_control_chip_info = { > > + .ops = &axi_fan_control_hwmon_ops, > > + .info = axi_fan_control_info, > > +}; > > + > > +static const u32 version_1_0_0 = ADI_AXI_PCORE_VER(1, 0, 'a'); > > + > > +static const struct of_device_id axi_fan_control_of_match[] = { > > + { .compatible = "adi,axi-fan-control-1.00.a", > > + .data = (void *)&version_1_0_0}, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, axi_fan_control_of_match); > > + > > +static int axi_fan_control_probe(struct platform_device *pdev) > > +{ > > + struct device *hwmon_dev; > > + struct axi_fan_control_data *ctl; > > + const struct of_device_id *id; > > + struct resource *res; > > + u32 version; > > + int ret; > > + > > + id = of_match_node(axi_fan_control_of_match, pdev- > > >dev.of_node); > > + if (!id) > > + return -EINVAL; > > + > > + ctl = devm_kzalloc(&pdev->dev, sizeof(*ctl), GFP_KERNEL); > > + if (!ctl) > > + return -ENOMEM; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + ctl->base = devm_ioremap_resource(&pdev->dev, res); > > devm_platform_ioremap_resource() > > > + if (IS_ERR(ctl->base)) { > > + dev_err(&pdev->dev, "ioremap failed with %ld\n", > > + PTR_ERR(ctl->base)); > > + return PTR_ERR(ctl->base); > > + } > > This does not require an extra error message. > > > + > > + ctl->clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(ctl->clk)) { > > + dev_err(&pdev->dev, "clk_get failed with %ld\n", > > + PTR_ERR(ctl->clk)); > > + return PTR_ERR(ctl->clk); > > + } > > + > > + dev_dbg(&pdev->dev, "Re-mapped from 0x%08llX to %p\n", > > + (unsigned long long)res->start, ctl->base); > > + > > + version = axi_fan_control_ioread(ADI_AXI_REG_VERSION, ctl); > > + if (ADI_AXI_PCORE_VER_MAJOR(version) != > > + ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data))) { > > + dev_err(&pdev->dev, "Major version mismatch. Expected > > %d.%.2d.%c, Reported %d.%.2d.%c\n", > > + ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data)), > > + ADI_AXI_PCORE_VER_MINOR((*(u32 *)id->data)), > > + ADI_AXI_PCORE_VER_PATCH((*(u32 *)id->data)), > > + ADI_AXI_PCORE_VER_MAJOR(version), > > + ADI_AXI_PCORE_VER_MINOR(version), > > + ADI_AXI_PCORE_VER_PATCH(version)); > > + return -ENODEV; > > + } > > + > > + 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... > > + dev_err(&pdev->dev, "platform_get_irq failed with > > %d\n", > > + ctl->irq); > > + return ctl->irq; > > + } > > + > > + ret = devm_request_threaded_irq(&pdev->dev, ctl->irq, NULL, > > + axi_fan_control_irq_handler, > > + IRQF_ONESHOT | > > IRQF_TRIGGER_HIGH, > > + pdev->driver_override, ctl); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to request an irq, %d", > > ret); > > + return ret; > > + } > > + > > + ret = axi_fan_control_init(ctl, pdev->dev.of_node); > > + if (ret) { > > + dev_err(&pdev->dev, "Failed to initialize device\n"); > > + return ret; > > + } > > + > > + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, > > + "axi_fan_control", > > + ctl, > > + &axi_fan_control_chip_i > > nfo, > > + NULL); > > Another alignment issue. Shorter function and global variable names > would help. > > > + > > + return PTR_ERR_OR_ZERO(hwmon_dev); > > +} > > + > > +static struct platform_driver axi_fan_control_driver = { > > + .driver = { > > + .name = "axi_fan_control_driver", > > + .of_match_table = axi_fan_control_of_match, > > + }, > > + .probe = axi_fan_control_probe, > > +}; > > +module_platform_driver(axi_fan_control_driver); > > + > > +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>"); > > +MODULE_DESCRIPTION("Analog Devices Fan Control HDL CORE driver"); > > +MODULE_LICENSE("GPL"); > > -- > > 2.23.0 > >
On Tue, Oct 08, 2019 at 03:59:49PM +0000, 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
On Tue, 2019-10-08 at 13:11 -0700, Guenter Roeck wrote: > > On Tue, Oct 08, 2019 at 03:59:49PM +0000, 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
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á <nuno.sa@analog.com> +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 <krzysztof.adamski@nokia.com> L: linux-i2c@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 000000000000..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 <linux/clk.h> +#include <linux/fpga/adi-axi-common.h> +#include <linux/hwmon.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> + +/* register map */ +#define ADI_REG_RSTN 0x0080 +#define ADI_REG_PWM_WIDTH 0x0084 +#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) +#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 0xFFFFFFFFU + +#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 = clamp_val(val, 0, SYSFS_PWM_MAX); + + new_width = DIV_ROUND_CLOSEST(__val * pwm_period, SYSFS_PWM_MAX); + + axi_fan_control_iowrite(new_width, ADI_REG_PWM_WIDTH, ctl); + + return 0; +} + +static long axi_fan_control_get_fan_rpm(const struct axi_fan_control_data *ctl) +{ + unsigned long clk_rate = clk_get_rate(ctl->clk); + + if (!clk_rate) + return -EINVAL; + /* + * The tacho period should be: + * TACH = 60/(ppr * rpm), where rpm is revolutions per second + * and ppr is pulses per revolution. + * Given the tacho period, we can multiply it by the input clock + * so that we know how many clocks we need to have this period. + * From this, we can derive the RPM value. + */ + return DIV_ROUND_CLOSEST(60 * clk_rate, + ctl->ppr * atomic_read(&ctl->tach)); +} + +static int axi_fan_control_read_temp(struct device *dev, u32 attr, long *val) +{ + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); + long raw_temp; + + switch (attr) { + case hwmon_temp_input: + raw_temp = axi_fan_control_ioread(ADI_REG_TEMPERATURE, ctl); + /* + * The formula for the temperature is: + * T = (ADC * 501.3743 / 2^bits) - 273.6777 + * It's multiplied by 1000 to have milidegrees as + * specified by the hwmon sysfs interface. + */ + *val = ((raw_temp * 501374) >> 16) - 273677; + return 0; + default: + return -ENOTSUPP; + } +} + +static int axi_fan_control_read_fan(struct device *dev, u32 attr, long *val) +{ + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); + + switch (attr) { + case hwmon_fan_fault: + *val = ctl->fan_fault; + return 0; + case hwmon_fan_input: + *val = axi_fan_control_get_fan_rpm(ctl); + return 0; + default: + return -ENOTSUPP; + } +} + +static int axi_fan_control_read_pwm(struct device *dev, u32 attr, long *val) +{ + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); + + switch (attr) { + case hwmon_pwm_input: + *val = axi_fan_control_get_pwm_duty(ctl); + return 0; + default: + return -ENOTSUPP; + } +} + +static int axi_fan_control_write_pwm(struct device *dev, u32 attr, long val) +{ + struct axi_fan_control_data *ctl = dev_get_drvdata(dev); + + switch (attr) { + case hwmon_pwm_input: + return axi_fan_control_set_pwm_duty(val, ctl); + default: + return -ENOTSUPP; + } +} + +static int axi_fan_control_read_labels(struct device *dev, + enum hwmon_sensor_types type, + u32 attr, int channel, const char **str) +{ + switch (type) { + case hwmon_fan: + *str = "FAN"; + return 0; + case hwmon_temp: + *str = "SYSMON4"; + return 0; + default: + return -ENOTSUPP; + } +} + +static int axi_fan_control_read(struct device *dev, + enum hwmon_sensor_types type, + u32 attr, int channel, long *val) +{ + switch (type) { + case hwmon_fan: + return axi_fan_control_read_fan(dev, attr, val); + case hwmon_pwm: + return axi_fan_control_read_pwm(dev, attr, val); + case hwmon_temp: + return axi_fan_control_read_temp(dev, attr, val); + default: + return -ENOTSUPP; + } +} + +static int axi_fan_control_write(struct device *dev, + enum hwmon_sensor_types type, + u32 attr, int channel, long val) +{ + switch (type) { + case hwmon_pwm: + return axi_fan_control_write_pwm(dev, attr, val); + default: + return -ENOTSUPP; + } +} + +static umode_t axi_fan_control_fan_is_visible(const u32 attr) +{ + switch (attr) { + case hwmon_fan_input: + case hwmon_fan_fault: + case hwmon_fan_label: + return 0444; + default: + return 0; + } +} + +static umode_t axi_fan_control_pwm_is_visible(const u32 attr) +{ + switch (attr) { + case hwmon_pwm_input: + return 0644; + default: + return 0; + } +} + +static umode_t axi_fan_control_temp_is_visible(const u32 attr) +{ + switch (attr) { + case hwmon_temp_input: + case hwmon_temp_label: + return 0444; + default: + return 0; + } +} + +static umode_t axi_fan_control_is_visible(const void *data, + enum hwmon_sensor_types type, + u32 attr, int channel) +{ + switch (type) { + case hwmon_fan: + return axi_fan_control_fan_is_visible(attr); + case hwmon_pwm: + return axi_fan_control_pwm_is_visible(attr); + case hwmon_temp: + return axi_fan_control_temp_is_visible(attr); + default: + return 0; + } +} + +static irqreturn_t axi_fan_control_irq_handler(int irq, void *data) +{ + struct axi_fan_control_data *ctl = (struct axi_fan_control_data *)data; + u32 irq_pending = axi_fan_control_ioread(ADI_REG_IRQ_PENDING, ctl); + u32 clear_mask; + + if (irq_pending & ADI_IRQ_SRC_NEW_MEASUR) { + u32 new_tach = axi_fan_control_ioread(ADI_REG_TACH_MEASUR, + ctl); + + if (ctl->update_tacho_params) { + /* get 25% tolerance */ + u32 tach_tol = DIV_ROUND_CLOSEST(new_tach * 25, 100); + /* set new tacho parameters */ + axi_fan_control_iowrite(new_tach, ADI_REG_TACH_PERIOD, + ctl); + axi_fan_control_iowrite(tach_tol, + ADI_REG_TACH_TOLERANCE, ctl); + ctl->update_tacho_params = false; + } + + atomic_set(&ctl->tach, new_tach); + } + + if (irq_pending & ADI_IRQ_SRC_PWM_CHANGED) { + /* + * if the pwm changes on behalf of software, + * we need to provide new tacho parameters to the core. + * Wait for the next measurement for that... + */ + if (!ctl->hw_pwm_req) + ctl->update_tacho_params = true; + + /* just set this to false even if it is already... */ + ctl->hw_pwm_req = false; + } + + if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE) + /* hardware requested a new pwm */ + ctl->hw_pwm_req = true; + + if (irq_pending & ADI_IRQ_SRC_TACH_ERR) + ctl->fan_fault = 1; + + /* 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; + /* + * Enable all IRQs + */ + axi_fan_control_iowrite((ADI_IRQ_MASK_OUT_ALL & + ~(ADI_IRQ_SRC_NEW_MEASUR | ADI_IRQ_SRC_TACH_ERR | + ADI_IRQ_SRC_PWM_CHANGED | ADI_IRQ_SRC_TEMP_INCREASE)), + ADI_REG_IRQ_MASK, ctl); + + /* bring the device out of reset */ + axi_fan_control_iowrite(0x01, ADI_REG_RSTN, ctl); + + return ret; +} + +static const struct hwmon_channel_info *axi_fan_control_info[] = { + HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT), + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_LABEL), + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL), + NULL +}; + +static const struct hwmon_ops axi_fan_control_hwmon_ops = { + .is_visible = axi_fan_control_is_visible, + .read = axi_fan_control_read, + .write = axi_fan_control_write, + .read_string = axi_fan_control_read_labels, +}; + +static const struct hwmon_chip_info axi_fan_control_chip_info = { + .ops = &axi_fan_control_hwmon_ops, + .info = axi_fan_control_info, +}; + +static const u32 version_1_0_0 = ADI_AXI_PCORE_VER(1, 0, 'a'); + +static const struct of_device_id axi_fan_control_of_match[] = { + { .compatible = "adi,axi-fan-control-1.00.a", + .data = (void *)&version_1_0_0}, + {}, +}; +MODULE_DEVICE_TABLE(of, axi_fan_control_of_match); + +static int axi_fan_control_probe(struct platform_device *pdev) +{ + struct device *hwmon_dev; + struct axi_fan_control_data *ctl; + const struct of_device_id *id; + struct resource *res; + u32 version; + int ret; + + id = of_match_node(axi_fan_control_of_match, pdev->dev.of_node); + if (!id) + return -EINVAL; + + ctl = devm_kzalloc(&pdev->dev, sizeof(*ctl), GFP_KERNEL); + if (!ctl) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + ctl->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(ctl->base)) { + dev_err(&pdev->dev, "ioremap failed with %ld\n", + PTR_ERR(ctl->base)); + return PTR_ERR(ctl->base); + } + + ctl->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(ctl->clk)) { + dev_err(&pdev->dev, "clk_get failed with %ld\n", + PTR_ERR(ctl->clk)); + return PTR_ERR(ctl->clk); + } + + dev_dbg(&pdev->dev, "Re-mapped from 0x%08llX to %p\n", + (unsigned long long)res->start, ctl->base); + + version = axi_fan_control_ioread(ADI_AXI_REG_VERSION, ctl); + if (ADI_AXI_PCORE_VER_MAJOR(version) != + ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data))) { + dev_err(&pdev->dev, "Major version mismatch. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n", + ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data)), + ADI_AXI_PCORE_VER_MINOR((*(u32 *)id->data)), + ADI_AXI_PCORE_VER_PATCH((*(u32 *)id->data)), + ADI_AXI_PCORE_VER_MAJOR(version), + ADI_AXI_PCORE_VER_MINOR(version), + ADI_AXI_PCORE_VER_PATCH(version)); + return -ENODEV; + } + + ctl->irq = platform_get_irq(pdev, 0); + if (ctl->irq < 0) { + dev_err(&pdev->dev, "platform_get_irq failed with %d\n", + ctl->irq); + return ctl->irq; + } + + ret = devm_request_threaded_irq(&pdev->dev, ctl->irq, NULL, + axi_fan_control_irq_handler, + IRQF_ONESHOT | IRQF_TRIGGER_HIGH, + pdev->driver_override, ctl); + if (ret) { + dev_err(&pdev->dev, "failed to request an irq, %d", ret); + return ret; + } + + ret = axi_fan_control_init(ctl, pdev->dev.of_node); + if (ret) { + dev_err(&pdev->dev, "Failed to initialize device\n"); + return ret; + } + + hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, + "axi_fan_control", + ctl, + &axi_fan_control_chip_info, + NULL); + + return PTR_ERR_OR_ZERO(hwmon_dev); +} + +static struct platform_driver axi_fan_control_driver = { + .driver = { + .name = "axi_fan_control_driver", + .of_match_table = axi_fan_control_of_match, + }, + .probe = axi_fan_control_probe, +}; +module_platform_driver(axi_fan_control_driver); + +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>"); +MODULE_DESCRIPTION("Analog Devices Fan Control HDL CORE driver"); +MODULE_LICENSE("GPL");
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á <nuno.sa@analog.com> --- 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