* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Henrik Nordström @ 2014-08-27 8:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826135341.GM15297@lukather>
tis 2014-08-26 klockan 15:53 +0200 skrev Maxime Ripard:
> > it should be fixed at the root. So a much better solution would be to
> > establish a way for firmware to communicate to the kernel that a given
> > resource has been enabled by firmware and shouldn't be disabled. Such a
> > solution can be implement for all types of resources and can be reused
> > by all drivers since they don't have to worry about these details.
>
> Mike Turquette repeatedly said that he was against such a DT property:
> https://lkml.org/lkml/2014/5/12/693
And not really what we want here. The clocks should be turned off if not
in use, i.e. when simplefb is unbound from the device and no other
driver has taken over. Maybe even in the case where there is no simplefb
driver in-kernel, but if the use case is to be generalized then this
kind of resources need to be held until explicitly released to allow
resources to be held until their driver is loaded, which may happen long
after kernel bootup.
But again, for many use cases it is a virtual hardware property, not
really hardware in it's own but still smells a lot like hardware. These
parameters is only valid until the hardware have been reconfigured.
Regards
Henrik
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Hans de Goede @ 2014-08-27 8:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140827065440.GG15640@ulmo>
Hi,
On 08/27/2014 08:54 AM, Thierry Reding wrote:
> On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote:
>> On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote:
>>>>>> Any decent enough SoC, with a decent support in the kernel will have
>>>>>> clocks for this, and I really wonder how simplefb will behave once its
>>>>>> clocks will be turned off...
>>>>>
>>>>> There are other devices besides ARM SoCs that may want to use this
>>>>> driver and that don't have clock support.
>>>>
>>>> And in this case, with this patch, simplefb will not claim any clock,
>>>> nor will fail probing.
>>>>
>>>>> But you're missing my point. What I'm saying is that the simplefb driver
>>>>> is meant to serve as a way to take over whatever framebuffer a firmware
>>>>> set up. Therefore I think it makes the most sense to assume that nothing
>>>>> needs to be controlled in any way since already been set up by firmware.
>>>>> Eventually there should be a driver that takes over from simplefb that
>>>>> knows how to properly handle the device's specifics, but that's not
>>>>> simplefb.
>>>>
>>>> I guess such a hand over if it were to happen in the kernel would
>>>> involve the second driver claiming the resources before the first one
>>>> release them. How is that different in this case?
>>>
>>> It's different in that that driver will be hardware specific and know
>>> exactly what clock and other resources are required. It will have a
>>> device-specific binding.
>>
>> Except that you made simplefb a generic driver. So we have two choices
>> here: either we don't anything but the trivial case, and no one with a
>> rather more advanced hardware will be able to use it, or we try to
>> grab any resource that might be of use, which means clocks,
>> regulators, reserved memory region, or whatever, so that we pretty
>> much cover all cases. It really is as simple as that.
>
> No, it should be even simpler. simplefb shouldn't have to know any of
> this. It's just taking what firmware has set up and uses that. It's a
> stop-gap solution to provide information on the display until a real
> driver can be loaded and initializes the display hardware properly.
>
>>>>> The goal of this patch series is to keep clocks from being turned off.
>>>>> But that's not what it does. What it does is turn clocks on to prevent
>>>>> them from being turned off. In my opinion that's a workaround for a
>>>>> deficiency in the kernel (and the firmware/kernel interface) and I think
>>>>> it should be fixed at the root. So a much better solution would be to
>>>>> establish a way for firmware to communicate to the kernel that a given
>>>>> resource has been enabled by firmware and shouldn't be disabled. Such a
>>>>> solution can be implement for all types of resources and can be reused
>>>>> by all drivers since they don't have to worry about these details.
>>>>
>>>> Mike Turquette repeatedly said that he was against such a DT property:
>>>> https://lkml.org/lkml/2014/5/12/693
>>>
>>> Mike says in that email that he's opposing the addition of a property
>>> for clocks that is the equivalent of regulator-always-on. That's not
>>> what this is about. If at all it'd be a property to mark a clock that
>>> should not be disabled by default because it's essential.
>>
>> It's just semantic. How is "a clock that should not be disabled by
>> default because it's essential" not a clock that stays always on?
>
> Because a clock that should not be disabled by default can be turned off
> when appropriate. A clock that is always on can't be turned off.
>
>> Plus, you should read the mail further. It's clearly said that
>> consumer drivers should call clk_prepare_enable themselves, and that
>> the only exception to that is the case where we don't have such a
>> driver (and only valid as a temporary exception, which I guess you'll
>> soon turn into temporary-until-a-drm-kms-driver-is-merged).
>
> Exactly. simplefb is only a temporary measure. It's not meant to be used
> as a full-blown framebuffer driver. There are two use-cases where it is
> an appropriate solution:
>
> 1) As a stop-gap solution for when the platform doesn't support a full
> display driver yet. In that case you will want simplefb to stay
> around forever.
>
> 2) To get early boot output before a full display driver can be loaded
> in which case simplefb should go away after the display driver has
> taken over.
>
> In case of 2) the most straightforward solution is to not disable any
> clocks until all drivers have had a chance to claim them. When the full
> driver has been probed it should have claimed the clocks so they would
> no longer get disabled.
Please read my long reply from yesterday evening why this will never
work, as there is not a well defined moment when "all drivers have had a
chance to claim them"
I've been doing low level Linux development for 10 years now and I've
worked a lot on storage (raid, iscsi, fcoe support, etc.) in the past.
We've been down this road before, and all it leads to is pain, some more
pain and then even more pain. Followed by the conclusion that everything
needed to be refactored to use a hotplug model, and that instead of
waiting for hardware / disk-enumeration to be done (so we can mount
filesystems), the right thing to do is to actually wait for the block
device(s) holding said filesystems to show up.
The same applies here, the right thing to do is to wait for the kms
driver claiming the clocks to actually show up. And the easiest and
cleanest way to do that is to have simplefb claim the clocks, and
have it release them when the kms driver loads and asks simplefb
to unload.
Oh and one more thing, this whole make a list of special clocks which
should be disabled later model is flawed too, because it assumes a
fixed list of clocks, but which clocks actually get used may very well
depend on whether e.g. a vga or hdmi monitor is attached, so u-boot
would then still need to communicate the list of clocks actually used
somehow, and if it needs to pass a list of clocks, the most natural
way to do so is through a clocks property...
Regards,
Hans
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Mark Brown @ 2014-08-27 7:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1409078409.2701.17.camel@localhost>
[-- Attachment #1: Type: text/plain, Size: 813 bytes --]
On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote:
> It is not clear to me where the hardware resources should be listed in
> DT, being it a simplefb node or part of the actual hardware device node
> properly marked as dynamic boot defaults or something else? It's
> somewhere inbetween hardware and virtual device, and somewhat volatile.
> As far as simplefb is concerned it is a hardware desription of the
> framebuffer, but for a kms driver it's no more than firmware handover of
> boottime settings and ceases to exists once the kms driver have
> reconfigured the hardware.
Is simplefb something that should be in the device tree distinctly in
the first place - shouldn't it be a subset of the functionality of the
video nodes? It's the same hardware being driven differently.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-27 6:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826210248.GO15297@lukather>
[-- Attachment #1: Type: text/plain, Size: 6360 bytes --]
On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote:
> On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote:
> > > > > Any decent enough SoC, with a decent support in the kernel will have
> > > > > clocks for this, and I really wonder how simplefb will behave once its
> > > > > clocks will be turned off...
> > > >
> > > > There are other devices besides ARM SoCs that may want to use this
> > > > driver and that don't have clock support.
> > >
> > > And in this case, with this patch, simplefb will not claim any clock,
> > > nor will fail probing.
> > >
> > > > But you're missing my point. What I'm saying is that the simplefb driver
> > > > is meant to serve as a way to take over whatever framebuffer a firmware
> > > > set up. Therefore I think it makes the most sense to assume that nothing
> > > > needs to be controlled in any way since already been set up by firmware.
> > > > Eventually there should be a driver that takes over from simplefb that
> > > > knows how to properly handle the device's specifics, but that's not
> > > > simplefb.
> > >
> > > I guess such a hand over if it were to happen in the kernel would
> > > involve the second driver claiming the resources before the first one
> > > release them. How is that different in this case?
> >
> > It's different in that that driver will be hardware specific and know
> > exactly what clock and other resources are required. It will have a
> > device-specific binding.
>
> Except that you made simplefb a generic driver. So we have two choices
> here: either we don't anything but the trivial case, and no one with a
> rather more advanced hardware will be able to use it, or we try to
> grab any resource that might be of use, which means clocks,
> regulators, reserved memory region, or whatever, so that we pretty
> much cover all cases. It really is as simple as that.
No, it should be even simpler. simplefb shouldn't have to know any of
this. It's just taking what firmware has set up and uses that. It's a
stop-gap solution to provide information on the display until a real
driver can be loaded and initializes the display hardware properly.
> > > > The goal of this patch series is to keep clocks from being turned off.
> > > > But that's not what it does. What it does is turn clocks on to prevent
> > > > them from being turned off. In my opinion that's a workaround for a
> > > > deficiency in the kernel (and the firmware/kernel interface) and I think
> > > > it should be fixed at the root. So a much better solution would be to
> > > > establish a way for firmware to communicate to the kernel that a given
> > > > resource has been enabled by firmware and shouldn't be disabled. Such a
> > > > solution can be implement for all types of resources and can be reused
> > > > by all drivers since they don't have to worry about these details.
> > >
> > > Mike Turquette repeatedly said that he was against such a DT property:
> > > https://lkml.org/lkml/2014/5/12/693
> >
> > Mike says in that email that he's opposing the addition of a property
> > for clocks that is the equivalent of regulator-always-on. That's not
> > what this is about. If at all it'd be a property to mark a clock that
> > should not be disabled by default because it's essential.
>
> It's just semantic. How is "a clock that should not be disabled by
> default because it's essential" not a clock that stays always on?
Because a clock that should not be disabled by default can be turned off
when appropriate. A clock that is always on can't be turned off.
> Plus, you should read the mail further. It's clearly said that
> consumer drivers should call clk_prepare_enable themselves, and that
> the only exception to that is the case where we don't have such a
> driver (and only valid as a temporary exception, which I guess you'll
> soon turn into temporary-until-a-drm-kms-driver-is-merged).
Exactly. simplefb is only a temporary measure. It's not meant to be used
as a full-blown framebuffer driver. There are two use-cases where it is
an appropriate solution:
1) As a stop-gap solution for when the platform doesn't support a full
display driver yet. In that case you will want simplefb to stay
around forever.
2) To get early boot output before a full display driver can be loaded
in which case simplefb should go away after the display driver has
taken over.
In case of 2) the most straightforward solution is to not disable any
clocks until all drivers have had a chance to claim them. When the full
driver has been probed it should have claimed the clocks so they would
no longer get disabled.
For 1) I think it's fair to say that it's only a temporary solution to
get something on the screen. There won't be any kind of acceleration at
all, no power saving. Nothing but a dumb framebuffer. In that case it
should be a simple matter of keeping a select number of clocks always on
in the clock driver until support is more complete and it can be
properly handled. It's not like it's going to make a difference anyway
since simplefb won't ever disable them either.
> > Adding Mike on this subthread too.
> >
> > Either way, Mark already suggested a different alternative in another
> > subthread, namely to add a new kind of checkpoint at which subsystems
> > can call a "disable unused" function that's called later than a late
> > initcall. This is not going to fix things for you immediately because
> > the clocks will still be switched off (only later) if you don't have
> > a real driver that's claiming the clocks.
>
> Great, I'm glad we found a solution for a completely unrelated issue.
It's not at all unrelated. See above.
> > But you can work around that for now by making the relevant clocks
> > always on and remove that workaround once a real driver is loaded
> > that knows how to handle them properly.
>
> So, let me get this straight. The clock provider driver should behave
> as a clock consumer because it knows that in some cases, it might not
> have any willingful enough consumer? Doesn't that even ring your
> this-is-a-clear-abstraction-violation-bell just a tiny bit?
No. The clock driver should simply not allow the clocks to be disabled
if it isn't safe to do so.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-26 21:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826143550.GB3027@ulmo>
[-- Attachment #1: Type: text/plain, Size: 4447 bytes --]
On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote:
> > > > Any decent enough SoC, with a decent support in the kernel will have
> > > > clocks for this, and I really wonder how simplefb will behave once its
> > > > clocks will be turned off...
> > >
> > > There are other devices besides ARM SoCs that may want to use this
> > > driver and that don't have clock support.
> >
> > And in this case, with this patch, simplefb will not claim any clock,
> > nor will fail probing.
> >
> > > But you're missing my point. What I'm saying is that the simplefb driver
> > > is meant to serve as a way to take over whatever framebuffer a firmware
> > > set up. Therefore I think it makes the most sense to assume that nothing
> > > needs to be controlled in any way since already been set up by firmware.
> > > Eventually there should be a driver that takes over from simplefb that
> > > knows how to properly handle the device's specifics, but that's not
> > > simplefb.
> >
> > I guess such a hand over if it were to happen in the kernel would
> > involve the second driver claiming the resources before the first one
> > release them. How is that different in this case?
>
> It's different in that that driver will be hardware specific and know
> exactly what clock and other resources are required. It will have a
> device-specific binding.
Except that you made simplefb a generic driver. So we have two choices
here: either we don't anything but the trivial case, and no one with a
rather more advanced hardware will be able to use it, or we try to
grab any resource that might be of use, which means clocks,
regulators, reserved memory region, or whatever, so that we pretty
much cover all cases. It really is as simple as that.
> > > The goal of this patch series is to keep clocks from being turned off.
> > > But that's not what it does. What it does is turn clocks on to prevent
> > > them from being turned off. In my opinion that's a workaround for a
> > > deficiency in the kernel (and the firmware/kernel interface) and I think
> > > it should be fixed at the root. So a much better solution would be to
> > > establish a way for firmware to communicate to the kernel that a given
> > > resource has been enabled by firmware and shouldn't be disabled. Such a
> > > solution can be implement for all types of resources and can be reused
> > > by all drivers since they don't have to worry about these details.
> >
> > Mike Turquette repeatedly said that he was against such a DT property:
> > https://lkml.org/lkml/2014/5/12/693
>
> Mike says in that email that he's opposing the addition of a property
> for clocks that is the equivalent of regulator-always-on. That's not
> what this is about. If at all it'd be a property to mark a clock that
> should not be disabled by default because it's essential.
It's just semantic. How is "a clock that should not be disabled by
default because it's essential" not a clock that stays always on?
Plus, you should read the mail further. It's clearly said that
consumer drivers should call clk_prepare_enable themselves, and that
the only exception to that is the case where we don't have such a
driver (and only valid as a temporary exception, which I guess you'll
soon turn into temporary-until-a-drm-kms-driver-is-merged).
> Adding Mike on this subthread too.
>
> Either way, Mark already suggested a different alternative in another
> subthread, namely to add a new kind of checkpoint at which subsystems
> can call a "disable unused" function that's called later than a late
> initcall. This is not going to fix things for you immediately because
> the clocks will still be switched off (only later) if you don't have
> a real driver that's claiming the clocks.
Great, I'm glad we found a solution for a completely unrelated issue.
> But you can work around that for now by making the relevant clocks
> always on and remove that workaround once a real driver is loaded
> that knows how to handle them properly.
So, let me get this straight. The clock provider driver should behave
as a clock consumer because it knows that in some cases, it might not
have any willingful enough consumer? Doesn't that even ring your
this-is-a-clear-abstraction-violation-bell just a tiny bit?
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Hans de Goede @ 2014-08-26 19:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826143550.GB3027@ulmo>
Hi,
On 08/26/2014 04:35 PM, Thierry Reding wrote:
> On Tue, Aug 26, 2014 at 03:53:41PM +0200, Maxime Ripard wrote:
>> On Tue, Aug 26, 2014 at 10:04:33AM +0200, Thierry Reding wrote:
>>>>> No. simplefb just wants to write to some memory that hardware has been
>>>>> set up to scan out. The platform requires that the clocks be on. Other
>>>>> platforms may not even allow turning off the clocks.
>>>>
>>>> Like what? the rpi? Come on. Just because the videocore is some black
>>>> box we know nothing about doesn't mean we should use it as an example.
>>>
>>> You make it sound like the Raspberry Pi is somehow less important than
>>> sunxi.
>>
>> No. What I mean is that it seems like we are somehow punished, or at
>> least blamed, for having a better and more complete kernel support.
>
> This isn't a competition. Nobody's punishing or blaming anyone. This is
> about finding the best solution for the problem at hand.
>
>>>> Any decent enough SoC, with a decent support in the kernel will have
>>>> clocks for this, and I really wonder how simplefb will behave once its
>>>> clocks will be turned off...
>>>
>>> There are other devices besides ARM SoCs that may want to use this
>>> driver and that don't have clock support.
>>
>> And in this case, with this patch, simplefb will not claim any clock,
>> nor will fail probing.
>>
>>> But you're missing my point. What I'm saying is that the simplefb driver
>>> is meant to serve as a way to take over whatever framebuffer a firmware
>>> set up. Therefore I think it makes the most sense to assume that nothing
>>> needs to be controlled in any way since already been set up by firmware.
>>> Eventually there should be a driver that takes over from simplefb that
>>> knows how to properly handle the device's specifics, but that's not
>>> simplefb.
>>
>> I guess such a hand over if it were to happen in the kernel would
>> involve the second driver claiming the resources before the first one
>> release them. How is that different in this case?
>
> It's different in that that driver will be hardware specific and know
> exactly what clock and other resources are required. It will have a
> device-specific binding.
>
>>> The goal of this patch series is to keep clocks from being turned off.
>>> But that's not what it does. What it does is turn clocks on to prevent
>>> them from being turned off. In my opinion that's a workaround for a
>>> deficiency in the kernel (and the firmware/kernel interface) and I think
>>> it should be fixed at the root. So a much better solution would be to
>>> establish a way for firmware to communicate to the kernel that a given
>>> resource has been enabled by firmware and shouldn't be disabled. Such a
>>> solution can be implement for all types of resources and can be reused
>>> by all drivers since they don't have to worry about these details.
>>
>> Mike Turquette repeatedly said that he was against such a DT property:
>> https://lkml.org/lkml/2014/5/12/693
>
> Mike says in that email that he's opposing the addition of a property
> for clocks that is the equivalent of regulator-always-on. That's not
> what this is about. If at all it'd be a property to mark a clock that
> should not be disabled by default because it's essential.
>
> Adding Mike on this subthread too.
>
> Either way, Mark already suggested a different alternative in another
> subthread, namely to add a new kind of checkpoint at which subsystems
> can call a "disable unused" function that's called later than a late
> initcall. This is not going to fix things for you immediately because
> the clocks will still be switched off (only later) if you don't have
> a real driver that's claiming the clocks. But you can work around that
> for now by making the relevant clocks always on and remove that
> workaround once a real driver is loaded that knows how to handle them
> properly.
This will simply not work, and is a ton more complicated then
simply teaching simplefb about a clocks property, which is a really simple
and clean patch.
First of all let me explain why this won't work. When should those
subsystems call this "later then a late initcall" call ? the kms driver
may very well be a kernel module, which will be loaded by userspace,
so we would need this to be triggered by userspace, but when ?
When the initrd is done? What then if the kms driver is not in the initrd,
so maybe when udev is done enumerating devices, but what then if the kernel
is still enumerating hardware at this time? Will we just put a sleep 30
in our initscripts to make sure the kernel is done scanning all busses,
will that be long enough? Maybe we need to re-introduce the scsi_wait_done
kernel module which was added as an ugly hack to allow userspace to wait
for the kernel to have finished scanning scsi (and ata / sata) busses,
for controllers found at the time the module was load. Which never worked
reliable, because it would be possible that the controller itself still
needed to be discovered. We've spend years cleaning up userspace enough
to be able to kill scsi_wait_done. We've already been down this road of
waiting for hw enumeration to be done, and the problem is that on
modern systems *it is never done*.
So second of all, Thierry, what exactly is the technical argument against
adding support for dealing with clocks to simplefb ? I've heard a lot of
people in favor of it, and only pretty much you against it, and your
argument so far seems to boil down to "I don't like it", which is not
really a technical sound argument IMHO. Moreover all the alternatives
seem to be horribly complicated and most of them also seem to have
several issues which will make them simply not work reliable.
Regards,
Hans
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Henrik Nordström @ 2014-08-26 18:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826105451.GD31124@ulmo>
tis 2014-08-26 klockan 12:55 +0200 skrev Thierry Reding:
> So the firmware does control the resources to a point where the display
> hardware is initialized and then it will happily forget about them. The
> device will keep scanning out the framebuffer and simplefb can update it
> with boot messages. To prevent the console from suddenly disappearing
> the kernel needs to make sure that the resources aren't inadvertently
> turned off at some more or less random point in time.
Which imho is until simplefb is unbound from the device.
Separately to this there needs to be a in-kernel handover procedure
where a new driver (i.e. video kms driver) can bind to a device and take
over resources, but it's completely unrelated to who owns the hardware
resources while simplefb is the active driver for the device.
It is not clear to me where the hardware resources should be listed in
DT, being it a simplefb node or part of the actual hardware device node
properly marked as dynamic boot defaults or something else? It's
somewhere inbetween hardware and virtual device, and somewhat volatile.
As far as simplefb is concerned it is a hardware desription of the
framebuffer, but for a kms driver it's no more than firmware handover of
boottime settings and ceases to exists once the kms driver have
reconfigured the hardware.
What I can do is to list some properties that these settings need to
provide:
1. Handover of hardware settings from bootloader to kernel.
2. Hardware description for simplefb use. It's at this level not really
any different from any other hardware.
3. Volatile and removed when kms driver have reconfigured the hardware
or it's been decided that the (virtual) device should not be active.
Regarding 3 and kexec as mentioned earlier. A new set of properties may
be added again for kexec usage to describe the current framebuffer for
the new kernel but op to the system which prepares for kexec to
determine. Should be no different in mechanism from bootloader to kernel
handover (the kexecing kernel is the bootloader in kexec case).
Regards
Henrik
^ permalink raw reply
* Re: [PATCHv2 27/27] OMAPDSS: connector-analog-tv: Add DT support
From: Laurent Pinchart @ 2014-08-26 16:58 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1387205794-32246-28-git-send-email-tomi.valkeinen-l0cyMroinI0@public.gmane.org>
Hi Tomi,
On Monday 16 December 2013 16:56:34 Tomi Valkeinen wrote:
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> .../video/omap2/displays-new/connector-analog-tv.c | 66 ++++++++++++++++++-
> 1 file changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/omap2/displays-new/connector-analog-tv.c
> b/drivers/video/omap2/displays-new/connector-analog-tv.c index
> ccd9073f706f..ebed25a86487 100644
> --- a/drivers/video/omap2/displays-new/connector-analog-tv.c
> +++ b/drivers/video/omap2/displays-new/connector-analog-tv.c
> @@ -12,6 +12,7 @@
> #include <linux/slab.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> +#include <linux/of.h>
>
> #include <video/omapdss.h>
> #include <video/omap-panel-data.h>
> @@ -42,6 +43,12 @@ static const struct omap_video_timings tvc_pal_timings > { .interlace = true,
> };
>
> +static const struct of_device_id tvc_of_match[];
> +
> +struct tvc_of_data {
> + enum omap_dss_venc_type connector_type;
> +};
> +
> #define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
>
> static int tvc_connect(struct omap_dss_device *dssdev)
> @@ -92,7 +99,10 @@ static int tvc_enable(struct omap_dss_device *dssdev)
> in->ops.atv->set_timings(in, &ddata->timings);
>
> in->ops.atv->set_type(in, ddata->connector_type);
> - in->ops.atv->invert_vid_out_polarity(in, ddata->invert_polarity);
> +
> + if (!ddata->dev->of_node)
> + in->ops.atv->invert_vid_out_polarity(in,
> + ddata->invert_polarity);
>
> r = in->ops.atv->enable(in);
> if (r)
> @@ -205,6 +215,35 @@ static int tvc_probe_pdata(struct platform_device
> *pdev) return 0;
> }
>
> +static int tvc_probe_of(struct platform_device *pdev)
> +{
> + struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> + struct device_node *node = pdev->dev.of_node;
> + struct omap_dss_device *in;
> + const struct of_device_id *match;
> + const struct tvc_of_data *data;
> +
> + match = of_match_node(tvc_of_match, pdev->dev.of_node);
> + if (!match) {
> + dev_err(&pdev->dev, "unsupported device\n");
> + return -ENODEV;
> + }
> +
> + data = match->data;
> +
> + in = omapdss_of_find_source_for_first_ep(node);
> + if (IS_ERR(in)) {
> + dev_err(&pdev->dev, "failed to find video source\n");
> + return PTR_ERR(in);
> + }
> +
> + ddata->in = in;
> +
> + ddata->connector_type = data->connector_type;
> +
> + return 0;
> +}
> +
> static int tvc_probe(struct platform_device *pdev)
> {
> struct panel_drv_data *ddata;
> @@ -222,6 +261,10 @@ static int tvc_probe(struct platform_device *pdev)
> r = tvc_probe_pdata(pdev);
> if (r)
> return r;
> + } else if (pdev->dev.of_node) {
> + r = tvc_probe_of(pdev);
> + if (r)
> + return r;
> } else {
> return -ENODEV;
> }
> @@ -263,12 +306,33 @@ static int __exit tvc_remove(struct platform_device
> *pdev) return 0;
> }
>
> +static const struct tvc_of_data tv_svideo_data = {
> + .connector_type = OMAP_DSS_VENC_TYPE_SVIDEO,
> +};
> +
> +static const struct tvc_of_data tv_composite_video_data = {
> + .connector_type = OMAP_DSS_VENC_TYPE_COMPOSITE,
> +};
> +
> +static const struct of_device_id tvc_of_match[] = {
> + {
> + .compatible = "svideo-connector",
> + .data = &tv_svideo_data,
> + },
> + {
> + .compatible = "composite-video-connector",
I've just noticed that this doesn't match the bindings that document the
compatible value to be "composite-connector".
> + .data = &tv_composite_video_data,
> + },
> + {},
> +};
> +
> static struct platform_driver tvc_connector_driver = {
> .probe = tvc_probe,
> .remove = __exit_p(tvc_remove),
> .driver = {
> .name = "connector-analog-tv",
> .owner = THIS_MODULE,
> + .of_match_table = tvc_of_match,
> },
> };
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-26 14:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826123357.GB22395@skynet.be>
[-- Attachment #1: Type: text/plain, Size: 2183 bytes --]
On Tue, Aug 26, 2014 at 02:33:57PM +0200, Luc Verhaegen wrote:
> On Tue, Aug 26, 2014 at 02:21:16PM +0200, Thierry Reding wrote:
> > On Tue, Aug 26, 2014 at 02:02:58PM +0200, Luc Verhaegen wrote:
> > > On Tue, Aug 26, 2014 at 09:52:49AM +0200, Thierry Reding wrote:
> > > > On Mon, Aug 25, 2014 at 05:09:00PM +0200, Luc Verhaegen wrote:
> > > > > On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote:
> > > > > >
> > > > > > No. simplefb just wants to write to some memory that hardware has been
> > > > > > set up to scan out. The platform requires that the clocks be on.
> > > > >
> > > > > Simplefb also requires that the memory is there and is persistent. Fine
> > > > > for discrete graphics cards, fine for rpi where most things are hidden
> > > > > from the ARM core anyway, not so fine for anybody else.
> > > >
> > > > I don't understand. This patch series isn't changing anything about the
> > > > memory aspects of the driver yet it's working for you on sunxi, isn't
> > > > it? So it can't be all that broken.
> > > >
> > > > Thierry
> > >
> > > Oh, i had to go wrestle with UBoot options and reserve 8MB off the top,
> > > which the kernel never gets told about. A nice throwback to x86 IGP
> > > bioses, a past i had thought i had left behind forgood.
> >
> > Can you not use the reserved memory code (drivers/of/of_reserved_mem.c
> > in the kernel)? I think that's the generally accepted way to do this
> > with DT.
> >
> > Thierry
>
> It was mentioned to me, and I probably could do that, but this seemed
> even more DT wrangling from within u-boot, and it didn't seem finished
> yet. Plus, all of this already was way more wrangling than i bargained
> for, especially with the promise of "simple"-fb. Then there is the
> really awkward way in which one gets to define clocks outside of a
> dts/compiler (you've seen the code, right?), i really did not feel like
> repeating something like that.
It could most probably be turned into fairly generic and reusable code
so that others can use it. And it would give you a way of recovering
those 8 MiB of memory that you'd otherwise loose.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-26 14:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826135341.GM15297@lukather>
[-- Attachment #1: Type: text/plain, Size: 3850 bytes --]
On Tue, Aug 26, 2014 at 03:53:41PM +0200, Maxime Ripard wrote:
> On Tue, Aug 26, 2014 at 10:04:33AM +0200, Thierry Reding wrote:
> > > > No. simplefb just wants to write to some memory that hardware has been
> > > > set up to scan out. The platform requires that the clocks be on. Other
> > > > platforms may not even allow turning off the clocks.
> > >
> > > Like what? the rpi? Come on. Just because the videocore is some black
> > > box we know nothing about doesn't mean we should use it as an example.
> >
> > You make it sound like the Raspberry Pi is somehow less important than
> > sunxi.
>
> No. What I mean is that it seems like we are somehow punished, or at
> least blamed, for having a better and more complete kernel support.
This isn't a competition. Nobody's punishing or blaming anyone. This is
about finding the best solution for the problem at hand.
> > > Any decent enough SoC, with a decent support in the kernel will have
> > > clocks for this, and I really wonder how simplefb will behave once its
> > > clocks will be turned off...
> >
> > There are other devices besides ARM SoCs that may want to use this
> > driver and that don't have clock support.
>
> And in this case, with this patch, simplefb will not claim any clock,
> nor will fail probing.
>
> > But you're missing my point. What I'm saying is that the simplefb driver
> > is meant to serve as a way to take over whatever framebuffer a firmware
> > set up. Therefore I think it makes the most sense to assume that nothing
> > needs to be controlled in any way since already been set up by firmware.
> > Eventually there should be a driver that takes over from simplefb that
> > knows how to properly handle the device's specifics, but that's not
> > simplefb.
>
> I guess such a hand over if it were to happen in the kernel would
> involve the second driver claiming the resources before the first one
> release them. How is that different in this case?
It's different in that that driver will be hardware specific and know
exactly what clock and other resources are required. It will have a
device-specific binding.
> > The goal of this patch series is to keep clocks from being turned off.
> > But that's not what it does. What it does is turn clocks on to prevent
> > them from being turned off. In my opinion that's a workaround for a
> > deficiency in the kernel (and the firmware/kernel interface) and I think
> > it should be fixed at the root. So a much better solution would be to
> > establish a way for firmware to communicate to the kernel that a given
> > resource has been enabled by firmware and shouldn't be disabled. Such a
> > solution can be implement for all types of resources and can be reused
> > by all drivers since they don't have to worry about these details.
>
> Mike Turquette repeatedly said that he was against such a DT property:
> https://lkml.org/lkml/2014/5/12/693
Mike says in that email that he's opposing the addition of a property
for clocks that is the equivalent of regulator-always-on. That's not
what this is about. If at all it'd be a property to mark a clock that
should not be disabled by default because it's essential.
Adding Mike on this subthread too.
Either way, Mark already suggested a different alternative in another
subthread, namely to add a new kind of checkpoint at which subsystems
can call a "disable unused" function that's called later than a late
initcall. This is not going to fix things for you immediately because
the clocks will still be switched off (only later) if you don't have
a real driver that's claiming the clocks. But you can work around that
for now by making the relevant clocks always on and remove that
workaround once a real driver is loaded that knows how to handle them
properly.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-26 13:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826080432.GD17263@ulmo>
[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]
On Tue, Aug 26, 2014 at 10:04:33AM +0200, Thierry Reding wrote:
> > > No. simplefb just wants to write to some memory that hardware has been
> > > set up to scan out. The platform requires that the clocks be on. Other
> > > platforms may not even allow turning off the clocks.
> >
> > Like what? the rpi? Come on. Just because the videocore is some black
> > box we know nothing about doesn't mean we should use it as an example.
>
> You make it sound like the Raspberry Pi is somehow less important than
> sunxi.
No. What I mean is that it seems like we are somehow punished, or at
least blamed, for having a better and more complete kernel support.
> > Any decent enough SoC, with a decent support in the kernel will have
> > clocks for this, and I really wonder how simplefb will behave once its
> > clocks will be turned off...
>
> There are other devices besides ARM SoCs that may want to use this
> driver and that don't have clock support.
And in this case, with this patch, simplefb will not claim any clock,
nor will fail probing.
> But you're missing my point. What I'm saying is that the simplefb driver
> is meant to serve as a way to take over whatever framebuffer a firmware
> set up. Therefore I think it makes the most sense to assume that nothing
> needs to be controlled in any way since already been set up by firmware.
> Eventually there should be a driver that takes over from simplefb that
> knows how to properly handle the device's specifics, but that's not
> simplefb.
I guess such a hand over if it were to happen in the kernel would
involve the second driver claiming the resources before the first one
release them. How is that different in this case?
> The goal of this patch series is to keep clocks from being turned off.
> But that's not what it does. What it does is turn clocks on to prevent
> them from being turned off. In my opinion that's a workaround for a
> deficiency in the kernel (and the firmware/kernel interface) and I think
> it should be fixed at the root. So a much better solution would be to
> establish a way for firmware to communicate to the kernel that a given
> resource has been enabled by firmware and shouldn't be disabled. Such a
> solution can be implement for all types of resources and can be reused
> by all drivers since they don't have to worry about these details.
Mike Turquette repeatedly said that he was against such a DT property:
https://lkml.org/lkml/2014/5/12/693
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-26 13:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826132207.GL15297@lukather>
[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]
On Tue, Aug 26, 2014 at 03:22:07PM +0200, Maxime Ripard wrote:
> On Tue, Aug 26, 2014 at 10:40:27AM +0200, Thierry Reding wrote:
> > On Mon, Aug 25, 2014 at 05:18:22PM +0200, Luc Verhaegen wrote:
> > > On Mon, Aug 25, 2014 at 05:12:58PM +0200, Thierry Reding wrote:
> > > >
> > > > Out of curiosity, how does this work in practice? How does the
> > > > bootloader create this entry? Does it scan the DT to see which clocks
> > > > the real hardware device references and then simply copies them to the
> > > > simplefb node?
> > > >
> > > > Thierry
> > >
> > > https://www.mail-archive.com/linux-sunxi@googlegroups.com/msg06619.html
> >
> > That looks like a royal pain. Again, I think it'd be much simpler (but
> > not less code, unfortunately) to do this on a per-resource basis. That
> > way these low-level firmware drivers in the kernel can stay trivial,
> > keeping the real complexity where they belong: in hardware-specific
> > drivers such as DRM/KMS.
>
> So we have to write a DRM/KMS driver in order to have display working
> while waiting for a DRM/KMS driver to be worked on? Come on...
Perhaps you should go and read this thread again. That's not at all what
I've been saying.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-26 13:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826084026.GF17263@ulmo>
[-- Attachment #1: Type: text/plain, Size: 1144 bytes --]
On Tue, Aug 26, 2014 at 10:40:27AM +0200, Thierry Reding wrote:
> On Mon, Aug 25, 2014 at 05:18:22PM +0200, Luc Verhaegen wrote:
> > On Mon, Aug 25, 2014 at 05:12:58PM +0200, Thierry Reding wrote:
> > >
> > > Out of curiosity, how does this work in practice? How does the
> > > bootloader create this entry? Does it scan the DT to see which clocks
> > > the real hardware device references and then simply copies them to the
> > > simplefb node?
> > >
> > > Thierry
> >
> > https://www.mail-archive.com/linux-sunxi@googlegroups.com/msg06619.html
>
> That looks like a royal pain. Again, I think it'd be much simpler (but
> not less code, unfortunately) to do this on a per-resource basis. That
> way these low-level firmware drivers in the kernel can stay trivial,
> keeping the real complexity where they belong: in hardware-specific
> drivers such as DRM/KMS.
So we have to write a DRM/KMS driver in order to have display working
while waiting for a DRM/KMS driver to be worked on? Come on...
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-26 13:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53FB555E.5040409@suse.de>
[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]
Hi,
On Mon, Aug 25, 2014 at 05:25:18PM +0200, Andreas Färber wrote:
> Hi,
>
> Am 25.08.2014 15:47, schrieb Hans de Goede:
> > On 08/25/2014 03:39 PM, Thierry Reding wrote:
> >> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> >>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> >>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> >>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> >>>>>> If not, perhaps the clock driver should force the clock to be
> >>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
> [...]
> > As for the suggestion to simply never disable the plls / ahb gates by blocking
> > them from ever being disabled in the sunxi clock driver, that is not really
> > a solution either, as we want to be able to turn these things off to safe
> > power on screen blank once control has been turned over to the kms driver.
>
> Without wanting to take sides on the simplefb matter, can't you just use
> clk_ignore_unused in bootargs to work around the issue at hand?
> That's what I do on the Spring Chromebook.
Leaving *all* the clocks enabled just to workaround a driver failing
to grab its clocks doesn't look like a solution either.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/2] msm: msm_fb: Add remove function for platform driver for clean unloading
From: Tomi Valkeinen @ 2014-08-26 12:52 UTC (permalink / raw)
To: Pramod Gurav, linux-kernel, linux-fbdev
Cc: Jean-Christophe Plagniol-Villard, Stephen Boyd, Jingoo Han,
Rob Clark
In-Reply-To: <1409055038-9204-1-git-send-email-pramod.gurav@smartplayin.com>
[-- Attachment #1: Type: text/plain, Size: 283 bytes --]
On 26/08/14 15:10, Pramod Gurav wrote:
> This adds a remove function to platform driver structure so that
> resources are released when driver is unloaded.
>
> Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com>
>
Thanks, queued this series for 3.18.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH v2 2/2] msm: msm_fb: Move to using managed resources of kzalloc
From: Pramod Gurav @ 2014-08-26 12:49 UTC (permalink / raw)
To: linux-kernel, linux-fbdev
Cc: Pramod Gurav, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Stephen Boyd, Jingoo Han, Rob Clark
Move to managed verion of kzalloc. Also checks return for failure case
which was missing.
Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com>
CC: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
CC: Stephen Boyd <sboyd@codeaurora.org>
CC: Jingoo Han <jg1.han@samsung.com>
CC: Rob Clark <robdclark@gmail.com>
---
drivers/video/fbdev/msm/msm_fb.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/msm/msm_fb.c b/drivers/video/fbdev/msm/msm_fb.c
index 4640188..2979d7e 100644
--- a/drivers/video/fbdev/msm/msm_fb.c
+++ b/drivers/video/fbdev/msm/msm_fb.c
@@ -569,8 +569,13 @@ static int msmfb_probe(struct platform_device *pdev)
mutex_init(&msmfb->panel_init_lock);
init_waitqueue_head(&msmfb->frame_wq);
INIT_WORK(&msmfb->resume_work, power_on_panel);
- msmfb->black = kzalloc(msmfb->fb->var.bits_per_pixel*msmfb->xres,
- GFP_KERNEL);
+ msmfb->black = devm_kzalloc(&pdev->dev,
+ msmfb->fb->var.bits_per_pixel*msmfb->xres,
+ GFP_KERNEL);
+ if (!msmfb->black) {
+ ret = -ENOMEM;
+ goto error_register_framebuffer;
+ }
printk(KERN_INFO "msmfb_probe() installing %d x %d panel\n",
msmfb->xres, msmfb->yres);
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v2 trivial 5/6] video: mx3fb: Update comment for dmaengine_prep_slave_sg() API
From: Tomi Valkeinen @ 2014-08-26 12:42 UTC (permalink / raw)
To: Geert Uytterhoeven, Vinod Koul, Dan Williams
Cc: dmaengine, linux-kernel, Jean-Christophe Plagniol-Villard,
Jiri Kosina, linux-fbdev
In-Reply-To: <1405095208-14642-5-git-send-email-geert+renesas@glider.be>
[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]
On 11/07/14 19:13, Geert Uytterhoeven wrote:
> Commit 16052827d98fbc13c31ebad560af4bd53e2b4dd5 ("dmaengine/dma_slave:
> introduce inline wrappers") changed the code to use the new API, but forgot
> to update a comment.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Jiri Kosina <trivial@kernel.org>
> Cc: linux-fbdev@vger.kernel.org
> --
> v2:
> - New
> ---
> drivers/video/fbdev/mx3fb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/mx3fb.c b/drivers/video/fbdev/mx3fb.c
> index c645a0a0c341..5e97baf92721 100644
> --- a/drivers/video/fbdev/mx3fb.c
> +++ b/drivers/video/fbdev/mx3fb.c
> @@ -1179,7 +1179,7 @@ static int mx3fb_pan_display(struct fb_var_screeninfo *var,
>
> /*
> * We enable the End of Frame interrupt, which will free a tx-descriptor,
> - * which we will need for the next device_prep_slave_sg(). The
> + * which we will need for the next dmaengine_prep_slave_sg(). The
> * IRQ-handler will disable the IRQ again.
> */
> init_completion(&mx3_fbi->flip_cmpl);
>
Thanks, queued for 3.18.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 1/1] video: vermilion: remove unnecessary break after goto
From: Tomi Valkeinen @ 2014-08-26 12:41 UTC (permalink / raw)
To: Fabian Frederick, linux-kernel
Cc: Jean-Christophe Plagniol-Villard, Jingoo Han, linux-fbdev
In-Reply-To: <1404836838-6953-1-git-send-email-fabf@skynet.be>
[-- Attachment #1: Type: text/plain, Size: 869 bytes --]
On 08/07/14 19:27, Fabian Frederick wrote:
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: linux-fbdev@vger.kernel.org
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
> ---
> drivers/video/fbdev/vermilion/vermilion.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/vermilion/vermilion.c b/drivers/video/fbdev/vermilion/vermilion.c
> index 048a666..5f930ae 100644
> --- a/drivers/video/fbdev/vermilion/vermilion.c
> +++ b/drivers/video/fbdev/vermilion/vermilion.c
> @@ -481,7 +481,6 @@ static int vml_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> default:
> err = -ENODEV;
> goto out_err_1;
> - break;
> }
>
> info = &vinfo->info;
>
Thanks, queued for 3.18.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] video: da8xx-fb: preserve display width when changing HSYNC
From: Tomi Valkeinen @ 2014-08-26 12:37 UTC (permalink / raw)
To: Ian Abbott, linux-fbdev; +Cc: Jean-Christophe Plagniol-Villard, linux-kernel
In-Reply-To: <1404395199-5726-1-git-send-email-abbotti@mev.co.uk>
[-- Attachment #1: Type: text/plain, Size: 1373 bytes --]
On 03/07/14 16:46, Ian Abbott wrote:
> When looking at this driver for a client, I noticed the code that
> configures the HSYNC pulse clobbers the display width in the same
> register. It only preserves the MS part of the width in bit 3 and zeros
> the LS part of the width in bits 9 to 4. This doesn't matter during
> initialization as the width is configured afterwards, but subsequent use
> of the FBIPUT_HSYNC ioctl would clobber the width.
>
> Preserve bits 9 to 0 of LCD_RASTER_TIMING_0_REG when configuring the
> horizontal sync.
>
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
> I haven't tested this change, but it's pretty trivial.
> ---
> drivers/video/fbdev/da8xx-fb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/da8xx-fb.c b/drivers/video/fbdev/da8xx-fb.c
> index 788f6b3..10c876c 100644
> --- a/drivers/video/fbdev/da8xx-fb.c
> +++ b/drivers/video/fbdev/da8xx-fb.c
> @@ -419,7 +419,7 @@ static void lcd_cfg_horizontal_sync(int back_porch, int pulse_width,
> {
> u32 reg;
>
> - reg = lcdc_read(LCD_RASTER_TIMING_0_REG) & 0xf;
> + reg = lcdc_read(LCD_RASTER_TIMING_0_REG) & 0x3ff;
> reg |= (((back_porch-1) & 0xff) << 24)
> | (((front_porch-1) & 0xff) << 16)
> | (((pulse_width-1) & 0x3f) << 10);
>
Thanks, queued for 3.17 fixes.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Luc Verhaegen @ 2014-08-26 12:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826122112.GC641@ulmo>
On Tue, Aug 26, 2014 at 02:21:16PM +0200, Thierry Reding wrote:
> On Tue, Aug 26, 2014 at 02:02:58PM +0200, Luc Verhaegen wrote:
> > On Tue, Aug 26, 2014 at 09:52:49AM +0200, Thierry Reding wrote:
> > > On Mon, Aug 25, 2014 at 05:09:00PM +0200, Luc Verhaegen wrote:
> > > > On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote:
> > > > >
> > > > > No. simplefb just wants to write to some memory that hardware has been
> > > > > set up to scan out. The platform requires that the clocks be on.
> > > >
> > > > Simplefb also requires that the memory is there and is persistent. Fine
> > > > for discrete graphics cards, fine for rpi where most things are hidden
> > > > from the ARM core anyway, not so fine for anybody else.
> > >
> > > I don't understand. This patch series isn't changing anything about the
> > > memory aspects of the driver yet it's working for you on sunxi, isn't
> > > it? So it can't be all that broken.
> > >
> > > Thierry
> >
> > Oh, i had to go wrestle with UBoot options and reserve 8MB off the top,
> > which the kernel never gets told about. A nice throwback to x86 IGP
> > bioses, a past i had thought i had left behind forgood.
>
> Can you not use the reserved memory code (drivers/of/of_reserved_mem.c
> in the kernel)? I think that's the generally accepted way to do this
> with DT.
>
> Thierry
It was mentioned to me, and I probably could do that, but this seemed
even more DT wrangling from within u-boot, and it didn't seem finished
yet. Plus, all of this already was way more wrangling than i bargained
for, especially with the promise of "simple"-fb. Then there is the
really awkward way in which one gets to define clocks outside of a
dts/compiler (you've seen the code, right?), i really did not feel like
repeating something like that.
Luc Verhaegen.
^ permalink raw reply
* [PATCH v2 1/2] msm: msm_fb: Add remove function for platform driver for clean unloading
From: Pramod Gurav @ 2014-08-26 12:22 UTC (permalink / raw)
To: linux-kernel, linux-fbdev
Cc: Pramod Gurav, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Stephen Boyd, Jingoo Han, Rob Clark
This adds a remove function to platform driver structure so that
resources are released when driver is unloaded.
Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com>
CC: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
CC: Stephen Boyd <sboyd@codeaurora.org>
CC: Jingoo Han <jg1.han@samsung.com>
CC: Rob Clark <robdclark@gmail.com>
---
drivers/video/fbdev/msm/msm_fb.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/video/fbdev/msm/msm_fb.c b/drivers/video/fbdev/msm/msm_fb.c
index 1374803..4640188 100644
--- a/drivers/video/fbdev/msm/msm_fb.c
+++ b/drivers/video/fbdev/msm/msm_fb.c
@@ -589,6 +589,8 @@ static int msmfb_probe(struct platform_device *pdev)
msmfb->sleeping = WAKING;
+ platform_set_drvdata(pdev, msmfb);
+
return 0;
error_register_framebuffer:
@@ -598,9 +600,23 @@ error_setup_fbmem:
return ret;
}
+static int msmfb_remove(struct platform_device *pdev)
+{
+ struct msmfb_info *msmfb;
+
+ msmfb = platform_get_drvdata(pdev);
+
+ unregister_framebuffer(msmfb->fb);
+ iounmap(msmfb->fb->screen_base);
+ framebuffer_release(msmfb->fb);
+
+ return 0;
+}
+
static struct platform_driver msm_panel_driver = {
/* need to write remove */
.probe = msmfb_probe,
+ .remove = msmfb_remove,
.driver = {.name = "msm_panel"},
};
--
1.7.9.5
^ permalink raw reply related
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-26 12:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826120258.GA22395@skynet.be>
[-- Attachment #1: Type: text/plain, Size: 1258 bytes --]
On Tue, Aug 26, 2014 at 02:02:58PM +0200, Luc Verhaegen wrote:
> On Tue, Aug 26, 2014 at 09:52:49AM +0200, Thierry Reding wrote:
> > On Mon, Aug 25, 2014 at 05:09:00PM +0200, Luc Verhaegen wrote:
> > > On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote:
> > > >
> > > > No. simplefb just wants to write to some memory that hardware has been
> > > > set up to scan out. The platform requires that the clocks be on.
> > >
> > > Simplefb also requires that the memory is there and is persistent. Fine
> > > for discrete graphics cards, fine for rpi where most things are hidden
> > > from the ARM core anyway, not so fine for anybody else.
> >
> > I don't understand. This patch series isn't changing anything about the
> > memory aspects of the driver yet it's working for you on sunxi, isn't
> > it? So it can't be all that broken.
> >
> > Thierry
>
> Oh, i had to go wrestle with UBoot options and reserve 8MB off the top,
> which the kernel never gets told about. A nice throwback to x86 IGP
> bioses, a past i had thought i had left behind forgood.
Can you not use the reserved memory code (drivers/of/of_reserved_mem.c
in the kernel)? I think that's the generally accepted way to do this
with DT.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Luc Verhaegen @ 2014-08-26 12:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826075247.GC17263@ulmo>
On Tue, Aug 26, 2014 at 09:52:49AM +0200, Thierry Reding wrote:
> On Mon, Aug 25, 2014 at 05:09:00PM +0200, Luc Verhaegen wrote:
> > On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote:
> > >
> > > No. simplefb just wants to write to some memory that hardware has been
> > > set up to scan out. The platform requires that the clocks be on.
> >
> > Simplefb also requires that the memory is there and is persistent. Fine
> > for discrete graphics cards, fine for rpi where most things are hidden
> > from the ARM core anyway, not so fine for anybody else.
>
> I don't understand. This patch series isn't changing anything about the
> memory aspects of the driver yet it's working for you on sunxi, isn't
> it? So it can't be all that broken.
>
> Thierry
Oh, i had to go wrestle with UBoot options and reserve 8MB off the top,
which the kernel never gets told about. A nice throwback to x86 IGP
bioses, a past i had thought i had left behind forgood.
Luc Verhaegen.
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Mark Brown @ 2014-08-26 11:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826105451.GD31124@ulmo>
[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]
On Tue, Aug 26, 2014 at 12:55:06PM +0200, Thierry Reding wrote:
> On Tue, Aug 26, 2014 at 11:06:12AM +0100, Mark Brown wrote:
> > On Tue, Aug 26, 2014 at 11:18:54AM +0200, Thierry Reding wrote:
> > > On Tue, Aug 26, 2014 at 10:00:35AM +0100, Mark Brown wrote:
> > > > We can't assume that the "proper" setup is that the supply should be
> > > > left on.
> > > Right, we can't assume it, but if we're given appropriate hints I think
> > > it's fair to keep resources set up by firmware untouched.
> > I really can't see any sensible way to do that in an OS neutral thing
> > like DT.
> It should be possible to simply define a property for this that the
> firmware can add to the device tree.
> There are other subsystems for example where we don't touch resources
> unless they are explicitly accessed. GPIO and pinmux are two examples of
> those. The reason is precisely so that we don't undo anything that the
> firmware already set up and that may be needed during boot.
GPIOs and pinmuxes are quite different in both how they get set up (you
don't typically have their configuration set in ROM and shoehorned into
the board which is what we're dealing with for a lot of PMICs) and in
the consequences of leaving them alone.
I really don't think defining a custom property to work around a timing
issue in a particular OS startup sequence is the best way forwards here
- we should solve our startup sequencing issue rather than layer on
hacks, otherwise we're adding fragility and work to the system. The
flag would need to be explicitly added and then it means we're stuck
with the behaviour even if the system gets smarter.
> Before booting the kernel it will modify the DTB and insert a node that
> contains information about the framebuffer that was set up. The node
> contains properties that define the resolution, the format and the
> physical address of the framebuffer memory. An in-kernel driver will
> then be able to use that information very early in the boot process to
> show the console (to replace the serial console that's usually not
> available on consumer devices). Later on when a real driver can be
> loaded it should take over the resources from simplefb and remove it.
OK, so this is not meaningfully different to just loading the driver
normally - it's exactly the same scenario.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [patch] video: of: display_timing: double free on error
From: Tomi Valkeinen @ 2014-08-26 11:30 UTC (permalink / raw)
To: linux-fbdev
[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]
On 11/07/14 12:21, Dan Carpenter wrote:
> The display_timings_release() function frees "disp" and we free it
> again on the next line.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
> index 987edf1..5c098d5 100644
> --- a/drivers/video/of_display_timing.c
> +++ b/drivers/video/of_display_timing.c
> @@ -236,6 +236,7 @@ timingfail:
> if (native_mode)
> of_node_put(native_mode);
> display_timings_release(disp);
> + disp = NULL;
> entryfail:
> kfree(disp);
> dispfail:
>
Thanks, queued for 3.17 fbdev fixes.
The code looks a bit messy to me, though. I'm not fond of "kalloc here,
kfree there" style.
Maybe this would be slightly cleaner, if display_timings_release() would
accept NULL parameter. Then the code above could just use
display_timings_release(), and there'd be no need for kfree.
In any case, that's stuff for an other patch. This one is fine fix for 3.17.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ 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