* Re: [PATCH 2/4] simplefb: add goto error path to probe
From: David Herrmann @ 2014-08-14 10:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140814102940.GA1000@skynet.be>
Hi
On Thu, Aug 14, 2014 at 12:29 PM, Luc Verhaegen <libv@skynet.be> wrote:
> On Wed, Aug 13, 2014 at 09:27:46AM +0200, David Herrmann wrote:
>> Hi
>>
>> On Wed, Aug 13, 2014 at 9:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>> > Signed-off-by: Luc Verhaegen <libv@skynet.be>
>> > ---
>> > drivers/video/fbdev/simplefb.c | 20 +++++++++++++-------
>> > 1 files changed, 13 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>> > index 32be590..72a4f20 100644
>> > --- a/drivers/video/fbdev/simplefb.c
>> > +++ b/drivers/video/fbdev/simplefb.c
>> > @@ -220,8 +220,8 @@ static int simplefb_probe(struct platform_device *pdev)
>> >
>> > info->apertures = alloc_apertures(1);
>> > if (!info->apertures) {
>> > - framebuffer_release(info);
>> > - return -ENOMEM;
>> > + ret = -ENOMEM;
>> > + goto error_fb_release;
>> > }
>> > info->apertures->ranges[0].base = info->fix.smem_start;
>> > info->apertures->ranges[0].size = info->fix.smem_len;
>> > @@ -231,8 +231,8 @@ static int simplefb_probe(struct platform_device *pdev)
>> > info->screen_base = ioremap_wc(info->fix.smem_start,
>> > info->fix.smem_len);
>> > if (!info->screen_base) {
>> > - framebuffer_release(info);
>> > - return -ENODEV;
>> > + ret = -ENODEV;
>> > + goto error_fb_release;
>> > }
>> > info->pseudo_palette = (void *) par->palette;
>> >
>> > @@ -247,14 +247,20 @@ static int simplefb_probe(struct platform_device *pdev)
>> > ret = register_framebuffer(info);
>> > if (ret < 0) {
>> > dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
>> > - iounmap(info->screen_base);
>> > - framebuffer_release(info);
>> > - return ret;
>> > + goto error_unmap;
>> > }
>> >
>> > dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
>> >
>> > return 0;
>> > +
>> > + error_unmap:
>> > + iounmap(info->screen_base);
>> > +
>> > + error_fb_release:
>> > + framebuffer_release(info);
>> > +
>> > + return ret;
>>
>> Again, I'd use different coding-style, but I will leave that to
>> Stephen and Tomi:
>>
>> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> While the discussion about the last two patches rages on, can you state
> what coding style changes you would like to see here, as i am not clear
> as to what exactly is off with the above code.
I'd skip the leading whitespace and the newlines, like this:
+error_unmap:
+ iounmap(info->screen_base);
+error_fb_release:
+ framebuffer_release(info);
+ return ret;
at least that's my conception how we format error paths in drivers/video/.
Thanks
David
^ permalink raw reply
* Re: [PATCH 2/4] simplefb: add goto error path to probe
From: Luc Verhaegen @ 2014-08-14 10:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CANq1E4TAf=Lyuz5psPJE+MWyUO8db8V+-BqXmVyvdLvmY5g=iQ@mail.gmail.com>
On Thu, Aug 14, 2014 at 12:33:55PM +0200, David Herrmann wrote:
> Hi
>
> I'd skip the leading whitespace and the newlines, like this:
>
> +error_unmap:
> + iounmap(info->screen_base);
> +error_fb_release:
> + framebuffer_release(info);
> + return ret;
>
> at least that's my conception how we format error paths in drivers/video/.
Will do. Thanks.
Luc Verhaegen.
^ permalink raw reply
* Re: [linux-sunxi] [PATCH 4/4] simplefb: add clock handling code
From: Luc Verhaegen @ 2014-08-14 10:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <9B9D62CA-8037-4653-BCF0-C79BFFEB4279@dominion.thruhere.net>
On Thu, Aug 14, 2014 at 12:31:50PM +0200, Koen Kooi wrote:
>
> How many DRM drivers are there on ARM and what's the size impact of building them all into the kernel? I know from experience that it's not possible on x86 especially with efifb in the mix, but I wonder what the situation on ARM is. I only have TI, sunxi and exynos boards to test on and building in both TI drm drivers and the exynos one seems to work.
> Note that I'm not talking about the non-DRM abortions that maskerade as graphics drivers for ARM SoCs, only proper DRM ones.
>
> regards,
>
> Koen
A quick check tells me that my current sunxi-3.4 sunxi_drm.ko measures
1.7MB, for currently about 8kloc (the binary seems excessively big
though), which is just display. This still lacks many key features (lcd
gpio, backlight pwm, lvds, ints), so it will grow ~1kloc still.
Also, i constantly am re-loading this (how else does one develop any
sizable amount of code?), so it does that cleanly and correctly, which
seems quite contrary to your experience with ARM drm drivers.
Luc Verhaegen.
^ permalink raw reply
* Re: [linux-sunxi] [PATCH 4/4] simplefb: add clock handling code
From: Hans de Goede @ 2014-08-14 11:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <9B9D62CA-8037-4653-BCF0-C79BFFEB4279@dominion.thruhere.net>
Hi,
On 08/14/2014 12:31 PM, Koen Kooi wrote:
>
> Op 14 aug. 2014, om 11:37 heeft Hans de Goede <hdegoede@redhat.com> het volgende geschreven:
>
>> Hi,
>>
>> On 08/13/2014 07:01 PM, Maxime Ripard wrote:
>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>>>> On 08/13/2014 01:17 AM, Luc Verhaegen wrote:
>>>>> This claims and enables clocks listed in the simple framebuffer dt node.
>>>>> This is needed so that the display engine, in case the required clocks
>>>>> are known by the kernel code and are described in the dt, will remain
>>>>> properly enabled.
>>>>
>>>> I think this make simplefb not simple any more, which rather goes
>>>> against the whole point of it.
>>>>
>>>> I specifically conceived simplefb to know about nothing more than
>>>> the memory address and pixel layout of the memory buffer. I
>>>> certainly don't like the idea of expanding simplefb to anything
>>>> beyond that, and IIRC *not* extending is was a condition agreed when
>>>> it was first merged. If more knowledge than that is required, then
>>>> there needs to be a HW-specific driver to manage any
>>>> clocks/resets/video registers, etc.
>>>
>>> I'm sorry, but how is that not simple? clocks enabling is step 1 in a
>>> driver in order to communicate somehow with the controller. Reset is a
>>> different story, because arguably, if simplefb is there, the
>>> controller is already out of reset.
>>>
>>> And I don't see why video registers are coming into the discussion
>>> here. The code Luc posted doesn't access any register, at all. It just
>>> makes sure the needed controller keep going.
>>>
>>>> The correct way to handle this without a complete DRM/KMS/... driver
>>>> is to avoid the clocks in question being turned off at boot.
>>>
>>> Which is exactly what this code does, using the generic DT bindings to
>>> express dependency for a given clock. How is this wrong?
>>>
>>>> I thought there was a per-clock flag to prevent disabling an unused
>>>> clock?
>>>
>>> No, last time I heard, Mike Turquette was against it.
>>>
>>>> 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.
>>>
>>>> For example, the Tegra clock driver has a clock initialization table
>>>> which IIRC was used for this purpose before we got a DRM/KMS driver.
>>>> That way, all the details are kept inside the kernel code, and don't
>>>> end up influencing the DT representation of simplefb.
>>>
>>> I don't really see how the optional usage of a generic property
>>> influences badly the DT representation of simplefb.
>>
>> +1 to all that Maxime said.
>>
>> Also as can be seen in other discussion on this patch set, simplefb
>> should not be seen as something orthogonal to having a full kms driver.
>>
>> So just write a full kms driver is not the answer IMHO. What we want
>> is for a bootloader setup console to be available through simplefb
>> bindings so that the kernel can show output without depending on
>> module loading, and thus can show errors if things go bad before
>> a kms driver gets loaded.
>>
>> And no build kms into the kernel is not the answer. We've all been
>> working hard to be able to build more generic kernels, so as to get
>> generic distro support. And generic distros will build kms as modules,
>> as there are simply to many different kms drivers to build them all
>> in.
>
> How many DRM drivers are there on ARM and what's the size impact of building them all into the kernel?
I don't know how many we've today, but I do know that we will have
twice as more next year, and 4x as more the year after that, etc.
IOW this does not scale.
Regards,
Hans
^ permalink raw reply
* Re: [linux-sunxi] [PATCH 4/4] simplefb: add clock handling code
From: Hans de Goede @ 2014-08-14 11:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140814105711.GC1000@skynet.be>
Hi,
On 08/14/2014 12:57 PM, Luc Verhaegen wrote:
> On Thu, Aug 14, 2014 at 12:31:50PM +0200, Koen Kooi wrote:
>>
>> How many DRM drivers are there on ARM and what's the size impact of building them all into the kernel? I know from experience that it's not possible on x86 especially with efifb in the mix, but I wonder what the situation on ARM is. I only have TI, sunxi and exynos boards to test on and building in both TI drm drivers and the exynos one seems to work.
>> Note that I'm not talking about the non-DRM abortions that maskerade as graphics drivers for ARM SoCs, only proper DRM ones.
>>
>> regards,
>>
>> Koen
>
> A quick check tells me that my current sunxi-3.4 sunxi_drm.ko measures
> 1.7MB
That likely is with debug-info, try running "strip --strip-debug" on the .ko
file. Note don't use plain "strip" that ruins kernel modules.
Regards,
Hans
p.s.
I'm leaving home (and the internet) for vacation tomorrow. I'll be back
on Mon Aug 25.
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: jonsmirl @ 2014-08-14 12:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACxGe6v1UPHMF5aPpe-boh+RuoPArJmKK_cQWr7aXsRjDUZrfQ@mail.gmail.com>
On Thu, Aug 14, 2014 at 6:15 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Aug 13, 2014 at 9:41 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
>> On Wed, Aug 13, 2014 at 3:14 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> On Wed, Aug 13, 2014 at 1:54 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
>>>> On Wed, Aug 13, 2014 at 6:19 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>>>> On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote:
>>>>>> On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote:
>>>>>> >
>>>>>> > The majority of the DT code is based on the assumption of a static
>>>>>> > tree. Pantelis has been working on being able to modify it at runtime
>>>>>> > with overlays, but he has had to go through a lot of rework because it
>>>>>> > is not a trivial task. When you get into modifying the DT, you need to
>>>>>> > have a lot more understanding of the side effects to changing the
>>>>>> > tree. The DT structure also has a lifecycle that can go beyond the
>>>>>> > current lifecycle of the kernel. The kexec tool will extract the
>>>>>> > current tree from the kernel, make the appropriate modifications, and
>>>>>> > use that to boot the next kernel. Allowing any driver to modify the
>>>>>> > tree has side effects beyond just the current kernel.
>>>>>> >
>>>>>> > In this specific case, it will interact badly with the work Pantelis
>>>>>> > is doing to make platform devices work with overlays. Modifying the
>>>>>> > status property will cause the associated struct device to get removed
>>>>>> > in the middle of probing a driver for that device! That will most
>>>>>> > likely cause an oops.
>>>>>> >
>>>>>> > Besides, Luc straight out *said*: "...even though it has no real value
>>>>>> > today". In what circumstance is that justification for modifying the
>>>>>> > tree?
>>>>>>
>>>>>> With that sentence i meant that given the current state of things, it
>>>>>> has no real value.
>>>>>>
>>>>>> It has no value currently as re-probing simplefb is not going to happen.
>>>>>> But it's not a big leap to turn simplefb into a proper module. Not that
>>>>>> that makes much sense, but that's never stopped anyone.
>>>>>>
>>>>>> To me it seemed simple, dt is what drives simplefb, so dt then also
>>>>>> becomes responsible for making sure that simplefb or another driver does
>>>>>> not attempt to blindly use this info again. The way this is implemented
>>>>>> i do not care for in any way, i just knew that i could not do nothing
>>>>>> here, given the catastrophic effect disabling the clocks has on simplefb
>>>>>> on sunxi. Given the discussion that errupted here, i'd say that this
>>>>>> does need some resolution, and altering the dt is going to have to be
>>>>>> part of the solution.
>>>>>>
>>>>>> In any case, i will gladly drop this patch, as it is not absolutely
>>>>>> necessary. But it should be very clear that there is no going back on
>>>>>> this dt node after the clocks were released once.
>>>>>>
>>>>>> Luc Verhaegen.
>>>>>
>>>>> What about approaching this from the other end? U-Boot could add a
>>>>> property named "once-only" or so.
>>>>
>>>> Device tree is supposed to be a static description of the hardware
>>>> usable on all operating systems. It is the wrong mechanism for
>>>> communicating between uboot and the kernel. Use something like atags
>>>> or the kernel command line to tell the kernel that the console has
>>>> already been set up.
>>>
>>> Not accurate. While it is primarily hardware description, it is also
>>> used for firmware communication. There is loads of precedence for
>>> this. The /chosen node is the most significant example, but there are
>>> other places where the tree is used to provide state. For example, the
>>> current-speed property on UART nodes.
>>
>> I do seem to recall you telling me a long time ago that those chosen
>> nodes were a mistake (or maybe it was Matt Sealey). I'm pretty wary of
>> opening to door to device trees carrying a bunch of state. Five years
>> from now the DT is going to look like a Christmas tree.
>
> Wasn't me. Carrying state in the DT provided by firmware is perfectly
> reasonable in my opinion.
So what are the rules going to be? Once the OS is up and starts
changing things the state in the DT and the OS are going to diverge.
How does this get handled for a kernel driver that can load/unload?
Should the kernel remove those state nodes as soon as it alters the
state?
>
> g.
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [PATCH v3 00/14] Rework OMAP4+ HDMI audio support
From: Mark Brown @ 2014-08-14 19:10 UTC (permalink / raw)
To: Jyri Sarha
Cc: alsa-devel, linux-fbdev, linux-omap, peter.ujfalusi,
liam.r.girdwood, tomi.valkeinen, detheridge
In-Reply-To: <53EC8127.7090405@ti.com>
[-- Attachment #1: Type: text/plain, Size: 672 bytes --]
On Thu, Aug 14, 2014 at 12:28:07PM +0300, Jyri Sarha wrote:
> After discussing with Peter and Tomi I decided to change the approach a bit.
> There should not be any new device registered, just ASoC components that are
> implemented under sound/soc as a library rather than a device driver.
> The library would export a register and unregister functions to be called
> from video driver directory. The functions will then register the necessary
> ASoC components under the video device.
> If there are no objections I'll go ahead implementing this approach, first
> for OMAP HDMI audio, and later for SiI9022 driver we are cooking.
That also sounds perfectly sensible.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Henrik Nordström @ 2014-08-15 0:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKON4OypgToTSXxjShCt6uTGZBU69LpJxRwVoz=PnotpL=zm4w@mail.gmail.com>
tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl@gmail.com:
> So what are the rules going to be? Once the OS is up and starts
> changing things the state in the DT and the OS are going to diverge.
> How does this get handled for a kernel driver that can load/unload?
> Should the kernel remove those state nodes as soon as it alters the
> state?
hardware that is no longer there should also not be represented/active
in DT.
A simple fb that have been torn down is no more, no different from never
having been there. It can be argued that it never was in the first place
(i.e. that it is not actually hardware) but that it another can of worms
and both have their benefits and drawbacks. One thing is certain
however, as far as simplefb is concerned it is hardware, not really any
different from a persistent framebuffer with hardwired settings in
hardware.
Regarding kexec, it's the responsibility of the kernel doing kexec to
make sure DT and hardware matches for the next started kernel. With
hardware that can be reconfigured it is not safe to assume that the DT
can be passed as-is from before the kernel reconfigured hardware. If the
currently running kernel wants to set up a framebuffer for simplefb use
by the next kernel than it's free to do so, but in either case it needs
to provide the right information to next kernel.
If it was given a simplefb node, but then killed the simplefb then no
simplefb node should be provided to the next kernel. If it was not given
a simplefb node, but have configured the hardware suitable for simplefb
node then it may provide a simplefb node.
A UART that have changed rate no longer have that rate.
Same applies to numerous other items as well. If a kernel remaps IRQs,
bus addresses, or whatever in hardware aspects where such changes is
possible then DT needs to be updated as well. A bit less common than a
simplefb being destroyed by driver reconfiguring the framebuffer, but if
you look at it from a little distance then the problem is exactly the
same. Hardware is not 100% static, and therefore hardware description
also can not be 100% static. And hardware is getting more and more
dynamically reconfigurable.
A DT that contains false information is generally worse than no DT / not
having that information at all.
Regards
Henrik
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: jonsmirl @ 2014-08-15 1:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1408063512.890.22.camel@localhost>
On Thu, Aug 14, 2014 at 8:45 PM, Henrik Nordström
<henrik@henriknordstrom.net> wrote:
> tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl@gmail.com:
>
>> So what are the rules going to be? Once the OS is up and starts
>> changing things the state in the DT and the OS are going to diverge.
>> How does this get handled for a kernel driver that can load/unload?
>> Should the kernel remove those state nodes as soon as it alters the
>> state?
>
> hardware that is no longer there should also not be represented/active
> in DT.
>
> A simple fb that have been torn down is no more, no different from never
> having been there. It can be argued that it never was in the first place
> (i.e. that it is not actually hardware) but that it another can of worms
> and both have their benefits and drawbacks. One thing is certain
> however, as far as simplefb is concerned it is hardware, not really any
> different from a persistent framebuffer with hardwired settings in
> hardware.
How does the kernel know what clocks to protect? A list in the
simplefb node? This list of clocks is a reason for adding simplefb to
the compatible list for the real hardware. That way it won't be
duplicated.
The issues with parameter conflicts between simplefb and the real
hardware can be dealt with. If the real hardware wants to add the
simplefb compatible field it will need to get its parameters sorted
out so that they don't conflict. Clock syntax is standardized in the
DTS so it shouldn't be a problem.
You can also argue that simplefb should never occur in a DTS file. It
is something that uboot will add. And then the kernel will remove when
simplefb loads KMS.
That logic should apply to all of these dynamic fields. You don't want
stale state data in the DT.
>
> Regarding kexec, it's the responsibility of the kernel doing kexec to
> make sure DT and hardware matches for the next started kernel. With
> hardware that can be reconfigured it is not safe to assume that the DT
> can be passed as-is from before the kernel reconfigured hardware. If the
> currently running kernel wants to set up a framebuffer for simplefb use
> by the next kernel than it's free to do so, but in either case it needs
> to provide the right information to next kernel.
>
> If it was given a simplefb node, but then killed the simplefb then no
> simplefb node should be provided to the next kernel. If it was not given
> a simplefb node, but have configured the hardware suitable for simplefb
> node then it may provide a simplefb node.
>
> A UART that have changed rate no longer have that rate.
>
> Same applies to numerous other items as well. If a kernel remaps IRQs,
> bus addresses, or whatever in hardware aspects where such changes is
> possible then DT needs to be updated as well. A bit less common than a
> simplefb being destroyed by driver reconfiguring the framebuffer, but if
> you look at it from a little distance then the problem is exactly the
> same. Hardware is not 100% static, and therefore hardware description
> also can not be 100% static. And hardware is getting more and more
> dynamically reconfigurable.
>
> A DT that contains false information is generally worse than no DT / not
> having that information at all.
>
> Regards
> Henrik
>
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Maxime Ripard @ 2014-08-15 6:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKON4Oyp0qCucAhTNeXu7LELv_MRXPkYum+jOH2QADHb4Lw_HA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2493 bytes --]
On Thu, Aug 14, 2014 at 09:27:14PM -0400, jonsmirl@gmail.com wrote:
> On Thu, Aug 14, 2014 at 8:45 PM, Henrik Nordström
> <henrik@henriknordstrom.net> wrote:
> > tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl@gmail.com:
> >
> >> So what are the rules going to be? Once the OS is up and starts
> >> changing things the state in the DT and the OS are going to diverge.
> >> How does this get handled for a kernel driver that can load/unload?
> >> Should the kernel remove those state nodes as soon as it alters the
> >> state?
> >
> > hardware that is no longer there should also not be represented/active
> > in DT.
> >
> > A simple fb that have been torn down is no more, no different from never
> > having been there. It can be argued that it never was in the first place
> > (i.e. that it is not actually hardware) but that it another can of worms
> > and both have their benefits and drawbacks. One thing is certain
> > however, as far as simplefb is concerned it is hardware, not really any
> > different from a persistent framebuffer with hardwired settings in
> > hardware.
>
> How does the kernel know what clocks to protect? A list in the
> simplefb node? This list of clocks is a reason for adding simplefb to
> the compatible list for the real hardware. That way it won't be
> duplicated.
>
> The issues with parameter conflicts between simplefb and the real
> hardware can be dealt with. If the real hardware wants to add the
> simplefb compatible field it will need to get its parameters sorted
> out so that they don't conflict. Clock syntax is standardized in the
> DTS so it shouldn't be a problem.
It shouldn't be, but apparently, some disagree.
Anyway, I'm not sure having the simplefb compatible would work in this
use-case, mainly for two reasons:
- Most likely, the bindings are going to be very different. Not only
about which properties you'll have, but also what you will place
in these properties. reg for example have the memory address of
the buffer in the simplefb case, while in the KMS driver case, it
would have the memory address of the registers.
- There's will be a single driver probed. So if you want to go down
the hand over road (which is a bit premature at this point if you
ask me, but anyway), you will have no driver probed to hand over
to.
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 3/4] simplefb: disable dt node upon remove
From: jonsmirl @ 2014-08-15 12:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140815064336.GX15297@lukather>
On Fri, Aug 15, 2014 at 2:43 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Thu, Aug 14, 2014 at 09:27:14PM -0400, jonsmirl@gmail.com wrote:
>> On Thu, Aug 14, 2014 at 8:45 PM, Henrik Nordström
>> <henrik@henriknordstrom.net> wrote:
>> > tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl@gmail.com:
>> >
>> >> So what are the rules going to be? Once the OS is up and starts
>> >> changing things the state in the DT and the OS are going to diverge.
>> >> How does this get handled for a kernel driver that can load/unload?
>> >> Should the kernel remove those state nodes as soon as it alters the
>> >> state?
>> >
>> > hardware that is no longer there should also not be represented/active
>> > in DT.
>> >
>> > A simple fb that have been torn down is no more, no different from never
>> > having been there. It can be argued that it never was in the first place
>> > (i.e. that it is not actually hardware) but that it another can of worms
>> > and both have their benefits and drawbacks. One thing is certain
>> > however, as far as simplefb is concerned it is hardware, not really any
>> > different from a persistent framebuffer with hardwired settings in
>> > hardware.
>>
>> How does the kernel know what clocks to protect? A list in the
>> simplefb node? This list of clocks is a reason for adding simplefb to
>> the compatible list for the real hardware. That way it won't be
>> duplicated.
>>
>> The issues with parameter conflicts between simplefb and the real
>> hardware can be dealt with. If the real hardware wants to add the
>> simplefb compatible field it will need to get its parameters sorted
>> out so that they don't conflict. Clock syntax is standardized in the
>> DTS so it shouldn't be a problem.
>
> It shouldn't be, but apparently, some disagree.
>
> Anyway, I'm not sure having the simplefb compatible would work in this
> use-case, mainly for two reasons:
> - Most likely, the bindings are going to be very different. Not only
> about which properties you'll have, but also what you will place
> in these properties. reg for example have the memory address of
> the buffer in the simplefb case, while in the KMS driver case, it
> would have the memory address of the registers.
> - There's will be a single driver probed. So if you want to go down
> the hand over road (which is a bit premature at this point if you
> ask me, but anyway), you will have no driver probed to hand over
> to.
The graphic drivers have already been down this road before. Simplefb
should be a driver library not a driver. DRM is also a driver library.
So to implement simplefb whip up a real skeleton driver for the video
hardware that parses the DT and then pokes the appropriate info into
the simplefb library. Initially that driver will just include the
calls out to the simplefb library. Later on it can get KMS
implemented. This real driver will get bound to the hardware and have
a dependency that loads the simplefb driver library.
Now there is no handover problem. Only a single driver is ever
attached to the hardware. When it wants to go into KMS mode it makes a
call into the simplefb library to shut down whatever it is doing.
But you still need a mechanism for uboot to signal that it has set up
simplefb. Adding a dynamic node like this might work...
video0: video@01c21800 {
compatible = "allwinner,sun4i-a10-video";
clocks = <&apb0_gates 6>, <&ir0_clk>;
clock-names = "apb", "video";
interrupts = <0 5 4>;
reg = <0x01c21800 0x40>;
status = "enabled";
simplefb {
new namespace for simplefb parameters
}
};
I would have suggested a driver library earlier but it has been a
while since I worked on DRM and I forgot about it.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* [PATCH] powerpc: Fix build error for vga16fb
From: Pranith Kumar @ 2014-08-18 21:13 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Arnd Bergmann,
Peter Griffin, Alexander Shiyan, Jean Delvare, Kukjin Kim,
Mark Brown, open list:FRAMEBUFFER LAYER, open list
Cc: geert, Andrew Morton
In-Reply-To: <1408396422-32658-1-git-send-email-bobby.prani@gmail.com>
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
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] powerpc: Fix build error for vga16fb
From: Geert Uytterhoeven @ 2014-08-18 21:28 UTC (permalink / raw)
To: Pranith Kumar
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Arnd Bergmann,
Peter Griffin, Alexander Shiyan, Jean Delvare, Kukjin Kim,
Mark Brown, open list:FRAMEBUFFER LAYER, open list, Andrew Morton
In-Reply-To: <1408396422-32658-2-git-send-email-bobby.prani@gmail.com>
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/.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH] powerpc: Fix build error for vga16fb
From: Pranith Kumar @ 2014-08-18 21:34 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Arnd Bergmann,
Peter Griffin, Alexander Shiyan, Jean Delvare, Kukjin Kim,
Mark Brown, open list:FRAMEBUFFER LAYER, open list, Andrew Morton
In-Reply-To: <CAMuHMdV_vAPCPh1JdCxtf0iz6haHULUV8t0oYt0jrEAky2WSLw@mail.gmail.com>
On Mon, Aug 18, 2014 at 5:28 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> 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/.
OK. I will remove the dependency on PPC for vga16fb.
--
Pranith
^ permalink raw reply
* [PATCH] powerpc: Fix build error caused by vga16fb
From: Pranith Kumar @ 2014-08-18 21:40 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Arnd Bergmann,
Peter Griffin, Jean Delvare, Jingoo Han, Alexander Shiyan,
Kukjin Kim, Pawel Moll, open list:FRAMEBUFFER LAYER, open list
Cc: geert, Andrew Morton
vga16fb causes a build failure on PPC 32 as follows
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
It was noted by Geert that in PPC, no one is setting vgacon_remap_base because
of which it working on PPC is suspect.
Fix the build failure by disabling vga16fb on PPC.
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..d236fe4 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
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
--
1.9.1
^ permalink raw reply related
* [PATCH] video: ARM CLCD: Ensure bits-per-pixel is a power of 2 and <= 32
From: Jon Medhurst (Tixy) @ 2014-08-19 13:07 UTC (permalink / raw)
To: linux-arm-kernel
If the device-tree specifies a max-memory-bandwidth property then
the CLCD driver uses that to calculate the bits-per-pixel supported,
however, 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.
Acked-by: Pawel Moll <pawel.moll@arm.com>
Signed-off-by: Jon Medhurst <tixy@linaro.org>
---
This fixes code which is new in 3.17 (commit d10715be03) and so I assume
is a candidate for adding to a coming -rc ? Without the fix, people can
be left (as I was) with a blank non-functioning screen even if they
create a valid device-tree for the new driver functionality.
drivers/video/fbdev/amba-clcd.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index beadd3e..98b66b7 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -24,6 +24,7 @@
#include <linux/list.h>
#include <linux/amba/bus.h>
#include <linux/amba/clcd.h>
+#include <linux/bitops.h>
#include <linux/clk.h>
#include <linux/hardirq.h>
#include <linux/dma-mapping.h>
@@ -650,6 +651,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
{
struct device_node *endpoint;
int err;
+ int bpp;
u32 max_bandwidth;
u32 tft_r0b0g0[3];
@@ -667,11 +669,15 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
err = of_property_read_u32(fb->dev->dev.of_node, "max-memory-bandwidth",
&max_bandwidth);
- if (!err)
- fb->panel->bpp = 8 * max_bandwidth / (fb->panel->mode.xres *
+ if (!err) {
+ bpp = 8 * max_bandwidth / (fb->panel->mode.xres *
fb->panel->mode.yres * fb->panel->mode.refresh);
- else
- fb->panel->bpp = 32;
+ bpp = rounddown_pow_of_two(bpp);
+ if (bpp > 32)
+ bpp = 32;
+ } else
+ bpp = 32;
+ fb->panel->bpp = bpp;
#ifdef CONFIG_CPU_BIG_ENDIAN
fb->panel->cntl |= CNTL_BEBO;
--
2.0.1
^ permalink raw reply related
* Re: [PATCH] video: ARM CLCD: Ensure bits-per-pixel is a power of 2 and <= 32
From: Russell King - ARM Linux @ 2014-08-19 14:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1408453651.3746.41.camel@linaro1.home>
On Tue, Aug 19, 2014 at 02:07:31PM +0100, 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, 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.
>
> Acked-by: Pawel Moll <pawel.moll@arm.com>
> Signed-off-by: Jon Medhurst <tixy@linaro.org>
> ---
>
> This fixes code which is new in 3.17 (commit d10715be03) and so I assume
> is a candidate for adding to a coming -rc ? Without the fix, people can
> be left (as I was) with a blank non-functioning screen even if they
> create a valid device-tree for the new driver functionality.
>
> drivers/video/fbdev/amba-clcd.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
> index beadd3e..98b66b7 100644
> --- a/drivers/video/fbdev/amba-clcd.c
> +++ b/drivers/video/fbdev/amba-clcd.c
> @@ -24,6 +24,7 @@
> #include <linux/list.h>
> #include <linux/amba/bus.h>
> #include <linux/amba/clcd.h>
> +#include <linux/bitops.h>
> #include <linux/clk.h>
> #include <linux/hardirq.h>
> #include <linux/dma-mapping.h>
> @@ -650,6 +651,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
> {
> struct device_node *endpoint;
> int err;
> + int bpp;
> u32 max_bandwidth;
> u32 tft_r0b0g0[3];
>
> @@ -667,11 +669,15 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
>
> err = of_property_read_u32(fb->dev->dev.of_node, "max-memory-bandwidth",
> &max_bandwidth);
> - if (!err)
> - fb->panel->bpp = 8 * max_bandwidth / (fb->panel->mode.xres *
> + if (!err) {
> + bpp = 8 * max_bandwidth / (fb->panel->mode.xres *
> fb->panel->mode.yres * fb->panel->mode.refresh);
This calculation is wrong in any case - this is the naïeve calculation
which assumes that the bandwidth is:
x * y * (bpp / 8) * refresh
That isn't the maximum bandwidth, it's the average bandwidth across a
full frame.
If we're interested in limiting the maximum bandwidth, because the
hardware can't cope with fetching the data above a certain rate, then
we need a different method.
We know the pixel rate. We know how many memory bits are fetched for
each pixel. So:
peak_bandwidth = pixel_clock * bpp / 8
Let's take 32bpp, 1024x768@60Hz, which has a pixel clock of 65MHz. Using
the first, we get:
1024 * 768 * 4 * 60 = 180MiB/s.
Using the second:
65 * 4 = 248MiB/s.
To see why there's this discrepency, realise that there's 320 clocks where
there's no pixel activity (so no memory fetches) per line, and 38 lines
where there are no memory fetches. That's 320 * 768 + 1344 * 38 = 296832
pixel clocks where there's no memory fetches, out of a total of 1344 * 806
= 1083264 total pixel clocks in a frame. So, that's about 27% of the frame
where the memory subsystem is inactive - which accounts for the difference
in the above figure.
So, if you are limiting the bandwidth of the mode due to the available bus
bandwidth to the controller (which is implied by the property name), then
you want to use the second equation to limit the peak bandwidth, and not
the average bandwidth.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [RFC] fbdev: mxsfb: Add support for mx6sl and mx6sx
From: Fabio Estevam @ 2014-08-19 18:23 UTC (permalink / raw)
To: linux-fbdev
mx6sl and mx6sx share the same LCD controller as mx23 and mx28.
Add support for it.
The basic difference is the number of clocks that are required:
- mx23/mx28: only one clock
- mx6sl: two clocks
- mx6sx: three clocks
Tested on a mx28evk and mx6slevk.
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
I am sending it as RFC because the scrolling is showing some artifacts when
I boot with console=tty0. The kernel messages are displayed correctly, but when
it scrolls I see some garbage.
Appreciate any comment/suggestion to improve this.
drivers/video/fbdev/Kconfig | 2 +-
drivers/video/fbdev/mxsfb.c | 47 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index e911b9c..fd28987 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -2420,7 +2420,7 @@ config FB_JZ4740
config FB_MXS
tristate "MXS LCD framebuffer support"
- depends on FB && ARCH_MXS
+ depends on FB && (ARCH_MXS || ARCH_MXC)
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
diff --git a/drivers/video/fbdev/mxsfb.c b/drivers/video/fbdev/mxsfb.c
index accf48a2..a567cc6 100644
--- a/drivers/video/fbdev/mxsfb.c
+++ b/drivers/video/fbdev/mxsfb.c
@@ -172,6 +172,10 @@ struct mxsfb_info {
struct fb_info fb_info;
struct platform_device *pdev;
struct clk *clk;
+ struct clk *clk_axi;
+ struct clk *clk_disp_axi;
+ bool clk_axi_enabled;
+ bool clk_disp_axi_enabled;
void __iomem *base; /* registers */
unsigned allocated_size;
int enabled;
@@ -185,6 +189,42 @@ struct mxsfb_info {
#define mxsfb_is_v3(host) (host->devdata->ipversion = 3)
#define mxsfb_is_v4(host) (host->devdata->ipversion = 4)
+/* Enable lcdif axi clock */
+static inline void clk_enable_axi(struct mxsfb_info *host)
+{
+ if (!host->clk_axi_enabled && (!IS_ERR(host->clk_axi))) {
+ clk_prepare_enable(host->clk_axi);
+ host->clk_axi_enabled = true;
+ }
+}
+
+/* Disable lcdif axi clock */
+static inline void clk_disable_axi(struct mxsfb_info *host)
+{
+ if (host->clk_axi_enabled && (!IS_ERR(host->clk_axi))) {
+ clk_disable_unprepare(host->clk_axi);
+ host->clk_axi_enabled = false;
+ }
+}
+
+/* Enable DISP axi clock */
+static inline void clk_enable_disp_axi(struct mxsfb_info *host)
+{
+ if (!host->clk_disp_axi_enabled && (!IS_ERR(host->clk_disp_axi))) {
+ clk_prepare_enable(host->clk_disp_axi);
+ host->clk_disp_axi_enabled = true;
+ }
+}
+
+/* Disable DISP axi clock */
+static inline void clk_disable_disp_axi(struct mxsfb_info *host)
+{
+ if (host->clk_disp_axi_enabled && (!IS_ERR(host->clk_disp_axi))) {
+ clk_disable_unprepare(host->clk_disp_axi);
+ host->clk_disp_axi_enabled = false;
+ }
+}
+
static const struct mxsfb_devdata mxsfb_devdata[] = {
[MXSFB_V3] = {
.transfer_count = LCDC_V3_TRANSFER_COUNT,
@@ -331,6 +371,8 @@ static void mxsfb_enable_controller(struct fb_info *fb_info)
}
}
+ clk_enable_axi(host);
+ clk_enable_disp_axi(host);
clk_prepare_enable(host->clk);
clk_set_rate(host->clk, PICOS2KHZ(fb_info->var.pixclock) * 1000U);
@@ -374,6 +416,8 @@ static void mxsfb_disable_controller(struct fb_info *fb_info)
writel(reg & ~VDCTRL4_SYNC_SIGNALS_ON, host->base + LCDC_VDCTRL4);
clk_disable_unprepare(host->clk);
+ clk_disable_disp_axi(host);
+ clk_disable_axi(host);
host->enabled = 0;
@@ -867,6 +911,9 @@ static int mxsfb_probe(struct platform_device *pdev)
goto fb_release;
}
+ host->clk_axi = devm_clk_get(&host->pdev->dev, "axi");
+ host->clk_disp_axi = devm_clk_get(&host->pdev->dev, "disp_axi");
+
host->reg_lcd = devm_regulator_get(&pdev->dev, "lcd");
if (IS_ERR(host->reg_lcd))
host->reg_lcd = NULL;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] video: ARM CLCD: Ensure bits-per-pixel is a power of 2 and <= 32
From: Jon Medhurst (Tixy) @ 2014-08-20 8:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140819144032.GD30401@n2100.arm.linux.org.uk>
On Tue, 2014-08-19 at 15:40 +0100, Russell King - ARM Linux wrote:
> On Tue, Aug 19, 2014 at 02:07:31PM +0100, 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, 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.
> >
> > Acked-by: Pawel Moll <pawel.moll@arm.com>
> > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > ---
> >
> > This fixes code which is new in 3.17 (commit d10715be03) and so I assume
> > is a candidate for adding to a coming -rc ? Without the fix, people can
> > be left (as I was) with a blank non-functioning screen even if they
> > create a valid device-tree for the new driver functionality.
> >
> > drivers/video/fbdev/amba-clcd.c | 14 ++++++++++----
> > 1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
> > index beadd3e..98b66b7 100644
> > --- a/drivers/video/fbdev/amba-clcd.c
> > +++ b/drivers/video/fbdev/amba-clcd.c
> > @@ -24,6 +24,7 @@
> > #include <linux/list.h>
> > #include <linux/amba/bus.h>
> > #include <linux/amba/clcd.h>
> > +#include <linux/bitops.h>
> > #include <linux/clk.h>
> > #include <linux/hardirq.h>
> > #include <linux/dma-mapping.h>
> > @@ -650,6 +651,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
> > {
> > struct device_node *endpoint;
> > int err;
> > + int bpp;
> > u32 max_bandwidth;
> > u32 tft_r0b0g0[3];
> >
> > @@ -667,11 +669,15 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
> >
> > err = of_property_read_u32(fb->dev->dev.of_node, "max-memory-bandwidth",
> > &max_bandwidth);
> > - if (!err)
> > - fb->panel->bpp = 8 * max_bandwidth / (fb->panel->mode.xres *
> > + if (!err) {
> > + bpp = 8 * max_bandwidth / (fb->panel->mode.xres *
> > fb->panel->mode.yres * fb->panel->mode.refresh);
>
> This calculation is wrong in any case - this is the naïeve calculation
> which assumes that the bandwidth is:
>
> x * y * (bpp / 8) * refresh
>
> That isn't the maximum bandwidth, it's the average bandwidth across a
> full frame.
>
> If we're interested in limiting the maximum bandwidth, because the
> hardware can't cope with fetching the data above a certain rate, then
> we need a different method.
>
> We know the pixel rate. We know how many memory bits are fetched for
> each pixel. So:
>
> peak_bandwidth = pixel_clock * bpp / 8
That all sounds logical and reasonable to me, especially as the binding
doc describes the property as "maximum bandwidth in bytes per second
that the cell's memory interface can handle".
I'll update the patch to also correct the calculation as you suggest.
--
Tixy
^ permalink raw reply
* [PATCH v2] video: ARM CLCD: Fix calculation of bits-per-pixel
From: Jon Medhurst (Tixy) @ 2014-08-20 12:41 UTC (permalink / raw)
To: linux-arm-kernel
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>
---
Changes since v1:
- Changed formula for calculating bits per pixel
- Changed commit message and subject, was:
"video: ARM CLCD: Ensure bits-per-pixel is a power of 2 and <= 32"
drivers/video/fbdev/amba-clcd.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index beadd3e..a7b6217 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -24,6 +24,7 @@
#include <linux/list.h>
#include <linux/amba/bus.h>
#include <linux/amba/clcd.h>
+#include <linux/bitops.h>
#include <linux/clk.h>
#include <linux/hardirq.h>
#include <linux/dma-mapping.h>
@@ -650,6 +651,7 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
{
struct device_node *endpoint;
int err;
+ unsigned int bpp;
u32 max_bandwidth;
u32 tft_r0b0g0[3];
@@ -667,11 +669,22 @@ static int clcdfb_of_init_display(struct clcd_fb *fb)
err = of_property_read_u32(fb->dev->dev.of_node, "max-memory-bandwidth",
&max_bandwidth);
- if (!err)
- fb->panel->bpp = 8 * max_bandwidth / (fb->panel->mode.xres *
- fb->panel->mode.yres * fb->panel->mode.refresh);
- else
- fb->panel->bpp = 32;
+ if (!err) {
+ /*
+ * max_bandwidth is in bytes per second and pixclock in
+ * pico-seconds, so the maximum allowed bits per pixel is
+ * 8 * max_bandwidth / (PICOS2KHZ(pixclock) * 1000)
+ * Rearrange this calculation to avoid overflow and then ensure
+ * result is a valid format.
+ */
+ bpp = max_bandwidth / (1000 / 8)
+ / PICOS2KHZ(fb->panel->mode.pixclock);
+ bpp = rounddown_pow_of_two(bpp);
+ if (bpp > 32)
+ bpp = 32;
+ } else
+ bpp = 32;
+ fb->panel->bpp = bpp;
#ifdef CONFIG_CPU_BIG_ENDIAN
fb->panel->cntl |= CNTL_BEBO;
--
2.1.0.rc1
^ permalink raw reply related
* Re: [PATCH v2] video: ARM CLCD: Fix calculation of bits-per-pixel
From: Pawel Moll @ 2014-08-20 13:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1408538464.3858.43.camel@linaro1.home>
On Wed, 2014-08-20 at 13:41 +0100, 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>
Acked-by: Pawel Moll <pawel.moll@arm.com>
^ permalink raw reply
* [PATCH] fbdev: Remove __init from chips_hw_init() to fix build failure
From: Pranith Kumar @ 2014-08-21 12:23 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen, Rob Clark,
Jingoo Han, Laurent Pinchart, Daniel Vetter,
open list:FRAMEBUFFER LAYER, open list
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;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v7][ 3/5] video: mx3fb: Introduce regulator support.
From: Mark Brown @ 2014-08-22 22:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5397083D.4090400@eukrea.com>
[-- Attachment #1: Type: text/plain, Size: 1411 bytes --]
On Tue, Jun 10, 2014 at 03:29:33PM +0200, Denis Carikli wrote:
> On 03/17/2014 07:20 AM, Sascha Hauer wrote:
> >This patch adds regulator support for the display of a i.MX3 IPU. The
> >problem Denis has to solve here is that he needs to get the regulator,
> >but the display devicenode doesn't have a struct device associated with
> >it, so he cannot provide one to regulator_get(). One way out here could
> >be a of_regulator_get(struct device_node *). Mark, would this be ok with
> >you?
> Here, the display devicenode has no struct device associated with it like
> mentioned above.
> Because of that, retriving the regulator from the devicetree, for instance
> like regulator_dev_lookup() does, would require a different approach.
> As I understand it, what happen in regulator_dev_lookup when
> regulator_get(NULL, regulator_name) is used is the following:
> Since the struct device is NULL, it skips the struct device based lookup,
> but also the devicetree lookup (it uses dev->of_node for that)
> At the end it falls back on a match on the regulator name to find it.
> would keeping regulator_get(NULL, regulator_name); in the driver instead be
> better for now?
It would be much better to have an actual device to get things for,
using a hard coded regulator name is very much deprecated. Using a hard
coded name is fragile and we don't have support for mapping this
properly in the device tree.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH 8/9 v2] video: fbdev: aty: use c99 initializers in structures
From: Julia Lawall @ 2014-08-23 15:50 UTC (permalink / raw)
To: Josh Triplett
Cc: Paul Mackerras, kernel-janitors, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev, linux-kernel
In-Reply-To: <20140823152015.GB2074@thin>
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/)
// <smpl>
@decl@
identifier i1,fld;
type T;
field list[n] fs;
@@
struct i1 {
fs
T fld;
...};
@bad@
identifier decl.i1,i2;
expression e;
initializer list[decl.n] is;
@@
struct i1 i2 = { is,
+ .fld = e
- e
,...};
// </smpl>
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
The patches in this series do not depend on each other.
v2: Move close braces down to the next line and add trailing commas, as
suggested by Josh Triplett.
diff --git a/drivers/video/fbdev/aty/aty128fb.c b/drivers/video/fbdev/aty/aty128fb.c
index ff60701..aedf2fb 100644
--- a/drivers/video/fbdev/aty/aty128fb.c
+++ b/drivers/video/fbdev/aty/aty128fb.c
@@ -324,14 +324,61 @@ struct aty128_meminfo {
};
/* various memory configurations */
-static const struct aty128_meminfo sdr_128 - { 4, 4, 3, 3, 1, 3, 1, 16, 30, 16, "128-bit SDR SGRAM (1:1)" };
-static const struct aty128_meminfo sdr_64 - { 4, 8, 3, 3, 1, 3, 1, 17, 46, 17, "64-bit SDR SGRAM (1:1)" };
-static const struct aty128_meminfo sdr_sgram - { 4, 4, 1, 2, 1, 2, 1, 16, 24, 16, "64-bit SDR SGRAM (2:1)" };
-static const struct aty128_meminfo ddr_sgram - { 4, 4, 3, 3, 2, 3, 1, 16, 31, 16, "64-bit DDR SGRAM" };
+static const struct aty128_meminfo sdr_128 = {
+ .ML = 4,
+ .MB = 4,
+ .Trcd = 3,
+ .Trp = 3,
+ .Twr = 1,
+ .CL = 3,
+ .Tr2w = 1,
+ .LoopLatency = 16,
+ .DspOn = 30,
+ .Rloop = 16,
+ .name = "128-bit SDR SGRAM (1:1)",
+};
+
+static const struct aty128_meminfo sdr_64 = {
+ .ML = 4,
+ .MB = 8,
+ .Trcd = 3,
+ .Trp = 3,
+ .Twr = 1,
+ .CL = 3,
+ .Tr2w = 1,
+ .LoopLatency = 17,
+ .DspOn = 46,
+ .Rloop = 17,
+ .name = "64-bit SDR SGRAM (1:1)",
+};
+
+static const struct aty128_meminfo sdr_sgram = {
+ .ML = 4,
+ .MB = 4,
+ .Trcd = 1,
+ .Trp = 2,
+ .Twr = 1,
+ .CL = 2,
+ .Tr2w = 1,
+ .LoopLatency = 16,
+ .DspOn = 24,
+ .Rloop = 16,
+ .name = "64-bit SDR SGRAM (2:1)",
+};
+
+static const struct aty128_meminfo ddr_sgram = {
+ .ML = 4,
+ .MB = 4,
+ .Trcd = 3,
+ .Trp = 3,
+ .Twr = 2,
+ .CL = 3,
+ .Tr2w = 1,
+ .LoopLatency = 16,
+ .DspOn = 31,
+ .Rloop = 16,
+ .name = "64-bit DDR SGRAM",
+};
static struct fb_fix_screeninfo aty128fb_fix = {
.id = "ATY Rage128",
^ permalink raw reply related
* Re: [PATCH 8/9 v2] video: fbdev: aty: use c99 initializers in structures
From: Josh Triplett @ 2014-08-23 16:03 UTC (permalink / raw)
To: Julia Lawall
Cc: Paul Mackerras, kernel-janitors, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev, linux-kernel
In-Reply-To: <alpine.DEB.2.02.1408231749030.2068@localhost6.localdomain6>
On Sat, Aug 23, 2014 at 05:50:28PM +0200, Julia Lawall wrote:
> 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/)
>
> // <smpl>
> @decl@
> identifier i1,fld;
> type T;
> field list[n] fs;
> @@
>
> struct i1 {
> fs
> T fld;
> ...};
>
> @bad@
> identifier decl.i1,i2;
> expression e;
> initializer list[decl.n] is;
> @@
>
> struct i1 i2 = { is,
> + .fld = e
> - e
> ,...};
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> v2: Move close braces down to the next line and add trailing commas, as
> suggested by Josh Triplett.
>
> diff --git a/drivers/video/fbdev/aty/aty128fb.c b/drivers/video/fbdev/aty/aty128fb.c
> index ff60701..aedf2fb 100644
> --- a/drivers/video/fbdev/aty/aty128fb.c
> +++ b/drivers/video/fbdev/aty/aty128fb.c
> @@ -324,14 +324,61 @@ struct aty128_meminfo {
> };
>
> /* various memory configurations */
> -static const struct aty128_meminfo sdr_128 > - { 4, 4, 3, 3, 1, 3, 1, 16, 30, 16, "128-bit SDR SGRAM (1:1)" };
> -static const struct aty128_meminfo sdr_64 > - { 4, 8, 3, 3, 1, 3, 1, 17, 46, 17, "64-bit SDR SGRAM (1:1)" };
> -static const struct aty128_meminfo sdr_sgram > - { 4, 4, 1, 2, 1, 2, 1, 16, 24, 16, "64-bit SDR SGRAM (2:1)" };
> -static const struct aty128_meminfo ddr_sgram > - { 4, 4, 3, 3, 2, 3, 1, 16, 31, 16, "64-bit DDR SGRAM" };
> +static const struct aty128_meminfo sdr_128 = {
> + .ML = 4,
> + .MB = 4,
> + .Trcd = 3,
> + .Trp = 3,
> + .Twr = 1,
> + .CL = 3,
> + .Tr2w = 1,
> + .LoopLatency = 16,
> + .DspOn = 30,
> + .Rloop = 16,
> + .name = "128-bit SDR SGRAM (1:1)",
> +};
> +
> +static const struct aty128_meminfo sdr_64 = {
> + .ML = 4,
> + .MB = 8,
> + .Trcd = 3,
> + .Trp = 3,
> + .Twr = 1,
> + .CL = 3,
> + .Tr2w = 1,
> + .LoopLatency = 17,
> + .DspOn = 46,
> + .Rloop = 17,
> + .name = "64-bit SDR SGRAM (1:1)",
> +};
> +
> +static const struct aty128_meminfo sdr_sgram = {
> + .ML = 4,
> + .MB = 4,
> + .Trcd = 1,
> + .Trp = 2,
> + .Twr = 1,
> + .CL = 2,
> + .Tr2w = 1,
> + .LoopLatency = 16,
> + .DspOn = 24,
> + .Rloop = 16,
> + .name = "64-bit SDR SGRAM (2:1)",
> +};
> +
> +static const struct aty128_meminfo ddr_sgram = {
> + .ML = 4,
> + .MB = 4,
> + .Trcd = 3,
> + .Trp = 3,
> + .Twr = 2,
> + .CL = 3,
> + .Tr2w = 1,
> + .LoopLatency = 16,
> + .DspOn = 31,
> + .Rloop = 16,
> + .name = "64-bit DDR SGRAM",
> +};
>
> static struct fb_fix_screeninfo aty128fb_fix = {
> .id = "ATY Rage128",
^ 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