* Re: [PATCH linux 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC
From: kbuild test robot @ 2017-01-01 0:25 UTC (permalink / raw)
To: eajames.ibm
Cc: kbuild-all, linux, jdelvare, corbet, mark.rutland, robh+dt, wsa,
andrew, joel, devicetree, linux-doc, linux-hwmon, linux-i2c,
linux-kernel, Edward A. James
In-Reply-To: <1483120568-21082-6-git-send-email-eajames.ibm@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]
Hi Edward,
[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on v4.10-rc1 next-20161224]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/eajames-ibm-gmail-com/drivers-hwmon-Add-On-Chip-Controller-driver/20161231-021324
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: x86_64-randconfig-n0-01010656 (attached as .config)
compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
>> ERROR: "occ_sysfs_start" [drivers/hwmon/occ/p8_occ_i2c.ko] undefined!
>> ERROR: "occ_sysfs_stop" [drivers/hwmon/occ/p8_occ_i2c.ko] undefined!
>> ERROR: "occ_stop" [drivers/hwmon/occ/occ_p8.ko] undefined!
>> ERROR: "occ_start" [drivers/hwmon/occ/occ_p8.ko] undefined!
>> ERROR: "occ_get_sensor" [drivers/hwmon/occ/occ_p8.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21665 bytes --]
^ permalink raw reply
* Re: [PATCH v6 7/9] i2c: i2c-mux-simple: new driver
From: Jonathan Cameron @ 2017-01-01 10:31 UTC (permalink / raw)
To: Peter Rosin, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480493823-21462-8-git-send-email-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 30/11/16 08:17, Peter Rosin wrote:
> This is a generic simple i2c mux that uses the generic multiplexer
> subsystem to do the muxing.
>
> The user can select if the mux is to be mux-locked and parent-locked
> as described in Documentation/i2c/i2c-topology.
>
> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Looks good to me though as I haven't really commented on it I'll give
an Ack rather than a reviewed by.
Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> drivers/i2c/muxes/Kconfig | 13 +++
> drivers/i2c/muxes/Makefile | 1 +
> drivers/i2c/muxes/i2c-mux-simple.c | 179 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 193 insertions(+)
> create mode 100644 drivers/i2c/muxes/i2c-mux-simple.c
>
> diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
> index 10b3d17ae3ea..565921e09a96 100644
> --- a/drivers/i2c/muxes/Kconfig
> +++ b/drivers/i2c/muxes/Kconfig
> @@ -73,6 +73,19 @@ config I2C_MUX_REG
> This driver can also be built as a module. If so, the module
> will be called i2c-mux-reg.
>
> +config I2C_MUX_SIMPLE
> + tristate "Simple I2C multiplexer"
> + select MULTIPLEXER
> + depends on OF
> + help
> + If you say yes to this option, support will be included for a
> + simple generic I2C multiplexer. This driver provides access to
> + I2C busses connected through a MUX, which is controlled
> + by a generic MUX controller.
> +
> + This driver can also be built as a module. If so, the module
> + will be called i2c-mux-simple.
> +
> config I2C_DEMUX_PINCTRL
> tristate "pinctrl-based I2C demultiplexer"
> depends on PINCTRL && OF
> diff --git a/drivers/i2c/muxes/Makefile b/drivers/i2c/muxes/Makefile
> index 9948fa45037f..6821d95c92a3 100644
> --- a/drivers/i2c/muxes/Makefile
> +++ b/drivers/i2c/muxes/Makefile
> @@ -11,5 +11,6 @@ obj-$(CONFIG_I2C_MUX_PCA9541) += i2c-mux-pca9541.o
> obj-$(CONFIG_I2C_MUX_PCA954x) += i2c-mux-pca954x.o
> obj-$(CONFIG_I2C_MUX_PINCTRL) += i2c-mux-pinctrl.o
> obj-$(CONFIG_I2C_MUX_REG) += i2c-mux-reg.o
> +obj-$(CONFIG_I2C_MUX_SIMPLE) += i2c-mux-simple.o
>
> ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> diff --git a/drivers/i2c/muxes/i2c-mux-simple.c b/drivers/i2c/muxes/i2c-mux-simple.c
> new file mode 100644
> index 000000000000..4a03493e1ad7
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-mux-simple.c
> @@ -0,0 +1,179 @@
> +/*
> + * Generic simple I2C multiplexer
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/i2c-mux.h>
> +#include <linux/module.h>
> +#include <linux/mux.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +
> +struct mux {
> + struct mux_control *control;
> +
> + bool do_not_deselect;
> +};
> +
> +static int i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
> +{
> + struct mux *mux = i2c_mux_priv(muxc);
> + int ret;
> +
> + ret = mux_control_select(mux->control, chan);
> + mux->do_not_deselect = ret < 0;
> +
> + return ret;
> +}
> +
> +static int i2c_mux_deselect(struct i2c_mux_core *muxc, u32 chan)
> +{
> + struct mux *mux = i2c_mux_priv(muxc);
> +
> + if (mux->do_not_deselect)
> + return 0;
> +
> + return mux_control_deselect(mux->control);
> +}
> +
> +static struct i2c_adapter *mux_parent_adapter(struct device *dev)
> +{
> + struct device_node *np = dev->of_node;
> + struct device_node *parent_np;
> + struct i2c_adapter *parent;
> +
> + parent_np = of_parse_phandle(np, "i2c-parent", 0);
> + if (!parent_np) {
> + dev_err(dev, "Cannot parse i2c-parent\n");
> + return ERR_PTR(-ENODEV);
> + }
> + parent = of_find_i2c_adapter_by_node(parent_np);
> + of_node_put(parent_np);
> + if (!parent)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + return parent;
> +}
> +
> +static const struct of_device_id i2c_mux_of_match[] = {
> + { .compatible = "i2c-mux-simple,parent-locked",
> + .data = (void *)0, },
> + { .compatible = "i2c-mux-simple,mux-locked",
> + .data = (void *)1, },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, i2c_mux_of_match);
> +
> +static int i2c_mux_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct device_node *child;
> + const struct of_device_id *match;
> + struct i2c_mux_core *muxc;
> + struct mux *mux;
> + struct i2c_adapter *parent;
> + int children;
> + int ret;
> +
> + if (!np)
> + return -ENODEV;
> +
> + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + return -ENOMEM;
> +
> + mux->control = devm_mux_control_get(dev, NULL);
> + if (IS_ERR(mux->control)) {
> + if (PTR_ERR(mux->control) != -EPROBE_DEFER)
> + dev_err(dev, "failed to get control-mux\n");
> + return PTR_ERR(mux->control);
> + }
> +
> + parent = mux_parent_adapter(dev);
> + if (IS_ERR(parent)) {
> + if (PTR_ERR(parent) != -EPROBE_DEFER)
> + dev_err(dev, "failed to get i2c-parent adapter\n");
> + return PTR_ERR(parent);
> + }
> +
> + children = of_get_child_count(np);
> +
> + muxc = i2c_mux_alloc(parent, dev, children, 0, 0,
> + i2c_mux_select, i2c_mux_deselect);
> + if (!muxc) {
> + ret = -ENOMEM;
> + goto err_parent;
> + }
> + muxc->priv = mux;
> +
> + platform_set_drvdata(pdev, muxc);
> +
> + match = of_match_device(of_match_ptr(i2c_mux_of_match), dev);
> + if (match)
> + muxc->mux_locked = !!of_device_get_match_data(dev);
> +
> + for_each_child_of_node(np, child) {
> + u32 chan;
> +
> + ret = of_property_read_u32(child, "reg", &chan);
> + if (ret < 0) {
> + dev_err(dev, "no reg property for node '%s'\n",
> + child->name);
> + goto err_children;
> + }
> +
> + if (chan >= mux->control->states) {
> + dev_err(dev, "invalid reg %u\n", chan);
> + ret = -EINVAL;
> + goto err_children;
> + }
> +
> + ret = i2c_mux_add_adapter(muxc, 0, chan, 0);
> + if (ret)
> + goto err_children;
> + }
> +
> + dev_info(dev, "%d-port mux on %s adapter\n", children, parent->name);
> +
> + return 0;
> +
> +err_children:
> + i2c_mux_del_adapters(muxc);
> +err_parent:
> + i2c_put_adapter(parent);
> +
> + return ret;
> +}
> +
> +static int i2c_mux_remove(struct platform_device *pdev)
> +{
> + struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
> +
> + i2c_mux_del_adapters(muxc);
> + i2c_put_adapter(muxc->parent);
> +
> + return 0;
> +}
> +
> +static struct platform_driver i2c_mux_driver = {
> + .probe = i2c_mux_probe,
> + .remove = i2c_mux_remove,
> + .driver = {
> + .name = "i2c-mux-simple",
> + .of_match_table = i2c_mux_of_match,
> + },
> +};
> +module_platform_driver(i2c_mux_driver);
> +
> +MODULE_DESCRIPTION("Simple I2C multiplexer driver");
> +MODULE_AUTHOR("Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply
* Re: [PATCH v6 8/9] dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G mux
From: Jonathan Cameron @ 2017-01-01 11:00 UTC (permalink / raw)
To: Peter Rosin, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <1480493823-21462-9-git-send-email-peda@axentia.se>
On 30/11/16 08:17, Peter Rosin wrote:
> Analog Devices ADG792A/G is a triple 4:1 mux.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
Few comments inline. Worth adding anything about the gpio (output pins) to
the binding at this stage as well? Would certainly be nice to support
them.
Jonathan
> ---
> .../devicetree/bindings/misc/mux-adg792a.txt | 64 ++++++++++++++++++++++
> 1 file changed, 64 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/misc/mux-adg792a.txt
>
> diff --git a/Documentation/devicetree/bindings/misc/mux-adg792a.txt b/Documentation/devicetree/bindings/misc/mux-adg792a.txt
> new file mode 100644
> index 000000000000..4677f9ab1c55
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/mux-adg792a.txt
> @@ -0,0 +1,64 @@
> +Bindings for Analog Devices ADG792A/G Triple 4:1 Multiplexers
> +
> +Required properties:
> +- compatible : "adi,adg792a" or "adi,adg792g"
> +- #mux-control-cells : <0> if parallel, or <1> if not.
> +* Standard mux-controller bindings as decribed in mux-controller.txt
> +
> +Optional properties:
> +- adi,parallel : if present, the three muxes are bound together with a single
> + mux controller, controlling all three muxes in parallel.
> +- adi,idle-state : if present, array of states the three mux controllers will
> + have when idle (or, if parallel, a single idle-state).
Hmm. These are actually a policy decision. As only one policy will make
sense for a given set of hardware probably fine to have it in here I guess.
Might be worth adding a note to say this though.
> +
> +Mux controller states 0 through 3 correspond to signals A through D in the
> +datasheet. Mux controller states 4 and 5 are only available as possible idle
> +states. State 4 represents that nothing is connected, and state 5 represents
> +that the mux controller keeps the mux in its previously selected state during
> +the idle period. State 5 is the default idle state.
I'm never a great fan of magic numbers. Can we represent this more cleanly by
breaking it into multiple properties?
Optional:
adi,idle-switch-to-channel : switch to this channel when idle.
adi,idle-high-impedance : <boolean> the nothing connected state?
If neither present leaves it in previous state?
> +
> +Example:
> +
> + /* three independent mux controllers (of which one is used) */
> + &i2c0 {
> + mux: adg792a@50 {
> + compatible = "adi,adg792a";
> + reg = <0x50>;
> + #mux-control-cells = <1>;
> + };
> + };
> +
> + adc-mux {
> + compatible = "iio-mux";
> + io-channels = <&adc 0>;
> + io-channel-names = "parent";
> +
> + mux-controls = <&mux 1>;
> +
> + channels = "sync-1", "", "out";
> + };
> +
> +
> + /*
> + * Three parallel muxes with one mux controller, useful e.g. if
> + * the adc is differential, thus needing two signals to be muxed
> + * simultaneously for correct operation.
> + */
> + &i2c0 {
> + pmux: adg792a@50 {
> + compatible = "adi,adg792a";
> + reg = <0x50>;
> + #mux-control-cells = <0>;
> + adi,parallel;
> + };
> + };
> +
> + diff-adc-mux {
> + compatible = "iio-mux";
> + io-channels = <&adc 0>;
> + io-channel-names = "parent";
> +
> + mux-controls = <&pmux>;
> +
> + channels = "sync-1", "", "out";
> + };
>
^ permalink raw reply
* Re: [PATCH] iio: adc: axp288: Drop bogus AXP288_ADC_TS_PIN_CTRL register modifications
From: Hans de Goede @ 2017-01-01 11:19 UTC (permalink / raw)
To: Jacob Pan, Jonathan Cameron
Cc: Chen-Yu Tsai, Lars-Peter Clausen, Peter Meerwald-Stadler,
russianneuromancer @ ya . ru, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161230101501.06958c0d@jacob-builder>
Hi,
On 30-12-16 19:15, Jacob Pan wrote:
> On Fri, 30 Dec 2016 16:46:03 +0000
> Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
>> On 14/12/16 13:55, Hans de Goede wrote:
>>> For some reason the axp288_adc driver was modifying the
>>> AXP288_ADC_TS_PIN_CTRL register, changing bits 0-1 depending on
>>> whether the GP_ADC channel or another channel was written.
>>>
>>> These bits control when a bias current is send to the TS_PIN, the
>>> GP_ADC has its own pin and a separate bit in another register to
>>> control the bias current.
>>>
> It has been a while. Just looked at the datasheet, reg 84h is for ADC
> and TS pin control. IIRC, we had to set the TS bits in order to make
> ADC read to work.
The bits the code I'm removing are manipulating are bits 0 & 1 of
reg 84h which are:
1-0 Current source from TS pin on/off enable bit [1:0]
00: off
01: on when charging battery, off when not charging
10: on in ADC phase and off when out of the ADC phase, for power saving
11: always on
And they are being toggled between 10 and 11, so in both
cases the current source is enabled when reading the adc
value. Specifically the code this patch removes are setting
these bits to 10 before reading and back to 11 after reading,
which makes no sense.
And to make things even funkier, this manipulation is only
done when reading the GP_ADC, which is the ADC function of
GPIO0, whereas these bits control the bias current source
for the TS pin, which is a completely different pin.
So all in all this entire bit of code seems to be big NOP.
> What is the other register?
The register to actually enable / disable the bias current
source for GPIO0 / the GPADC pin is register 85h, where setting
bit 2 enables the GPIO0 output current.
Regards,
Hans
>>> Not only does changing when to enable the TS_PIN bias current
>>> (always or only when sampling) when reading the GP_ADC make no sense
>>> at all, the code is modifying these bits is writing the entire
>>> register, assuming that all the other bits have their default value.
>>>
> Agreed, it would be better to do read-modify-write and not to
> touch the other bits.
>>> So if the firmware has configured a different bias-current for
>>> either pin, then that change gets clobbered by the write, likewise
>>> if the firmware has set bit 2 to indicate that the battery has no
>>> thermal sensor, this will get clobbered by the write.
>>>
>>> This commit fixes all this, by simply removing all writes to the
>>> AXP288_ADC_TS_PIN_CTRL register, they are not needed to read the
>>> GP_ADC pin, and can actually be harmful.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> I guess you probably have more up to date contact details than I do,
>> but seems worth trying to cc Jacob Pan on this to see if we can find
>> out what the original reasoning behind this was. Seems a very odd
>> thing to do with no purpose!
>>
>> If Jacob isn't contactable we'll fall back to guessing it was just
>> an oddity of driver evolution.
>>
>> Jonathan
>>> ---
>>> drivers/iio/adc/axp288_adc.c | 32 +-------------------------------
>>> 1 file changed, 1 insertion(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/axp288_adc.c
>>> b/drivers/iio/adc/axp288_adc.c index 7fd2494..64799ad 100644
>>> --- a/drivers/iio/adc/axp288_adc.c
>>> +++ b/drivers/iio/adc/axp288_adc.c
>>> @@ -28,8 +28,6 @@
>>> #include <linux/iio/driver.h>
>>>
>>> #define AXP288_ADC_EN_MASK 0xF1
>>> -#define AXP288_ADC_TS_PIN_GPADC 0xF2
>>> -#define AXP288_ADC_TS_PIN_ON 0xF3
>>>
>>> enum axp288_adc_id {
>>> AXP288_ADC_TS,
>>> @@ -123,16 +121,6 @@ static int axp288_adc_read_channel(int *val,
>>> unsigned long address, return IIO_VAL_INT;
>>> }
>>>
>>> -static int axp288_adc_set_ts(struct regmap *regmap, unsigned int
>>> mode,
>>> - unsigned long address)
>>> -{
>>> - /* channels other than GPADC do not need to switch TS pin
>>> */
>>> - if (address != AXP288_GP_ADC_H)
>>> - return 0;
>>> -
>>> - return regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL, mode);
>>> -}
>>> -
>>> static int axp288_adc_read_raw(struct iio_dev *indio_dev,
>>> struct iio_chan_spec const *chan,
>>> int *val, int *val2, long mask)
>>> @@ -143,16 +131,7 @@ static int axp288_adc_read_raw(struct iio_dev
>>> *indio_dev, mutex_lock(&indio_dev->mlock);
>>> switch (mask) {
>>> case IIO_CHAN_INFO_RAW:
>>> - if (axp288_adc_set_ts(info->regmap,
>>> AXP288_ADC_TS_PIN_GPADC,
>>> - chan->address)) {
>>> - dev_err(&indio_dev->dev, "GPADC mode\n");
>>> - ret = -EINVAL;
>>> - break;
>>> - }
>>> ret = axp288_adc_read_channel(val, chan->address,
>>> info->regmap);
>>> - if (axp288_adc_set_ts(info->regmap,
>>> AXP288_ADC_TS_PIN_ON,
>>> - chan->address))
>>> - dev_err(&indio_dev->dev, "TS pin
>>> restore\n"); break;
>>> default:
>>> ret = -EINVAL;
>>> @@ -162,15 +141,6 @@ static int axp288_adc_read_raw(struct iio_dev
>>> *indio_dev, return ret;
>>> }
>>>
>>> -static int axp288_adc_set_state(struct regmap *regmap)
>>> -{
>>> - /* ADC should be always enabled for internal FG to
>>> function */
>>> - if (regmap_write(regmap, AXP288_ADC_TS_PIN_CTRL,
>>> AXP288_ADC_TS_PIN_ON))
>>> - return -EIO;
>>> -
>>> - return regmap_write(regmap, AXP20X_ADC_EN1,
>>> AXP288_ADC_EN_MASK); -}
>>> -
>>> static const struct iio_info axp288_adc_iio_info = {
>>> .read_raw = &axp288_adc_read_raw,
>>> .driver_module = THIS_MODULE,
>>> @@ -199,7 +169,7 @@ static int axp288_adc_probe(struct
>>> platform_device *pdev)
>>> * Set ADC to enabled state at all time, including system
>>> suspend.
>>> * otherwise internal fuel gauge functionality may be
>>> affected. */
>>> - ret = axp288_adc_set_state(axp20x->regmap);
>>> + ret = regmap_write(info->regmap, AXP20X_ADC_EN1,
>>> AXP288_ADC_EN_MASK); if (ret) {
>>> dev_err(&pdev->dev, "unable to enable ADC
>>> device\n"); return ret;
>>>
>>
>
> [Jacob Pan]
>
^ permalink raw reply
* Re: [PATCH v6 9/9] misc: mux-adg792a: add mux controller driver for ADG792A/G
From: Jonathan Cameron @ 2017-01-01 11:24 UTC (permalink / raw)
To: Peter Rosin, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <1480493823-21462-10-git-send-email-peda@axentia.se>
On 30/11/16 08:17, Peter Rosin wrote:
> Analog Devices ADG792A/G is a triple 4:1 mux.
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
Looks pretty good. Some minor suggestions inline.
This convinced me of two things:
1. Need a separate subsystem directory for muxes - having them under misc
is going to lead to long term mess.
2. Devm alloc and registration functions will make the drivers all simpler.
Also, browsing through ADIs list of muxes and switches it's clear that
one classic case will be where an i2c octal or similar switch is used with
outputs wired together in weird combinations to act as a mux. Going to
be 'fun' describing that.
There are also potentially cross point switches to be described ;)
(I had to look up what one of those was ;)
Crosspoints aren't implausible as front ends for ADCs as you might
want to be able rapidly sample any 2 of say 16 channels coming from
for example a max14661. We'd have to figure out how to add buffered
capture support with sensible restrictions to the iio-mux driver
to do that - realistically I think we would just not allow buffered
capture with the mux having to switch. Without hardware support
(i.e. an ADC with external mux control) it would be too slow.
Always good to bury some idle thoughts deep in the review of a random
driver ;) I'll never be able to remember where they were let alone
anyone else.
Jonathan
> ---
> drivers/misc/Kconfig | 12 ++++
> drivers/misc/Makefile | 1 +
> drivers/misc/mux-adg792a.c | 154 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 167 insertions(+)
> create mode 100644 drivers/misc/mux-adg792a.c
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 2ce675e410c5..45567a444bbf 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -780,6 +780,18 @@ menuconfig MULTIPLEXER
>
> if MULTIPLEXER
>
> +config MUX_ADG792A
> + tristate "Analog Devices ADG792A/ADG792G Multiplexers"
> + depends on I2C
> + help
> + ADG792A and ADG792G Wide Bandwidth Triple 4:1 Multiplexers
> +
> + The driver supports both operating the three multiplexers in
> + parellel and operating them independently.
parallel
> +
> + To compile the driver as a module, choose M here: the module will
> + be called mux-adg792a.
> +
> config MUX_GPIO
> tristate "GPIO-controlled Multiplexer"
> depends on OF && GPIOLIB
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index 0befa2bba762..10ab8d34c9e5 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
> obj-$(CONFIG_CXL_BASE) += cxl/
> obj-$(CONFIG_PANEL) += panel.o
> obj-$(CONFIG_MULTIPLEXER) += mux-core.o
> +obj-$(CONFIG_MUX_ADG792A) += mux-adg792a.o
> obj-$(CONFIG_MUX_GPIO) += mux-gpio.o
>
> lkdtm-$(CONFIG_LKDTM) += lkdtm_core.o
> diff --git a/drivers/misc/mux-adg792a.c b/drivers/misc/mux-adg792a.c
> new file mode 100644
> index 000000000000..7d309a78af65
> --- /dev/null
> +++ b/drivers/misc/mux-adg792a.c
> @@ -0,0 +1,154 @@
> +/*
> + * Multiplexer driver for Analog Devices ADG792A/G Triple 4:1 mux
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda@axentia.se>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mux.h>
> +
> +#define ADG792A_LDSW BIT(0)
> +#define ADG792A_RESET BIT(1)
> +#define ADG792A_DISABLE(mux) (0x50 | (mux))
> +#define ADG792A_DISABLE_ALL (0x5f)
> +#define ADG792A_MUX(mux, state) (0xc0 | (((mux) + 1) << 2) | (state))
> +#define ADG792A_MUX_ALL(state) (0xc0 | (state))
> +
> +#define ADG792A_DISABLE_STATE (4)
> +#define ADG792A_KEEP_STATE (5)
> +
> +static int adg792a_set(struct mux_control *mux, int state)
> +{
> + struct i2c_client *i2c = to_i2c_client(mux->chip->dev.parent);
> + u8 cmd;
> +
> + if (mux->chip->controllers == 1) {
> + /* parallel mux controller operation */
> + if (state == ADG792A_DISABLE_STATE)
> + cmd = ADG792A_DISABLE_ALL;
> + else
> + cmd = ADG792A_MUX_ALL(state);
> + } else {
> + unsigned int controller = mux_control_get_index(mux);
> +
> + if (state == ADG792A_DISABLE_STATE)
> + cmd = ADG792A_DISABLE(controller);
> + else
> + cmd = ADG792A_MUX(controller, state);
> + }
> +
> + return i2c_smbus_write_byte_data(i2c, cmd, ADG792A_LDSW);
> +}
> +
> +static const struct mux_control_ops adg792a_ops = {
> + .set = adg792a_set,
> +};
> +
> +static int adg792a_probe(struct i2c_client *i2c,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &i2c->dev;
> + struct mux_chip *mux_chip;
> + bool parallel;
> + int ret;
> + int i;
> +
> + parallel = of_property_read_bool(i2c->dev.of_node, "adi,parallel");
> +
> + mux_chip = mux_chip_alloc(dev, parallel ? 1 : 3, 0);
This makes me wonder if we can have a more generic binding.
mux-poles = 3 vs mux-poles = 1?
> + if (!mux_chip)
> + return -ENOMEM;
> +
> + mux_chip->ops = &adg792a_ops;
> + dev_set_drvdata(dev, mux_chip);
> +
> + ret = i2c_smbus_write_byte_data(i2c, ADG792A_DISABLE_ALL,
> + ADG792A_RESET | ADG792A_LDSW);
> + if (ret < 0)
> + goto free_mux_chip;
> +
> + for (i = 0; i < mux_chip->controllers; ++i) {
> + struct mux_control *mux = &mux_chip->mux[i];
> + u32 idle_state;
> +
> + mux->states = 4;
> +
> + ret = of_property_read_u32_index(i2c->dev.of_node,
> + "adi,idle-state", i,
> + &idle_state);
> + if (ret >= 0) {
> + if (idle_state > ADG792A_KEEP_STATE) {
> + dev_err(dev, "invalid idle-state %u\n",
> + idle_state);
> + ret = -EINVAL;
> + goto free_mux_chip;
> + }
> + if (idle_state != ADG792A_KEEP_STATE)
> + mux->idle_state = idle_state;
> + }
> + }
> +
> + ret = mux_chip_register(mux_chip);
> + if (ret < 0) {
> + dev_err(dev, "failed to register mux-chip\n");
> + goto free_mux_chip;
> + }
> +
> + if (parallel)
> + dev_info(dev, "1 triple 4-way mux-controller registered\n");
I'd use the relay / switch standard description for this so
'triple pole, quadruple throw mux registered'.
> + else
> + dev_info(dev, "3 4-way mux-controllers registered\n");
'3x single pole, quadruple throw muxes registered'.
> +
> + return 0;
> +
> +free_mux_chip:
> + mux_chip_free(mux_chip);
> + return ret;
> +}
> +
> +static int adg792a_remove(struct i2c_client *i2c)
> +{
> + struct mux_chip *mux_chip = dev_get_drvdata(&i2c->dev);
> +
Definitely looking like it's worth managed versions of mux_chip_register and
mux_chip_alloc given this is another case where they would let us get rid
of the remove function entirely.
> + mux_chip_unregister(mux_chip);
> + mux_chip_free(mux_chip);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id adg792a_id[] = {
> + { .name = "adg792a", },
> + { .name = "adg792g", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, adg792a_id);
> +
> +static const struct of_device_id adg792a_of_match[] = {
> + { .compatible = "adi,adg792a", },
> + { .compatible = "adi,adg792g", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, adg792a_of_match);
> +
> +static struct i2c_driver adg792a_driver = {
> + .driver = {
> + .name = "adg792a",
> + .of_match_table = of_match_ptr(adg792a_of_match),
> + },
> + .probe = adg792a_probe,
> + .remove = adg792a_remove,
> + .id_table = adg792a_id,
> +};
> +module_i2c_driver(adg792a_driver);
> +
> +MODULE_DESCRIPTION("Analog Devices ADG792A/G Triple 4:1 mux driver");
> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply
* Re: [PATCH v6 0/9] mux controller abstraction and iio/i2c muxes
From: Jonathan Cameron @ 2017-01-01 11:28 UTC (permalink / raw)
To: Peter Rosin, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480493823-21462-1-git-send-email-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 30/11/16 08:16, Peter Rosin wrote:
> Hi!
Just a quick note to encourage people to take a look at this series
if they have the time (I can't talk as it took me almost exactly a
month to take a proper look).
It actually ended up a good deal simpler than I expected so I'm falling
on the side of this making sense rather than pushing all mux complexity
into a 'card driver'. Covers a lot of cases without a proliferation of
code which is nice.
One addition that may make sense is to provide hooks to allow 'card
drivers' where they are needed. I can certainly conceive of devices
where the complexity does rise to the point where they make sense.
However, we can add them when needed.
Peter, don't give up on this set just yet!
Happy New Year,
Jonathan
>
> v5 -> v6 changes
> - fix stupidity in mux_chip_priv, mux_gpio_remove and adg792a_remove.
> - change the devicetree bindings for the iio-mux to use a list of strings
> (channels property) instead of a list children.
>
> v4 -> v5 changes
> - remove support for fancier dt layouts and go back to the phandle
> approach from v2 and before, killing the horrible non-working
> refcounting crap from v4 and avoiding a bunch of life-time issues
> in v3.
> - introduce the concept of a mux-chip, that can hold one or more
> mux-controllers (inspired by the pwm subsystem).
> - add dt #mux-control-cells property needed to get to the desired
> mux controller if a mux chip provides more than one.
> - take away the option to build the mux-core as a module.
> - if the mux controller has an idle state, make sure the mux controller
> is set up in the idle state initially (when it should be idle).
> - do not use a variable length array on the stack in mux_gpio_set to
> temporarily store the gpio state, preallocate space instead.
> - fix resource leak on one failure path in mux_gpio_probe.
> - driver for Analog Devices ADG792A/G, literally the first mux chip
> I found on the Internet with an i2c interface (that was not a
> dedicated i2c multiplexer like PCA9547) which I used to verify
> that the abstractions in the mux core are up to the task. Untested,
> just proof of concept that at least looks pretty and compiles...
> - various touch-ups.
>
> v3 -> v4 changes
> - rebased onto next-20161122 (depends on recent _available iio changes).
> - added support for having the mux-controller in a child node of a
> mux-consumer if it is a sole consumer, to hopefully even further satisfy
> the complaint from Rob (and later Lars-Peter) about dt complexity.
> - the above came at the cost of some rather horrible refcounting code,
> please review and suggest how it should be done...
> - changed to register a device class instead of a bus.
> - pass in the parent device into mux_control_alloc and require less
> work from mux-control drivers.
> - changed device names from mux:control%d to mux%d
> - move kernel-doc from mux-core.c to mux.h (and add some bits).
> - give the gpio driver a chance to update all mux pins at once.
> - factor out iio ext_info lookup into new helper function. /Lars-Peter
> - use an unsigned type for the iio ext_info count. /Lars-Peter
> - unified "brag strings" in the file headers.
>
> v2 -> v3 changes
> - have the mux-controller in the parent node of any mux-controller consumer,
> to hopefully satisfy complaint from Rob about dt complexity.
> - improve commit message of the mux subsystem commit, making it more
> general, as requested by Jonathan.
> - remove priv member from struct mux_control and calculate it on the
> fly. /Jonathan
> - make the function comments in mux-core.c kernel doc. /Jonathan
> - add devm_mux_control_* to Documentation/driver.model/devres.txt. /Jonathan
> - add common dt bindings for mux-controllers, refer to them from the
> mux-gpio bindings. /Rob
> - clarify how the gpio pins map to the mux state. /Rob
> - separate CONFIG_ variables for the mux core and the mux gpio driver.
> - improve Kconfig help texts.
> - make CONFIG_MUX_GPIO depend on CONFIG_GPIOLIB.
> - keep track of the number of mux states in the mux core.
> - since the iio channel number is used as mux state, it was possible
> to drop the state member from the mux_child struct.
> - cleanup dt bindings for i2c-mux-simple, it had some of copy-paste
> problems from ots origin (i2c-mux-gpio).
> - select the mux control subsystem in config for the i2c-mux-simple driver.
> - add entries to MAINTAINERS and my sign-off, I'm now satisfied and know
> nothing in this to be ashamed of.
>
> v1 -> v2 changes
> - fixup export of mux_control_put reported by kbuild
> - drop devicetree iio-ext-info property as noted by Lars-Peter,
> and replace the functionality by exposing all ext_info
> attributes of the parent channel for each of the muxed
> channels. A cache on top of that and each muxed channel
> gets its own view of the ext_info of the parent channel.
> - implement idle-state for muxes
> - clear out the cache on failure in order to force a mux
> update on the following use
> - cleanup the probe of i2c-mux-simple driver
> - fix a bug in the i2c-mux-simple driver, where failure in
> the selection of the mux caused a deadlock when the mux
> was later unconditionally deselected.
>
> I have a piece of hardware that is using the same 3 GPIO pins
> to control four 8-way muxes. Three of them control ADC lines
> to an ADS1015 chip with an iio driver, and the last one
> controls the SDA line of an i2c bus. We have some deployed
> code to handle this, but you do not want to see it or ever
> hear about it. I'm not sure why I even mention it. Anyway,
> the situation has nagged me to no end for quite some time.
>
> So, after first getting more intimate with the i2c muxing code
> and later discovering the drivers/iio/inkern.c file and
> writing a couple of drivers making use of it, I came up with
> what I think is an acceptable solution; add a generic mux
> controller driver (and subsystem) that is shared between all
> instances, and combine that with an iio mux driver and a new
> generic i2c mux driver. The new i2c mux I called "simple"
> since it is only hooking the i2c muxing and the new mux
> controller (much like the alsa simple card driver does for ASoC).
>
> One thing that I would like to do, but don't see a solution
> for, is to move the mux control code that is present in
> various drivers in drivers/i2c/muxes to this new minimalistic
> muxing subsystem, thus converting all present i2c muxes (but
> perhaps not gates and arbitrators) to be i2c-mux-simple muxes.
>
> I'm using an rwsem to lock a mux, but that isn't really a
> perfect fit. Is there a better locking primitive that I don't
> know about that fits better? I had a mutex at one point, but
> that didn't allow any concurrent accesses at all. At least
> the rwsem allows concurrent access as long as all users
> agree on the mux state, but I suspect that the rwsem will
> degrade to the mutex situation pretty quickly if there is
> any contention.
>
> Also, the "mux" name feels a bit ambitious, there are many muxes
> in the world, and this tiny bit of code is probably not good
> enough to be a nice fit for all...
>
> Cheers,
> Peter
>
> Peter Rosin (9):
> dt-bindings: document devicetree bindings for mux-controllers and
> mux-gpio
> misc: minimal mux subsystem and gpio-based mux controller
> iio: inkern: api for manipulating ext_info of iio channels
> dt-bindings: iio: iio-mux: document iio-mux bindings
> iio: multiplexer: new iio category and iio-mux driver
> dt-bindings: i2c: i2c-mux-simple: document i2c-mux-simple bindings
> i2c: i2c-mux-simple: new driver
> dt-bindings: mux-adg792a: document devicetree bindings for ADG792A/G
> mux
> misc: mux-adg792a: add mux controller driver for ADG792A/G
>
> .../devicetree/bindings/i2c/i2c-mux-simple.txt | 81 ++++
> .../bindings/iio/multiplexer/iio-mux.txt | 40 ++
> .../devicetree/bindings/misc/mux-adg792a.txt | 64 +++
> .../devicetree/bindings/misc/mux-controller.txt | 127 ++++++
> .../devicetree/bindings/misc/mux-gpio.txt | 68 +++
> Documentation/driver-model/devres.txt | 6 +-
> MAINTAINERS | 14 +
> drivers/i2c/muxes/Kconfig | 13 +
> drivers/i2c/muxes/Makefile | 1 +
> drivers/i2c/muxes/i2c-mux-simple.c | 179 ++++++++
> drivers/iio/Kconfig | 1 +
> drivers/iio/Makefile | 1 +
> drivers/iio/inkern.c | 60 +++
> drivers/iio/multiplexer/Kconfig | 18 +
> drivers/iio/multiplexer/Makefile | 6 +
> drivers/iio/multiplexer/iio-mux.c | 456 +++++++++++++++++++++
> drivers/misc/Kconfig | 42 ++
> drivers/misc/Makefile | 3 +
> drivers/misc/mux-adg792a.c | 154 +++++++
> drivers/misc/mux-core.c | 311 ++++++++++++++
> drivers/misc/mux-gpio.c | 138 +++++++
> include/linux/iio/consumer.h | 37 ++
> include/linux/mux.h | 197 +++++++++
> 23 files changed, 2016 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
> create mode 100644 Documentation/devicetree/bindings/iio/multiplexer/iio-mux.txt
> create mode 100644 Documentation/devicetree/bindings/misc/mux-adg792a.txt
> create mode 100644 Documentation/devicetree/bindings/misc/mux-controller.txt
> create mode 100644 Documentation/devicetree/bindings/misc/mux-gpio.txt
> create mode 100644 drivers/i2c/muxes/i2c-mux-simple.c
> create mode 100644 drivers/iio/multiplexer/Kconfig
> create mode 100644 drivers/iio/multiplexer/Makefile
> create mode 100644 drivers/iio/multiplexer/iio-mux.c
> create mode 100644 drivers/misc/mux-adg792a.c
> create mode 100644 drivers/misc/mux-core.c
> create mode 100644 drivers/misc/mux-gpio.c
> create mode 100644 include/linux/mux.h
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH 0/1] i2c: designware-baytrail: Fix race when resetting the semaphore
From: Hans de Goede @ 2017-01-01 11:30 UTC (permalink / raw)
To: Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx
Hi All,
Here is one more i2c-designware-baytrail patch fixing a (potential) issue
I noticed while looking into i915 <-> pmic-i2c-bus-semaphore code
interactions.
Note this patch applies on top of my previous i2c-designware-baytrail
patches.
Regards,
Hans
^ permalink raw reply
* [PATCH] i2c: designware-baytrail: Fix race when resetting the semaphore
From: Hans de Goede @ 2017-01-01 11:30 UTC (permalink / raw)
To: Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx,
Hans de Goede
In-Reply-To: <20170101113022.6791-1-hdegoede@redhat.com>
Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so that
we keep the iosf_mbi_lock locked during the read-modify-write done to
reset the semaphore.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/i2c/busses/i2c-designware-baytrail.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 9481411..9634544 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -64,18 +64,16 @@ static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
static void reset_semaphore(struct dw_i2c_dev *dev)
{
- u32 addr = get_sem_addr(dev);
- u32 data;
+ int ret;
- if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, addr, &data)) {
- dev_err(dev->dev, "iosf failed to reset punit semaphore during read\n");
+ ret = iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, get_sem_addr(dev),
+ 0, PUNIT_SEMAPHORE_BIT);
+ if (ret) {
+ dev_err(dev->dev, "iosf failed to reset punit semaphore: %d\n",
+ ret);
return;
}
- data &= ~PUNIT_SEMAPHORE_BIT;
- if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, addr, data))
- dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
-
pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
}
--
2.9.3
^ permalink raw reply related
* Re: [PATCH v6 7/9] i2c: i2c-mux-simple: new driver
From: Andy Shevchenko @ 2017-01-01 15:38 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel@vger.kernel.org, Wolfram Sang, Rob Herring,
Mark Rutland, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, Linux Documentation List
In-Reply-To: <1480493823-21462-8-git-send-email-peda@axentia.se>
On Wed, Nov 30, 2016 at 10:17 AM, Peter Rosin <peda@axentia.se> wrote:
> This is a generic simple i2c mux that uses the generic multiplexer
> subsystem to do the muxing.
> +static const struct of_device_id i2c_mux_of_match[] = {
> + { .compatible = "i2c-mux-simple,parent-locked",
> + .data = (void *)0, },
> + { .compatible = "i2c-mux-simple,mux-locked",
> + .data = (void *)1, },
> + {},
> +};
Perhaps
#define I2C_MUX_LOCKED_PARENT 0
#define I2C_MUX_LOCKED 1
?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [RFC 0/4] Coordinate pmic i2c bus and i915 punit accesses
From: Hans de Goede @ 2017-01-01 20:13 UTC (permalink / raw)
To: Jarkko Nikula, Len Brown
Cc: Jani Nikula, Ville Syrjälä,
russianneuromancer @ ya . ru, linux-i2c, intel-gfx
Hi,
So one user is reporting problems with my patches to get the
i2c pmic bus (cherrytrail punit semaphore support) + axp288 fuel_gauge /
charger drivers working in combination with i915:
https://bugzilla.kernel.org/show_bug.cgi?id=155241#c37
"My device is a laptop with no USB charging or OTG. So, I'd tried only SDIO _ADR patch, i2c and axp288_fuel_gauge patches. Everything works well for upto 3 mins after boot, then the device freezes. I hadn't tried any drm patches BTW. Here's the log:
[drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
clocksource: timekeeping watchdog on CPU0: Marking clocksource 'tsc' as unstable because the skew is too large:
clocksource: 'refined-jiffies' wd_now: 10002ee30 wd_last: 10002edb8 mask: ffffffff
clocksource: 'tsc' cs_now: 16ac2c7744a cs_last: 16a8d9bd8f2 mask: ffffffffffffffff
clocksource: Switched to clocksource refined-jiffies
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 2
i2c_designware 808622C1:06: couldn't acquire bus ownership
axp288_fuel_gauge axp288_fuel_gauge: axp288 reg read err:-110
axp288_fuel_gauge axp288_fuel_gauge: PWR STAT read failed:-110
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
usb 1-2: reset high-speed USB device number 2 using xhci_hcd
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 0
i2c_designware 808622C1:06: couldn't acquire bus ownership
axp288_fuel_gauge axp288_fuel_gauge: IIO channel read error: fffffffb, 0
power_supply axp288_fuel_gauge: driver failed to report `voltage_now' property: -5
***SYSTEM FREEZE***
If I blacklist axp288_fuel_gauge, then there were no errors."
This sounds a lot like things go wrong when the i915 driver asks some
changes from the punit while i2c-designware-baytrail is holding the
pmic i2c bus semaphore, just like changing C-states while holding
the semaphore seems to lock up the punit,
So this patch-set is an attempt at fixing that. I'm still waiting for the
reporter to report back if it actually fixes things, in the mean time
any input on this issue (or the proposed fix) is welcome.
Note the i2c-designware-baytrail patch applies on top of v5 of my
i2c-designware series, which I will send out directly after this.
Regards,
Hans
^ permalink raw reply
* [RFC 1/4] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Hans de Goede @ 2017-01-01 20:14 UTC (permalink / raw)
To: Jarkko Nikula, Len Brown
Cc: Jani Nikula, Ville Syrjälä,
russianneuromancer @ ya . ru, linux-i2c, intel-gfx, Hans de Goede
In-Reply-To: <20170101201403.12132-1-hdegoede@redhat.com>
The punit on baytrail / cherrytrail systems is not only accessed through
the iosf_mbi functions, but also by the i915 code. Add a mutex to protect
the punit against simultaneous accesses and 2 functions to lock / unlock
this mutex.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
arch/x86/include/asm/iosf_mbi.h | 19 +++++++++++++++++++
arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++
2 files changed, 32 insertions(+)
diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index b41ee16..02963bd 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -88,6 +88,21 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr);
*/
int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask);
+/**
+ * iosf_mbi_punit_lock() - Lock the punit mutex
+ *
+ * This function must be called before accessing the punit or the pmic, be it
+ * through iosf_mbi_* or through other means.
+ *
+ * This function locks a mutex, as such it may sleep.
+ */
+void iosf_mbi_punit_lock(void);
+
+/**
+ * iosf_mbi_punit_unlock() - Unlock the punit mutex
+ */
+void iosf_mbi_punit_unlock(void);
+
#else /* CONFIG_IOSF_MBI is not enabled */
static inline
bool iosf_mbi_available(void)
@@ -115,6 +130,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask)
WARN(1, "IOSF_MBI driver not available");
return -EPERM;
}
+
+static inline void iosf_mbi_punit_lock(void) {}
+static inline void iosf_mbi_punit_unlock(void) {}
+
#endif /* CONFIG_IOSF_MBI */
#endif /* IOSF_MBI_SYMS_H */
diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
index edf2c54..75d8135 100644
--- a/arch/x86/platform/intel/iosf_mbi.c
+++ b/arch/x86/platform/intel/iosf_mbi.c
@@ -34,6 +34,7 @@
static struct pci_dev *mbi_pdev;
static DEFINE_SPINLOCK(iosf_mbi_lock);
+static DEFINE_MUTEX(iosf_mbi_punit_mutex);
static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
{
@@ -190,6 +191,18 @@ bool iosf_mbi_available(void)
}
EXPORT_SYMBOL(iosf_mbi_available);
+void iosf_mbi_punit_lock(void)
+{
+ mutex_lock(&iosf_mbi_punit_mutex);
+}
+EXPORT_SYMBOL(iosf_mbi_punit_lock);
+
+void iosf_mbi_punit_unlock(void)
+{
+ mutex_unlock(&iosf_mbi_punit_mutex);
+}
+EXPORT_SYMBOL(iosf_mbi_punit_unlock);
+
#ifdef CONFIG_IOSF_MBI_DEBUG
static u32 dbg_mdr;
static u32 dbg_mcr;
--
2.9.3
^ permalink raw reply related
* [RFC 2/4] i2c: designware-baytrail: Take punit lock on bus acquire
From: Hans de Goede @ 2017-01-01 20:14 UTC (permalink / raw)
To: Jarkko Nikula, Len Brown
Cc: Jani Nikula, Ville Syrjälä,
russianneuromancer @ ya . ru, linux-i2c, intel-gfx, Hans de Goede
In-Reply-To: <20170101201403.12132-1-hdegoede@redhat.com>
Take the punit lock to stop others from accessing the punit while the
pmic i2c bus is in use. This is necessary because accessing the punit
from the kernel may result in the punit trying to access the pmic i2c
bus, which results in a hang when it happens while we own the pmic i2c
bus semaphore.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 3effc9a..507875d 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -61,6 +61,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
0, PUNIT_SEMAPHORE_BIT))
dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
+ iosf_mbi_punit_unlock();
+
pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
}
@@ -86,6 +88,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
*/
pm_qos_update_request(&dev->pm_qos, 0);
+ iosf_mbi_punit_lock();
+
/* host driver writes to side band semaphore register */
ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, addr, sem);
if (ret) {
--
2.9.3
^ permalink raw reply related
* [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA
From: Hans de Goede @ 2017-01-01 20:14 UTC (permalink / raw)
To: Jarkko Nikula, Len Brown
Cc: Jani Nikula, Ville Syrjälä,
russianneuromancer @ ya . ru, linux-i2c, intel-gfx, Hans de Goede
In-Reply-To: <20170101201403.12132-1-hdegoede@redhat.com>
All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
for intel_set_rps(). Since intel_set_rps can for example be called from
sysfs store functions, there is no guarantee this is already done, so add
an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4b12637..cc4fbd7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
{
- if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+ if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+ /* Wake up the media well, as that takes a lot less
+ * power than the Render well.
+ */
+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
valleyview_set_rps(dev_priv, val);
- else
+ intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA);
+ } else
gen6_set_rps(dev_priv, val);
}
--
2.9.3
^ permalink raw reply related
* [RFC 4/4] drm/i915: valleyview: Take punit lock when modifying punit settings
From: Hans de Goede @ 2017-01-01 20:14 UTC (permalink / raw)
To: Jarkko Nikula, Len Brown
Cc: Jani Nikula, Ville Syrjälä,
russianneuromancer @ ya . ru, linux-i2c, intel-gfx, Hans de Goede
In-Reply-To: <20170101201403.12132-1-hdegoede@redhat.com>
Make sure the punit i2c bus is not in use when we send a request to
the punit by calling iosf_mbi_punit_lock() / iosf_mbi_punit_unlock()
around punit write accesses.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/gpu/drm/i915/Kconfig | 1 +
drivers/gpu/drm/i915/intel_display.c | 6 ++++++
drivers/gpu/drm/i915/intel_pm.c | 21 +++++++++++++++++++++
drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
4 files changed, 37 insertions(+)
diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 5ddde73..0fe7443 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -4,6 +4,7 @@ config DRM_I915
depends on X86 && PCI
select INTEL_GTT
select INTERVAL_TREE
+ select IOSF_MBI
# we need shmfs for the swappable backing store, and in particular
# the shmem_readpage() which depends upon tmpfs
select SHMEM
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fec8eb3..b8be6ea 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -47,6 +47,7 @@
#include <drm/drm_rect.h>
#include <linux/dma_remapping.h>
#include <linux/reservation.h>
+#include <asm/iosf_mbi.h>
static bool is_mmio_work(struct intel_flip_work *work)
{
@@ -6423,6 +6424,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
cmd = 0;
mutex_lock(&dev_priv->rps.hw_lock);
+ iosf_mbi_punit_lock();
+
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
val &= ~DSPFREQGUAR_MASK;
val |= (cmd << DSPFREQGUAR_SHIFT);
@@ -6432,6 +6435,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
50)) {
DRM_ERROR("timed out waiting for CDclk change\n");
}
+ iosf_mbi_punit_unlock();
mutex_unlock(&dev_priv->rps.hw_lock);
mutex_lock(&dev_priv->sb_lock);
@@ -6499,6 +6503,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
mutex_lock(&dev_priv->rps.hw_lock);
+ iosf_mbi_punit_lock();
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
val &= ~DSPFREQGUAR_MASK_CHV;
val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
@@ -6508,6 +6513,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
50)) {
DRM_ERROR("timed out waiting for CDclk change\n");
}
+ iosf_mbi_punit_unlock();
mutex_unlock(&dev_priv->rps.hw_lock);
intel_update_cdclk(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index cc4fbd7..31f88f1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -32,6 +32,7 @@
#include "../../../platform/x86/intel_ips.h"
#include <linux/module.h>
#include <drm/drm_atomic_helper.h>
+#include <asm/iosf_mbi.h>
/**
* DOC: RC6
@@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
u32 val;
mutex_lock(&dev_priv->rps.hw_lock);
+ iosf_mbi_punit_lock();
val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
if (enable)
@@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
+ iosf_mbi_punit_unlock();
mutex_unlock(&dev_priv->rps.hw_lock);
}
@@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
u32 val;
mutex_lock(&dev_priv->rps.hw_lock);
+ iosf_mbi_punit_lock();
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
if (enable)
@@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
val &= ~DSP_MAXFIFO_PM5_ENABLE;
vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
+ iosf_mbi_punit_unlock();
mutex_unlock(&dev_priv->rps.hw_lock);
}
@@ -4546,6 +4551,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
if (IS_CHERRYVIEW(dev_priv)) {
mutex_lock(&dev_priv->rps.hw_lock);
+ iosf_mbi_punit_lock();
val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
if (val & DSP_MAXFIFO_PM5_ENABLE)
@@ -4575,6 +4581,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
wm->level = VLV_WM_LEVEL_DDR_DVFS;
}
+ iosf_mbi_punit_unlock();
mutex_unlock(&dev_priv->rps.hw_lock);
}
@@ -5004,11 +5011,15 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
if (dev_priv->rps.cur_freq <= val)
return;
+ iosf_mbi_punit_lock();
+
/* Wake up the media well, as that takes a lot less
* power than the Render well. */
intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
valleyview_set_rps(dev_priv, val);
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA);
+
+ iosf_mbi_punit_unlock();
}
void gen6_rps_busy(struct drm_i915_private *dev_priv)
@@ -5097,12 +5108,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
{
if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+ iosf_mbi_punit_lock();
/* Wake up the media well, as that takes a lot less
* power than the Render well.
*/
intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
valleyview_set_rps(dev_priv, val);
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA);
+ iosf_mbi_punit_unlock();
} else
gen6_set_rps(dev_priv, val);
}
@@ -5999,6 +6012,8 @@ static void cherryview_enable_rps(struct drm_i915_private *dev_priv)
cherryview_check_pctx(dev_priv);
+ iosf_mbi_punit_lock();
+
/* 1a & 1b: Get forcewake during program sequence. Although the driver
* hasn't enabled a state yet where we need forcewake, BIOS may have.*/
intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
@@ -6068,6 +6083,8 @@ static void cherryview_enable_rps(struct drm_i915_private *dev_priv)
reset_rps(dev_priv, valleyview_set_rps);
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+
+ iosf_mbi_punit_unlock();
}
static void valleyview_enable_rps(struct drm_i915_private *dev_priv)
@@ -6087,6 +6104,8 @@ static void valleyview_enable_rps(struct drm_i915_private *dev_priv)
I915_WRITE(GTFIFODBG, gtfifodbg);
}
+ iosf_mbi_punit_lock();
+
/* If VLV, Forcewake all wells, else re-direct to regular path */
intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
@@ -6149,6 +6168,8 @@ static void valleyview_enable_rps(struct drm_i915_private *dev_priv)
reset_rps(dev_priv, valleyview_set_rps);
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+
+ iosf_mbi_punit_unlock();
}
static unsigned long intel_pxfreq(u32 vidfreq)
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index c0b7e95..17922ae 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -28,6 +28,7 @@
#include <linux/pm_runtime.h>
#include <linux/vgaarb.h>
+#include <asm/iosf_mbi.h>
#include "i915_drv.h"
#include "intel_drv.h"
@@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
if (COND)
goto out;
+ iosf_mbi_punit_lock();
+
ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
ctrl &= ~mask;
ctrl |= state;
@@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
state,
vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
+ iosf_mbi_punit_unlock();
+
#undef COND
out:
@@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
if (COND)
goto out;
+ iosf_mbi_punit_lock();
+
ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
ctrl &= ~DP_SSC_MASK(pipe);
ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
@@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
state,
vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
+ iosf_mbi_punit_unlock();
+
#undef COND
out:
--
2.9.3
^ permalink raw reply related
* [PATCH v5 6/6] i2c: designware-baytrail: Add support for cherrytrail
From: Hans de Goede @ 2017-01-01 20:15 UTC (permalink / raw)
To: Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Takashi Iwai, russianneuromancer @ ya . ru, intel-gfx,
Hans de Goede, linux-i2c, Mika Westerberg
In-Reply-To: <20170101201521.12364-1-hdegoede@redhat.com>
The cherrytrail punit has the pmic i2c bus access semaphore at a
different register address.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
Changes in v2:
-Adjust for accessor_flags -> flags rename
-Add flags field to struct dw_pci_controller
-Add get_sem_addr() helper replacing MODEL_CHERRYTRAIL flag checking in
PUNIT_SEMAPHORE macro
Changes in v3:
-Add a gap between ACCESS_* and MODEL_* flags as reserved space for
future ACCESS_* flags
Changes in v4:
-s/CHV/CHT/
---
drivers/i2c/busses/i2c-designware-baytrail.c | 19 +++++++++++++++----
drivers/i2c/busses/i2c-designware-core.h | 2 ++
drivers/i2c/busses/i2c-designware-pcidrv.c | 26 +++++++++++++++++++-------
drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
4 files changed, 37 insertions(+), 12 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 8df529c..3effc9a 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -24,17 +24,27 @@
#define SEMAPHORE_TIMEOUT 100
#define PUNIT_SEMAPHORE 0x7
+#define PUNIT_SEMAPHORE_CHT 0x10e
#define PUNIT_SEMAPHORE_BIT BIT(0)
#define PUNIT_SEMAPHORE_ACQUIRE BIT(1)
static unsigned long acquired;
+static u32 get_sem_addr(struct dw_i2c_dev *dev)
+{
+ if (dev->flags & MODEL_CHERRYTRAIL)
+ return PUNIT_SEMAPHORE_CHT;
+ else
+ return PUNIT_SEMAPHORE;
+}
+
static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
{
+ u32 addr = get_sem_addr(dev);
u32 data;
int ret;
- ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data);
+ ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, addr, &data);
if (ret) {
dev_err(dev->dev, "iosf failed to read punit semaphore\n");
return ret;
@@ -47,7 +57,7 @@ static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
static void reset_semaphore(struct dw_i2c_dev *dev)
{
- if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE,
+ if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, get_sem_addr(dev),
0, PUNIT_SEMAPHORE_BIT))
dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
@@ -56,6 +66,7 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
{
+ u32 addr = get_sem_addr(dev);
u32 sem = PUNIT_SEMAPHORE_ACQUIRE;
int ret;
unsigned long start, end;
@@ -76,7 +87,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
pm_qos_update_request(&dev->pm_qos, 0);
/* host driver writes to side band semaphore register */
- ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, sem);
+ ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, addr, sem);
if (ret) {
dev_err(dev->dev, "iosf punit semaphore request failed\n");
goto out;
@@ -101,7 +112,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
out:
reset_semaphore(dev);
- ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &sem);
+ ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, addr, &sem);
if (ret)
dev_err(dev->dev, "iosf failed to read punit semaphore\n");
else
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 871970e..cd4a874 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -127,6 +127,8 @@ struct dw_i2c_dev {
#define ACCESS_16BIT 0x00000002
#define ACCESS_INTR_MASK 0x00000004
+#define MODEL_CHERRYTRAIL 0x00000100
+
extern int i2c_dw_init(struct dw_i2c_dev *dev);
extern void i2c_dw_disable(struct dw_i2c_dev *dev);
extern void i2c_dw_disable_int(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 96f8230..4e53a9f 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -45,6 +45,7 @@ enum dw_pci_ctl_id_t {
medfield,
merrifield,
baytrail,
+ cherrytrail,
haswell,
};
@@ -63,6 +64,7 @@ struct dw_pci_controller {
u32 rx_fifo_depth;
u32 clk_khz;
u32 functionality;
+ u32 flags;
struct dw_scl_sda_cfg *scl_sda_cfg;
int (*setup)(struct pci_dev *pdev, struct dw_pci_controller *c);
};
@@ -174,6 +176,15 @@ static struct dw_pci_controller dw_pci_controllers[] = {
.functionality = I2C_FUNC_10BIT_ADDR,
.scl_sda_cfg = &hsw_config,
},
+ [cherrytrail] = {
+ .bus_num = -1,
+ .bus_cfg = INTEL_MID_STD_CFG | DW_IC_CON_SPEED_FAST,
+ .tx_fifo_depth = 32,
+ .rx_fifo_depth = 32,
+ .functionality = I2C_FUNC_10BIT_ADDR,
+ .flags = MODEL_CHERRYTRAIL,
+ .scl_sda_cfg = &byt_config,
+ },
};
#ifdef CONFIG_PM
@@ -241,6 +252,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
dev->base = pcim_iomap_table(pdev)[0];
dev->dev = &pdev->dev;
dev->irq = pdev->irq;
+ dev->flags |= controller->flags;
if (controller->setup) {
r = controller->setup(pdev, controller);
@@ -321,13 +333,13 @@ static const struct pci_device_id i2_designware_pci_ids[] = {
{ PCI_VDEVICE(INTEL, 0x9c61), haswell },
{ PCI_VDEVICE(INTEL, 0x9c62), haswell },
/* Braswell / Cherrytrail */
- { PCI_VDEVICE(INTEL, 0x22C1), baytrail },
- { PCI_VDEVICE(INTEL, 0x22C2), baytrail },
- { PCI_VDEVICE(INTEL, 0x22C3), baytrail },
- { PCI_VDEVICE(INTEL, 0x22C4), baytrail },
- { PCI_VDEVICE(INTEL, 0x22C5), baytrail },
- { PCI_VDEVICE(INTEL, 0x22C6), baytrail },
- { PCI_VDEVICE(INTEL, 0x22C7), baytrail },
+ { PCI_VDEVICE(INTEL, 0x22C1), cherrytrail },
+ { PCI_VDEVICE(INTEL, 0x22C2), cherrytrail },
+ { PCI_VDEVICE(INTEL, 0x22C3), cherrytrail },
+ { PCI_VDEVICE(INTEL, 0x22C4), cherrytrail },
+ { PCI_VDEVICE(INTEL, 0x22C5), cherrytrail },
+ { PCI_VDEVICE(INTEL, 0x22C6), cherrytrail },
+ { PCI_VDEVICE(INTEL, 0x22C7), cherrytrail },
{ 0,}
};
MODULE_DEVICE_TABLE(pci, i2_designware_pci_ids);
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index b0a64a2..985c4de 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -123,7 +123,7 @@ static const struct acpi_device_id dw_i2c_acpi_match[] = {
{ "INT3432", 0 },
{ "INT3433", 0 },
{ "80860F41", 0 },
- { "808622C1", 0 },
+ { "808622C1", MODEL_CHERRYTRAIL },
{ "AMD0010", ACCESS_INTR_MASK },
{ "AMDI0010", ACCESS_INTR_MASK },
{ "AMDI0510", 0 },
--
2.9.3
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related
* [PATCH v5 1/6] i2c: designware: Rename accessor_flags to flags
From: Hans de Goede @ 2017-01-01 20:15 UTC (permalink / raw)
To: Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx,
Hans de Goede
Rename accessor_flags to flags, so that we can use the field for
other flags too. This is a preparation patch for adding cherrytrail
support to the punit semaphore code.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Tested-by: Takashi Iwai <tiwai@suse.de>
---
drivers/i2c/busses/i2c-designware-core.c | 14 +++++++-------
drivers/i2c/busses/i2c-designware-core.h | 2 +-
drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index b403fa5..b6a7989 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -177,13 +177,13 @@ static u32 dw_readl(struct dw_i2c_dev *dev, int offset)
{
u32 value;
- if (dev->accessor_flags & ACCESS_16BIT)
+ if (dev->flags & ACCESS_16BIT)
value = readw_relaxed(dev->base + offset) |
(readw_relaxed(dev->base + offset + 2) << 16);
else
value = readl_relaxed(dev->base + offset);
- if (dev->accessor_flags & ACCESS_SWAP)
+ if (dev->flags & ACCESS_SWAP)
return swab32(value);
else
return value;
@@ -191,10 +191,10 @@ static u32 dw_readl(struct dw_i2c_dev *dev, int offset)
static void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset)
{
- if (dev->accessor_flags & ACCESS_SWAP)
+ if (dev->flags & ACCESS_SWAP)
b = swab32(b);
- if (dev->accessor_flags & ACCESS_16BIT) {
+ if (dev->flags & ACCESS_16BIT) {
writew_relaxed((u16)b, dev->base + offset);
writew_relaxed((u16)(b >> 16), dev->base + offset + 2);
} else {
@@ -339,10 +339,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
reg = dw_readl(dev, DW_IC_COMP_TYPE);
if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
/* Configure register endianess access */
- dev->accessor_flags |= ACCESS_SWAP;
+ dev->flags |= ACCESS_SWAP;
} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
/* Configure register access mode 16bit */
- dev->accessor_flags |= ACCESS_16BIT;
+ dev->flags |= ACCESS_16BIT;
} else if (reg != DW_IC_COMP_TYPE_VALUE) {
dev_err(dev->dev, "Unknown Synopsys component type: "
"0x%08x\n", reg);
@@ -886,7 +886,7 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
tx_aborted:
if ((stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET)) || dev->msg_err)
complete(&dev->cmd_complete);
- else if (unlikely(dev->accessor_flags & ACCESS_INTR_MASK)) {
+ else if (unlikely(dev->flags & ACCESS_INTR_MASK)) {
/* workaround to trigger pending interrupt */
stat = dw_readl(dev, DW_IC_INTR_MASK);
i2c_dw_disable_int(dev);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 0d44d2a..fb143f5 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -95,7 +95,7 @@ struct dw_i2c_dev {
unsigned int status;
u32 abort_source;
int irq;
- u32 accessor_flags;
+ u32 flags;
struct i2c_adapter adapter;
u32 functionality;
u32 master_cfg;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0b42a12..97a2ca1 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -112,7 +112,7 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
if (id && id->driver_data)
- dev->accessor_flags |= (u32)id->driver_data;
+ dev->flags |= (u32)id->driver_data;
return 0;
}
--
2.9.3
^ permalink raw reply related
* [PATCH v5 2/6] i2c: designware-baytrail: Pass dw_i2c_dev into helper functions
From: Hans de Goede @ 2017-01-01 20:15 UTC (permalink / raw)
To: Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx,
Hans de Goede
In-Reply-To: <20170101201521.12364-1-hdegoede@redhat.com>
Pass dw_i2c_dev into the helper functions, this is a preparation patch
for the punit semaphore fixes done in the other patches in this set.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
drivers/i2c/busses/i2c-designware-baytrail.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 1590ad0..a3f581c 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -28,14 +28,14 @@
static unsigned long acquired;
-static int get_sem(struct device *dev, u32 *sem)
+static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
{
u32 data;
int ret;
ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data);
if (ret) {
- dev_err(dev, "iosf failed to read punit semaphore\n");
+ dev_err(dev->dev, "iosf failed to read punit semaphore\n");
return ret;
}
@@ -44,18 +44,18 @@ static int get_sem(struct device *dev, u32 *sem)
return 0;
}
-static void reset_semaphore(struct device *dev)
+static void reset_semaphore(struct dw_i2c_dev *dev)
{
u32 data;
if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data)) {
- dev_err(dev, "iosf failed to reset punit semaphore during read\n");
+ dev_err(dev->dev, "iosf failed to reset punit semaphore during read\n");
return;
}
data &= ~PUNIT_SEMAPHORE_BIT;
if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, data))
- dev_err(dev, "iosf failed to reset punit semaphore during write\n");
+ dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
}
static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
@@ -83,7 +83,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
start = jiffies;
end = start + msecs_to_jiffies(SEMAPHORE_TIMEOUT);
do {
- ret = get_sem(dev->dev, &sem);
+ ret = get_sem(dev, &sem);
if (!ret && sem) {
acquired = jiffies;
dev_dbg(dev->dev, "punit semaphore acquired after %ums\n",
@@ -95,7 +95,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
} while (time_before(jiffies, end));
dev_err(dev->dev, "punit semaphore timed out, resetting\n");
- reset_semaphore(dev->dev);
+ reset_semaphore(dev);
ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &sem);
if (ret)
@@ -116,7 +116,7 @@ static void baytrail_i2c_release(struct dw_i2c_dev *dev)
if (!dev->acquire_lock)
return;
- reset_semaphore(dev->dev);
+ reset_semaphore(dev);
dev_dbg(dev->dev, "punit semaphore held for %ums\n",
jiffies_to_msecs(jiffies - acquired));
}
--
2.9.3
^ permalink raw reply related
* [PATCH v5 3/6] i2c: designware-baytrail: Only check iosf_mbi_available() for shared hosts
From: Hans de Goede @ 2017-01-01 20:15 UTC (permalink / raw)
To: Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx,
Hans de Goede
In-Reply-To: <20170101201521.12364-1-hdegoede@redhat.com>
If (!shared_host) simply return 0, this avoids delaying the probe if
iosf_mbi_available() returns false when an i2c bus is not using the
punit semaphore.
Also move the if (!iosf_mbi_available()) check to above the
dev_info, so that we do not repeat the dev_info on every probe
until iosf_mbi_available() returns true.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Tested-by: Takashi Iwai <tiwai@suse.de>
---
Changes in v2:
-New patch in v2 of this set
Changes in v3:
-Use if (!shared_host) return 0, to simplify the non-shared_host path
and to avoid nested ifs
---
drivers/i2c/busses/i2c-designware-baytrail.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index a3f581c..cf02222 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -138,15 +138,16 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
if (ACPI_FAILURE(status))
return 0;
- if (shared_host) {
- dev_info(dev->dev, "I2C bus managed by PUNIT\n");
- dev->acquire_lock = baytrail_i2c_acquire;
- dev->release_lock = baytrail_i2c_release;
- dev->pm_runtime_disabled = true;
- }
+ if (!shared_host)
+ return 0;
if (!iosf_mbi_available())
return -EPROBE_DEFER;
+ dev_info(dev->dev, "I2C bus managed by PUNIT\n");
+ dev->acquire_lock = baytrail_i2c_acquire;
+ dev->release_lock = baytrail_i2c_release;
+ dev->pm_runtime_disabled = true;
+
return 0;
}
--
2.9.3
^ permalink raw reply related
* [PATCH v5 4/6] i2c: designware-baytrail: Disallow the CPU to enter C6 or C7 while holding the punit semaphore
From: Hans de Goede @ 2017-01-01 20:15 UTC (permalink / raw)
To: Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx,
Hans de Goede
In-Reply-To: <20170101201521.12364-1-hdegoede@redhat.com>
On my cherrytrail tablet with axp288 pmic, just doing a bunch of repeated
reads from the pmic, e.g. "i2cdump -y 14 0x34" would lookup the tablet in
1 - 3 runs guaranteed.
This seems to be causes by the cpu trying to enter C6 or C7 while we hold
the punit bus semaphore, at which point everything just hangs.
Avoid this by disallowing the CPU to enter C6 or C7 before acquiring the
punit bus semaphore.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109051
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Takashi Iwai <tiwai@suse.de>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
Changes in v2:
-New patch in v2 of this set
Changes in v3:
-Change commit message and comment in the code from "force the CPU to C1"
to "Disallow the CPU to enter C6 or C7", as the CPU may still be in either
C0 or C1 with the request pm_qos
Changes in v4:
-Rename i2c_dw_eval_lock_support to i2c_dw_probe_lock_support so that we can
add a matching i2c_dw_remove_lock_support cleanup function
-Move qm_pos removal to new i2c_dw_remove_lock_support function
-Move pm_qos_add_request to the end of i2c_dw_probe_lock_support
Changes in v5:
-Update the pm_qos for a latency of 0 *before* requesting the semaphore,
instead of doing it while waiting for the request to be acked
---
drivers/i2c/busses/i2c-designware-baytrail.c | 24 ++++++++++++++++++++++--
drivers/i2c/busses/i2c-designware-core.h | 9 +++++++--
drivers/i2c/busses/i2c-designware-platdrv.c | 4 +++-
3 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index cf02222..650a700 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -16,6 +16,7 @@
#include <linux/acpi.h>
#include <linux/i2c.h>
#include <linux/interrupt.h>
+#include <linux/pm_qos.h>
#include <asm/iosf_mbi.h>
@@ -56,6 +57,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
data &= ~PUNIT_SEMAPHORE_BIT;
if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, data))
dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
+
+ pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
}
static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
@@ -72,11 +75,18 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
if (!dev->release_lock)
return 0;
+ /*
+ * Disallow the CPU to enter C6 or C7 state, entering these states
+ * requires the punit to talk to the pmic and if this happens while
+ * we're holding the semaphore, the SoC hangs.
+ */
+ pm_qos_update_request(&dev->pm_qos, 0);
+
/* host driver writes to side band semaphore register */
ret = iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, sem);
if (ret) {
dev_err(dev->dev, "iosf punit semaphore request failed\n");
- return ret;
+ goto out;
}
/* host driver waits for bit 0 to be set in semaphore register */
@@ -95,6 +105,7 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
} while (time_before(jiffies, end));
dev_err(dev->dev, "punit semaphore timed out, resetting\n");
+out:
reset_semaphore(dev);
ret = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &sem);
@@ -121,7 +132,7 @@ static void baytrail_i2c_release(struct dw_i2c_dev *dev)
jiffies_to_msecs(jiffies - acquired));
}
-int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
+int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev)
{
acpi_status status;
unsigned long long shared_host = 0;
@@ -149,5 +160,14 @@ int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev)
dev->release_lock = baytrail_i2c_release;
dev->pm_runtime_disabled = true;
+ pm_qos_add_request(&dev->pm_qos, PM_QOS_CPU_DMA_LATENCY,
+ PM_QOS_DEFAULT_VALUE);
+
return 0;
}
+
+void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev)
+{
+ if (dev->acquire_lock)
+ pm_qos_remove_request(&dev->pm_qos);
+}
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index fb143f5..871970e 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -22,6 +22,7 @@
*
*/
+#include <linux/pm_qos.h>
#define DW_IC_CON_MASTER 0x1
#define DW_IC_CON_SPEED_STD 0x2
@@ -67,6 +68,7 @@
* @fp_lcnt: fast plus LCNT value
* @hs_hcnt: high speed HCNT value
* @hs_lcnt: high speed LCNT value
+ * @pm_qos: pm_qos_request used while holding a hardware lock on the bus
* @acquire_lock: function to acquire a hardware lock on the bus
* @release_lock: function to release a hardware lock on the bus
* @pm_runtime_disabled: true if pm runtime is disabled
@@ -114,6 +116,7 @@ struct dw_i2c_dev {
u16 fp_lcnt;
u16 hs_hcnt;
u16 hs_lcnt;
+ struct pm_qos_request pm_qos;
int (*acquire_lock)(struct dw_i2c_dev *dev);
void (*release_lock)(struct dw_i2c_dev *dev);
bool pm_runtime_disabled;
@@ -131,7 +134,9 @@ extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev);
extern int i2c_dw_probe(struct dw_i2c_dev *dev);
#if IS_ENABLED(CONFIG_I2C_DESIGNWARE_BAYTRAIL)
-extern int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev);
+extern int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev);
+extern void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev);
#else
-static inline int i2c_dw_eval_lock_support(struct dw_i2c_dev *dev) { return 0; }
+static inline int i2c_dw_probe_lock_support(struct dw_i2c_dev *dev) { return 0; }
+static inline void i2c_dw_remove_lock_support(struct dw_i2c_dev *dev) {}
#endif
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 97a2ca1..b0a64a2 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -210,7 +210,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
return -EINVAL;
}
- r = i2c_dw_eval_lock_support(dev);
+ r = i2c_dw_probe_lock_support(dev);
if (r)
return r;
@@ -291,6 +291,8 @@ static int dw_i2c_plat_remove(struct platform_device *pdev)
if (!dev->pm_runtime_disabled)
pm_runtime_disable(&pdev->dev);
+ i2c_dw_remove_lock_support(dev);
+
return 0;
}
--
2.9.3
^ permalink raw reply related
* [PATCH v5 5/6] i2c: designware-baytrail: Fix race when resetting the semaphore
From: Hans de Goede @ 2017-01-01 20:15 UTC (permalink / raw)
To: Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Mika Westerberg, Jani Nikula, Ville Syrjälä,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c, intel-gfx,
Hans de Goede
In-Reply-To: <20170101201521.12364-1-hdegoede@redhat.com>
Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so that
we keep the iosf_mbi_lock locked during the read-modify-write done to
reset the semaphore.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
-New patch in v5 of this patch-set
---
drivers/i2c/busses/i2c-designware-baytrail.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 650a700..8df529c 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -47,15 +47,8 @@ static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
static void reset_semaphore(struct dw_i2c_dev *dev)
{
- u32 data;
-
- if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data)) {
- dev_err(dev->dev, "iosf failed to reset punit semaphore during read\n");
- return;
- }
-
- data &= ~PUNIT_SEMAPHORE_BIT;
- if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, data))
+ if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE,
+ 0, PUNIT_SEMAPHORE_BIT))
dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
--
2.9.3
^ permalink raw reply related
* Re: [Intel-gfx] [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA
From: Chris Wilson @ 2017-01-01 20:24 UTC (permalink / raw)
To: Hans de Goede
Cc: Jarkko Nikula, Len Brown, russianneuromancer @ ya . ru, intel-gfx,
linux-i2c
In-Reply-To: <20170101201403.12132-4-hdegoede@redhat.com>
On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> for intel_set_rps(). Since intel_set_rps can for example be called from
> sysfs store functions, there is no guarantee this is already done, so add
> an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4b12637..cc4fbd7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
>
> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> {
> - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> + /* Wake up the media well, as that takes a lot less
> + * power than the Render well.
> + */
> + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> valleyview_set_rps(dev_priv, val);
Both powerwells are woken for rps. (Taking one but not the other has no
benefit, and very misleading.) The forcewake is already held by the
lower level routines, taking the wakelock in the caller is an optimisation
that is only interesting if there is a danger from the forcewake being
dropped mid-sequence (due to preemption whatever).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply
* Re: [RFC 3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA
From: Hans de Goede @ 2017-01-01 20:48 UTC (permalink / raw)
To: Chris Wilson, Jarkko Nikula, Len Brown,
russianneuromancer @ ya . ru, intel-gfx, linux-i2c
In-Reply-To: <20170101202400.GG13154@nuc-i3427.alporthouse.com>
Hi,
On 01-01-17 21:24, Chris Wilson wrote:
> On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
>> All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
>> for intel_set_rps(). Since intel_set_rps can for example be called from
>> sysfs store functions, there is no guarantee this is already done, so add
>> an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 4b12637..cc4fbd7 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
>>
>> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
>> {
>> - if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> + if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>> + /* Wake up the media well, as that takes a lot less
>> + * power than the Render well.
>> + */
>> + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
>> valleyview_set_rps(dev_priv, val);
>
> Both powerwells are woken for rps. (Taking one but not the other has no
> benefit, and very misleading.)
The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
existing code in vlv_set_rps_idle().
> The forcewake is already held by the
> lower level routines, taking the wakelock in the caller is an optimisation
> that is only interesting if there is a danger from the forcewake being
> dropped mid-sequence (due to preemption whatever).
We're also accessing the punit in valleyview_set_rps() and I've seen several
patches (in the android x86 kernel) suggesting that we need to take a wakelock
while doing this.
Regards,
Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Соберем для Вас в интернет базу данных клиентов Email: prodawez392@gmail.com Skype: prodawez390 тел +79139230330 (whatsapp\viber\telegram) Узнайте подробнее!
From: 128128stbernelm@htdiocese.org @ 2016-12-28 16:20 UTC (permalink / raw)
Соберем для Вас в интернет базу данных клиентов Email: prodawez392@gmail.com Skype: prodawez390 тел +79139230330 (whatsapp\viber\telegram) Узнайте подробнее!
^ permalink raw reply
* Re: [PATCH v2 4/5] i2c: designware-baytrail: Force the CPU to C1 state while holding the punit semaphore
From: Andy Shevchenko @ 2017-01-02 8:26 UTC (permalink / raw)
To: Hans de Goede, Len Brown, jani.nikula
Cc: Jarkko Nikula, Wolfram Sang, Mika Westerberg, Takashi Iwai,
russianneuromancer @ ya . ru, Vincent Gerris, linux-i2c,
tagore.chandan, intel-gfx
In-Reply-To: <c6933f49-6f1b-d24a-1878-8893bfe88090@redhat.com>
On Sat, 2016-12-31 at 22:29 +0100, Hans de Goede wrote:
> This makes sense, the iosf_mbi code which is used by the
> i2c bus semaphore code has this:
>
> arch/x86/platform/intel/iosf_mbi.c:
>
> int iosf_mbi_read(u8 port, u8 opcode, u32 offset, u32 *mdr)
> {
> u32 mcr, mcrx;
> unsigned long flags;
> int ret;
>
> /* Access to the GFX unit is handled by GPU code */
> if (port == BT_MBI_UNIT_GFX) {
> WARN_ON(1);
> return -EPERM;
> }
>
> ...
> }
>
> So the i915 driver definitely is interacting with the punit
> through the mailbox interface too...
Yes, they have private mailbox support. Once I talked to Ville to make
at least definitions common between two: iosf_mbi.h and whatever i915 is
using, but have no time to implement that.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v6 2/9] misc: minimal mux subsystem and gpio-based mux controller
From: Peter Rosin @ 2017-01-02 9:14 UTC (permalink / raw)
To: Jonathan Cameron, linux-kernel
Cc: Wolfram Sang, Rob Herring, Mark Rutland, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Arnd Bergmann, Greg Kroah-Hartman, linux-i2c, devicetree,
linux-iio, linux-doc
In-Reply-To: <8df0d60d-cc95-b0ce-f149-0e123af5046b@kernel.org>
On 2016-12-31 17:19, Jonathan Cameron wrote:
> On 30/11/16 08:16, Peter Rosin wrote:
>> Add a new minimalistic subsystem that handles multiplexer controllers.
>> When multiplexers are used in various places in the kernel, and the
>> same multiplexer controller can be used for several independent things,
>> there should be one place to implement support for said multiplexer
>> controller.
>>
>> A single multiplexer controller can also be used to control several
>> parallel multiplexers, that are in turn used by different subsystems
>> in the kernel, leading to a need to coordinate multiplexer accesses.
>> The multiplexer subsystem handles this coordination.
>>
>> This new mux controller subsystem initially comes with a single backend
>> driver that controls gpio based multiplexers. Even though not needed by
>> this initial driver, the mux controller subsystem is prepared to handle
>> chips with multiple (independent) mux controllers.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
> Few trivial bits inline + question of whether misc is the right location..
> It's small, but not totally trivial as subsystems go, so perhaps it needs it's
> own directory.
In [9/9] you come to the conclusion that muxes should have their own
directory, but do you think it should be
drivers/mux/*
or
drivers/misc/mux/*
?
>> ---
>> Documentation/driver-model/devres.txt | 6 +-
>> MAINTAINERS | 2 +
>> drivers/misc/Kconfig | 30 ++++
>> drivers/misc/Makefile | 2 +
>> drivers/misc/mux-core.c | 311 ++++++++++++++++++++++++++++++++++
>> drivers/misc/mux-gpio.c | 138 +++++++++++++++
>> include/linux/mux.h | 197 +++++++++++++++++++++
>> 7 files changed, 685 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/misc/mux-core.c
>> create mode 100644 drivers/misc/mux-gpio.c
>> create mode 100644 include/linux/mux.h
>>
>> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
>> index ca9d1eb46bc0..d64ede85b61b 100644
>> --- a/Documentation/driver-model/devres.txt
>> +++ b/Documentation/driver-model/devres.txt
>> @@ -330,7 +330,11 @@ MEM
>> devm_kzalloc()
>>
>> MFD
>> - devm_mfd_add_devices()
> Technically should be in a separate cleanup patch...
>> + devm_mfd_add_devices()
>> +
>> +MUX
>> + devm_mux_control_get()
>> + devm_mux_control_put()
>>
>> PER-CPU MEM
>> devm_alloc_percpu()
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 3d4d0efc2b64..dc7498682752 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -8407,6 +8407,8 @@ MULTIPLEXER SUBSYSTEM
>> M: Peter Rosin <peda@axentia.se>
>> S: Maintained
>> F: Documentation/devicetree/bindings/misc/mux-*
>> +F: include/linux/mux.h
>> +F: drivers/misc/mux-*
>>
>> MULTISOUND SOUND DRIVER
>> M: Andrew Veliath <andrewtv@usa.net>
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 64971baf11fa..2ce675e410c5 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -766,6 +766,36 @@ config PANEL_BOOT_MESSAGE
>> An empty message will only clear the display at driver init time. Any other
>> printf()-formatted message is valid with newline and escape codes.
>>
>> +menuconfig MULTIPLEXER
>> + bool "Multiplexer subsystem"
>> + help
>> + Multiplexer controller subsystem. Multiplexers are used in a
>> + variety of settings, and this subsystem abstracts their use
>> + so that the rest of the kernel sees a common interface. When
>> + multiple parallel multiplexers are controlled by one single
>> + multiplexer controller, this subsystem also coordinates the
>> + multiplexer accesses.
>> +
>> + If unsure, say no.
>> +
> Fun question of the day. Is misc the place to put this or should it
> have it's own directory. I'd go for own directory...
I thought it too small for its own dir and that it could always be
moved later. But I don't really care...
> On the plus side, whilst it's in misc you get to pester Greg and Arnd
> for review.
I know :-]
>> +if MULTIPLEXER
>> +
>> +config MUX_GPIO
>> + tristate "GPIO-controlled Multiplexer"
>> + depends on OF && GPIOLIB
>> + help
>> + GPIO-controlled Multiplexer controller.
>> +
>> + The driver builds a single multiplexer controller using a number
>> + of gpio pins. For N pins, there will be 2^N possible multiplexer
>> + states. The GPIO pins can be connected (by the hardware) to several
*snip*
>> +
>> +void mux_chip_free(struct mux_chip *mux_chip)
>> +{
>> + if (!mux_chip)
>> + return;
> I'd drop a blank line in here. Slightly nicer to read.
Right.
>> + put_device(&mux_chip->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(mux_chip_free);
*snip*
>> +
>> +static int mux_gpio_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = pdev->dev.of_node;
>> + struct mux_chip *mux_chip;
>> + struct mux_gpio *mux_gpio;
>> + int pins;
>> + u32 idle_state;
>> + int ret;
>> +
>> + if (!np)
>> + return -ENODEV;
>> +
>> + pins = gpiod_count(dev, "mux");
>> + if (pins < 0)
>> + return pins;
>> +
>> + mux_chip = mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
>> + pins * sizeof(*mux_gpio->val));
>> + if (!mux_chip)
>> + return -ENOMEM;
> Rather feels like mux_chip_alloc is a good candidate for a managed
> allocator. Might be worth doing the register as well then the remove
> below would go away. I guess perhaps not that worthwhile as not many
> mux types are likely to ever exist...
To me it seemed like too much extra support code for just two drivers.
And it can always be added later if/when more mux drivers show up...
>> +
>> + mux_gpio = mux_chip_priv(mux_chip);
>> + mux_gpio->val = (int *)(mux_gpio + 1);
*snip*
Cheers,
peda
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox