* Re: [PATCH v8 07/12] dt-bindings: i2c: i2c-mux-simple: document i2c-mux-simple bindings
From: Peter Rosin @ 2017-01-28 22:42 UTC (permalink / raw)
To: Rob Herring
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Wolfram Sang, Mark Rutland,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler, Jonathan Corbet, Andrew Morton,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170127193917.6xuunrkjkqh4f2v7@rob-hp-laptop>
On 2017-01-27 20:39, Rob Herring wrote:
> On Wed, Jan 18, 2017 at 04:57:10PM +0100, Peter Rosin wrote:
>> Describe how a generic multiplexer controller is used to mux an i2c bus.
>>
>> Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>> ---
>> .../devicetree/bindings/i2c/i2c-mux-simple.txt | 81 ++++++++++++++++++++++
>> 1 file changed, 81 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
>> new file mode 100644
>> index 000000000000..253d5027843b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
>> @@ -0,0 +1,81 @@
>> +Simple I2C Bus Mux
>> +
>> +This binding describes an I2C bus multiplexer that uses a mux controller
>> +from the mux subsystem to route the I2C signals.
>> +
>> + .-----. .-----.
>> + | dev | | dev |
>> + .------------. '-----' '-----'
>> + | SoC | | |
>> + | | .--------+--------'
>> + | .------. | .------+ child bus A, on MUX value set to 0
>> + | | I2C |-|--| Mux |
>> + | '------' | '--+---+ child bus B, on MUX value set to 1
>> + | .------. | | '----------+--------+--------.
>> + | | MUX- | | | | | |
>> + | | Ctrl |-|-----+ .-----. .-----. .-----.
>> + | '------' | | dev | | dev | | dev |
>> + '------------' '-----' '-----' '-----'
>> +
>> +Required properties:
>> +- compatible: i2c-mux-simple,mux-locked or i2c-mux-simple,parent-locked
>
> Not a fan of using "simple" nor the ','. Perhaps lock type should be
> separate property.
How about just i2c-mux for the compatible? Because i2c-mux-mux (which
follows the naming of previous i2c muxes) looks really stupid. Or
perhaps i2c-mux-generic?
I'm also happy to have the lock type as a separate property. One lock
type, e.g. parent-locked, could be the default and adding a 'mux-locked'
property could change that. Would that be ok?
Or should it be a requirement that one of 'mux-locked'/'parent-locked'
is always present?
> I'm not sure I get the mux vs. parent locked fully. How do I determine
> what type I have? We already have bindings for several types of i2c
> muxes. How does the locking annotation fit into those?
We have briefly discussed this before [1] in the context of i2c-mux-gpio
and i2c-mux-pinctrl, when I added the mux-locked/parent-locked distinction
to the i2c-mux infrastructure (it wasn't named mux-locked/parent-locked
way back then though). There is more detail on what the difference is
between the two in Documentation/i2c/i2c-topology.
Side note regarding your remark "use an I2C controlled mux instead"; it
appears that I'm not alone [2] with this kind of requirement...
[1] https://lkml.org/lkml/2016/1/6/437
[2] http://marc.info/?t=147877959100002&r=1&w=2
But, now that I have pondered on this for a year or so, I firmly
believe it was a mistake to have the code in i2c-mux-gpio and
i2c-mux-pinctrl automatically try to deduce if the mux should be
mux-locked or parent-locked. It might be easy to make that call
in some trivial cases, but it is not difficult to dream up
scenarios where it would be extremely hard for the code to get
this decision right. It's just fragile. But now we have code in
those two muxes that has unwanted tentacles into the guts of the
gpio and pinctrl subsystems. Hopefully those unwanted tentacles
can be replaced with something based on device links? However, it
is still not hard to come up with scenarios that will require
manual intervention in order to select the right kind of i2c mux
locking. So, I fear that we have inadequate code trying to make a
decision automatically, and that we at some point down the line
will have some impossible case needing a binding that trumps the
heuristic. Why have a heuristic at all in that case? In short, it
should have been a binding from the start, methinks.
That was a long rant regarding i2c-mux-gpio and i2c-mux-pinctrl.
I obviously think it is bad to have this new i2c mux go in the
same direction as those two drivers. I.e. I think a binding that
specifies the desired locking is preferable to any attempted
heuristic.
Cheers,
peda
^ permalink raw reply
* Re: [PATCH] i2c: busses: constify i2c_algorithm structures
From: Uwe Kleine-König @ 2017-01-29 10:28 UTC (permalink / raw)
To: Bhumika Goyal
Cc: jdelvare, andriy.shevchenko, kernel, wsa, patrice.chotard,
adi-buildroot-devel, linux-kernel, julia.lawall,
ludovic.desroches, jarkko.nikula, linux-i2c, sonic.zhang,
mika.westerberg, linux-arm-kernel
In-Reply-To: <1485540377-13686-1-git-send-email-bhumirks@gmail.com>
Hello,
On Fri, Jan 27, 2017 at 11:36:17PM +0530, Bhumika Goyal wrote:
> drivers/i2c/busses/i2c-imx.c | 2 +-
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> for
i2c-imx.c
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH] i2c: piix4: Fix request_region size
From: Ricardo Ribalda Delgado @ 2017-01-30 1:11 UTC (permalink / raw)
To: Jean Delvare; +Cc: Wolfram Sang, Andy Shevchenko, linux-i2c, LKML
In-Reply-To: <20170128121346.1e530c1c@endymion>
Hi Jean and Wolfram
On Sat, Jan 28, 2017 at 12:13 PM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Wolfram,
>
> On Sat, 28 Jan 2017 12:00:21 +0100, Wolfram Sang wrote:
>>
>> > > /* count for request_region */
>> > > -#define SMBIOSIZE 8
>> > > +#define SMBIOSIZE 9
>> >
>> > Are you certain that all supported devices have this extra register?
>>
>> Isn't it better to have a potentially unused register mapped than a
>> potentially used register unmapped
We have been "lucky" that it is a ioport and not a mmap region,
otherwise we would have seen a nice oops :).
>
> My concern is that the region request could fail due to a conflict with
> another device, if the physical I/O region is only 8 and we try to
> request 9.
I do not think that this is the case, if you check the top of the file
you can see how there are many other definitions for registers. I
expect that SMBIOSIZE=8 is just the original author being
conservative.
Of course I cannot say that there is one platform where one extra
ioport can cause a conflict, but I believe that we must request all
the ports that we will use.
Regards!
>
> --
> Jean Delvare
> SUSE L3 Support
--
Ricardo Ribalda
^ permalink raw reply
* [PATCH] I2c: busses - Fix possible NULL derefrence.
From: Shailendra Verma @ 2017-01-30 5:03 UTC (permalink / raw)
To: Laxman Dewangan, Wolfram Sang, Stephen Warren, Thierry Reding,
Alexandre Courbot, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
p.shailesh-Sze3O3UU22JBDgjK7y7TUQ,
ashish.kalra-Sze3O3UU22JBDgjK7y7TUQ, Shailendra Verma,
Shailendra Verma
In-Reply-To: <CGME20170130050324epcas3p28c9adcffd7bc40e1733cdeef1e5240d5@epcas3p2.samsung.com>
of_device_get_match_data could return NULL, and so can cause
a NULL pointer dereference later.
Signed-off-by: Shailendra Verma <shailendra.v-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
drivers/i2c/busses/i2c-tegra.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 4af9bba..93ac1e1 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -920,6 +920,10 @@ static int tegra_i2c_probe(struct platform_device *pdev)
tegra_i2c_parse_dt(i2c_dev);
i2c_dev->hw = of_device_get_match_data(&pdev->dev);
+ if (!i2c_dev->hw) {
+ dev_err(&pdev->dev, "no device match found\n");
+ return -ENODEV;
+ }
i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
"nvidia,tegra20-i2c-dvc");
init_completion(&i2c_dev->msg_complete);
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v2 1/2] dt-bindings: Add Tegra186 BPMP I2C binding
From: Thierry Reding @ 2017-01-30 6:47 UTC (permalink / raw)
To: Rob Herring, Wolfram Sang
Cc: Stephen Warren, Alexandre Courbot, Jon Hunter,
linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAL_Jsq+Sr=dOw7_9UAL4jO0zXycT7sHoCvUM4C2-9LBgdx=25g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4166 bytes --]
On Fri, Jan 27, 2017 at 04:52:37PM -0600, Rob Herring wrote:
> On Fri, Jan 27, 2017 at 4:19 PM, Thierry Reding
> <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Fri, Jan 27, 2017 at 09:39:38AM +0100, Thierry Reding wrote:
> >> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >>
> >> In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
> >> HW devices, such as the I2C controller for the power management I2C bus.
> >> Software running on other CPUs must perform IPC to the BPMP in order to
> >> execute transactions on that I2C bus. This binding describes an I2C bus
> >> that is accessed in such a fashion.
> >>
> >> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> Signed-off-by: Tom Warren <twarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> ---
> >> .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt | 42 ++++++++++++++++++++++
> >> 1 file changed, 42 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> >
> > Rob, sorry for not Cc'ing you earlier on this, but I had thought this
> > binding had already been reviewed and acked since it is merged in
> > U-Boot.
>
> Ha! Good one. :(
So I went looking for earlier discussion on this patch because I
distinctly remembered you being part of it, and found this:
https://patchwork.ozlabs.org/patch/650385/
You already gave an Acked-by, but I failed to pick it up for some
reason. Given what you said below I'm going to assume that it still
applies.
Wolfram, do you want me to resend with Rob's Acked-by from back in
July or do you want to add it yourself when applying?
Thanks,
Thierry
> > Wolfram was concerned in particular about the nvidia,bpmp-bus-id
> > property below.
> >
> > Can you give this a quick look, please?
> >
> > Thanks,
> > Thierry
> >
> >> diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> >> new file mode 100644
> >> index 000000000000..ab240e10debc
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> >> @@ -0,0 +1,42 @@
> >> +NVIDIA Tegra186 BPMP I2C controller
> >> +
> >> +In Tegra186, the BPMP (Boot and Power Management Processor) owns certain HW
> >> +devices, such as the I2C controller for the power management I2C bus. Software
> >> +running on other CPUs must perform IPC to the BPMP in order to execute
> >> +transactions on that I2C bus. This binding describes an I2C bus that is
> >> +accessed in such a fashion.
> >> +
> >> +The BPMP I2C node must be located directly inside the main BPMP node. See
> >> +../firmware/nvidia,tegra186-bpmp.txt for details of the BPMP binding.
> >> +
> >> +This node represents an I2C controller. See ../i2c/i2c.txt for details of the
> >> +core I2C binding.
> >> +
> >> +Required properties:
> >> +- compatible:
> >> + Array of strings.
> >> + One of:
> >> + - "nvidia,tegra186-bpmp-i2c".
> >> +- #address-cells: Address cells for I2C device address.
> >> + Single-cell integer.
> >> + Must be <1>.
> >> +- #size-cells:
> >> + Single-cell integer.
> >> + Must be <0>.
> >> +- nvidia,bpmp-bus-id:
> >> + Single-cell integer.
> >> + Indicates the I2C bus number this DT node represent, as defined by the
> >> + BPMP firmware.
>
> This seems okay to me given it's a pretty specific use and dictated by
> the firmware. If there were multiple, then perhaps we should use reg,
> but that may collide with other BPMP child nodes.
>
> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: Add Tegra186 BPMP I2C binding
From: Thierry Reding @ 2017-01-30 6:50 UTC (permalink / raw)
To: Stephen Warren
Cc: Wolfram Sang, Rob Herring, Alexandre Courbot, Jon Hunter,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <8841854b-d058-1a8d-3e40-955bf3546cef-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1986 bytes --]
On Fri, Jan 27, 2017 at 10:42:27PM -0700, Stephen Warren wrote:
> On 01/27/2017 03:19 PM, Thierry Reding wrote:
> > On Fri, Jan 27, 2017 at 09:39:38AM +0100, Thierry Reding wrote:
> > > From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > >
> > > In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
> > > HW devices, such as the I2C controller for the power management I2C bus.
> > > Software running on other CPUs must perform IPC to the BPMP in order to
> > > execute transactions on that I2C bus. This binding describes an I2C bus
> > > that is accessed in such a fashion.
> > >
> > > Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > Signed-off-by: Tom Warren <twarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > ---
> > > .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt | 42 ++++++++++++++++++++++
> > > 1 file changed, 42 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> >
> > Rob, sorry for not Cc'ing you earlier on this, but I had thought this
> > binding had already been reviewed and acked since it is merged in
> > U-Boot.
>
> I'm pretty sure all the DT binding patches for Tegra186 that I submitted
> were ack'd by Rob. They were certainly all posted to relevant Linux lists
> and discussed and ack'd by someone before I pushed them into U-Boot.
> Probably 2nd quarter last year, or maybe 3rd.
Yes, I thought I had seen an Acked-by myself, and I know that you're
very diligent about this. And true enough, it's in patchwork. I probably
missed it because I was cherry-picking it directly from U-Boot where the
Acked-by wasn't present.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] I2c: busses - Fix possible NULL derefrence.
From: Thierry Reding @ 2017-01-30 7:12 UTC (permalink / raw)
To: Shailendra Verma
Cc: Laxman Dewangan, Wolfram Sang, Stephen Warren, Alexandre Courbot,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
p.shailesh-Sze3O3UU22JBDgjK7y7TUQ,
ashish.kalra-Sze3O3UU22JBDgjK7y7TUQ, Shailendra Verma
In-Reply-To: <1485752587-30107-1-git-send-email-shailendra.v-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 503 bytes --]
On Mon, Jan 30, 2017 at 10:33:07AM +0530, Shailendra Verma wrote:
> of_device_get_match_data could return NULL, and so can cause
> a NULL pointer dereference later.
>
> Signed-off-by: Shailendra Verma <shailendra.v-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
> drivers/i2c/busses/i2c-tegra.c | 4 ++++
> 1 file changed, 4 insertions(+)
This will never happen. Any match in the OF table that would cause the
->probe() to occur has a valid .data pointer associated with it.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v8 12/12] mux: support simplified bindings for single-user gpio mux
From: Peter Rosin @ 2017-01-30 8:02 UTC (permalink / raw)
To: Rob Herring
Cc: Jonathan Cameron, linux-kernel, Wolfram Sang, Mark Rutland,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Jonathan Corbet, Andrew Morton, linux-i2c, devicetree, linux-iio,
linux-doc
In-Reply-To: <20170127155202.z2ufbq5bdtclk5oo@rob-hp-laptop>
[-- Attachment #1: Type: text/plain, Size: 2895 bytes --]
On 2017-01-27 16:52, Rob Herring wrote:
> On Mon, Jan 23, 2017 at 11:24:18AM +0100, Peter Rosin wrote:
>> On 2017-01-22 14:30, Jonathan Cameron wrote:
>>> On 18/01/17 15:57, Peter Rosin wrote:
>>>> Allow bindings for a GPIO controlled mux to be specified in the
>>>> mux consumer node.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>> Code is good as far as I am concerned. Only question is whether this
>>
>> Hmmm, now that I think some more about it, the code supporting the
>> simplified binding (patch 12/12) is a bit fishy in one respect.
>>
>> A driver that calls mux_control_get and gets a mux_control that happens
>> to be backed by an implicit mux chip (i.e. using the simplified binding)
>> will not be able to reverse the resource allocation with less than a
>> complete destruction of itself. Now, this is likely not a problem in
>> most cases, but I bet it will creep up at the most inopportune time. And
>> your remark that I'm the one that has to maintain this makes me dislike
>> this concept...
>>
>> I.e. mux_control_put *should* reverse mux_control_get, but this simply
>> does not happen for the implicit mux chips, as implicit mux chips are
>> not put away until the owning device is put away.
>
> I think this is because you aren't creating a device in this case. Nodes
> in DT are not the only way to create devices. Drivers can create a child
> device when they find mux-gpios property.
Yes, but even with such a child device, a flag is needed somewhere that
triggers cleanup when the mux_control is put away. And then it is possible
to cleanup w/o the help of a child device. I wrote some code for this when
I realized the problem, and it looks simple enough, but I haven't tested
it yet, so who knows... It is attached (patch to be applied on top of 12/12)
if anyone cares.
>> Every time I have tried to come up with a way to implement the simplified
>> bindings I seem to hit one of these subtleties.
>>
>>> is worth the hassle given the normal bindings don't give that high
>>> a burden in complexity!
>
> I was going to change my mind here, but we already have "mux-gpios" as a
> binding at least for i2c-gpio-mux. So really the question is do we want
> to support that here?
I think my preference is to drop the simplified binding, but I can also
live with it. But as there appears to be no strong feelings, let's just
drop it. It is always possible to add it later. Ok?
>> I am missing an ack from Rob though.
>>
>>> I don't really care either way:)
>>
>> But Rob seems to care, this series just has to find a way to get out of
>> his too-much-churn-will-look-at-it-later list. I sadly don't know how to
>> pull that trick...
>
> By complaining that I'm putting it off... :) I guess I'm okay with this
> series in general. I will reply on the specific patches today.
Great, it appears that I'm quite the magician. :-) Thanks!
Cheers,
peda
[-- Attachment #2: 0001-mux-fix-cleanup-for-simplified-bindings.patch --]
[-- Type: text/plain, Size: 2402 bytes --]
>From 5c448b8dfd831c7bc501d9543d48b2077ee1ba7b Mon Sep 17 00:00:00 2001
From: Peter Rosin <peda@axentia.se>
Date: Tue, 24 Jan 2017 16:58:58 +0100
Subject: [PATCH] mux: fix cleanup for simplified bindings
---
drivers/mux/mux-core.c | 19 ++++++++++++++++++-
include/linux/mux.h | 2 ++
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
index 0caafd6f5a77..53954bd12709 100644
--- a/drivers/mux/mux-core.c
+++ b/drivers/mux/mux-core.c
@@ -321,9 +321,12 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
if (ret == -ENOENT && !mux_name) {
mux_chip = mux_gpio_alloc(dev);
if (!IS_ERR(mux_chip)) {
+ mux_chip->private = true;
ret = devm_mux_chip_register(dev, mux_chip);
- if (ret < 0)
+ if (ret < 0) {
+ devm_mux_chip_free(dev, mux_chip);
return ERR_PTR(ret);
+ }
get_device(&mux_chip->dev);
return mux_chip->mux;
}
@@ -344,6 +347,12 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
if (!mux_chip)
return ERR_PTR(-EPROBE_DEFER);
+ if (mux_chip->private) {
+ dev_err(dev, "%s: private mux chip specified in %s\n",
+ np->full_name, args.np->full_name);
+ return ERR_PTR(-EBUSY);
+ }
+
if (args.args_count > 1 ||
(!args.args_count && (mux_chip->controllers > 1))) {
dev_err(dev, "%s: wrong #mux-control-cells for %s\n",
@@ -368,7 +377,15 @@ EXPORT_SYMBOL_GPL(mux_control_get);
void mux_control_put(struct mux_control *mux)
{
+ bool private = mux->chip->private;
+ struct device *parent = mux->chip->dev.parent;
+
put_device(&mux->chip->dev);
+
+ if (private) {
+ devm_mux_chip_unregister(parent, mux->chip);
+ devm_mux_chip_free(parent, mux->chip);
+ }
}
EXPORT_SYMBOL_GPL(mux_control_put);
diff --git a/include/linux/mux.h b/include/linux/mux.h
index ec9e605d8acf..3ad2e475c9dd 100644
--- a/include/linux/mux.h
+++ b/include/linux/mux.h
@@ -49,6 +49,7 @@ struct mux_control {
* @mux: Array of mux controllers that is handled.
* @dev: Device structure.
* @id: Used to identify the device internally.
+ * @private: The mux chip is implicitly allocated by a single user.
* @ops: Mux controller operations.
*/
struct mux_chip {
@@ -56,6 +57,7 @@ struct mux_chip {
struct mux_control *mux;
struct device dev;
int id;
+ bool private;
const struct mux_control_ops *ops;
};
--
2.1.4
^ permalink raw reply related
* Re: [PATCH] I2c: busses - Fix possible NULL derefrence.
From: Uwe Kleine-König @ 2017-01-30 8:07 UTC (permalink / raw)
To: Thierry Reding
Cc: Shailendra Verma, Laxman Dewangan, Wolfram Sang, Stephen Warren,
Alexandre Courbot, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
p.shailesh-Sze3O3UU22JBDgjK7y7TUQ,
ashish.kalra-Sze3O3UU22JBDgjK7y7TUQ, Shailendra Verma
In-Reply-To: <20170130071217.GG3585-EkSeR96xj6Pcmrwk2tT4+A@public.gmane.org>
Hello,
On Mon, Jan 30, 2017 at 08:12:17AM +0100, Thierry Reding wrote:
> On Mon, Jan 30, 2017 at 10:33:07AM +0530, Shailendra Verma wrote:
> > of_device_get_match_data could return NULL, and so can cause
> > a NULL pointer dereference later.
> >
> > Signed-off-by: Shailendra Verma <shailendra.v-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > ---
> > drivers/i2c/busses/i2c-tegra.c | 4 ++++
> > 1 file changed, 4 insertions(+)
>
> This will never happen. Any match in the OF table that would cause the
> ->probe() to occur has a valid .data pointer associated with it.
Theoretically you could (I think) bind that driver to a node with
compatible = "tegra-i2c";
Anyhow, even if today there was no possibility this could happen, that's
something that might easily be changed by a future change. So I doubt
"this will never happen" stays true for sure and being defensive is a
good idea. And even a BUG would be better than a silent NULL pointer
dereference.
Just my € 0.02
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH] I2c: busses - Fix possible NULL derefrence.
From: Thierry Reding @ 2017-01-30 8:54 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Shailendra Verma, Laxman Dewangan, Wolfram Sang, Stephen Warren,
Alexandre Courbot, linux-i2c, linux-tegra, linux-kernel,
p.shailesh, ashish.kalra, Shailendra Verma
In-Reply-To: <20170130080715.wuvx7ro3kuneolnu@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]
On Mon, Jan 30, 2017 at 09:07:15AM +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Mon, Jan 30, 2017 at 08:12:17AM +0100, Thierry Reding wrote:
> > On Mon, Jan 30, 2017 at 10:33:07AM +0530, Shailendra Verma wrote:
> > > of_device_get_match_data could return NULL, and so can cause
> > > a NULL pointer dereference later.
> > >
> > > Signed-off-by: Shailendra Verma <shailendra.v@samsung.com>
> > > ---
> > > drivers/i2c/busses/i2c-tegra.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> >
> > This will never happen. Any match in the OF table that would cause the
> > ->probe() to occur has a valid .data pointer associated with it.
>
> Theoretically you could (I think) bind that driver to a node with
>
> compatible = "tegra-i2c";
That's not a valid compatible string and I don't think this could end up
anywhere that would make the driver bind. Even if it did I think it'd be
good to crash rather than error out to make it very obvious that you've
made a mistake that needs to be immediately fixed.
If you error out it's much more likely that people won't notice.
> Anyhow, even if today there was no possibility this could happen, that's
> something that might easily be changed by a future change. So I doubt
> "this will never happen" stays true for sure and being defensive is a
> good idea.
Let's revisit this again *if* this ever becomes a real issue. There's no
use in adding dead code to the kernel to handle hypothetical use-cases.
> And even a BUG would be better than a silent NULL pointer dereference.
I've never encountered a NULL pointer dereference that was silent. =)
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] i2c: busses: constify i2c_algorithm structures
From: Patrice CHOTARD @ 2017-01-30 9:41 UTC (permalink / raw)
To: Bhumika Goyal, julia.lawall@lip6.fr, ludovic.desroches@atmel.com,
wsa@the-dreams.de, sonic.zhang@analog.com,
jarkko.nikula@linux.intel.com, andriy.shevchenko@linux.intel.com,
mika.westerberg@linux.intel.com, jdelvare@suse.com,
linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
adi-buildroot-devel@lists.sourceforge.net,
linux-arm-kernel@lists.infradead.org, kernel@stlinux.com
In-Reply-To: <1485540377-13686-1-git-send-email-bhumirks@gmail.com>
On 01/27/2017 07:06 PM, Bhumika Goyal wrote:
> Declare i2c_algorithm structures as const as they are only stored in the
> algo field of an i2c_adapter structure. This field is of type const, so
> i2c_algorithm structures having this property can be made const too.
> Done using Coccinelle:
>
> @r disable optional_qualifier@
> identifier i;
> position p;
> @@
> static struct i2c_algorithm i@p={...};
>
> @ok@
> identifier r.i,x;
> position p;
> struct i2c_adapter adapter;
> struct xlr_i2c_private priv;
> @@
> (
> adapter.algo=&i@p;
> |
> priv.adap.algo=&i@p;
> )
>
> @bad@
> position p!={r.p,ok.p};
> identifier r.i;
> @@
> i@p
>
> @depends on !bad disable optional_qualifier@
> identifier r.i;
> @@
> +const
> struct i2c_algorithm i;
>
> File size details before and after patching.
> First line of every .o file shows the file size before patching
> and second line shows the size after patching.
>
> text data bss dec hex filename
>
> 9138 1328 8 10474 28ea drivers/i2c/busses/i2c-at91.o
> 9178 1280 8 10466 28e2 drivers/i2c/busses/i2c-at91.o
>
> 7555 657 8 8220 201c i2c/busses/i2c-designware-core.o
> 7619 617 8 8244 2034 i2c/busses/i2c-designware-core.o
>
> 7240 2240 120 9600 2580 drivers/i2c/busses/i2c-eg20t.o
> 7304 2176 120 9600 2580 drivers/i2c/busses/i2c-eg20t.o
>
> 2399 248 8 2655 a5f drivers/i2c/busses/i2c-emev2.o
> 2455 192 8 2655 a5f drivers/i2c/busses/i2c-emev2.o
>
> 3766 688 8 4462 116e i2c/busses/i2c-imx-lpi2c.o
> 3814 632 8 4454 1166 i2c/busses/i2c-imx-lpi2c.o
>
> 11793 4280 16 16089 3ed9 drivers/i2c/busses/i2c-imx.o
> 11857 4216 16 16089 3ed9 drivers/i2c/busses/i2c-imx.o
>
> 4046 656 8 4710 1266 drivers/i2c/busses/i2c-nforce2.o
> 4110 592 8 4710 1266 drivers/i2c/busses/i2c-nforce2.o
>
> 1397 464 0 1861 745 i2c/busses/i2c-robotfuzz-osif.o
> 1437 408 0 1845 735 i2c/busses/i2c-robotfuzz-osif.o
>
> 9622 1216 16 10854 2a66 i2c/busses/i2c-sh_mobile.o
> 9662 1160 16 10838 2a56 i2c/busses/i2c-sh_mobile.o
>
> 4936 576 8 5520 1590 drivers/i2c/busses/i2c-st.o
> 5000 512 8 5520 1590 drivers/i2c/busses/i2c-st.o
>
> 3404 248 8 3660 e4c i2c/busses/i2c-xgene-slimpro.o
> 3460 192 8 3660 e4c i2c/busses/i2c-xgene-slimpro.o
>
> 2741 600 8 3349 d15 drivers/i2c/busses/i2c-xlp9xx.o
> 2797 544 8 3349 d15 drivers/i2c/busses/i2c-xlp9xx.o
>
> 2643 248 8 2899 b53 drivers/i2c/busses/i2c-xlr.o
> 2707 192 8 2907 b5b drivers/i2c/busses/i2c-xlr.o
>
> Signed-off-by: Bhumika Goyal <bhumirks@gmail.com>
> ---
> drivers/i2c/busses/i2c-at91.c | 2 +-
> drivers/i2c/busses/i2c-bfin-twi.c | 2 +-
> drivers/i2c/busses/i2c-designware-core.c | 2 +-
> drivers/i2c/busses/i2c-eg20t.c | 2 +-
> drivers/i2c/busses/i2c-emev2.c | 2 +-
> drivers/i2c/busses/i2c-imx-lpi2c.c | 2 +-
> drivers/i2c/busses/i2c-imx.c | 2 +-
> drivers/i2c/busses/i2c-nforce2.c | 2 +-
> drivers/i2c/busses/i2c-robotfuzz-osif.c | 2 +-
> drivers/i2c/busses/i2c-sh_mobile.c | 2 +-
> drivers/i2c/busses/i2c-st.c | 2 +-
> drivers/i2c/busses/i2c-xgene-slimpro.c | 2 +-
> drivers/i2c/busses/i2c-xlp9xx.c | 2 +-
> drivers/i2c/busses/i2c-xlr.c | 2 +-
> 14 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 0b86c61..e4fcb0c 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -820,7 +820,7 @@ static u32 at91_twi_func(struct i2c_adapter *adapter)
> | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> }
>
> -static struct i2c_algorithm at91_twi_algorithm = {
> +static const struct i2c_algorithm at91_twi_algorithm = {
> .master_xfer = at91_twi_xfer,
> .functionality = at91_twi_func,
> };
> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c
> index 29d00c4..9fe942b 100644
> --- a/drivers/i2c/busses/i2c-bfin-twi.c
> +++ b/drivers/i2c/busses/i2c-bfin-twi.c
> @@ -563,7 +563,7 @@ static u32 bfin_twi_functionality(struct i2c_adapter *adap)
> I2C_FUNC_I2C | I2C_FUNC_SMBUS_I2C_BLOCK;
> }
>
> -static struct i2c_algorithm bfin_twi_algorithm = {
> +static const struct i2c_algorithm bfin_twi_algorithm = {
> .master_xfer = bfin_twi_master_xfer,
> .smbus_xfer = bfin_twi_smbus_xfer,
> .functionality = bfin_twi_functionality,
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 6d81c56..a62c14c 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -822,7 +822,7 @@ static u32 i2c_dw_func(struct i2c_adapter *adap)
> return dev->functionality;
> }
>
> -static struct i2c_algorithm i2c_dw_algo = {
> +static const struct i2c_algorithm i2c_dw_algo = {
> .master_xfer = i2c_dw_xfer,
> .functionality = i2c_dw_func,
> };
> diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
> index 5ce71ce..bdeab01 100644
> --- a/drivers/i2c/busses/i2c-eg20t.c
> +++ b/drivers/i2c/busses/i2c-eg20t.c
> @@ -715,7 +715,7 @@ static u32 pch_i2c_func(struct i2c_adapter *adap)
> return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_10BIT_ADDR;
> }
>
> -static struct i2c_algorithm pch_algorithm = {
> +static const struct i2c_algorithm pch_algorithm = {
> .master_xfer = pch_i2c_xfer,
> .functionality = pch_i2c_func
> };
> diff --git a/drivers/i2c/busses/i2c-emev2.c b/drivers/i2c/busses/i2c-emev2.c
> index 96bb4e7..3129127 100644
> --- a/drivers/i2c/busses/i2c-emev2.c
> +++ b/drivers/i2c/busses/i2c-emev2.c
> @@ -347,7 +347,7 @@ static int em_i2c_unreg_slave(struct i2c_client *slave)
> return 0;
> }
>
> -static struct i2c_algorithm em_i2c_algo = {
> +static const struct i2c_algorithm em_i2c_algo = {
> .master_xfer = em_i2c_xfer,
> .functionality = em_i2c_func,
> .reg_slave = em_i2c_reg_slave,
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index c62b7cd..934f548 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -537,7 +537,7 @@ static u32 lpi2c_imx_func(struct i2c_adapter *adapter)
> I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> }
>
> -static struct i2c_algorithm lpi2c_imx_algo = {
> +static const struct i2c_algorithm lpi2c_imx_algo = {
> .master_xfer = lpi2c_imx_xfer,
> .functionality = lpi2c_imx_func,
> };
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 47fc1f1..95ed171 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -1037,7 +1037,7 @@ static u32 i2c_imx_func(struct i2c_adapter *adapter)
> | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
> }
>
> -static struct i2c_algorithm i2c_imx_algo = {
> +static const struct i2c_algorithm i2c_imx_algo = {
> .master_xfer = i2c_imx_xfer,
> .functionality = i2c_imx_func,
> };
> diff --git a/drivers/i2c/busses/i2c-nforce2.c b/drivers/i2c/busses/i2c-nforce2.c
> index 374b35e..3241bb9 100644
> --- a/drivers/i2c/busses/i2c-nforce2.c
> +++ b/drivers/i2c/busses/i2c-nforce2.c
> @@ -296,7 +296,7 @@ static u32 nforce2_func(struct i2c_adapter *adapter)
> I2C_FUNC_SMBUS_BLOCK_DATA : 0);
> }
>
> -static struct i2c_algorithm smbus_algorithm = {
> +static const struct i2c_algorithm smbus_algorithm = {
> .smbus_xfer = nforce2_access,
> .functionality = nforce2_func,
> };
> diff --git a/drivers/i2c/busses/i2c-robotfuzz-osif.c b/drivers/i2c/busses/i2c-robotfuzz-osif.c
> index 89d8b41..9c0f52b 100644
> --- a/drivers/i2c/busses/i2c-robotfuzz-osif.c
> +++ b/drivers/i2c/busses/i2c-robotfuzz-osif.c
> @@ -117,7 +117,7 @@ static u32 osif_func(struct i2c_adapter *adapter)
> return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> }
>
> -static struct i2c_algorithm osif_algorithm = {
> +static const struct i2c_algorithm osif_algorithm = {
> .master_xfer = osif_xfer,
> .functionality = osif_func,
> };
> diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c
> index 3d9ebe6..3d75593 100644
> --- a/drivers/i2c/busses/i2c-sh_mobile.c
> +++ b/drivers/i2c/busses/i2c-sh_mobile.c
> @@ -781,7 +781,7 @@ static u32 sh_mobile_i2c_func(struct i2c_adapter *adapter)
> return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_PROTOCOL_MANGLING;
> }
>
> -static struct i2c_algorithm sh_mobile_i2c_algorithm = {
> +static const struct i2c_algorithm sh_mobile_i2c_algorithm = {
> .functionality = sh_mobile_i2c_func,
> .master_xfer = sh_mobile_i2c_xfer,
> };
> diff --git a/drivers/i2c/busses/i2c-st.c b/drivers/i2c/busses/i2c-st.c
> index 1371547..1eb9fa8 100644
> --- a/drivers/i2c/busses/i2c-st.c
> +++ b/drivers/i2c/busses/i2c-st.c
> @@ -776,7 +776,7 @@ static u32 st_i2c_func(struct i2c_adapter *adap)
> return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> }
>
> -static struct i2c_algorithm st_i2c_algo = {
> +static const struct i2c_algorithm st_i2c_algo = {
> .master_xfer = st_i2c_xfer,
> .functionality = st_i2c_func,
> };
> diff --git a/drivers/i2c/busses/i2c-xgene-slimpro.c b/drivers/i2c/busses/i2c-xgene-slimpro.c
> index 0ab1e55..dbe7e44 100644
> --- a/drivers/i2c/busses/i2c-xgene-slimpro.c
> +++ b/drivers/i2c/busses/i2c-xgene-slimpro.c
> @@ -372,7 +372,7 @@ static u32 xgene_slimpro_i2c_func(struct i2c_adapter *adapter)
> I2C_FUNC_SMBUS_I2C_BLOCK;
> }
>
> -static struct i2c_algorithm xgene_slimpro_i2c_algorithm = {
> +static const struct i2c_algorithm xgene_slimpro_i2c_algorithm = {
> .smbus_xfer = xgene_slimpro_i2c_xfer,
> .functionality = xgene_slimpro_i2c_func,
> };
> diff --git a/drivers/i2c/busses/i2c-xlp9xx.c b/drivers/i2c/busses/i2c-xlp9xx.c
> index 84a8b2e..66b464d 100644
> --- a/drivers/i2c/busses/i2c-xlp9xx.c
> +++ b/drivers/i2c/busses/i2c-xlp9xx.c
> @@ -334,7 +334,7 @@ static u32 xlp9xx_i2c_functionality(struct i2c_adapter *adapter)
> I2C_FUNC_10BIT_ADDR;
> }
>
> -static struct i2c_algorithm xlp9xx_i2c_algo = {
> +static const struct i2c_algorithm xlp9xx_i2c_algo = {
> .master_xfer = xlp9xx_i2c_xfer,
> .functionality = xlp9xx_i2c_functionality,
> };
> diff --git a/drivers/i2c/busses/i2c-xlr.c b/drivers/i2c/busses/i2c-xlr.c
> index ad17d88..484bfa1 100644
> --- a/drivers/i2c/busses/i2c-xlr.c
> +++ b/drivers/i2c/busses/i2c-xlr.c
> @@ -335,7 +335,7 @@ static u32 xlr_func(struct i2c_adapter *adap)
> return (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) | I2C_FUNC_I2C;
> }
>
> -static struct i2c_algorithm xlr_i2c_algo = {
> +static const struct i2c_algorithm xlr_i2c_algo = {
> .master_xfer = xlr_i2c_xfer,
> .functionality = xlr_func,
> };
>
Hi
for i2c-st:
Acked-by: Patrice Chotard <patrice.chotard@st.com>
Thanks
^ permalink raw reply
* Re: [PATCH v3 3/3] spi: acpi: Initialize modalias from of_compatible
From: Rafael J. Wysocki @ 2017-01-30 9:41 UTC (permalink / raw)
To: Dan O'Donovan, Mark Brown
Cc: ACPI Devel Maling List, Rafael J . Wysocki, Jarkko Nikula,
Mika Westerberg, Len Brown, linux-i2c, Wolfram Sang, linux-spi,
Linux Kernel Mailing List, Crestez Dan Leonard
In-Reply-To: <1485187737-22414-4-git-send-email-dan@emutex.com>
On Mon, Jan 23, 2017 at 5:08 PM, Dan O'Donovan <dan@emutex.com> wrote:
> From: Crestez Dan Leonard <leonard.crestez@intel.com>
>
> When using devicetree spi_device.modalias is set to the compatible
> string with the vendor prefix removed. For SPI devices described via
> ACPI the spi_device.modalias string is initialized by acpi_device_hid.
> When using ACPI and DT ids this string ends up something like "PRP0001".
>
> Change acpi_register_spi_device to use the of_compatible property if
> present. This makes it easier to instantiate spi drivers through ACPI
> with DT ids.
>
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> Signed-off-by: Dan O'Donovan <dan@emutex.com>
> ---
> drivers/spi/spi.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 656dd3e..4b562e8 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1722,13 +1722,21 @@ static acpi_status acpi_register_spi_device(struct spi_master *master,
> return AE_OK;
> }
>
> + /*
> + * Populate modalias from compatible property if available,
> + * otherwise use native ACPI information
> + */
> + if ((!adev->data.of_compatible) ||
> + acpi_of_modalias(adev, spi->modalias, sizeof(spi->modalias)))
> + strlcpy(spi->modalias, acpi_device_hid(adev),
> + sizeof(spi->modalias));
> +
> if (spi->irq < 0)
> spi->irq = acpi_dev_gpio_irq_get(adev, 0);
>
> acpi_device_set_enumerated(adev);
>
> adev->power.flags.ignore_parent = true;
> - strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias));
> if (spi_add_device(spi)) {
> adev->power.flags.ignore_parent = false;
> dev_err(&master->dev, "failed to add SPI device %s from ACPI\n",
> --
Mark, any objections here?
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH] i2c: busses: constify i2c_algorithm structures
From: Jarkko Nikula @ 2017-01-30 11:15 UTC (permalink / raw)
To: Bhumika Goyal, julia.lawall, ludovic.desroches, wsa, sonic.zhang,
andriy.shevchenko, mika.westerberg, jdelvare, patrice.chotard,
linux-i2c, linux-kernel, adi-buildroot-devel, linux-arm-kernel,
kernel
In-Reply-To: <1485540377-13686-1-git-send-email-bhumirks@gmail.com>
On 01/27/2017 08:06 PM, Bhumika Goyal wrote:
> Declare i2c_algorithm structures as const as they are only stored in the
> algo field of an i2c_adapter structure. This field is of type const, so
> i2c_algorithm structures having this property can be made const too.
> Done using Coccinelle:
>
...
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 6d81c56..a62c14c 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -822,7 +822,7 @@ static u32 i2c_dw_func(struct i2c_adapter *adap)
> return dev->functionality;
> }
>
> -static struct i2c_algorithm i2c_dw_algo = {
> +static const struct i2c_algorithm i2c_dw_algo = {
> .master_xfer = i2c_dw_xfer,
> .functionality = i2c_dw_func,
> };
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
^ permalink raw reply
* Re: [PATCH] I2c: busses - Fix possible NULL derefrence.
From: Uwe Kleine-König @ 2017-01-30 11:15 UTC (permalink / raw)
To: Thierry Reding
Cc: Shailendra Verma, Laxman Dewangan, Wolfram Sang, Stephen Warren,
Alexandre Courbot, linux-i2c, linux-tegra, linux-kernel,
p.shailesh, ashish.kalra, Shailendra Verma
In-Reply-To: <20170130085455.GP3585@ulmo.ba.sec>
On Mon, Jan 30, 2017 at 09:54:55AM +0100, Thierry Reding wrote:
> On Mon, Jan 30, 2017 at 09:07:15AM +0100, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Mon, Jan 30, 2017 at 08:12:17AM +0100, Thierry Reding wrote:
> > > On Mon, Jan 30, 2017 at 10:33:07AM +0530, Shailendra Verma wrote:
> > > > of_device_get_match_data could return NULL, and so can cause
> > > > a NULL pointer dereference later.
> > > >
> > > > Signed-off-by: Shailendra Verma <shailendra.v@samsung.com>
> > > > ---
> > > > drivers/i2c/busses/i2c-tegra.c | 4 ++++
> > > > 1 file changed, 4 insertions(+)
> > >
> > > This will never happen. Any match in the OF table that would cause the
> > > ->probe() to occur has a valid .data pointer associated with it.
> >
> > Theoretically you could (I think) bind that driver to a node with
> >
> > compatible = "tegra-i2c";
>
> That's not a valid compatible string and I don't think this could end up
> anywhere that would make the driver bind. Even if it did I think it'd be
Look at platform_match() in drivers/base/platform.c. If
of_driver_match_device fails it might still match based on
strcmp(pdev->name, drv->name).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH] I2c: busses - Fix possible NULL derefrence.
From: Thierry Reding @ 2017-01-30 11:24 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Shailendra Verma, Laxman Dewangan, Wolfram Sang, Stephen Warren,
Alexandre Courbot, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
p.shailesh-Sze3O3UU22JBDgjK7y7TUQ,
ashish.kalra-Sze3O3UU22JBDgjK7y7TUQ, Shailendra Verma
In-Reply-To: <20170130111553.cbwxu23lzd46qtt7-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1656 bytes --]
On Mon, Jan 30, 2017 at 12:15:53PM +0100, Uwe Kleine-König wrote:
> On Mon, Jan 30, 2017 at 09:54:55AM +0100, Thierry Reding wrote:
> > On Mon, Jan 30, 2017 at 09:07:15AM +0100, Uwe Kleine-König wrote:
> > > Hello,
> > >
> > > On Mon, Jan 30, 2017 at 08:12:17AM +0100, Thierry Reding wrote:
> > > > On Mon, Jan 30, 2017 at 10:33:07AM +0530, Shailendra Verma wrote:
> > > > > of_device_get_match_data could return NULL, and so can cause
> > > > > a NULL pointer dereference later.
> > > > >
> > > > > Signed-off-by: Shailendra Verma <shailendra.v-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > > > > ---
> > > > > drivers/i2c/busses/i2c-tegra.c | 4 ++++
> > > > > 1 file changed, 4 insertions(+)
> > > >
> > > > This will never happen. Any match in the OF table that would cause the
> > > > ->probe() to occur has a valid .data pointer associated with it.
> > >
> > > Theoretically you could (I think) bind that driver to a node with
> > >
> > > compatible = "tegra-i2c";
> >
> > That's not a valid compatible string and I don't think this could end up
> > anywhere that would make the driver bind. Even if it did I think it'd be
>
> Look at platform_match() in drivers/base/platform.c. If
> of_driver_match_device fails it might still match based on
> strcmp(pdev->name, drv->name).
pdev->name is never influenced by the compatible string. The only way
you could create a device that would match this driver is if you were to
manually create it using of_platform_device_create() or similar,
something which we can easily prevent (or revert should anyone ever get
such code into the kernel again).
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings
From: Ville Syrjälä @ 2017-01-30 13:10 UTC (permalink / raw)
To: Hans de Goede
Cc: Wolfram Sang, Takashi Iwai, russianneuromancer @ ya . ru,
intel-gfx, linux-i2c, Jarkko Nikula, dri-devel, H . Peter Anvin,
Daniel Vetter, Thomas Gleixner, Andy Shevchenko, Mika Westerberg,
Len Brown
In-Reply-To: <2eea197f-2485-5f93-1691-42a9a2e11267@redhat.com>
On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
> Hi,
>
> On 01/28/2017 05:25 PM, Hans de Goede wrote:
> > Hi,
> >
> > On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
> >> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
> >>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
> >>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
> >>> around P-Unit write accesses.
> >>
> >> Can't we just stuff the calls into the actual punit write function
> >> rather than sprinkling them all over the place?
> >
> > punit access is acquired across sections like this:
> >
> > iosf_mbi_punit_acquire();
> >
> > val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> > val &= ~DSPFREQGUAR_MASK;
> > val |= (cmd << DSPFREQGUAR_SHIFT);
> > vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> > if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
> > DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
> > 50)) {
> > DRM_ERROR("timed out waiting for CDclk change\n");
> > }
> > iosf_mbi_punit_release();
> >
> > Where we want to wait for the requested change to have taken
> > effect before releasing the punit.
Hmm. That's somewhat unfortunate. It also highlights a problem with the
patch wrt. RPS. We don't wait for the GPU to actually change frequencies
in set_rps() because that would slow things down too much. So I have to
wonder how much luck is needed to make this workaround really effective.
> >
> >>
> >> + a comment would be nice why it's there.
> >
> > I will add comments to the acquire calls.
> >
> >> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
> >> ifdefs?
> >
> > No, the iosf_mbi header defines empty inline versions of
> > iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
> > this does mean that iosf_mbi must be builtin if the i915
> > driver is. I'll add:
> >
> > depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
> >
> > To the i915 Kconfig to enforce this.
>
> Hmm, ok so that does not work (long cyclic dependency through the
> selection of ACPI_VIDEO).
>
> So I've now added this instead:
>
> # iosf_mbi needs to be builtin if we are builtin
> select IOSF_MBI if DRM_I915=y
That's probably not going to help anyone since i915 is usually a module.
>
> 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>
> >>> ---
> >>> Changes in v2:
> >>> -Spelling: P-Unit, PMIC
> >>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
> >>> ---
> >>> 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 5604701..13e5152 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)
> >>> {
> >>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> >>> cmd = 0;
> >>>
> >>> mutex_lock(&dev_priv->rps.hw_lock);
> >>> + iosf_mbi_punit_acquire();
> >>> +
> >>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>> val &= ~DSPFREQGUAR_MASK;
> >>> val |= (cmd << DSPFREQGUAR_SHIFT);
> >>> @@ -6430,6 +6433,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_release();
> >>> mutex_unlock(&dev_priv->rps.hw_lock);
> >>>
> >>> mutex_lock(&dev_priv->sb_lock);
> >>> @@ -6497,6 +6501,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_acquire();
> >>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>> val &= ~DSPFREQGUAR_MASK_CHV;
> >>> val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
> >>> @@ -6506,6 +6511,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_release();
> >>> 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 249623d..adff84a 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_acquire();
> >>>
> >>> 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_release();
> >>> 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_acquire();
> >>>
> >>> 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_release();
> >>> mutex_unlock(&dev_priv->rps.hw_lock);
> >>> }
> >>>
> >>> @@ -4553,6 +4558,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_acquire();
> >>>
> >>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>> if (val & DSP_MAXFIFO_PM5_ENABLE)
> >>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> >>> wm->level = VLV_WM_LEVEL_DDR_DVFS;
> >>> }
> >>>
> >>> + iosf_mbi_punit_release();
> >>> mutex_unlock(&dev_priv->rps.hw_lock);
> >>> }
> >>>
> >>> @@ -4988,7 +4995,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_acquire();
> >>> vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
> >>> + iosf_mbi_punit_release();
> >>> 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..e66bcc8 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_acquire();
> >>> +
> >>> 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_release();
> >>> +
> >>> #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_acquire();
> >>> +
> >>> 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_release();
> >>> +
> >>> #undef COND
> >>>
> >>> out:
> >>> --
> >>> 2.9.3
> >>
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings
From: Hans de Goede @ 2017-01-30 15:02 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Daniel Vetter, Jani Nikula, Jarkko Nikula, Wolfram Sang,
Len Brown, Andy Shevchenko, Thomas Gleixner, H . Peter Anvin,
intel-gfx, dri-devel, Mika Westerberg, Takashi Iwai,
russianneuromancer @ ya . ru, linux-i2c
In-Reply-To: <20170130131041.GM31595@intel.com>
Hi,
On 30-01-17 14:10, Ville Syrjälä wrote:
> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>>>> around P-Unit write accesses.
>>>>
>>>> Can't we just stuff the calls into the actual punit write function
>>>> rather than sprinkling them all over the place?
>>>
>>> punit access is acquired across sections like this:
>>>
>>> iosf_mbi_punit_acquire();
>>>
>>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>> val &= ~DSPFREQGUAR_MASK;
>>> val |= (cmd << DSPFREQGUAR_SHIFT);
>>> vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>> if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>>> DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>>> 50)) {
>>> DRM_ERROR("timed out waiting for CDclk change\n");
>>> }
>>> iosf_mbi_punit_release();
>>>
>>> Where we want to wait for the requested change to have taken
>>> effect before releasing the punit.
>
> Hmm. That's somewhat unfortunate. It also highlights a problem with the
> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
> in set_rps() because that would slow things down too much. So I have to
> wonder how much luck is needed to make this workaround really effective.
So the history of this patch-set is that I wrote this patch before
writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
active (patch 12/13). Since a lot of testing was done with this
patch included in the patch-set and since it seemed a good idea
regardless (given my experience with accessing the punit vs
pmic bus accesses) I decided to leave it in.
Possibly just the patch to get FORCEWAKE_ALL is enough, that one
actually fixed things for me. That is also why I made this the
last patch in the set. I asked tagorereddy to test his system
without this patch, but he did not get around to that.
After all we do tell the punit to not touch the bus by acquiring
the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
for RPS freq changes it honors that and properly waits. Maybe it
honors that for all punit requests i915 does and the only real
problem is the forcewake stuff ?
I can try to drop this patch from my queue and run without it
for a while and see if things don't regress. And also ask
tagorereddy again to test his system that way.
Does that (dropping this patch for now) sound like a good idea?
>>>> + a comment would be nice why it's there.
>>>
>>> I will add comments to the acquire calls.
>>>
>>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
>>>> ifdefs?
>>>
>>> No, the iosf_mbi header defines empty inline versions of
>>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
>>> this does mean that iosf_mbi must be builtin if the i915
>>> driver is. I'll add:
>>>
>>> depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
>>>
>>> To the i915 Kconfig to enforce this.
>>
>> Hmm, ok so that does not work (long cyclic dependency through the
>> selection of ACPI_VIDEO).
>>
>> So I've now added this instead:
>>
>> # iosf_mbi needs to be builtin if we are builtin
>> select IOSF_MBI if DRM_I915=y
>
> That's probably not going to help anyone since i915 is usually a module.
Right, that is fine, then either the IOSF_MBI symbols are available,
or IOSF_MBI is disabled and we get the inline nops from the header.
The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
realistic IMHO, but will get triggered by the random-config testing
several contributors do and lead to an unresolved symbol error there.
Hmm, thinking about this, this hunk actually belongs in 12/13 as that
is the first patch to use iosf_mbi functions.
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>
>>>>> ---
>>>>> Changes in v2:
>>>>> -Spelling: P-Unit, PMIC
>>>>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
>>>>> ---
>>>>> 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 5604701..13e5152 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)
>>>>> {
>>>>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>> cmd = 0;
>>>>>
>>>>> mutex_lock(&dev_priv->rps.hw_lock);
>>>>> + iosf_mbi_punit_acquire();
>>>>> +
>>>>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>> val &= ~DSPFREQGUAR_MASK;
>>>>> val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>> @@ -6430,6 +6433,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_release();
>>>>> mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>
>>>>> mutex_lock(&dev_priv->sb_lock);
>>>>> @@ -6497,6 +6501,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_acquire();
>>>>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>> val &= ~DSPFREQGUAR_MASK_CHV;
>>>>> val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
>>>>> @@ -6506,6 +6511,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_release();
>>>>> 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 249623d..adff84a 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_acquire();
>>>>>
>>>>> 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_release();
>>>>> 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_acquire();
>>>>>
>>>>> 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_release();
>>>>> mutex_unlock(&dev_priv->rps.hw_lock);
>>>>> }
>>>>>
>>>>> @@ -4553,6 +4558,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_acquire();
>>>>>
>>>>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>> if (val & DSP_MAXFIFO_PM5_ENABLE)
>>>>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>>> wm->level = VLV_WM_LEVEL_DDR_DVFS;
>>>>> }
>>>>>
>>>>> + iosf_mbi_punit_release();
>>>>> mutex_unlock(&dev_priv->rps.hw_lock);
>>>>> }
>>>>>
>>>>> @@ -4988,7 +4995,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_acquire();
>>>>> vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
>>>>> + iosf_mbi_punit_release();
>>>>> 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..e66bcc8 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_acquire();
>>>>> +
>>>>> 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_release();
>>>>> +
>>>>> #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_acquire();
>>>>> +
>>>>> 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_release();
>>>>> +
>>>>> #undef COND
>>>>>
>>>>> out:
>>>>> --
>>>>> 2.9.3
>>>>
>
^ permalink raw reply
* Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings
From: Ville Syrjälä @ 2017-01-30 15:11 UTC (permalink / raw)
To: Hans de Goede
Cc: Wolfram Sang, Takashi Iwai, russianneuromancer @ ya . ru,
intel-gfx, linux-i2c, Jarkko Nikula, dri-devel, H . Peter Anvin,
Daniel Vetter, Thomas Gleixner, Andy Shevchenko, Mika Westerberg,
Len Brown
In-Reply-To: <f3585fe1-9bc3-08a0-f747-ee0336f97965@redhat.com>
On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
> Hi,
>
> On 30-01-17 14:10, Ville Syrjälä wrote:
> > On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 01/28/2017 05:25 PM, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
> >>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
> >>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
> >>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
> >>>>> around P-Unit write accesses.
> >>>>
> >>>> Can't we just stuff the calls into the actual punit write function
> >>>> rather than sprinkling them all over the place?
> >>>
> >>> punit access is acquired across sections like this:
> >>>
> >>> iosf_mbi_punit_acquire();
> >>>
> >>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>> val &= ~DSPFREQGUAR_MASK;
> >>> val |= (cmd << DSPFREQGUAR_SHIFT);
> >>> vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> >>> if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
> >>> DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
> >>> 50)) {
> >>> DRM_ERROR("timed out waiting for CDclk change\n");
> >>> }
> >>> iosf_mbi_punit_release();
> >>>
> >>> Where we want to wait for the requested change to have taken
> >>> effect before releasing the punit.
> >
> > Hmm. That's somewhat unfortunate. It also highlights a problem with the
> > patch wrt. RPS. We don't wait for the GPU to actually change frequencies
> > in set_rps() because that would slow things down too much. So I have to
> > wonder how much luck is needed to make this workaround really effective.
>
> So the history of this patch-set is that I wrote this patch before
> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
> active (patch 12/13). Since a lot of testing was done with this
> patch included in the patch-set and since it seemed a good idea
> regardless (given my experience with accessing the punit vs
> pmic bus accesses) I decided to leave it in.
>
> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
> actually fixed things for me. That is also why I made this the
> last patch in the set. I asked tagorereddy to test his system
> without this patch, but he did not get around to that.
>
> After all we do tell the punit to not touch the bus by acquiring
> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
> for RPS freq changes it honors that and properly waits. Maybe it
> honors that for all punit requests i915 does and the only real
> problem is the forcewake stuff ?
>
> I can try to drop this patch from my queue and run without it
> for a while and see if things don't regress. And also ask
> tagorereddy again to test his system that way.
>
> Does that (dropping this patch for now) sound like a good idea?
More test results couldn't hurt at least. It also makes me wonder if
just bumping the timeouts to some ridiculously high value would fix
the problem as well.
>
> >>>> + a comment would be nice why it's there.
> >>>
> >>> I will add comments to the acquire calls.
> >>>
> >>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
> >>>> ifdefs?
> >>>
> >>> No, the iosf_mbi header defines empty inline versions of
> >>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
> >>> this does mean that iosf_mbi must be builtin if the i915
> >>> driver is. I'll add:
> >>>
> >>> depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
> >>>
> >>> To the i915 Kconfig to enforce this.
> >>
> >> Hmm, ok so that does not work (long cyclic dependency through the
> >> selection of ACPI_VIDEO).
> >>
> >> So I've now added this instead:
> >>
> >> # iosf_mbi needs to be builtin if we are builtin
> >> select IOSF_MBI if DRM_I915=y
> >
> > That's probably not going to help anyone since i915 is usually a module.
>
> Right, that is fine, then either the IOSF_MBI symbols are available,
> or IOSF_MBI is disabled and we get the inline nops from the header.
>
> The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
> realistic IMHO, but will get triggered by the random-config testing
> several contributors do and lead to an unresolved symbol error there.
Well, from the user POV anything with IOSF_MBI==n can be a problem.
So I'm not sure if we should allow that.
>
> Hmm, thinking about this, this hunk actually belongs in 12/13 as that
> is the first patch to use iosf_mbi functions.
>
> 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>
> >>>>> ---
> >>>>> Changes in v2:
> >>>>> -Spelling: P-Unit, PMIC
> >>>>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
> >>>>> ---
> >>>>> 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 5604701..13e5152 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)
> >>>>> {
> >>>>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>>> cmd = 0;
> >>>>>
> >>>>> mutex_lock(&dev_priv->rps.hw_lock);
> >>>>> + iosf_mbi_punit_acquire();
> >>>>> +
> >>>>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>> val &= ~DSPFREQGUAR_MASK;
> >>>>> val |= (cmd << DSPFREQGUAR_SHIFT);
> >>>>> @@ -6430,6 +6433,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_release();
> >>>>> mutex_unlock(&dev_priv->rps.hw_lock);
> >>>>>
> >>>>> mutex_lock(&dev_priv->sb_lock);
> >>>>> @@ -6497,6 +6501,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_acquire();
> >>>>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>> val &= ~DSPFREQGUAR_MASK_CHV;
> >>>>> val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
> >>>>> @@ -6506,6 +6511,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_release();
> >>>>> 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 249623d..adff84a 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_acquire();
> >>>>>
> >>>>> 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_release();
> >>>>> 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_acquire();
> >>>>>
> >>>>> 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_release();
> >>>>> mutex_unlock(&dev_priv->rps.hw_lock);
> >>>>> }
> >>>>>
> >>>>> @@ -4553,6 +4558,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_acquire();
> >>>>>
> >>>>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>> if (val & DSP_MAXFIFO_PM5_ENABLE)
> >>>>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> >>>>> wm->level = VLV_WM_LEVEL_DDR_DVFS;
> >>>>> }
> >>>>>
> >>>>> + iosf_mbi_punit_release();
> >>>>> mutex_unlock(&dev_priv->rps.hw_lock);
> >>>>> }
> >>>>>
> >>>>> @@ -4988,7 +4995,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_acquire();
> >>>>> vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
> >>>>> + iosf_mbi_punit_release();
> >>>>> 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..e66bcc8 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_acquire();
> >>>>> +
> >>>>> 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_release();
> >>>>> +
> >>>>> #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_acquire();
> >>>>> +
> >>>>> 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_release();
> >>>>> +
> >>>>> #undef COND
> >>>>>
> >>>>> out:
> >>>>> --
> >>>>> 2.9.3
> >>>>
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings
From: Hans de Goede @ 2017-01-30 15:27 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Wolfram Sang, russianneuromancer @ ya . ru, intel-gfx, linux-i2c,
Jarkko Nikula, dri-devel, H . Peter Anvin, Daniel Vetter,
Thomas Gleixner, Andy Shevchenko, Mika Westerberg, Len Brown
In-Reply-To: <20170130151144.GP31595@intel.com>
Hi,
On 30-01-17 16:11, Ville Syrjälä wrote:
> On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 30-01-17 14:10, Ville Syrjälä wrote:
>>> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>>>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>>>>>> around P-Unit write accesses.
>>>>>>
>>>>>> Can't we just stuff the calls into the actual punit write function
>>>>>> rather than sprinkling them all over the place?
>>>>>
>>>>> punit access is acquired across sections like this:
>>>>>
>>>>> iosf_mbi_punit_acquire();
>>>>>
>>>>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>> val &= ~DSPFREQGUAR_MASK;
>>>>> val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>> vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>>> if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>>>>> DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>>>>> 50)) {
>>>>> DRM_ERROR("timed out waiting for CDclk change\n");
>>>>> }
>>>>> iosf_mbi_punit_release();
>>>>>
>>>>> Where we want to wait for the requested change to have taken
>>>>> effect before releasing the punit.
>>>
>>> Hmm. That's somewhat unfortunate. It also highlights a problem with the
>>> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
>>> in set_rps() because that would slow things down too much. So I have to
>>> wonder how much luck is needed to make this workaround really effective.
>>
>> So the history of this patch-set is that I wrote this patch before
>> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
>> active (patch 12/13). Since a lot of testing was done with this
>> patch included in the patch-set and since it seemed a good idea
>> regardless (given my experience with accessing the punit vs
>> pmic bus accesses) I decided to leave it in.
>>
>> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
>> actually fixed things for me. That is also why I made this the
>> last patch in the set. I asked tagorereddy to test his system
>> without this patch, but he did not get around to that.
>>
>> After all we do tell the punit to not touch the bus by acquiring
>> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
>> for RPS freq changes it honors that and properly waits. Maybe it
>> honors that for all punit requests i915 does and the only real
>> problem is the forcewake stuff ?
>>
>> I can try to drop this patch from my queue and run without it
>> for a while and see if things don't regress. And also ask
>> tagorereddy again to test his system that way.
>>
>> Does that (dropping this patch for now) sound like a good idea?
>
> More test results couldn't hurt at least. It also makes me wonder if
> just bumping the timeouts to some ridiculously high value would fix
> the problem as well.
I've already tried bumping the forcewake timeout from 50 to 250ms,
before writing the patch to just get forcewake_all before the pmic
bus access begins, that does not fix things, and since we busy wait
for this timeout from non-sleeping context 250ms already is way too
high.
>>>>>> + a comment would be nice why it's there.
>>>>>
>>>>> I will add comments to the acquire calls.
>>>>>
>>>>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
>>>>>> ifdefs?
>>>>>
>>>>> No, the iosf_mbi header defines empty inline versions of
>>>>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
>>>>> this does mean that iosf_mbi must be builtin if the i915
>>>>> driver is. I'll add:
>>>>>
>>>>> depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
>>>>>
>>>>> To the i915 Kconfig to enforce this.
>>>>
>>>> Hmm, ok so that does not work (long cyclic dependency through the
>>>> selection of ACPI_VIDEO).
>>>>
>>>> So I've now added this instead:
>>>>
>>>> # iosf_mbi needs to be builtin if we are builtin
>>>> select IOSF_MBI if DRM_I915=y
>>>
>>> That's probably not going to help anyone since i915 is usually a module.
>>
>> Right, that is fine, then either the IOSF_MBI symbols are available,
>> or IOSF_MBI is disabled and we get the inline nops from the header.
>>
>> The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
>> realistic IMHO, but will get triggered by the random-config testing
>> several contributors do and lead to an unresolved symbol error there.
>
> Well, from the user POV anything with IOSF_MBI==n can be a problem.
> So I'm not sure if we should allow that.
So you're suggesting we just add an unconditional "select IOSF_MBI"
to the i915 Kconfig entry?
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>
>>>>>>> ---
>>>>>>> Changes in v2:
>>>>>>> -Spelling: P-Unit, PMIC
>>>>>>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
>>>>>>> ---
>>>>>>> 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 5604701..13e5152 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)
>>>>>>> {
>>>>>>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>>> cmd = 0;
>>>>>>>
>>>>>>> mutex_lock(&dev_priv->rps.hw_lock);
>>>>>>> + iosf_mbi_punit_acquire();
>>>>>>> +
>>>>>>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>> val &= ~DSPFREQGUAR_MASK;
>>>>>>> val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>>>> @@ -6430,6 +6433,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_release();
>>>>>>> mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>>>
>>>>>>> mutex_lock(&dev_priv->sb_lock);
>>>>>>> @@ -6497,6 +6501,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_acquire();
>>>>>>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>> val &= ~DSPFREQGUAR_MASK_CHV;
>>>>>>> val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
>>>>>>> @@ -6506,6 +6511,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_release();
>>>>>>> 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 249623d..adff84a 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_acquire();
>>>>>>>
>>>>>>> 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_release();
>>>>>>> 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_acquire();
>>>>>>>
>>>>>>> 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_release();
>>>>>>> mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>>> }
>>>>>>>
>>>>>>> @@ -4553,6 +4558,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_acquire();
>>>>>>>
>>>>>>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>> if (val & DSP_MAXFIFO_PM5_ENABLE)
>>>>>>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>>>>> wm->level = VLV_WM_LEVEL_DDR_DVFS;
>>>>>>> }
>>>>>>>
>>>>>>> + iosf_mbi_punit_release();
>>>>>>> mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>>> }
>>>>>>>
>>>>>>> @@ -4988,7 +4995,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_acquire();
>>>>>>> vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
>>>>>>> + iosf_mbi_punit_release();
>>>>>>> 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..e66bcc8 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_acquire();
>>>>>>> +
>>>>>>> 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_release();
>>>>>>> +
>>>>>>> #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_acquire();
>>>>>>> +
>>>>>>> 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_release();
>>>>>>> +
>>>>>>> #undef COND
>>>>>>>
>>>>>>> out:
>>>>>>> --
>>>>>>> 2.9.3
>>>>>>
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings
From: Ville Syrjälä @ 2017-01-30 15:38 UTC (permalink / raw)
To: Hans de Goede
Cc: Wolfram Sang, Takashi Iwai, russianneuromancer @ ya . ru,
intel-gfx, linux-i2c, Jarkko Nikula, dri-devel, H . Peter Anvin,
Daniel Vetter, Thomas Gleixner, Andy Shevchenko, Mika Westerberg,
Len Brown
In-Reply-To: <dd41cb48-dfa1-e3d9-7850-7886f029944e@redhat.com>
On Mon, Jan 30, 2017 at 04:27:58PM +0100, Hans de Goede wrote:
> Hi,
>
> On 30-01-17 16:11, Ville Syrjälä wrote:
> > On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 30-01-17 14:10, Ville Syrjälä wrote:
> >>> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
> >>>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
> >>>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
> >>>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
> >>>>>>> around P-Unit write accesses.
> >>>>>>
> >>>>>> Can't we just stuff the calls into the actual punit write function
> >>>>>> rather than sprinkling them all over the place?
> >>>>>
> >>>>> punit access is acquired across sections like this:
> >>>>>
> >>>>> iosf_mbi_punit_acquire();
> >>>>>
> >>>>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>> val &= ~DSPFREQGUAR_MASK;
> >>>>> val |= (cmd << DSPFREQGUAR_SHIFT);
> >>>>> vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> >>>>> if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
> >>>>> DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
> >>>>> 50)) {
> >>>>> DRM_ERROR("timed out waiting for CDclk change\n");
> >>>>> }
> >>>>> iosf_mbi_punit_release();
> >>>>>
> >>>>> Where we want to wait for the requested change to have taken
> >>>>> effect before releasing the punit.
> >>>
> >>> Hmm. That's somewhat unfortunate. It also highlights a problem with the
> >>> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
> >>> in set_rps() because that would slow things down too much. So I have to
> >>> wonder how much luck is needed to make this workaround really effective.
> >>
> >> So the history of this patch-set is that I wrote this patch before
> >> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
> >> active (patch 12/13). Since a lot of testing was done with this
> >> patch included in the patch-set and since it seemed a good idea
> >> regardless (given my experience with accessing the punit vs
> >> pmic bus accesses) I decided to leave it in.
> >>
> >> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
> >> actually fixed things for me. That is also why I made this the
> >> last patch in the set. I asked tagorereddy to test his system
> >> without this patch, but he did not get around to that.
> >>
> >> After all we do tell the punit to not touch the bus by acquiring
> >> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
> >> for RPS freq changes it honors that and properly waits. Maybe it
> >> honors that for all punit requests i915 does and the only real
> >> problem is the forcewake stuff ?
> >>
> >> I can try to drop this patch from my queue and run without it
> >> for a while and see if things don't regress. And also ask
> >> tagorereddy again to test his system that way.
> >>
> >> Does that (dropping this patch for now) sound like a good idea?
> >
> > More test results couldn't hurt at least. It also makes me wonder if
> > just bumping the timeouts to some ridiculously high value would fix
> > the problem as well.
>
> I've already tried bumping the forcewake timeout from 50 to 250ms,
> before writing the patch to just get forcewake_all before the pmic
> bus access begins, that does not fix things,
And you bumped the i2c mutex timeout as well? Or does that fail somehow
gracefully if it can't get the mutex?
> and since we busy wait
> for this timeout from non-sleeping context 250ms already is way too
> high.
Sure, but I'm just trying to understand if the problem is simply caused
by proceeding with some hardware access without getting the i2c mutex.
>
> >>>>>> + a comment would be nice why it's there.
> >>>>>
> >>>>> I will add comments to the acquire calls.
> >>>>>
> >>>>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
> >>>>>> ifdefs?
> >>>>>
> >>>>> No, the iosf_mbi header defines empty inline versions of
> >>>>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
> >>>>> this does mean that iosf_mbi must be builtin if the i915
> >>>>> driver is. I'll add:
> >>>>>
> >>>>> depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
> >>>>>
> >>>>> To the i915 Kconfig to enforce this.
> >>>>
> >>>> Hmm, ok so that does not work (long cyclic dependency through the
> >>>> selection of ACPI_VIDEO).
> >>>>
> >>>> So I've now added this instead:
> >>>>
> >>>> # iosf_mbi needs to be builtin if we are builtin
> >>>> select IOSF_MBI if DRM_I915=y
> >>>
> >>> That's probably not going to help anyone since i915 is usually a module.
> >>
> >> Right, that is fine, then either the IOSF_MBI symbols are available,
> >> or IOSF_MBI is disabled and we get the inline nops from the header.
> >>
> >> The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
> >> realistic IMHO, but will get triggered by the random-config testing
> >> several contributors do and lead to an unresolved symbol error there.
> >
> > Well, from the user POV anything with IOSF_MBI==n can be a problem.
> > So I'm not sure if we should allow that.
>
> So you're suggesting we just add an unconditional "select IOSF_MBI"
> to the i915 Kconfig entry?
Yeah, that should at least cut down the number of people accidentally
misconfiguring their kernels and hitting this problem in the future.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH v2 13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings
From: Hans de Goede @ 2017-01-30 16:33 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Wolfram Sang, russianneuromancer @ ya . ru, intel-gfx, linux-i2c,
Jarkko Nikula, dri-devel, H . Peter Anvin, Daniel Vetter,
Thomas Gleixner, Andy Shevchenko, Mika Westerberg, Len Brown
In-Reply-To: <20170130153805.GQ31595@intel.com>
Hi,
On 30-01-17 16:38, Ville Syrjälä wrote:
> On Mon, Jan 30, 2017 at 04:27:58PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 30-01-17 16:11, Ville Syrjälä wrote:
>>> On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 30-01-17 14:10, Ville Syrjälä wrote:
>>>>> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>>>>>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>>>>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>>>>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>>>>>>>> around P-Unit write accesses.
>>>>>>>>
>>>>>>>> Can't we just stuff the calls into the actual punit write function
>>>>>>>> rather than sprinkling them all over the place?
>>>>>>>
>>>>>>> punit access is acquired across sections like this:
>>>>>>>
>>>>>>> iosf_mbi_punit_acquire();
>>>>>>>
>>>>>>> val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>> val &= ~DSPFREQGUAR_MASK;
>>>>>>> val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>>>> vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>>>>> if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>>>>>>> DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>>>>>>> 50)) {
>>>>>>> DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>>> }
>>>>>>> iosf_mbi_punit_release();
>>>>>>>
>>>>>>> Where we want to wait for the requested change to have taken
>>>>>>> effect before releasing the punit.
>>>>>
>>>>> Hmm. That's somewhat unfortunate. It also highlights a problem with the
>>>>> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
>>>>> in set_rps() because that would slow things down too much. So I have to
>>>>> wonder how much luck is needed to make this workaround really effective.
>>>>
>>>> So the history of this patch-set is that I wrote this patch before
>>>> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
>>>> active (patch 12/13). Since a lot of testing was done with this
>>>> patch included in the patch-set and since it seemed a good idea
>>>> regardless (given my experience with accessing the punit vs
>>>> pmic bus accesses) I decided to leave it in.
>>>>
>>>> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
>>>> actually fixed things for me. That is also why I made this the
>>>> last patch in the set. I asked tagorereddy to test his system
>>>> without this patch, but he did not get around to that.
>>>>
>>>> After all we do tell the punit to not touch the bus by acquiring
>>>> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
>>>> for RPS freq changes it honors that and properly waits. Maybe it
>>>> honors that for all punit requests i915 does and the only real
>>>> problem is the forcewake stuff ?
>>>>
>>>> I can try to drop this patch from my queue and run without it
>>>> for a while and see if things don't regress. And also ask
>>>> tagorereddy again to test his system that way.
>>>>
>>>> Does that (dropping this patch for now) sound like a good idea?
>>>
>>> More test results couldn't hurt at least. It also makes me wonder if
>>> just bumping the timeouts to some ridiculously high value would fix
>>> the problem as well.
>>
>> I've already tried bumping the forcewake timeout from 50 to 250ms,
>> before writing the patch to just get forcewake_all before the pmic
>> bus access begins, that does not fix things,
>
> And you bumped the i2c mutex timeout as well? Or does that fail somehow
> gracefully if it can't get the mutex?
It will fail the i2c transfer with -ETIMEOUT, which will make the driver
report an error instead of e.g. the battery level, but it should not
affect the forcewake calls and those still failed with the large
timeout. So yes basically the i2c mutex fails gracefully.
>
>> and since we busy wait
>> for this timeout from non-sleeping context 250ms already is way too
>> high.
>
> Sure, but I'm just trying to understand if the problem is simply caused
> by proceeding with some hardware access without getting the i2c mutex.
Understood.
>>>>>>>> + a comment would be nice why it's there.
>>>>>>>
>>>>>>> I will add comments to the acquire calls.
>>>>>>>
>>>>>>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
>>>>>>>> ifdefs?
>>>>>>>
>>>>>>> No, the iosf_mbi header defines empty inline versions of
>>>>>>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
>>>>>>> this does mean that iosf_mbi must be builtin if the i915
>>>>>>> driver is. I'll add:
>>>>>>>
>>>>>>> depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
>>>>>>>
>>>>>>> To the i915 Kconfig to enforce this.
>>>>>>
>>>>>> Hmm, ok so that does not work (long cyclic dependency through the
>>>>>> selection of ACPI_VIDEO).
>>>>>>
>>>>>> So I've now added this instead:
>>>>>>
>>>>>> # iosf_mbi needs to be builtin if we are builtin
>>>>>> select IOSF_MBI if DRM_I915=y
>>>>>
>>>>> That's probably not going to help anyone since i915 is usually a module.
>>>>
>>>> Right, that is fine, then either the IOSF_MBI symbols are available,
>>>> or IOSF_MBI is disabled and we get the inline nops from the header.
>>>>
>>>> The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
>>>> realistic IMHO, but will get triggered by the random-config testing
>>>> several contributors do and lead to an unresolved symbol error there.
>>>
>>> Well, from the user POV anything with IOSF_MBI==n can be a problem.
>>> So I'm not sure if we should allow that.
>>
>> So you're suggesting we just add an unconditional "select IOSF_MBI"
>> to the i915 Kconfig entry?
>
> Yeah, that should at least cut down the number of people accidentally
> misconfiguring their kernels and hitting this problem in the future.
Ok.
Regards,
Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v8 07/12] dt-bindings: i2c: i2c-mux-simple: document i2c-mux-simple bindings
From: Rob Herring @ 2017-01-30 17:20 UTC (permalink / raw)
To: Peter Rosin
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Wolfram Sang, Mark Rutland, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
Andrew Morton, linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <835415bf-77af-6131-d018-d0bc7d8233b0-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On Sat, Jan 28, 2017 at 4:42 PM, Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org> wrote:
> On 2017-01-27 20:39, Rob Herring wrote:
>> On Wed, Jan 18, 2017 at 04:57:10PM +0100, Peter Rosin wrote:
>>> Describe how a generic multiplexer controller is used to mux an i2c bus.
>>>
>>> Acked-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
>>> ---
>>> .../devicetree/bindings/i2c/i2c-mux-simple.txt | 81 ++++++++++++++++++++++
>>> 1 file changed, 81 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
>>> new file mode 100644
>>> index 000000000000..253d5027843b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-simple.txt
>>> @@ -0,0 +1,81 @@
>>> +Simple I2C Bus Mux
>>> +
>>> +This binding describes an I2C bus multiplexer that uses a mux controller
>>> +from the mux subsystem to route the I2C signals.
>>> +
>>> + .-----. .-----.
>>> + | dev | | dev |
>>> + .------------. '-----' '-----'
>>> + | SoC | | |
>>> + | | .--------+--------'
>>> + | .------. | .------+ child bus A, on MUX value set to 0
>>> + | | I2C |-|--| Mux |
>>> + | '------' | '--+---+ child bus B, on MUX value set to 1
>>> + | .------. | | '----------+--------+--------.
>>> + | | MUX- | | | | | |
>>> + | | Ctrl |-|-----+ .-----. .-----. .-----.
>>> + | '------' | | dev | | dev | | dev |
>>> + '------------' '-----' '-----' '-----'
>>> +
>>> +Required properties:
>>> +- compatible: i2c-mux-simple,mux-locked or i2c-mux-simple,parent-locked
>>
>> Not a fan of using "simple" nor the ','. Perhaps lock type should be
>> separate property.
>
> How about just i2c-mux for the compatible? Because i2c-mux-mux (which
> follows the naming of previous i2c muxes) looks really stupid. Or
> perhaps i2c-mux-generic?
I like "generic" only slightly more than "simple". :)
If the mux is gpio controlled, then it should still be called
i2c-gpio-mux. Let's not invent brand new bindings when current ones
are easily extended. We already have pretty generic names here, let's
not make them more generic.
>
> I'm also happy to have the lock type as a separate property. One lock
> type, e.g. parent-locked, could be the default and adding a 'mux-locked'
> property could change that. Would that be ok?
I prefer this. Then existing bindings can use it.
> Or should it be a requirement that one of 'mux-locked'/'parent-locked'
> is always present?
I would make it boolean and make not present be the more common case.
Not present could also mean determined via other means as you have
today with existing bindings. Maybe then you need both properties.
>> I'm not sure I get the mux vs. parent locked fully. How do I determine
>> what type I have? We already have bindings for several types of i2c
>> muxes. How does the locking annotation fit into those?
>
> We have briefly discussed this before [1] in the context of i2c-mux-gpio
> and i2c-mux-pinctrl, when I added the mux-locked/parent-locked distinction
> to the i2c-mux infrastructure (it wasn't named mux-locked/parent-locked
> way back then though). There is more detail on what the difference is
> between the two in Documentation/i2c/i2c-topology.
>
> Side note regarding your remark "use an I2C controlled mux instead"; it
> appears that I'm not alone [2] with this kind of requirement...
>
> [1] https://lkml.org/lkml/2016/1/6/437
> [2] http://marc.info/?t=147877959100002&r=1&w=2
>
> But, now that I have pondered on this for a year or so, I firmly
> believe it was a mistake to have the code in i2c-mux-gpio and
> i2c-mux-pinctrl automatically try to deduce if the mux should be
> mux-locked or parent-locked. It might be easy to make that call
> in some trivial cases, but it is not difficult to dream up
> scenarios where it would be extremely hard for the code to get
> this decision right. It's just fragile. But now we have code in
> those two muxes that has unwanted tentacles into the guts of the
> gpio and pinctrl subsystems. Hopefully those unwanted tentacles
> can be replaced with something based on device links? However, it
> is still not hard to come up with scenarios that will require
> manual intervention in order to select the right kind of i2c mux
> locking. So, I fear that we have inadequate code trying to make a
> decision automatically, and that we at some point down the line
> will have some impossible case needing a binding that trumps the
> heuristic. Why have a heuristic at all in that case? In short, it
> should have been a binding from the start, methinks.
>
> That was a long rant regarding i2c-mux-gpio and i2c-mux-pinctrl.
> I obviously think it is bad to have this new i2c mux go in the
> same direction as those two drivers. I.e. I think a binding that
> specifies the desired locking is preferable to any attempted
> heuristic.
Having a separate, common property(ies) for these would solve this then.
Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] i2c: at91: ensure state is restored after suspending
From: Alexandre Belloni @ 2017-01-30 17:25 UTC (permalink / raw)
To: Wolfram Sang
Cc: Ludovic Desroches, Alexandre Belloni, linux-i2c, linux-arm-kernel,
linux-kernel
When going to suspend, the I2C registers may be lost because the power to
VDDcore is cut. Save them and restore them when resuming.
Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
drivers/i2c/busses/i2c-at91.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 0b86c6173e07..633bdd899952 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -140,6 +140,12 @@ struct at91_twi_dev {
unsigned transfer_status;
struct i2c_adapter adapter;
unsigned twi_cwgr_reg;
+ struct {
+ u32 mmr;
+ u32 imr;
+ u32 fmr;
+ u32 fimr;
+ } cache;
struct at91_twi_pdata *pdata;
bool use_dma;
bool use_alt_cmd;
@@ -1172,6 +1178,15 @@ static int at91_twi_runtime_resume(struct device *dev)
static int at91_twi_suspend_noirq(struct device *dev)
{
+ struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
+
+ twi_dev->cache.mmr = at91_twi_read(twi_dev, AT91_TWI_MMR);
+ twi_dev->cache.imr = at91_twi_read(twi_dev, AT91_TWI_IMR);
+ if (twi_dev->fifo_size) {
+ twi_dev->cache.fmr = at91_twi_read(twi_dev, AT91_TWI_FMR);
+ twi_dev->cache.fimr = at91_twi_read(twi_dev, AT91_TWI_FIMR);
+ }
+
if (!pm_runtime_status_suspended(dev))
at91_twi_runtime_suspend(dev);
@@ -1180,6 +1195,7 @@ static int at91_twi_suspend_noirq(struct device *dev)
static int at91_twi_resume_noirq(struct device *dev)
{
+ struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
int ret;
if (!pm_runtime_status_suspended(dev)) {
@@ -1191,6 +1207,14 @@ static int at91_twi_resume_noirq(struct device *dev)
pm_runtime_mark_last_busy(dev);
pm_request_autosuspend(dev);
+ at91_init_twi_bus(twi_dev);
+ at91_twi_write(twi_dev, AT91_TWI_MMR, twi_dev->cache.mmr);
+ at91_twi_write(twi_dev, AT91_TWI_IER, twi_dev->cache.imr);
+ if (twi_dev->fifo_size) {
+ at91_twi_write(twi_dev, AT91_TWI_FMR, twi_dev->cache.fmr);
+ at91_twi_write(twi_dev, AT91_TWI_FIER, twi_dev->cache.fimr);
+ }
+
return 0;
}
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] i2c: at91: ensure state is restored after suspending
From: Wolfram Sang @ 2017-01-30 17:44 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Ludovic Desroches, linux-i2c, linux-arm-kernel, linux-kernel
In-Reply-To: <20170130172533.31995-1-alexandre.belloni@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 181 bytes --]
> + at91_init_twi_bus(twi_dev);
Can't this function reinit the registers to the needed values? I am not
convinced that a cache will always reflect the proper state after
resume.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: Issue with i2c-designware-platdrv's suspend/runtime-suspend handling
From: John Stultz @ 2017-01-30 22:05 UTC (permalink / raw)
To: Jarkko Nikula
Cc: Andy Shevchenko, Mika Westerberg, Wolfram Sang, linux-i2c, lkml,
zhangfei, Guodong Xu, Amit Pundir
In-Reply-To: <CALAqxLU7-77hEwUFNNdsvnRnrPPu+-eZfkDMHxeTT3MLMiKhfA@mail.gmail.com>
On Tue, Jan 24, 2017 at 2:03 PM, John Stultz <john.stultz@linaro.org> wrote:
> I noticed that with my hikey board, on resume from suspend I'm getting
> the following WARNING:
>
> [ 54.334054] ------------[ cut here ]------------
> [ 54.334077] WARNING: CPU: 0 PID: 2217 at drivers/clk/clk.c:594
> clk_core_disable+0x20/0x78
> [ 54.334080]
> [ 54.334090] CPU: 0 PID: 2217 Comm: system_server Not tainted
> 4.9.0-00046-gee9ec2c #2067
> [ 54.334094] Hardware name: HiKey Development Board (DT)
> [ 54.334099] task: ffffffc074863200 task.stack: ffffffc061d1c000
> [ 54.334105] PC is at clk_core_disable+0x20/0x78
> [ 54.334111] LR is at clk_core_disable_lock+0x20/0x38
> [ 54.334116] pc : [<ffffff80084ab728>] lr : [<ffffff80084ab848>]
> pstate: 800001c5
> [ 54.334119] sp : ffffffc061d1faf0
> [ 54.334128] x29: ffffffc061d1faf0 x28: 0000000000000000
> [ 54.334136] x27: 0000000000000002 x26: ffffff8008d47000
> [ 54.334143] x25: ffffff8008596000 x24: ffffff8008d15498
> [ 54.334151] x23: ffffff8008db9000 x22: ffffffc075074870
> [ 54.334158] x21: 0000000000000002 x20: ffffffc005f09500
> [ 54.334165] x19: 0000000000000140 x18: 0000000000000001
> [ 54.334172] x17: 0000000000000000 x16: 0000000000000000
> [ 54.334180] x15: ffffffc005f43dc0 x14: 0000000000010000
> [ 54.334187] x13: ffffff8008d906f8 x12: ffffff8008d15790
> [ 54.334196] x11: ffffff8008d15000 x10: ffffff8008d8d000
> [ 54.334204] x9 : 0000000000000000 x8 : ffffffc077f26218
> [ 54.334212] x7 : 0000000000000000 x6 : 0000000000000000
> [ 54.334220] x5 : ffffffc077f2b090 x4 : ffffffc061d1fa80
> [ 54.334228] x3 : ffffff80084ab62c x2 : ffffff80084ab62c
> [ 54.334236] x1 : 0000000000000000 x0 : ffffffc005f09500
> [ 54.334239]
> [ 54.334243] ---[ end trace 6474a624fb2fd658 ]---
> [ 54.334248] Call trace:
> [ 54.334254] Exception stack(0xffffffc061d1f920 to 0xffffffc061d1fa50)
> [ 54.334262] f920: 0000000000000140 0000008000000000
> ffffffc061d1faf0 ffffff80084ab728
> [ 54.334269] f940: ffffff8008ccc090 ffffffc074863200
> 0000000000000000 6f6674616c703d4d
> [ 54.334276] f960: ffffffc061d1f970 ffffff8008087990
> ffffffc061d1f9b0 ffffff8008450c6c
> [ 54.334283] f980: ffffff8008cc8000 ffffffc061d1fa90
> ffffff8008ccc090 ffffffc074863200
> [ 54.334290] f9a0: ffffff8008db6170 ffffffc061d1fa08
> ffffffc061d1f9c0 ffffff8008087990
> [ 54.334297] f9c0: ffffffc005f09500 0000000000000000
> ffffff80084ab62c ffffff80084ab62c
> [ 54.334303] f9e0: ffffffc061d1fa80 ffffffc077f2b090
> 0000000000000000 0000000000000000
> [ 54.334310] fa00: ffffffc077f26218 0000000000000000
> ffffff8008d8d000 ffffff8008d15000
> [ 54.334317] fa20: ffffff8008d15790 ffffff8008d906f8
> 0000000000010000 ffffffc005f43dc0
> [ 54.334322] fa40: 0000000000000000 0000000000000000
> [ 54.334328] [<ffffff80084ab728>] clk_core_disable+0x20/0x78
> [ 54.334336] [<ffffff80084ad26c>] clk_disable+0x1c/0x30
> [ 54.334349] [<ffffff80086d926c>] i2c_dw_plat_prepare_clk.isra.0+0x44/0x80
> [ 54.334356] [<ffffff80086d930c>] dw_i2c_plat_suspend+0x24/0x38
> [ 54.334367] [<ffffff800858ad54>] platform_pm_suspend+0x24/0x58
> [ 54.334377] [<ffffff8008595558>] dpm_run_callback.isra.7+0x20/0x68
> [ 54.334385] [<ffffff8008595fb4>] __device_suspend+0x10c/0x298
> [ 54.334393] [<ffffff8008597154>] dpm_suspend+0x10c/0x228
> [ 54.334401] [<ffffff8008597540>] dpm_suspend_start+0x68/0x78
> [ 54.334412] [<ffffff80080fa058>] suspend_devices_and_enter+0xb8/0x458
> [ 54.334419] [<ffffff80080fa5e4>] pm_suspend+0x1ec/0x248
> [ 54.334426] [<ffffff80080f924c>] state_store+0x94/0xa8
> [ 54.334434] [<ffffff80084366dc>] kobj_attr_store+0x14/0x28
> [ 54.334444] [<ffffff8008249a28>] sysfs_kf_write+0x48/0x58
> [ 54.334451] [<ffffff8008248d20>] kernfs_fop_write+0xb0/0x1d8
> [ 54.334461] [<ffffff80081cd714>] __vfs_write+0x1c/0x100
> [ 54.334469] [<ffffff80081cd9a0>] vfs_write+0xa0/0x1b8
> [ 54.334477] [<ffffff80081cdb9c>] SyS_write+0x44/0xa0
> [ 54.334485] [<ffffff8008082ef0>] el0_svc_naked+0x24/0x28
>
>
> Doing some further debugging, it seems the problem is that the device
> is being runtime suspended, and then at suspend time, we're calling
> the same logic, calling i2c_dw_plat_prepare_clk, which causes the clk
> count warning.
>
> Removing the runtime pm ops:
> - SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
> +// SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
>
> seems to avoid the warning, but clearly isn't ideal. :)
>
> Should there be some logic keep track of the suspend state for the
> dw_i2c_dev device so we don't try to suspend (or resume) it twice? Or
> is there something else I'm missing to keep this from happening?
Ping? Any thoughts on how best to fix this? I'm leaning towards
adding a suspended state to the struct dw_i2c_dev. Any objections?
thanks
-john
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox