[PATCH linux v3 2/3] drivers: hwmon: Hwmon driver for ASPEED AST2400/2500 PWM support
Jaghathiswari Rankappagounder Natarajan
jaghu at google.com
Fri Dec 2 11:01:56 AEDT 2016
On Sun, Nov 27, 2016 at 10:13 PM, Joel Stanley <joel at jms.id.au> wrote:
> Hello Jaghu,
>
> This is looking better. I have some more comments below.
>
> On Thu, Nov 24, 2016 at 7:56 PM, Jaghathiswari Rankappagounder
> Natarajan <jaghu at google.com> wrote:
>
> > +The ASPEED AST2400/2500 PWM controller supports 8 PWM output ports.
> > +PWM clock types M, N and 0 are three types just to have three
> independent PWM
> > +sources. Each port can be assigned a different PWM clock type.
>
> I suggest you describe the types.
>
> And add some newlines between your paragraphs to make it easier to read.
>
> > +The device driver matches on the device tree node. The configuration
> values
> > +are read from the device tree and written to the respective registers.
> > +The driver provides a sysfs entry "pwm1" through which the user can
> > +configure the duty-cycle value (ranging from 0 to 255; 255 is 100
> percent).
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index d4de8d5..7f75b01 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -341,6 +341,19 @@ config SENSORS_ASB100
> > This driver can also be built as a module. If so, the module
> > will be called asb100.
> >
> > +config SENSORS_ASPEED_PWM
> > + tristate "ASPEED AST2400/AST2500 PWM driver"
> > + help
> > + This driver provides support for ASPEED AST2400/AST2500 PWM
> > + controller and output ports. The ASPEED PWM controller can
> support 8
> > + PWM outputs. PWM clock types M, N and 0 are three types just
> to have three
> > + independent PWM sources. Each could be assigned to the 8 PWM
> port
> > + with its own settings. The user can configure duty cycle
> through
> > + the exposed hwmon sysfs entry "pwm".
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called aspeed-pwm.
>
> Make sure you test the code both as a module and built in to the kernel.
>
> > +
> > +struct aspeed_pwm_port_data {
> > + u8 pwm_port;
> > + u8 pwm_enable;
> > + u8 pwm_type;
> > + u8 pwm_fan_ctrl;
> > + void __iomem *base;
> > +};
> > +
> > +enum pwm_clock_type { TYPEM, TYPEN, TYPEO };
> > +
> > +struct pwm_clock_type_params {
> > + u32 l_value;
> > + u32 l_mask;
> > + u32 h_value;
> > + u32 h_mask;
> > + u32 unit_value;
> > + u32 unit_mask;
> > + u32 ctrl_reg_offset;
>
> ctrl_reg or even just ctrl will do.
>
> > +};
> > +
>
> I think this is an improvement on the duplicated swtich statements we
> had before. Do you agree?
>
> I'm wondering if there are still ways to clean up. The mask and the
> value both tell us where in the register each value needs to go;
> perhaps we can only include one of them in the pwm_clock_type_params?
>
> > +static const struct pwm_clock_type_params pwm_clock_type_params[] = {
> > + [TYPEM] = {
> > + .l_value = ASPEED_PTCR_CLK_CTRL_TYPEM_L,
> > + .l_mask = ASPEED_PTCR_CLK_CTRL_TYPEM_L_MASK,
> > + .h_value = ASPEED_PTCR_CLK_CTRL_TYPEM_H,
> > + .h_mask = ASPEED_PTCR_CLK_CTRL_TYPEM_H_MASK,
> > + .unit_value = ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT,
> > + .unit_mask = ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT_MASK,
> > + .ctrl_reg_offset = ASPEED_PTCR_CLK_CTRL,
> > + },
> > + [TYPEN] = {
> > + .l_value = ASPEED_PTCR_CLK_CTRL_TYPEN_L,
> > + .l_mask = ASPEED_PTCR_CLK_CTRL_TYPEN_L_MASK,
> > + .h_value = ASPEED_PTCR_CLK_CTRL_TYPEN_H,
> > + .h_mask = ASPEED_PTCR_CLK_CTRL_TYPEN_H_MASK,
> > + .unit_value = ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT,
> > + .unit_mask = ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT_MASK,
> > + .ctrl_reg_offset = ASPEED_PTCR_CLK_CTRL,
> > + },
> > + [TYPEO] = {
> > + .l_value = ASPEED_PTCR_CLK_CTRL_TYPEO_L,
> > + .l_mask = ASPEED_PTCR_CLK_CTRL_TYPEO_L_MASK,
> > + .h_value = ASPEED_PTCR_CLK_CTRL_TYPEO_H,
> > + .h_mask = ASPEED_PTCR_CLK_CTRL_TYPEO_H_MASK,
> > + .unit_value = ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT,
> > + .unit_mask = ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT_MASK,
> > + .ctrl_reg_offset = ASPEED_PTCR_CLK_CTRL_EXT,
> > + }
> > +};
> > +
> > +enum pwm_port { PWMA, PWMB, PWMC, PWMD, PWME, PWMF, PWMG, PWMH };
> > +
> > +struct pwm_port_params {
> > + u32 pwm_en;
> > + u32 ctrl_reg_offset;
> > + u32 pwm_type_part1;
> > + u32 pwm_type_part2;
> > + u32 pwm_type_mask;
> > + u32 duty_ctrl_rise_point;
> > + u32 duty_ctrl_fall_point;
> > + u32 duty_ctrl_reg_offset;
> > + u8 duty_ctrl_calc_type;
> > +};
> > +
> > +static const struct pwm_port_params pwm_port_params[] = {
> > + [PWMA] = {
> > + .pwm_en = ASPEED_PTCR_CTRL_PWMA_EN,
> > + .ctrl_reg_offset = ASPEED_PTCR_CTRL,
> > + .pwm_type_part1 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART1,
> > + .pwm_type_part2 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART2,
> > + .pwm_type_mask = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_MASK,
> > + .duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
> > + .duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
> > + .duty_ctrl_reg_offset = ASPEED_PTCR_DUTY0_CTRL,
> > + .duty_ctrl_calc_type = 0,
> > + },
> > +
> > +static inline void
> > +aspeed_pwm_write(void __iomem *base, u32 val, u32 reg)
> > +{
> > + writel(val, base + reg);
>
> Is there a reason you're using writel and not iowrite32?
>
> > +}
> > +
> > +static inline u32
> > +aspeed_pwm_read(void __iomem *base, u32 reg)
> > +{
> > + u32 val = readl(base + reg);
> > + return val;
>
> You could just do return readl(base + reg).
>
> That said, do you think the helpers for _read and _write are worth it?
>
> > +}
> > +
> > +static void
> > +aspeed_set_pwm_clock_enable(void __iomem *base, bool val)
>
> Most kernel code puts the type and storage class specifiers on the
> same line as the function:
>
> static void aspeed_set_pwm_clock_enable(void __iomem *base, bool val)
>
> > +{
> > + u32 reg_value = aspeed_pwm_read(base, ASPEED_PTCR_CTRL);
> > +
> > + if (val)
> > + reg_value |= ASPEED_PTCR_CTRL_CLK_EN;
> > + else
> > + reg_value &= ~ASPEED_PTCR_CTRL_CLK_EN;
> > +
> > + aspeed_pwm_write(base, reg_value, ASPEED_PTCR_CTRL);
> > +}
> > +
> > +static void
> > +aspeed_set_pwm_clock_source(void __iomem *base, bool val)
>
> When I call this function I'm going to write something like this:
>
> aspeed_set_pwm_clock_source(foo, true);
>
> It doesn't make sense to be setting a clock source to "true". I think
> you could call it aspeed_pwm_use_pclk() or similar, so the boolean
> makes sense.
>
> > +{
> > + u32 reg_value = aspeed_pwm_read(base, ASPEED_PTCR_CTRL);
> > +
> > + if (val)
> > + reg_value |= ASPEED_PTCR_CTRL_CLK_SRC;
> > + else
> > + reg_value &= ~ASPEED_PTCR_CTRL_CLK_SRC;
> > +
> > + aspeed_pwm_write(base, reg_value, ASPEED_PTCR_CTRL);
> > +}
> > +
> > +static void
> > +aspeed_set_pwm_clock_division_h(void __iomem *base, u8 pwm_clock_type,
> > + u8 div_high)
> > +{
> > + u32 reg_offset = pwm_clock_type_params[pwm_
> clock_type].ctrl_reg_offset;
> > + u32 reg_value = aspeed_pwm_read(base, reg_offset);
> > +
> > + reg_value &= ~pwm_clock_type_params[pwm_clock_type].h_mask;
> > + reg_value |= div_high << pwm_clock_type_params[pwm_
> clock_type].h_value;
> > +
> > + aspeed_pwm_write(base, reg_value, reg_offset);
> > +}
> > +
> > +static void
> > +aspeed_set_pwm_clock_division_l(void __iomem *base, u8 pwm_clock_type,
> > + u8 div_low)
> > +{
>
> You could pass around a pointer to the clock type, instead of the index.
>
> Even better would be some kind of struct that includes both base and
> the clock type.
>
> > + u32 reg_offset = pwm_clock_type_params[pwm_
> clock_type].ctrl_reg_offset;
> > + u32 reg_value = aspeed_pwm_read(base, reg_offset);
> > +
> > + reg_value &= ~pwm_clock_type_params[pwm_clock_type].l_mask;
> > + reg_value |= div_low << pwm_clock_type_params[pwm_
> clock_type].l_value;
> > +
> > + aspeed_pwm_write(base, reg_value, reg_offset);
> > +}
>
> Is there ever a case where you will set the high but not the low bit?
> If not you could do this:
>
> aspeed_set_pwm_clock_division(void __iomem *base, u8 pwm_clock_type,
> u8 div_low, u8 div_high)
> {
> struct pwm_clock_type *pwm = &pwm_clock_type_params[pwm_
> clock_type];
> u32 reg = aspeed_pwm_read(base, pwm->ctrl_reg_offset);
>
> reg &= ~(pwm->l_mask | pwm->h_mask);
> reg |= (div_low << pwm->l_value) | (div_high << pwm->h_value);
>
> aspeed_pwm_write(base, reg, pwm->ctrl_reg_offset);
> }
>
> I suggest attempting cleanups like this to other parts of your code.
>
> > +
> > +static void
> > +aspeed_set_pwm_clock_unit(void __iomem *base, u8 pwm_clock_type,
> > + u8 unit)
> > +{
> > + u32 reg_offset = pwm_clock_type_params[pwm_
> clock_type].ctrl_reg_offset;
> > + u32 reg_value = aspeed_pwm_read(base, reg_offset);
> > +
> > + reg_value &= ~pwm_clock_type_params[pwm_clock_type].unit_mask;
> > + reg_value |= unit << pwm_clock_type_params[pwm_
> clock_type].unit_value;
> > +
> > + aspeed_pwm_write(base, reg_value, reg_offset);
> > +}
> > +
>
> > +static void
> > +aspeed_set_pwm_fan_ctrl(struct aspeed_pwm_port_data *priv, u8 fan_ctrl)
> > +{
> > + u16 period;
> > + u16 dc_time_on;
> > +
> > + period = aspeed_get_pwm_clock_unit(priv, priv->pwm_type);
> > + period += 1;
> > + dc_time_on = (fan_ctrl * period) / PWM_MAX;
> > +
> > + if (dc_time_on == 0) {
> > + aspeed_set_pwm_enable(priv, 0);
>
> Doesn't aspeed_set_pwm_enable take a boolean as the second parameter?
>
> > + } else {
> > + if (dc_time_on == period)
> > + dc_time_on = 0;
> > +
> > + aspeed_set_pwm_duty_rising(priv, 0);
> > + aspeed_set_pwm_duty_falling(priv, dc_time_on);
> > + aspeed_set_pwm_enable(priv, 1);
> > + }
> > +}
> > +
> > +static ssize_t
> > +set_pwm(struct device *dev, struct device_attribute *attr, const char
> *buf,
> > + size_t count)
> > +{
> > + struct aspeed_pwm_port_data *priv = dev_get_drvdata(dev);
> > + u8 fan_ctrl;
> > + int ret;
> > +
> > + ret = kstrtou8(buf, 10, &fan_ctrl);
>
> If you specify 0 as the format, then users can pass their clock rate
> in as hexadecimal (not that it would make much sense...) or decimal
> and the sysfs file will do the correct thing.
The user is passing the duty cycle value here (and not the clock rate).
> > + if (ret)
> > + return -EINVAL;
> > +
> > + if (fan_ctrl < 0 || fan_ctrl > PWM_MAX)
> > + return -EINVAL;
> > +
> > + if (priv->pwm_fan_ctrl == fan_ctrl)
> > + return count;
> > +
> > + priv->pwm_fan_ctrl = fan_ctrl;
> > + aspeed_set_pwm_fan_ctrl(priv, fan_ctrl);
> > +
> > + return count;
> > +}
> > +
> > +static ssize_t
> > +show_pwm(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > + struct aspeed_pwm_port_data *priv = dev_get_drvdata(dev);
> > +
> > + return sprintf(buf, "%u\n", priv->pwm_fan_ctrl);
> > +}
> > +
> > +static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, show_pwm, set_pwm,
> 0);
> > +
> > +static struct attribute *pwm_dev_attrs[] = {
> > + &sensor_dev_attr_pwm1.dev_attr.attr,
> > + NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(pwm_dev);
> > +
> > +static int
> > +aspeed_create_pwm_port(struct device *dev, struct device_node *child,
> > + void __iomem *base)
> > +{
> > + struct device *hwmon;
> > + u8 val;
> > + struct aspeed_pwm_port_data *priv;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + hwmon = devm_hwmon_device_register_with_groups(dev,
> "aspeed_pwm",
> > + priv,
> pwm_dev_groups);
> > + if (IS_ERR(hwmon)) {
> > + dev_err(dev, "Failed to register hwmon device\n");
> > + return PTR_ERR(hwmon);
> > + }
> > + priv->base = base;
> > +
> > + of_property_read_u8(child, "pwm_port", &val);
> > + priv->pwm_port = val;
> > +
> > + of_property_read_u8(child, "pwm_enable", &val);
> > + priv->pwm_enable = val;
> > + aspeed_set_pwm_enable(priv, val);
> > +
> > + of_property_read_u8(child, "pwm_type", &val);
> > + priv->pwm_type = val;
> > + aspeed_set_pwm_type(priv, val);
> > +
> > + of_property_read_u8(child, "pwm_fan_ctrl", &val);
> > + priv->pwm_fan_ctrl = val;
> > + aspeed_set_pwm_fan_ctrl(priv, val);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * If the clock type is type M then :
> > + * The PWM frequency = 24MHz / (type M clock division L bit *
> > + * type M clock division H bit * (type M PWM period bit + 1))
> > + * Calculate type M clock division L bit and H bits given the other
> values
> > + */
> > +static bool
> > +set_clock_values(u32 pwm_frequency, u32 period, u8 type, void __iomem
> *base)
> > +{
> > + u32 src_frequency = 24000000;
>
> Where does this come from? It's the pclk I think. This means your
> binding should do something like this:
>
> clocks = <&clk_apb>;
>
> Then in your code,
>
> pclk = devm_clk_get(&pdev->dev, NULL);
> pclk_rate = clk_get_rate(pclk);
>
> and then use clck_rate where you have 'src_frequency' to calculate the
> divisor values. A hwmon driver that does this is g762.c.
For the PWM and Fan Tach controller, in the register PTCR00 (AST2500
datasheet page - 671) it is given as :
Clock source selection
0: from 24MHz
1: from MCLK
24MHz seems to be hardcoded. I am not sure which clock producer to use
for 24MHz and for MCLK.
CLKIN seems to have the option of being either 24MHz and 25 MHz.
I am not sure how the pclk is related here.
> > + u32 divisor = src_frequency / pwm_frequency;
> > + u32 clock_divisor = divisor / (period + 1);
> > + u32 tmp_clock_divisor;
> > + u8 low;
> > + u8 high;
> > + u32 calc_high;
> > + u32 calc_low;
> > +
> > + for (high = 0; high <= MAX_HIGH_LOW_BIT; high += 1) {
> > + for (low = 0; low <= MAX_HIGH_LOW_BIT; low += 1) {
> > + calc_high = 0x1 << high;
> > + calc_low = (low == 0 ? 1 : (2 * low));
> > + tmp_clock_divisor = calc_high * calc_low;
> > + if (tmp_clock_divisor >= clock_divisor)
> > + goto set_value;
> > + }
> > + }
> > +
> > + return false;
> > +
> > +set_value:
> > + aspeed_set_pwm_clock_division_h(base, type, high);
> > + aspeed_set_pwm_clock_division_l(base, type, low);
> > + aspeed_set_pwm_clock_unit(base, type, period);
> > +
> > + return true;
> > +}
> > +
> > +static int
> > +aspeed_pwm_probe(struct platform_device *pdev)
> > +{
> > + u32 period;
> > + struct device_node *np, *child;
> > + struct resource *res;
> > + void __iomem *base;
> > + bool success;
> > + struct clk *typem_clk, *typen_clk, *typeo_clk;
> > + u32 pwm_typem_freq, pwm_typen_freq, pwm_typeo_freq;
> > + int err;
> > +
> > + np = pdev->dev.of_node;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (res == NULL)
> > + return -ENOENT;
> > +
> > + base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> > + if (!base)
> > + return -ENOMEM;
> > +
> > + /* Enable PWM clock */
> > + aspeed_set_pwm_clock_enable(base, 1);
>
> You're passing a '1' to a function that expects a boolean value.
>
> > + /* Select clock source as 24MHz */
> > + aspeed_set_pwm_clock_source(base, 0);
>
> This would be selected by your device tree bindings. You could have it
> so if a clock node is populated, it uses that value, and if it's not
> it falls back on the 1MHz clock.
>
> > +
> > + typem_clk = of_clk_get(np, 0);
>
> This isn't what I meant. See the comment above about referencing the
> parent clock in the bindings.
>
> > + if (!IS_ERR(typem_clk))
> > + pwm_typem_freq = clk_get_rate(typem_clk);
> > + else
> > + return -EINVAL;
> > +
> > + typen_clk = of_clk_get(np, 1);
> > + if (!IS_ERR(typen_clk))
> > + pwm_typen_freq = clk_get_rate(typen_clk);
> > +
> > + typeo_clk = of_clk_get(np, 2);
> > + if (!IS_ERR(typeo_clk))
> > + pwm_typeo_freq = clk_get_rate(typeo_clk);
>
> This doesn't make any sense. Did you test this code?
>
> > +
> > + err = of_property_read_u32(np, "pwm_typem_period", &period);
> > + if (!err) {
> > + success = set_clock_values(pwm_typem_freq, period,
> TYPEM, base);
> > + if (!success)
> > + return -EINVAL;
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + err = of_property_read_u32(np, "pwm_typen_period", &period);
> > + if (!err) {
> > + success = set_clock_values(pwm_typen_freq, period,
> TYPEN, base);
> > + if (!success)
> > + return -EINVAL;
> > + }
> > +
> > + err = of_property_read_u32(np, "pwm_typeo_period", &period);
> > + if (!err) {
> > + success = set_clock_values(pwm_typeo_freq, period,
> TYPEO, base);
> > + if (!success)
> > + return -EINVAL;
> > + }
> > +
> > + for_each_child_of_node(np, child) {
> > + aspeed_create_pwm_port(&pdev->dev,
> > + child, base);
> > + of_node_put(child);
> > + }
> > + of_node_put(np);
> > +
> > + return 0;
> > +}
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20161201/c1635ad6/attachment-0001.html>
More information about the openbmc
mailing list