* [PATCH v3 2/5] dt: bindings: i2c-mux-pca954x: Add documentation for interrupt controller
From: Phil Reid @ 2017-01-09 9:02 UTC (permalink / raw)
To: peda, wsa, robh+dt, mark.rutland, preid, linux-i2c, devicetree
In-Reply-To: <1483952576-5308-1-git-send-email-preid@electromag.com.au>
Various muxes can aggregate multiple irq lines and provide a control
register to determine the active line. Add bindings for interrupt
controller support.
Acked-by: Peter Rosin <peda@axentia.se>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index cf53d5f..aa09704 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -19,7 +19,14 @@ Optional Properties:
- i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
children in idle state. This is necessary for example, if there are several
multiplexers on the bus and the devices behind them use same I2C addresses.
-
+ - interrupt-parent: Phandle for the interrupt controller that services
+ interrupts for this device.
+ - interrupts: Interrupt mapping for IRQ.
+ - interrupt-controller: Marks the device node as an interrupt controller.
+ - #interrupt-cells : Should be two.
+ - first cell is the pin number
+ - second cell is used to specify flags.
+ See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
Example:
@@ -29,6 +36,11 @@ Example:
#size-cells = <0>;
reg = <0x74>;
+ interrupt-parent = <&ipic>;
+ interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+
i2c@2 {
#address-cells = <1>;
#size-cells = <0>;
--
1.8.3.1
^ permalink raw reply related
* [PATCH v3 1/5] i2c: mux: pca954x: Add missing pca9542 definition to chip_desc
From: Phil Reid @ 2017-01-09 9:02 UTC (permalink / raw)
To: peda, wsa, robh+dt, mark.rutland, preid, linux-i2c, devicetree
In-Reply-To: <1483952576-5308-1-git-send-email-preid@electromag.com.au>
The spec for the pca954x was missing. This chip is the same as the pca9540
except that it has interrupt lines. While the i2c_device_id table mapped
the pca9542 to the pca9540 definition the compatible table did not. In
preparation for irq support add the pca9542 definition.
Acked-by: Peter Rosin <peda@axentia.se>
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index dd18b9c..bbf088e 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -84,6 +84,11 @@ struct pca954x {
.enable = 0x4,
.muxtype = pca954x_ismux,
},
+ [pca_9542] = {
+ .nchans = 2,
+ .enable = 0x4,
+ .muxtype = pca954x_ismux,
+ },
[pca_9543] = {
.nchans = 2,
.muxtype = pca954x_isswi,
@@ -110,7 +115,7 @@ struct pca954x {
static const struct i2c_device_id pca954x_id[] = {
{ "pca9540", pca_9540 },
- { "pca9542", pca_9540 },
+ { "pca9542", pca_9542 },
{ "pca9543", pca_9543 },
{ "pca9544", pca_9544 },
{ "pca9545", pca_9545 },
@@ -124,7 +129,7 @@ struct pca954x {
#ifdef CONFIG_ACPI
static const struct acpi_device_id pca954x_acpi_ids[] = {
{ .id = "PCA9540", .driver_data = pca_9540 },
- { .id = "PCA9542", .driver_data = pca_9540 },
+ { .id = "PCA9542", .driver_data = pca_9542 },
{ .id = "PCA9543", .driver_data = pca_9543 },
{ .id = "PCA9544", .driver_data = pca_9544 },
{ .id = "PCA9545", .driver_data = pca_9545 },
--
1.8.3.1
^ permalink raw reply related
* [PATCH v3 5/5] i2c: mux: pca954x: Add irq_mask_en to delay enabling irqs
From: Phil Reid @ 2017-01-09 9:02 UTC (permalink / raw)
To: peda-koto5C5qi+TLoDKTGw+V6w, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1483952576-5308-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
Unfortunately some hardware device will assert their irq line immediately
on power on and provide no mechanism to mask the irq. As the i2c muxes
provide no method to mask irq line this provides a work around by keeping
the parent irq masked until enough device drivers have loaded to service
all pending interrupts.
For example the the ltc1760 assert its SMBALERT irq immediately on power
on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
device is registered irq are enabled and fire continuously as the second
device driver has not yet loaded. Setting this parameter to 0x3 while
delay the irq being enabled until both devices are ready.
Acked-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
drivers/i2c/muxes/i2c-mux-pca954x.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 84fc767..fb1d245 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -75,6 +75,19 @@ struct chip_desc {
} muxtype;
};
+/*
+ * irq_mask_enable: Provides a mechanism to work around hardware that asserts
+ * their irq immediately on power on. It allows the enabling of the irq to be
+ * delayed until the corresponding bits in the the irq_mask are set thru
+ * irq_unmask.
+ * For example the ltc1760 assert its SMBALERT irq immediately on power on.
+ * With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
+ * device is registered irq are enabled and fire continuously as the second
+ * device driver has not yet loaded. Setting this parameter to 0x3 while
+ * delay the irq being enabled until both devices are ready.
+ * This workaround will not work if two devices share an interrupt on the
+ * same bus segment.
+ */
struct pca954x {
const struct chip_desc *chip;
@@ -84,6 +97,7 @@ struct pca954x {
struct irq_domain *irq;
unsigned int irq_mask;
+ unsigned int irq_mask_enable;
};
/* Provide specs for the PCA954x types we know about */
@@ -270,9 +284,12 @@ static void pca954x_irq_unmask(struct irq_data *idata)
struct pca954x *data = irq_data_get_irq_chip_data(idata);
unsigned int pos = idata->hwirq;
- if (!data->irq_mask)
+ if (!data->irq_mask_enable && !data->irq_mask)
enable_irq(data->client->irq);
data->irq_mask |= BIT(pos);
+ if (data->irq_mask_enable &&
+ (data->irq_mask & data->irq_mask) == data->irq_mask_enable)
+ enable_irq(data->client->irq);
}
static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
@@ -395,6 +412,9 @@ static int pca954x_probe(struct i2c_client *client,
idle_disconnect_dt = of_node &&
of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
+ of_property_read_u32(of_node, "nxp,irq-mask-enable",
+ &data->irq_mask_enable);
+
ret = pca954x_irq_setup(muxc);
if (ret)
goto fail_del_adapters;
--
1.8.3.1
--
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 related
* [PATCH v3 4/5] dt: bindings: i2c-mux-pca954x: Add documentation for i2c-mux-irq-mask-en
From: Phil Reid @ 2017-01-09 9:02 UTC (permalink / raw)
To: peda-koto5C5qi+TLoDKTGw+V6w, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1483952576-5308-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
Unfortunately some hardware device will assert their irq line immediately
on power on and provide no mechanism to mask the irq. As the i2c muxes
provide no method to mask irq line this provides a work around by keeping
the parent irq masked until enough device drivers have loaded to service
all pending interrupts.
For example the the ltc1760 assert its SMBALERT irq immediately on power
on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
device is registered irq are enabled and fire continuously as the second
device driver has not yet loaded. Setting this parameter to 0x3 while
delay the irq being enabled until both devices are ready.
Acked-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index aa09704..6de1e8e 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -19,6 +19,8 @@ Optional Properties:
- i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
children in idle state. This is necessary for example, if there are several
multiplexers on the bus and the devices behind them use same I2C addresses.
+ - nxp,irq-mask-enable: BitMask; Defines a mask for which irq lines need to be
+ unmasked before the parent irq line in enabled.
- interrupt-parent: Phandle for the interrupt controller that services
interrupts for this device.
- interrupts: Interrupt mapping for IRQ.
@@ -36,6 +38,7 @@ Example:
#size-cells = <0>;
reg = <0x74>;
+ nxp,irq-mask-enable = <0x3>;
interrupt-parent = <&ipic>;
interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
interrupt-controller;
--
1.8.3.1
--
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 related
* [PATCH v3 0/5] i2c: mux: pca954x: Add interrupt controller support
From: Phil Reid @ 2017-01-09 9:02 UTC (permalink / raw)
To: peda-koto5C5qi+TLoDKTGw+V6w, wsa-z923LK4zBo2bacvFa/9K2g,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Various muxes can aggregate multiple interrupts from each i2c bus.
All of the muxes with interrupt support combine the active low irq lines
using an internal 'and' function and generate a combined active low
output. The muxes do provide the ability to read a control register to
determine which irq is active. By making the mux an irq controller isr
latenct can potentially be reduced by reading the status register and
then only calling the registered isr on that bus segment.
In addition an additional enable mask is added to work around devices
that assert irq immediately before being setup buy disabling the irq
from the mux until all devices are registered.
Changes from v2:
- p1: Added Acked-by
- p5: fixup 2 typos
Changes from v1:
- Update for new ACPI table
- Fix typo in documentation
- Fix typo in function names
- Fix typo in irq name
- Added spaces around '+' / '='
- Change goto label names
- Change property name from i2c-mux-irq-mask-en to nxp,irq-mask-enable
- Change variable name irq_mask_en to irq_mask_enable
- Add commentt about irq_mask_enable
- Added Acked-By's
Phil Reid (5):
i2c: mux: pca954x: Add missing pca9542 definition to chip_desc
dt: bindings: i2c-mux-pca954x: Add documentation for interrupt
controller
i2c: mux: pca954x: Add interrupt controller support
dt: bindings: i2c-mux-pca954x: Add documentation for
i2c-mux-irq-mask-en
i2c: mux: pca954x: Add irq_mask_en to delay enabling irqs
.../devicetree/bindings/i2c/i2c-mux-pca954x.txt | 17 ++-
drivers/i2c/muxes/i2c-mux-pca954x.c | 156 ++++++++++++++++++++-
2 files changed, 168 insertions(+), 5 deletions(-)
--
1.8.3.1
--
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
* Re: [PATCH v2 1/5] i2c: mux: pca954x: Add missing pca9542 definition to chip_desc
From: Peter Rosin @ 2017-01-09 7:55 UTC (permalink / raw)
To: Phil Reid, wsa, robh+dt, mark.rutland, linux-i2c, devicetree
In-Reply-To: <1483589463-35380-2-git-send-email-preid@electromag.com.au>
On 2017-01-05 05:10, Phil Reid wrote:
> The spec for the pca954x was missing. This chip is the same as the pca9540
> except that it has interrupt lines. While the i2c_device_id table mapped
> the pca9542 to the pca9540 definition the compatible table did not. In
> preparation for irq support add the pca9542 definition.
>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
Acked-by: Peter Rosin <peda@axentia.se>
Cheers,
peda
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index dd18b9c..bbf088e 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -84,6 +84,11 @@ struct pca954x {
> .enable = 0x4,
> .muxtype = pca954x_ismux,
> },
> + [pca_9542] = {
> + .nchans = 2,
> + .enable = 0x4,
> + .muxtype = pca954x_ismux,
> + },
> [pca_9543] = {
> .nchans = 2,
> .muxtype = pca954x_isswi,
> @@ -110,7 +115,7 @@ struct pca954x {
>
> static const struct i2c_device_id pca954x_id[] = {
> { "pca9540", pca_9540 },
> - { "pca9542", pca_9540 },
> + { "pca9542", pca_9542 },
> { "pca9543", pca_9543 },
> { "pca9544", pca_9544 },
> { "pca9545", pca_9545 },
> @@ -124,7 +129,7 @@ struct pca954x {
> #ifdef CONFIG_ACPI
> static const struct acpi_device_id pca954x_acpi_ids[] = {
> { .id = "PCA9540", .driver_data = pca_9540 },
> - { .id = "PCA9542", .driver_data = pca_9540 },
> + { .id = "PCA9542", .driver_data = pca_9542 },
> { .id = "PCA9543", .driver_data = pca_9543 },
> { .id = "PCA9544", .driver_data = pca_9544 },
> { .id = "PCA9545", .driver_data = pca_9545 },
>
^ permalink raw reply
* Re: [PATCH v2 5/5] i2c: mux: pca954x: Add irq_mask_en to delay enabling irqs
From: Peter Rosin @ 2017-01-09 7:54 UTC (permalink / raw)
To: Phil Reid, wsa, robh+dt, mark.rutland, linux-i2c, devicetree
In-Reply-To: <1483589463-35380-6-git-send-email-preid@electromag.com.au>
On 2017-01-05 05:11, Phil Reid wrote:
> Unfortunately some hardware device will assert their irq line immediately
> on power on and provide no mechanism to mask the irq. As the i2c muxes
> provide no method to mask irq line this provides a work around by keeping
> the parent irq masked until enough device drivers have loaded to service
> all pending interrupts.
>
> For example the the ltc1760 assert its SMBALERT irq immediately on power
> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
> device is registered irq are enabled and fire continuously as the second
> device driver has not yet loaded. Setting this parameter to 0x3 while
> delay the irq being enabled until both devices are ready.
>
> Acked-by: Peter Rosin <peda@axentia.se>
Ooops, too soon apparently, but the below nitpicks are not that important.
Maybe Wolfram can fix it up instead of you sending a new version?
Cheers,
peda
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
> drivers/i2c/muxes/i2c-mux-pca954x.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 84fc767..581a75d 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -75,6 +75,19 @@ struct chip_desc {
> } muxtype;
> };
>
> +/*
> + * irq_mask_enable: Provides a mechanism to work around hardware that asserts
> + * their irq immediately on power on. It allows the enabling of the irq to be
double space: "the irq"
> + * delayed until the corresponding bits in the the irq_mask are set thru
> + * irq_unmask.
> + * For example the the ltc1760 assert its SMBALERT irq immediately on power
"the the"
> + * on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
> + * device is registered irq are enabled and fire continuously as the second
> + * device driver has not yet loaded. Setting this parameter to 0x3 while
> + * delay the irq being enabled until both devices are ready.
> + * This workaround will not work if two devices share an interrupt on the
> + * same bus segment.
> + */
> struct pca954x {
> const struct chip_desc *chip;
>
> @@ -84,6 +97,7 @@ struct pca954x {
>
> struct irq_domain *irq;
> unsigned int irq_mask;
> + unsigned int irq_mask_enable;
> };
>
> /* Provide specs for the PCA954x types we know about */
> @@ -270,9 +284,12 @@ static void pca954x_irq_unmask(struct irq_data *idata)
> struct pca954x *data = irq_data_get_irq_chip_data(idata);
> unsigned int pos = idata->hwirq;
>
> - if (!data->irq_mask)
> + if (!data->irq_mask_enable && !data->irq_mask)
> enable_irq(data->client->irq);
> data->irq_mask |= BIT(pos);
> + if (data->irq_mask_enable &&
> + (data->irq_mask & data->irq_mask) == data->irq_mask_enable)
> + enable_irq(data->client->irq);
> }
>
> static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
> @@ -395,6 +412,9 @@ static int pca954x_probe(struct i2c_client *client,
> idle_disconnect_dt = of_node &&
> of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
>
> + of_property_read_u32(of_node, "nxp,irq-mask-enable",
> + &data->irq_mask_enable);
> +
> ret = pca954x_irq_setup(muxc);
> if (ret)
> goto fail_del_adapters;
>
^ permalink raw reply
* Re: [PATCH linux 2/6] hwmon: occ: Add sysfs interface
From: Andrew Jeffery @ 2017-01-08 23:52 UTC (permalink / raw)
To: Guenter Roeck, Edward James
Cc: corbet, devicetree, eajames.ibm, jdelvare, joel, linux-doc,
linux-hwmon, linux-i2c, linux-kernel, mark.rutland, robh+dt, wsa
In-Reply-To: <8b182766-32a0-9eb1-7917-14abf811cef5@roeck-us.net>
[-- Attachment #1: Type: text/plain, Size: 4017 bytes --]
On Sat, 2017-01-07 at 09:15 -0800, Guenter Roeck wrote:
> On 01/06/2017 02:17 PM, Edward James wrote:
>
> [ ... ]
>
> > > > +}
> > > > +
> > > > +static DEVICE_ATTR(online, S_IWUSR | S_IRUGO, show_occ_online,
> > > > + store_occ_online);
> > > > +
> > > > +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> > > > + struct occ_sysfs_config *config)
> > > > +{
> > > > + struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(struct occ_sysfs),
> > > > + GFP_KERNEL);
> > > > + int rc;
> > > > +
> > > > + if (!hwmon)
> > > > + return ERR_PTR(-ENOMEM);
> > > > +
> > > > + hwmon->occ = occ;
> > > > + hwmon->num_caps_fields = config->num_caps_fields;
> > > > + hwmon->caps_names = config->caps_names;
> > > > +
> > > > + dev_set_drvdata(dev, hwmon);
> > > > +
> > > > + rc = device_create_file(dev, &dev_attr_online);
> > > > + if (rc)
> > > > + return ERR_PTR(rc);
> > > > +
> > > > + return hwmon;
> > > > +}
> > > > +EXPORT_SYMBOL(occ_sysfs_start);
> > > > +
> > > > +int occ_sysfs_stop(struct device *dev, struct occ_sysfs *driver)
> > > > +{
> > > > + if (driver->dev) {
> > > > + occ_remove_hwmon_attrs(driver);
> > > > + hwmon_device_unregister(driver->dev);
> > > > + }
> > > > +
> > > > + device_remove_file(driver->dev, &dev_attr_online);
> > > > +
> > > > + devm_kfree(dev, driver);
> > >
> > > Thw point of using devm_ functions is not to require remove/free functions.
> > > Something is completely wrong here if you need that call.
> > >
> > > Overall, this is architectually completely wrong. One does not register
> > > or instantiate drivers based on writing into sysfs attributes. Please
> > > reconsider your approach.
> >
> > We had some trouble designing this driver because the BMC only has
> > access to the OCC once the processor is powered on. This will happen
> > sometime after the BMC boots (this driver runs only on the BMC). With
> > no access to the OCC, we don't know what sensors are present on the
> > system without a large static enumeration. Also any sysfs files created
> > before we have OCC access won't be able to return any data.
> >
> > Instead of the "online" attribute, what do you think about using the
> > "bind"/"unbind" API to probe the device from user space once the system
> > is powered on? All the hwmon registration would take place in the probe
> > function, it would just occur some time after boot.
> >
>
> A more common approach would be to have a platform driver. That platform
> driver would need a means to detect if the OCC is up and running, and
> instantiate everything else once it is.
>
> A trigger from user space is problematic because there is no guarantee
> that the OCC is really up (or that it even exists).
This is true in general, but for the BMC case we have more information:
The host CPU power supply is controlled by several GPIOs from
userspace. Once we receive the "power-good" signal for the host CPU we
can bind the OCC driver and trigger the probe.
Alternatively, in the style of your first para, we could push the host
CPU state management into the kernel and expose a boot/reboot/power-off
API to userspace. That would give us a place to hook calls for
configuring and cleaning up any host-dependent drivers on the BMC.
The solution to the host-power-state problem is also applicable to the
OpenFSI patches that were recently sent out:
https://lkml.org/lkml/2016/12/6/732
The OpenFSI infra needs to re-scan for CFAMs when the host is powered
up.
>
> An alternative might be to have the hwmon driver poll for the OCC,
> but that would be a bit more difficult and might require a kernel thread
> or maybe asynchronous probing.
This was our thought as a fallback solution.
Andrew
>
> Guenter
>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v7 00/12] mux controller abstraction and iio/i2c muxes
From: Peter Rosin @ 2017-01-08 21:56 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-kernel, 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-doc
In-Reply-To: <20170108105129.GB1983@katana>
On 2017-01-08 11:51, Wolfram Sang wrote:
> Hi peda,
>
>> 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.
>
> In a few lines, what is preventing that?
i2c-mux-gpio has the auto-detect for mux-/parent-locked and (old
style?) support for specifying what gpios to use with platform data
from code (i.e. not from dt/acpi). All of that gets messier if
someone else (the mux) owns the gpios.
i2c-mux-pinctrl has similar issues.
i2c-mux-reg has the platform data from code issue, plus a read-write
mode (mux is write only, at least as-is).
i2c-mux-pca954x has things going on like irqs (not applied yet) and
a reset gpio that makes it a poor candidate for something generic.
i2c-mux-mlxcplc could probably be converted (I don't think it existed
when I wrote the quoted paragraph).
So, since there are issues with just about all of the i2c muxes, the
only way forward that I see would be to instantiate the mux locally
and feed it to the generic i2c-mux-simple, but what would be the point
of that? There would still be a handful of i2c-mux drivers under
drivers/i2c/muxes.
Maybe some of the above are non-issues and maybe I have failed to see
some issue? I didn't think too hard about it...
>> 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.
>
> Maybe ask this question in a seperate email thread on lkml cc-ing the
> locking gurus (with a pointer to this thread)?
I don't think there is a suitable primitive. In order to get
something that really matches, I think the users need to hint
how long they (think they) are going to lock the mux. And
then it no longer a primitive, methinks. It something that is
much heavier...
>> 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...
>
> "... and it probably never will support anything other than
> AT-harddisks, as that's all I have..." ;))
:-)
> Thanks for this work!
And thanks for looking!
Cheers,
peda
^ permalink raw reply
* Re: [PATCH v7 05/12] mux: support simplified bindings for single-user gpio mux
From: Peter Rosin @ 2017-01-08 21:56 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: <9652928a-5d54-5fa4-48ad-6df57bc8513c@kernel.org>
On 2017-01-08 11:28, Jonathan Cameron wrote:
> On 05/01/17 16:21, Peter Rosin wrote:
>> On 2017-01-04 13:16, Peter Rosin wrote:
>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>> ---
>>> drivers/mux/mux-core.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++--
>>> drivers/mux/mux-gpio.c | 56 ++--------------------------------
>>> include/linux/mux.h | 7 +++++
>>> 3 files changed, 89 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
>>> index 21da15a264ad..d887ae1c0e55 100644
>>> --- a/drivers/mux/mux-core.c
>>> +++ b/drivers/mux/mux-core.c
>>> @@ -15,6 +15,7 @@
>>> #include <linux/device.h>
>>> #include <linux/err.h>
>>> #include <linux/idr.h>
>>> +#include <linux/gpio/consumer.h>
>>> #include <linux/module.h>
>>> #include <linux/mux.h>
>>> #include <linux/of.h>
>>> @@ -288,6 +289,63 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
>>> return dev ? to_mux_chip(dev) : NULL;
>>> }
>>>
>>> +#ifdef CONFIG_MUX_GPIO
>>> +
>>> +static int mux_gpio_set(struct mux_control *mux, int state)
>>> +{
>>> + struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
>>> + int i;
>>> +
>>> + for (i = 0; i < mux_gpio->gpios->ndescs; i++)
>>> + mux_gpio->val[i] = (state >> i) & 1;
>>> +
>>> + gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
>>> + mux_gpio->gpios->desc,
>>> + mux_gpio->val);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct mux_control_ops mux_gpio_ops = {
>>> + .set = mux_gpio_set,
>>> +};
>>> +
>>> +struct mux_chip *mux_gpio_alloc(struct device *dev)
>>> +{
>>> + struct mux_chip *mux_chip;
>>> + struct mux_gpio *mux_gpio;
>>> + int pins;
>>> + int ret;
>>> +
>>> + pins = gpiod_count(dev, "mux");
>>> + if (pins < 0)
>>> + return ERR_PTR(pins);
>>> +
>>> + mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
>>> + pins * sizeof(*mux_gpio->val));
>>> + if (!mux_chip)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + mux_gpio = mux_chip_priv(mux_chip);
>>> + mux_gpio->val = (int *)(mux_gpio + 1);
>>> + mux_chip->ops = &mux_gpio_ops;
>>> +
>>> + mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
>>> + if (IS_ERR(mux_gpio->gpios)) {
>>> + ret = PTR_ERR(mux_gpio->gpios);
>>> + if (ret != -EPROBE_DEFER)
>>> + dev_err(dev, "failed to get gpios\n");
>>> + return ERR_PTR(ret);
>>> + }
>>> + WARN_ON(pins != mux_gpio->gpios->ndescs);
>>> + mux_chip->mux->states = 1 << pins;
>>> +
>>> + return mux_chip;
>>> +}
>>> +EXPORT_SYMBOL_GPL(mux_gpio_alloc);
>>> +
>>> +#endif /* CONFIG_MUX_GPIO */
>>> +
>>> struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>> {
>>> struct device_node *np = dev->of_node;
>>> @@ -307,9 +365,28 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
>>> ret = of_parse_phandle_with_args(np,
>>> "mux-controls", "#mux-control-cells",
>>> index, &args);
>>> +
>>> +#ifdef CONFIG_MUX_GPIO
>>> + if (ret == -ENOENT && !mux_name && gpiod_count(dev, "mux") > 0) {
>>> + mux_chip = mux_gpio_alloc(dev);
>>> + if (!IS_ERR(mux_chip)) {
>>> + ret = devm_mux_chip_register(dev, mux_chip);
>>> + if (ret < 0) {
>>> + dev_err(dev, "failed to register mux-chip\n");
>>> + return ERR_PTR(ret);
>>> + }
>>> + get_device(&mux_chip->dev);
>>> + return mux_chip->mux;
>>> + }
>>> +
>>> + ret = PTR_ERR(mux_chip);
>>> + }
>>> +#endif
>>> +
>>> if (ret) {
>>> - dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
>>> - np->full_name, mux_name ?: "", index);
>>> + if (ret != -EPROBE_DEFER)
>>> + dev_err(dev, "%s: failed to get mux-control %s(%i)\n",
>>> + np->full_name, mux_name ?: "", index);
>>> return ERR_PTR(ret);
>>> }
>>>
>>> diff --git a/drivers/mux/mux-gpio.c b/drivers/mux/mux-gpio.c
>>> index 76b52bc63470..8a7bfbc0c4bb 100644
>>> --- a/drivers/mux/mux-gpio.c
>>> +++ b/drivers/mux/mux-gpio.c
>>> @@ -11,37 +11,12 @@
>>> */
>>>
>>> #include <linux/err.h>
>>> -#include <linux/gpio/consumer.h>
>>> #include <linux/module.h>
>>> #include <linux/mux.h>
>>> #include <linux/of_platform.h>
>>> #include <linux/platform_device.h>
>>> #include <linux/property.h>
>>
>> Instead of moving the mux-gpio guts from mux-gpio.c to mux-core.c, I
>> will instead make CONFIG_MUX_GPIO a bool option (no module possible)
>> and call it from the mux-core. That will be cleaner and less of a
>> break of abstractions in my opinion.
> Hmm. I wonder if the balance is right here or whether we should just not have the
> simplified binding at all as it breaks the assumption that all muxes are of the
> same level...
>
> I like the binding, but it is causing significant complexity in here.
Yes, in this patch it looks pretty bad. But with the suggested
changes it at least looks fine (apart from the remaining ifdef
in mux_control_get). But then there's of course the conceptual
badness of the core depending on a specific driver that you
mention...
But. I expect that gpio based muxes will be in vast majority, and
giving them some extra freedoms is probably not wrong considering
the upside of the simpler binding.
>>
>> Cheers,
>> Peter
>>
>>> -struct mux_gpio {
>>> - struct gpio_descs *gpios;
>>> - int *val;
>>> -};
>>> -
>>> -static int mux_gpio_set(struct mux_control *mux, int state)
>>> -{
>>> - struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
>>> - int i;
>>> -
>>> - for (i = 0; i < mux_gpio->gpios->ndescs; i++)
>>> - mux_gpio->val[i] = (state >> i) & 1;
>>> -
>>> - gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
>>> - mux_gpio->gpios->desc,
>>> - mux_gpio->val);
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -static const struct mux_control_ops mux_gpio_ops = {
>>> - .set = mux_gpio_set,
>>> -};
>>> -
>>> static const struct of_device_id mux_gpio_dt_ids[] = {
>>> { .compatible = "mux-gpio", },
>>> { /* sentinel */ }
>>> @@ -51,38 +26,13 @@ MODULE_DEVICE_TABLE(of, mux_gpio_dt_ids);
>>> static int mux_gpio_probe(struct platform_device *pdev)
>>> {
>>> struct device *dev = &pdev->dev;
>>> - struct device_node *np = 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 = devm_mux_chip_alloc(dev, 1, sizeof(*mux_gpio) +
>>> - pins * sizeof(*mux_gpio->val));
>>> - if (!mux_chip)
>>> - return -ENOMEM;
>>> -
>>> - mux_gpio = mux_chip_priv(mux_chip);
>>> - mux_gpio->val = (int *)(mux_gpio + 1);
>>> - mux_chip->ops = &mux_gpio_ops;
>>> -
>>> - mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
>>> - if (IS_ERR(mux_gpio->gpios)) {
>>> - ret = PTR_ERR(mux_gpio->gpios);
>>> - if (ret != -EPROBE_DEFER)
>>> - dev_err(dev, "failed to get gpios\n");
>>> - return ret;
>>> - }
>>> - WARN_ON(pins != mux_gpio->gpios->ndescs);
>>> - mux_chip->mux->states = 1 << pins;
>>> + mux_chip = mux_gpio_alloc(dev);
>>> + if (IS_ERR(mux_chip))
>>> + return PTR_ERR(mux_chip);
>>>
>>> ret = device_property_read_u32(dev, "idle-state", &idle_state);
>>> if (ret >= 0) {
>>> diff --git a/include/linux/mux.h b/include/linux/mux.h
>>> index 3b9439927f11..3bfee23cfb8c 100644
>>> --- a/include/linux/mux.h
>>> +++ b/include/linux/mux.h
>>> @@ -241,4 +241,11 @@ struct mux_control *devm_mux_control_get(struct device *dev,
>>> */
>>> void devm_mux_control_put(struct device *dev, struct mux_control *mux);
>>>
>>> +struct mux_gpio {
>>> + struct gpio_descs *gpios;
>>> + int *val;
>>> +};
>>> +
>>> +struct mux_chip *mux_gpio_alloc(struct device *dev);
>>> +
>>> #endif /* _LINUX_MUX_H */
>>>
>>
>
^ permalink raw reply
* Re: [PATCH v7 03/12] mux: minimal mux subsystem and gpio-based mux controller
From: Peter Rosin @ 2017-01-08 21:55 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: <9e4ee54f-f781-8d3b-1c46-1549392a9f2f@kernel.org>
On 2017-01-08 11:23, Jonathan Cameron wrote:
> On 04/01/17 12: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>
> Reviewed-by: Jonathan Cameron <jic23@kernel.org>
>> ---
>> Documentation/driver-model/devres.txt | 8 +
>> MAINTAINERS | 2 +
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 1 +
>> drivers/mux/Kconfig | 33 +++
>> drivers/mux/Makefile | 6 +
>> drivers/mux/mux-core.c | 398 ++++++++++++++++++++++++++++++++++
>> drivers/mux/mux-gpio.c | 120 ++++++++++
>> include/linux/mux.h | 244 +++++++++++++++++++++
>> 9 files changed, 814 insertions(+)
>> create mode 100644 drivers/mux/Kconfig
>> create mode 100644 drivers/mux/Makefile
>> create mode 100644 drivers/mux/mux-core.c
>> create mode 100644 drivers/mux/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 dc51fb024190..1e9ae701a587 100644
>> --- a/Documentation/driver-model/devres.txt
>> +++ b/Documentation/driver-model/devres.txt
>> @@ -332,6 +332,14 @@ MEM
>> MFD
>> devm_mfd_add_devices()
>>
>> +MUX
>> + devm_mux_chip_alloc()
>> + devm_mux_chip_free()
>> + devm_mux_chip_register()
>> + devm_mux_chip_unregister()
>> + devm_mux_control_get()
>> + devm_mux_control_put()
>> +
>> PER-CPU MEM
>> devm_alloc_percpu()
>> devm_free_percpu()
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 32abef2b6d05..ebe96f3e25a0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -8442,6 +8442,8 @@ MULTIPLEXER SUBSYSTEM
>> M: Peter Rosin <peda@axentia.se>
>> S: Maintained
>> F: Documentation/devicetree/bindings/mux/
>> +F: include/linux/mux.h
>> +F: drivers/mux/
>>
>> MULTISOUND SOUND DRIVER
>> M: Andrew Veliath <andrewtv@usa.net>
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index e1e2066cecdb..993aeb65affa 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -76,6 +76,8 @@ source "drivers/hwmon/Kconfig"
>>
>> source "drivers/thermal/Kconfig"
>>
>> +source "drivers/mux/Kconfig"
>> +
>
> Why this location in the list? I think the convention for new subystems is
> to just go last in the list.
Oh. I'll move it to the end...
Thanks for all your acks/reviews!
Cheers,
peda
^ permalink raw reply
* Re: [PATCH 7/7] drm/i915: Take punit lock when modifying punit settings
From: Hans de Goede @ 2017-01-08 15:42 UTC (permalink / raw)
To: Andy Shevchenko, Daniel Vetter, Jani Nikula,
Ville Syrjälä, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: intel-gfx, dri-devel, Mika Westerberg, Takashi Iwai,
russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <1483889675.26691.18.camel@linux.intel.com>
Hi,
On 08-01-17 16:34, Andy Shevchenko wrote:
> On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
>> 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.
>>
>
> But should not i915 drm eventually share the same iosf_mbi driver?
> Currently what you are doing you create notifier and all that not-best-
> ever stuff due to having two accessors to IOSF MB. If we have only one
> driver which i915 will not ignore you don't need to create such things.
>
> That's what I was trying to imply when commenting one of the patch in
> previous series.
Ah, yes that as an interesting point. So what do the i915 devs think
of changing the i915 code to use the iosf_mbi functions for at least
punit accesses, instead of doing those through the i915 pci config space?
Note that we will still need to have some higher level locking, or
a new iosf_mbi function which does this under a lock, for code paths
where the i915 code does a write followed by multiple reads to wait
for the punit to have changed the value to the requested value.
Regards,
Hans
>
>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 6 ++++++
>> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++++
>> drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
>> 3 files changed, 24 insertions(+)
>>
>> 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 4b12637..0d55b61 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);
>> }
>>
>> @@ -4981,7 +4988,9 @@ static void valleyview_set_rps(struct
>> drm_i915_private *dev_priv, u8 val)
>> I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>>
>> if (val != dev_priv->rps.cur_freq) {
>> + iosf_mbi_punit_lock();
>> vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
>> val);
>> + iosf_mbi_punit_unlock();
>> if (!IS_CHERRYVIEW(dev_priv))
>> gen6_set_rps_thresholds(dev_priv, val);
>> }
>> 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:
>
^ permalink raw reply
* Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire
From: Hans de Goede @ 2017-01-08 15:39 UTC (permalink / raw)
To: Andy Shevchenko, Daniel Vetter, Jani Nikula,
Ville Syrjälä, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: intel-gfx, dri-devel, Mika Westerberg, Takashi Iwai,
russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <1483889239.26691.16.camel@linux.intel.com>
Hi,
On 08-01-17 16:27, Andy Shevchenko wrote:
> On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
>> 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.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.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..cf7a2a0 100644
>> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
>> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
>>
>
>> @@ -62,6 +62,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
>
>> + iosf_mbi_punit_unlock();
>
>> @@ -79,6 +81,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev
>> + iosf_mbi_punit_lock();
>
> For me it looks a bit asymmetrical.
>
> Can we use acquire()/release()?
Sure I can change things to use acquire()/release() instead. I will
change this for v2.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 1/7] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Andy Shevchenko @ 2017-01-08 15:35 UTC (permalink / raw)
To: Hans de Goede, Daniel Vetter, Jani Nikula,
Ville Syrjälä, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: intel-gfx, dri-devel, Mika Westerberg, Takashi Iwai,
russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <d6acfed3-f778-fe6a-eda7-3de5e2ce9bb9@redhat.com>
On Sun, 2017-01-08 at 16:30 +0100, Hans de Goede wrote:
> Hi,
>
> On 08-01-17 16:16, Andy Shevchenko wrote:
> > On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> > > One some systems the punit accesses the pmic to change various
> > > voltages
> > > through the same bus as other kernel drivers use for e.g. battery
> > > monitoring.
> > >
> > > If a driver sends requests to the punit which require the punit to
> > > access
> > > the pmic bus while another driver is also accessing the pmic bus
> > > various
> > > bad things happen.
> > >
> > > This commit adds a mutex to protect the punit against simultaneous
> > > accesses
> > > and 2 functions to lock / unlock this mutex.
> > >
> > > Note on these systems the i2c-bus driver will request a sempahore
> > > from
> > > the
> > > punit for exclusive access to the pmic bus when i2c drivers are
> > > accessing
> > > it, but this does not appear to be sufficient, we still need to
> > > avoid
> > > making certain punit requests during the access window to avoid
> > > problems.
> >
> > I'm fine with the patch, but please spell
> > P-Unit
> > PMIC
>
> In the commit msg and comments, not in code you mean I assume ?
Correct.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH 7/7] drm/i915: Take punit lock when modifying punit settings
From: Andy Shevchenko @ 2017-01-08 15:34 UTC (permalink / raw)
To: Hans de Goede, Daniel Vetter, Jani Nikula,
Ville Syrjälä, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: Takashi Iwai, russianneuromancer @ ya . ru, intel-gfx, dri-devel,
linux-i2c, Mika Westerberg
In-Reply-To: <20170108134427.8392-8-hdegoede@redhat.com>
On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> 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.
>
But should not i915 drm eventually share the same iosf_mbi driver?
Currently what you are doing you create notifier and all that not-best-
ever stuff due to having two accessors to IOSF MB. If we have only one
driver which i915 will not ignore you don't need to create such things.
That's what I was trying to imply when commenting one of the patch in
previous series.
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 6 ++++++
> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++++
> drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
> 3 files changed, 24 insertions(+)
>
> 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 4b12637..0d55b61 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);
> }
>
> @@ -4981,7 +4988,9 @@ static void valleyview_set_rps(struct
> drm_i915_private *dev_priv, u8 val)
> I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>
> if (val != dev_priv->rps.cur_freq) {
> + iosf_mbi_punit_lock();
> vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> val);
> + iosf_mbi_punit_unlock();
> if (!IS_CHERRYVIEW(dev_priv))
> gen6_set_rps_thresholds(dev_priv, val);
> }
> 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:
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH 1/7] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Hans de Goede @ 2017-01-08 15:30 UTC (permalink / raw)
To: Andy Shevchenko, Daniel Vetter, Jani Nikula,
Ville Syrjälä, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: intel-gfx, dri-devel, Mika Westerberg, Takashi Iwai,
russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <1483888594.26691.8.camel@linux.intel.com>
Hi,
On 08-01-17 16:16, Andy Shevchenko wrote:
> On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
>> One some systems the punit accesses the pmic to change various
>> voltages
>> through the same bus as other kernel drivers use for e.g. battery
>> monitoring.
>>
>> If a driver sends requests to the punit which require the punit to
>> access
>> the pmic bus while another driver is also accessing the pmic bus
>> various
>> bad things happen.
>>
>> This commit adds a mutex to protect the punit against simultaneous
>> accesses
>> and 2 functions to lock / unlock this mutex.
>>
>> Note on these systems the i2c-bus driver will request a sempahore from
>> the
>> punit for exclusive access to the pmic bus when i2c drivers are
>> accessing
>> it, but this does not appear to be sufficient, we still need to avoid
>> making certain punit requests during the access window to avoid
>> problems.
>
> I'm fine with the patch, but please spell
> P-Unit
> PMIC
In the commit msg and comments, not in code you mean I assume ?
Regards,
Hans
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>> ---
>> arch/x86/include/asm/iosf_mbi.h | 31
>> +++++++++++++++++++++++++++++++
>> arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++
>> 2 files changed, 44 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/iosf_mbi.h
>> b/arch/x86/include/asm/iosf_mbi.h
>> index b41ee16..91f5d16 100644
>> --- a/arch/x86/include/asm/iosf_mbi.h
>> +++ b/arch/x86/include/asm/iosf_mbi.h
>> @@ -88,6 +88,33 @@ 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
>> + *
>> + * One some systems the punit accesses the pmic to change various
>> voltages
>> + * through the same bus as other kernel drivers use for e.g. battery
>> monitoring.
>> + *
>> + * If a driver sends requests to the punit which require the punit to
>> access the
>> + * pmic bus while another driver is also accessing the pmic bus
>> various bad
>> + * things happen.
>> + *
>> + * To avoid these problems this function must be called before
>> accessing the
>> + * punit or the pmic, be it through iosf_mbi* functions or through
>> other means.
>> + *
>> + * Note on these systems the i2c-bus driver will request a sempahore
>> from the
>> + * punit for exclusive access to the pmic bus when i2c drivers are
>> accessing it,
>> + * but this does not appear to be sufficient, we still need to avoid
>> making
>> + * certain punit requests during the access window to avoid problems.
>> + *
>> + * 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 +142,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;
>
^ permalink raw reply
* Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire
From: Andy Shevchenko @ 2017-01-08 15:27 UTC (permalink / raw)
To: Hans de Goede, Daniel Vetter, Jani Nikula,
Ville Syrjälä, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: intel-gfx, dri-devel, Mika Westerberg, Takashi Iwai,
russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20170108134427.8392-4-hdegoede@redhat.com>
On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> 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.
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.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..cf7a2a0 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
>
> @@ -62,6 +62,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
> + iosf_mbi_punit_unlock();
> @@ -79,6 +81,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev
> + iosf_mbi_punit_lock();
For me it looks a bit asymmetrical.
Can we use acquire()/release()?
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH 4/7] i2c: designware-baytrail: Call pmic_bus_access_notifier_chain
From: Andy Shevchenko @ 2017-01-08 15:23 UTC (permalink / raw)
To: Hans de Goede, Daniel Vetter, Jani Nikula,
Ville Syrjälä, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: intel-gfx, dri-devel, Mika Westerberg, Takashi Iwai,
russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20170108134427.8392-5-hdegoede@redhat.com>
On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> Call the iosf_mbi pmic_bus_access_notifier_chain on bus acquire /
> release.
>
FWIW:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.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 cf7a2a0..e8cf10a 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -63,6 +63,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
>
> pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
>
> + iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACC
> ESS_END,
> + NULL);
> iosf_mbi_punit_unlock();
> }
>
> @@ -82,6 +84,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev
> *dev)
> return 0;
>
> iosf_mbi_punit_lock();
> + iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACC
> ESS_BEGIN,
> + NULL);
>
> /*
> * Disallow the CPU to enter C6 or C7 state, entering these
> states
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH 2/7] x86/platform/intel/iosf_mbi: Add a pmic bus access notifier
From: Andy Shevchenko @ 2017-01-08 15:21 UTC (permalink / raw)
To: Hans de Goede, Daniel Vetter, Jani Nikula,
Ville Syrjälä, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: intel-gfx, dri-devel, Mika Westerberg, Takashi Iwai,
russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20170108134427.8392-3-hdegoede@redhat.com>
On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> Some drivers may need to acquire punit managed resources from
> interrupt
> context, where they cannot call iosf_mbi_punit_lock().
>
> This commit adds a notifier chain which allows a driver to get
> notified
> (in a process context) before other drivers start accessing the pmic
> bus,
> so that the driver can acquire any resources, which it may need during
> the window the other driver is accessing the pmic, before hand.
>
Same comments as per patch 1.
I'm okay with the patch, but I would hear from other stakeholders that's
_the_ way we are going.
So, FWIW:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> ---
> arch/x86/include/asm/iosf_mbi.h | 54
> ++++++++++++++++++++++++++++++++++++++
> arch/x86/platform/intel/iosf_mbi.c | 36 +++++++++++++++++++++++++
> 2 files changed, 90 insertions(+)
>
> diff --git a/arch/x86/include/asm/iosf_mbi.h
> b/arch/x86/include/asm/iosf_mbi.h
> index 91f5d16..b8733bb 100644
> --- a/arch/x86/include/asm/iosf_mbi.h
> +++ b/arch/x86/include/asm/iosf_mbi.h
> @@ -47,6 +47,10 @@
> #define QRK_MBI_UNIT_MM 0x05
> #define QRK_MBI_UNIT_SOC 0x31
>
> +/* Action values for the pmic_bus_access_notifier functions */
> +#define MBI_PMIC_BUS_ACCESS_BEGIN 1
> +#define MBI_PMIC_BUS_ACCESS_END 2
> +
> #if IS_ENABLED(CONFIG_IOSF_MBI)
>
> bool iosf_mbi_available(void);
> @@ -115,6 +119,38 @@ void iosf_mbi_punit_lock(void);
> */
> void iosf_mbi_punit_unlock(void);
>
> +/**
> + * iosf_mbi_register_pmic_bus_access_notifier - Register pmic bus
> notifier
> + *
> + * This function can be used by drivers which may need to acquire
> punit
> + * managed resources from interrupt context, where
> iosf_mbi_punit_lock()
> + * can not be used.
> + *
> + * This function allows a driver to register a notifier to get
> notified (in a
> + * process context) before other drivers start accessing the pmic
> bus.
> + *
> + * This allows the driver to acquire any resources, which it may need
> during
> + * the window the other driver is accessing the pmic, before hand.
> + *
> + * @nb: notifier_block to register
> + */
> +int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block
> *nb);
> +
> +/**
> + * iosf_mbi_register_pmic_bus_access_notifier - Unregister pmic bus
> notifier
> + *
> + * @nb: notifier_block to unregister
> + */
> +int iosf_mbi_unregister_pmic_bus_access_notifier(struct
> notifier_block *nb);
> +
> +/**
> + * iosf_mbi_call_pmic_bus_access_notifier_chain - Call pmic bus
> notifier chain
> + *
> + * @val: action to pass into listener's notifier_call function
> + * @v: data pointer to pass into listener's notifier_call function
> + */
> +int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val,
> void *v);
> +
> #else /* CONFIG_IOSF_MBI is not enabled */
> static inline
> bool iosf_mbi_available(void)
> @@ -146,6 +182,24 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32
> offset, u32 mdr, u32 mask)
> static inline void iosf_mbi_punit_lock(void) {}
> static inline void iosf_mbi_punit_unlock(void) {}
>
> +static inline
> +int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block
> *nb)
> +{
> + return 0;
> +}
> +
> +static inline
> +int iosf_mbi_unregister_pmic_bus_access_notifier(struct
> notifier_block *nb)
> +{
> + return 0;
> +}
> +
> +static inline
> +int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val,
> void *v)
> +{
> + return 0;
> +}
> +
> #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 75d8135..a995789 100644
> --- a/arch/x86/platform/intel/iosf_mbi.c
> +++ b/arch/x86/platform/intel/iosf_mbi.c
> @@ -35,6 +35,7 @@
> static struct pci_dev *mbi_pdev;
> static DEFINE_SPINLOCK(iosf_mbi_lock);
> static DEFINE_MUTEX(iosf_mbi_punit_mutex);
> +static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
>
> static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
> {
> @@ -203,6 +204,41 @@ void iosf_mbi_punit_unlock(void)
> }
> EXPORT_SYMBOL(iosf_mbi_punit_unlock);
>
> +int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block
> *nb)
> +{
> + int ret;
> +
> + /* Wait for the bus to go inactive before registering */
> + mutex_lock(&iosf_mbi_punit_mutex);
> + ret = blocking_notifier_chain_register(
> + &iosf_mbi_pmic_bus_access_notifier,
> nb);
> + mutex_unlock(&iosf_mbi_punit_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(iosf_mbi_register_pmic_bus_access_notifier);
> +
> +int iosf_mbi_unregister_pmic_bus_access_notifier(struct
> notifier_block *nb)
> +{
> + int ret;
> +
> + /* Wait for the bus to go inactive before unregistering */
> + mutex_lock(&iosf_mbi_punit_mutex);
> + ret = blocking_notifier_chain_unregister(
> + &iosf_mbi_pmic_bus_access_notifier,
> nb);
> + mutex_unlock(&iosf_mbi_punit_mutex);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier);
> +
> +int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val,
> void *v)
> +{
> + return blocking_notifier_call_chain(
> + &iosf_mbi_pmic_bus_access_notifier,
> val, v);
> +}
> +EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain);
> +
> #ifdef CONFIG_IOSF_MBI_DEBUG
> static u32 dbg_mdr;
> static u32 dbg_mcr;
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH 1/7] x86/platform/intel/iosf_mbi: Add a mutex for punit access
From: Andy Shevchenko @ 2017-01-08 15:16 UTC (permalink / raw)
To: Hans de Goede, Daniel Vetter, Jani Nikula,
Ville Syrjälä, Jarkko Nikula, Wolfram Sang, Len Brown
Cc: intel-gfx, dri-devel, Mika Westerberg, Takashi Iwai,
russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20170108134427.8392-2-hdegoede@redhat.com>
On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> One some systems the punit accesses the pmic to change various
> voltages
> through the same bus as other kernel drivers use for e.g. battery
> monitoring.
>
> If a driver sends requests to the punit which require the punit to
> access
> the pmic bus while another driver is also accessing the pmic bus
> various
> bad things happen.
>
> This commit adds a mutex to protect the punit against simultaneous
> accesses
> and 2 functions to lock / unlock this mutex.
>
> Note on these systems the i2c-bus driver will request a sempahore from
> the
> punit for exclusive access to the pmic bus when i2c drivers are
> accessing
> it, but this does not appear to be sufficient, we still need to avoid
> making certain punit requests during the access window to avoid
> problems.
I'm fine with the patch, but please spell
P-Unit
PMIC
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> ---
> arch/x86/include/asm/iosf_mbi.h | 31
> +++++++++++++++++++++++++++++++
> arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++
> 2 files changed, 44 insertions(+)
>
> diff --git a/arch/x86/include/asm/iosf_mbi.h
> b/arch/x86/include/asm/iosf_mbi.h
> index b41ee16..91f5d16 100644
> --- a/arch/x86/include/asm/iosf_mbi.h
> +++ b/arch/x86/include/asm/iosf_mbi.h
> @@ -88,6 +88,33 @@ 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
> + *
> + * One some systems the punit accesses the pmic to change various
> voltages
> + * through the same bus as other kernel drivers use for e.g. battery
> monitoring.
> + *
> + * If a driver sends requests to the punit which require the punit to
> access the
> + * pmic bus while another driver is also accessing the pmic bus
> various bad
> + * things happen.
> + *
> + * To avoid these problems this function must be called before
> accessing the
> + * punit or the pmic, be it through iosf_mbi* functions or through
> other means.
> + *
> + * Note on these systems the i2c-bus driver will request a sempahore
> from the
> + * punit for exclusive access to the pmic bus when i2c drivers are
> accessing it,
> + * but this does not appear to be sufficient, we still need to avoid
> making
> + * certain punit requests during the access window to avoid problems.
> + *
> + * 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 +142,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;
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* [PATCH 7/7] drm/i915: Take punit lock when modifying punit settings
From: Hans de Goede @ 2017-01-08 13:44 UTC (permalink / raw)
To: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Hans de Goede, intel-gfx, dri-devel, Mika Westerberg,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20170108134427.8392-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.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
drivers/gpu/drm/i915/intel_display.c | 6 ++++++
drivers/gpu/drm/i915/intel_pm.c | 9 +++++++++
drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
3 files changed, 24 insertions(+)
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 4b12637..0d55b61 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);
}
@@ -4981,7 +4988,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
if (val != dev_priv->rps.cur_freq) {
+ iosf_mbi_punit_lock();
vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
+ iosf_mbi_punit_unlock();
if (!IS_CHERRYVIEW(dev_priv))
gen6_set_rps_thresholds(dev_priv, val);
}
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 6/7] drm/i915: Listen for pmic bus access notifications
From: Hans de Goede @ 2017-01-08 13:44 UTC (permalink / raw)
To: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Hans de Goede, intel-gfx, dri-devel, Mika Westerberg,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20170108134427.8392-1-hdegoede@redhat.com>
Listen for pmic bus access notifications and get FORCEWAKE_ALL while
the bus is accessed to avoid needing to do any forcewakes, which need
pmic bus access, while the pmic bus is busy:
This fixes errors like these showing up in dmesg, usually followed
by a gfx or system freeze:
[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.
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
Downside of this approach is that it causes wakeups whenever the pmic
bus is accessed. Unfortunately we cannot simply wait for the pmic bus
to go idle when we hit a race, as forcewakes may be done from interrupt
handlers where we cannot sleep to wait for the i2c pmic bus access to
finish.
Note that the notifications and thus the wakeups will only happen on
baytrail / cherrytrail devices using pmic-s with a shared i2c bus for
punit and host pmic access (i2c busses with a _SEM method in their
APCI node), e.g. an axp288 pmic.
I plan to write some patches for drivers accessing the pmic bus to
limit their bus accesses to a bare minimum (e.g. cache registers, do not
update battery level more often then 4 times a minute), to limit the
amount of wakeups.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_uncore.c | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7cd0363..60297de 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -731,6 +731,7 @@ struct intel_uncore {
const struct intel_forcewake_range *fw_domains_table;
unsigned int fw_domains_table_entries;
+ struct notifier_block pmic_bus_access_nb;
struct intel_uncore_funcs funcs;
unsigned fifo_count;
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c7fa222..1668de4 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -25,6 +25,7 @@
#include "intel_drv.h"
#include "i915_vgpu.h"
+#include <asm/iosf_mbi.h>
#include <linux/pm_runtime.h>
#define FORCEWAKE_ACK_TIMEOUT_MS 50
@@ -429,12 +430,16 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
void intel_uncore_suspend(struct drm_i915_private *dev_priv)
{
+ iosf_mbi_unregister_pmic_bus_access_notifier(
+ &dev_priv->uncore.pmic_bus_access_nb);
__intel_uncore_forcewake_reset(dev_priv, false);
}
void intel_uncore_resume(struct drm_i915_private *dev_priv)
{
__intel_uncore_early_sanitize(dev_priv, true);
+ iosf_mbi_register_pmic_bus_access_notifier(
+ &dev_priv->uncore.pmic_bus_access_nb);
i915_check_and_clear_faults(dev_priv);
}
@@ -1390,6 +1395,28 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
dev_priv->uncore.fw_domains_table_entries = ARRAY_SIZE((d)); \
}
+static int i915_pmic_bus_access_notifier(struct notifier_block *nb,
+ unsigned long action, void *data)
+{
+ struct drm_i915_private *dev_priv = container_of(nb,
+ struct drm_i915_private, uncore.pmic_bus_access_nb);
+
+ switch (action) {
+ case MBI_PMIC_BUS_ACCESS_BEGIN:
+ /*
+ * forcewake all to make sure that we don't need to forcewake
+ * any power-planes while the pmic bus is busy.
+ */
+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+ break;
+ case MBI_PMIC_BUS_ACCESS_END:
+ intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+ break;
+ }
+
+ return NOTIFY_OK;
+}
+
void intel_uncore_init(struct drm_i915_private *dev_priv)
{
i915_check_vgpu(dev_priv);
@@ -1399,6 +1426,8 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
__intel_uncore_early_sanitize(dev_priv, false);
dev_priv->uncore.unclaimed_mmio_check = 1;
+ dev_priv->uncore.pmic_bus_access_nb.notifier_call =
+ i915_pmic_bus_access_notifier;
switch (INTEL_INFO(dev_priv)->gen) {
default:
@@ -1458,6 +1487,9 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
ASSIGN_READ_MMIO_VFUNCS(vgpu);
}
+ iosf_mbi_register_pmic_bus_access_notifier(
+ &dev_priv->uncore.pmic_bus_access_nb);
+
i915_check_and_clear_faults(dev_priv);
}
#undef ASSIGN_WRITE_MMIO_VFUNCS
@@ -1465,6 +1497,9 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
void intel_uncore_fini(struct drm_i915_private *dev_priv)
{
+ iosf_mbi_unregister_pmic_bus_access_notifier(
+ &dev_priv->uncore.pmic_bus_access_nb);
+
/* Paranoia: make sure we have disabled everything before we exit. */
intel_uncore_sanitize(dev_priv);
__intel_uncore_forcewake_reset(dev_priv, false);
--
2.9.3
^ permalink raw reply related
* [PATCH 5/7] drm/i915: Add intel_uncore_suspend / resume functions
From: Hans de Goede @ 2017-01-08 13:44 UTC (permalink / raw)
To: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Hans de Goede, intel-gfx, dri-devel, Mika Westerberg,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20170108134427.8392-1-hdegoede@redhat.com>
Rename intel_uncore_early_sanitize to intel_uncore_resume, dropping the
(always true) restore_forcewake argument and add a new intel_uncore_resume
function to replace the intel_uncore_forcewake_reset(dev_priv, false)
calls done from the suspend / runtime_suspend functions and make
intel_uncore_forcewake_reset private.
This is a preparation patch for adding pmic bus access notifier support.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
drivers/gpu/drm/i915/i915_drv.c | 6 +++---
drivers/gpu/drm/i915/i915_drv.h | 6 ++----
drivers/gpu/drm/i915/intel_uncore.c | 18 +++++++++++-------
3 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2c020ea..ec61e6e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1445,7 +1445,7 @@ static int i915_drm_suspend(struct drm_device *dev)
opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold;
intel_opregion_notify_adapter(dev_priv, opregion_target_state);
- intel_uncore_forcewake_reset(dev_priv, false);
+ intel_uncore_suspend(dev_priv);
intel_opregion_unregister(dev_priv);
intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
@@ -1690,7 +1690,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
DRM_ERROR("Resume prepare failed: %d, continuing anyway\n",
ret);
- intel_uncore_early_sanitize(dev_priv, true);
+ intel_uncore_resume(dev_priv);
if (IS_BROXTON(dev_priv)) {
if (!dev_priv->suspended_to_idle)
@@ -2343,7 +2343,7 @@ static int intel_runtime_suspend(struct device *kdev)
return ret;
}
- intel_uncore_forcewake_reset(dev_priv, false);
+ intel_uncore_suspend(dev_priv);
enable_rpm_wakeref_asserts(dev_priv);
WARN_ON_ONCE(atomic_read(&dev_priv->pm.wakeref_count));
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5194686..7cd0363 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3052,14 +3052,12 @@ int intel_irq_install(struct drm_i915_private *dev_priv);
void intel_irq_uninstall(struct drm_i915_private *dev_priv);
extern void intel_uncore_sanitize(struct drm_i915_private *dev_priv);
-extern void intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
- bool restore_forcewake);
extern void intel_uncore_init(struct drm_i915_private *dev_priv);
extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
extern bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv);
extern void intel_uncore_fini(struct drm_i915_private *dev_priv);
-extern void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
- bool restore);
+extern void intel_uncore_suspend(struct drm_i915_private *dev_priv);
+extern void intel_uncore_resume(struct drm_i915_private *dev_priv);
const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
void intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
enum forcewake_domains domains);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 8fc5f29..c7fa222 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -250,7 +250,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
return HRTIMER_NORESTART;
}
-void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
+static void __intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
bool restore)
{
unsigned long irqflags;
@@ -424,13 +424,17 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_B_LAST))
info->has_decoupled_mmio = false;
- intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
+ __intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
}
-void intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
- bool restore_forcewake)
+void intel_uncore_suspend(struct drm_i915_private *dev_priv)
{
- __intel_uncore_early_sanitize(dev_priv, restore_forcewake);
+ __intel_uncore_forcewake_reset(dev_priv, false);
+}
+
+void intel_uncore_resume(struct drm_i915_private *dev_priv)
+{
+ __intel_uncore_early_sanitize(dev_priv, true);
i915_check_and_clear_faults(dev_priv);
}
@@ -1463,7 +1467,7 @@ void intel_uncore_fini(struct drm_i915_private *dev_priv)
{
/* Paranoia: make sure we have disabled everything before we exit. */
intel_uncore_sanitize(dev_priv);
- intel_uncore_forcewake_reset(dev_priv, false);
+ __intel_uncore_forcewake_reset(dev_priv, false);
}
#define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1)
@@ -1679,7 +1683,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
ret = gen6_hw_domain_reset(dev_priv, hw_mask);
- intel_uncore_forcewake_reset(dev_priv, true);
+ __intel_uncore_forcewake_reset(dev_priv, true);
return ret;
}
--
2.9.3
^ permalink raw reply related
* [PATCH 4/7] i2c: designware-baytrail: Call pmic_bus_access_notifier_chain
From: Hans de Goede @ 2017-01-08 13:44 UTC (permalink / raw)
To: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Hans de Goede, intel-gfx, dri-devel, Mika Westerberg,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20170108134427.8392-1-hdegoede@redhat.com>
Call the iosf_mbi pmic_bus_access_notifier_chain on bus acquire / release.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.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 cf7a2a0..e8cf10a 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -63,6 +63,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
+ iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_END,
+ NULL);
iosf_mbi_punit_unlock();
}
@@ -82,6 +84,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
return 0;
iosf_mbi_punit_lock();
+ iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_BEGIN,
+ NULL);
/*
* Disallow the CPU to enter C6 or C7 state, entering these states
--
2.9.3
^ permalink raw reply related
* [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire
From: Hans de Goede @ 2017-01-08 13:44 UTC (permalink / raw)
To: Daniel Vetter, Jani Nikula, Ville Syrjälä,
Jarkko Nikula, Wolfram Sang, Len Brown, Andy Shevchenko
Cc: Hans de Goede, intel-gfx, dri-devel, Mika Westerberg,
Takashi Iwai, russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20170108134427.8392-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.
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.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..cf7a2a0 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -62,6 +62,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
+
+ iosf_mbi_punit_unlock();
}
static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
@@ -79,6 +81,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
if (!dev->release_lock)
return 0;
+ iosf_mbi_punit_lock();
+
/*
* 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
--
2.9.3
^ permalink raw reply related
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