* Re: [GIT PULL] On-demand device probing
From: Alexandre Courbot @ 2015-10-19 23:47 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Uwe Kleine-König, David Woodhouse, Mark Brown, Rob Herring,
Greg Kroah-Hartman, Tomeu Vizoso, Michael Turquette, Stephen Boyd,
Vinod Koul, Dan Williams, Linus Walleij, Thierry Reding,
David Airlie, Terje Bergström, Stephen Warren, Wolfram Sang,
Frank Rowand, Grant Likely, Kishon Vijay Abraham I,
Sebastian Reichel, Dmitry Eremin-Solenikov
In-Reply-To: <20151019183905.GS32532@n2100.arm.linux.org.uk>
On Tue, Oct 20, 2015 at 3:39 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Oct 19, 2015 at 08:27:44PM +0200, Uwe Kleine-König wrote:
>> Hello,
>>
>> On Mon, Oct 19, 2015 at 04:43:24PM +0100, Russell King - ARM Linux wrote:
>> > It's a bit ironic that you've chosen GPIO as an example there. The
>> > "new" GPIO API (the gpiod_* stuff) only has a fwnode way to get the
>> > gpio descriptor. There's no of_* method.
>>
>> Without following all that fwnode discussion:
>> gpiod_get et al. should work for you here, doesn't it? It just takes a
>> struct device * and I'm happy with it.
>
> What if you don't have a struct device? I had that problem recently
> when modifying the mvebu PCIe code. The 'struct device' node doesn't
> contain the GPIOs, it's the PCIe controller. Individual ports on the
> controller are described in DT as sub-nodes, and the sub-nodes can
> have a GPIO for card reset purposes. These sub-nodes don't have a
> struct device.
>
> Right now, I'm having to do this to work around this issue:
>
> reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags);
> if (reset_gpio = -EPROBE_DEFER) {
> ret = reset_gpio;
> goto err;
> }
>
> if (gpio_is_valid(reset_gpio)) {
> unsigned long gpio_flags;
>
> port->reset_name = devm_kasprintf(dev, GFP_KERNEL, "%s-reset",
> port->name);
> if (!port->reset_name) {
> ret = -ENOMEM;
> goto err;
> }
>
> if (flags & OF_GPIO_ACTIVE_LOW) {
> dev_info(dev, "%s: reset gpio is active low\n",
> of_node_full_name(child));
> gpio_flags = GPIOF_ACTIVE_LOW |
> GPIOF_OUT_INIT_LOW;
> } else {
> gpio_flags = GPIOF_OUT_INIT_HIGH;
> }
>
> ret = devm_gpio_request_one(dev, reset_gpio, gpio_flags,
> port->reset_name);
> if (ret) {
> if (ret = -EPROBE_DEFER)
> goto err;
> goto skip;
> }
>
> port->reset_gpio = gpio_to_desc(reset_gpio);
> }
>
> Not nice, is it? Not nice to have that in lots of drivers either.
>
> However, switching to use any of_* or fwnode_* thing also carries with
> it another problem: you can't control the name appearing in the
> allocation, so you end up with a bunch of GPIOs requested with a "reset"
> name - meaning you lose any identification of which port the GPIO was
> bound to.
There are a few holes in the gpiod API. I see two solutions here:
1) extend devm_get_gpiod_from_child() to take an optional name argument
2) add a function to explicitly change a GPIO's name
2) seems to be the most generic solution, would that do the trick?
(sorry for the off-topic)
^ permalink raw reply
* Re: [GIT PULL] On-demand device probing
From: Rafael J. Wysocki @ 2015-10-20 7:56 UTC (permalink / raw)
To: Rob Herring
Cc: David Woodhouse, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso,
Russell King, Michael Turquette, Stephen Boyd, Vinod Koul,
Dan Williams, Linus Walleij, Alexandre Courbot, Thierry Reding,
David Airlie, Terje Bergström, Stephen Warren, Wolfram Sang,
Frank Rowand, Grant Likely, Kishon Vijay Abraham I,
Sebastian Reichel, Dmitry Eremin-Solenikov
In-Reply-To: <CAL_JsqKa3MFJUWKV2KxPE_NmrP2g4dOD3zr+0Kyx4yBkDOg2HA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Monday, October 19, 2015 05:58:40 PM Rob Herring wrote:
> On Mon, Oct 19, 2015 at 4:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Monday, October 19, 2015 10:58:25 AM Rob Herring wrote:
> >> On Mon, Oct 19, 2015 at 10:29 AM, David Woodhouse <dwmw2@infradead.org> wrote:
> >> > On Mon, 2015-10-19 at 15:50 +0100, Mark Brown wrote:
> >> >> > But the point I'm making is that we are working towards *fixing* that,
> >> >> > and *not* using DT-specific code in places where we should be using the
> >> >> > generic APIs.
> >> >>
> >> >> What is the plan for fixing things here? It's not obvious (at least to
> >> >> me) that we don't want to have the subsystems having knowledge of how
> >> >> they are bound to a specific firmware which is what you seem to imply
> >> >> here.
> >> >
> >> > I don't know that there *is* a coherent plan here to address it all.
> >> >
> >> > Certainly, we *will* need subsystems to have firmware-specific
> >> > knowledge in some cases. Take GPIO as an example; ACPI *has* a way to
> >> > describe GPIO, and properties which reference GPIO pins are intended to
> >> > work through that — while in DT, properties which reference GPIO pins
> >> > will have different contents. They'll be compatible at the driver
> >> > level, in the sense that there's a call to get a given GPIO given the
> >> > property name, but the subsystems *will* be doing different things
> >> > behind the scenes.
> >> >
> >> > My plan, such as it is, is to go through the leaf-node drivers which
> >> > almost definitely *should* be firmware-agnostic, and convert those. And
> >> > then take stock of what we have left, and work out what, if anything,
> >> > still needs to be done.
> >>
> >> Many cases are already agnostic in the drivers in terms of the *_get()
> >> functions. Some are DT specific, but probably because those subsystems
> >> are new and DT only. In any case, I don't think these 1 line changes
> >> do anything to make doing conversions here harder.
> >>
> >> >> It seems like we're going to have to refactor these bits of code when
> >> >> they get generalised anyway so I'm not sure that the additional cost
> >> >> here is that big.
> >> >
> >> > That's an acceptable answer — "we're adding legacy code here but we
> >> > know it's going to be refactored anyway". If that's true, all it takes
> >> > is a note in the commit comment to that effect. That's different from
> >> > having not thought about it :)
> >>
> >> Considering at one point we did create a fwnode based API, we did
> >> think about it. Plus there was little input from ACPI folks as to
> >> whether the change was even useful for ACPI case.
> >
> > Well, sorry, but who was asking whom, specifically?
>
> You and linux-acpi have been copied on v2 and later of the entire
> series I think.
Yes, but it wasn't like a direct request, say "We need your input, so can you
please have a look and BTW we want this in 4.4, so please do it ASAP". In
which case I'd prioritize that before other things I needed to take care of.
> > The underlying problem is present in ACPI too and we don't really have a good
> > solution for it. We might benefit from a common one if it existed.
>
> The problem for DT is we don't generically know what are the
> dependencies at a core level. We could know some or most dependencies
> if phandles (links to other nodes) were typed, but they are not. If
> the core had this information, we could simply control the device
> creation to order probing. Instead, this information is encoded into
> the bindings and binding parsing resides in the subsystems. That
> parsing happens during probe of the client side and is done by the
> subsystems (for common bindings). Since we already do the parsing at
> this point, it is a convenient place to trigger the probe of the
> dependency. Is ACPI going to be similar in this regard?
It is similar in some ways. For example, if a device's functionality depends
on an I2C resource (connection), the core doesn't know that at the device
creation time at least in some cases. Same for GPIO, SPI, DMA engines etc.
There is a _DEP object in ACPI that can be used by firmware to tell the OS
about those dependencies, but there's no way in the driver core to use that
information anyway today.
> Fundamentally, it is a question of probe devices when their
> dependencies are present or drivers ensure their dependencies are
> ready. IIRC, init systems went thru a similar debate for service
> dependencies.
The probe ordering is not the entire picture, though.
Even if you get the probe ordering right, the problem is going to show up in
multiple other places: system suspend/resume, runtime PM, system shutdown,
unbinding of drivers. In all of those cases it is necessary to handle things
in a specific order if there is a dependency.
> >> In any case, we're talking about adding 1 line.
> >
> > But also about making the driver core slighly OF-centric.
>
> How so? The one line is in DT binding parsing code in subsystems, not
> driver core. The driver core change is we add every device (that
> happened to be created by DT) to the deferred probe list, so they
> don't probe right away.
The "that happened to be created by DT" part is of concern here. What is there
that makes DT special in that respect? Why shouldn't that be applicable to
devices created by the ACPI core, for example, or by a board file or something
else?
> > Sure, we need OF-specific code and ACPI-specific code wherever different
> > handling is required, but doing that at the driver core level seems to be
> > a bit of a stretch to me.
> >
> > Please note that we don't really have ACPI-specific calls in the driver core,
> > although we might have added them long ago even before the OF stuff appeared
> > in the kernel for the first time. We didn't do that, (among other things)
> > because we didn't want that particular firmware interface to appear special
> > in any way and I'm not really sure why it is now OK to make OF look special
> > instead.
>
> I don't think DT is special and we avoid DT specific core changes as
> much as possible. I think the difference is DT uses platform_device
> and ACPI does not.
ACPI uses platform devices too. In fact, ACPI device objects are enumerated as
platform devices by default now.
Or do you means something else here?
> It used to be separate, but got merged together primarily to support the
> plethora of existing drivers. Anyway, that is all outside of anything in this
> series.
It explains the context of the series, so it is useful to talk about IMO.
>
> > If it is trivial to avoid that (and you seem to be arguing that it is), why
> > do we have to do it?
>
> Sorry, I don't follow what "that" or "it" is.
OK, so maybe I misunderstood you, sorry about that.
Thanks,
Rafael
^ permalink raw reply
* Re: [GIT PULL] On-demand device probing
From: David Woodhouse @ 2015-10-20 11:12 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Mark Brown, Rob Herring, Greg Kroah-Hartman, Tomeu Vizoso,
Michael Turquette, Stephen Boyd, Vinod Koul, Dan Williams,
Linus Walleij, Alexandre Courbot, Thierry Reding, David Airlie,
Terje Bergström, Stephen Warren, Wolfram Sang, Frank Rowand,
Grant Likely, Kishon Vijay Abraham I, Sebastian Reichel,
Dmitry Eremin-Solenikov, Liam Girdwood, Felipe Balbi
In-Reply-To: <20151019154324.GN32532@n2100.arm.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2757 bytes --]
On Mon, 2015-10-19 at 16:43 +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 19, 2015 at 04:29:40PM +0100, David Woodhouse wrote:
> > I don't know that there *is* a coherent plan here to address it
> > all.
> >
> > Certainly, we *will* need subsystems to have firmware-specific
> > knowledge in some cases. Take GPIO as an example; ACPI *has* a way
> > to
> > describe GPIO, and properties which reference GPIO pins are
> > intended to
> > work through that — while in DT, properties which reference GPIO
> > pins
> > will have different contents. They'll be compatible at the driver
> > level, in the sense that there's a call to get a given GPIO given
> > the
> > property name, but the subsystems *will* be doing different things
> > behind the scenes.
>
> It's a bit ironic that you've chosen GPIO as an example there. The
> "new" GPIO API (the gpiod_* stuff) only has a fwnode way to get the
> gpio descriptor. There's no of_* method.
I think that part is already being worked on, but...
> If ACPI already handles GPIOs internally, then I'm left wondering
> why GPIO descriptor stuff went down the fwnode route at all - it
> seems rather pointless in this case,
ACPI already had a way for a given device to say that it uses certain
other GPIOs. But until we had device properties in ACPI, it could say
*what* it used them for. So sure, we could say that we used GPIO#15
from <this> controller and GPIOs #27 and #31 from <that> controller.
But there was no way to say that the former was the shotdown pin and
the latter was the reset pin.
While a GPIO property in DT will contain a phandle and basically be a
complete reference to find the pin you're after, the same property
represented in ACPI will just be an index into the resources that ACPI
could already refer to.
So referring to the example in Documentation/acpi/gpio-properties.txt:
Name (_CRS, ResourceTemplate ()
{
GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
"\\_SB.GPO0", 0, ResourceConsumer) {15}
GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
"\\_SB.GPO0", 0, ResourceConsumer) {27, 31}
})
That part, ACPI already had. But..
Name (_DSD, Package ()
{
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package ()
{
Package () {"reset-gpio", Package() {^BTH, 1, 1, 0 }},
Package () {"shutdown-gpio", Package() {^BTH, 0, 0, 0 }},
}
})
...this part is new, and allows us the full flexibility of device
properties. And the appropriate gpiod_get* function is supposed to
transparently work on either DT or ACPI.
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]
^ permalink raw reply
* Re: [GIT PULL] On-demand device probing
From: Rob Herring @ 2015-10-20 14:15 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: David Woodhouse, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso,
Russell King, Michael Turquette, Stephen Boyd, Vinod Koul,
Dan Williams, Linus Walleij, Alexandre Courbot, Thierry Reding,
David Airlie, Terje Bergström, Stephen Warren, Wolfram Sang,
Frank Rowand, Grant Likely, Kishon Vijay Abraham I,
Sebastian Reichel, Dmitry Eremin-Solenikov
In-Reply-To: <4025469.zmyipZqCsP@vostro.rjw.lan>
On Tue, Oct 20, 2015 at 2:56 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, October 19, 2015 05:58:40 PM Rob Herring wrote:
>> On Mon, Oct 19, 2015 at 4:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Monday, October 19, 2015 10:58:25 AM Rob Herring wrote:
>> >> On Mon, Oct 19, 2015 at 10:29 AM, David Woodhouse <dwmw2@infradead.org> wrote:
>> >> > On Mon, 2015-10-19 at 15:50 +0100, Mark Brown wrote:
>> >> >> > But the point I'm making is that we are working towards *fixing* that,
>> >> >> > and *not* using DT-specific code in places where we should be using the
>> >> >> > generic APIs.
>> >> >>
>> >> >> What is the plan for fixing things here? It's not obvious (at least to
>> >> >> me) that we don't want to have the subsystems having knowledge of how
>> >> >> they are bound to a specific firmware which is what you seem to imply
>> >> >> here.
>> >> >
>> >> > I don't know that there *is* a coherent plan here to address it all.
>> >> >
>> >> > Certainly, we *will* need subsystems to have firmware-specific
>> >> > knowledge in some cases. Take GPIO as an example; ACPI *has* a way to
>> >> > describe GPIO, and properties which reference GPIO pins are intended to
>> >> > work through that — while in DT, properties which reference GPIO pins
>> >> > will have different contents. They'll be compatible at the driver
>> >> > level, in the sense that there's a call to get a given GPIO given the
>> >> > property name, but the subsystems *will* be doing different things
>> >> > behind the scenes.
>> >> >
>> >> > My plan, such as it is, is to go through the leaf-node drivers which
>> >> > almost definitely *should* be firmware-agnostic, and convert those. And
>> >> > then take stock of what we have left, and work out what, if anything,
>> >> > still needs to be done.
>> >>
>> >> Many cases are already agnostic in the drivers in terms of the *_get()
>> >> functions. Some are DT specific, but probably because those subsystems
>> >> are new and DT only. In any case, I don't think these 1 line changes
>> >> do anything to make doing conversions here harder.
>> >>
>> >> >> It seems like we're going to have to refactor these bits of code when
>> >> >> they get generalised anyway so I'm not sure that the additional cost
>> >> >> here is that big.
>> >> >
>> >> > That's an acceptable answer — "we're adding legacy code here but we
>> >> > know it's going to be refactored anyway". If that's true, all it takes
>> >> > is a note in the commit comment to that effect. That's different from
>> >> > having not thought about it :)
>> >>
>> >> Considering at one point we did create a fwnode based API, we did
>> >> think about it. Plus there was little input from ACPI folks as to
>> >> whether the change was even useful for ACPI case.
>> >
>> > Well, sorry, but who was asking whom, specifically?
>>
>> You and linux-acpi have been copied on v2 and later of the entire
>> series I think.
>
> Yes, but it wasn't like a direct request, say "We need your input, so can you
> please have a look and BTW we want this in 4.4, so please do it ASAP". In
> which case I'd prioritize that before other things I needed to take care of.
Fair enough. Can you please review and comment on v7 of the series? We
can discuss at KS as well.
>> > The underlying problem is present in ACPI too and we don't really have a good
>> > solution for it. We might benefit from a common one if it existed.
>>
>> The problem for DT is we don't generically know what are the
>> dependencies at a core level. We could know some or most dependencies
>> if phandles (links to other nodes) were typed, but they are not. If
>> the core had this information, we could simply control the device
>> creation to order probing. Instead, this information is encoded into
>> the bindings and binding parsing resides in the subsystems. That
>> parsing happens during probe of the client side and is done by the
>> subsystems (for common bindings). Since we already do the parsing at
>> this point, it is a convenient place to trigger the probe of the
>> dependency. Is ACPI going to be similar in this regard?
>
> It is similar in some ways. For example, if a device's functionality depends
> on an I2C resource (connection), the core doesn't know that at the device
> creation time at least in some cases. Same for GPIO, SPI, DMA engines etc.
So you will need to create devices, defer their probing and then probe
on demand as well unless you have other ideas how you would do it.
> There is a _DEP object in ACPI that can be used by firmware to tell the OS
> about those dependencies, but there's no way in the driver core to use that
> information anyway today.
I would think that the equivalent function for ACPI to of_device_probe
could process these if they are generic and you can associate the
dependency to a struct device.
>> Fundamentally, it is a question of probe devices when their
>> dependencies are present or drivers ensure their dependencies are
>> ready. IIRC, init systems went thru a similar debate for service
>> dependencies.
>
> The probe ordering is not the entire picture, though.
>
> Even if you get the probe ordering right, the problem is going to show up in
> multiple other places: system suspend/resume, runtime PM, system shutdown,
> unbinding of drivers. In all of those cases it is necessary to handle things
> in a specific order if there is a dependency.
My understanding was with deferred probe that it also solves suspend
ordering problems because things are suspended in reverse order of
probing. I suppose you could have slightly different dependencies for
suspend, runtime PM, etc. than for probe? Perhaps we need to save the
list of dependencies as we probe them. I don't think that would be too
hard to add on to this series, but then if we don't need it now, why
add it?
>> >> In any case, we're talking about adding 1 line.
>> >
>> > But also about making the driver core slighly OF-centric.
>>
>> How so? The one line is in DT binding parsing code in subsystems, not
>> driver core. The driver core change is we add every device (that
>> happened to be created by DT) to the deferred probe list, so they
>> don't probe right away.
>
> The "that happened to be created by DT" part is of concern here. What is there
> that makes DT special in that respect? Why shouldn't that be applicable to
> devices created by the ACPI core, for example, or by a board file or something
> else?
DT is first. I think both examples could use this. Board files avoid
the problem by controlling the registration order with initcall levels
and just the call order in the code. You could come up with some way
to define dependencies for devices in board files and reuse this
mechanism. ACPI could use this as well if the dependencies are handled
in a similar way and it seems like they could be.
>> > Sure, we need OF-specific code and ACPI-specific code wherever different
>> > handling is required, but doing that at the driver core level seems to be
>> > a bit of a stretch to me.
>> >
>> > Please note that we don't really have ACPI-specific calls in the driver core,
>> > although we might have added them long ago even before the OF stuff appeared
>> > in the kernel for the first time. We didn't do that, (among other things)
>> > because we didn't want that particular firmware interface to appear special
>> > in any way and I'm not really sure why it is now OK to make OF look special
>> > instead.
>>
>> I don't think DT is special and we avoid DT specific core changes as
>> much as possible. I think the difference is DT uses platform_device
>> and ACPI does not.
>
> ACPI uses platform devices too. In fact, ACPI device objects are enumerated as
> platform devices by default now.
Okay, I should have grepped for that:
drivers/base/platform.c: ACPI_COMPANION_SET(&pdev->dev, NULL);
drivers/base/platform.c: len = acpi_device_modalias(dev, buf,
PAGE_SIZE -1);
drivers/base/platform.c: rc = acpi_device_uevent_modalias(dev, env);
drivers/base/platform.c: /* Then try ACPI style match */
drivers/base/platform.c: if (acpi_driver_match_device(dev, drv))
These are all cases which have DT version as well, so we're not really
all that different here. There's a few more for DT, but that probably
means you have just not hit the problems we have yet. For example,
what happens if you have an interrupt line in which the controller is
probed after the device connected to the interrupt line? That required
resolving irqs in platform_get_irq rather than using static resources
to support deferred probe.
Converting things like this to fwnode calls isn't hard to do. There
just hasn't been a pressing need or mandate to do so.
Rob
^ permalink raw reply
* Re: [GIT PULL] On-demand device probing
From: Alan Stern @ 2015-10-20 14:40 UTC (permalink / raw)
To: Rob Herring
Cc: Rafael J. Wysocki, David Woodhouse, Mark Brown,
Greg Kroah-Hartman, Tomeu Vizoso, Russell King, Michael Turquette,
Stephen Boyd, Vinod Koul, Dan Williams, Linus Walleij,
Alexandre Courbot, Thierry Reding, David Airlie,
Terje Bergström, Stephen Warren, Wolfram Sang, Frank Rowand,
Grant Likely, Kishon Vijay Abraham I, Sebastian Reichel
In-Reply-To: <CAL_JsqJuu5_Osqi+X6M6UeRDZFQB+_8riYDF1gvsGayk5-4SFw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, 20 Oct 2015, Rob Herring wrote:
> > The probe ordering is not the entire picture, though.
> >
> > Even if you get the probe ordering right, the problem is going to show up in
> > multiple other places: system suspend/resume, runtime PM, system shutdown,
> > unbinding of drivers. In all of those cases it is necessary to handle things
> > in a specific order if there is a dependency.
>
> My understanding was with deferred probe that it also solves suspend
> ordering problems because things are suspended in reverse order of
> probing.
Devices are suspended in reverse order of _registration_. Not of
probing.
Furthermore, that applies only to devices that use synchronous suspend.
Async suspend is becoming common, and there the only restrictions are
parent-child relations plus whatever explicit requirements that drivers
impose by calling device_pm_wait_for_dev().
Alan Stern
^ permalink raw reply
* Re: [GIT PULL] On-demand device probing
From: Mark Brown @ 2015-10-20 15:36 UTC (permalink / raw)
To: Alan Stern
Cc: Rob Herring, Rafael J. Wysocki, David Woodhouse,
Greg Kroah-Hartman, Tomeu Vizoso, Russell King, Michael Turquette,
Stephen Boyd, Vinod Koul, Dan Williams, Linus Walleij,
Alexandre Courbot, Thierry Reding, David Airlie,
Terje Bergström, Stephen Warren, Wolfram Sang, Frank Rowand,
Grant Likely, Kishon Vijay Abraham I, Sebastian Reichel
In-Reply-To: <Pine.LNX.4.44L0.1510201036290.1855-100000@iolanthe.rowland.org>
[-- Attachment #1: Type: text/plain, Size: 780 bytes --]
On Tue, Oct 20, 2015 at 10:40:03AM -0400, Alan Stern wrote:
> Furthermore, that applies only to devices that use synchronous suspend.
> Async suspend is becoming common, and there the only restrictions are
> parent-child relations plus whatever explicit requirements that drivers
> impose by calling device_pm_wait_for_dev().
Hrm, this is the first I'd noticed that feature though I see the initial
commit dates from January. It looks like most of the users are PCs at
the minute but we should be using it more widely for embedded things,
there's definitely some cases I'm aware of where it will allow us to
remove some open coding.
It does seem like we want to be feeding dependency information we
discover for probing way into the suspend dependencies...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: Alternative approach to solve the deferred probe (was: [GIT PULL] On-demand device probing)
From: Russell King - ARM Linux @ 2015-10-20 15:46 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Tomeu Vizoso, Mark Brown, Greg Kroah-Hartman, Rob Herring,
Michael Turquette, Stephen Boyd, Vinod Koul, Dan Williams,
Linus Walleij, Alexandre Courbot, Thierry Reding, David Airlie,
Terje Bergström, Stephen Warren, Wolfram Sang, Frank Rowand,
Grant Likely, Kishon Vijay Abraham I, Sebastian Reichel,
Dmitry Eremin-Solenikov, David Woodhouse, Liam Girdwood
In-Reply-To: <CAMuHMdVv=ieYM-Nehp0qV8CEggHEeJ22j-6i_N09rRA_U=2ePw@mail.gmail.com>
On Mon, Oct 19, 2015 at 06:21:40PM +0200, Geert Uytterhoeven wrote:
> Hi Russell,
>
> On Mon, Oct 19, 2015 at 5:35 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> >> > What you can do is print those devices which have failed to probe at
> >> > late_initcall() time - possibly augmenting that with reports from
> >> > subsystems showing what resources are not available, but that's only
> >> > a guide, because of the "it might or might not be in a kernel module"
> >> > problem.
> >>
> >> Well, adding those reports would give you a changelog similar to the
> >> one in this series...
> >
> > I'm not sure about that, because what I was thinking of is adding
> > a flag which would be set at late_initcall() time prior to running
> > a final round of deferred device probing.
>
> Which round is the final round?
> That's the one which didn't manage to bind any new devices to drivers,
> which is something you only know _after_ the round has been run.
>
> So I think we need one extra round to handle this.
>
> > This flag would then be used in a deferred_warn() printk function
> > which would normally be silent, but when this flag is set, it would
> > print the reason for the deferral - and this would replace (or be
> > added) to the subsystems and drivers which return -EPROBE_DEFER.
> >
> > That has the effect of hiding all the deferrals up until just before
> > launching into userspace, which should then acomplish two things -
> > firstly, getting rid of the rather useless deferred messages up to
> > that point, and secondly printing the reason why the remaining
> > deferrals are happening.
> >
> > That should be a small number of new lines plus a one-line change
> > in subsystems and drivers.
>
> Apart from the extra round we probably can't get rid of, that sounds OK to me.
Something like this. I haven't put a lot of effort into it to change all
the places which return an -EPROBE_DEFER, and it also looks like we need
some helpers to report when we have only an device_node (or should that
be fwnode?) See the commented out of_warn_deferred() in
drivers/gpio/gpiolib-of.c. Adding this stuff in the subsystems searching
for resources should make debugging why things are getting deferred easier.
We could make driver_deferred_probe_report something that can be
deactivated again after the last deferred probe run, and provide the
user with a knob that they can turn it back on again.
I've tried this out on two of my platforms, including forcing
driver_deferred_probe_report to be enabled, and I get exactly one
deferred probe, so not a particularly good test.
The patch won't apply as-is to mainline for all files; it's based on my
tree which has some 360 additional patches (which seems to be about
normal for my tree now.)
drivers/base/dd.c | 29 +++++++++++++++++++++++++++++
drivers/base/power/domain.c | 7 +++++--
drivers/clk/clkdev.c | 9 ++++++++-
drivers/gpio/gpiolib-of.c | 5 +++++
drivers/gpu/drm/bridge/dw_hdmi.c | 2 +-
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 2 +-
drivers/gpu/drm/imx/imx-ldb.c | 5 +++--
drivers/gpu/drm/msm/dsi/dsi.c | 2 +-
drivers/gpu/drm/msm/msm_drv.c | 3 ++-
drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 ++-
drivers/of/irq.c | 5 ++++-
drivers/pci/host/pci-mvebu.c | 1 +
drivers/pinctrl/core.c | 5 +++--
drivers/pinctrl/devicetree.c | 4 ++--
drivers/regulator/core.c | 5 +++--
include/linux/device.h | 1 +
16 files changed, 71 insertions(+), 17 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index be0eb4639128..bb12224f2901 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -129,7 +129,29 @@ void driver_deferred_probe_del(struct device *dev)
mutex_unlock(&deferred_probe_mutex);
}
+static bool driver_deferred_probe_report;
+
+/**
+ * dev_warn_deferred() - report why a probe has been deferred
+ */
+void dev_warn_deferred(struct device *dev, const char *fmt, ...)
+{
+ if (driver_deferred_probe_report) {
+ struct va_format vaf;
+ va_list ap;
+
+ va_start(ap, fmt);
+ vaf.fmt = fmt;
+ vaf.va = ≈
+
+ dev_warn(dev, "deferring probe: %pV", &vaf);
+ va_end(ap);
+ }
+}
+EXPORT_SYMBOL_GPL(dev_warn_deferred);
+
static bool driver_deferred_probe_enable = false;
+
/**
* driver_deferred_probe_trigger() - Kick off re-probing deferred devices
*
@@ -188,6 +210,13 @@ static int deferred_probe_initcall(void)
driver_deferred_probe_trigger();
/* Sort as many dependencies as possible before exiting initcalls */
flush_workqueue(deferred_wq);
+
+ /* Now one final round, reporting any devices that remain deferred */
+ driver_deferred_probe_report = true;
+ driver_deferred_probe_trigger();
+ /* Sort as many dependencies as possible before exiting initcalls */
+ flush_workqueue(deferred_wq);
+
return 0;
}
late_initcall(deferred_probe_initcall);
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 16550c63d611..9f4d687d7268 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1997,8 +1997,8 @@ int genpd_dev_pm_attach(struct device *dev)
pd = of_genpd_get_from_provider(&pd_args);
if (IS_ERR(pd)) {
- dev_dbg(dev, "%s() failed to find PM domain: %ld\n",
- __func__, PTR_ERR(pd));
+ dev_warn_deferred(dev, "%s() failed to find PM domain: %ld\n",
+ __func__, PTR_ERR(pd));
of_node_put(dev->of_node);
return -EPROBE_DEFER;
}
@@ -2026,6 +2026,9 @@ int genpd_dev_pm_attach(struct device *dev)
ret = pm_genpd_poweron(pd);
out:
+ if (ret)
+ dev_warn_deferred(dev, "%s() deferring probe: %d\n",
+ __func__, ret);
return ret ? -EPROBE_DEFER : 0;
}
EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 779b6ff0c7ad..66f4212c63fd 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -201,7 +201,14 @@ struct clk *clk_get(struct device *dev, const char *con_id)
if (dev) {
clk = __of_clk_get_by_name(dev->of_node, dev_id, con_id);
- if (!IS_ERR(clk) || PTR_ERR(clk) = -EPROBE_DEFER)
+ if (IS_ERR(clk) && PTR_ERR(clk) = -EPROBE_DEFER) {
+ if (dev)
+ dev_warn_deferred(dev,
+ "unable to locate clock for connection %s\n",
+ con_id);
+ return clk;
+ }
+ if (!IS_ERR(clk))
return clk;
}
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index fa6e3c8823d6..36f09ab1c215 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -101,6 +101,11 @@ struct gpio_desc *of_get_named_gpiod_flags(struct device_node *np,
pr_debug("%s: parsed '%s' property of node '%s[%d]' - status (%d)\n",
__func__, propname, np->full_name, index,
PTR_ERR_OR_ZERO(gg_data.out_gpio));
+
+// if (gg_data.out_gpio = -EPROBE_DEFER)
+// of_warn_deferred(np, "%s: unable to locate GPIO for %s[%d]\n",
+// __func__, propname, index);
+
return gg_data.out_gpio;
}
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index cb8764eecd70..088f5dd58424 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1785,7 +1785,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node);
of_node_put(ddc_node);
if (!hdmi->ddc) {
- dev_dbg(hdmi->dev, "failed to read ddc node\n");
+ dev_warn_deferred(hdmi->dev, "failed to read ddc node\n");
return -EPROBE_DEFER;
}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 12b03b364703..3155798d8245 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1899,7 +1899,7 @@ static int exynos_dsi_probe(struct platform_device *pdev)
ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(dsi->supplies),
dsi->supplies);
if (ret) {
- dev_info(dev, "failed to get regulators: %d\n", ret);
+ dev_warn_deferred(dev, "failed to get regulators: %d\n", ret);
return -EPROBE_DEFER;
}
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index abacc8f67469..0b57054c886a 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -595,8 +595,9 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
else
return -EPROBE_DEFER;
if (!channel->panel) {
- dev_err(dev, "panel not found: %s\n",
- remote->full_name);
+ dev_warn_deferred(dev,
+ "panel not found: %s\n",
+ remote->full_name);
return -EPROBE_DEFER;
}
}
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 6edcd6f57e70..3ba94a2bca65 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -42,7 +42,7 @@ static int dsi_get_phy(struct msm_dsi *msm_dsi)
of_node_put(phy_node);
if (!phy_pdev || !msm_dsi->phy) {
- dev_err(&pdev->dev, "%s: phy driver is not ready\n", __func__);
+ dev_warn_deferred(&pdev->dev, "%s: phy driver is not ready\n", __func__);
return -EPROBE_DEFER;
}
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 0339c5d82d37..e1cfcd38c0dd 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1117,7 +1117,8 @@ static int msm_pdev_probe(struct platform_device *pdev)
dev = bus_find_device_by_name(&platform_bus_type,
NULL, devnames[i]);
if (!dev) {
- dev_info(&pdev->dev, "still waiting for %s\n", devnames[i]);
+ dev_warn_deferred(&pdev->dev, "waiting for %s\n",
+ devnames[i]);
return -EPROBE_DEFER;
}
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 48cb19949ca3..bbf36f68d4e0 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -600,7 +600,8 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index)
if (!IS_ERR(clk)) {
rcrtc->extclock = clk;
} else if (PTR_ERR(rcrtc->clock) = -EPROBE_DEFER) {
- dev_info(rcdu->dev, "can't get external clock %u\n", index);
+ dev_warn_deferred(rcdu->dev, "can't get external clock %u\n",
+ index);
return -EPROBE_DEFER;
}
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 55317fa9c9dc..2056bb9e4c43 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -404,8 +404,11 @@ int of_irq_get(struct device_node *dev, int index)
return rc;
domain = irq_find_host(oirq.np);
- if (!domain)
+ if (!domain) {
+ dev_warn_deferred(dev, "%s() failed to locate IRQ domain\n",
+ __func__);
return -EPROBE_DEFER;
+ }
return irq_create_of_mapping(&oirq);
}
diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 0e9b82095dc9..b49ae4822a5b 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -1203,6 +1203,7 @@ static int mvebu_pcie_parse_port(struct mvebu_pcie *pcie,
reset_gpio = of_get_named_gpio_flags(child, "reset-gpios", 0, &flags);
if (reset_gpio = -EPROBE_DEFER) {
+ dev_warn_deferred(dev, "unable to find reset gpio\n");
ret = reset_gpio;
goto err;
}
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 9638a00c67c2..299aae3bca14 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -741,8 +741,9 @@ static int add_setting(struct pinctrl *p, struct pinctrl_map const *map)
* OK let us guess that the driver is not there yet, and
* let's defer obtaining this pinctrl handle to later...
*/
- dev_info(p->dev, "unknown pinctrl device %s in map entry, deferring probe",
- map->ctrl_dev_name);
+ dev_warn_deferred(p->dev,
+ "unknown pinctrl device %s in map entry, deferring probe",
+ map->ctrl_dev_name);
return -EPROBE_DEFER;
}
diff --git a/drivers/pinctrl/devicetree.c b/drivers/pinctrl/devicetree.c
index fe04e748dfe4..358f946471c9 100644
--- a/drivers/pinctrl/devicetree.c
+++ b/drivers/pinctrl/devicetree.c
@@ -115,8 +115,8 @@ static int dt_to_map_one_config(struct pinctrl *p, const char *statename,
for (;;) {
np_pctldev = of_get_next_parent(np_pctldev);
if (!np_pctldev || of_node_is_root(np_pctldev)) {
- dev_info(p->dev, "could not find pctldev for node %s, deferring probe\n",
- np_config->full_name);
+ dev_warn_deferred(p->dev, "could not find pctldev for node %s, deferring probe\n",
+ np_config->full_name);
of_node_put(np_pctldev);
/* OK let's just assume this will appear later then */
return -EPROBE_DEFER;
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 8a34f6acc801..8d8ea0518283 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1410,8 +1410,9 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
if (have_full_constraints()) {
r = dummy_regulator_rdev;
} else {
- dev_err(dev, "Failed to resolve %s-supply for %s\n",
- rdev->supply_name, rdev->desc->name);
+ dev_warn_deferred(dev,
+ "Failed to resolve %s-supply for %s\n",
+ rdev->supply_name, rdev->desc->name);
return -EPROBE_DEFER;
}
}
diff --git a/include/linux/device.h b/include/linux/device.h
index 5d7bc6349930..5050ce7d73b3 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1087,6 +1087,7 @@ extern void device_shutdown(void);
/* debugging and troubleshooting/diagnostic helpers. */
extern const char *dev_driver_string(const struct device *dev);
+void dev_warn_deferred(struct device *dev, const char *fmt, ...);
#ifdef CONFIG_PRINTK
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply related
* Re: [GIT PULL] On-demand device probing
From: Alan Stern @ 2015-10-20 16:04 UTC (permalink / raw)
To: Mark Brown
Cc: Rob Herring, Rafael J. Wysocki, David Woodhouse,
Greg Kroah-Hartman, Tomeu Vizoso, Russell King, Michael Turquette,
Stephen Boyd, Vinod Koul, Dan Williams, Linus Walleij,
Alexandre Courbot, Thierry Reding, David Airlie,
Terje Bergström, Stephen Warren, Wolfram Sang, Frank Rowand,
Grant Likely, Kishon Vijay Abraham I, Sebastian Reichel
In-Reply-To: <20151020153612.GB32054@sirena.org.uk>
On Tue, 20 Oct 2015, Mark Brown wrote:
> On Tue, Oct 20, 2015 at 10:40:03AM -0400, Alan Stern wrote:
>
> > Furthermore, that applies only to devices that use synchronous suspend.
> > Async suspend is becoming common, and there the only restrictions are
> > parent-child relations plus whatever explicit requirements that drivers
> > impose by calling device_pm_wait_for_dev().
>
> Hrm, this is the first I'd noticed that feature though I see the initial
> commit dates from January.
Async suspend and device_pm_wait_for_dev() were added in January 2010,
not 2015!
> It looks like most of the users are PCs at
> the minute but we should be using it more widely for embedded things,
> there's definitely some cases I'm aware of where it will allow us to
> remove some open coding.
>
> It does seem like we want to be feeding dependency information we
> discover for probing way into the suspend dependencies...
Rafael has been thinking about a way to do this systematically.
Nothing concrete has emerged yet.
Alan Stern
^ permalink raw reply
* Re: [GIT PULL] On-demand device probing
From: Tomeu Vizoso @ 2015-10-20 16:21 UTC (permalink / raw)
To: Alan Stern
Cc: Mark Brown, Rob Herring, Rafael J. Wysocki, David Woodhouse,
Greg Kroah-Hartman, Russell King, Michael Turquette, Stephen Boyd,
Vinod Koul, Dan Williams, Linus Walleij, Alexandre Courbot,
Thierry Reding, David Airlie, Terje Bergström,
Stephen Warren, Wolfram Sang, Frank Rowand, Grant Likely,
Kishon Vijay Abraham I, Sebastian Reichel,
Dmitry Eremin-Solenikov
In-Reply-To: <Pine.LNX.4.44L0.1510201159400.1855-100000@iolanthe.rowland.org>
On 20 October 2015 at 18:04, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 20 Oct 2015, Mark Brown wrote:
>
>> On Tue, Oct 20, 2015 at 10:40:03AM -0400, Alan Stern wrote:
>>
>> > Furthermore, that applies only to devices that use synchronous suspend.
>> > Async suspend is becoming common, and there the only restrictions are
>> > parent-child relations plus whatever explicit requirements that drivers
>> > impose by calling device_pm_wait_for_dev().
>>
>> Hrm, this is the first I'd noticed that feature though I see the initial
>> commit dates from January.
>
> Async suspend and device_pm_wait_for_dev() were added in January 2010,
> not 2015!
>
>> It looks like most of the users are PCs at
>> the minute but we should be using it more widely for embedded things,
>> there's definitely some cases I'm aware of where it will allow us to
>> remove some open coding.
>>
>> It does seem like we want to be feeding dependency information we
>> discover for probing way into the suspend dependencies...
>
> Rafael has been thinking about a way to do this systematically.
> Nothing concrete has emerged yet.
This iteration of the series would make this quite easy, as
dependencies are calculated before probes are attempted:
https://lkml.org/lkml/2015/6/17/311
Regards,
Tomeu
> Alan Stern
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [GIT PULL] On-demand device probing
From: Alan Stern @ 2015-10-20 17:14 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: Mark Brown, Rob Herring, Rafael J. Wysocki, David Woodhouse,
Greg Kroah-Hartman, Russell King, Michael Turquette, Stephen Boyd,
Vinod Koul, Dan Williams, Linus Walleij, Alexandre Courbot,
Thierry Reding, David Airlie, Terje Bergström,
Stephen Warren, Wolfram Sang, Frank Rowand, Grant Likely,
Kishon Vijay Abraham I, Sebastian Reichel,
Dmitry Eremin-Solenikov
In-Reply-To: <CAAObsKChQN4gg91dCWJS_Co_MrYyBrB1J7gxgGo4hsPXNZRp3g@mail.gmail.com>
On Tue, 20 Oct 2015, Tomeu Vizoso wrote:
> On 20 October 2015 at 18:04, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, 20 Oct 2015, Mark Brown wrote:
> >
> >> On Tue, Oct 20, 2015 at 10:40:03AM -0400, Alan Stern wrote:
> >>
> >> > Furthermore, that applies only to devices that use synchronous suspend.
> >> > Async suspend is becoming common, and there the only restrictions are
> >> > parent-child relations plus whatever explicit requirements that drivers
> >> > impose by calling device_pm_wait_for_dev().
> >>
> >> Hrm, this is the first I'd noticed that feature though I see the initial
> >> commit dates from January.
> >
> > Async suspend and device_pm_wait_for_dev() were added in January 2010,
> > not 2015!
> >
> >> It looks like most of the users are PCs at
> >> the minute but we should be using it more widely for embedded things,
> >> there's definitely some cases I'm aware of where it will allow us to
> >> remove some open coding.
> >>
> >> It does seem like we want to be feeding dependency information we
> >> discover for probing way into the suspend dependencies...
> >
> > Rafael has been thinking about a way to do this systematically.
> > Nothing concrete has emerged yet.
>
> This iteration of the series would make this quite easy, as
> dependencies are calculated before probes are attempted:
>
> https://lkml.org/lkml/2015/6/17/311
But what Rafael is proposing is quite general; it would apply to _all_
dependencies as opposed to just those present in DT drivers or those
affecting platform_devices.
Alan Stern
^ permalink raw reply
* Re: [GIT PULL] On-demand device probing
From: Mark Brown @ 2015-10-20 19:35 UTC (permalink / raw)
To: Alan Stern
Cc: Tomeu Vizoso, Rob Herring, Rafael J. Wysocki, David Woodhouse,
Greg Kroah-Hartman, Russell King, Michael Turquette, Stephen Boyd,
Vinod Koul, Dan Williams, Linus Walleij, Alexandre Courbot,
Thierry Reding, David Airlie, Terje Bergström,
Stephen Warren, Wolfram Sang, Frank Rowand, Grant Likely,
Kishon Vijay Abraham I, Sebastian Reichel
In-Reply-To: <Pine.LNX.4.44L0.1510201311260.1855-100000@iolanthe.rowland.org>
[-- Attachment #1: Type: text/plain, Size: 909 bytes --]
On Tue, Oct 20, 2015 at 01:14:46PM -0400, Alan Stern wrote:
> On Tue, 20 Oct 2015, Tomeu Vizoso wrote:
> > This iteration of the series would make this quite easy, as
> > dependencies are calculated before probes are attempted:
> > https://lkml.org/lkml/2015/6/17/311
> But what Rafael is proposing is quite general; it would apply to _all_
> dependencies as opposed to just those present in DT drivers or those
> affecting platform_devices.
We'll still need most of the DT bits that are there at the minute (the
ones strewn around the subsystems) AFAICT since it's at the point where
we parse the DT and work out what the dependencies are which we probably
want to do prior to getting the drivers up and will be different for
ACPI. I think the level of DT dependency here looks a lot larger than
it actually is due to the fact that a lot of what's being modified is DT
parsing code.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [PATCH] fbcon: initialize blink interval before calling fb_set_par
From: Benjamin Herrenschmidt @ 2015-10-20 21:00 UTC (permalink / raw)
To: Scot Doyle, Tomi Valkeinen, Jean-Christophe Plagniol-Villard
Cc: Greg Kroah-Hartman, Alistair Popple, Pavel Machek, airlied,
linux-fbdev, linux-kernel
In-Reply-To: <alpine.DEB.2.11.1510091500100.1816@local>
On Fri, 2015-10-09 at 15:08 +0000, Scot Doyle wrote:
> Since commit 27a4c827c34ac4256a190cc9d24607f953c1c459
> fbcon: use the cursor blink interval provided by vt
>
> a PPC64LE kernel fails to boot when fbcon_add_cursor_timer uses an
> uninitialized ops->cur_blink_jiffies. Prevent by initializing
> in fbcon_init before the call to info->fbops->fb_set_par.
Any reason that hasn't hit upstream (and stable) yet ? This is pretty
major...
> Reported-and-tested-by: Alistair Popple <alistair@popple.id.au>
> Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
> ---
> drivers/video/console/fbcon.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/video/console/fbcon.c
> b/drivers/video/console/fbcon.c
> index 1aaf893..92f3949 100644
> --- a/drivers/video/console/fbcon.c
> +++ b/drivers/video/console/fbcon.c
> @@ -1093,6 +1093,7 @@ static void fbcon_init(struct vc_data *vc, int
> init)
> con_copy_unimap(vc, svc);
>
> ops = info->fbcon_par;
> + ops->cur_blink_jiffies = msecs_to_jiffies(vc
> ->vc_cur_blink_ms);
> p->con_rotate = initial_rotation;
> set_blitting_type(vc, info);
>
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux
> -fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] fbcon: initialize blink interval before calling fb_set_par
From: Scot Doyle @ 2015-10-20 22:04 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Greg Kroah-Hartman, Alistair Popple, Pavel Machek, airlied,
linux-fbdev, linux-kernel
In-Reply-To: <1445374804.3025.16.camel@kernel.crashing.org>
On Wed, 21 Oct 2015, Benjamin Herrenschmidt wrote:
> On Fri, 2015-10-09 at 15:08 +0000, Scot Doyle wrote:
> > Since commit 27a4c827c34ac4256a190cc9d24607f953c1c459
> > fbcon: use the cursor blink interval provided by vt
> >
> > a PPC64LE kernel fails to boot when fbcon_add_cursor_timer uses an
> > uninitialized ops->cur_blink_jiffies. Prevent by initializing
> > in fbcon_init before the call to info->fbops->fb_set_par.
>
> Any reason that hasn't hit upstream (and stable) yet ? This is pretty
> major...
It's currently in the tty-linus branch of Greg's tty tree, so should be
included in the next rc.
^ permalink raw reply
* Re: [GIT PULL] On-demand device probing
From: Rafael J. Wysocki @ 2015-10-20 23:34 UTC (permalink / raw)
To: Rob Herring
Cc: David Woodhouse, Mark Brown, Greg Kroah-Hartman, Tomeu Vizoso,
Russell King, Michael Turquette, Stephen Boyd, Vinod Koul,
Dan Williams, Linus Walleij, Alexandre Courbot, Thierry Reding,
David Airlie, Terje Bergström, Stephen Warren, Wolfram Sang,
Frank Rowand, Grant Likely, Kishon Vijay Abraham I,
Sebastian Reichel, Dmitry Eremin-Solenikov
In-Reply-To: <CAL_JsqJuu5_Osqi+X6M6UeRDZFQB+_8riYDF1gvsGayk5-4SFw@mail.gmail.com>
On Tuesday, October 20, 2015 09:15:01 AM Rob Herring wrote:
> On Tue, Oct 20, 2015 at 2:56 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Monday, October 19, 2015 05:58:40 PM Rob Herring wrote:
> >> On Mon, Oct 19, 2015 at 4:40 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Monday, October 19, 2015 10:58:25 AM Rob Herring wrote:
> >> >> On Mon, Oct 19, 2015 at 10:29 AM, David Woodhouse <dwmw2@infradead.org> wrote:
> >> >> > On Mon, 2015-10-19 at 15:50 +0100, Mark Brown wrote:
> >> >> >> > But the point I'm making is that we are working towards *fixing* that,
> >> >> >> > and *not* using DT-specific code in places where we should be using the
> >> >> >> > generic APIs.
> >> >> >>
> >> >> >> What is the plan for fixing things here? It's not obvious (at least to
> >> >> >> me) that we don't want to have the subsystems having knowledge of how
> >> >> >> they are bound to a specific firmware which is what you seem to imply
> >> >> >> here.
> >> >> >
> >> >> > I don't know that there *is* a coherent plan here to address it all.
> >> >> >
> >> >> > Certainly, we *will* need subsystems to have firmware-specific
> >> >> > knowledge in some cases. Take GPIO as an example; ACPI *has* a way to
> >> >> > describe GPIO, and properties which reference GPIO pins are intended to
> >> >> > work through that — while in DT, properties which reference GPIO pins
> >> >> > will have different contents. They'll be compatible at the driver
> >> >> > level, in the sense that there's a call to get a given GPIO given the
> >> >> > property name, but the subsystems *will* be doing different things
> >> >> > behind the scenes.
> >> >> >
> >> >> > My plan, such as it is, is to go through the leaf-node drivers which
> >> >> > almost definitely *should* be firmware-agnostic, and convert those. And
> >> >> > then take stock of what we have left, and work out what, if anything,
> >> >> > still needs to be done.
> >> >>
> >> >> Many cases are already agnostic in the drivers in terms of the *_get()
> >> >> functions. Some are DT specific, but probably because those subsystems
> >> >> are new and DT only. In any case, I don't think these 1 line changes
> >> >> do anything to make doing conversions here harder.
> >> >>
> >> >> >> It seems like we're going to have to refactor these bits of code when
> >> >> >> they get generalised anyway so I'm not sure that the additional cost
> >> >> >> here is that big.
> >> >> >
> >> >> > That's an acceptable answer — "we're adding legacy code here but we
> >> >> > know it's going to be refactored anyway". If that's true, all it takes
> >> >> > is a note in the commit comment to that effect. That's different from
> >> >> > having not thought about it :)
> >> >>
> >> >> Considering at one point we did create a fwnode based API, we did
> >> >> think about it. Plus there was little input from ACPI folks as to
> >> >> whether the change was even useful for ACPI case.
> >> >
> >> > Well, sorry, but who was asking whom, specifically?
> >>
> >> You and linux-acpi have been copied on v2 and later of the entire
> >> series I think.
> >
> > Yes, but it wasn't like a direct request, say "We need your input, so can you
> > please have a look and BTW we want this in 4.4, so please do it ASAP". In
> > which case I'd prioritize that before other things I needed to take care of.
>
> Fair enough. Can you please review and comment on v7 of the series? We
> can discuss at KS as well.
I'll do that.
I'm in the middle of travel right now and speaking at a conference tomorrow,
so I may not be able to get to that for a couple of days, but will do my best
to do it as soon as I possibly can.
> >> > The underlying problem is present in ACPI too and we don't really have a good
> >> > solution for it. We might benefit from a common one if it existed.
> >>
> >> The problem for DT is we don't generically know what are the
> >> dependencies at a core level. We could know some or most dependencies
> >> if phandles (links to other nodes) were typed, but they are not. If
> >> the core had this information, we could simply control the device
> >> creation to order probing. Instead, this information is encoded into
> >> the bindings and binding parsing resides in the subsystems. That
> >> parsing happens during probe of the client side and is done by the
> >> subsystems (for common bindings). Since we already do the parsing at
> >> this point, it is a convenient place to trigger the probe of the
> >> dependency. Is ACPI going to be similar in this regard?
> >
> > It is similar in some ways. For example, if a device's functionality depends
> > on an I2C resource (connection), the core doesn't know that at the device
> > creation time at least in some cases. Same for GPIO, SPI, DMA engines etc.
>
> So you will need to create devices, defer their probing and then probe
> on demand as well unless you have other ideas how you would do it.
Right.
> > There is a _DEP object in ACPI that can be used by firmware to tell the OS
> > about those dependencies, but there's no way in the driver core to use that
> > information anyway today.
>
> I would think that the equivalent function for ACPI to of_device_probe
> could process these if they are generic and you can associate the
> dependency to a struct device.
Well, something along these lines probably.
> >> Fundamentally, it is a question of probe devices when their
> >> dependencies are present or drivers ensure their dependencies are
> >> ready. IIRC, init systems went thru a similar debate for service
> >> dependencies.
> >
> > The probe ordering is not the entire picture, though.
> >
> > Even if you get the probe ordering right, the problem is going to show up in
> > multiple other places: system suspend/resume, runtime PM, system shutdown,
> > unbinding of drivers. In all of those cases it is necessary to handle things
> > in a specific order if there is a dependency.
>
> My understanding was with deferred probe that it also solves suspend
> ordering problems because things are suspended in reverse order of
> probing. I suppose you could have slightly different dependencies for
> suspend, runtime PM, etc. than for probe? Perhaps we need to save the
> list of dependencies as we probe them. I don't think that would be too
> hard to add on to this series, but then if we don't need it now, why
> add it?
As Alan said, there are two problems here. First off, the ordering of the
list used by system suspend/resume is the registration ordering, not the
probe ordering. Moreover, though, even if we get the ordering right, it
still is not sufficient for devices with async_suspend set.
To address this, the core will have to make the involved async threads wait
for each other in accordance with the dependencies too. That, in turn, is
very close to what's needed for runtime PM.
> >> >> In any case, we're talking about adding 1 line.
> >> >
> >> > But also about making the driver core slighly OF-centric.
> >>
> >> How so? The one line is in DT binding parsing code in subsystems, not
> >> driver core. The driver core change is we add every device (that
> >> happened to be created by DT) to the deferred probe list, so they
> >> don't probe right away.
> >
> > The "that happened to be created by DT" part is of concern here. What is there
> > that makes DT special in that respect? Why shouldn't that be applicable to
> > devices created by the ACPI core, for example, or by a board file or something
> > else?
>
> DT is first. I think both examples could use this. Board files avoid
> the problem by controlling the registration order with initcall levels
> and just the call order in the code. You could come up with some way
> to define dependencies for devices in board files and reuse this
> mechanism. ACPI could use this as well if the dependencies are handled
> in a similar way and it seems like they could be.
>
> >> > Sure, we need OF-specific code and ACPI-specific code wherever different
> >> > handling is required, but doing that at the driver core level seems to be
> >> > a bit of a stretch to me.
> >> >
> >> > Please note that we don't really have ACPI-specific calls in the driver core,
> >> > although we might have added them long ago even before the OF stuff appeared
> >> > in the kernel for the first time. We didn't do that, (among other things)
> >> > because we didn't want that particular firmware interface to appear special
> >> > in any way and I'm not really sure why it is now OK to make OF look special
> >> > instead.
> >>
> >> I don't think DT is special and we avoid DT specific core changes as
> >> much as possible. I think the difference is DT uses platform_device
> >> and ACPI does not.
> >
> > ACPI uses platform devices too. In fact, ACPI device objects are enumerated as
> > platform devices by default now.
>
> Okay, I should have grepped for that:
> drivers/base/platform.c: ACPI_COMPANION_SET(&pdev->dev, NULL);
> drivers/base/platform.c: len = acpi_device_modalias(dev, buf,
> PAGE_SIZE -1);
> drivers/base/platform.c: rc = acpi_device_uevent_modalias(dev, env);
> drivers/base/platform.c: /* Then try ACPI style match */
> drivers/base/platform.c: if (acpi_driver_match_device(dev, drv))
>
> These are all cases which have DT version as well, so we're not really
> all that different here. There's a few more for DT, but that probably
> means you have just not hit the problems we have yet. For example,
> what happens if you have an interrupt line in which the controller is
> probed after the device connected to the interrupt line? That required
> resolving irqs in platform_get_irq rather than using static resources
> to support deferred probe.
We don't have this particular problem, because the IRQ controllers are
enumerated in a special way.
> Converting things like this to fwnode calls isn't hard to do. There
> just hasn't been a pressing need or mandate to do so.
Well, to me the problem is actually generic, so it is better to use generic
concepts to start with when trying to address it where that doesn't add too
much overhead. Otherwise it's very easy to lose the broader context from
one's sight and then to start cutting corners.
Thanks,
Rafael
^ permalink raw reply
* Re: [GIT PULL] On-demand device probing
From: Rafael J. Wysocki @ 2015-10-20 23:35 UTC (permalink / raw)
To: Mark Brown
Cc: Alan Stern, Tomeu Vizoso, Rob Herring, David Woodhouse,
Greg Kroah-Hartman, Russell King, Michael Turquette, Stephen Boyd,
Vinod Koul, Dan Williams, Linus Walleij, Alexandre Courbot,
Thierry Reding, David Airlie, Terje Bergström,
Stephen Warren, Wolfram Sang, Frank Rowand, Grant Likely,
Kishon Vijay Abraham I, Sebastian Reichel
In-Reply-To: <20151020193528.GE32054@sirena.org.uk>
On Tuesday, October 20, 2015 08:35:28 PM Mark Brown wrote:
>
> --7fVr/IRGAG9sAW4J
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
>
> On Tue, Oct 20, 2015 at 01:14:46PM -0400, Alan Stern wrote:
> > On Tue, 20 Oct 2015, Tomeu Vizoso wrote:
>
> > > This iteration of the series would make this quite easy, as
> > > dependencies are calculated before probes are attempted:
>
> > > https://lkml.org/lkml/2015/6/17/311
>
> > But what Rafael is proposing is quite general; it would apply to _all_
> > dependencies as opposed to just those present in DT drivers or those
> > affecting platform_devices.
>
> We'll still need most of the DT bits that are there at the minute (the
> ones strewn around the subsystems) AFAICT since it's at the point where
> we parse the DT and work out what the dependencies are which we probably
> want to do prior to getting the drivers up and will be different for
> ACPI. I think the level of DT dependency here looks a lot larger than
> it actually is due to the fact that a lot of what's being modified is DT
> parsing code.
Right, something will have to register the dependency, or a "link" between
devices, with the core once we find out that the dependency is there.
Thanks,
Rafael
^ permalink raw reply
* Re: Alternative approach to solve the deferred probe
From: Frank Rowand @ 2015-10-21 3:58 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Geert Uytterhoeven, Tomeu Vizoso, Mark Brown, Greg Kroah-Hartman,
Rob Herring, Michael Turquette, Stephen Boyd, Vinod Koul,
Dan Williams, Linus Walleij, Alexandre Courbot, Thierry Reding,
David Airlie, Terje Bergström, Stephen Warren, Wolfram Sang,
Grant Likely, Kishon Vijay Abraham I, Sebastian Reichel,
Dmitry Eremin-Solenikov, David Woodhouse
In-Reply-To: <20151020154656.GY32532@n2100.arm.linux.org.uk>
On 10/20/2015 8:46 AM, Russell King - ARM Linux wrote:
> On Mon, Oct 19, 2015 at 06:21:40PM +0200, Geert Uytterhoeven wrote:
>> Hi Russell,
>>
>> On Mon, Oct 19, 2015 at 5:35 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>>>> What you can do is print those devices which have failed to probe at
>>>>> late_initcall() time - possibly augmenting that with reports from
>>>>> subsystems showing what resources are not available, but that's only
>>>>> a guide, because of the "it might or might not be in a kernel module"
>>>>> problem.
>>>>
>>>> Well, adding those reports would give you a changelog similar to the
>>>> one in this series...
>>>
>>> I'm not sure about that, because what I was thinking of is adding
>>> a flag which would be set at late_initcall() time prior to running
>>> a final round of deferred device probing.
>>
>> Which round is the final round?
>> That's the one which didn't manage to bind any new devices to drivers,
>> which is something you only know _after_ the round has been run.
>>
>> So I think we need one extra round to handle this.
>>
>>> This flag would then be used in a deferred_warn() printk function
>>> which would normally be silent, but when this flag is set, it would
>>> print the reason for the deferral - and this would replace (or be
>>> added) to the subsystems and drivers which return -EPROBE_DEFER.
>>>
>>> That has the effect of hiding all the deferrals up until just before
>>> launching into userspace, which should then acomplish two things -
>>> firstly, getting rid of the rather useless deferred messages up to
>>> that point, and secondly printing the reason why the remaining
>>> deferrals are happening.
>>>
>>> That should be a small number of new lines plus a one-line change
>>> in subsystems and drivers.
>>
>> Apart from the extra round we probably can't get rid of, that sounds OK to me.
>
> Something like this. I haven't put a lot of effort into it to change all
> the places which return an -EPROBE_DEFER, and it also looks like we need
> some helpers to report when we have only an device_node (or should that
> be fwnode?) See the commented out of_warn_deferred() in
> drivers/gpio/gpiolib-of.c. Adding this stuff in the subsystems searching
> for resources should make debugging why things are getting deferred easier.
>
> We could make driver_deferred_probe_report something that can be
> deactivated again after the last deferred probe run, and provide the
> user with a knob that they can turn it back on again.
>
> I've tried this out on two of my platforms, including forcing
> driver_deferred_probe_report to be enabled, and I get exactly one
> deferred probe, so not a particularly good test.
>
> The patch won't apply as-is to mainline for all files; it's based on my
> tree which has some 360 additional patches (which seems to be about
> normal for my tree now.)
I like the concept (I have been thinking along similar lines lately).
But I think this might make the console messages more confusing than
they are now. The problem is that debug, warn, and error messages
come from a somewhat random set of locations at the moment. Some
come from the driver probe routines and some come from the subsystems
that the probe routines call. So the patch is suppressing some
messages, but not others.
One thing that seemed pretty obvious from the patches is that the
current probe routines are somewhat inconsistent in terms of messages,
and that there is room for a set of best practices for messaging. That
is on my long term wish list, but I'm not sure I'll ever chase after
those windmills.
A couple of specific comments below.
>
> drivers/base/dd.c | 29 +++++++++++++++++++++++++++++
> drivers/base/power/domain.c | 7 +++++--
> drivers/clk/clkdev.c | 9 ++++++++-
> drivers/gpio/gpiolib-of.c | 5 +++++
> drivers/gpu/drm/bridge/dw_hdmi.c | 2 +-
> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 2 +-
> drivers/gpu/drm/imx/imx-ldb.c | 5 +++--
> drivers/gpu/drm/msm/dsi/dsi.c | 2 +-
> drivers/gpu/drm/msm/msm_drv.c | 3 ++-
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 3 ++-
> drivers/of/irq.c | 5 ++++-
> drivers/pci/host/pci-mvebu.c | 1 +
> drivers/pinctrl/core.c | 5 +++--
> drivers/pinctrl/devicetree.c | 4 ++--
> drivers/regulator/core.c | 5 +++--
> include/linux/device.h | 1 +
> 16 files changed, 71 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index be0eb4639128..bb12224f2901 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -129,7 +129,29 @@ void driver_deferred_probe_del(struct device *dev)
> mutex_unlock(&deferred_probe_mutex);
> }
>
> +static bool driver_deferred_probe_report;
> +
> +/**
> + * dev_warn_deferred() - report why a probe has been deferred
> + */
> +void dev_warn_deferred(struct device *dev, const char *fmt, ...)
> +{
> + if (driver_deferred_probe_report) {
> + struct va_format vaf;
> + va_list ap;
> +
> + va_start(ap, fmt);
> + vaf.fmt = fmt;
> + vaf.va = ≈
> +
> + dev_warn(dev, "deferring probe: %pV", &vaf);
> + va_end(ap);
> + }
> +}
> +EXPORT_SYMBOL_GPL(dev_warn_deferred);
The places where dev_warn_deferred() replaces dev_dbg(), we lose the
ability to turn on debugging and observe the driver reporting the
specific reason the deferral is occurring. So it would be useful to
add an "else dev_dbg()" in dev_warn_deferred() to retain that capability.
> +
> static bool driver_deferred_probe_enable = false;
> +
> /**
> * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
> *
> @@ -188,6 +210,13 @@ static int deferred_probe_initcall(void)
> driver_deferred_probe_trigger();
Couldn't you put the "driver_deferred_probe_report = true" here? And then
not add another round of probes.
> /* Sort as many dependencies as possible before exiting initcalls */
> flush_workqueue(deferred_wq);
> +
> + /* Now one final round, reporting any devices that remain deferred */
> + driver_deferred_probe_report = true;
> + driver_deferred_probe_trigger();
> + /* Sort as many dependencies as possible before exiting initcalls */
> + flush_workqueue(deferred_wq);
> +
> return 0;
> }
< snip >
-Frank
^ permalink raw reply
* [PATCH RFC v2 0/2] simplefb: Add regulator handling support
From: Chen-Yu Tsai @ 2015-10-21 5:58 UTC (permalink / raw)
To: linux-arm-kernel
Hi everyone,
This is v2 of the simplefb regulator support series. This series adds
regulator claiming and enabling support for simplefb.
Sometimes the simplefb display output path consits of external conversion
chips and/or LCD drivers and backlights. These devices normally have
GPIOs to turn them on and/or bring them out of reset, and regulators
supplying power to them.
While the kernel does not touch unclaimed GPIOs, the regulator core
happily disables unused regulators. Thus we need simplefb to claim
and enable the regulators used throughout the display pipeline.
Instead of the unnamed "vinN-supply" properties, v2 supports any named
regulator supplies under its device node. It will look through its
properties, and claim any regulators by matching "*-supply", as Mark
suggested.
I've not done a generic helper in the regulator core yet, instead doing
the regulator property handling in the simplefb code for now.
Patch 1 adds the regulator properties to the DT binding.
Patch 2 adds code to the simplefb driver to claim and enable regulators.
Regards
ChenYu
Chen-Yu Tsai (2):
dt-bindings: simplefb: Support a list of regulator supply properties
simplefb: Claim and enable regulators
.../bindings/video/simple-framebuffer.txt | 13 ++-
drivers/video/fbdev/simplefb.c | 122 ++++++++++++++++++++-
2 files changed, 130 insertions(+), 5 deletions(-)
--
2.6.1
^ permalink raw reply
* [PATCH RFC v2 1/2] dt-bindings: simplefb: Support a list of regulator supply properties
From: Chen-Yu Tsai @ 2015-10-21 5:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1445407141-16459-1-git-send-email-wens@csie.org>
The physical display tied to the framebuffer may have regulators
providing power to it, such as power for LCDs or interface conversion
chips.
The number of regulators in use may vary, but the regulator supply
binding can not be a list. Work around this by adding a "num-supplies"
property to communicate the number of supplies, and a list of 0 ~ N
"vinN-supply" properties for the actual regulator supply.
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
.../devicetree/bindings/video/simple-framebuffer.txt | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 4474ef6e0b95..8c9e9f515c87 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -47,10 +47,14 @@ Required properties:
- a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
Optional properties:
-- clocks : List of clocks used by the framebuffer. Clocks listed here
- are expected to already be configured correctly. The OS must
- ensure these clocks are not modified or disabled while the
- simple framebuffer remains active.
+- clocks : List of clocks used by the framebuffer.
+- *-supply : Any number of regulators used by the framebuffer. These should
+ be named according to the names in the device's design.
+
+ The above resources are expected to already be configured correctly.
+ The OS must ensure they are not modified or disabled while the simple
+ framebuffer remains active.
+
- display : phandle pointing to the primary display hardware node
Example:
@@ -68,6 +72,7 @@ chosen {
stride = <(1600 * 2)>;
format = "r5g6b5";
clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
+ lcd-supply = <®_dc1sw>;
display = <&lcdc0>;
};
stdout-path = "display0";
--
2.6.1
^ permalink raw reply related
* [PATCH RFC v2 2/2] simplefb: Claim and enable regulators
From: Chen-Yu Tsai @ 2015-10-21 5:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1445407141-16459-1-git-send-email-wens@csie.org>
This claims and enables regulators listed in the simple framebuffer dt
node. This is needed so that regulators powering the display pipeline
and external hardware, described in the device node and known by the
kernel code, will remain properly enabled.
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
drivers/video/fbdev/simplefb.c | 122 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 121 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 52c5c7e63b52..c4ee10d83a70 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -28,7 +28,10 @@
#include <linux/platform_device.h>
#include <linux/clk.h>
#include <linux/clk-provider.h>
+#include <linux/of.h>
#include <linux/of_platform.h>
+#include <linux/parser.h>
+#include <linux/regulator/consumer.h>
static struct fb_fix_screeninfo simplefb_fix = {
.id = "simple",
@@ -174,6 +177,10 @@ struct simplefb_par {
int clk_count;
struct clk **clks;
#endif
+#if defined CONFIG_OF && defined CONFIG_REGULATOR
+ u32 regulator_count;
+ struct regulator **regulators;
+#endif
};
#if defined CONFIG_OF && defined CONFIG_COMMON_CLK
@@ -269,6 +276,112 @@ static int simplefb_clocks_init(struct simplefb_par *par,
static void simplefb_clocks_destroy(struct simplefb_par *par) { }
#endif
+#if defined CONFIG_OF && defined CONFIG_REGULATOR
+
+#define SUPPLY_SUFFIX "-supply"
+
+/*
+ * Regulator handling code.
+ *
+ * Here we handle the num-supplies and vin*-supply properties of our
+ * "simple-framebuffer" dt node. This is necessary so that we can make sure
+ * that any regulators needed by the display hardware that the bootloader
+ * set up for us (and for which it provided a simplefb dt node), stay up,
+ * for the life of the simplefb driver.
+ *
+ * When the driver unloads, we cleanly disable, and then release the
+ * regulators.
+ *
+ * We only complain about errors here, no action is taken as the most likely
+ * error can only happen due to a mismatch between the bootloader which set
+ * up simplefb, and the regulator definitions in the device tree. Chances are
+ * that there are no adverse effects, and if there are, a clean teardown of
+ * the fb probe will not help us much either. So just complain and carry on,
+ * and hope that the user actually gets a working fb at the end of things.
+ */
+static int simplefb_regulators_init(struct simplefb_par *par,
+ struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct property *prop;
+ struct regulator *regulator;
+ const char *p;
+ int count = 0, i = 0, ret;
+
+ if (dev_get_platdata(&pdev->dev) || !np)
+ return 0;
+
+ /* Count the number of regulator supplies */
+ for_each_property_of_node(np, prop) {
+ p = strstr(prop->name, SUPPLY_SUFFIX);
+ if (p && p != prop->name)
+ count++;
+ }
+
+ if (!count)
+ return 0;
+
+ par->regulators = devm_kcalloc(&pdev->dev, count,
+ sizeof(struct regulator *), GFP_KERNEL);
+ if (!par->regulators)
+ return -ENOMEM;
+
+ /* Get all the regulators */
+ for_each_property_of_node(np, prop) {
+ char name[32]; /* 32 is max size of property name */
+
+ p = strstr(prop->name, SUPPLY_SUFFIX);
+ if (p && p != prop->name)
+ continue;
+
+ strlcpy(name, prop->name,
+ strlen(prop->name) - sizeof(SUPPLY_SUFFIX) + 1);
+ regulator = devm_regulator_get_optional(&pdev->dev, name);
+ if (IS_ERR(regulator)) {
+ if (PTR_ERR(regulator) = -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+ dev_err(&pdev->dev, "regulator %s not found: %ld\n",
+ name, PTR_ERR(regulator));
+ continue;
+ }
+ par->regulators[i++] = regulator;
+ }
+ par->regulator_count = i;
+
+ /* Enable all the regulators */
+ for (i = 0; i < par->regulator_count; i++) {
+ if (par->regulators[i]) {
+ ret = regulator_enable(par->regulators[i]);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "failed to enable regulator %d: %d\n",
+ i, ret);
+ devm_regulator_put(par->regulators[i]);
+ par->regulators[i] = NULL;
+ }
+ }
+ }
+
+ return 0;
+}
+
+static void simplefb_regulators_destroy(struct simplefb_par *par)
+{
+ int i;
+
+ if (!par->regulators)
+ return;
+
+ for (i = 0; i < par->regulator_count; i++)
+ if (par->regulators[i])
+ regulator_disable(par->regulators[i]);
+}
+#else
+static int simplefb_regulators_init(struct simplefb_par *par,
+ struct platform_device *pdev) { return 0; }
+static void simplefb_regulators_destroy(struct simplefb_par *par) { }
+#endif
+
static int simplefb_probe(struct platform_device *pdev)
{
int ret;
@@ -340,6 +453,10 @@ static int simplefb_probe(struct platform_device *pdev)
if (ret < 0)
goto error_unmap;
+ ret = simplefb_regulators_init(par, pdev);
+ if (ret < 0)
+ goto error_clocks;
+
dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
info->fix.smem_start, info->fix.smem_len,
info->screen_base);
@@ -351,13 +468,15 @@ static int simplefb_probe(struct platform_device *pdev)
ret = register_framebuffer(info);
if (ret < 0) {
dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
- goto error_clocks;
+ goto error_regulators;
}
dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
return 0;
+error_regulators:
+ simplefb_regulators_destroy(par);
error_clocks:
simplefb_clocks_destroy(par);
error_unmap:
@@ -373,6 +492,7 @@ static int simplefb_remove(struct platform_device *pdev)
struct simplefb_par *par = info->par;
unregister_framebuffer(info);
+ simplefb_regulators_destroy(par);
simplefb_clocks_destroy(par);
framebuffer_release(info);
--
2.6.1
^ permalink raw reply related
* Re: [GIT PULL] On-demand device probing
From: Jean-Francois Moine @ 2015-10-21 6:15 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Mark Brown, linux-fbdev@vger.kernel.org, Wolfram Sang,
Michael Turquette, dri-devel, Liam Girdwood,
linux-i2c@vger.kernel.org, Frank Rowand, linux-clk, Russell King,
Dmitry Eremin-Solenikov, Lee Jones, Stephen Warren,
Linux PWM List, Kishon Vijay Abraham I, Tomi Valkeinen,
Alan Stern, Grant Likely, Jean-Christophe Plagniol-Villard,
devicetree@vger.kernel.org, Alexandre
In-Reply-To: <23698237.227MziouDO@vostro.rjw.lan>
Sorry to enter this thread a bit late.
About the number of probe deferred messages, I proposed a simple patch to
reduce them:
https://lkml.org/lkml/2013/8/20/218
I was wondering how many messages this patch could save...
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
^ permalink raw reply
* Re: [PATCH RFC v2 1/2] dt-bindings: simplefb: Support a list of regulator supply properties
From: Hans de Goede @ 2015-10-21 7:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1445407141-16459-2-git-send-email-wens@csie.org>
Hi,
On 21-10-15 07:59, Chen-Yu Tsai wrote:
> The physical display tied to the framebuffer may have regulators
> providing power to it, such as power for LCDs or interface conversion
> chips.
>
> The number of regulators in use may vary, but the regulator supply
> binding can not be a list. Work around this by adding a "num-supplies"
> property to communicate the number of supplies, and a list of 0 ~ N
> "vinN-supply" properties for the actual regulator supply.
This bit of the commit message is no longer accurate. Other then that
this patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
> .../devicetree/bindings/video/simple-framebuffer.txt | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index 4474ef6e0b95..8c9e9f515c87 100644
> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> @@ -47,10 +47,14 @@ Required properties:
> - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r).
>
> Optional properties:
> -- clocks : List of clocks used by the framebuffer. Clocks listed here
> - are expected to already be configured correctly. The OS must
> - ensure these clocks are not modified or disabled while the
> - simple framebuffer remains active.
> +- clocks : List of clocks used by the framebuffer.
> +- *-supply : Any number of regulators used by the framebuffer. These should
> + be named according to the names in the device's design.
> +
> + The above resources are expected to already be configured correctly.
> + The OS must ensure they are not modified or disabled while the simple
> + framebuffer remains active.
> +
> - display : phandle pointing to the primary display hardware node
>
> Example:
> @@ -68,6 +72,7 @@ chosen {
> stride = <(1600 * 2)>;
> format = "r5g6b5";
> clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
> + lcd-supply = <®_dc1sw>;
> display = <&lcdc0>;
> };
> stdout-path = "display0";
>
^ permalink raw reply
* Re: [PATCH RFC v2 2/2] simplefb: Claim and enable regulators
From: Hans de Goede @ 2015-10-21 7:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1445407141-16459-3-git-send-email-wens@csie.org>
Hi,
On 21-10-15 07:59, Chen-Yu Tsai wrote:
> This claims and enables regulators listed in the simple framebuffer dt
> node. This is needed so that regulators powering the display pipeline
> and external hardware, described in the device node and known by the
> kernel code, will remain properly enabled.
>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
> drivers/video/fbdev/simplefb.c | 122 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 121 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 52c5c7e63b52..c4ee10d83a70 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -28,7 +28,10 @@
> #include <linux/platform_device.h>
> #include <linux/clk.h>
> #include <linux/clk-provider.h>
> +#include <linux/of.h>
> #include <linux/of_platform.h>
> +#include <linux/parser.h>
> +#include <linux/regulator/consumer.h>
>
> static struct fb_fix_screeninfo simplefb_fix = {
> .id = "simple",
> @@ -174,6 +177,10 @@ struct simplefb_par {
> int clk_count;
> struct clk **clks;
> #endif
> +#if defined CONFIG_OF && defined CONFIG_REGULATOR
> + u32 regulator_count;
> + struct regulator **regulators;
> +#endif
> };
>
> #if defined CONFIG_OF && defined CONFIG_COMMON_CLK
> @@ -269,6 +276,112 @@ static int simplefb_clocks_init(struct simplefb_par *par,
> static void simplefb_clocks_destroy(struct simplefb_par *par) { }
> #endif
>
> +#if defined CONFIG_OF && defined CONFIG_REGULATOR
> +
> +#define SUPPLY_SUFFIX "-supply"
> +
> +/*
> + * Regulator handling code.
> + *
> + * Here we handle the num-supplies and vin*-supply properties of our
> + * "simple-framebuffer" dt node. This is necessary so that we can make sure
> + * that any regulators needed by the display hardware that the bootloader
> + * set up for us (and for which it provided a simplefb dt node), stay up,
> + * for the life of the simplefb driver.
> + *
> + * When the driver unloads, we cleanly disable, and then release the
> + * regulators.
> + *
> + * We only complain about errors here, no action is taken as the most likely
> + * error can only happen due to a mismatch between the bootloader which set
> + * up simplefb, and the regulator definitions in the device tree. Chances are
> + * that there are no adverse effects, and if there are, a clean teardown of
> + * the fb probe will not help us much either. So just complain and carry on,
> + * and hope that the user actually gets a working fb at the end of things.
> + */
> +static int simplefb_regulators_init(struct simplefb_par *par,
> + struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct property *prop;
> + struct regulator *regulator;
> + const char *p;
> + int count = 0, i = 0, ret;
> +
> + if (dev_get_platdata(&pdev->dev) || !np)
> + return 0;
> +
> + /* Count the number of regulator supplies */
> + for_each_property_of_node(np, prop) {
> + p = strstr(prop->name, SUPPLY_SUFFIX);
> + if (p && p != prop->name)
> + count++;
> + }
> +
> + if (!count)
> + return 0;
> +
> + par->regulators = devm_kcalloc(&pdev->dev, count,
> + sizeof(struct regulator *), GFP_KERNEL);
> + if (!par->regulators)
> + return -ENOMEM;
> +
> + /* Get all the regulators */
> + for_each_property_of_node(np, prop) {
> + char name[32]; /* 32 is max size of property name */
> +
> + p = strstr(prop->name, SUPPLY_SUFFIX);
> + if (p && p != prop->name)
> + continue;
> +
> + strlcpy(name, prop->name,
> + strlen(prop->name) - sizeof(SUPPLY_SUFFIX) + 1);
> + regulator = devm_regulator_get_optional(&pdev->dev, name);
> + if (IS_ERR(regulator)) {
> + if (PTR_ERR(regulator) = -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + dev_err(&pdev->dev, "regulator %s not found: %ld\n",
> + name, PTR_ERR(regulator));
> + continue;
> + }
> + par->regulators[i++] = regulator;
So you only fill slots when the regulator_get has succeeded
> + }
> + par->regulator_count = i;
and regulator_count now is the amount of successfully gotten regulators
(which may be different from count).
> +
> + /* Enable all the regulators */
> + for (i = 0; i < par->regulator_count; i++) {
> + if (par->regulators[i]) {
That means that this "if" is not necessary, it will always be true.
> + ret = regulator_enable(par->regulators[i]);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "failed to enable regulator %d: %d\n",
> + i, ret);
> + devm_regulator_put(par->regulators[i]);
> + par->regulators[i] = NULL;
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +static void simplefb_regulators_destroy(struct simplefb_par *par)
> +{
> + int i;
> +
> + if (!par->regulators)
> + return;
> +
> + for (i = 0; i < par->regulator_count; i++)
> + if (par->regulators[i])
And idem for this if.
Other then that this patch looks good and is:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> + regulator_disable(par->regulators[i]);
> +}
> +#else
> +static int simplefb_regulators_init(struct simplefb_par *par,
> + struct platform_device *pdev) { return 0; }
> +static void simplefb_regulators_destroy(struct simplefb_par *par) { }
> +#endif
> +
> static int simplefb_probe(struct platform_device *pdev)
> {
> int ret;
> @@ -340,6 +453,10 @@ static int simplefb_probe(struct platform_device *pdev)
> if (ret < 0)
> goto error_unmap;
>
> + ret = simplefb_regulators_init(par, pdev);
> + if (ret < 0)
> + goto error_clocks;
> +
> dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
> info->fix.smem_start, info->fix.smem_len,
> info->screen_base);
> @@ -351,13 +468,15 @@ static int simplefb_probe(struct platform_device *pdev)
> ret = register_framebuffer(info);
> if (ret < 0) {
> dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
> - goto error_clocks;
> + goto error_regulators;
> }
>
> dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
>
> return 0;
>
> +error_regulators:
> + simplefb_regulators_destroy(par);
> error_clocks:
> simplefb_clocks_destroy(par);
> error_unmap:
> @@ -373,6 +492,7 @@ static int simplefb_remove(struct platform_device *pdev)
> struct simplefb_par *par = info->par;
>
> unregister_framebuffer(info);
> + simplefb_regulators_destroy(par);
> simplefb_clocks_destroy(par);
> framebuffer_release(info);
>
>
^ permalink raw reply
* Re: [PATCH RFC v2 2/2] simplefb: Claim and enable regulators
From: Chen-Yu Tsai @ 2015-10-21 8:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <562744A9.1070705@redhat.com>
Hi,
On Wed, Oct 21, 2015 at 3:54 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 21-10-15 07:59, Chen-Yu Tsai wrote:
>>
>> This claims and enables regulators listed in the simple framebuffer dt
>> node. This is needed so that regulators powering the display pipeline
>> and external hardware, described in the device node and known by the
>> kernel code, will remain properly enabled.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>> drivers/video/fbdev/simplefb.c | 122
>> ++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 121 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/simplefb.c
>> b/drivers/video/fbdev/simplefb.c
>> index 52c5c7e63b52..c4ee10d83a70 100644
>> --- a/drivers/video/fbdev/simplefb.c
>> +++ b/drivers/video/fbdev/simplefb.c
>> @@ -28,7 +28,10 @@
>> #include <linux/platform_device.h>
>> #include <linux/clk.h>
>> #include <linux/clk-provider.h>
>> +#include <linux/of.h>
>> #include <linux/of_platform.h>
>> +#include <linux/parser.h>
>> +#include <linux/regulator/consumer.h>
>>
>> static struct fb_fix_screeninfo simplefb_fix = {
>> .id = "simple",
>> @@ -174,6 +177,10 @@ struct simplefb_par {
>> int clk_count;
>> struct clk **clks;
>> #endif
>> +#if defined CONFIG_OF && defined CONFIG_REGULATOR
>> + u32 regulator_count;
>> + struct regulator **regulators;
>> +#endif
>> };
>>
>> #if defined CONFIG_OF && defined CONFIG_COMMON_CLK
>> @@ -269,6 +276,112 @@ static int simplefb_clocks_init(struct simplefb_par
>> *par,
>> static void simplefb_clocks_destroy(struct simplefb_par *par) { }
>> #endif
>>
>> +#if defined CONFIG_OF && defined CONFIG_REGULATOR
>> +
>> +#define SUPPLY_SUFFIX "-supply"
>> +
>> +/*
>> + * Regulator handling code.
>> + *
>> + * Here we handle the num-supplies and vin*-supply properties of our
>> + * "simple-framebuffer" dt node. This is necessary so that we can make
>> sure
>> + * that any regulators needed by the display hardware that the bootloader
>> + * set up for us (and for which it provided a simplefb dt node), stay up,
>> + * for the life of the simplefb driver.
>> + *
>> + * When the driver unloads, we cleanly disable, and then release the
>> + * regulators.
>> + *
>> + * We only complain about errors here, no action is taken as the most
>> likely
>> + * error can only happen due to a mismatch between the bootloader which
>> set
>> + * up simplefb, and the regulator definitions in the device tree. Chances
>> are
>> + * that there are no adverse effects, and if there are, a clean teardown
>> of
>> + * the fb probe will not help us much either. So just complain and carry
>> on,
>> + * and hope that the user actually gets a working fb at the end of
>> things.
>> + */
>> +static int simplefb_regulators_init(struct simplefb_par *par,
>> + struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct property *prop;
>> + struct regulator *regulator;
>> + const char *p;
>> + int count = 0, i = 0, ret;
>> +
>> + if (dev_get_platdata(&pdev->dev) || !np)
>> + return 0;
>> +
>> + /* Count the number of regulator supplies */
>> + for_each_property_of_node(np, prop) {
>> + p = strstr(prop->name, SUPPLY_SUFFIX);
>> + if (p && p != prop->name)
>> + count++;
>> + }
>> +
>> + if (!count)
>> + return 0;
>> +
>> + par->regulators = devm_kcalloc(&pdev->dev, count,
>> + sizeof(struct regulator *),
>> GFP_KERNEL);
>> + if (!par->regulators)
>> + return -ENOMEM;
>> +
>> + /* Get all the regulators */
>> + for_each_property_of_node(np, prop) {
>> + char name[32]; /* 32 is max size of property name */
>> +
>> + p = strstr(prop->name, SUPPLY_SUFFIX);
>> + if (p && p != prop->name)
>> + continue;
>> +
>> + strlcpy(name, prop->name,
>> + strlen(prop->name) - sizeof(SUPPLY_SUFFIX) + 1);
>> + regulator = devm_regulator_get_optional(&pdev->dev, name);
>> + if (IS_ERR(regulator)) {
>> + if (PTR_ERR(regulator) = -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> + dev_err(&pdev->dev, "regulator %s not found:
>> %ld\n",
>> + name, PTR_ERR(regulator));
>> + continue;
>> + }
>> + par->regulators[i++] = regulator;
>
>
> So you only fill slots when the regulator_get has succeeded
>
>> + }
>> + par->regulator_count = i;
>
>
> and regulator_count now is the amount of successfully gotten regulators
> (which may be different from count).
>
>> +
>> + /* Enable all the regulators */
>> + for (i = 0; i < par->regulator_count; i++) {
>> + if (par->regulators[i]) {
>
>
> That means that this "if" is not necessary, it will always be true.
Right. This is leftover code from the first version. I'll remove it.
>> + ret = regulator_enable(par->regulators[i]);
>> + if (ret) {
>> + dev_err(&pdev->dev,
>> + "failed to enable regulator %d:
>> %d\n",
>> + i, ret);
>> + devm_regulator_put(par->regulators[i]);
>> + par->regulators[i] = NULL;
Note here.
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void simplefb_regulators_destroy(struct simplefb_par *par)
>> +{
>> + int i;
>> +
>> + if (!par->regulators)
>> + return;
>> +
>> + for (i = 0; i < par->regulator_count; i++)
>> + if (par->regulators[i])
>
>
> And idem for this if.
This is still needed, since if we fail to enable any regulator, we just
ignore it, call regulator_put() on it, and forget about it (set the entry
to NULL). See the noted place above.
>
> Other then that this patch looks good and is:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Thanks. I'll wait a bit before sending the next version, in case Mark has
some comments on the regulator usage.
Regards
ChenYu
>> + regulator_disable(par->regulators[i]);
>> +}
>> +#else
>> +static int simplefb_regulators_init(struct simplefb_par *par,
>> + struct platform_device *pdev) { return 0; }
>> +static void simplefb_regulators_destroy(struct simplefb_par *par) { }
>> +#endif
>> +
>> static int simplefb_probe(struct platform_device *pdev)
>> {
>> int ret;
>> @@ -340,6 +453,10 @@ static int simplefb_probe(struct platform_device
>> *pdev)
>> if (ret < 0)
>> goto error_unmap;
>>
>> + ret = simplefb_regulators_init(par, pdev);
>> + if (ret < 0)
>> + goto error_clocks;
>> +
>> dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to
>> 0x%p\n",
>> info->fix.smem_start, info->fix.smem_len,
>> info->screen_base);
>> @@ -351,13 +468,15 @@ static int simplefb_probe(struct platform_device
>> *pdev)
>> ret = register_framebuffer(info);
>> if (ret < 0) {
>> dev_err(&pdev->dev, "Unable to register simplefb: %d\n",
>> ret);
>> - goto error_clocks;
>> + goto error_regulators;
>> }
>>
>> dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
>>
>> return 0;
>>
>> +error_regulators:
>> + simplefb_regulators_destroy(par);
>> error_clocks:
>> simplefb_clocks_destroy(par);
>> error_unmap:
>> @@ -373,6 +492,7 @@ static int simplefb_remove(struct platform_device
>> *pdev)
>> struct simplefb_par *par = info->par;
>>
>> unregister_framebuffer(info);
>> + simplefb_regulators_destroy(par);
>> simplefb_clocks_destroy(par);
>> framebuffer_release(info);
>>
>>
>
^ permalink raw reply
* Re: [PATCH RFC v2 2/2] simplefb: Claim and enable regulators
From: Hans de Goede @ 2015-10-21 8:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAGb2v643sDnNv5DPYocu-50UB3q-itpwGdw8hZ3YN=V+nEBkMg@mail.gmail.com>
Hi,
On 21-10-15 10:04, Chen-Yu Tsai wrote:
> Hi,
>
> On Wed, Oct 21, 2015 at 3:54 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 21-10-15 07:59, Chen-Yu Tsai wrote:
>>>
>>> This claims and enables regulators listed in the simple framebuffer dt
>>> node. This is needed so that regulators powering the display pipeline
>>> and external hardware, described in the device node and known by the
>>> kernel code, will remain properly enabled.
>>>
>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>>> ---
>>> drivers/video/fbdev/simplefb.c | 122
>>> ++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 121 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/video/fbdev/simplefb.c
>>> b/drivers/video/fbdev/simplefb.c
>>> index 52c5c7e63b52..c4ee10d83a70 100644
>>> --- a/drivers/video/fbdev/simplefb.c
>>> +++ b/drivers/video/fbdev/simplefb.c
>>> @@ -28,7 +28,10 @@
>>> #include <linux/platform_device.h>
>>> #include <linux/clk.h>
>>> #include <linux/clk-provider.h>
>>> +#include <linux/of.h>
>>> #include <linux/of_platform.h>
>>> +#include <linux/parser.h>
>>> +#include <linux/regulator/consumer.h>
>>>
>>> static struct fb_fix_screeninfo simplefb_fix = {
>>> .id = "simple",
>>> @@ -174,6 +177,10 @@ struct simplefb_par {
>>> int clk_count;
>>> struct clk **clks;
>>> #endif
>>> +#if defined CONFIG_OF && defined CONFIG_REGULATOR
>>> + u32 regulator_count;
>>> + struct regulator **regulators;
>>> +#endif
>>> };
>>>
>>> #if defined CONFIG_OF && defined CONFIG_COMMON_CLK
>>> @@ -269,6 +276,112 @@ static int simplefb_clocks_init(struct simplefb_par
>>> *par,
>>> static void simplefb_clocks_destroy(struct simplefb_par *par) { }
>>> #endif
>>>
>>> +#if defined CONFIG_OF && defined CONFIG_REGULATOR
>>> +
>>> +#define SUPPLY_SUFFIX "-supply"
>>> +
>>> +/*
>>> + * Regulator handling code.
>>> + *
>>> + * Here we handle the num-supplies and vin*-supply properties of our
>>> + * "simple-framebuffer" dt node. This is necessary so that we can make
>>> sure
>>> + * that any regulators needed by the display hardware that the bootloader
>>> + * set up for us (and for which it provided a simplefb dt node), stay up,
>>> + * for the life of the simplefb driver.
>>> + *
>>> + * When the driver unloads, we cleanly disable, and then release the
>>> + * regulators.
>>> + *
>>> + * We only complain about errors here, no action is taken as the most
>>> likely
>>> + * error can only happen due to a mismatch between the bootloader which
>>> set
>>> + * up simplefb, and the regulator definitions in the device tree. Chances
>>> are
>>> + * that there are no adverse effects, and if there are, a clean teardown
>>> of
>>> + * the fb probe will not help us much either. So just complain and carry
>>> on,
>>> + * and hope that the user actually gets a working fb at the end of
>>> things.
>>> + */
>>> +static int simplefb_regulators_init(struct simplefb_par *par,
>>> + struct platform_device *pdev)
>>> +{
>>> + struct device_node *np = pdev->dev.of_node;
>>> + struct property *prop;
>>> + struct regulator *regulator;
>>> + const char *p;
>>> + int count = 0, i = 0, ret;
>>> +
>>> + if (dev_get_platdata(&pdev->dev) || !np)
>>> + return 0;
>>> +
>>> + /* Count the number of regulator supplies */
>>> + for_each_property_of_node(np, prop) {
>>> + p = strstr(prop->name, SUPPLY_SUFFIX);
>>> + if (p && p != prop->name)
>>> + count++;
>>> + }
>>> +
>>> + if (!count)
>>> + return 0;
>>> +
>>> + par->regulators = devm_kcalloc(&pdev->dev, count,
>>> + sizeof(struct regulator *),
>>> GFP_KERNEL);
>>> + if (!par->regulators)
>>> + return -ENOMEM;
>>> +
>>> + /* Get all the regulators */
>>> + for_each_property_of_node(np, prop) {
>>> + char name[32]; /* 32 is max size of property name */
>>> +
>>> + p = strstr(prop->name, SUPPLY_SUFFIX);
>>> + if (p && p != prop->name)
>>> + continue;
>>> +
>>> + strlcpy(name, prop->name,
>>> + strlen(prop->name) - sizeof(SUPPLY_SUFFIX) + 1);
>>> + regulator = devm_regulator_get_optional(&pdev->dev, name);
>>> + if (IS_ERR(regulator)) {
>>> + if (PTR_ERR(regulator) = -EPROBE_DEFER)
>>> + return -EPROBE_DEFER;
>>> + dev_err(&pdev->dev, "regulator %s not found:
>>> %ld\n",
>>> + name, PTR_ERR(regulator));
>>> + continue;
>>> + }
>>> + par->regulators[i++] = regulator;
>>
>>
>> So you only fill slots when the regulator_get has succeeded
>>
>>> + }
>>> + par->regulator_count = i;
>>
>>
>> and regulator_count now is the amount of successfully gotten regulators
>> (which may be different from count).
>>
>>> +
>>> + /* Enable all the regulators */
>>> + for (i = 0; i < par->regulator_count; i++) {
>>> + if (par->regulators[i]) {
>>
>>
>> That means that this "if" is not necessary, it will always be true.
>
> Right. This is leftover code from the first version. I'll remove it.
>
>>> + ret = regulator_enable(par->regulators[i]);
>>> + if (ret) {
>>> + dev_err(&pdev->dev,
>>> + "failed to enable regulator %d:
>>> %d\n",
>>> + i, ret);
>>> + devm_regulator_put(par->regulators[i]);
>>> + par->regulators[i] = NULL;
>
> Note here.
>
>>> + }
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void simplefb_regulators_destroy(struct simplefb_par *par)
>>> +{
>>> + int i;
>>> +
>>> + if (!par->regulators)
>>> + return;
>>> +
>>> + for (i = 0; i < par->regulator_count; i++)
>>> + if (par->regulators[i])
>>
>>
>> And idem for this if.
>
> This is still needed, since if we fail to enable any regulator, we just
> ignore it, call regulator_put() on it, and forget about it (set the entry
> to NULL). See the noted place above.
Ah right, ok lets keep that in then :)
Regards,
Hans
^ permalink raw reply
* Re: Alternative approach to solve the deferred probe
From: Russell King - ARM Linux @ 2015-10-21 8:18 UTC (permalink / raw)
To: Frank Rowand
Cc: Geert Uytterhoeven, Tomeu Vizoso, Mark Brown, Greg Kroah-Hartman,
Rob Herring, Michael Turquette, Stephen Boyd, Vinod Koul,
Dan Williams, Linus Walleij, Alexandre Courbot, Thierry Reding,
David Airlie, Terje Bergström, Stephen Warren, Wolfram Sang,
Grant Likely, Kishon Vijay Abraham I, Sebastian Reichel,
Dmitry Eremin-Solenikov, David Woodhouse
In-Reply-To: <56270D5B.5010902@gmail.com>
On Tue, Oct 20, 2015 at 08:58:19PM -0700, Frank Rowand wrote:
> On 10/20/2015 8:46 AM, Russell King - ARM Linux wrote:
> > On Mon, Oct 19, 2015 at 06:21:40PM +0200, Geert Uytterhoeven wrote:
> >> Hi Russell,
> >>
> >> On Mon, Oct 19, 2015 at 5:35 PM, Russell King - ARM Linux
> >> <linux@arm.linux.org.uk> wrote:
> >>>>> What you can do is print those devices which have failed to probe at
> >>>>> late_initcall() time - possibly augmenting that with reports from
> >>>>> subsystems showing what resources are not available, but that's only
> >>>>> a guide, because of the "it might or might not be in a kernel module"
> >>>>> problem.
> >>>>
> >>>> Well, adding those reports would give you a changelog similar to the
> >>>> one in this series...
> >>>
> >>> I'm not sure about that, because what I was thinking of is adding
> >>> a flag which would be set at late_initcall() time prior to running
> >>> a final round of deferred device probing.
> >>
> >> Which round is the final round?
> >> That's the one which didn't manage to bind any new devices to drivers,
> >> which is something you only know _after_ the round has been run.
> >>
> >> So I think we need one extra round to handle this.
> >>
> >>> This flag would then be used in a deferred_warn() printk function
> >>> which would normally be silent, but when this flag is set, it would
> >>> print the reason for the deferral - and this would replace (or be
> >>> added) to the subsystems and drivers which return -EPROBE_DEFER.
> >>>
> >>> That has the effect of hiding all the deferrals up until just before
> >>> launching into userspace, which should then acomplish two things -
> >>> firstly, getting rid of the rather useless deferred messages up to
> >>> that point, and secondly printing the reason why the remaining
> >>> deferrals are happening.
> >>>
> >>> That should be a small number of new lines plus a one-line change
> >>> in subsystems and drivers.
> >>
> >> Apart from the extra round we probably can't get rid of, that sounds OK to me.
> >
> > Something like this. I haven't put a lot of effort into it to change all
> > the places which return an -EPROBE_DEFER, and it also looks like we need
> > some helpers to report when we have only an device_node (or should that
> > be fwnode?) See the commented out of_warn_deferred() in
> > drivers/gpio/gpiolib-of.c. Adding this stuff in the subsystems searching
> > for resources should make debugging why things are getting deferred easier.
> >
> > We could make driver_deferred_probe_report something that can be
> > deactivated again after the last deferred probe run, and provide the
> > user with a knob that they can turn it back on again.
> >
> > I've tried this out on two of my platforms, including forcing
> > driver_deferred_probe_report to be enabled, and I get exactly one
> > deferred probe, so not a particularly good test.
> >
> > The patch won't apply as-is to mainline for all files; it's based on my
> > tree which has some 360 additional patches (which seems to be about
> > normal for my tree now.)
>
> I like the concept (I have been thinking along similar lines lately).
> But I think this might make the console messages more confusing than
> they are now.
If messages end up being given from the subsystem rather than the driver,
surely they become more consistent?
> The problem is that debug, warn, and error messages
> come from a somewhat random set of locations at the moment. Some
> come from the driver probe routines and some come from the subsystems
> that the probe routines call. So the patch is suppressing some
> messages, but not others.
The patch is not complete (read the description above).
> > +void dev_warn_deferred(struct device *dev, const char *fmt, ...)
> > +{
> > + if (driver_deferred_probe_report) {
> > + struct va_format vaf;
> > + va_list ap;
> > +
> > + va_start(ap, fmt);
> > + vaf.fmt = fmt;
> > + vaf.va = ≈
> > +
> > + dev_warn(dev, "deferring probe: %pV", &vaf);
> > + va_end(ap);
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(dev_warn_deferred);
>
> The places where dev_warn_deferred() replaces dev_dbg(), we lose the
> ability to turn on debugging and observe the driver reporting the
> specific reason the deferral is occurring. So it would be useful to
> add an "else dev_dbg()" in dev_warn_deferred() to retain that capability.
That's a possibility.
>
> > +
> > static bool driver_deferred_probe_enable = false;
> > +
> > /**
> > * driver_deferred_probe_trigger() - Kick off re-probing deferred devices
> > *
> > @@ -188,6 +210,13 @@ static int deferred_probe_initcall(void)
> > driver_deferred_probe_trigger();
>
> Couldn't you put the "driver_deferred_probe_report = true" here? And then
> not add another round of probes.
The idea is not to report anything for drivers that were deferred
during the normal bootup. The above is part of the normal bootup,
and the deferred activity should not be warned about.
If we have any devices still deferring after _this_ round, that must
indicate that some resource they want is not available, and that
should be warned about.
Of course, modules can defer too - and I made some suggestions in my
waffle above the patch about that.
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ 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