Re: [patch v6 0/3] JTAG driver introduction
On Sun, Aug 27, 2017 at 3:30 PM, Linus Walleij wrote: > On Fri, Aug 25, 2017 at 6:52 PM, Rick Altherr wrote: > >>> Incidentally, people are sending patches to expose the FTDI >>> expanders as common GPIO chips under Linux, so we can >>> internally in the kernel or from the usersapce character device >>> access them as "some GPIOs". >> >> I know my team at Google has an internal patch for exactly that. FTDI >> expanders are complicated as they can be used as UART, GPIO, I2C, SPI >> depending on configuration. Our project was using a mix of I2C and >> GPIO so I directly my team to approach it as an MFD. I'd like to see >> all of these use cases handled by the kernel but I understand the >> other viewpoint of relying on libusb for cross-platform compatiblity. > > Hm. I see. But I see people pushing the in-kernel method so I think > it will eventually win out. > SGTM. I'll see if my team can clean up the MFD-based FTDI driver and submit it upstream. >>>>> In my worst nightmare they export GPIO lines using >>>>> the horrid ABI in /sys/gpio/* >>>> >>>> https://sourceforge.net/p/openocd/code/ci/v0.10.0/tree/src/jtag/drivers/sysfsgpio.c >>> >>> Gnah! >>> Whoever writes a slot-in replacement making the character device >>> take precendence wins lots of karma. >> >> If they show up at Linux Plumbers or visit San Jose, I'll take them to >> dinner. I didn't see any docs for the chardev in Documentation. I >> _think_ I understand how it works from reading the relevant sections >> of gpiolib.c but I can see how users end up using sysfs instead. > > I intended tools/gpio/* to be the documentation: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/gpio > > If we need more written documentation we can do it I guess, I haven't been in the habit of looking in tools/. Guess I should be. > > Yours, > Linus Walleij
Re: [patch v6 0/3] JTAG driver introduction
On Fri, Aug 25, 2017 at 1:50 AM, Stuart Longland wrote: > [Note: dropping vad...@maellanox.com as SMTP server complained about the > DNS server returning NXDOMAIN. Apologies.] > On 25/08/17 18:32, Linus Walleij wrote: >> Gnah! >> Whoever writes a slot-in replacement making the character device >> take precendence wins lots of karma. > > What would such a replacement look like though? See Linus's comments about using the existing kernel GPIO chardev interface. It already supports requesting multiple GPIO line state changes in a single request. > > Some sort of system whereby you can read/write single-line commands as > if talking to a GPIO expander over a UART? > > Would you access the GPIOs one by one, or would you perhaps map them > into a bitmap (maybe arbitrarily, up to 64-bits wide) and perform masked > operations on the bitmap? > > I'm no fan of the sysfs GPIO interface, but it beats poking around at > registers behind the kernel's back. > -- > Stuart Longland (aka Redhatter, VK4MSL) > > I haven't lost my mind... > ...it's backed up on a tape somewhere. >
Re: [patch v6 0/3] JTAG driver introduction
On Fri, Aug 25, 2017 at 1:32 AM, Linus Walleij wrote: > On Thu, Aug 24, 2017 at 11:37 PM, Rick Altherr wrote: >> On Thu, Aug 24, 2017 at 2:07 PM, Linus Walleij >> wrote: >>> On Tue, Aug 22, 2017 at 6:10 PM, Oleksandr Shamray >>> wrote: >>> >>>> SoC which are not equipped with JTAG master interface, can be built >>>> on top of JTAG core driver infrastructure, by applying bit-banging of >>>> TDI, TDO, TCK and TMS pins within the hardware specific driver. >>> >>> I guess you mean it should then use GPIO lines for bit-banging? >>> >>> I was wondering about how some JTAG clients like openOCD does >>> this in some cases. >> >> Many common uses of OpenOCD leverage USB devices, such as FTDI FT232R, >> that have a command queue for bitbanging operations. Managing these >> via libusb is ugly but platform-agnostic. > > Incidentally, people are sending patches to expose the FTDI > expanders as common GPIO chips under Linux, so we can > internally in the kernel or from the usersapce character device > access them as "some GPIOs". > I know my team at Google has an internal patch for exactly that. FTDI expanders are complicated as they can be used as UART, GPIO, I2C, SPI depending on configuration. Our project was using a mix of I2C and GPIO so I directly my team to approach it as an MFD. I'd like to see all of these use cases handled by the kernel but I understand the other viewpoint of relying on libusb for cross-platform compatiblity. >>> In my worst nightmare they export GPIO lines using >>> the horrid ABI in /sys/gpio/* >> >> https://sourceforge.net/p/openocd/code/ci/v0.10.0/tree/src/jtag/drivers/sysfsgpio.c > > Gnah! > Whoever writes a slot-in replacement making the character device > take precendence wins lots of karma. > If they show up at Linux Plumbers or visit San Jose, I'll take them to dinner. I didn't see any docs for the chardev in Documentation. I _think_ I understand how it works from reading the relevant sections of gpiolib.c but I can see how users end up using sysfs instead. >> While that is certainly horrible (and slow), mapping in the GPIO >> registers via /dev/mem strikes me as worse: >> >> https://sourceforge.net/p/openocd/code/ci/v0.10.0/tree/src/jtag/drivers/bcm2835gpio.c > > Yeah that is quite horrible. > > There were reasons to do things like that, but since we have > developed .set_multiple() to hammer several lines in a register > at once, the same efficiency can be achieved using the standard > character device. Agreed that that helps with user-space GPIO-based JTAG implementations. The problem this patch series is trying to address is for SoCs like the Aspeed AST2400/2500 that include a hardware accelerated JTAG master. It _can_ be run in a pure-software mode where it acts like GPIOs but the intended use case is to operate an interrupt-driven state machine. That requires a higher-level abstraction for managing the standard JTAG state machine. Similar to GPIO .set_multiple, user-space feeding a JTAG kernel API a buffer of JTAG state changes would be useful. That's how I recall OpenOCD's internals working: run short (1-7?) state change sequences to move to the next decision point. I know of at least one vendor pushing for the use of JTAG on BMCs as a way to debug host processors in large deployments instead of using dongles. I'm supportive of the adding a JTAG driver abstraction. I haven't reviewed this patch series in detail yet. > > Yours, > Linus Walleij
Re: [patch v6 0/3] JTAG driver introduction
On Thu, Aug 24, 2017 at 2:07 PM, Linus Walleij wrote: > On Tue, Aug 22, 2017 at 6:10 PM, Oleksandr Shamray > wrote: > >> SoC which are not equipped with JTAG master interface, can be built >> on top of JTAG core driver infrastructure, by applying bit-banging of >> TDI, TDO, TCK and TMS pins within the hardware specific driver. > > I guess you mean it should then use GPIO lines for bit-banging? > > I was wondering about how some JTAG clients like openOCD does > this in some cases. > Many common uses of OpenOCD leverage USB devices, such as FTDI FT232R, that have a command queue for bitbanging operations. Managing these via libusb is ugly but platform-agnostic. > In my worst nightmare they export GPIO lines using > the horrid ABI in /sys/gpio/* > https://sourceforge.net/p/openocd/code/ci/v0.10.0/tree/src/jtag/drivers/sysfsgpio.c While that is certainly horrible (and slow), mapping in the GPIO registers via /dev/mem strikes me as worse: https://sourceforge.net/p/openocd/code/ci/v0.10.0/tree/src/jtag/drivers/bcm2835gpio.c > In best case they use the GPIO character device or even > libgpiod. > > But having a JTAG abstraction inside the kernel that can > grab a few lines for JTAG defined in a device tree, ACPI DSDT > or similar makes sense too, as it abstracts the hardware so the > JTAG client can then just open whatever /dev/jtag0 is on the machine > and go ahead without having to bother about what GPIO lines > are connected exactly where. > > Yours, > Linus Walleij
Re: [PATCH v2 1/1] i2c: aspeed: add proper support fo 24xx clock params
Is clk_fractional_divider from include/linux/clk-provider.h appropriate here?
On Fri, Jul 28, 2017 at 1:57 PM, Rick Altherr wrote:
> Is clk_fractional_divider from include/linux/clk-provider.h appropriate
> here?
>
> On Fri, Jul 28, 2017 at 1:45 PM, Brendan Higgins
> wrote:
>>
>> 24xx BMCs have larger clock divider granularity which can cause problems
>> when trying to set them as 25xx clock dividers; this adds clock setting
>> code specific to 24xx.
>>
>> This also fixes a potential issue where clock dividers were rounded down
>> instead of up.
>>
>> Signed-off-by: Brendan Higgins
>> ---
>> Changes for v2:
>> - Fixed off by one error to check for divisors with base_clk == 0
>> - Simplified some of the divisor math
>> ---
>> drivers/i2c/busses/i2c-aspeed.c | 74
>> +++--
>> 1 file changed, 56 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c
>> b/drivers/i2c/busses/i2c-aspeed.c
>> index f19348328a71..60afab866494 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -132,6 +132,7 @@ struct aspeed_i2c_bus {
>> /* Synchronizes I/O mem access to base. */
>> spinlock_t lock;
>> struct completion cmd_complete;
>> + u32 (*get_clk_reg_val)(u32 divisor);
>> unsigned long parent_clk_frequency;
>> u32 bus_frequency;
>> /* Transaction state. */
>> @@ -674,7 +675,7 @@ static const struct i2c_algorithm aspeed_i2c_algo = {
>> #endif /* CONFIG_I2C_SLAVE */
>> };
>>
>> -static u32 aspeed_i2c_get_clk_reg_val(u32 divisor)
>> +static u32 aspeed_i2c_get_clk_reg_val(u32 clk_high_low_max, u32 divisor)
>> {
>> u32 base_clk, clk_high, clk_low, tmp;
>>
>> @@ -694,16 +695,22 @@ static u32 aspeed_i2c_get_clk_reg_val(u32 divisor)
>> * Thus,
>> * SCL_freq = APB_freq /
>> * ((1 << base_clk) * (clk_high + 1 + clk_low + 1))
>> -* The documentation recommends clk_high >= 8 and clk_low >= 7
>> when
>> -* possible; this last constraint gives us the following solution:
>> +* The documentation recommends clk_high >= clk_high_max / 2 and
>> +* clk_low >= clk_low_max / 2 - 1 when possible; this last
>> constraint
>> +* gives us the following solution:
>> */
>> - base_clk = divisor > 33 ? ilog2((divisor - 1) / 32) + 1 : 0;
>> - tmp = divisor / (1 << base_clk);
>> - clk_high = tmp / 2 + tmp % 2;
>> - clk_low = tmp - clk_high;
>> + base_clk = divisor > clk_high_low_max ?
>> + ilog2((divisor - 1) / clk_high_low_max) + 1 : 0;
>> + tmp = (divisor + (1 << base_clk) - 1) >> base_clk;
>> + clk_low = tmp / 2;
>> + clk_high = tmp - clk_low;
>> +
>> + if (clk_high)
>> + clk_high--;
>> +
>> + if (clk_low)
>> + clk_low--;
>>
>> - clk_high -= 1;
>> - clk_low -= 1;
>>
>> return ((clk_high << ASPEED_I2CD_TIME_SCL_HIGH_SHIFT)
>> & ASPEED_I2CD_TIME_SCL_HIGH_MASK)
>> @@ -712,13 +719,31 @@ static u32 aspeed_i2c_get_clk_reg_val(u32 divisor)
>> | (base_clk & ASPEED_I2CD_TIME_BASE_DIVISOR_MASK);
>> }
>>
>> +static u32 aspeed_i2c_24xx_get_clk_reg_val(u32 divisor)
>> +{
>> + /*
>> +* clk_high and clk_low are each 3 bits wide, so each can hold a
>> max
>> +* value of 8 giving a clk_high_low_max of 16.
>> +*/
>> + return aspeed_i2c_get_clk_reg_val(16, divisor);
>> +}
>> +
>> +static u32 aspeed_i2c_25xx_get_clk_reg_val(u32 divisor)
>> +{
>> + /*
>> +* clk_high and clk_low are each 4 bits wide, so each can hold a
>> max
>> +* value of 16 giving a clk_high_low_max of 32.
>> +*/
>> + return aspeed_i2c_get_clk_reg_val(32, divisor);
>> +}
>> +
>> /* precondition: bus.lock has been acquired. */
>> static int aspeed_i2c_init_clk(struct aspeed_i2c_bus *bus)
>> {
>> u32 divisor, clk_reg_val;
>>
>> - divisor = bus->parent_clk_frequency / bus->bus_frequency;
>> - clk_reg_val = aspeed_i2c_get_clk_reg_val(divisor);
>> + diviso
Re: [PATCH] iio: Aspeed ADC - Handle return value of clk_prepare_enable
Reviewed-by: Rick Altherr On Sat, Jun 3, 2017 at 1:50 AM, Jonathan Cameron wrote: > On Mon, 29 May 2017 13:12:12 +0530 > Arvind Yadav wrote: > >> clk_prepare_enable() can fail here and we must check its return value. >> >> Signed-off-by: Arvind Yadav > Please make sure to cc the driver author. This is a fairly new > driver, so chances are Rick will still be answering this email > address. > > Patch looks fine to me, but I would like to give Rick the opportunity > to respond. > > Jonathan >> --- >> drivers/iio/adc/aspeed_adc.c | 6 +- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c >> index 62670cb..e0ea411 100644 >> --- a/drivers/iio/adc/aspeed_adc.c >> +++ b/drivers/iio/adc/aspeed_adc.c >> @@ -212,7 +212,10 @@ static int aspeed_adc_probe(struct platform_device >> *pdev) >> } >> >> /* Start all channels in normal mode. */ >> - clk_prepare_enable(data->clk_scaler->clk); >> + ret = clk_prepare_enable(data->clk_scaler->clk); >> + if (ret) >> + goto clk_enable_error; >> + >> adc_engine_control_reg_val = GENMASK(31, 16) | >> ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE; >> writel(adc_engine_control_reg_val, >> @@ -236,6 +239,7 @@ static int aspeed_adc_probe(struct platform_device *pdev) >> writel(ASPEED_OPERATION_MODE_POWER_DOWN, >> data->base + ASPEED_REG_ENGINE_CONTROL); >> clk_disable_unprepare(data->clk_scaler->clk); >> +clk_enable_error: >> clk_hw_unregister_divider(data->clk_scaler); >> >> scaler_error: >
[PATCH v2 2/2] hw_random: timeriomem_rng: Allow setting RNG quality from platform data
When a hw_random device's quality is non-zero, it will automatically be
used to fill the kernel's entropy pool. Since timeriomem_rng is used by
many different devices, the quality needs to be provided by platform
data or device tree.
Signed-off-by: Rick Altherr
---
Changes in v2:
- Adjust header comment for platform data quality property to match code
and DT bindings
drivers/char/hw_random/timeriomem-rng.c | 7 +++
include/linux/timeriomem-rng.h | 3 +++
2 files changed, 10 insertions(+)
diff --git a/drivers/char/hw_random/timeriomem-rng.c
b/drivers/char/hw_random/timeriomem-rng.c
index a0faa5f05deb..03ff5483d865 100644
--- a/drivers/char/hw_random/timeriomem-rng.c
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -151,8 +151,15 @@ static int timeriomem_rng_probe(struct platform_device
*pdev)
dev_err(&pdev->dev, "missing period\n");
return -EINVAL;
}
+
+ if (!of_property_read_u32(pdev->dev.of_node,
+ "quality", &i))
+ priv->rng_ops.quality = i;
+ else
+ priv->rng_ops.quality = 0;
} else {
period = pdata->period;
+ priv->rng_ops.quality = pdata->quality;
}
priv->period = ns_to_ktime(period * NSEC_PER_USEC);
diff --git a/include/linux/timeriomem-rng.h b/include/linux/timeriomem-rng.h
index 46eb27ddbfab..3e00122bcf88 100644
--- a/include/linux/timeriomem-rng.h
+++ b/include/linux/timeriomem-rng.h
@@ -13,4 +13,7 @@ struct timeriomem_rng_data {
/* measures in usecs */
unsigned intperiod;
+
+ /* bits of entropy per 1024 bits read */
+ unsigned intquality;
};
--
2.13.0.303.g4ebf302169-goog
[PATCH v2 1/2] dt-bindings: timeriomem_rng: Add entropy quality property
Signed-off-by: Rick Altherr --- Changes in v2: None Documentation/devicetree/bindings/rng/timeriomem_rng.txt | 7 +++ 1 file changed, 7 insertions(+) diff --git a/Documentation/devicetree/bindings/rng/timeriomem_rng.txt b/Documentation/devicetree/bindings/rng/timeriomem_rng.txt index 6616d15866a3..214940093b55 100644 --- a/Documentation/devicetree/bindings/rng/timeriomem_rng.txt +++ b/Documentation/devicetree/bindings/rng/timeriomem_rng.txt @@ -5,6 +5,13 @@ Required properties: - reg : base address to sample from - period : wait time in microseconds to use between samples +Optional properties: +- quality : estimated number of bits of true entropy per 1024 bits read from the +rng. Defaults to zero which causes the kernel's default quality to +be used instead. Note that the default quality is usually zero +which disables using this rng to automatically fill the kernel's +entropy pool. + N.B. currently 'reg' must be four bytes wide and aligned Example: -- 2.13.0.303.g4ebf302169-goog
[PATCH v1 1/2] dt-bindings: timeriomem_rng: Add entropy quality property
Signed-off-by: Rick Altherr --- Documentation/devicetree/bindings/rng/timeriomem_rng.txt | 7 +++ 1 file changed, 7 insertions(+) diff --git a/Documentation/devicetree/bindings/rng/timeriomem_rng.txt b/Documentation/devicetree/bindings/rng/timeriomem_rng.txt index 6616d15866a3..214940093b55 100644 --- a/Documentation/devicetree/bindings/rng/timeriomem_rng.txt +++ b/Documentation/devicetree/bindings/rng/timeriomem_rng.txt @@ -5,6 +5,13 @@ Required properties: - reg : base address to sample from - period : wait time in microseconds to use between samples +Optional properties: +- quality : estimated number of bits of true entropy per 1024 bits read from the +rng. Defaults to zero which causes the kernel's default quality to +be used instead. Note that the default quality is usually zero +which disables using this rng to automatically fill the kernel's +entropy pool. + N.B. currently 'reg' must be four bytes wide and aligned Example: -- 2.13.0.303.g4ebf302169-goog
[PATCH v1 2/2] hw_random: timeriomem_rng: Allow setting RNG quality from platform data
When a hw_random device's quality is non-zero, it will automatically be
used to fill the kernel's entropy pool. Since timeriomem_rng is used by
many different devices, the quality needs to be provided by platform
data or device tree.
Signed-off-by: Rick Altherr
---
drivers/char/hw_random/timeriomem-rng.c | 7 +++
include/linux/timeriomem-rng.h | 3 +++
2 files changed, 10 insertions(+)
diff --git a/drivers/char/hw_random/timeriomem-rng.c
b/drivers/char/hw_random/timeriomem-rng.c
index a0faa5f05deb..03ff5483d865 100644
--- a/drivers/char/hw_random/timeriomem-rng.c
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -151,8 +151,15 @@ static int timeriomem_rng_probe(struct platform_device
*pdev)
dev_err(&pdev->dev, "missing period\n");
return -EINVAL;
}
+
+ if (!of_property_read_u32(pdev->dev.of_node,
+ "quality", &i))
+ priv->rng_ops.quality = i;
+ else
+ priv->rng_ops.quality = 0;
} else {
period = pdata->period;
+ priv->rng_ops.quality = pdata->quality;
}
priv->period = ns_to_ktime(period * NSEC_PER_USEC);
diff --git a/include/linux/timeriomem-rng.h b/include/linux/timeriomem-rng.h
index 46eb27ddbfab..320f743bf06b 100644
--- a/include/linux/timeriomem-rng.h
+++ b/include/linux/timeriomem-rng.h
@@ -13,4 +13,7 @@ struct timeriomem_rng_data {
/* measures in usecs */
unsigned intperiod;
+
+ /* bits of entropy per 1000 bits read */
+ unsigned intquality;
};
--
2.13.0.303.g4ebf302169-goog
Re: [PATCH v4 2/2] iio: Aspeed ADC
On Wed, Apr 5, 2017 at 6:10 PM, Stephen Boyd wrote:
> On 04/05, Rick Altherr wrote:
>> On Wed, Apr 5, 2017 at 1:27 PM, Stephen Boyd wrote:
>> > On 03/23, Rick Altherr wrote:
>> >> +
>> >> +static int aspeed_adc_probe(struct platform_device *pdev)
>> >> +{
>> >> + struct iio_dev *indio_dev;
>> >> + struct aspeed_adc_data *data;
>> >> + const struct aspeed_adc_model_data *model_data;
>> >> + struct resource *res;
>> >> + const char *clk_parent_name;
>> >> + int ret;
>> >> + u32 adc_engine_control_reg_val;
>> >> +
>> >> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
>> >> + if (!indio_dev)
>> >> + return -ENOMEM;
>> >> +
>> >> + data = iio_priv(indio_dev);
>> >> + data->dev = &pdev->dev;
>> >> +
>> >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >> + data->base = devm_ioremap_resource(&pdev->dev, res);
>> >> + if (IS_ERR(data->base))
>> >> + return PTR_ERR(data->base);
>> >> +
>> >> + /* Register ADC clock prescaler with source specified by device
>> >> tree. */
>> >> + spin_lock_init(&data->clk_lock);
>> >> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
>> >
>> > What if the parent clk is not registered yet? Or if we're not
>> > always using DT in this driver? Put another way, this code is
>> > fragile. But I guess it probably works well enough for now so no
>> > big deal, just pointing out my fear.
>>
>> I'm not terribly worried about not using DT for this driver as it is
>> for an ARM SoC's built-in ADC which is only supported via DT. I take
>> your point though. What's the right way to do this? Use clk_get() to
>> request by name and clock-names in the DT?
>
> Yes that will work. When we add probe defer to clk_get() things
> will work better. Presumably the clocks property already exists
> for this device so that of_clk_get_parent_name() works so it's
> not a binding change?
The bindings include clocks but not clock-names. In this case,
clk_register_divider() only has variants that take a parent clock
name. I can't see what I'd do with the result of clk_get(). If the
bindings provide clock-names, I can provide a fixed string for the
parent name instead of relying on of_clk_get_parent_name(). The
removes an explicit driver dependency on DT. I'm unclear if it
resolves your concerns about registration ordering.
>
>>
>> >
>> >> +
>> >> + data->clk_prescaler = clk_hw_register_divider(
>> >> + &pdev->dev, "prescaler", clk_parent_name, 0,
>> >> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> >> + 17, 15, 0, &data->clk_lock);
>> >> + if (IS_ERR(data->clk_prescaler))
>> >> + return PTR_ERR(data->clk_prescaler);
>> >> +
>> >> + /*
>> >> + * Register ADC clock scaler downstream from the prescaler. Allow
>> >> rate
>> >> + * setting to adjust the prescaler as well.
>> >> + */
>> >> + data->clk_scaler = clk_hw_register_divider(
>> >> + &pdev->dev, "scaler", "prescaler",
>> >> + CLK_SET_RATE_PARENT,
>> >> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> >> + 0, 10, 0, &data->clk_lock);
>> >> + if (IS_ERR(data->clk_scaler)) {
>> >> + ret = PTR_ERR(data->clk_scaler);
>> >> + goto scaler_error;
>> >> + }
>> >> +
>> >> + /* Start all channels in normal mode. */
>> >> + clk_prepare_enable(data->clk_scaler->clk);
>> >
>> > Eventually we'd like to get rid of hw->clk pointer so that users
>> > have to call some sort of clk_get() API and then we get warm
>> > fuzzies from knowing who is consuming a clk structure. Can you
>> > change this to register a clk provider and call clk_get()? I
>> > think a device that references itself should be OK in DT still,
>> > and would properly reflect what's going on.
>>
>> Do you mean call of_clk_add_hw_provider with of_clk_hw_simple_get or
>> something else? I'm still wrapping my head around the distinction
>> between clk, clk_hw, and a clk provider.
>>
>
> Yes. Unless you don't want to expose these details in DT because
> it's all internal to the device?
>
There's no reason to expose the scaler or prescaler in DT. These are
clocks that internal to the ADC and have no way for other devices to
use them. I only used the clock framework to avoid reinventing the
wheel on calculating divider values for a desired clock rate.
> In that case we need to go merge that patch on the clk list to
> have clk_hw_create_clk() or something like that, so we can turn a
> clk_hw structure into a clk pointer without directly referencing
> the clk member of clk_hw.
This makes the most sense to me.
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
Re: [PATCH v4 2/2] iio: Aspeed ADC
On Wed, Apr 5, 2017 at 1:27 PM, Stephen Boyd wrote:
> On 03/23, Rick Altherr wrote:
>> +
>> +static int aspeed_adc_probe(struct platform_device *pdev)
>> +{
>> + struct iio_dev *indio_dev;
>> + struct aspeed_adc_data *data;
>> + const struct aspeed_adc_model_data *model_data;
>> + struct resource *res;
>> + const char *clk_parent_name;
>> + int ret;
>> + u32 adc_engine_control_reg_val;
>> +
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + data = iio_priv(indio_dev);
>> + data->dev = &pdev->dev;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + data->base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(data->base))
>> + return PTR_ERR(data->base);
>> +
>> + /* Register ADC clock prescaler with source specified by device tree.
>> */
>> + spin_lock_init(&data->clk_lock);
>> + clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
>
> What if the parent clk is not registered yet? Or if we're not
> always using DT in this driver? Put another way, this code is
> fragile. But I guess it probably works well enough for now so no
> big deal, just pointing out my fear.
I'm not terribly worried about not using DT for this driver as it is
for an ARM SoC's built-in ADC which is only supported via DT. I take
your point though. What's the right way to do this? Use clk_get() to
request by name and clock-names in the DT?
>
>> +
>> + data->clk_prescaler = clk_hw_register_divider(
>> + &pdev->dev, "prescaler", clk_parent_name, 0,
>> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> + 17, 15, 0, &data->clk_lock);
>> + if (IS_ERR(data->clk_prescaler))
>> + return PTR_ERR(data->clk_prescaler);
>> +
>> + /*
>> + * Register ADC clock scaler downstream from the prescaler. Allow rate
>> + * setting to adjust the prescaler as well.
>> + */
>> + data->clk_scaler = clk_hw_register_divider(
>> + &pdev->dev, "scaler", "prescaler",
>> + CLK_SET_RATE_PARENT,
>> + data->base + ASPEED_REG_CLOCK_CONTROL,
>> + 0, 10, 0, &data->clk_lock);
>> + if (IS_ERR(data->clk_scaler)) {
>> + ret = PTR_ERR(data->clk_scaler);
>> + goto scaler_error;
>> + }
>> +
>> + /* Start all channels in normal mode. */
>> + clk_prepare_enable(data->clk_scaler->clk);
>
> Eventually we'd like to get rid of hw->clk pointer so that users
> have to call some sort of clk_get() API and then we get warm
> fuzzies from knowing who is consuming a clk structure. Can you
> change this to register a clk provider and call clk_get()? I
> think a device that references itself should be OK in DT still,
> and would properly reflect what's going on.
Do you mean call of_clk_add_hw_provider with of_clk_hw_simple_get or
something else? I'm still wrapping my head around the distinction
between clk, clk_hw, and a clk provider.
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
[PATCH v2 1/3] hw_random: Migrate timeriomem_rng to new API
Preserves the existing behavior of only returning 32-bits per call.
Signed-off-by: Rick Altherr
---
Changes in v2:
- Split API migration into separate patch
drivers/char/hw_random/timeriomem-rng.c | 60 -
1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/drivers/char/hw_random/timeriomem-rng.c
b/drivers/char/hw_random/timeriomem-rng.c
index cf37db263ecd..17574452fd35 100644
--- a/drivers/char/hw_random/timeriomem-rng.c
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -20,18 +20,16 @@
* TODO: add support for reading sizes other than 32bits and masking
*/
-#include
-#include
-#include
-#include
+#include
#include
#include
+#include
+#include
+#include
+#include
#include
#include
-#include
-#include
#include
-#include
struct timeriomem_rng_private_data {
void __iomem*io_base;
@@ -45,32 +43,36 @@ struct timeriomem_rng_private_data {
struct hwrngtimeriomem_rng_ops;
};
-#define to_rng_priv(rng) \
- ((struct timeriomem_rng_private_data *)rng->priv)
-
-/*
- * have data return 1, however return 0 if we have nothing
- */
-static int timeriomem_rng_data_present(struct hwrng *rng, int wait)
+static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
+ size_t max, bool wait)
{
- struct timeriomem_rng_private_data *priv = to_rng_priv(rng);
+ struct timeriomem_rng_private_data *priv =
+ container_of(hwrng, struct timeriomem_rng_private_data,
+ timeriomem_rng_ops);
+ unsigned long cur;
+ s32 delay;
- if (!wait || priv->present)
- return priv->present;
+ /* The RNG provides 32-bit per read. Ensure there is enough space. */
+ if (max < sizeof(u32))
+ return 0;
- wait_for_completion(&priv->completion);
+ /*
+* There may not have been enough time for new data to be generated
+* since the last request. If the caller doesn't want to wait, let them
+* bail out. Otherwise, wait for the completion. If the new data has
+* already been generated, the completion should already be available.
+*/
+ if (!wait && !priv->present)
+ return 0;
- return 1;
-}
-
-static int timeriomem_rng_data_read(struct hwrng *rng, u32 *data)
-{
- struct timeriomem_rng_private_data *priv = to_rng_priv(rng);
- unsigned long cur;
- s32 delay;
+ wait_for_completion(&priv->completion);
- *data = readl(priv->io_base);
+ *(u32 *)data = readl(priv->io_base);
+ /*
+* Block any new callers until the RNG has had time to generate new
+* data.
+*/
cur = jiffies;
delay = cur - priv->expires;
@@ -154,9 +156,7 @@ static int timeriomem_rng_probe(struct platform_device
*pdev)
setup_timer(&priv->timer, timeriomem_rng_trigger, (unsigned long)priv);
priv->timeriomem_rng_ops.name = dev_name(&pdev->dev);
- priv->timeriomem_rng_ops.data_present = timeriomem_rng_data_present;
- priv->timeriomem_rng_ops.data_read = timeriomem_rng_data_read;
- priv->timeriomem_rng_ops.priv = (unsigned long)priv;
+ priv->timeriomem_rng_ops.read = timeriomem_rng_read;
priv->io_base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(priv->io_base)) {
--
2.12.2.715.g7642488e1d-goog
[PATCH v2 2/3] hw_random: timeriomem_rng: Shorten verbose type and variable names
No functional changes.
Signed-off-by: Rick Altherr
---
Changes in v2:
- Split type and variable renames into separate patch
drivers/char/hw_random/timeriomem-rng.c | 27 +--
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/char/hw_random/timeriomem-rng.c
b/drivers/char/hw_random/timeriomem-rng.c
index 17574452fd35..024bdff7999f 100644
--- a/drivers/char/hw_random/timeriomem-rng.c
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -31,7 +31,7 @@
#include
#include
-struct timeriomem_rng_private_data {
+struct timeriomem_rng_private {
void __iomem*io_base;
unsigned intexpires;
unsigned intperiod;
@@ -40,15 +40,14 @@ struct timeriomem_rng_private_data {
struct timer_list timer;
struct completion completion;
- struct hwrngtimeriomem_rng_ops;
+ struct hwrngrng_ops;
};
static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
size_t max, bool wait)
{
- struct timeriomem_rng_private_data *priv =
- container_of(hwrng, struct timeriomem_rng_private_data,
- timeriomem_rng_ops);
+ struct timeriomem_rng_private *priv =
+ container_of(hwrng, struct timeriomem_rng_private, rng_ops);
unsigned long cur;
s32 delay;
@@ -89,8 +88,8 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void
*data,
static void timeriomem_rng_trigger(unsigned long data)
{
- struct timeriomem_rng_private_data *priv
- = (struct timeriomem_rng_private_data *)data;
+ struct timeriomem_rng_private *priv
+ = (struct timeriomem_rng_private *)data;
priv->present = 1;
complete(&priv->completion);
@@ -99,7 +98,7 @@ static void timeriomem_rng_trigger(unsigned long data)
static int timeriomem_rng_probe(struct platform_device *pdev)
{
struct timeriomem_rng_data *pdata = pdev->dev.platform_data;
- struct timeriomem_rng_private_data *priv;
+ struct timeriomem_rng_private *priv;
struct resource *res;
int err = 0;
int period;
@@ -121,7 +120,7 @@ static int timeriomem_rng_probe(struct platform_device
*pdev)
/* Allocate memory for the device structure (and zero it) */
priv = devm_kzalloc(&pdev->dev,
- sizeof(struct timeriomem_rng_private_data), GFP_KERNEL);
+ sizeof(struct timeriomem_rng_private), GFP_KERNEL);
if (!priv)
return -ENOMEM;
@@ -155,8 +154,8 @@ static int timeriomem_rng_probe(struct platform_device
*pdev)
setup_timer(&priv->timer, timeriomem_rng_trigger, (unsigned long)priv);
- priv->timeriomem_rng_ops.name = dev_name(&pdev->dev);
- priv->timeriomem_rng_ops.read = timeriomem_rng_read;
+ priv->rng_ops.name = dev_name(&pdev->dev);
+ priv->rng_ops.read = timeriomem_rng_read;
priv->io_base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(priv->io_base)) {
@@ -164,7 +163,7 @@ static int timeriomem_rng_probe(struct platform_device
*pdev)
goto out_timer;
}
- err = hwrng_register(&priv->timeriomem_rng_ops);
+ err = hwrng_register(&priv->rng_ops);
if (err) {
dev_err(&pdev->dev, "problem registering\n");
goto out_timer;
@@ -182,9 +181,9 @@ static int timeriomem_rng_probe(struct platform_device
*pdev)
static int timeriomem_rng_remove(struct platform_device *pdev)
{
- struct timeriomem_rng_private_data *priv = platform_get_drvdata(pdev);
+ struct timeriomem_rng_private *priv = platform_get_drvdata(pdev);
- hwrng_unregister(&priv->timeriomem_rng_ops);
+ hwrng_unregister(&priv->rng_ops);
del_timer_sync(&priv->timer);
--
2.12.2.715.g7642488e1d-goog
[PATCH v2 0/3] hw_random: timeriomem_rng: Migrate to new API and improve performance
AST2400 can generate 32-bits of random data every 1us. Original driver was limited to one 32-bit read every jiffie due to deprecated API and use of timers. Migrating to new hwrng API and switching to hrtimers improves read performance of /dev/hwrng to 13Mb/s. Changes in v2: - Split API migration into separate patch - Split type and variable renames into separate patch - Split performance improvements into separate patch Rick Altherr (3): hw_random: Migrate timeriomem_rng to new API hw_random: timeriomem_rng: Shorten verbose type and variable names hw_random: timeriomem_rng: Improve performance for sub-jiffie update periods drivers/char/hw_random/timeriomem-rng.c | 157 1 file changed, 80 insertions(+), 77 deletions(-) -- 2.12.2.715.g7642488e1d-goog
[PATCH v2 3/3] hw_random: timeriomem_rng: Improve performance for sub-jiffie update periods
Some hardware RNGs provide a single register for obtaining random data.
Instead of signaling when new data is available, the reader must wait a
fixed amount of time between reads for new data to be generated.
timeriomem_rng implements this scheme with the period specified in
platform data or device tree. While the period is specified in
microseconds, the implementation used a standard timer which has a
minimum delay of 1 jiffie and caused a significant bottleneck for
devices that can update at 1us. By switching to an hrtimer, 1us periods
now only delay at most 2us per read.
Signed-off-by: Rick Altherr
---
Changes in v2:
- Split performance improvements into separate patch
drivers/char/hw_random/timeriomem-rng.c | 86 +
1 file changed, 45 insertions(+), 41 deletions(-)
diff --git a/drivers/char/hw_random/timeriomem-rng.c
b/drivers/char/hw_random/timeriomem-rng.c
index 024bdff7999f..a0faa5f05deb 100644
--- a/drivers/char/hw_random/timeriomem-rng.c
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -21,23 +21,24 @@
*/
#include
+#include
+#include
#include
#include
-#include
+#include
#include
#include
#include
#include
+#include
#include
-#include
struct timeriomem_rng_private {
void __iomem*io_base;
- unsigned intexpires;
- unsigned intperiod;
+ ktime_t period;
unsigned intpresent:1;
- struct timer_list timer;
+ struct hrtimer timer;
struct completion completion;
struct hwrngrng_ops;
@@ -48,10 +49,13 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void
*data,
{
struct timeriomem_rng_private *priv =
container_of(hwrng, struct timeriomem_rng_private, rng_ops);
- unsigned long cur;
- s32 delay;
+ int retval = 0;
+ int period_us = ktime_to_us(priv->period);
- /* The RNG provides 32-bit per read. Ensure there is enough space. */
+ /*
+* The RNG provides 32-bits per read. Ensure there is enough space for
+* at minimum one read.
+*/
if (max < sizeof(u32))
return 0;
@@ -66,33 +70,44 @@ static int timeriomem_rng_read(struct hwrng *hwrng, void
*data,
wait_for_completion(&priv->completion);
- *(u32 *)data = readl(priv->io_base);
+ do {
+ /*
+* After the first read, all additional reads will need to wait
+* for the RNG to generate new data. Since the period can have
+* a wide range of values (1us to 1s have been observed), allow
+* for 1% tolerance in the sleep time rather than a fixed value.
+*/
+ if (retval > 0)
+ usleep_range(period_us,
+ period_us + min(1, period_us / 100));
+
+ *(u32 *)data = readl(priv->io_base);
+ retval += sizeof(u32);
+ data += sizeof(u32);
+ max -= sizeof(u32);
+ } while (wait && max > sizeof(u32));
/*
* Block any new callers until the RNG has had time to generate new
* data.
*/
- cur = jiffies;
-
- delay = cur - priv->expires;
- delay = priv->period - (delay % priv->period);
-
- priv->expires = cur + delay;
priv->present = 0;
-
reinit_completion(&priv->completion);
- mod_timer(&priv->timer, priv->expires);
+ hrtimer_forward_now(&priv->timer, priv->period);
+ hrtimer_restart(&priv->timer);
- return 4;
+ return retval;
}
-static void timeriomem_rng_trigger(unsigned long data)
+static enum hrtimer_restart timeriomem_rng_trigger(struct hrtimer *timer)
{
struct timeriomem_rng_private *priv
- = (struct timeriomem_rng_private *)data;
+ = container_of(timer, struct timeriomem_rng_private, timer);
priv->present = 1;
complete(&priv->completion);
+
+ return HRTIMER_NORESTART;
}
static int timeriomem_rng_probe(struct platform_device *pdev)
@@ -140,43 +155,33 @@ static int timeriomem_rng_probe(struct platform_device
*pdev)
period = pdata->period;
}
- priv->period = usecs_to_jiffies(period);
- if (priv->period < 1) {
- dev_err(&pdev->dev, "period is less than one jiffy\n");
- return -EINVAL;
- }
-
- priv->expires = jiffies;
- priv->present = 1;
-
+ priv->period = ns_to_ktime(period * NSEC_PER_USEC);
init_completion(&priv->completion);
- complete(&priv->completion);
-
- setup_timer(&priv->timer, timeriomem_rng_trigger, (unsigned long)priv);
+ hrtimer_init(&
[PATCH v1] hw_random: Fix timeriomem_rng for sub-jiffie update periods
Some hardware RNGs provide a single register for obtaining random data.
Instead of signaling when new data is available, the reader must wait a
fixed amount of time between reads for new data to be generated.
timeriomem_rng implements this scheme with the period specified in
platform data or device tree. While the period is specified in
microseconds, the implementation used a standard timer which has a
minimum delay of 1 jiffie and caused a significant bottleneck for
devices that can update at 1us. By switching to an hrtimer, 1us periods
now only delay at most 2us per read.
Migrated to new hw_random API while I in this driver.
Signed-off-by: Rick Altherr
---
drivers/char/hw_random/timeriomem-rng.c | 153
1 file changed, 75 insertions(+), 78 deletions(-)
diff --git a/drivers/char/hw_random/timeriomem-rng.c
b/drivers/char/hw_random/timeriomem-rng.c
index cf37db263ecd..7482de2ca71c 100644
--- a/drivers/char/hw_random/timeriomem-rng.c
+++ b/drivers/char/hw_random/timeriomem-rng.c
@@ -20,90 +20,99 @@
* TODO: add support for reading sizes other than 32bits and masking
*/
-#include
-#include
-#include
-#include
+#include
+#include
+#include
#include
#include
+#include
+#include
+#include
+#include
#include
+#include
#include
-#include
-#include
-#include
-#include
-struct timeriomem_rng_private_data {
+struct timeriomem_rng_private {
void __iomem*io_base;
- unsigned intexpires;
- unsigned intperiod;
+ ktime_t period;
unsigned intpresent:1;
- struct timer_list timer;
+ struct hrtimer timer;
struct completion completion;
- struct hwrngtimeriomem_rng_ops;
+ struct hwrngrng_ops;
};
-#define to_rng_priv(rng) \
- ((struct timeriomem_rng_private_data *)rng->priv)
-
-/*
- * have data return 1, however return 0 if we have nothing
- */
-static int timeriomem_rng_data_present(struct hwrng *rng, int wait)
+static int timeriomem_rng_read(struct hwrng *hwrng, void *data,
+ size_t max, bool wait)
{
- struct timeriomem_rng_private_data *priv = to_rng_priv(rng);
-
- if (!wait || priv->present)
- return priv->present;
+ struct timeriomem_rng_private *priv =
+ container_of(hwrng, struct timeriomem_rng_private, rng_ops);
+ int retval = 0;
+ int period_us = ktime_to_us(priv->period);
+
+ /*
+* There may not have been enough time for new data to be generated
+* since the last request. If the caller doesn't want to wait, let them
+* bail out. Otherwise, wait for the completion. If the new data has
+* already been generated, the completion should already be available.
+*/
+ if (!wait && !priv->present)
+ return 0;
wait_for_completion(&priv->completion);
- return 1;
-}
-
-static int timeriomem_rng_data_read(struct hwrng *rng, u32 *data)
-{
- struct timeriomem_rng_private_data *priv = to_rng_priv(rng);
- unsigned long cur;
- s32 delay;
-
- *data = readl(priv->io_base);
-
- cur = jiffies;
-
- delay = cur - priv->expires;
- delay = priv->period - (delay % priv->period);
-
- priv->expires = cur + delay;
+ do {
+ /*
+* After the first read, all additional reads will need to wait
+* for the RNG to generate new data. Since the period can have
+* a wide range of values (1us to 1s have been observed), allow
+* for 1% tolerance in the sleep time rather than a fixed value.
+*/
+ if (retval > 0)
+ usleep_range(period_us,
+ period_us + min(1, period_us / 100));
+
+ *(u32 *)data = readl(priv->io_base);
+ retval += sizeof(u32);
+ data += sizeof(u32);
+ max -= sizeof(u32);
+ } while (wait && max > sizeof(u32));
+
+ /*
+* Block any new callers until the RNG has had time to generate new
+* data.
+*/
priv->present = 0;
-
reinit_completion(&priv->completion);
- mod_timer(&priv->timer, priv->expires);
+ hrtimer_forward_now(&priv->timer, priv->period);
+ hrtimer_restart(&priv->timer);
- return 4;
+ return retval;
}
-static void timeriomem_rng_trigger(unsigned long data)
+static enum hrtimer_restart timeriomem_rng_trigger(struct hrtimer *timer)
{
- struct timeriomem_rng_private_data *priv
- = (struct timeriomem_rng_private_data *)data;
+ struct timeriomem_rng_private *priv
+ = container_of(timer
[PATCH v5 2/2] iio: Aspeed ADC
Aspeed BMC SoCs include a 16 channel, 10-bit ADC. Low and high threshold
interrupts are supported by the hardware but are not currently implemented.
Signed-off-by: Rick Altherr
---
Changes in v5:
- Return EINVAL for unimplemented read/write channel infos.
- Return EPERM for write attempts to read-only channel infos.
Changes in v4:
- Avoid copying per-model data to per-instance data
Changes in v3:
- Drop model numbers from description as same IP is used in every generation
- Remove unused macros
- Shorten macro prefix to just ASPEED_
- Remove extraneous whitespace
- Rename 2nd argument of ASPEED_CHAN() to clarify purpose
- Use existing macros in place of magic constants
- Remove unnecessary logging in failure conditions during probe
- Disable ADC hardware during remove()
- Add a NULL terminator to of_match_table
- Add per-model data to accomodate slight variations (vref, min/max sample rate)
- Added missing Kconfig depends on COMMON_CLK
Changes in v2:
- Rewritten as an IIO device
- Renamed register macros to describe the register's purpose
- Replaced awkward reading of 16-bit data registers with readw()
- Added Kconfig dependency on COMPILE_TEST
drivers/iio/adc/Kconfig | 11 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/aspeed_adc.c | 294 +++
3 files changed, 306 insertions(+)
create mode 100644 drivers/iio/adc/aspeed_adc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 2268a6fb9865..236d05f99e80 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -130,6 +130,17 @@ config AD799X
To compile this driver as a module, choose M here: the module will be
called ad799x.
+config ASPEED_ADC
+ tristate "Aspeed ADC"
+ depends on ARCH_ASPEED || COMPILE_TEST
+ depends on COMMON_CLK
+ help
+ If you say yes here you get support for the ADC included in Aspeed
+ BMC SoCs.
+
+ To compile this driver as a module, choose M here: the module will be
+ called aspeed_adc.
+
config AT91_ADC
tristate "Atmel AT91 ADC"
depends on ARCH_AT91
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 73dbe399f894..306f10ffca74 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AD7791) += ad7791.o
obj-$(CONFIG_AD7793) += ad7793.o
obj-$(CONFIG_AD7887) += ad7887.o
obj-$(CONFIG_AD799X) += ad799x.o
+obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
obj-$(CONFIG_AT91_ADC) += at91_adc.o
obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
new file mode 100644
index ..2283ed202194
--- /dev/null
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -0,0 +1,294 @@
+/*
+ * Aspeed AST2400/2500 ADC
+ *
+ * Copyright (C) 2017 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#define ASPEED_RESOLUTION_BITS 10
+#define ASPEED_CLOCKS_PER_SAMPLE 12
+
+#define ASPEED_REG_ENGINE_CONTROL 0x00
+#define ASPEED_REG_INTERRUPT_CONTROL 0x04
+#define ASPEED_REG_VGA_DETECT_CONTROL 0x08
+#define ASPEED_REG_CLOCK_CONTROL 0x0C
+#define ASPEED_REG_MAX 0xC0
+
+#define ASPEED_OPERATION_MODE_POWER_DOWN (0x0 << 1)
+#define ASPEED_OPERATION_MODE_STANDBY (0x1 << 1)
+#define ASPEED_OPERATION_MODE_NORMAL (0x7 << 1)
+
+#define ASPEED_ENGINE_ENABLE BIT(0)
+
+struct aspeed_adc_model_data {
+ const char *model_name;
+ unsigned int min_sampling_rate; // Hz
+ unsigned int max_sampling_rate; // Hz
+ unsigned int vref_voltage; // mV
+};
+
+struct aspeed_adc_data {
+ struct device *dev;
+ void __iomem*base;
+ spinlock_t clk_lock;
+ struct clk_hw *clk_prescaler;
+ struct clk_hw *clk_scaler;
+};
+
+#define ASPEED_CHAN(_idx, _data_reg_addr) {\
+ .type = IIO_VOLTAGE,\
+ .indexed = 1, \
+ .channel = (_idx), \
+ .address = (_data_reg_addr),\
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+}
+
+static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
+ ASPEED_CHAN(0, 0x10),
+ ASPEED_CHAN(1, 0x12),
+ ASPEED_CHAN(2, 0x14),
+ ASPEED_CHAN(3, 0x16),
+ ASPEED
[PATCH v5 1/2] Documentation: dt-bindings: Document bindings for Aspeed ADC
Signed-off-by: Rick Altherr
---
Changes in v5: None
Changes in v4: None
Changes in v3:
- Consistently write hex contstants with lowercase letters
- Drop model numbers from description as same IP is used in every generation
Changes in v2:
- Rewritten as an IIO ADC device
.../devicetree/bindings/iio/adc/aspeed_adc.txt | 20
1 file changed, 20 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
new file mode 100644
index ..674e133b7cd7
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
@@ -0,0 +1,20 @@
+Aspeed ADC
+
+This device is a 10-bit converter for 16 voltage channels. All inputs are
+single ended.
+
+Required properties:
+- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
+- reg: memory window mapping address and length
+- clocks: Input clock used to derive the sample clock. Expected to be the
+ SoC's APB clock.
+- #io-channel-cells: Must be set to <1> to indicate channels are selected
+ by index.
+
+Example:
+ adc@1e6e9000 {
+ compatible = "aspeed,ast2400-adc";
+ reg = <0x1e6e9000 0xb0>;
+ clocks = <&clk_apb>;
+ #io-channel-cells = <1>;
+ };
--
2.12.2.564.g063fe858b8-goog
Re: [PATCH v4 2/2] iio: Aspeed ADC
On Sat, Mar 25, 2017 at 10:32 AM, Jonathan Cameron wrote: > On 23/03/17 18:41, Rick Altherr wrote: >> Aspeed BMC SoCs include a 16 channel, 10-bit ADC. Low and high threshold >> interrupts are supported by the hardware but are not currently implemented. >> >> Signed-off-by: Rick Altherr > Nice little driver. > > I'd like input from clk maintainers on the clk stuff though before applying. > Michael, Stephen MAINTAINERS suggests this is you two. Any comments? > > Thanks, > > Jonathan > >> --- >> >> Changes in v4: >> - Avoid copying per-model data to per-instance data >> >> Changes in v3: >> - Drop model numbers from description as same IP is used in every generation >> - Remove unused macros >> - Shorten macro prefix to just ASPEED_ >> - Remove extraneous whitespace >> - Rename 2nd argument of ASPEED_CHAN() to clarify purpose >> - Use existing macros in place of magic constants >> - Remove unnecessary logging in failure conditions during probe >> - Disable ADC hardware during remove() >> - Add a NULL terminator to of_match_table >> - Add per-model data to accomodate slight variations (vref, min/max sample >> rate) >> - Added missing Kconfig depends on COMMON_CLK >> >> Changes in v2: >> - Rewritten as an IIO device >> - Renamed register macros to describe the register's purpose >> - Replaced awkward reading of 16-bit data registers with readw() >> - Added Kconfig dependency on COMPILE_TEST >> >> drivers/iio/adc/Kconfig | 11 ++ >> drivers/iio/adc/Makefile | 1 + >> drivers/iio/adc/aspeed_adc.c | 284 >> +++ >> 3 files changed, 296 insertions(+) >> create mode 100644 drivers/iio/adc/aspeed_adc.c >> >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >> index 2268a6fb9865..236d05f99e80 100644 >> --- a/drivers/iio/adc/Kconfig >> +++ b/drivers/iio/adc/Kconfig >> @@ -130,6 +130,17 @@ config AD799X >> To compile this driver as a module, choose M here: the module will be >> called ad799x. >> >> +config ASPEED_ADC >> + tristate "Aspeed ADC" >> + depends on ARCH_ASPEED || COMPILE_TEST >> + depends on COMMON_CLK >> + help >> + If you say yes here you get support for the ADC included in Aspeed >> + BMC SoCs. >> + >> + To compile this driver as a module, choose M here: the module will be >> + called aspeed_adc. >> + >> config AT91_ADC >> tristate "Atmel AT91 ADC" >> depends on ARCH_AT91 >> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile >> index 73dbe399f894..306f10ffca74 100644 >> --- a/drivers/iio/adc/Makefile >> +++ b/drivers/iio/adc/Makefile >> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7791) += ad7791.o >> obj-$(CONFIG_AD7793) += ad7793.o >> obj-$(CONFIG_AD7887) += ad7887.o >> obj-$(CONFIG_AD799X) += ad799x.o >> +obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o >> obj-$(CONFIG_AT91_ADC) += at91_adc.o >> obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o >> obj-$(CONFIG_AXP288_ADC) += axp288_adc.o >> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c >> new file mode 100644 >> index ..e051fdf8ecbd >> --- /dev/null >> +++ b/drivers/iio/adc/aspeed_adc.c >> @@ -0,0 +1,284 @@ >> +/* >> + * Aspeed AST2400/2500 ADC >> + * >> + * Copyright (C) 2017 Google, Inc. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> + >> +#define ASPEED_RESOLUTION_BITS 10 >> +#define ASPEED_CLOCKS_PER_SAMPLE 12 >> + >> +#define ASPEED_REG_ENGINE_CONTROL0x00 >> +#define ASPEED_REG_INTERRUPT_CONTROL 0x04 >> +#define ASPEED_REG_VGA_DETECT_CONTROL0x08 >> +#define ASPEED_REG_CLOCK_CONTROL 0x0C >> +#define ASPEED_REG_MAX 0xC0 >> + >> +#define ASPEED_OPERATION_MODE_POWER_DOWN (0x0 << 1) >> +#define ASPEED_OPERATION_MODE_STANDBY(0x1 << 1) >> +#define ASPEED_OPERATION_MODE
[PATCH v4 2/2] iio: Aspeed ADC
Aspeed BMC SoCs include a 16 channel, 10-bit ADC. Low and high threshold
interrupts are supported by the hardware but are not currently implemented.
Signed-off-by: Rick Altherr
---
Changes in v4:
- Avoid copying per-model data to per-instance data
Changes in v3:
- Drop model numbers from description as same IP is used in every generation
- Remove unused macros
- Shorten macro prefix to just ASPEED_
- Remove extraneous whitespace
- Rename 2nd argument of ASPEED_CHAN() to clarify purpose
- Use existing macros in place of magic constants
- Remove unnecessary logging in failure conditions during probe
- Disable ADC hardware during remove()
- Add a NULL terminator to of_match_table
- Add per-model data to accomodate slight variations (vref, min/max sample rate)
- Added missing Kconfig depends on COMMON_CLK
Changes in v2:
- Rewritten as an IIO device
- Renamed register macros to describe the register's purpose
- Replaced awkward reading of 16-bit data registers with readw()
- Added Kconfig dependency on COMPILE_TEST
drivers/iio/adc/Kconfig | 11 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/aspeed_adc.c | 284 +++
3 files changed, 296 insertions(+)
create mode 100644 drivers/iio/adc/aspeed_adc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 2268a6fb9865..236d05f99e80 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -130,6 +130,17 @@ config AD799X
To compile this driver as a module, choose M here: the module will be
called ad799x.
+config ASPEED_ADC
+ tristate "Aspeed ADC"
+ depends on ARCH_ASPEED || COMPILE_TEST
+ depends on COMMON_CLK
+ help
+ If you say yes here you get support for the ADC included in Aspeed
+ BMC SoCs.
+
+ To compile this driver as a module, choose M here: the module will be
+ called aspeed_adc.
+
config AT91_ADC
tristate "Atmel AT91 ADC"
depends on ARCH_AT91
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 73dbe399f894..306f10ffca74 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AD7791) += ad7791.o
obj-$(CONFIG_AD7793) += ad7793.o
obj-$(CONFIG_AD7887) += ad7887.o
obj-$(CONFIG_AD799X) += ad799x.o
+obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
obj-$(CONFIG_AT91_ADC) += at91_adc.o
obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
new file mode 100644
index ..e051fdf8ecbd
--- /dev/null
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -0,0 +1,284 @@
+/*
+ * Aspeed AST2400/2500 ADC
+ *
+ * Copyright (C) 2017 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#define ASPEED_RESOLUTION_BITS 10
+#define ASPEED_CLOCKS_PER_SAMPLE 12
+
+#define ASPEED_REG_ENGINE_CONTROL 0x00
+#define ASPEED_REG_INTERRUPT_CONTROL 0x04
+#define ASPEED_REG_VGA_DETECT_CONTROL 0x08
+#define ASPEED_REG_CLOCK_CONTROL 0x0C
+#define ASPEED_REG_MAX 0xC0
+
+#define ASPEED_OPERATION_MODE_POWER_DOWN (0x0 << 1)
+#define ASPEED_OPERATION_MODE_STANDBY (0x1 << 1)
+#define ASPEED_OPERATION_MODE_NORMAL (0x7 << 1)
+
+#define ASPEED_ENGINE_ENABLE BIT(0)
+
+struct aspeed_adc_model_data {
+ const char *model_name;
+ unsigned int min_sampling_rate; // Hz
+ unsigned int max_sampling_rate; // Hz
+ unsigned int vref_voltage; // mV
+};
+
+struct aspeed_adc_data {
+ struct device *dev;
+ void __iomem*base;
+ spinlock_t clk_lock;
+ struct clk_hw *clk_prescaler;
+ struct clk_hw *clk_scaler;
+};
+
+#define ASPEED_CHAN(_idx, _data_reg_addr) {\
+ .type = IIO_VOLTAGE,\
+ .indexed = 1, \
+ .channel = (_idx), \
+ .address = (_data_reg_addr),\
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+}
+
+static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
+ ASPEED_CHAN(0, 0x10),
+ ASPEED_CHAN(1, 0x12),
+ ASPEED_CHAN(2, 0x14),
+ ASPEED_CHAN(3, 0x16),
+ ASPEED_CHAN(4, 0x18),
+ ASPEED_CHAN(5, 0x1A),
+ ASPEED_CHAN(6, 0x1C),
+ ASPEED_CHAN(7, 0x1E),
+ ASPEED
[PATCH v4 1/2] Documentation: dt-bindings: Document bindings for Aspeed ADC
Signed-off-by: Rick Altherr
---
Changes in v4: None
Changes in v3:
- Consistently write hex contstants with lowercase letters
- Drop model numbers from description as same IP is used in every generation
Changes in v2:
- Rewritten as an IIO ADC device
.../devicetree/bindings/iio/adc/aspeed_adc.txt | 20
1 file changed, 20 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
new file mode 100644
index ..674e133b7cd7
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
@@ -0,0 +1,20 @@
+Aspeed ADC
+
+This device is a 10-bit converter for 16 voltage channels. All inputs are
+single ended.
+
+Required properties:
+- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
+- reg: memory window mapping address and length
+- clocks: Input clock used to derive the sample clock. Expected to be the
+ SoC's APB clock.
+- #io-channel-cells: Must be set to <1> to indicate channels are selected
+ by index.
+
+Example:
+ adc@1e6e9000 {
+ compatible = "aspeed,ast2400-adc";
+ reg = <0x1e6e9000 0xb0>;
+ clocks = <&clk_apb>;
+ #io-channel-cells = <1>;
+ };
--
2.12.1.500.gab5fba24ee-goog
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
On Thu, Mar 23, 2017 at 12:52 AM, Quentin Schulz
wrote:
> Hi,
>
> On 22/03/2017 21:46, Rick Altherr wrote:
>> On Wed, Mar 22, 2017 at 12:21 AM, Quentin Schulz
>> wrote:
>>> Hi,
>>>
>>> On 21/03/2017 21:48, Rick Altherr wrote:
>>>> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. Low
>>>> and high threshold interrupts are supported by the hardware but are not
>>>> currently implemented.
>>>>
>>>> Signed-off-by: Rick Altherr
>>>> ---
>>>>
>>>> Changes in v2:
>>>> - Rewritten as an IIO device
>>>> - Renamed register macros to describe the register's purpose
>>>> - Replaced awkward reading of 16-bit data registers with readw()
>>>> - Added Kconfig dependency on COMPILE_TEST
>>>>
> [...]
>>>> +#define ASPEED_ADC_CHAN(_idx, _addr) { \
>>>> + .type = IIO_VOLTAGE,\
>>>> + .indexed = 1, \
>>>> + .channel = (_idx), \
>>>> + .address = (_addr), \
>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>>>> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>>> +}
>>>> +
>>>> +static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
>>>> + ASPEED_ADC_CHAN(0, 0x10),
>>>> + ASPEED_ADC_CHAN(1, 0x12),
>>>> + ASPEED_ADC_CHAN(2, 0x14),
>>>> + ASPEED_ADC_CHAN(3, 0x16),
>>>> + ASPEED_ADC_CHAN(4, 0x18),
>>>> + ASPEED_ADC_CHAN(5, 0x1A),
>>>> + ASPEED_ADC_CHAN(6, 0x1C),
>>>> + ASPEED_ADC_CHAN(7, 0x1E),
>>>> + ASPEED_ADC_CHAN(8, 0x20),
>>>> + ASPEED_ADC_CHAN(9, 0x22),
>>>> + ASPEED_ADC_CHAN(10, 0x24),
>>>> + ASPEED_ADC_CHAN(11, 0x26),
>>>> + ASPEED_ADC_CHAN(12, 0x28),
>>>> + ASPEED_ADC_CHAN(13, 0x2A),
>>>> + ASPEED_ADC_CHAN(14, 0x2C),
>>>> + ASPEED_ADC_CHAN(15, 0x2E),
>>>
>>> It would make sense to name the registers (the _addr parameter of your
>>> macro) so it's easier to understand what it refers to.
>>>
>>
>> I agree with Joel on this. There isn't really a better name than
>> ADC_CHAN_0_DATA. I'll change the macro parameter to _data_reg_addr to
>> make that clearer.
>>
>
> Is it the name in the datasheet?
>
No, the datasheet has such helpful register names as ADC10 where 0x10
is the offset in the register map.
> [...]
>>>> + indio_dev->name = dev_name(&pdev->dev);
>>>
>>> This isn't good practice (cf.: https://lkml.org/lkml/2017/1/28/145 end
>>> of the mail in the probe function). Better name it aspeed-adc or even
>>> better to have a different name per compatible: ast2400-adc or ast2500-adc.
>>
>> Ack. Will use aspeed-adc to avoid parsing the compatible match and
>> stripping the 'aspeed,' prefix.
>>
>
> You don't need to parse the compatible match. You could use the data
> void pointer in your struct of_device_id
> (http://lxr.free-electrons.com/source/include/linux/mod_devicetable.h#L234)
> like it's done here: https://lkml.org/lkml/2017/3/21/675
> (sun4i_gpadc_of_id).
>
I figured that out a bit later and did so in v3 I sent out last night.
> Note: please reply to all :)
Whoops. Looks like I did that for all the replies I did yesterday.
>
> Quentin
>
> --
> Quentin Schulz, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
Restoring the list after an accidental direct reply.
On Wed, Mar 22, 2017 at 2:32 PM, Rick Altherr wrote:
> On Wed, Mar 22, 2017 at 2:47 AM, Joel Stanley wrote:
>> On Wed, Mar 22, 2017 at 7:18 AM, Rick Altherr wrote:
>>> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. Low
>>> and high threshold interrupts are supported by the hardware but are not
>>> currently implemented.
>>>
>>> Signed-off-by: Rick Altherr
>>
>> Looks good Rick. I gave it a review from the perspective of the Aspeed
>> soc. I also gave it a spin on the Aspeed AST2500 EVB which mostly
>> worked, but uncovered some things that need addressing.
>>
>> My device tree additions looked like this:
>>
>> adc: adc@1e6e9000 {
>> compatible = "aspeed,ast2500-adc";
>> reg = <0x1e6e9000 0xb0>;
>> clocks = <&clk_apb>;
>> #io-channel-cells = <1>;
>>
>> pinctrl-names = "default";
>> pinctrl-0 = <&pinctrl_adc0_default>;
>
> This shouldn't be strictly necessary as the ADCs are the default
> function for the pins they are available on.
>
>> };
>>
>> iio-hwmon {
>> compatible = "iio-hwmon";
>> io-channels = <&adc 0>;
>> };
>
> This is necessary to make it show up as hwmon. You can see the iio
> device directly at /sys/bus/iio/devices/.
>
>>
>> I got this output from lm-sensors when booted:
>>
>> iio_hwmon-isa-
>> Adapter: ISA adapter
>> in1: +1.28 V
>>
>> I then wired up ADC0 to ADC_12V_TW on the EVB. The above changed to:
>>
>> iio_hwmon-isa-
>> Adapter: ISA adapter
>> in1: +1.80 V
>>
>> ADC_12V_TW is the 12V rail sampled through a voltage divider. The
>> voltage should be: 12 * 680 / ( 5600 + 680) = 1.299
>>
>> cat /sys/bus/iio/devices/iio:device0/in_voltage1_raw
>> 738
>>
>> 738 / 1023 * 1.8 = 1.2975
>>
>> Looks like the first channel is working! However our reference is
>> incorrect. Your driver has ASPEED_ADC_REF_VOLTAGE but doesn't use it.
>> It does hardcode 2500 in the aspeed_adc_read_raw callback:
>>
>> case IIO_CHAN_INFO_SCALE:
>> *val = 2500; // mV
>> *val2 = 10;
>> return IIO_VAL_FRACTIONAL_LOG2;
>>
>> Should this value the the constant you define?
>>
>> Regardless, I don't think the reference voltage should be a constant.
>> This is going to vary from system to system. Can we put it in the
>> device tree? I notice other devices have vref-supply in their
>> bindings.
>>
>
> Good catch. AST2500 uses a 1.8Vref while AST2400 uses a 2.5Vref. I
> decided against putting a vref-supply in the device tree as neither
> part allows for an external reference. Both the 1.8V and 2.5V are
> fixed, internal references. It just happens that the reference
> voltage changes with the chip generation. Looking at the data sheet
> more, I also see the min/max sampling rate has changed for AST2500 as
> well. This is probably best handled by attaching data to the
> of_device_ids in the of_match_table. That would solve all of these
> per-chip variations.
>
>> I noticed that in_voltage_scale is writable. However, it did not
>> accept any of the values I give it. Is this because we do not handle
>> it in aspeed_adc_write_raw?
>>
>
> I think all attributes become writable because I implement write_raw
> even though only the sample frequency is actually writable. I'm of
> two minds on allowing the scale to be written. If the user knows
> there is a voltage divider before the ADC, why don't they apply that
> correction in userspace? On the other hand, the existence of the
> voltage divider _could_ be specified in the device tree and the kernel
> takes care of the scaling entirely. The latter seems like a
> general-purpose concept but I couldn't find an existing binding for
> specifying it. I decided to start with the minimal functionality of
> only reporting the ADC's natural scaling and letting userspace deal
> with any additional information it has.
>
>> I suggest we add the reference in the device tree bindings, and also
>> allow the value to be updated from userspace.
>>
>>> ---
>>>
>>> Changes in v2:
>>> - Rewritten as an IIO device
>>> - Renamed register macros to describe the register's purpose
>>> - Replaced awkward reading of 16-bit data registers with readw()
Re: [PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
Restoring the list after an accidental direct reply. On Wed, Mar 22, 2017 at 1:30 PM, Rick Altherr wrote: > On Tue, Mar 21, 2017 at 2:14 PM, Peter Meerwald-Stadler > wrote: >> >>> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. Low >>> and high threshold interrupts are supported by the hardware but are not >>> currently implemented. >> >> comments below >> link to a datasheet would be nice >> > > No public datasheet is available. I've tried. > >>> Signed-off-by: Rick Altherr >>> --- >>> >>> Changes in v2: >>> - Rewritten as an IIO device >>> - Renamed register macros to describe the register's purpose >>> - Replaced awkward reading of 16-bit data registers with readw() >>> - Added Kconfig dependency on COMPILE_TEST >>> >>> drivers/iio/adc/Kconfig | 10 ++ >>> drivers/iio/adc/Makefile | 1 + >>> drivers/iio/adc/aspeed_adc.c | 271 >>> +++ >>> 3 files changed, 282 insertions(+) >>> create mode 100644 drivers/iio/adc/aspeed_adc.c >>> >>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >>> index 2268a6fb9865..9672d799a3fb 100644 >>> --- a/drivers/iio/adc/Kconfig >>> +++ b/drivers/iio/adc/Kconfig >>> @@ -130,6 +130,16 @@ config AD799X >>> To compile this driver as a module, choose M here: the module will >>> be >>> called ad799x. >>> >>> +config ASPEED_ADC >>> + tristate "Aspeed AST2400/AST2500 ADC" >>> + depends on ARCH_ASPEED || COMPILE_TEST >>> + help >>> + If you say yes here you get support for the Aspeed AST2400/AST2500 >>> + ADC. >>> + >>> + To compile this driver as a module, choose M here: the module will >>> be >>> + called aspeed_adc. >>> + >>> config AT91_ADC >>> tristate "Atmel AT91 ADC" >>> depends on ARCH_AT91 >>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile >>> index 73dbe399f894..306f10ffca74 100644 >>> --- a/drivers/iio/adc/Makefile >>> +++ b/drivers/iio/adc/Makefile >>> @@ -14,6 +14,7 @@ obj-$(CONFIG_AD7791) += ad7791.o >>> obj-$(CONFIG_AD7793) += ad7793.o >>> obj-$(CONFIG_AD7887) += ad7887.o >>> obj-$(CONFIG_AD799X) += ad799x.o >>> +obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o >>> obj-$(CONFIG_AT91_ADC) += at91_adc.o >>> obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o >>> obj-$(CONFIG_AXP288_ADC) += axp288_adc.o >>> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c >>> new file mode 100644 >>> index ..9220909aefd4 >>> --- /dev/null >>> +++ b/drivers/iio/adc/aspeed_adc.c >>> @@ -0,0 +1,271 @@ >>> +/* >>> + * Aspeed AST2400/2500 ADC >>> + * >>> + * Copyright (C) 2017 Google, Inc. >>> + * >>> + * This program is free software; you can redistribute it and/or modify it >>> + * under the terms and conditions of the GNU General Public License, >>> + * version 2, as published by the Free Software Foundation. >>> + * >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include >>> +#include >>> + >>> +#define ASPEED_ADC_NUM_CHANNELS 16 >> >> _NUM_CHANNELS is not used, remove > > Ack > >> >>> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */ >> >> should be used below > > Ack > >> >>> +#define ASPEED_ADC_RESOLUTION_BITS 10 >> >> should be used below > > Ack > >> >>> +#define ASPEED_ADC_MIN_SAMP_RATE 1 >>> +#define ASPEED_ADC_MAX_SAMP_RATE 50 >>> +#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12 >>> + >>> +#define ASPEED_ADC_REG_ENGINE_CONTROL0x00 >>> +#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04 >>> +#define ASPEED_ADC_REG_VGA_DETECT_CONTROL0x08 >>> +#define ASPEED_ADC_REG_CLOCK_CONTROL 0x0C >>> +#define ASPEED_ADC_REG_MAX 0xC0 >>> + >>> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 &
[PATCH v3 1/2] Documentation: dt-bindings: Document bindings for Aspeed ADC
Signed-off-by: Rick Altherr
---
Changes in v3:
- Consistently write hex contstants with lowercase letters
- Drop model numbers from description as same IP is used in every generation
Changes in v2:
- Rewritten as an IIO ADC device
.../devicetree/bindings/iio/adc/aspeed_adc.txt | 20
1 file changed, 20 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
new file mode 100644
index ..674e133b7cd7
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
@@ -0,0 +1,20 @@
+Aspeed ADC
+
+This device is a 10-bit converter for 16 voltage channels. All inputs are
+single ended.
+
+Required properties:
+- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
+- reg: memory window mapping address and length
+- clocks: Input clock used to derive the sample clock. Expected to be the
+ SoC's APB clock.
+- #io-channel-cells: Must be set to <1> to indicate channels are selected
+ by index.
+
+Example:
+ adc@1e6e9000 {
+ compatible = "aspeed,ast2400-adc";
+ reg = <0x1e6e9000 0xb0>;
+ clocks = <&clk_apb>;
+ #io-channel-cells = <1>;
+ };
--
2.12.1.500.gab5fba24ee-goog
[PATCH v3 2/2] iio: Aspeed ADC
Aspeed BMC SoCs include a 16 channel, 10-bit ADC. Low and high threshold
interrupts are supported by the hardware but are not currently implemented.
Signed-off-by: Rick Altherr
---
Changes in v3:
- Drop model numbers from description as same IP is used in every generation
- Remove unused macros
- Shorten macro prefix to just ASPEED_
- Remove extraneous whitespace
- Rename 2nd argument of ASPEED_CHAN() to clarify purpose
- Use existing macros in place of magic constants
- Remove unnecessary logging in failure conditions during probe
- Disable ADC hardware during remove()
- Add a NULL terminator to of_match_table
- Add per-model data to accomodate slight variations (vref, min/max sample rate)
- Added missing Kconfig depends on COMMON_CLK
Changes in v2:
- Rewritten as an IIO device
- Renamed register macros to describe the register's purpose
- Replaced awkward reading of 16-bit data registers with readw()
- Added Kconfig dependency on COMPILE_TEST
drivers/iio/adc/Kconfig | 11 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/aspeed_adc.c | 285 +++
3 files changed, 297 insertions(+)
create mode 100644 drivers/iio/adc/aspeed_adc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 2268a6fb9865..236d05f99e80 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -130,6 +130,17 @@ config AD799X
To compile this driver as a module, choose M here: the module will be
called ad799x.
+config ASPEED_ADC
+ tristate "Aspeed ADC"
+ depends on ARCH_ASPEED || COMPILE_TEST
+ depends on COMMON_CLK
+ help
+ If you say yes here you get support for the ADC included in Aspeed
+ BMC SoCs.
+
+ To compile this driver as a module, choose M here: the module will be
+ called aspeed_adc.
+
config AT91_ADC
tristate "Atmel AT91 ADC"
depends on ARCH_AT91
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 73dbe399f894..306f10ffca74 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AD7791) += ad7791.o
obj-$(CONFIG_AD7793) += ad7793.o
obj-$(CONFIG_AD7887) += ad7887.o
obj-$(CONFIG_AD799X) += ad799x.o
+obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
obj-$(CONFIG_AT91_ADC) += at91_adc.o
obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
new file mode 100644
index ..09043bbb6a60
--- /dev/null
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -0,0 +1,285 @@
+/*
+ * Aspeed AST2400/2500 ADC
+ *
+ * Copyright (C) 2017 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#define ASPEED_RESOLUTION_BITS 10
+#define ASPEED_CLOCKS_PER_SAMPLE 12
+
+#define ASPEED_REG_ENGINE_CONTROL 0x00
+#define ASPEED_REG_INTERRUPT_CONTROL 0x04
+#define ASPEED_REG_VGA_DETECT_CONTROL 0x08
+#define ASPEED_REG_CLOCK_CONTROL 0x0C
+#define ASPEED_REG_MAX 0xC0
+
+#define ASPEED_OPERATION_MODE_POWER_DOWN (0x0 << 1)
+#define ASPEED_OPERATION_MODE_STANDBY (0x1 << 1)
+#define ASPEED_OPERATION_MODE_NORMAL (0x7 << 1)
+
+#define ASPEED_ENGINE_ENABLE BIT(0)
+
+struct aspeed_adc_model_data {
+ const char *model_name;
+ unsigned int min_sampling_rate; // Hz
+ unsigned int max_sampling_rate; // Hz
+ unsigned int vref_voltage; // mV
+};
+
+struct aspeed_adc_data {
+ struct device *dev;
+ void __iomem*base;
+ unsigned intmin_sampling_rate;
+ unsigned intmax_sampling_rate;
+ unsigned intvref_voltage;
+ spinlock_t clk_lock;
+ struct clk_hw *clk_prescaler;
+ struct clk_hw *clk_scaler;
+};
+
+#define ASPEED_CHAN(_idx, _data_reg_addr) {\
+ .type = IIO_VOLTAGE,\
+ .indexed = 1, \
+ .channel = (_idx), \
+ .address = (_data_reg_addr),\
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+}
+
+static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
+ ASPEED_CHAN(0, 0x10),
+ ASPEED_CHAN(1, 0x12),
+ ASPEED_CHAN(2, 0x14),
+ ASPEED_CHAN(3, 0x16),
+ ASPEED_CHAN(4, 0x18),
+ ASPEED_CHAN(5, 0x1A),
+ ASPEED_CHAN(6, 0x1C),
+
[PATCH v2 2/2] iio: Aspeed AST2400/AST2500 ADC
Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. Low
and high threshold interrupts are supported by the hardware but are not
currently implemented.
Signed-off-by: Rick Altherr
---
Changes in v2:
- Rewritten as an IIO device
- Renamed register macros to describe the register's purpose
- Replaced awkward reading of 16-bit data registers with readw()
- Added Kconfig dependency on COMPILE_TEST
drivers/iio/adc/Kconfig | 10 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/aspeed_adc.c | 271 +++
3 files changed, 282 insertions(+)
create mode 100644 drivers/iio/adc/aspeed_adc.c
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 2268a6fb9865..9672d799a3fb 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -130,6 +130,16 @@ config AD799X
To compile this driver as a module, choose M here: the module will be
called ad799x.
+config ASPEED_ADC
+ tristate "Aspeed AST2400/AST2500 ADC"
+ depends on ARCH_ASPEED || COMPILE_TEST
+ help
+ If you say yes here you get support for the Aspeed AST2400/AST2500
+ ADC.
+
+ To compile this driver as a module, choose M here: the module will be
+ called aspeed_adc.
+
config AT91_ADC
tristate "Atmel AT91 ADC"
depends on ARCH_AT91
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 73dbe399f894..306f10ffca74 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_AD7791) += ad7791.o
obj-$(CONFIG_AD7793) += ad7793.o
obj-$(CONFIG_AD7887) += ad7887.o
obj-$(CONFIG_AD799X) += ad799x.o
+obj-$(CONFIG_ASPEED_ADC) += aspeed_adc.o
obj-$(CONFIG_AT91_ADC) += at91_adc.o
obj-$(CONFIG_AT91_SAMA5D2_ADC) += at91-sama5d2_adc.o
obj-$(CONFIG_AXP288_ADC) += axp288_adc.o
diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
new file mode 100644
index ..9220909aefd4
--- /dev/null
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -0,0 +1,271 @@
+/*
+ * Aspeed AST2400/2500 ADC
+ *
+ * Copyright (C) 2017 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#define ASPEED_ADC_NUM_CHANNELS16
+#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
+#define ASPEED_ADC_RESOLUTION_BITS 10
+#define ASPEED_ADC_MIN_SAMP_RATE 1
+#define ASPEED_ADC_MAX_SAMP_RATE 50
+#define ASPEED_ADC_CLOCKS_PER_SAMPLE 12
+
+#define ASPEED_ADC_REG_ENGINE_CONTROL 0x00
+#define ASPEED_ADC_REG_INTERRUPT_CONTROL 0x04
+#define ASPEED_ADC_REG_VGA_DETECT_CONTROL 0x08
+#define ASPEED_ADC_REG_CLOCK_CONTROL 0x0C
+#define ASPEED_ADC_REG_MAX 0xC0
+
+#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 << 1)
+#define ASPEED_ADC_OPERATION_MODE_STANDBY (0x1 << 1)
+#define ASPEED_ADC_OPERATION_MODE_NORMAL (0x7 << 1)
+
+#define ASPEED_ADC_ENGINE_ENABLE BIT(0)
+
+struct aspeed_adc_data {
+ struct device *dev;
+ void __iomem*base;
+
+ spinlock_t clk_lock;
+ struct clk_hw *clk_prescaler;
+ struct clk_hw *clk_scaler;
+};
+
+#define ASPEED_ADC_CHAN(_idx, _addr) { \
+ .type = IIO_VOLTAGE,\
+ .indexed = 1, \
+ .channel = (_idx), \
+ .address = (_addr), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+}
+
+static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
+ ASPEED_ADC_CHAN(0, 0x10),
+ ASPEED_ADC_CHAN(1, 0x12),
+ ASPEED_ADC_CHAN(2, 0x14),
+ ASPEED_ADC_CHAN(3, 0x16),
+ ASPEED_ADC_CHAN(4, 0x18),
+ ASPEED_ADC_CHAN(5, 0x1A),
+ ASPEED_ADC_CHAN(6, 0x1C),
+ ASPEED_ADC_CHAN(7, 0x1E),
+ ASPEED_ADC_CHAN(8, 0x20),
+ ASPEED_ADC_CHAN(9, 0x22),
+ ASPEED_ADC_CHAN(10, 0x24),
+ ASPEED_ADC_CHAN(11, 0x26),
+ ASPEED_ADC_CHAN(12, 0x28),
+ ASPEED_ADC_CHAN(13, 0x2A),
+ ASPEED_ADC_CHAN(14, 0x2C),
+ ASPEED_ADC_CHAN(15, 0x2E),
+};
+
+static int aspeed_adc_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct aspeed_adc_data *data = iio_priv(indio_dev);
+
+ swi
[PATCH v2 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC
Signed-off-by: Rick Altherr
---
Changes in v2:
- Rewritten as an IIO ADC device
.../devicetree/bindings/iio/adc/aspeed_adc.txt | 20
1 file changed, 20 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
new file mode 100644
index ..7748c2c2ad0c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
@@ -0,0 +1,20 @@
+Aspeed AST2400/2500 ADC
+
+This device is a 10-bit converter for 16 voltage channels. All inputs are
+single ended.
+
+Required properties:
+- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
+- reg: memory window mapping address and length
+- clocks: Input clock used to derive the sample clock. Expected to be the
+ SoC's APB clock.
+- #io-channel-cells: Must be set to <1> to indicate channels are selected
+ by index.
+
+Example:
+ adc@1e6e9000 {
+ compatible = "aspeed,ast2400-adc";
+ reg = <0x1e6e9000 0xB0>;
+ clocks = <&clk_apb>;
+ #io-channel-cells = <1>;
+ };
--
2.12.1.500.gab5fba24ee-goog
Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC
On Sun, Mar 5, 2017 at 8:28 AM, Guenter Roeck wrote: > On 03/01/2017 07:29 PM, Rick Altherr wrote: >> >> Resending in plain text. >> >> On Tue, Feb 28, 2017 at 7:45 PM, Guenter Roeck wrote: >>> >>> On 02/28/2017 04:49 PM, Joel Stanley wrote: >>>> >>>> >>>> On Wed, Mar 1, 2017 at 6:44 AM, Rick Altherr >>>> wrote: >>>>> >>>>> >>>>> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. This >>>>> driver implements reading the ADC values, enabling channels via device >>>>> tree, and optionally providing channel labels via device tree. Low and >>>>> high threshold interrupts are supported by the hardware but not >>>>> implemented. >>>>> >>>>> Signed-off-by: Rick Altherr >>>> >>>> >>>> >>>> Looks good. Some minor comments below. >>>> >>>> Is there a reason you wrote a hwmon driver instead of an iio driver? I >>>> wasn't sure what the recommended subsystem is. >>> >>> >>> >>> Excellent point. Question is really if there is a plan to add support for >>> thresholds. If not, an iio driver might be more appropriate. >>> >>> Guenter >>> >> >> The hardware supports firing interrupts on high and low thresholds. >> I'm not planning to use that feature yet so I didn't implement it. >> Would you prefer that I leave it as is (maybe with a TODO), implement >> the thresholding, or change to iio? >> > > Let's try to determine the intended use of the ADC. I don't find the > datasheet > online; all I can find is brief summaries which don't me tell much, but > suggest > that it is a general purpose ADC and not specifically intended for hardware > monitoring. What is the minimum sampling rate ? That should give us an idea. > If it is in the uS range, iio would be more appropriate (and the iio-hwmon > bridge could be used if a channel is in fact used for hardware monitoring). > > Thanks, > Guenter > AST2500 is a BMC SoC from Aspeed (https://www.aspeedtech.com/products.php?fPath=20&rId=440). The BMC is a separate, always-on computer that manages the health and remote management for a server. The driver I sent is for the ADCs included in the SoC that are intended to monitor power rails on the server motherboard but are really just general-purpose ADCs. According to the (not public) datasheet, the sampling rate is 10-500kHz, resolution is 10-bit, and precision +/- 2%. This is my first time writing a linux driver for ADCs. My cursory look at iio indicates that that will work fine for this driver and the hwmon-iio bridge will suffice for the userspace stack which is currently expecting hwmon APIs.
Re: [PATCH 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC
On Thu, Mar 2, 2017 at 10:21 PM, Rob Herring wrote:
> On Tue, Feb 28, 2017 at 12:14:03PM -0800, Rick Altherr wrote:
>> Signed-off-by: Rick Altherr
>> ---
>> .../devicetree/bindings/hwmon/aspeed_adc.txt | 48
>> ++
>
> ADCs should really be documented in one place regardless of whether
> hwmon or IIO is used. Don't need to move it now, but certainly the
> bindings need to be compatible.
>
hwmon maintainers suggested IIO as well. Looks like there is an IIO
to hwmon bridge so no concerns there. I think IIO looks like a
plausible framework for this part based on my cursory look at the IIO
docs. I'll be working on v2 as an IIO driver.
>> 1 file changed, 48 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
>> b/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
>> new file mode 100644
>> index ..9e481668c4d3
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
>> @@ -0,0 +1,48 @@
>> +Aspeed AST2400/2500 ADC
>> +
>> +This device is a 10-bit converter for 16 voltage channels. All inputs are
>> +single ended. Each channel can be individually enabled to allow for use of
>> +alternate pin functions.
>> +
>> +1) adc node
>> +
>> + Required properties:
>> + - compatible : Should be one of
>> + "aspeed,ast2400-adc"
>> + "aspeed,ast2500-adc"
>> + - reg : memory window mapping address and length
>> + - #address-cells : must be <1> corresponding to the channel child binding
>> + - #size-cells : must be <0> corresponding to the channel child binding
>> + - clocks : Input clock used to derive the sample clock. Expected to be the
>> +SoC's APB clock.
>> + - update-interval-ms : initial time between updates on a channel
>
> This is like sampling rate? I think we have a standard ADC property for
> that.
>
Will look.
>> +
>> + The node contains child nodes for each channel that the platform uses.
>> +
>> + Example adc node:
>> +adc@1e6e9000 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + compatible = "aspeed,ast2400-adc";
>> + reg = <0x1e6e9000 0xB0>;
>> + clocks = <&clk_apb>;
>> + update-interval-ms = <100>;
>> +
>> + [ child node definitions... ]
>> +};
>> +
>> +2) channel nodes
>> +
>> + Optional properties:
>> + - status: indicates the operational status of the device.
>> +Value must be either "disabled" or "okay".
>
> Don't need to document this.
>
OK
>> + - label : string describing the monitored value
>
> You need a reg property for the channel number.
>
Missed that when I fixed the code. Will include in next revision.
>> +
>> + Example channel node:
>> + channel@1 {
>> + status = "okay";
>> + label = "3V3 rail";
>> +
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_adc0_default>;
>
> Need to document these.
These are described in the pinctrl bindings. I only put them in the
example as they will be commonly used together but the driver does not
use them.
>
>> + };
>> --
>> 2.11.0.483.g087da7b7c-goog
>>
Re: [PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC
Resending in plain text. On Tue, Feb 28, 2017 at 7:45 PM, Guenter Roeck wrote: > On 02/28/2017 04:49 PM, Joel Stanley wrote: >> >> On Wed, Mar 1, 2017 at 6:44 AM, Rick Altherr wrote: >>> >>> Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. This >>> driver implements reading the ADC values, enabling channels via device >>> tree, and optionally providing channel labels via device tree. Low and >>> high threshold interrupts are supported by the hardware but not >>> implemented. >>> >>> Signed-off-by: Rick Altherr >> >> >> Looks good. Some minor comments below. >> >> Is there a reason you wrote a hwmon driver instead of an iio driver? I >> wasn't sure what the recommended subsystem is. > > > Excellent point. Question is really if there is a plan to add support for > thresholds. If not, an iio driver might be more appropriate. > > Guenter > The hardware supports firing interrupts on high and low thresholds. I'm not planning to use that feature yet so I didn't implement it. Would you prefer that I leave it as is (maybe with a TODO), implement the thresholding, or change to iio? Rick >> >>> --- >>> drivers/hwmon/Kconfig | 10 ++ >>> drivers/hwmon/Makefile | 1 + >>> drivers/hwmon/aspeed_adc.c | 383 >>> + >>> 3 files changed, 394 insertions(+) >>> create mode 100644 drivers/hwmon/aspeed_adc.c >>> >>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >>> index 0649d53f3d16..c99a67b4afe4 100644 >>> --- a/drivers/hwmon/Kconfig >>> +++ b/drivers/hwmon/Kconfig >>> @@ -261,6 +261,16 @@ config SENSORS_ASC7621 >>> This driver can also be built as a module. If so, the module >>> will be called asc7621. >>> >>> +config SENSORS_ASPEED_ADC >>> + tristate "Aspeed AST2400/AST2500 ADC" >>> + depends on ARCH_ASPEED >> >> >> depends on ARCH_ASPEED || COMPILE_TEST. >> >>> + help >>> + If you say yes here you get support for the Aspeed >>> AST2400/AST2500 >>> + ADC. >>> + >>> + This driver can also be built as a module. If so, the module >>> + will be called aspeed_adc. >>> + >>> 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 5509edf6186a..eede049c9d0d 100644 >>> --- a/drivers/hwmon/Makefile >>> +++ b/drivers/hwmon/Makefile >>> @@ -46,6 +46,7 @@ obj-$(CONFIG_SENSORS_ADT7475) += adt7475.o >>> obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o >>> obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o >>> obj-$(CONFIG_SENSORS_ASC7621) += asc7621.o >>> +obj-$(CONFIG_SENSORS_ASPEED_ADC) += aspeed_adc.o >>> obj-$(CONFIG_SENSORS_ATXP1)+= atxp1.o >>> obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o >>> obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o >>> diff --git a/drivers/hwmon/aspeed_adc.c b/drivers/hwmon/aspeed_adc.c >>> new file mode 100644 >>> index ..098e32315264 >>> --- /dev/null >>> +++ b/drivers/hwmon/aspeed_adc.c >>> @@ -0,0 +1,383 @@ >>> +/* >>> + * Aspeed AST2400/2500 ADC >>> + * >>> + * Copyright (C) 2017 Google, Inc. >>> + * >>> + * This program is free software; you can redistribute it and/or modify >>> it >>> + * under the terms and conditions of the GNU General Public License, >>> + * version 2, as published by the Free Software Foundation. >>> + * >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#define ASPEED_ADC_NUM_CHANNELS16 >>> +#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */ >>> + >>> +#define ASPEED_ADC_REG_ADC00 0x00 >>> +#define ASPEED_ADC_REG_ADC04 0x04 >>> +#define ASPEED_ADC_REG_ADC08 0x08 >>> +#define ASPEED_ADC_REG_ADC0C 0x0C >>> +#define ASPEED_ADC_REG_ADC10 0x10 >>> +#define ASPEED_ADC_REG_ADC14 0x14 >>> +#define ASPEED_ADC_REG_ADC18 0x18 >>> +#define
[PATCH 1/2] Documentation: dt-bindings: Document bindings for Aspeed AST2400/AST2500 ADC
Signed-off-by: Rick Altherr
---
.../devicetree/bindings/hwmon/aspeed_adc.txt | 48 ++
1 file changed, 48 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
diff --git a/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
b/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
new file mode 100644
index ..9e481668c4d3
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/aspeed_adc.txt
@@ -0,0 +1,48 @@
+Aspeed AST2400/2500 ADC
+
+This device is a 10-bit converter for 16 voltage channels. All inputs are
+single ended. Each channel can be individually enabled to allow for use of
+alternate pin functions.
+
+1) adc node
+
+ Required properties:
+ - compatible : Should be one of
+ "aspeed,ast2400-adc"
+ "aspeed,ast2500-adc"
+ - reg : memory window mapping address and length
+ - #address-cells : must be <1> corresponding to the channel child binding
+ - #size-cells : must be <0> corresponding to the channel child binding
+ - clocks : Input clock used to derive the sample clock. Expected to be the
+SoC's APB clock.
+ - update-interval-ms : initial time between updates on a channel
+
+ The node contains child nodes for each channel that the platform uses.
+
+ Example adc node:
+adc@1e6e9000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "aspeed,ast2400-adc";
+ reg = <0x1e6e9000 0xB0>;
+ clocks = <&clk_apb>;
+ update-interval-ms = <100>;
+
+ [ child node definitions... ]
+};
+
+2) channel nodes
+
+ Optional properties:
+ - status: indicates the operational status of the device.
+Value must be either "disabled" or "okay".
+ - label : string describing the monitored value
+
+ Example channel node:
+ channel@1 {
+ status = "okay";
+ label = "3V3 rail";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_adc0_default>;
+ };
--
2.11.0.483.g087da7b7c-goog
[PATCH 2/2] hwmon: Aspeed AST2400/AST2500 ADC
Aspeed AST2400/AST2500 BMC SoCs include a 16 channel, 10-bit ADC. This
driver implements reading the ADC values, enabling channels via device
tree, and optionally providing channel labels via device tree. Low and
high threshold interrupts are supported by the hardware but not
implemented.
Signed-off-by: Rick Altherr
---
drivers/hwmon/Kconfig | 10 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/aspeed_adc.c | 383 +
3 files changed, 394 insertions(+)
create mode 100644 drivers/hwmon/aspeed_adc.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 0649d53f3d16..c99a67b4afe4 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -261,6 +261,16 @@ config SENSORS_ASC7621
This driver can also be built as a module. If so, the module
will be called asc7621.
+config SENSORS_ASPEED_ADC
+ tristate "Aspeed AST2400/AST2500 ADC"
+ depends on ARCH_ASPEED
+ help
+ If you say yes here you get support for the Aspeed AST2400/AST2500
+ ADC.
+
+ This driver can also be built as a module. If so, the module
+ will be called aspeed_adc.
+
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 5509edf6186a..eede049c9d0d 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_SENSORS_ADT7475) += adt7475.o
obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o
obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o
obj-$(CONFIG_SENSORS_ASC7621) += asc7621.o
+obj-$(CONFIG_SENSORS_ASPEED_ADC) += aspeed_adc.o
obj-$(CONFIG_SENSORS_ATXP1)+= atxp1.o
obj-$(CONFIG_SENSORS_CORETEMP) += coretemp.o
obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
diff --git a/drivers/hwmon/aspeed_adc.c b/drivers/hwmon/aspeed_adc.c
new file mode 100644
index ..098e32315264
--- /dev/null
+++ b/drivers/hwmon/aspeed_adc.c
@@ -0,0 +1,383 @@
+/*
+ * Aspeed AST2400/2500 ADC
+ *
+ * Copyright (C) 2017 Google, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define ASPEED_ADC_NUM_CHANNELS16
+#define ASPEED_ADC_REF_VOLTAGE 2500 /* millivolts */
+
+#define ASPEED_ADC_REG_ADC00 0x00
+#define ASPEED_ADC_REG_ADC04 0x04
+#define ASPEED_ADC_REG_ADC08 0x08
+#define ASPEED_ADC_REG_ADC0C 0x0C
+#define ASPEED_ADC_REG_ADC10 0x10
+#define ASPEED_ADC_REG_ADC14 0x14
+#define ASPEED_ADC_REG_ADC18 0x18
+#define ASPEED_ADC_REG_ADC1C 0x1C
+#define ASPEED_ADC_REG_ADC20 0x20
+#define ASPEED_ADC_REG_ADC24 0x24
+#define ASPEED_ADC_REG_ADC28 0x28
+#define ASPEED_ADC_REG_ADC2C 0x2C
+
+#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN (0x0 << 1)
+#define ASPEED_ADC_OPERATION_MODE_STANDBY (0x1 << 1)
+#define ASPEED_ADC_OPERATION_MODE_NORMAL (0x7 << 1)
+
+#define ASPEED_ADC_ENGINE_ENABLE BIT(0)
+
+struct aspeed_adc_data {
+ struct device *dev;
+ void __iomem*base;
+ struct clk *pclk;
+ struct mutexlock;
+ unsigned long update_interval_ms;
+ u32 enabled_channel_mask;
+ const char* channel_labels[ASPEED_ADC_NUM_CHANNELS];
+};
+
+static int aspeed_adc_set_adc_clock_frequency(
+ struct aspeed_adc_data *data,
+ unsigned long desired_update_interval_ms)
+{
+ long pclk_hz = clk_get_rate(data->pclk);
+ long adc_combined_divisor;
+ long adc_pre_divisor;
+ long adc_divisor;
+ long adc_clock_control_reg_val;
+long num_enabled_channels = hweight32(data->enabled_channel_mask);
+
+ if (desired_update_interval_ms < 1)
+ return -EINVAL;
+
+ /* From the AST2400 datasheet:
+* adc_period_s = pclk_period_s * 2 * (pre_divisor+1) * (divisor+1)
+*
+* and
+*
+* sample_period_s = 12 * adc_period_s
+*
+* Substitute pclk_period_s = (1 / pclk_hz)
+*
+* Solve for (pre-divisor+1) * (divisor+1) as those are settings we need
+* to program:
+* (pre-divisor+1) * (divisor+1) = (sample_period_s * pclk_hz) / 24
+*/
+
+ /* Assume PCLK runs at a fairly high frequency so dividing it first
+* loses little accuracy. Also note that the above formulas have
+* sample_period in seconds while our desired_update_interval is in
+* milliseconds, hence the divide by 1000.
+*/
+ adc_combined_divisor = desired_update_interval_ms *
+ (pclk_hz / 24 / 1000 / num_enabled_channels);
+
+ /* Prefer to use
[RESEND PATCH linux] aspeed: pinctrl: Allow disabling Port D and Port E loopback mode
Port D and port E GPIO loopback modes are commonly enabled via hardware straps for use with front-panel buttons. When the BMC is powered off or fails to boot, the front-panel buttons are directly connected to the host chipset via the loopback to allow direct power-on and reset control. Once the BMC has booted, the loopback mode must be disabled for the BMC to take over control of host power-on and reset. Disabling these loopback modes requires writing to the hardware strap register which violates the current design of assuming the system designer chose the strap settings for a specific reason and they should be treated as read-only. Only the two bits of the strap register related to these loopback modes are allowed to be written and comments have been added to explain why. Signed-off-by: Rick Altherr --- drivers/pinctrl/aspeed/pinctrl-aspeed.c | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c index 76f62bd45f02..5b49952e5fad 100644 --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c @@ -198,9 +198,19 @@ static int aspeed_sig_expr_set(const struct aspeed_sig_expr *expr, * them. This may mean that certain functions cannot be * deconfigured and is the reason we re-evaluate after writing * all descriptor bits. +* +* Port D and port E GPIO loopback modes are the only exception +* as those are commonly used with front-panel buttons to allow +* normal operation of the host when the BMC is powered off or +* fails to boot. Once the BMC has booted, the loopback mode +* must be disabled for the BMC to control host power-on and +* reset. */ - if ((desc->reg == HW_STRAP1 || desc->reg == HW_STRAP2) && - desc->ip == ASPEED_IP_SCU) + if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP1 && + !(desc->mask & (BIT(21) | BIT(22 + continue; + + if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP2) continue; ret = regmap_update_bits(maps[desc->ip], desc->reg, -- 2.11.0.483.g087da7b7c-goog
[PATCH linux] aspeed: pinctrl: Allow disabling Port D and Port E loopback mode
Port D and port E GPIO loopback modes are commonly enabled via hardware straps for use with front-panel buttons. When the BMC is powered off or fails to boot, the front-panel buttons are directly connected to the host chipset via the loopback to allow direct power-on and reset control. Once the BMC has booted, the loopback mode must be disabled for the BMC to take over control of host power-on and reset. Disabling these loopback modes requires writing to the hardware strap register which violates the current design of assuming the system designer chose the strap settings for a specific reason and they should be treated as read-only. Only the two bits of the strap register related to these loopback modes are allowed to be written and comments have been added to explain why. Signed-off-by: Rick Altherr --- drivers/pinctrl/aspeed/pinctrl-aspeed.c | 14 -- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c index 76f62bd45f02..5b49952e5fad 100644 --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c @@ -198,9 +198,19 @@ static int aspeed_sig_expr_set(const struct aspeed_sig_expr *expr, * them. This may mean that certain functions cannot be * deconfigured and is the reason we re-evaluate after writing * all descriptor bits. +* +* Port D and port E GPIO loopback modes are the only exception +* as those are commonly used with front-panel buttons to allow +* normal operation of the host when the BMC is powered off or +* fails to boot. Once the BMC has booted, the loopback mode +* must be disabled for the BMC to control host power-on and +* reset. */ - if ((desc->reg == HW_STRAP1 || desc->reg == HW_STRAP2) && - desc->ip == ASPEED_IP_SCU) + if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP1 && + !(desc->mask & (BIT(21) | BIT(22 + continue; + + if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP2) continue; ret = regmap_update_bits(maps[desc->ip], desc->reg, -- 2.11.0.483.g087da7b7c-goog
