* Re: [PATCH v3 00/14] Rework OMAP4+ HDMI audio support
From: Jyri Sarha @ 2014-08-13 12:51 UTC (permalink / raw)
To: Mark Brown
Cc: alsa-devel, linux-fbdev, linux-omap, peter.ujfalusi,
liam.r.girdwood, tomi.valkeinen, detheridge
In-Reply-To: <20140813110425.GL17528@sirena.org.uk>
On 08/13/2014 02:04 PM, Mark Brown wrote:
> On Fri, Aug 08, 2014 at 04:23:41PM +0300, Jyri Sarha wrote:
>> Question to ASoC maintainers:
>> Do feel it is Ok to put ASoC code outside sound/soc the way I do
>> in these patches (in patch 0006 particularly)?
>
> I'm not thrilled about the idea, it's going to result in breakage during
> API updates and more cross tree coordination needs.
>
I guess then the least bad alternative would be cutting the cpu-dai
driver away from drivers/video/fbdev/omap2/dss/hdmi_audio.c and placing
it under sound/soc/omap and registering it from hdmi_audio.c the same
way as hdmi-audio-codec and asoc-simple-card is now registered.
There would be two devices for a single ip, but at least the ASoC side
driver would receive its resources directly from the HDMI video driver
with necessary tools to synchronize the access.
This approach would work for other HDMI audio situation too, where there
would usually (for tda998x and SiI9022 at least) be a need to register
an i2s codec driver associated with the HDMI encoder. It would probably
be possible to make a single codec driver to work with multiple HDMI
encoders if the API between the codec and endcoder drivers is designed
that in mind.
How does this sound?
Best regards,
Jyri
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: jonsmirl @ 2014-08-13 12:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140813101923.GD31326@skynet.be>
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.
The switch over from simple to KMS should not be done via a node
add/del to the device tree either. No one has removed the device from
the system, the device tree should not be changing.
Some Linux mechanism inside the kernel needs to handle that
transition. Somehow simple needs to hang onto the clocks, let go of
the device, load KMS, then exit?
>
> Luc Verhaegen.
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [PATCH] backlight: remove .owner field for drivers using module_platform_driver
From: Lee Jones @ 2014-08-13 13:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1407933518-7214-1-git-send-email-peter.griffin@linaro.org>
On Wed, 13 Aug 2014, Peter Griffin wrote:
> This patch removes the superflous .owner field for drivers which
> use the module_platform_driver or platform_driver_register api,
> as this is overriden in __platform_driver_register.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
> drivers/video/backlight/88pm860x_bl.c | 1 -
> drivers/video/backlight/aat2870_bl.c | 1 -
> drivers/video/backlight/adp5520_bl.c | 1 -
> drivers/video/backlight/as3711_bl.c | 1 -
> drivers/video/backlight/da903x_bl.c | 1 -
> drivers/video/backlight/da9052_bl.c | 1 -
> drivers/video/backlight/ep93xx_bl.c | 1 -
> drivers/video/backlight/gpio_backlight.c | 1 -
> drivers/video/backlight/lm3533_bl.c | 1 -
> drivers/video/backlight/lp8788_bl.c | 1 -
> drivers/video/backlight/max8925_bl.c | 1 -
> drivers/video/backlight/ot200_bl.c | 1 -
> drivers/video/backlight/pandora_bl.c | 1 -
> drivers/video/backlight/platform_lcd.c | 1 -
> drivers/video/backlight/pwm_bl.c | 1 -
> drivers/video/backlight/tps65217_bl.c | 1 -
> drivers/video/backlight/wm831x_bl.c | 1 -
> 17 files changed, 17 deletions(-)
Applied, thanks.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 4/4] simplefb: add clock handling code
From: Stephen Warren @ 2014-08-13 16:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1407914239-12054-5-git-send-email-libv@skynet.be>
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.
The correct way to handle this without a complete DRM/KMS/... driver is
to avoid the clocks in question being turned off at boot. I thought
there was a per-clock flag to prevent disabling an unused clock? If not,
perhaps the clock driver should force the clock to be enabled (perhaps
only if the DRM/KMS driver isn't enabled?). 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.
^ permalink raw reply
* Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Stephen Warren @ 2014-08-13 16:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CANq1E4ReOB87RX8VhnW1Ln=SK3Tw5V962ujcsfuz-2E9Uc61yQ@mail.gmail.com>
On 08/13/2014 02:49 AM, David Herrmann wrote:
> Hi
>
> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
>> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>> The next commit will handle clocks correctly, so that these do not get
>>> automatically disabled on certain SoC simplefb implementations. As a
>>> result, the removal of this simplefb driver, and the release of the
>>> clocks, is rather final, and only a full display driver can work after
>>> this. So, it makes sense to also flag the dt node as disabled, even
>>> though it has no real value today.
>>>
>>> Signed-off-by: Luc Verhaegen <libv@skynet.be>
>>
>> Please, no.
>>
>> Drivers should not be modifying the device tree without and
>> exceptionally good reason for doing so. Drivers are to treat the DT as
>> immutable.
>>
>> * the exception is an overlay driver which add new devices to the
>> kernel. Definitely not the case here.
>
> Why? I think we have exactly that case:
> * DT describes the real hw properly and those parts are immutable
> * Additionally, bootloaders create firmware-framebuffers and
> create simple-framebuffer devices for them. Those are
> valid as long as no driver reconfigured the real hw.
> * Once a real hw-driver loads, it might destroy the existing
> framebuffers, thus, it should also destroy the platform device.
> * If the real hw-driver is unloaded, it might re-create the FB
> and thus create a new (or enable the old) platform device.
My intention was always that a bootloader's addition of a simplefb node
to the DT would be user-configurable or driven by the original DT
content. As such, there shouldn't ever be both a DT node describing the
"real" HW and simplefb. In other words, if the DT already has the "real"
DT node, the bootloader should automatically (or under user command) not
add the simpefb node.
^ permalink raw reply
* Re: [PATCH 1/4] simplefb: formalize pseudo palette handling
From: Stephen Warren @ 2014-08-13 16:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1407914239-12054-2-git-send-email-libv@skynet.be>
On 08/13/2014 01:17 AM, Luc Verhaegen wrote:
> Signed-off-by: Luc Verhaegen <libv@skynet.be>
Patch description?
Assuming any comments anyone else had are addressed, patches 1 and 2 both,
Acked-by: Stephen Warren <swarren@nvidia.com>
^ permalink raw reply
* Re: [PATCH 4/4] simplefb: add clock handling code
From: Luc Verhaegen @ 2014-08-13 16:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53EB9471.3090204@wwwdotorg.org>
On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>
> 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.
Yes. Simplefb quickly becomes anything but, doesn't it. Perhaps DenialFB
would've been a better name for it ;p
> The correct way to handle this without a complete DRM/KMS/... driver is
> to avoid the clocks in question being turned off at boot. I thought
> there was a per-clock flag to prevent disabling an unused clock? If not,
> perhaps the clock driver should force the clock to be enabled (perhaps
> only if the DRM/KMS driver isn't enabled?). 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.
How was simplefb handled on tegra? Where is the code for that? I didn't
see anything in u-boot for instance.
But the code for handling clocks where they are supposed to be handled
is pretty generic from where i sit.
Luc Verhaegen.
^ permalink raw reply
* Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-13 17:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53EB9471.3090204@wwwdotorg.org>
[-- Attachment #1: Type: text/plain, Size: 2674 bytes --]
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.
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-13 17:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53EB95D4.6010500@wwwdotorg.org>
On Wed, Aug 13, 2014 at 12:44 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/13/2014 02:49 AM, David Herrmann wrote:
>>
>> Hi
>>
>> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
>> <grant.likely@secretlab.ca> wrote:
>>>
>>> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>>>
>>>> The next commit will handle clocks correctly, so that these do not get
>>>> automatically disabled on certain SoC simplefb implementations. As a
>>>> result, the removal of this simplefb driver, and the release of the
>>>> clocks, is rather final, and only a full display driver can work after
>>>> this. So, it makes sense to also flag the dt node as disabled, even
>>>> though it has no real value today.
>>>>
>>>> Signed-off-by: Luc Verhaegen <libv@skynet.be>
>>>
>>>
>>> Please, no.
>>>
>>> Drivers should not be modifying the device tree without and
>>> exceptionally good reason for doing so. Drivers are to treat the DT as
>>> immutable.
>>>
>>> * the exception is an overlay driver which add new devices to the
>>> kernel. Definitely not the case here.
>>
>>
>> Why? I think we have exactly that case:
>> * DT describes the real hw properly and those parts are immutable
>> * Additionally, bootloaders create firmware-framebuffers and
>> create simple-framebuffer devices for them. Those are
>> valid as long as no driver reconfigured the real hw.
>> * Once a real hw-driver loads, it might destroy the existing
>> framebuffers, thus, it should also destroy the platform device.
>> * If the real hw-driver is unloaded, it might re-create the FB
>> and thus create a new (or enable the old) platform device.
>
>
> My intention was always that a bootloader's addition of a simplefb node to
> the DT would be user-configurable or driven by the original DT content. As
> such, there shouldn't ever be both a DT node describing the "real" HW and
> simplefb. In other words, if the DT already has the "real" DT node, the
> bootloader should automatically (or under user command) not add the simpefb
> node.
DT is just the wrong mechanism to signal this, use an ATAG or kernel
command line parameter.
real hardware has compatible = "real-hardware-name, simplefb"
simplefb is built into kernel. It will attach to the device because of
the compatible string. Then it can look at the command line and see if
the bootloader also supported simplefb and already set things up.
Then some mechanism will have to be designed to arrange a handoff
between simplefb and the chip specific KMS driver. But that's a Linux
problem, not a DT one.
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [PATCH v3 00/14] Rework OMAP4+ HDMI audio support
From: Mark Brown @ 2014-08-13 17:26 UTC (permalink / raw)
To: Jyri Sarha
Cc: alsa-devel, linux-fbdev, linux-omap, peter.ujfalusi,
liam.r.girdwood, tomi.valkeinen, detheridge
In-Reply-To: <53EB5F6A.9060308@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]
On Wed, Aug 13, 2014 at 03:51:54PM +0300, Jyri Sarha wrote:
> I guess then the least bad alternative would be cutting the cpu-dai driver
> away from drivers/video/fbdev/omap2/dss/hdmi_audio.c and placing it under
> sound/soc/omap and registering it from hdmi_audio.c the same way as
> hdmi-audio-codec and asoc-simple-card is now registered.
> There would be two devices for a single ip, but at least the ASoC side
> driver would receive its resources directly from the HDMI video driver with
> necessary tools to synchronize the access.
Just a MFD really.
> This approach would work for other HDMI audio situation too, where there
> would usually (for tda998x and SiI9022 at least) be a need to register an
> i2s codec driver associated with the HDMI encoder. It would probably be
> possible to make a single codec driver to work with multiple HDMI encoders
> if the API between the codec and endcoder drivers is designed that in mind.
> How does this sound?
This sounds like a way forward, I definitely think it's a good idea to
standardise the interfaces as much as we can.
[-- 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: Stephen Warren @ 2014-08-13 17:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKON4OxoPDCAvOK8UEeMaJDuzFfQHwb0pVbFgoqSkn=_W5pAYw@mail.gmail.com>
On 08/13/2014 11:26 AM, jonsmirl@gmail.com wrote:
> On Wed, Aug 13, 2014 at 12:44 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/13/2014 02:49 AM, David Herrmann wrote:
>>>
>>> Hi
>>>
>>> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
>>> <grant.likely@secretlab.ca> wrote:
>>>>
>>>> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>>>>
>>>>> The next commit will handle clocks correctly, so that these do not get
>>>>> automatically disabled on certain SoC simplefb implementations. As a
>>>>> result, the removal of this simplefb driver, and the release of the
>>>>> clocks, is rather final, and only a full display driver can work after
>>>>> this. So, it makes sense to also flag the dt node as disabled, even
>>>>> though it has no real value today.
>>>>>
>>>>> Signed-off-by: Luc Verhaegen <libv@skynet.be>
>>>>
>>>>
>>>> Please, no.
>>>>
>>>> Drivers should not be modifying the device tree without and
>>>> exceptionally good reason for doing so. Drivers are to treat the DT as
>>>> immutable.
>>>>
>>>> * the exception is an overlay driver which add new devices to the
>>>> kernel. Definitely not the case here.
>>>
>>>
>>> Why? I think we have exactly that case:
>>> * DT describes the real hw properly and those parts are immutable
>>> * Additionally, bootloaders create firmware-framebuffers and
>>> create simple-framebuffer devices for them. Those are
>>> valid as long as no driver reconfigured the real hw.
>>> * Once a real hw-driver loads, it might destroy the existing
>>> framebuffers, thus, it should also destroy the platform device.
>>> * If the real hw-driver is unloaded, it might re-create the FB
>>> and thus create a new (or enable the old) platform device.
>>
>>
>> My intention was always that a bootloader's addition of a simplefb node to
>> the DT would be user-configurable or driven by the original DT content. As
>> such, there shouldn't ever be both a DT node describing the "real" HW and
>> simplefb. In other words, if the DT already has the "real" DT node, the
>> bootloader should automatically (or under user command) not add the simpefb
>> node.
>
> DT is just the wrong mechanism to signal this, use an ATAG or kernel
> command line parameter.
>
> real hardware has compatible = "real-hardware-name, simplefb"
>
> simplefb is built into kernel. It will attach to the device because of
> the compatible string. Then it can look at the command line and see if
> the bootloader also supported simplefb and already set things up.
>
> Then some mechanism will have to be designed to arrange a handoff
> between simplefb and the chip specific KMS driver. But that's a Linux
> problem, not a DT one.
Having a single DT node that conforms to both the binding for
"real-hardware-name" and "simplefb" doesn't seem like a good approach.
What if the properties required by the two bindings conflict in some
way? The approach you advocate certainly hasn't ever been used AFAIK.
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: jonsmirl @ 2014-08-13 17:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53EBA18F.1030309@wwwdotorg.org>
On Wed, Aug 13, 2014 at 1:34 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/13/2014 11:26 AM, jonsmirl@gmail.com wrote:
>>
>> On Wed, Aug 13, 2014 at 12:44 PM, Stephen Warren <swarren@wwwdotorg.org>
>> wrote:
>>>
>>> On 08/13/2014 02:49 AM, David Herrmann wrote:
>>>>
>>>>
>>>> Hi
>>>>
>>>> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely
>>>> <grant.likely@secretlab.ca> wrote:
>>>>>
>>>>>
>>>>> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote:
>>>>>>
>>>>>>
>>>>>> The next commit will handle clocks correctly, so that these do not get
>>>>>> automatically disabled on certain SoC simplefb implementations. As a
>>>>>> result, the removal of this simplefb driver, and the release of the
>>>>>> clocks, is rather final, and only a full display driver can work after
>>>>>> this. So, it makes sense to also flag the dt node as disabled, even
>>>>>> though it has no real value today.
>>>>>>
>>>>>> Signed-off-by: Luc Verhaegen <libv@skynet.be>
>>>>>
>>>>>
>>>>>
>>>>> Please, no.
>>>>>
>>>>> Drivers should not be modifying the device tree without and
>>>>> exceptionally good reason for doing so. Drivers are to treat the DT as
>>>>> immutable.
>>>>>
>>>>> * the exception is an overlay driver which add new devices to the
>>>>> kernel. Definitely not the case here.
>>>>
>>>>
>>>>
>>>> Why? I think we have exactly that case:
>>>> * DT describes the real hw properly and those parts are immutable
>>>> * Additionally, bootloaders create firmware-framebuffers and
>>>> create simple-framebuffer devices for them. Those are
>>>> valid as long as no driver reconfigured the real hw.
>>>> * Once a real hw-driver loads, it might destroy the existing
>>>> framebuffers, thus, it should also destroy the platform device.
>>>> * If the real hw-driver is unloaded, it might re-create the FB
>>>> and thus create a new (or enable the old) platform device.
>>>
>>>
>>>
>>> My intention was always that a bootloader's addition of a simplefb node
>>> to
>>> the DT would be user-configurable or driven by the original DT content.
>>> As
>>> such, there shouldn't ever be both a DT node describing the "real" HW and
>>> simplefb. In other words, if the DT already has the "real" DT node, the
>>> bootloader should automatically (or under user command) not add the
>>> simpefb
>>> node.
>>
>>
>> DT is just the wrong mechanism to signal this, use an ATAG or kernel
>> command line parameter.
>>
>> real hardware has compatible = "real-hardware-name, simplefb"
>>
>> simplefb is built into kernel. It will attach to the device because of
>> the compatible string. Then it can look at the command line and see if
>> the bootloader also supported simplefb and already set things up.
>>
>> Then some mechanism will have to be designed to arrange a handoff
>> between simplefb and the chip specific KMS driver. But that's a Linux
>> problem, not a DT one.
>
>
> Having a single DT node that conforms to both the binding for
> "real-hardware-name" and "simplefb" doesn't seem like a good approach. What
> if the properties required by the two bindings conflict in some way? The
> approach you advocate certainly hasn't ever been used AFAIK.
I believe we do have something like this - SPI core implements a lot
of core DT functions. All SPI nodes use this core. Then hardware
specific attributes are added.
The conflicts would need to get sorted out. That's why we should have
a schema in place for device tree. Then the properties for specific
video hardware would inherit from the properties for simplefb.
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Grant Likely @ 2014-08-13 19:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKON4OwLOAq1EOzZmuefcnZ4hw0i35e3k0F6XGNRy-5jer2vyA@mail.gmail.com>
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.
> The switch over from simple to KMS should not be done via a node
> add/del to the device tree either. No one has removed the device from
> the system, the device tree should not be changing.
The simple-framebuffer binding appears to be insufficient in this
regard in that it doesn't have any linkage with the actual device
providing the framebuffer. Ideally, I would put the simple framebuffer
state directly into the video device node and use the chosen node to
point to the stdout device (probably with the stdout-path property).
Then the driver already knows it can just ignore the simple properties
because it owns the device node when it binds.
That said, simple-framebuffer as it stands is in use so we're not
going to deprecate it. I would like to see an addition that specifies
how a controller can be associated with a simple framebuffer node.
BTW, Is anyone currently using the simple framebuffer for early
console? For early console we would want to start using it well before
setting up platform devices.
g.
^ permalink raw reply
* Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Luc Verhaegen @ 2014-08-13 19:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACxGe6szX3LyhGHbVHrEFfaWGYUcgcxgkuhfc9ac-R0qJs-HXw@mail.gmail.com>
On Wed, Aug 13, 2014 at 08:14:37PM +0100, Grant Likely wrote:
>
> BTW, Is anyone currently using the simple framebuffer for early
> console? For early console we would want to start using it well before
> setting up platform devices.
The code that sets up simplefb for sunxi is primarily for providing a
console in u-boot (using the ancient cfbconsole infrastructure). From
what i can tell, the u-boot code for rpi is about showing a splash
screen (using the much newer lcd infrastructure).
With u-boot showing a console, it really seemed only a small step to add
simplefb. And quite a few people in our sunxi community are interested
in it, primarily for u-boot and early console, and only secondarily as a
stop-gap for a full driver.
Luc Verhaegen.
^ permalink raw reply
* Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Stephen Warren @ 2014-08-13 19:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140813192506.GH31326@skynet.be>
On 08/13/2014 01:25 PM, Luc Verhaegen wrote:
> On Wed, Aug 13, 2014 at 08:14:37PM +0100, Grant Likely wrote:
>>
>> BTW, Is anyone currently using the simple framebuffer for early
>> console? For early console we would want to start using it well before
>> setting up platform devices.
>
> The code that sets up simplefb for sunxi is primarily for providing a
> console in u-boot (using the ancient cfbconsole infrastructure). From
> what i can tell, the u-boot code for rpi is about showing a splash
> screen (using the much newer lcd infrastructure).
There's no splash screen on the Pi in the upstream U-Boot code. The LCD
displays the U-Boot console/stdout. If USB support for the Pi's USB host
is ever sent upstream, that will allow the user to use USB/LCD for
U-Boot control, rather than serial.
^ permalink raw reply
* Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Grant Likely @ 2014-08-13 20:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140813192506.GH31326@skynet.be>
On Wed, Aug 13, 2014 at 8:25 PM, Luc Verhaegen <libv@skynet.be> wrote:
> On Wed, Aug 13, 2014 at 08:14:37PM +0100, Grant Likely wrote:
>>
>> BTW, Is anyone currently using the simple framebuffer for early
>> console? For early console we would want to start using it well before
>> setting up platform devices.
>
> The code that sets up simplefb for sunxi is primarily for providing a
> console in u-boot (using the ancient cfbconsole infrastructure). From
> what i can tell, the u-boot code for rpi is about showing a splash
> screen (using the much newer lcd infrastructure).
>
> With u-boot showing a console, it really seemed only a small step to add
> simplefb. And quite a few people in our sunxi community are interested
> in it, primarily for u-boot and early console, and only secondarily as a
> stop-gap for a full driver.
Both of which make sense and should be supported, so I agree we need
to find a solution.
The problem I think comes down to the handoff mechanism. There are a
lot of different video controllers which could all be configured for a
simple framebuffer.
I don't think the "run this only once" test is the right approach.
Sometimes the simple framebuffer will never be torn down. It is
conceivable that a simple framebuffer will get /added/ at runtime with
an overlay, or even rebound to the driver. The simple framebuffer
driver really needs to be explicitly told from outside itself that it
is being taken over since it doesn't actually have the information to
know when a framebuffer becomes invalid. Only the real video driver
can provide that information.
How does the sunxi driver currently take over from the simplefb?
g.
^ permalink raw reply
* Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Luc Verhaegen @ 2014-08-13 20:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53EBC36C.7050800@wwwdotorg.org>
On Wed, Aug 13, 2014 at 01:58:36PM -0600, Stephen Warren wrote:
> On 08/13/2014 01:25 PM, Luc Verhaegen wrote:
>>
>> The code that sets up simplefb for sunxi is primarily for providing a
>> console in u-boot (using the ancient cfbconsole infrastructure). From
>> what i can tell, the u-boot code for rpi is about showing a splash
>> screen (using the much newer lcd infrastructure).
>
> There's no splash screen on the Pi in the upstream U-Boot code. The LCD
> displays the U-Boot console/stdout. If USB support for the Pi's USB host
> is ever sent upstream, that will allow the user to use USB/LCD for
> U-Boot control, rather than serial.
Ah ok, i hadn't immediately found a tie with the lcd code and cfbconsole
code, so apparently i didn't dig down far enough.
Luc Verhaegen.
^ permalink raw reply
* Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Luc Verhaegen @ 2014-08-13 20:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACxGe6t3+7cqgOSjof60oCO_QD65Qx_cqtMSnSgU0jttyuK9tg@mail.gmail.com>
On Wed, Aug 13, 2014 at 09:00:07PM +0100, Grant Likely wrote:
>
> Both of which make sense and should be supported, so I agree we need
> to find a solution.
>
> The problem I think comes down to the handoff mechanism. There are a
> lot of different video controllers which could all be configured for a
> simple framebuffer.
>
> I don't think the "run this only once" test is the right approach.
> Sometimes the simple framebuffer will never be torn down. It is
> conceivable that a simple framebuffer will get /added/ at runtime with
> an overlay, or even rebound to the driver. The simple framebuffer
> driver really needs to be explicitly told from outside itself that it
> is being taken over since it doesn't actually have the information to
> know when a framebuffer becomes invalid. Only the real video driver
> can provide that information.
There really are two separate issues here:
1) making sure that nothing tries to use the freshly died simplefb
again.
2) doing a clean handover to another, usually hw specific, driver.
1 is what i hoped to superficially solve with this patch.
2 can be very smart about things as the replacement driver actually
should know the hw.
I kind of like the idea of an "only-once" (there must be a better name
for this) property. This allows the driver/device infrastructure to
handle things much more cleanly, and allows me to state this from u-boot
for sunxi, while Stephen wouldn't need to for rpi. I currently don't
know how or where this could be handled, i just know that this smells
like a decent solution which could solve my concern while at least
avoiding the probe issue you mentioned earlier.
> How does the sunxi driver currently take over from the simplefb?
Not at all. A big blob of KMS code exists for our sunxi-3.4 kernel,
where i can load the original display driver quickly and easily. This
tree predates most of DT. I was rather hoping that the simplefb stop-gap
didn't require me to have fully engineered everything yesterday already.
Luc Verhaegen.
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: jonsmirl @ 2014-08-13 20:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACxGe6szX3LyhGHbVHrEFfaWGYUcgcxgkuhfc9ac-R0qJs-HXw@mail.gmail.com>
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.
>
>> The switch over from simple to KMS should not be done via a node
>> add/del to the device tree either. No one has removed the device from
>> the system, the device tree should not be changing.
>
> The simple-framebuffer binding appears to be insufficient in this
> regard in that it doesn't have any linkage with the actual device
> providing the framebuffer. Ideally, I would put the simple framebuffer
> state directly into the video device node and use the chosen node to
> point to the stdout device (probably with the stdout-path property).
> Then the driver already knows it can just ignore the simple properties
> because it owns the device node when it binds.
>
> That said, simple-framebuffer as it stands is in use so we're not
> going to deprecate it. I would like to see an addition that specifies
> how a controller can be associated with a simple framebuffer node.
>
> BTW, Is anyone currently using the simple framebuffer for early
> console? For early console we would want to start using it well before
> setting up platform devices.
>
> g.
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [PATCH v3 00/14] Rework OMAP4+ HDMI audio support
From: Jyri Sarha @ 2014-08-14 9:28 UTC (permalink / raw)
To: Mark Brown
Cc: alsa-devel, linux-fbdev, linux-omap, peter.ujfalusi,
liam.r.girdwood, tomi.valkeinen, detheridge
In-Reply-To: <20140813172632.GE17528@sirena.org.uk>
On 08/13/2014 08:26 PM, Mark Brown wrote:
> On Wed, Aug 13, 2014 at 03:51:54PM +0300, Jyri Sarha wrote:
>
>> I guess then the least bad alternative would be cutting the cpu-dai driver
>> away from drivers/video/fbdev/omap2/dss/hdmi_audio.c and placing it under
>> sound/soc/omap and registering it from hdmi_audio.c the same way as
>> hdmi-audio-codec and asoc-simple-card is now registered.
>
>> There would be two devices for a single ip, but at least the ASoC side
>> driver would receive its resources directly from the HDMI video driver with
>> necessary tools to synchronize the access.
>
> Just a MFD really.
>
>> This approach would work for other HDMI audio situation too, where there
>> would usually (for tda998x and SiI9022 at least) be a need to register an
>> i2s codec driver associated with the HDMI encoder. It would probably be
>> possible to make a single codec driver to work with multiple HDMI encoders
>> if the API between the codec and endcoder drivers is designed that in mind.
>
>> How does this sound?
>
> This sounds like a way forward, I definitely think it's a good idea to
> standardise the interfaces as much as we can.
>
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.
Best regards,
Jyri
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Hans de Goede @ 2014-08-14 9:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140813170106.GT15297@lukather>
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.
Regards,
Hans
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 3/4] simplefb: disable dt node upon remove
From: Grant Likely @ 2014-08-14 10:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKON4OzQbuaQsbTCDXuaqxLRxvat_5+wmf71S-rWXdfiT1gkvA@mail.gmail.com>
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.
g.
^ permalink raw reply
* Re: [PATCH 2/4] simplefb: add goto error path to probe
From: Luc Verhaegen @ 2014-08-14 10:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CANq1E4Tsi6-a5wL8=SFZuX=+K68txAzp-WBjLSM+ZhW+k4tMfg@mail.gmail.com>
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.
Luc Verhaegen.
^ permalink raw reply
* Re: [linux-sunxi] [PATCH 4/4] simplefb: add clock handling code
From: Koen Kooi @ 2014-08-14 10:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53EC834F.6020307@redhat.com>
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 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
^ permalink raw reply
* 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
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