* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Andreas Färber @ 2014-08-25 15:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53FB3E7F.4000503@redhat.com>
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.
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: jonsmirl @ 2014-08-25 15:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825151255.GA14967@ulmo.nvidia.com>
On Mon, Aug 25, 2014 at 11:12 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Aug 25, 2014 at 04:27:04PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 08/25/2014 04:23 PM, jonsmirl@gmail.com wrote:
>> > On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding
>> > <thierry.reding@gmail.com> wrote:
>> >> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
>> >>> 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?).
>> >>>>>>>
>> >>>>>>> I'm sorry, but I'm not going to take any code that will do that in our
>> >>>>>>> clock driver.
>> >>>>>>>
>> >>>>>>> I'm not going to have a huge list of ifdef depending on configuration
>> >>>>>>> options to know which clock to enable, especially when clk_get should
>> >>>>>>> have the consumer device as an argument.
>> >>>>>>
>> >>>>>> Are you saying is that you want to solve a platform-specific problem by
>> >>>>>> pushing code into simple, generic drivers so that your platform code can
>> >>>>>> stay "clean"?
>> >>>>>
>> >>>>> Are you saying that this driver would become "dirty" with such a patch?
>> >>>>
>> >>>> Yes. Others have said the same and even provided alternative solutions
>> >>>> on how to solve what's seemingly a platform-specific problem in a
>> >>>> platform-specific way.
>> >>>
>> >>> This is not platform specific, any platform with a complete clock driver
>> >>> will suffer from the same problem (the clock driver disabling unclaimed
>> >>> ahb gates, and thus killing the video output) if it wants to use simplefb
>> >>> for early console support.
>> >>
>> >> It is platform specific in that your platform may require certain clocks
>> >> to remain on. The next platform may require power domains to remain on
>> >> during boot and yet another one may rely on regulators to stay on during
>> >> boot. By your argument simplefb will need to be taught to handle pretty
>> >> much every type of resource that the kernel has.
>> >
>> > Why can't simplefb be a driver library that is called from a device
>> > specific device driver that only claims the clocks (or regulators)?
>> > Then build all of these device specific drivers into the generic ARM
>> > kernel. They will be quite small since all they do is claim the clocks
>> > (or regulator). Maybe we can even figure out some protocol for
>> > removing the unused ones from memory later.
>> >
>> > Later during the boot process the device specific driver can load its
>> > KMS code which has also been implemented as a driver library. Maybe
>> > use E_PROBE_DEFER to do this. Match on the device ID, claim the
>> > clocks, defer until the full KMS library can be loaded.
>>
>> There is no need for all this complexity, all that is needed is for the
>> simplefb driver to be thought to claim + enable any clocks listed in
>> its dt node.
>
> 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?
That's why this is such a problem. There is a general rule in Linux -
one device, one driver. All of the kernel device driver code is
written around this assumption. We're trying to make two drivers for
one device and the kernel is not designed to support that. There
shouldn't be an independent 'simplefb' node. That is creating two
device descriptions for a single device.
Adding 'chosen' information to the devices nodes might work.
video@1345 {
compatible = "sunxi-a20-video";
reg = <...>;
clocks = <...>;
chosen {
compatible = "simplefb"; // dynamically added by uboot
buffer = <0x4564>; // dynamically added by uboot
};
};
This will initialize the simplefb driver without attaching it to any
specific hardware. It can then look in its parent node for clocks and
regulators and claim them.
>
> Thierry
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* [PATCH v4 00/15] Rework OMAP4+ HDMI audio support
From: Jyri Sarha @ 2014-08-25 19:04 UTC (permalink / raw)
To: alsa-devel, linux-fbdev, linux-omap
Cc: peter.ujfalusi, broonie, liam.r.girdwood, tomi.valkeinen,
detheridge, Jyri Sarha
The patches are based on:
git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git for-next
The base, the patches, and couple of additional not-to-be-merged
omap2plus_defconfig patches can be found here:
git://git.ti.com/~jyrisarha/ti-linux-kernel/jyrisarhas-audio-video-linux-feature-tree.git omap-hdmi-audio
Changes since v3:
- Move ASoC parts into library module under sound/soc/omap
- Come up with API for the library
- Some cleaning up
The patch set fixes OMAP4+ HDMI audio. The structure of the
implementation looks a bit different than before. Instead of creating
a driver specific API for a separate ASoC component driver to connect
to, these patches implement a single audio library module under
sound/soc/omap for ASoC side implementation. The library exports
omap_hdmi_audio_register() and omap_hdmi_audio_unregister()
functions. With the functions OMAPDSS HDMI implementation registers
and unregisters all ASoC components needed for OMAP HDMI audio.
The library implements cpu-dai component using the callbacks provided
by OMAPDSS. Omap-pcm is registered for platform component, dummy
hdmi-audio-codec is registered for codec component, and
asoc-simple-card is registered for machine driver.
Big part of the HDMI audio code is still unchanged and there is a need
for a cleanup there. Also there is still probably something wrong with
speaker mapping of multi-channel streams. I will get back to cleaning
up these issues later.
Best regards,
Jyri
Jyri Sarha (15):
OMAPDSS: hdmi.h: Add HDMI_AUDIO_LAYOUT_6CH enum value
OMAPDSS: hdmi: Remove most of OMAP[45]_DSS_HDMI_AUDIO ifdefs
OMAPDSS: hdmi4_core: Remove unused hdmi4_audio_get_dma_port()
OMAPDSS: hdmi_wp: Add function for getting audio dma address
OMAPDSS: hdmi: Make hdmi structure public
OMAPDSS: hdmi: Add exported functions for storing HDMI audio data
OMAPDSS: hdmi: Make hdmi_mode_has_audio() more user friedly
ASoC: omap-hdmi-audio: Add OMAP HDMI audio support library
OMAPDSS: Kconfig: Implement options for OMAP4 and OMAP5 HDMI audio
support
OMAPDSS: hdmi4: Register HDMI audio with omap_hdmi_audio_register()
OMAPDSS: hdmi5: Register HDMI audio with omap_hdmi_audio_register()
ASoC: omap: Remove obsolete HDMI audio code and Kconfig options
OMAPDSS: hdmi4: Remove callbacks for an external ASoC DAI driver
OMAPDSS: hdmi5: Remove callbacks for an external ASoC DAI driver
OMAPDSS: Remove all references to obsolete HDMI audio callbacks
.../fbdev/omap2/displays-new/connector-hdmi.c | 99 ------
.../fbdev/omap2/displays-new/encoder-tpd12s015.c | 56 ---
drivers/video/fbdev/omap2/dss/Kconfig | 28 +-
drivers/video/fbdev/omap2/dss/hdmi.h | 35 +-
drivers/video/fbdev/omap2/dss/hdmi4.c | 233 ++++++-------
drivers/video/fbdev/omap2/dss/hdmi4_core.c | 14 -
drivers/video/fbdev/omap2/dss/hdmi4_core.h | 4 -
drivers/video/fbdev/omap2/dss/hdmi5.c | 216 +++++-------
drivers/video/fbdev/omap2/dss/hdmi5_core.c | 6 -
drivers/video/fbdev/omap2/dss/hdmi5_core.h | 2 -
drivers/video/fbdev/omap2/dss/hdmi_common.c | 18 +-
drivers/video/fbdev/omap2/dss/hdmi_wp.c | 8 +-
include/sound/omap-hdmi-audio.h | 50 +++
include/video/omapdss.h | 34 +-
sound/soc/omap/Kconfig | 15 +-
sound/soc/omap/Makefile | 6 +-
sound/soc/omap/omap-hdmi-audio.c | 357 +++++++++++++++++++
sound/soc/omap/omap-hdmi-card.c | 87 -----
sound/soc/omap/omap-hdmi.c | 364 --------------------
sound/soc/omap/omap-hdmi.h | 38 --
20 files changed, 676 insertions(+), 994 deletions(-)
create mode 100644 include/sound/omap-hdmi-audio.h
create mode 100644 sound/soc/omap/omap-hdmi-audio.c
delete mode 100644 sound/soc/omap/omap-hdmi-card.c
delete mode 100644 sound/soc/omap/omap-hdmi.c
delete mode 100644 sound/soc/omap/omap-hdmi.h
--
1.7.9.5
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-26 7:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825150900.GB21020@skynet.be>
[-- Attachment #1: Type: text/plain, Size: 691 bytes --]
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
[-- 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 8:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825152232.GE15297@lukather>
[-- Attachment #1: Type: text/plain, Size: 4277 bytes --]
On Mon, Aug 25, 2014 at 05:22:32PM +0200, Maxime Ripard wrote:
> On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote:
> > On Mon, Aug 25, 2014 at 04:58:54PM +0200, Maxime Ripard wrote:
> > > On Mon, Aug 25, 2014 at 04:16:29PM +0200, Thierry Reding wrote:
> > > > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
> > > > > 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?).
> > > > > >>>>
> > > > > >>>> I'm sorry, but I'm not going to take any code that will do that in our
> > > > > >>>> clock driver.
> > > > > >>>>
> > > > > >>>> I'm not going to have a huge list of ifdef depending on configuration
> > > > > >>>> options to know which clock to enable, especially when clk_get should
> > > > > >>>> have the consumer device as an argument.
> > > > > >>>
> > > > > >>> Are you saying is that you want to solve a platform-specific problem by
> > > > > >>> pushing code into simple, generic drivers so that your platform code can
> > > > > >>> stay "clean"?
> > > > > >>
> > > > > >> Are you saying that this driver would become "dirty" with such a patch?
> > > > > >
> > > > > > Yes. Others have said the same and even provided alternative solutions
> > > > > > on how to solve what's seemingly a platform-specific problem in a
> > > > > > platform-specific way.
> > > > >
> > > > > This is not platform specific, any platform with a complete clock driver
> > > > > will suffer from the same problem (the clock driver disabling unclaimed
> > > > > ahb gates, and thus killing the video output) if it wants to use simplefb
> > > > > for early console support.
> > > >
> > > > It is platform specific in that your platform may require certain clocks
> > > > to remain on.
> > >
> > > The platform doesn't. simplefb does. simplefb is the obvious consumer
> > > for these clocks, and given the current API and abstraction we have,
> > > it should be the one claiming the clocks too.
> >
> > 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.
> 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.
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.
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.
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 8:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825150705.GB15297@lukather>
[-- Attachment #1: Type: text/plain, Size: 2793 bytes --]
On Mon, Aug 25, 2014 at 05:07:05PM +0200, Maxime Ripard wrote:
> On Mon, Aug 25, 2014 at 04:53:06PM +0200, Thierry Reding wrote:
> > Hmm... that's true. But we already have a way to deal with exactly this
> > situation for regulators. There's a property called regulator-boot-on
> > which a bootloader should set whet it has enabled a given regulator. It
> > can of course also be set statically in a DTS if it's know upfront that
> > a bootloader will always enable it. Perhaps what we need is a similar
> > property for clocks so that the clock framework will not inadvertently
> > turn off a clock that's still being used.
>
> Except that such a property won't work either.
Of course it won't work if it's designed not to work. The solution to
that is to design it in a way that it works and does exactly what we
want it to do.
> Regulators with regulator-boot-on will still be disabled if there's no
> one to claim it. Just like what happens currently for the clocks.
I was somewhat surprised by this, but it can indeed easily be verified.
It seems to me somewhat at odds with the definition of such a property.
Mark,
You've probably not read the whole backlog, but the discussion revolves
around hand-off of resources from firmware to kernel (via DT in this
case). If firmware initializes a device (display controller in this
particular case) and enables resources needed by the device to work
properly then in order to properly hand over resources from firmware to
kernel we need a way to communicate the state of these resources. For
regulators the regulator-boot-on property is specified to do exactly
that. However the implementation will automatically disable a regulator
marked boot-on if it hasn't been claimed by any driver after the
initcall stage completes.
I find that rather odd since I always assumed that a regulator marked
boot-on would not be touched by the core at all, assuming that firmware
set it up properly and that it would be required (even if not explicitly
claimed).
The issue that this causes is that we can't properly hand-off devices
initialized by firmware because the regulator will be disabled after the
initcall stage and then enabled when the driver loads. In case of
display the result will usually be flicker. The same applies to other
types of resources (in this case clocks).
Two categories of drivers have this issue: drivers built as modules (or
that defer probing) and therefore won't be probed until after initcalls
have run and generic low-level drivers that take over firmware devices
(simplefb in this case) that don't know anything about the resource that
the devices need.
Also Cc'ing Mike, perhaps he has ideas on how to solve this problem for
clocks specifically.
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 8:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825151822.GC21020@skynet.be>
[-- Attachment #1: Type: text/plain, Size: 796 bytes --]
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.
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: Michal Suchanek @ 2014-08-26 8:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826080432.GD17263@ulmo>
On 26 August 2014 10:04, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Aug 25, 2014 at 05:22:32PM +0200, Maxime Ripard wrote:
>> On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote:
>> > On Mon, Aug 25, 2014 at 04:58:54PM +0200, Maxime Ripard wrote:
>> > > On Mon, Aug 25, 2014 at 04:16:29PM +0200, Thierry Reding wrote:
>> > > > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
>> > > > > 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?).
>> > > > > >>>>
>> > > > > >>>> I'm sorry, but I'm not going to take any code that will do that in our
>> > > > > >>>> clock driver.
>> > > > > >>>>
>> > > > > >>>> I'm not going to have a huge list of ifdef depending on configuration
>> > > > > >>>> options to know which clock to enable, especially when clk_get should
>> > > > > >>>> have the consumer device as an argument.
>> > > > > >>>
>> > > > > >>> Are you saying is that you want to solve a platform-specific problem by
>> > > > > >>> pushing code into simple, generic drivers so that your platform code can
>> > > > > >>> stay "clean"?
>> > > > > >>
>> > > > > >> Are you saying that this driver would become "dirty" with such a patch?
>> > > > > >
>> > > > > > Yes. Others have said the same and even provided alternative solutions
>> > > > > > on how to solve what's seemingly a platform-specific problem in a
>> > > > > > platform-specific way.
>> > > > >
>> > > > > This is not platform specific, any platform with a complete clock driver
>> > > > > will suffer from the same problem (the clock driver disabling unclaimed
>> > > > > ahb gates, and thus killing the video output) if it wants to use simplefb
>> > > > > for early console support.
>> > > >
>> > > > It is platform specific in that your platform may require certain clocks
>> > > > to remain on.
>> > >
>> > > The platform doesn't. simplefb does. simplefb is the obvious consumer
>> > > for these clocks, and given the current API and abstraction we have,
>> > > it should be the one claiming the clocks too.
>> >
>> > 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. It's just bad example for driver development because on rpi you
are not in control of the hardware. Then the issue of enabling and
disabling clock becomes moot because you do not have access to clock
at all.
>
>> 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.
Then the clock support part should be compiled out in that case. Just
like any other driver that has code that is disabled when some kernel
susbsystem is compiled out.
>
> 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.
The simplefb driver is not controlling anything but is using resources
set up by firmware.
Currently DT has no way to express that state so the proposed solution
is to state in DT that simplefb controls the resources that are needed
for the framebuffer on the platform. Which resources are that is
platform specific and that platform specific is handled by the
bootloader that fills in what resources simplefb so 'controls' in its
DT node.
This way the resource is not disabled by the generic kernel framework
such as the clock framework.
>
> 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.
That's not the case here. If you later decide you do not want a
framebuffer you should be able to stop simplefb, disable the clock
that it claimed and reclaim the fb memory even if you do not replace
it with a full KMS driver. That way you would not need a special
'headless server bootloader' so long as you do not mind your server os
installation enables the display temporarily during boot.
Thanks
Michal
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Mark Brown @ 2014-08-26 9:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826082626.GE17263@ulmo>
[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]
On Tue, Aug 26, 2014 at 10:26:27AM +0200, Thierry Reding wrote:
> On Mon, Aug 25, 2014 at 05:07:05PM +0200, Maxime Ripard wrote:
> > Regulators with regulator-boot-on will still be disabled if there's no
> > one to claim it. Just like what happens currently for the clocks.
> I was somewhat surprised by this, but it can indeed easily be verified.
> It seems to me somewhat at odds with the definition of such a property.
That depends what you think it should do - it's there for handover from
the bootloader in cases where we can't read the state.
> that. However the implementation will automatically disable a regulator
> marked boot-on if it hasn't been claimed by any driver after the
> initcall stage completes.
> I find that rather odd since I always assumed that a regulator marked
> boot-on would not be touched by the core at all, assuming that firmware
> set it up properly and that it would be required (even if not explicitly
> claimed).
No, there's a separate always-on property if we don't want to disable.
We can't assume that the "proper" setup is that the supply should be
left on.
> Two categories of drivers have this issue: drivers built as modules (or
> that defer probing) and therefore won't be probed until after initcalls
We really need a later initcall to manage handover to userspace
(probably triggered by a combination of userspace saying it's done doing
initial module enumeration and the deferred probe grinding to a halt).
> have run and generic low-level drivers that take over firmware devices
> (simplefb in this case) that don't know anything about the resource that
> the devices need.
I don't understand this use case, sorry - it sounds like a buggy system
design and/or integration. If the firmware is managing the resource
then Linux really shouldn't be talking to it at all without coordinating
with firmware. How can such a system work otherwise?
[-- 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 9:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKON4OzR=JACaVE2ATm6n95LCTbH2rXBzG_ExkD1kO8Qzzf1CA@mail.gmail.com>
Hi,
On 08/25/2014 05:49 PM, jonsmirl@gmail.com wrote:
> On Mon, Aug 25, 2014 at 11:12 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>> On Mon, Aug 25, 2014 at 04:27:04PM +0200, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 08/25/2014 04:23 PM, jonsmirl@gmail.com wrote:
>>>> On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding
>>>> <thierry.reding@gmail.com> wrote:
>>>>> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
>>>>>> 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?).
>>>>>>>>>>
>>>>>>>>>> I'm sorry, but I'm not going to take any code that will do that in our
>>>>>>>>>> clock driver.
>>>>>>>>>>
>>>>>>>>>> I'm not going to have a huge list of ifdef depending on configuration
>>>>>>>>>> options to know which clock to enable, especially when clk_get should
>>>>>>>>>> have the consumer device as an argument.
>>>>>>>>>
>>>>>>>>> Are you saying is that you want to solve a platform-specific problem by
>>>>>>>>> pushing code into simple, generic drivers so that your platform code can
>>>>>>>>> stay "clean"?
>>>>>>>>
>>>>>>>> Are you saying that this driver would become "dirty" with such a patch?
>>>>>>>
>>>>>>> Yes. Others have said the same and even provided alternative solutions
>>>>>>> on how to solve what's seemingly a platform-specific problem in a
>>>>>>> platform-specific way.
>>>>>>
>>>>>> This is not platform specific, any platform with a complete clock driver
>>>>>> will suffer from the same problem (the clock driver disabling unclaimed
>>>>>> ahb gates, and thus killing the video output) if it wants to use simplefb
>>>>>> for early console support.
>>>>>
>>>>> It is platform specific in that your platform may require certain clocks
>>>>> to remain on. The next platform may require power domains to remain on
>>>>> during boot and yet another one may rely on regulators to stay on during
>>>>> boot. By your argument simplefb will need to be taught to handle pretty
>>>>> much every type of resource that the kernel has.
>>>>
>>>> Why can't simplefb be a driver library that is called from a device
>>>> specific device driver that only claims the clocks (or regulators)?
>>>> Then build all of these device specific drivers into the generic ARM
>>>> kernel. They will be quite small since all they do is claim the clocks
>>>> (or regulator). Maybe we can even figure out some protocol for
>>>> removing the unused ones from memory later.
>>>>
>>>> Later during the boot process the device specific driver can load its
>>>> KMS code which has also been implemented as a driver library. Maybe
>>>> use E_PROBE_DEFER to do this. Match on the device ID, claim the
>>>> clocks, defer until the full KMS library can be loaded.
>>>
>>> There is no need for all this complexity, all that is needed is for the
>>> simplefb driver to be thought to claim + enable any clocks listed in
>>> its dt node.
>>
>> 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?
>
> That's why this is such a problem. There is a general rule in Linux -
> one device, one driver.
A general rule with exceptions such as, drumroll ... , video devices,
e.g. efifb vs kms drivers are the exact same thing, or good old
vgacon vs kms drivers.
Or any other special firmware interfaces for early output (be it serial
or video) vs a later loaded / initialized more complete hardware specific
driver.
Also in the usb world there are quite a few examples where your once
device one driver rule does not hold either, but that is not really
relevant.
Regards,
Hans
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-26 9:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826090035.GB17528@sirena.org.uk>
[-- Attachment #1: Type: text/plain, Size: 3035 bytes --]
On Tue, Aug 26, 2014 at 10:00:35AM +0100, Mark Brown wrote:
> On Tue, Aug 26, 2014 at 10:26:27AM +0200, Thierry Reding wrote:
> > On Mon, Aug 25, 2014 at 05:07:05PM +0200, Maxime Ripard wrote:
>
> > > Regulators with regulator-boot-on will still be disabled if there's no
> > > one to claim it. Just like what happens currently for the clocks.
>
> > I was somewhat surprised by this, but it can indeed easily be verified.
> > It seems to me somewhat at odds with the definition of such a property.
>
> That depends what you think it should do - it's there for handover from
> the bootloader in cases where we can't read the state.
True. The description leaves a lot of room for interpretation, though.
- regulator-boot-on: bootloader/firmware enabled regulator
> > that. However the implementation will automatically disable a regulator
> > marked boot-on if it hasn't been claimed by any driver after the
> > initcall stage completes.
>
> > I find that rather odd since I always assumed that a regulator marked
> > boot-on would not be touched by the core at all, assuming that firmware
> > set it up properly and that it would be required (even if not explicitly
> > claimed).
>
> No, there's a separate always-on property if we don't want to disable.
But always-on means that it can't ever be disabled. In this case what
we'd need is a "don't disable automatically because it's needed, but we
may want to switch it off at a later point to save power."
> 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.
> > Two categories of drivers have this issue: drivers built as modules (or
> > that defer probing) and therefore won't be probed until after initcalls
>
> We really need a later initcall to manage handover to userspace
> (probably triggered by a combination of userspace saying it's done doing
> initial module enumeration and the deferred probe grinding to a halt).
Yes, perhaps that would be another option.
> > have run and generic low-level drivers that take over firmware devices
> > (simplefb in this case) that don't know anything about the resource that
> > the devices need.
>
> I don't understand this use case, sorry - it sounds like a buggy system
> design and/or integration. If the firmware is managing the resource
> then Linux really shouldn't be talking to it at all without coordinating
> with firmware. How can such a system work otherwise?
The firmware isn't really managing resources. It's setting up state that
the kernel shouldn't be modifying. Currently the kernel assumes that no
firmware exists and just disables everything that's not being used. That
is a reasonable default, but it also limits what we can do. I think if
we provided a good interface to communicate state between firmware and
kernel then we could easily do this kind of hand-off.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Questions about cea_modes database in modedb.c
From: 佐々木大輔 @ 2014-08-26 9:38 UTC (permalink / raw)
To: linux-fbdev
Dear experts,
This is a first time for me to write a email to this mailing list.
I am trying to output the video signal from the LCD controller of the
embedded system. I want to output the signal whose timings are defined in the
"CEA Standard ".
I am looking at the documentation for the CEA-816-D.
I understand that the signal timing of CEA Standard is defined as
"drivers/fbdev/core/modedb.c".
I have some questions on cea_modes database in modedb.c.
Q.1) In interlaced mode, for the values of the upper_margin, lower_margin
and vsync_len, do I need to set the doubled number of the specs
of the CEA Standard?
Q.2) When the two pixel clock frequencies are defined in one format, how
would you choose the value of pixclock?
For example, Format 5 can be selected 74.025MHz and 74.176MHz.
The modedb.c file, how do I manage about this format?
(Can I use in common to this file? Or, rewritten for each system?)
Q.3) Format 5: is the value of pixclock correct?
In CEA Standard, pixclock of this format is 74.176 or 74.250MHz.
Q.4) Format 7 and 9: is the value of pixclock correct?
In CEA Standard, pixclock of this format is 27.000 or 27.027MHz.
Q.5) Format 20: The value of hsync_len is wrong?
I think, hsync_len should be 44.
Any feedback is really appreciated.
diff --git drivers/video/fbdev/core/modedb.c drivers/video/fbdev/core/modedb.c
index a9a907c..ed6b883 100644
--- drivers/video/fbdev/core/modedb.c
+++ drivers/video/fbdev/core/modedb.c
@@ -305,18 +305,18 @@ const struct fb_videomode cea_modes[64] = {
},
/* #5: 1920x1080i@59.94/60Hz */
[5] = {
- NULL, 60, 1920, 1080, 13763, 148, 88, 15, 2, 44, 5,
+ NULL, 60, 1920, 1080, 13468, 148, 88, 30, 4, 44, 10,
FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
FB_VMODE_INTERLACED, 0,
},
/* #7: 720(1440)x480iH@59.94/60Hz */
[7] = {
- NULL, 60, 1440, 480, 18554/*37108*/, 114, 38, 15, 4, 124, 3, 0,
+ NULL, 60, 1440, 480, 37037, 114, 38, 30, 8, 124, 6, 0,
FB_VMODE_INTERLACED, 0,
},
/* #9: 720(1440)x240pH@59.94/60Hz */
[9] = {
- NULL, 60, 1440, 240, 18554, 114, 38, 16, 4, 124, 3, 0,
+ NULL, 60, 1440, 240, 37037, 114, 38, 16, 4, 124, 3, 0,
FB_VMODE_NONINTERLACED, 0,
},
/* #18: 720x576pH@50Hz */
@@ -332,7 +332,7 @@ const struct fb_videomode cea_modes[64] = {
},
/* #20: 1920x1080i@50Hz */
[20] = {
- NULL, 50, 1920, 1080, 13480, 148, 528, 15, 5, 528, 5,
+ NULL, 50, 1920, 1080, 13468, 148, 528, 30, 4, 44, 10,
FB_SYNC_HOR_HIGH_ACT | FB_SYNC_VERT_HIGH_ACT,
FB_VMODE_INTERLACED, 0,
},
--
Daisuke Sasaki
Atmark Techno, Inc.
^ permalink raw reply related
* Re: [PATCH] fbdev: Remove __init from chips_hw_init() to fix build failure
From: Tomi Valkeinen @ 2014-08-26 9:50 UTC (permalink / raw)
To: Pranith Kumar
Cc: Jean-Christophe Plagniol-Villard, Rob Clark, Jingoo Han,
Laurent Pinchart, Daniel Vetter, FRAMEBUFFER LAYER, open list
In-Reply-To: <1408623838-8275-1-git-send-email-bobby.prani@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]
On 21/08/14 15:23, Pranith Kumar wrote:
> Fix build failure caused as follows:
>
> The function chipsfb_pci_init() references
> the function __init chips_hw_init().
> This is often because chipsfb_pci_init lacks a __init
> annotation or the annotation of chips_hw_init is wrong.
>
> make: *** [drivers] Error 2
>
> by removing the __init annotation from chips_hw_init(). The other thing that
> could have been done was annotating chipsfb_pci_init(). But that cannot be done
> since chipsfb_pci_init() is called from non __init functions.
>
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
> drivers/video/fbdev/chipsfb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/chipsfb.c b/drivers/video/fbdev/chipsfb.c
> index 206a66b..59abdc6 100644
> --- a/drivers/video/fbdev/chipsfb.c
> +++ b/drivers/video/fbdev/chipsfb.c
> @@ -273,7 +273,7 @@ static struct chips_init_reg chips_init_xr[] = {
> { 0xa8, 0x00 }
> };
>
> -static void __init chips_hw_init(void)
> +static void chips_hw_init(void)
> {
> int i;
>
>
Thanks, queued for 3.17 fbdev fixes.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v2] video: ARM CLCD: Fix calculation of bits-per-pixel
From: Tomi Valkeinen @ 2014-08-26 9:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1408538464.3858.43.camel@linaro1.home>
[-- Attachment #1: Type: text/plain, Size: 707 bytes --]
On 20/08/14 15:41, Jon Medhurst (Tixy) wrote:
> If the device-tree specifies a max-memory-bandwidth property then the
> CLCD driver uses that to calculate the bits-per-pixel supported,
> however, this calculation is faulty for two reasons.
>
> 1. It doesn't ensure that the result is a sane value, i.e. a power of 2
> and <= 32 as the rest of the code assumes.
>
> 2. It uses the displayed resolution and calculates the average bandwidth
> across the whole frame. It should instead calculate the peak
> bandwidth based on the pixel clock.
>
> This patch fixes both the above.
>
> Signed-off-by: Jon Medhurst <tixy@linaro.org>
Thanks, queued for 3.17 fbdev fixes.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc: Fix build error for vga16fb
From: Tomi Valkeinen @ 2014-08-26 10:02 UTC (permalink / raw)
To: Geert Uytterhoeven, Pranith Kumar, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman
Cc: Jean-Christophe Plagniol-Villard, Arnd Bergmann, Peter Griffin,
Alexander Shiyan, Jean Delvare, Kukjin Kim, Mark Brown,
FRAMEBUFFER LAYER, open list, Andrew Morton
In-Reply-To: <CAMuHMdV_vAPCPh1JdCxtf0iz6haHULUV8t0oYt0jrEAky2WSLw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1801 bytes --]
On 19/08/14 00:28, Geert Uytterhoeven wrote:
> Hi Pranith,
>
> On Mon, Aug 18, 2014 at 11:13 PM, Pranith Kumar <bobby.prani@gmail.com> wrote:
>> Fix a build error caused by
>>
>> drivers/built-in.o: In function `vga16fb_probe':
>> drivers/video/fbdev/vga16fb.c:1326: undefined reference to `vgacon_remap_base'
>> drivers/video/fbdev/vga16fb.c:1326: undefined reference to `vgacon_remap_base'
>> make: *** [vmlinux] Error 1
>>
>> This is occuring as vgacon_remap_base is exported only when
>> CONFIG_VGA_CONSOLE=y. Fix the error by making FB_VBA16 depend on
>> CONFIG_VGA_CONSOLE.
>>
>> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
>> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> CC: Andrew Morton <akpm@linux-foundation.org>
>> ---
>> drivers/video/fbdev/Kconfig | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
>> index e911b9c..09df564 100644
>> --- a/drivers/video/fbdev/Kconfig
>> +++ b/drivers/video/fbdev/Kconfig
>> @@ -547,7 +547,7 @@ config FB_IMSTT
>>
>> config FB_VGA16
>> tristate "VGA 16-color graphics support"
>> - depends on FB && (X86 || PPC)
>> + depends on FB && (X86 || PPC) && CONFIG_VGA_CONSOLE
>
> The dependency on CONFIG_VGA_CONSOLE only exists on PPC, not on x86.
> So this is not correct.
>
> Digging a bit deeper: There's no code left in arch/powerpc that actually
> sets vgacon_remap_base, so it's not gonna work anyway.
> It seems vga16 support on PPC died with the removal of arch/ppc/.
Hmm, so I wonder if vga16fb is supposed to work on ppc or not. Pranith's
follow-up patch disables vga16fb for PPC, which obviously fixes the
compilation problem, but is that really a correct fix?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 4/4] drivers: video: fbdev: atmel_lcdfb.c: fix error return code
From: Tomi Valkeinen @ 2014-08-26 10:05 UTC (permalink / raw)
To: Julia Lawall, Nicolas Ferre
Cc: kernel-janitors, Jean-Christophe Plagniol-Villard, linux-fbdev,
linux-kernel
In-Reply-To: <1407355938-10932-4-git-send-email-Julia.Lawall@lip6.fr>
[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]
On 06/08/14 23:12, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> Convert a zero return value on error to a negative one, as returned
> elsewhere in the function.
>
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> (
> if@p1 (\(ret < 0\|ret != 0\))
> { ... return ret; }
> |
> ret@p1 = 0
> )
> ... when != ret = e1
> when != &ret
> *if(...)
> {
> ... when != ret = e2
> when forall
> return ret;
> }
>
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---
> The error codes chosen are the ones that are commonly used elsewhere or
> these functions. Perhaps something else is wanted.
>
> drivers/video/fbdev/atmel_lcdfb.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c
> index 92640d4..1d8bdb9 100644
> --- a/drivers/video/fbdev/atmel_lcdfb.c
> +++ b/drivers/video/fbdev/atmel_lcdfb.c
> @@ -1102,12 +1102,14 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo)
> timings = of_get_display_timings(display_np);
> if (!timings) {
> dev_err(dev, "failed to get display timings\n");
> + ret = -EINVAL;
> goto put_display_node;
> }
>
> timings_np = of_find_node_by_name(display_np, "display-timings");
> if (!timings_np) {
> dev_err(dev, "failed to find display-timings node\n");
> + ret = -ENODEV;
> goto put_display_node;
> }
>
>
Thanks, queued for 3.17 fbdev 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: Mark Brown @ 2014-08-26 10:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826091853.GI17263@ulmo>
[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]
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:
> > > I find that rather odd since I always assumed that a regulator marked
> > > boot-on would not be touched by the core at all, assuming that firmware
> > > set it up properly and that it would be required (even if not explicitly
> > > claimed).
> > No, there's a separate always-on property if we don't want to disable.
> But always-on means that it can't ever be disabled. In this case what
> we'd need is a "don't disable automatically because it's needed, but we
> may want to switch it off at a later point to save power."
Such a property wouldn't make much sense, it should also apply to the
driver at which point it's just the same as always-on.
> > 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.
> > > have run and generic low-level drivers that take over firmware devices
> > > (simplefb in this case) that don't know anything about the resource that
> > > the devices need.
> > I don't understand this use case, sorry - it sounds like a buggy system
> > design and/or integration. If the firmware is managing the resource
> > then Linux really shouldn't be talking to it at all without coordinating
> > with firmware. How can such a system work otherwise?
> The firmware isn't really managing resources. It's setting up state that
> the kernel shouldn't be modifying. Currently the kernel assumes that no
> firmware exists and just disables everything that's not being used. That
> is a reasonable default, but it also limits what we can do. I think if
> we provided a good interface to communicate state between firmware and
> kernel then we could easily do this kind of hand-off.
I'm afraid that's confusing me even further. If the "firmware devices"
don't know anything about the resources as you say then presumably they
need to be always on since they obviously won't be able to control them
and there is going to be no handoff?
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH V2] video : remove redundant error check
From: Tomi Valkeinen @ 2014-08-26 10:34 UTC (permalink / raw)
To: Daeseok Youn, plagnioj, Antonino Daplas
Cc: jg1.han, laurent.pinchart, robdclark, daniel.vetter, Julia.Lawall,
linux-fbdev, linux-kernel
In-Reply-To: <20140516093123.GA17245@devel.8.8.4.4>
[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]
On 16/05/14 12:31, Daeseok Youn wrote:
> It doesn't need to check "err" for printing info.
> And also use pr_info instead of printk.
>
> Signed-off-by: Daeseok Youn <daeseok.youn@gmail.com>
> ---
> V2: removes unneeded lines for sending a patch
>
> drivers/video/fbdev/i810/i810_main.c | 7 +++----
> 1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/fbdev/i810/i810_main.c b/drivers/video/fbdev/i810/i810_main.c
> index bb674e4..15cb397 100644
> --- a/drivers/video/fbdev/i810/i810_main.c
> +++ b/drivers/video/fbdev/i810/i810_main.c
> @@ -1910,13 +1910,12 @@ static void i810fb_find_init_mode(struct fb_info *info)
>
> for (i = 0; i < par->ddc_num + 1; i++) {
> err = i810_probe_i2c_connector(info, &par->edid, i);
> - if (!err)
> + if (!err) {
> + pr_info("i810fb_init_pci: DDC probe successful\n");
> break;
> + }
> }
>
> - if (!err)
> - printk("i810fb_init_pci: DDC probe successful\n");
> -
> fb_edid_to_monspecs(par->edid, specs);
>
> if (specs->modedb == NULL)
>
I don't know... I think I personally like more the original version. In
fact, the whole print looks quite useless to me, or at least it should
be a debug print.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 14/14] video: of: display_timing: delete unneeded test before of_node_put
From: Tomi Valkeinen @ 2014-08-26 10:36 UTC (permalink / raw)
To: Julia Lawall, Jean-Christophe Plagniol-Villard
Cc: kernel-janitors, linux-fbdev, linux-kernel, Uwe Kleine-König
In-Reply-To: <1407492475-26283-14-git-send-email-Julia.Lawall@lip6.fr>
[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]
On 08/08/14 13:07, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> Of_node_put supports NULL as its argument, so the initial test is not
> necessary. Furthermore, native_mode cannot be NULL at this point.
>
> Suggested by Uwe Kleine-König.
>
> The semantic patch that fixes this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression e;
> @@
>
> -if (e)
> of_node_put(e);
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---
> drivers/video/of_display_timing.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
> index 987edf1..339f831 100644
> --- a/drivers/video/of_display_timing.c
> +++ b/drivers/video/of_display_timing.c
> @@ -233,8 +233,7 @@ struct display_timings *of_get_display_timings(struct device_node *np)
> return disp;
>
> timingfail:
> - if (native_mode)
> - of_node_put(native_mode);
> + of_node_put(native_mode);
> display_timings_release(disp);
> entryfail:
> kfree(disp);
>
Thanks, queued for 3.18.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 8/9 v2] video: fbdev: aty: use c99 initializers in structures
From: Tomi Valkeinen @ 2014-08-26 10:43 UTC (permalink / raw)
To: Julia Lawall, Josh Triplett
Cc: Paul Mackerras, kernel-janitors, Jean-Christophe Plagniol-Villard,
linux-fbdev, linux-kernel
In-Reply-To: <alpine.DEB.2.02.1408231749030.2068@localhost6.localdomain6>
[-- Attachment #1: Type: text/plain, Size: 830 bytes --]
Hi,
On 23/08/14 18:50, Julia Lawall wrote:
> From nobody Sat Aug 23 12:48:19 CEST 2014
> From: Julia Lawall <Julia.Lawall@lip6.fr>
> To: Paul Mackerras <paulus@samba.org>
> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>,Tomi Valkeinen <tomi.valkeinen@ti.com>,linux-fbdev@vger.kernel.org,linux-kernel@vger.kernel.org
> Subject: [PATCH 8/9] video: fbdev: aty: use c99 initializers in structures
>
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> Use c99 initializers for structures.
>
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
This mail had some extra stuff above (From nobody... etc). I cleaned it
up, hopefully correctly.
I've applied the fbdev patches from this series, 2, 7 and 8, for v3.18.
Thanks!
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 6/7] video: fbdev: sis: delete double assignment
From: Tomi Valkeinen @ 2014-08-26 10:51 UTC (permalink / raw)
To: Geert Uytterhoeven, Julia Lawall
Cc: Thomas Winischhofer, Joe Perches, kernel-janitors@vger.kernel.org,
Jean-Christophe Plagniol-Villard, Linux Fbdev development list,
linux-kernel@vger.kernel.org
In-Reply-To: <CAMuHMdVERt9XBN5OW8QqT_PvQVHd6xgnCdjRpiSNp7ZSEgXWSg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 665 bytes --]
On 24/08/14 21:21, Geert Uytterhoeven wrote:
> On Sat, Aug 23, 2014 at 8:33 PM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
>> ---
>> The patches in this series do not depend on each other.
>>
>> This changes the semantics of the code and is not tested.
>
> Hence I think you should change the subject of the patch, so it's obvious
> some bug is fixed, e.g. "video: fbdev: sis: Fix double assignment".
Well, to me "fix double assignment" sounds the same as "delete double
assignment".
Looking at the code, I think what's done here is "fix 1280x768 panel
timings".
I can apply this and do the change to subject, if nobody complains.
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: Thierry Reding @ 2014-08-26 10:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140826100612.GH17528@sirena.org.uk>
[-- Attachment #1: Type: text/plain, Size: 4689 bytes --]
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:
>
> > > > I find that rather odd since I always assumed that a regulator marked
> > > > boot-on would not be touched by the core at all, assuming that firmware
> > > > set it up properly and that it would be required (even if not explicitly
> > > > claimed).
>
> > > No, there's a separate always-on property if we don't want to disable.
>
> > But always-on means that it can't ever be disabled. In this case what
> > we'd need is a "don't disable automatically because it's needed, but we
> > may want to switch it off at a later point to save power."
>
> Such a property wouldn't make much sense, it should also apply to the
> driver at which point it's just the same as always-on.
>
> > > 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.
> > > > have run and generic low-level drivers that take over firmware devices
> > > > (simplefb in this case) that don't know anything about the resource that
> > > > the devices need.
>
> > > I don't understand this use case, sorry - it sounds like a buggy system
> > > design and/or integration. If the firmware is managing the resource
> > > then Linux really shouldn't be talking to it at all without coordinating
> > > with firmware. How can such a system work otherwise?
>
> > The firmware isn't really managing resources. It's setting up state that
> > the kernel shouldn't be modifying. Currently the kernel assumes that no
> > firmware exists and just disables everything that's not being used. That
> > is a reasonable default, but it also limits what we can do. I think if
> > we provided a good interface to communicate state between firmware and
> > kernel then we could easily do this kind of hand-off.
>
> I'm afraid that's confusing me even further. If the "firmware devices"
> don't know anything about the resources as you say then presumably they
> need to be always on since they obviously won't be able to control them
> and there is going to be no handoff?
How this works with simplefb is that the firmware (U-Boot for example)
sets up everything needed to scan out a framebuffer. The firmware will
typically use this for a boot splash or a console. It's akin to the
standard VGA console on x86.
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.
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.
The resources don't need to be always on, only as long as there isn't a
proper driver to manage them. Once that driver has requested the
resources it will likely want to control them to save power. Therefore I
think the most straightforward way would be for the kernel not to touch
any of these resources until they've been explicitly requested. Even if
we don't want to rely on firmware doing the right thing by default, I
think it's still valuable to take hints from firmware when it provides
them so rather than defaulting to not touching any resources what I
proposed is to mark resources that shouldn't be touched.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 2/7] video: fbdev: intelfb: delete double assignment
From: Tomi Valkeinen @ 2014-08-26 10:56 UTC (permalink / raw)
To: Julia Lawall, Maik Broemme
Cc: joe, kernel-janitors, Jean-Christophe Plagniol-Villard,
linux-fbdev, linux-kernel
In-Reply-To: <1408818808-18850-3-git-send-email-Julia.Lawall@lip6.fr>
[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]
On 23/08/14 21:33, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> Delete successive assignments to the same location.
>
> In the second case, = is converted to |=, which looks appropriate based on
> the values involved.
>
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression i;
> @@
>
> *i = ...;
> i = ...;
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> ---
> The patches in this series do not depend on each other.
>
> This changes the behavior of the code in the second case and is not tested.
In this patch, as in the sis one, the behavior is changed. The subject
might perhaps be "fix cursor initialization"?
The patch does look like it could cause a regression, though. Anyone
able to test this?
Tomi
>
> drivers/video/fbdev/intelfb/intelfbhw.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/intelfb/intelfbhw.c b/drivers/video/fbdev/intelfb/intelfbhw.c
> index fbad61d..d31ed4e 100644
> --- a/drivers/video/fbdev/intelfb/intelfbhw.c
> +++ b/drivers/video/fbdev/intelfb/intelfbhw.c
> @@ -1191,7 +1191,6 @@ int intelfbhw_mode_to_hw(struct intelfb_info *dinfo,
> vsync_end = vsync_start + var->vsync_len;
> vtotal = vsync_end + var->upper_margin;
> vblank_start = vactive;
> - vblank_end = vtotal;
> vblank_end = vsync_end + 1;
>
> DBG_MSG("V: act %d, ss %d, se %d, tot %d bs %d, be %d\n",
> @@ -1859,7 +1858,7 @@ void intelfbhw_cursor_init(struct intelfb_info *dinfo)
> tmp = INREG(CURSOR_CONTROL);
> tmp &= ~(CURSOR_FORMAT_MASK | CURSOR_GAMMA_ENABLE |
> CURSOR_ENABLE | CURSOR_STRIDE_MASK);
> - tmp = CURSOR_FORMAT_3C;
> + tmp |= CURSOR_FORMAT_3C;
> OUTREG(CURSOR_CONTROL, tmp);
> OUTREG(CURSOR_A_BASEADDR, dinfo->cursor.offset << 12);
> tmp = (64 << CURSOR_SIZE_H_SHIFT) |
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] msm: msm_fb: Add remove function for platform driver for clean unloading
From: Tomi Valkeinen @ 2014-08-26 11:19 UTC (permalink / raw)
To: Pramod Gurav, linux-fbdev, linux-kernel
Cc: Jean-Christophe Plagniol-Villard, Stephen Boyd, Jingoo Han,
Rob Clark
In-Reply-To: <1407163433-13528-1-git-send-email-pramod.gurav@smartplayin.com>
[-- Attachment #1: Type: text/plain, Size: 2624 bytes --]
Hi,
On 04/08/14 17:43, Pramod Gurav wrote:
> This adds a remove function to platform driver structure so that
> resources are released when driver is unloaded.
>
> Moved kzalloc to use managed resource and adds a return check if failed.
Please split this patch into two. You change two unrelated things here,
the unloading and the kzalloc part.
> 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>
>
> Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com>
> ---
> drivers/video/fbdev/msm/msm_fb.c | 28 +++++++++++++++++++++++++---
> 1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/msm/msm_fb.c b/drivers/video/fbdev/msm/msm_fb.c
> index 1374803..ac277e2 100644
> --- a/drivers/video/fbdev/msm/msm_fb.c
> +++ b/drivers/video/fbdev/msm/msm_fb.c
> @@ -553,6 +553,7 @@ static int msmfb_probe(struct platform_device *pdev)
> fb = framebuffer_alloc(sizeof(struct msmfb_info), &pdev->dev);
> if (!fb)
> return -ENOMEM;
> +
> msmfb = fb->par;
> msmfb->fb = fb;
> msmfb->panel = panel;
> @@ -569,8 +570,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);
> @@ -589,6 +595,8 @@ static int msmfb_probe(struct platform_device *pdev)
>
> msmfb->sleeping = WAKING;
>
> + platform_set_drvdata(pdev, msmfb);
> +
> return 0;
>
> error_register_framebuffer:
> @@ -598,13 +606,27 @@ error_setup_fbmem:
> return ret;
> }
>
> +static int msmfb_remove(struct platform_device *pdev)
> +{
> + struct msmfb_info *msmfb = NULL;
No need to initialize to NULL, msmfb is initialized always below.
> +
> + msmfb = platform_get_drvdata(pdev);
> + if (msmfb) {
Can msmfb ever be NULL here? If I'm not mistaken, remove is only called
if the probe had succeeded. And if probe had succeeded, the drvdata is
always set to a proper pointer to msmfb_info.
Tomi
[-- Attachment #2: OpenPGP 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