* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Russell King - ARM Linux @ 2016-02-23 12:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAFEAcA9U3V04o2_93bS5G7+k=UT7kTWt4=v7t1Ju0X9W=5oo9A@mail.gmail.com>
On Tue, Feb 23, 2016 at 11:56:34AM +0000, Peter Maydell wrote:
> On 23 February 2016 at 09:58, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > I'm not quite sure how it works if, as in versatile display case, there
> > are multiple DT overlays of which one has to be enabled. I hope there's
> > support to choose which one to use via kernel cmdline or similar.
> >
> > I would personally like it much more if the bootloader would either
> > compose a final dtb from DT fragments and pass it to the kernel, or
> > alternatively the bootloader would pass the base dtb image and a bunch
> > of DT overlays to the kernel, and the kernel would deal with the DT
> > overlays.
>
> Speaking as somebody who's written the "bootloader" code that's
> used for what I guess are the majority of versatile kernel boots,
> i.e. the one in QEMU, I think that requiring the bootloader to do this
> would be a significant worsening from the current state.
>
> Right now the bootloader doesn't need to do much at all with device
> trees, except pass the kernel the DT that the user gave us, which
> is just the kernel's own data structures in a separate file for
> convenience. You need to do some very minor tweaks to the /chosen
> node, but these can be handled the same way for any board and aren't
> hardware specific. There's no need to worry about dt fragments
> either for the bootloader or for the user. Imposing a new requirement
> for the bootloader to have to probe hardware which it otherwise
> has no need to even care about, and then edit and update the DT
> in a board-specific manner, or have board-specific DT fragments,
> seems like a totally unnecessary imposition on both bootloader
> authors and end-users, and of course it would break booting newer
> kernels on the great mass of already existing boot loaders and
> QEMU installs.
>
> The kernel is in a position to probe the display hardware and determine
> what is there, and do the right thing, and that's exactly what it
> does today. The kernel should continue to do this.
> The advantage of DT is that it allows moving information about
> non-probeable hardware that was previously hardwired in the kernel
> C sources into a separate data structure, but the versatile displays
> are not non-probeable. I can see no benefit at all from hardwiring into
> the dt something which the kernel has previously been successfully
> dynamically getting right without any bootloader intervention -- it just
> makes the kernel less flexible and less user-friendly.
+1.
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Tomi Valkeinen @ 2016-02-23 12:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAFEAcA9U3V04o2_93bS5G7+k=UT7kTWt4=v7t1Ju0X9W=5oo9A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4243 bytes --]
On 23/02/16 13:56, Peter Maydell wrote:
> On 23 February 2016 at 09:58, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> I'm not quite sure how it works if, as in versatile display case, there
>> are multiple DT overlays of which one has to be enabled. I hope there's
>> support to choose which one to use via kernel cmdline or similar.
>>
>> I would personally like it much more if the bootloader would either
>> compose a final dtb from DT fragments and pass it to the kernel, or
>> alternatively the bootloader would pass the base dtb image and a bunch
>> of DT overlays to the kernel, and the kernel would deal with the DT
>> overlays.
>
> Speaking as somebody who's written the "bootloader" code that's
> used for what I guess are the majority of versatile kernel boots,
> i.e. the one in QEMU, I think that requiring the bootloader to do this
> would be a significant worsening from the current state.
>
> Right now the bootloader doesn't need to do much at all with device
> trees, except pass the kernel the DT that the user gave us, which
> is just the kernel's own data structures in a separate file for
> convenience. You need to do some very minor tweaks to the /chosen
> node, but these can be handled the same way for any board and aren't
> hardware specific. There's no need to worry about dt fragments
> either for the bootloader or for the user. Imposing a new requirement
> for the bootloader to have to probe hardware which it otherwise
> has no need to even care about, and then edit and update the DT
> in a board-specific manner, or have board-specific DT fragments,
> seems like a totally unnecessary imposition on both bootloader
> authors and end-users, and of course it would break booting newer
> kernels on the great mass of already existing boot loaders and
> QEMU installs.
I'm looking for a good generic solution for going forward that can be
used on new boards, for both non-probeable and probeable displays.
When we have that solution, we can see if and how the solution could be
used for current boards. I'm sure that for some boards we need to
support whatever legacy methods are out there already.
> The kernel is in a position to probe the display hardware and determine
> what is there, and do the right thing, and that's exactly what it
> does today. The kernel should continue to do this.
> The advantage of DT is that it allows moving information about
> non-probeable hardware that was previously hardwired in the kernel
> C sources into a separate data structure, but the versatile displays
> are not non-probeable. I can see no benefit at all from hardwiring into
> the dt something which the kernel has previously been successfully
> dynamically getting right without any bootloader intervention -- it just
> makes the kernel less flexible and less user-friendly.
My opinion is that the bootloader should be responsible for telling the
kernel what hardware there is on the board. For busses like PCI we have
proper probing mechanism with global unique identifiers for the devices,
and nothing is needed from the bootloader.
In the Versatile case the panels are kind of probeable, but not in the
same sense as PCI: all that can be probed on Versatile is a board
specific ID, which in itself doesn't tell what kind of panel there is.
In addition to the ID we need board specific tables listing the details
of the panels.
So, true, there's probing going on, but it's all board specific,
requiring a board specific driver to support it in the kernel. And I
think that makes the bootloader much better place for supporting it.
But, again, for legacy reasons that may not be possible.
Now, _if_ the Versatile panels were hotpluggable, and it would be a
normal use case to switch the panels at runtime and having the kernel
automatically switch to the correct video mode, we would obviously need
a kernel driver for it. But afaik that's not the case.
I think one of the core questions here is: do we want to start adding
board specific drivers to the kernel, instead of dealing with it in the
bootloader when possible? My understanding is that we've been trying to
reduce board specific code from the kernel.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* [PATCH] fbdev: n411: check return value
From: Sudip Mukherjee @ 2016-02-23 12:56 UTC (permalink / raw)
To: Jean-Christophe Plagniol-Villard, Tomi Valkeinen
Cc: linux-kernel, linux-fbdev, Sudip Mukherjee
We were not checking the return value of platform_device_add_data()
which can fail.
Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
---
drivers/video/fbdev/n411.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/n411.c b/drivers/video/fbdev/n411.c
index 935830f..053deac 100644
--- a/drivers/video/fbdev/n411.c
+++ b/drivers/video/fbdev/n411.c
@@ -165,16 +165,22 @@ static int __init n411_init(void)
if (!n411_device)
return -ENOMEM;
- platform_device_add_data(n411_device, &n411_board, sizeof(n411_board));
+ ret = platform_device_add_data(n411_device, &n411_board,
+ sizeof(n411_board));
+ if (ret)
+ goto put_plat_device;
/* this _add binds hecubafb to n411. hecubafb refcounts n411 */
ret = platform_device_add(n411_device);
if (ret)
- platform_device_put(n411_device);
+ goto put_plat_device;
- return ret;
+ return 0;
+put_plat_device:
+ platform_device_put(n411_device);
+ return ret;
}
static void __exit n411_exit(void)
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Tomi Valkeinen @ 2016-02-23 13:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <6859810.tb1t0IEONi@wuerfel>
[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]
On 23/02/16 13:22, Arnd Bergmann wrote:
> On Tuesday 23 February 2016 11:10:33 Linus Walleij wrote:
>>
>> ERROR (duplicate_label): ERROR (duplicate_label): Duplicate label
>> 'clcd_panel' on /panel@1/port/endpoint and /panel@0/port/endpoint
>> Duplicate label 'clcd_panel' on /panel@1/port/endpoint and
>> /panel@0/port/endpoint
>>
>> So the dtc does not allow duplicate node labels for the phandle
>> even if the node it resides in is disabled. Tough luck...
>
> Ah, too bad. I guess we could still do it by also setting the
> remote-endpoint property to node->phandle, but then it's
> not as simple any more.
I think it could be done with all panels in an endpoint of their own, as
it is currently in this patch. All the panels would be disabled by
default, and one of them gets enabled at some early boot phase. The
fbdev driver would then iterate the endpoints and pick the first panel
that's enabled.
That would be almost the same as what's already in this patch, except
(if I'm not mistaken) the detection part could be in platform code, and
the fbdev driver itself would know nothing about board specific
detection or board specific panel lists.
So maybe that would be a bit cleaner. Still ugly, I think =). I really
don't like having possible-panels in the Schrödinger's DT data
(http://www.angryflower.com/387.html).
That said, maybe this is the best way to deal with Versatile, without
requiring any change to the bootloader or the boot mechanism.
What is the current status of Versatile? Have we had working display
with Versatile when booting with device tree? Or has the display been
supported only with legacy boot?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Linus Walleij @ 2016-02-23 13:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAFEAcA9U3V04o2_93bS5G7+k=UT7kTWt4=v7t1Ju0X9W=5oo9A@mail.gmail.com>
On Tue, Feb 23, 2016 at 12:56 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 23 February 2016 at 09:58, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> I'm not quite sure how it works if, as in versatile display case, there
>> are multiple DT overlays of which one has to be enabled. I hope there's
>> support to choose which one to use via kernel cmdline or similar.
>>
>> I would personally like it much more if the bootloader would either
>> compose a final dtb from DT fragments and pass it to the kernel, or
>> alternatively the bootloader would pass the base dtb image and a bunch
>> of DT overlays to the kernel, and the kernel would deal with the DT
>> overlays.
>
> Speaking as somebody who's written the "bootloader" code that's
> used for what I guess are the majority of versatile kernel boots,
> i.e. the one in QEMU, I think that requiring the bootloader to do this
> would be a significant worsening from the current state.
>
> Right now the bootloader doesn't need to do much at all with device
> trees, except pass the kernel the DT that the user gave us, which
> is just the kernel's own data structures in a separate file for
> convenience. You need to do some very minor tweaks to the /chosen
> node, but these can be handled the same way for any board and aren't
> hardware specific. There's no need to worry about dt fragments
> either for the bootloader or for the user. Imposing a new requirement
> for the bootloader to have to probe hardware which it otherwise
> has no need to even care about, and then edit and update the DT
> in a board-specific manner, or have board-specific DT fragments,
> seems like a totally unnecessary imposition on both bootloader
> authors and end-users, and of course it would break booting newer
> kernels on the great mass of already existing boot loaders and
> QEMU installs.
>
> The kernel is in a position to probe the display hardware and determine
> what is there, and do the right thing, and that's exactly what it
> does today. The kernel should continue to do this.
> The advantage of DT is that it allows moving information about
> non-probeable hardware that was previously hardwired in the kernel
> C sources into a separate data structure, but the versatile displays
> are not non-probeable. I can see no benefit at all from hardwiring into
> the dt something which the kernel has previously been successfully
> dynamically getting right without any bootloader intervention -- it just
> makes the kernel less flexible and less user-friendly.
I agree with Peter and Russell on this, as you could guess.
Today the kernel knows about all the hardware and it
JustWorks(TM) and that is so neat.
I'm still trying to appeal to the subsystem maintainer as well.
Pretty tough deal.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Linus Walleij @ 2016-02-23 13:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <56CC57FC.20707@ti.com>
On Tue, Feb 23, 2016 at 2:00 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> That would be almost the same as what's already in this patch, except
> (if I'm not mistaken) the detection part could be in platform code, and
> the fbdev driver itself would know nothing about board specific
> detection or board specific panel lists.
In this patch set the board/platform-specific plugins are separated
out adding the opaque .board_init() and .panel_init() to the driver.
The platform-specific code is in completely separate files this way,
and the CLCD driver itself just handles various versions of that
IP block.
> So maybe that would be a bit cleaner. Still ugly, I think =). I really
> don't like having possible-panels in the Schrödinger's DT data
> (http://www.angryflower.com/387.html).
OK I will focus my work on the DT-augment code instead.
> That said, maybe this is the best way to deal with Versatile, without
> requiring any change to the bootloader or the boot mechanism.
Depends on if the OF core maintainers will accept my patches to
dynamically alter DT properties at runtime. If I can't do that then
I have to go back to the Schrödinger patch.
> What is the current status of Versatile? Have we had working display
> with Versatile when booting with device tree? Or has the display been
> supported only with legacy boot?
Versatile is DT only as of kernel v4.5. The DT boot uses AUXDATA
which is the Frankenstein solution, bolting on a boardfile piece
to the DT boot and ignoring the existing panel bindings, and of
course standing in the way of cleaning things up.
IMO Schrödinger > Frankenstein.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Tomi Valkeinen @ 2016-02-23 13:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACRpkdYNaNtQQJ-tsBe5ksa4-z380ZQj-zN8ZnK0ED2kNPUwvg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3124 bytes --]
On 23/02/16 15:16, Linus Walleij wrote:
> On Tue, Feb 23, 2016 at 2:00 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>> That would be almost the same as what's already in this patch, except
>> (if I'm not mistaken) the detection part could be in platform code, and
>> the fbdev driver itself would know nothing about board specific
>> detection or board specific panel lists.
>
> In this patch set the board/platform-specific plugins are separated
> out adding the opaque .board_init() and .panel_init() to the driver.
> The platform-specific code is in completely separate files this way,
> and the CLCD driver itself just handles various versions of that
> IP block.
I see. Yes, it's in separate files but still part of the fbdev driver.
One thing to mention is that I'm looking at this maybe from a slightly
different perspective.
TI makes SoCs which may be used on a lot of different boards from
different vendors. I will not allow any solution on TI display subsystem
that would contain any board specific data, as that would quickly expand
to an unmaintainable mess. All the display data has to come from the
bootloader.
Maybe Versatile is different. If CLCD is only used on that board, or a
small family of boards, from one vendor, I guess it is maintainable to
have board specific driver parts for CLCD. But if CLCD can be used by
many vendors in many different boards, I'd steer clear of board specific
driver code.
>> So maybe that would be a bit cleaner. Still ugly, I think =). I really
>> don't like having possible-panels in the Schrödinger's DT data
>> (http://www.angryflower.com/387.html).
>
> OK I will focus my work on the DT-augment code instead.
Yeah, I don't know if that will fly either, so I think it's better to
see if these discussion go somewhere first.
>> That said, maybe this is the best way to deal with Versatile, without
>> requiring any change to the bootloader or the boot mechanism.
>
> Depends on if the OF core maintainers will accept my patches to
> dynamically alter DT properties at runtime. If I can't do that then
> I have to go back to the Schrödinger patch.
>
>> What is the current status of Versatile? Have we had working display
>> with Versatile when booting with device tree? Or has the display been
>> supported only with legacy boot?
>
> Versatile is DT only as of kernel v4.5. The DT boot uses AUXDATA
> which is the Frankenstein solution, bolting on a boardfile piece
> to the DT boot and ignoring the existing panel bindings, and of
> course standing in the way of cleaning things up.
Ok. I feel everyone is trying to push the ugly part out of their domain.
I want the board specific hacks out of fbdev. Bootloader people don't
want it there. arch/arm/ people don't want it there. =)
So what you're saying is that Versatile boots now with DT, and supports
display without any panel info in the DT data? If so, it means that when
you add panel data to the .dts, the old .dts will not support display
anymore, except if you leave all the current boardfile stuff there.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Tom Rini @ 2016-02-23 13:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160223120101.GJ19428@n2100.arm.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2759 bytes --]
On Tue, Feb 23, 2016 at 12:01:01PM +0000, Russell King - ARM Linux wrote:
> On Tue, Feb 23, 2016 at 11:56:34AM +0000, Peter Maydell wrote:
> > On 23 February 2016 at 09:58, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > > I'm not quite sure how it works if, as in versatile display case, there
> > > are multiple DT overlays of which one has to be enabled. I hope there's
> > > support to choose which one to use via kernel cmdline or similar.
> > >
> > > I would personally like it much more if the bootloader would either
> > > compose a final dtb from DT fragments and pass it to the kernel, or
> > > alternatively the bootloader would pass the base dtb image and a bunch
> > > of DT overlays to the kernel, and the kernel would deal with the DT
> > > overlays.
> >
> > Speaking as somebody who's written the "bootloader" code that's
> > used for what I guess are the majority of versatile kernel boots,
> > i.e. the one in QEMU, I think that requiring the bootloader to do this
> > would be a significant worsening from the current state.
> >
> > Right now the bootloader doesn't need to do much at all with device
> > trees, except pass the kernel the DT that the user gave us, which
> > is just the kernel's own data structures in a separate file for
> > convenience. You need to do some very minor tweaks to the /chosen
> > node, but these can be handled the same way for any board and aren't
> > hardware specific. There's no need to worry about dt fragments
> > either for the bootloader or for the user. Imposing a new requirement
> > for the bootloader to have to probe hardware which it otherwise
> > has no need to even care about, and then edit and update the DT
> > in a board-specific manner, or have board-specific DT fragments,
> > seems like a totally unnecessary imposition on both bootloader
> > authors and end-users, and of course it would break booting newer
> > kernels on the great mass of already existing boot loaders and
> > QEMU installs.
> >
> > The kernel is in a position to probe the display hardware and determine
> > what is there, and do the right thing, and that's exactly what it
> > does today. The kernel should continue to do this.
> > The advantage of DT is that it allows moving information about
> > non-probeable hardware that was previously hardwired in the kernel
> > C sources into a separate data structure, but the versatile displays
> > are not non-probeable. I can see no benefit at all from hardwiring into
> > the dt something which the kernel has previously been successfully
> > dynamically getting right without any bootloader intervention -- it just
> > makes the kernel less flexible and less user-friendly.
>
> +1.
+1 from me too.
--
Tom
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Peter Maydell @ 2016-02-23 13:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <56CC546A.9070705@ti.com>
On 23 February 2016 at 12:45, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> So, true, there's probing going on, but it's all board specific,
> requiring a board specific driver to support it in the kernel. And I
> think that makes the bootloader much better place for supporting it.
This doesn't seem to me like a reason to put the requirement
in the bootloader. A huge part of the purpose of the kernel
is to support the hardware (whether that's completely generic
and probeable, like PCI, or generic but not probeable, or
completely specific to a particular board). The kernel has to
support the hardware, and just because it happens to be board
specific hardware rather than generic hardware doesn't seem to
me to imply that the kernel gets to drop part of its core purpose.
> I think one of the core questions here is: do we want to start adding
> board specific drivers to the kernel, instead of dealing with it in the
> bootloader when possible? My understanding is that we've been trying to
> reduce board specific code from the kernel.
I think there's a difference between "reduce board specific code
in the kernel by replacing it with the combination of generic
or parameterisable code in the kernel plus a kernel data structure
(DT) that supplies the parameterisation needed", and "reduce
board specific code in the kernel by forcing the bootloader to
do the kernel's job for it".
thanks
-- PMM
^ permalink raw reply
* Re: [PATCH v3 00/12] pwm: add support for atomic update
From: Thierry Reding @ 2016-02-23 14:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAD=FV=UCErW3EjYRtwt49_ztNR7cyY4U0o4R_dxdomer-Ac2Dg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8943 bytes --]
On Mon, Feb 22, 2016 at 11:15:09AM -0800, Doug Anderson wrote:
> Thierry,
>
> On Mon, Feb 22, 2016 at 9:59 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
[...]
> >> When we add a new feature then it's expected that only updated drivers
> >> will support that feature.
> >>
> >> We need to make sure that we don't regress / negatively change the
> >> behavior for anyone running non-updated drivers. ...and we should
> >> strive to require as few changes to drivers as possible. ...but if
> >> the best we can do requires changes to the PWM driver API then we will
> >> certainly have differences depending on the PWM driver.
> >
> > How so? Drivers should behave consistently, irrespective of the API. Of
> > course if you need to change behaviour of the user driver depending on
> > the availability of a certain feature, that's perfectly fine.
> >
> > Furthermore it's out of the question that changes to the API will be
> > required. That's precisely the reason why the atomic PWM proposal came
> > about. It's an attempt to solve the shortcomings of the current API for
> > cases such as Rockchip.
>
> I _think_ we're on the same page here. If there are shortcomings with
> the current API that make it impossible to implement a feature, we've
> got to change and/or add to the existing API. ...but we don't want to
> break existing users / drivers.
>
> Note that historically I remember that Linus Torvalds has stated that
> there is no stable API within the Linux kernel and that forcing the
> in-kernel API to never change was bad for software development. I
> tracked down my memory and found
> <http://lwn.net/1999/0211/a/lt-binary.html>. Linus is rabid about not
> breaking userspace, but in general there's no strong requirement to
> never change the driver API inside the kernel. That being said,
> changing the driver API causes a lot of churn, so presumably changing
> it in a backward compatible way (like adding to the API instead of
> changing it) will make things happier.
I didn't say anything about stable API. All I said is that new API
should be well-thought-out. Those are two very different things.
> >> So all we need is a new API call that lets you read the hardware
> >> values and make sure that the PWM regulator calls that before anyone
> >> calls pwm_config(). That's roughly B) above.
> >
> > Yes. I'm thinking that we should have a pwm_get_state() which retrieves
> > the current state of the PWM. For drivers that support hardware readout
> > this state should match the hardware state. For other drivers it should
> > reflect whatever was specified in DT; essentially what pwm_get_period()
> > and friends return today.
>
> Excellent, so pwm_get_period() gets the period as specified in the
> device tree (or other board config) and pwm_get_state() returns the
> hardware state. SGTM.
That's not quite what I was thinking. If hardware readout is supported
then whatever we report back should be the current hardware state unless
we're explicitly asked for something else. If we start mixing the state
and legacy APIs this way, we'll get into a situation where drivers that
support hardware readout behave differently than drivers that don't.
For example: A PWM device that's controlled by a driver that supports
hardware readout has a current period of 50000 ns and the firmware set
the period to 25000 ns. pwm_get_period() for this PWM device will return
50000 ns. If you reconfigure the PWM to generate a PWM signal with a
period of 30000 ns, pwm_get_period() would still return 50000 ns.
A driver that doesn't support hardware readout, on the contrary, would
return 50000 ns from pwm_get_period() on the first call, but after you
have reconfigured it using pwm_config() it will return the new period.
> > That way if you want to get the current voltage in the regulator-pwm
> > driver you'd simply do a pwm_get_state() and compute the voltage from
> > the period and duty cycle. If the PWM driver that you happen to use
> > doesn't support hardware readout, you'll get an initial output voltage
> > of 0, which is as good as any, really.
>
> Sounds fine to me. PWM regulator is in charge of calling
> pwm_get_state(), which can return 0 (or an error?) if driver (or
> underlying hardware) doesn't support hardware readout. PWM regulator
> is in charge of using the resulting period / duty cycle to calculate a
> percentage.
I'm not sure if pwm_get_state() should ever return an error. For drivers
that support hardware readout, the resulting state should match what's
programmed to the hardware.
But for drivers without hardware readout support pwm_get_state() still
makes sense because after a pwm_apply_state() the internal logical state
would again match hardware.
The simplest way to get rid of this would be to change the core to apply
an initial configuration on probe. However that's probably going to
break your use-case again (it would set a 0 duty cycle because it isn't
configured in DT).
To allow your use-case to work we'd need to deal with two states: the
current hardware state and the "initial" state as defined by DT.
Unfortunately the PWM specifier in DT is not a full definition, it is
more like a partial initial configuration. The problem with that, and
I think that's what Mark was originally objecting to, is that it isn't
clear when to use the "initial" state and when to use the read hardware
state. After the first pwm_apply_state() you wouldn't ever have to use
the "initial" state again, because it's the same as the current state
(modulo the duty cycle).
Also for drivers such as clk-pwm the usefulness of the "initial" state
is reduced even more, because it doesn't even need the period specified
in DT. It uses only the flags (if at all).
Perhaps to avoid this confusion a new type of object, e.g. pwm_args,
could be introduced to hold configuration arguments given in the PWM
specifier (in DT) or the PWM lookup table (in board files).
It would then be the responsibility of the users to deal with that
information in a sensible way. In (almost?) all cases I would expect
that to be to program the PWM device in the user's ->probe(). In the
case of regulator-pwm I'd expect that ->probe() would do something
along these lines (error handling excluded):
struct pwm_state state;
struct pwm_args args;
unsigned int ratio;
pwm = pwm_get(...);
pwm_get_state(pwm, &state);
pwm_get_args(pwm, &args);
ratio = (state.duty_cycle * 100) / state.period;
state.duty_cycle = (ratio * args.period) / 100;
state.period = args.period;
state.flags = args.flags;
pwm_apply_state(pwm, &state);
The ->set_voltage() implementation would then never need to care about
the PWM args, but rather do something like this:
struct pwm_state state;
unsigned int ratio;
pwm_get_state(pwm, &state);
state.duty_cycle = (ratio * state.period) / 100;
pwm_apply_state(pwm, &state);
Does that sound about right?
> I _think_ the end result of all this is just:
>
> 1. Introduce pwm_get_state() that gets hardware state. Up for debate
> if this returns 0 or ERROR if a driver doesn't implement this.
Like I said above, I don't think pwm_get_state() should ever fail. It
should simply return the current state of the PWM, which might coincide
with the hardware state (for drivers that support hardware readout) or
will be the logical state (for drivers that don't).
Note that in the above example the logical state of the PWM, in cases
where the driver doesn't support hardware readout, the duty cycle will
be assumed to be 0, so the regulator-pwm driver would at ->probe() time
disable the regulator, at which point hardware state and logical state
will coincide again.
> 2. PWM regulator calls pwm_get_state at probe time to get hardware
> state, calculates a percentage (and voltage) with this.
I don't think that's enough. If we do this, we'll keep carrying around
the mismatch between hardware state and logical state indefinitely.
> 3. PWM regulator does nothing else until it is asked to set the
> voltage, but uses the voltage calculated from #2 to satisfy any "get
> voltage" calls.
This should work out of the box in the above. The initial state would
yield a voltage of 0 if hardware readout is not supported, whereas for
drivers that support hardware readout, the proper value can be derived
from duty cycle, period and the lookup table.
> 4. When asked to set the voltage, PWM regulator uses pwm_get_period()
> and calculates a duty cycle based on that, just like it does today.
> This uses pwm_config() which includes a duty cycle and period and is
> thus "atomic".
pwm_config() isn't atomic. pwm_apply_state() would be. The difference is
that pwm_config() can't at the same time enable/disable the PWM or set
the polarity.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v3 00/12] pwm: add support for atomic update
From: Thierry Reding @ 2016-02-23 14:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160204150150.553d36df@bbrezillon>
[-- Attachment #1: Type: text/plain, Size: 4079 bytes --]
On Thu, Feb 04, 2016 at 03:01:50PM +0100, Boris Brezillon wrote:
> Hi Mark, Thierry,
>
> On Thu, 4 Feb 2016 11:02:03 +0000
> Mark Brown <broonie@kernel.org> wrote:
>
> > On Wed, Feb 03, 2016 at 11:04:20AM -0800, Doug Anderson wrote:
> >
> > > Sure. ...but you agree that somehow you need a new API call for this,
> > > right? Somehow the PWM regulator needs to be able to say that it
> > > wants the hardware state, not the initial state as specified in the
> > > device tree.
> >
> > Wouldn't the most direct way to do that be to just not specify anything
> > in the DT? If there *is* something in DT but we ignore it that's a bit
> > weird.
>
> Just adding some inputs on this specific aspect. The reason we have to
> specify a period (and, to a lesser extent, the polarity) in the DT or
> PWM lookup table is because what most PWM users want is to specify a
> dutycycle relatively to a predefined period value.
That's not quite correct. The reason why we need the information in DT
is because it can't be derived from anything else. It is board-specific
data for which there's no heuristic that will work in all cases.
> If we decide to remove those information from the DT, then you'll need
> a way to define it somewhere else, and then the is question is 'where?'.
>
> Users that really want to control their period (this could the case for
> the clk-pwm driver) could completely ignore DT/lookup-table information
> and set the period and absolute dutycycle directly.
Yes, I think clk-pwm is very special in this regard because the period
can be derived from the requested clock rate. It would be complicated to
implement DT parsing that ignores parts of the specifier in some cases
but not in others. Simply having the clk-pwm driver ignore whatever is
in the table (or perhaps bail out on periods other than 0 for example).
> Now, from what I seen, what most PWM users want to do is:
>
> pwm_set_rel_duty_scale(pwm, rel_value, scale);
> or
> rel_duty = pwm_get_rel_duty_cycle(pwm, scale);
>
> where scale depends on the precision you need for your use case (most
> of the time it's expressed in percent).
>
> So, how about providing this kind of API (this is what I proposed in
> one of my previous email)?
>
> This would not only solve our problem (say you have a period at
> boot-time that differs from the one you'll set when first applying a
> new relative duty cycle, then the resulting relative value would still
> be correct), but it would also remove a lot of boiler plate code from
> PWM users code (if you take a look at pwm-regulator, pwm-leds, pwm-fan
> and probably others, you'll see that they are all doing this conversion
> manually).
I don't think this gains us much. The above would work for pwm-regulator
and pwm-fan, in both cases it'd replace a single line with two lines and
fitting the expressions into function arguments is likely going to be
hideous. For leds-pwm this wouldn't work, because of the low-active case
that it supports.
> Now, the last blocking point is, what if the PWM driver does not
> implement HW-readout. In this case, the pwm-regulator will probably
> expose a 0V output (IIRC, dutycycle is set to 0 by default) when it's
> actually providing something else. But is this really important? I
> mean, if the user really wants to have a reliable information, then he
> will implement initial-state retrieval in its PWM controller driver.
> Alternatively, we could put a flag specifying whether the PWM chip
> supports initial state retrieval.
I reached the same conclusion in another subthread. If hardware readout
isn't supported, I think the most natural thing to do is simply use the
initial state (i.e. what's defined in DT or board files) instead. There
is an argument, I think, to be made for having users apply the initial
state at probe time to forcibly apply the logical state to hardware and
subsequently not care about the initial state anymore. For most cases
that might not even be necessary, though.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v3 00/12] pwm: add support for atomic update
From: Doug Anderson @ 2016-02-23 17:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160223143859.GA27656@ulmo>
Thierry,
On Tue, Feb 23, 2016 at 6:38 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
>> > Furthermore it's out of the question that changes to the API will be
>> > required. That's precisely the reason why the atomic PWM proposal came
>> > about. It's an attempt to solve the shortcomings of the current API for
>> > cases such as Rockchip.
>>
>> I _think_ we're on the same page here. If there are shortcomings with
>> the current API that make it impossible to implement a feature, we've
>> got to change and/or add to the existing API. ...but we don't want to
>> break existing users / drivers.
>>
>> Note that historically I remember that Linus Torvalds has stated that
>> there is no stable API within the Linux kernel and that forcing the
>> in-kernel API to never change was bad for software development. I
>> tracked down my memory and found
>> <http://lwn.net/1999/0211/a/lt-binary.html>. Linus is rabid about not
>> breaking userspace, but in general there's no strong requirement to
>> never change the driver API inside the kernel. That being said,
>> changing the driver API causes a lot of churn, so presumably changing
>> it in a backward compatible way (like adding to the API instead of
>> changing it) will make things happier.
>
> I didn't say anything about stable API. All I said is that new API
> should be well-thought-out. Those are two very different things.
I guess I just misunderstood "it's out of the question that changes to
the API will be required". In any case, I think everyone's on the
same page here, so no need to debate further. :)
>> >> So all we need is a new API call that lets you read the hardware
>> >> values and make sure that the PWM regulator calls that before anyone
>> >> calls pwm_config(). That's roughly B) above.
>> >
>> > Yes. I'm thinking that we should have a pwm_get_state() which retrieves
>> > the current state of the PWM. For drivers that support hardware readout
>> > this state should match the hardware state. For other drivers it should
>> > reflect whatever was specified in DT; essentially what pwm_get_period()
>> > and friends return today.
>>
>> Excellent, so pwm_get_period() gets the period as specified in the
>> device tree (or other board config) and pwm_get_state() returns the
>> hardware state. SGTM.
>
> That's not quite what I was thinking. If hardware readout is supported
> then whatever we report back should be the current hardware state unless
> we're explicitly asked for something else. If we start mixing the state
> and legacy APIs this way, we'll get into a situation where drivers that
> support hardware readout behave differently than drivers that don't.
>
> For example: A PWM device that's controlled by a driver that supports
> hardware readout has a current period of 50000 ns and the firmware set
> the period to 25000 ns. pwm_get_period() for this PWM device will return
> 50000 ns. If you reconfigure the PWM to generate a PWM signal with a
> period of 30000 ns, pwm_get_period() would still return 50000 ns.
>
> A driver that doesn't support hardware readout, on the contrary, would
> return 50000 ns from pwm_get_period() on the first call, but after you
> have reconfigured it using pwm_config() it will return the new period.
Ah, right! I forgot that the existing API will be updated if you've
reconfigured the period via pwm_config(). Ugh, you're right that's a
little ugly then.
So do we define it as:
pwm_get_state(): always get the hardware state w/ no caching (maybe
even pwm_get_raw_state() or pwm_get_hw_state())
pwm_get_period(): get the period of the PWM; if the PWM has not yet
been configured by software this gets the default period (possibly
specified by the device tree).
Is that OK? That is well defined and doesn't change the existing
behavior of pwm_get_period().
>> > That way if you want to get the current voltage in the regulator-pwm
>> > driver you'd simply do a pwm_get_state() and compute the voltage from
>> > the period and duty cycle. If the PWM driver that you happen to use
>> > doesn't support hardware readout, you'll get an initial output voltage
>> > of 0, which is as good as any, really.
>>
>> Sounds fine to me. PWM regulator is in charge of calling
>> pwm_get_state(), which can return 0 (or an error?) if driver (or
>> underlying hardware) doesn't support hardware readout. PWM regulator
>> is in charge of using the resulting period / duty cycle to calculate a
>> percentage.
>
> I'm not sure if pwm_get_state() should ever return an error. For drivers
> that support hardware readout, the resulting state should match what's
> programmed to the hardware.
>
> But for drivers without hardware readout support pwm_get_state() still
> makes sense because after a pwm_apply_state() the internal logical state
> would again match hardware.
I guess it depends on how you define things. With my above
definitions it seems clearest if pwm_get_state() returns an error if
hardware readout is not supported. If we call it pwm_get_hw_state()
it's even clearer that it should return an error.
> The simplest way to get rid of this would be to change the core to apply
> an initial configuration on probe. However that's probably going to
> break your use-case again (it would set a 0 duty cycle because it isn't
> configured in DT).
Right, so we can't do that.
> To allow your use-case to work we'd need to deal with two states: the
> current hardware state and the "initial" state as defined by DT.
> Unfortunately the PWM specifier in DT is not a full definition, it is
> more like a partial initial configuration. The problem with that, and
> I think that's what Mark was originally objecting to, is that it isn't
> clear when to use the "initial" state and when to use the read hardware
> state. After the first pwm_apply_state() you wouldn't ever have to use
> the "initial" state again, because it's the same as the current state
> (modulo the duty cycle).
>
> Also for drivers such as clk-pwm the usefulness of the "initial" state
> is reduced even more, because it doesn't even need the period specified
> in DT. It uses only the flags (if at all).
>
> Perhaps to avoid this confusion a new type of object, e.g. pwm_args,
> could be introduced to hold configuration arguments given in the PWM
> specifier (in DT) or the PWM lookup table (in board files).
>
> It would then be the responsibility of the users to deal with that
> information in a sensible way. In (almost?) all cases I would expect
> that to be to program the PWM device in the user's ->probe(). In the
> case of regulator-pwm I'd expect that ->probe() would do something
> along these lines (error handling excluded):
>
> struct pwm_state state;
> struct pwm_args args;
> unsigned int ratio;
>
> pwm = pwm_get(...);
>
> pwm_get_state(pwm, &state);
> pwm_get_args(pwm, &args);
>
> ratio = (state.duty_cycle * 100) / state.period;
>
> state.duty_cycle = (ratio * args.period) / 100;
> state.period = args.period;
> state.flags = args.flags;
>
> pwm_apply_state(pwm, &state);
>
> The ->set_voltage() implementation would then never need to care about
> the PWM args, but rather do something like this:
>
> struct pwm_state state;
> unsigned int ratio;
>
> pwm_get_state(pwm, &state);
>
> state.duty_cycle = (ratio * state.period) / 100;
>
> pwm_apply_state(pwm, &state);
>
> Does that sound about right?
That should work with one minor problem. If HW readout isn't
supported then pwm_get_state() in probe will presumably return 0 for
the duty cycle. That means it will change the voltage. That's in
contrast to how I think things work today where the voltage isn't
changed until the first set_voltage() call. At least the last time I
tested things get_voltage() would simply report an incorrect value
until the first set_voltage(). I think existing behavior (reporting
the wrong value) is better than new behavior (change the value at
probe).
I'm curious, though. In your proposal, how does pwm_get_period()
behave? To be backward compatible, I'd imagine that even in your
proposal we'd have the same definition as I had above:
pwm_get_period(): get the period of the PWM; if the PWM has not yet
been configured by software this gets the default period (possibly
specified by the device tree).
If you have a different definition of pwm_get_period() in your
proposal, please let me know! If my definition matches your thoughts
then I think we can actually just not touch the "set_voltage" call.
It can always use pwm_get_period() and always use pwm_config() just
like today.
...and if set_voltage() remains untouched then we can solve my probe
problem by renaming pwm_get_state() to pwm_get_hw_state() and having
it return an error if HW readout is not supported. Then we only call
pwm_get_args() / pwm_apply_state() when we support HW readout.
Thus, if HW readout:
* In probe, we read HW state (pwm_get_hw_state) and atomically adjust
(pwm_apply_state) based on arguments (pwm_get_args).
* In set_voltage we use pwm_get_period which will return the period we
applied in pwm_apply_state() and use pwm_config() to change the duty
cycle.
If no HW readout, no behavior change at all from today:
* In probe we don't do anything to change the PWM
* Upon the first set_voltage we use pwm_get_period() to get the period
as specified in DT and use pwm_config() to change the duty cycle.
That seems pretty sane to me. What do you think?
-Doug
^ permalink raw reply
* Re: [PATCH v3 00/12] pwm: add support for atomic update
From: Thierry Reding @ 2016-02-23 18:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAD=FV=UvxG3MYD4MKQJqN7W68uMjd-s+FH-e8tkrLCpS0RLmZQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 10267 bytes --]
On Tue, Feb 23, 2016 at 09:35:48AM -0800, Doug Anderson wrote:
> On Tue, Feb 23, 2016 at 6:38 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
[...]
> > That's not quite what I was thinking. If hardware readout is supported
> > then whatever we report back should be the current hardware state unless
> > we're explicitly asked for something else. If we start mixing the state
> > and legacy APIs this way, we'll get into a situation where drivers that
> > support hardware readout behave differently than drivers that don't.
> >
> > For example: A PWM device that's controlled by a driver that supports
> > hardware readout has a current period of 50000 ns and the firmware set
> > the period to 25000 ns. pwm_get_period() for this PWM device will return
> > 50000 ns. If you reconfigure the PWM to generate a PWM signal with a
> > period of 30000 ns, pwm_get_period() would still return 50000 ns.
> >
> > A driver that doesn't support hardware readout, on the contrary, would
> > return 50000 ns from pwm_get_period() on the first call, but after you
> > have reconfigured it using pwm_config() it will return the new period.
>
> Ah, right! I forgot that the existing API will be updated if you've
> reconfigured the period via pwm_config(). Ugh, you're right that's a
> little ugly then.
>
> So do we define it as:
>
> pwm_get_state(): always get the hardware state w/ no caching (maybe
> even pwm_get_raw_state() or pwm_get_hw_state())
Caching vs. no caching should be irrelevant here. Unless PWM hardware is
autonomous the current state will always match the hardware state after
the initial hardware readout.
> pwm_get_period(): get the period of the PWM; if the PWM has not yet
> been configured by software this gets the default period (possibly
> specified by the device tree).
No. I think we'll need a different construct for the period defined by
DT or board files. pwm_get_period() is the legacy API to retrieve the
"current" period, even if it was lying a little before the atomic API.
> Is that OK? That is well defined and doesn't change the existing
> behavior of pwm_get_period().
pwm_get_period() is legacy API and in order to transition to the atomic
API it should be implemented in terms of atomic API. So the goal is to
get everything internally converted to deal with states only, then wrap
the existing API around that concept. pwm_get_period() would become:
unsigned int pwm_get_period(struct pwm_device *pwm)
{
struct pwm_state state;
pwm_get_state(pwm, &state);
return state.period;
}
If we don't do that, we'll never be able to get rid of the legacy API.
> >> > That way if you want to get the current voltage in the regulator-pwm
> >> > driver you'd simply do a pwm_get_state() and compute the voltage from
> >> > the period and duty cycle. If the PWM driver that you happen to use
> >> > doesn't support hardware readout, you'll get an initial output voltage
> >> > of 0, which is as good as any, really.
> >>
> >> Sounds fine to me. PWM regulator is in charge of calling
> >> pwm_get_state(), which can return 0 (or an error?) if driver (or
> >> underlying hardware) doesn't support hardware readout. PWM regulator
> >> is in charge of using the resulting period / duty cycle to calculate a
> >> percentage.
> >
> > I'm not sure if pwm_get_state() should ever return an error. For drivers
> > that support hardware readout, the resulting state should match what's
> > programmed to the hardware.
> >
> > But for drivers without hardware readout support pwm_get_state() still
> > makes sense because after a pwm_apply_state() the internal logical state
> > would again match hardware.
>
> I guess it depends on how you define things. With my above
> definitions it seems clearest if pwm_get_state() returns an error if
> hardware readout is not supported. If we call it pwm_get_hw_state()
> it's even clearer that it should return an error.
Again, if we do this, we'll have to keep the legacy API around forever
and keep special-casing atomic vs. legacy API in every user. The goal is
to converge on the atomic API as the standard API in users so that the
legacy API can be removed when all users have been converted.
> > To allow your use-case to work we'd need to deal with two states: the
> > current hardware state and the "initial" state as defined by DT.
> > Unfortunately the PWM specifier in DT is not a full definition, it is
> > more like a partial initial configuration. The problem with that, and
> > I think that's what Mark was originally objecting to, is that it isn't
> > clear when to use the "initial" state and when to use the read hardware
> > state. After the first pwm_apply_state() you wouldn't ever have to use
> > the "initial" state again, because it's the same as the current state
> > (modulo the duty cycle).
> >
> > Also for drivers such as clk-pwm the usefulness of the "initial" state
> > is reduced even more, because it doesn't even need the period specified
> > in DT. It uses only the flags (if at all).
> >
> > Perhaps to avoid this confusion a new type of object, e.g. pwm_args,
> > could be introduced to hold configuration arguments given in the PWM
> > specifier (in DT) or the PWM lookup table (in board files).
> >
> > It would then be the responsibility of the users to deal with that
> > information in a sensible way. In (almost?) all cases I would expect
> > that to be to program the PWM device in the user's ->probe(). In the
> > case of regulator-pwm I'd expect that ->probe() would do something
> > along these lines (error handling excluded):
> >
> > struct pwm_state state;
> > struct pwm_args args;
> > unsigned int ratio;
> >
> > pwm = pwm_get(...);
> >
> > pwm_get_state(pwm, &state);
> > pwm_get_args(pwm, &args);
> >
> > ratio = (state.duty_cycle * 100) / state.period;
> >
> > state.duty_cycle = (ratio * args.period) / 100;
> > state.period = args.period;
> > state.flags = args.flags;
> >
> > pwm_apply_state(pwm, &state);
> >
> > The ->set_voltage() implementation would then never need to care about
> > the PWM args, but rather do something like this:
> >
> > struct pwm_state state;
> > unsigned int ratio;
> >
> > pwm_get_state(pwm, &state);
> >
> > state.duty_cycle = (ratio * state.period) / 100;
> >
> > pwm_apply_state(pwm, &state);
> >
> > Does that sound about right?
>
> That should work with one minor problem. If HW readout isn't
> supported then pwm_get_state() in probe will presumably return 0 for
> the duty cycle. That means it will change the voltage. That's in
> contrast to how I think things work today where the voltage isn't
> changed until the first set_voltage() call. At least the last time I
> tested things get_voltage() would simply report an incorrect value
> until the first set_voltage(). I think existing behavior (reporting
> the wrong value) is better than new behavior (change the value at
> probe).
That's exactly the point. Reporting a wrong value isn't really a good
option. Changing the voltage on boot is the only way to make the logical
state match the hardware state on boot. Chances are that if you don't
have hardware readout support you probably don't care what state your
regulator will be in.
Then again, if we don't support hardware readout, setting up the logical
state with data from DT (or board files) and defaulting the duty cycle
to 0, we end up with exactly what we had before, even with the atomic
API, right? Maybe that's okay, too.
> I'm curious, though. In your proposal, how does pwm_get_period()
> behave? To be backward compatible, I'd imagine that even in your
> proposal we'd have the same definition as I had above:
>
> pwm_get_period(): get the period of the PWM; if the PWM has not yet
> been configured by software this gets the default period (possibly
> specified by the device tree).
It would simply return the logical period of the PWM. For drivers that
support hardware readout it would always match the hardware period, but
for drivers that don't it might be wrong until state is first applied.
> If you have a different definition of pwm_get_period() in your
> proposal, please let me know! If my definition matches your thoughts
> then I think we can actually just not touch the "set_voltage" call.
> It can always use pwm_get_period() and always use pwm_config() just
> like today.
>
> ...and if set_voltage() remains untouched then we can solve my probe
> problem by renaming pwm_get_state() to pwm_get_hw_state() and having
> it return an error if HW readout is not supported. Then we only call
> pwm_get_args() / pwm_apply_state() when we support HW readout.
The problem is that we make the API clumsy to use. If we don't sync the
"initial" state (as defined by DT or board files) to hardware at any
point, then we need to add the pwm_args construct and always stick to
it. I think it weird to have to use the pwm_args.period instead of the
current period.
So we're back to square one, really. That's exactly what Mark brought up
originally.
> Thus, if HW readout:
>
> * In probe, we read HW state (pwm_get_hw_state) and atomically adjust
> (pwm_apply_state) based on arguments (pwm_get_args).
>
> * In set_voltage we use pwm_get_period which will return the period we
> applied in pwm_apply_state() and use pwm_config() to change the duty
> cycle.
>
>
> If no HW readout, no behavior change at all from today:
>
> * In probe we don't do anything to change the PWM
>
> * Upon the first set_voltage we use pwm_get_period() to get the period
> as specified in DT and use pwm_config() to change the duty cycle.
>
>
> That seems pretty sane to me. What do you think?
This has the big disadvantage of having to special case hardware readout
vs. non-hardware readout providers. I think that makes the API really
difficult to use. It requires too many details to be aware of.
I guess this boils down to whether applying the "initial" state on probe
really is problematic.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v3 00/12] pwm: add support for atomic update
From: Doug Anderson @ 2016-02-23 18:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160223181448.GA14754@ulmo.nvidia.com>
Thierry,
On Tue, Feb 23, 2016 at 10:14 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
>> pwm_get_period(): get the period of the PWM; if the PWM has not yet
>> been configured by software this gets the default period (possibly
>> specified by the device tree).
>
> No. I think we'll need a different construct for the period defined by
> DT or board files. pwm_get_period() is the legacy API to retrieve the
> "current" period, even if it was lying a little before the atomic API.
Ah, got it. I think I missed that you considered pwm_get_period()
legacy and that you eventually wanted to get rid of it. OK, then what
you say makes sense.
>> That should work with one minor problem. If HW readout isn't
>> supported then pwm_get_state() in probe will presumably return 0 for
>> the duty cycle. That means it will change the voltage. That's in
>> contrast to how I think things work today where the voltage isn't
>> changed until the first set_voltage() call. At least the last time I
>> tested things get_voltage() would simply report an incorrect value
>> until the first set_voltage(). I think existing behavior (reporting
>> the wrong value) is better than new behavior (change the value at
>> probe).
>
> That's exactly the point. Reporting a wrong value isn't really a good
> option. Changing the voltage on boot is the only way to make the logical
> state match the hardware state on boot. Chances are that if you don't
> have hardware readout support you probably don't care what state your
> regulator will be in.
>
> Then again, if we don't support hardware readout, setting up the logical
> state with data from DT (or board files) and defaulting the duty cycle
> to 0, we end up with exactly what we had before, even with the atomic
> API, right? Maybe that's okay, too.
IMHO this is a change in behavior that will break existing users.
Anyone using a PWM regulator will suddenly find their voltage changing
at bootup. Certainly today all users of the PWM regulator don't seem
to mind (apparently) the the voltage is reported incorrectly at bootup
but I bet they'd mind if the voltage suddenly started changing for
them at bootup.
It seems better to preserve existing behavior and print a warning that
the voltage will be reported incorrectly until HW Readout is
supported.
Of course, we're only talking about two real users in mainline here:
Rockchip boards and the "stih407-family". If we just fix both of
those to support HW Readout before landing the change then I'm fine
with doing what you say.
>> ...and if set_voltage() remains untouched then we can solve my probe
>> problem by renaming pwm_get_state() to pwm_get_hw_state() and having
>> it return an error if HW readout is not supported. Then we only call
>> pwm_get_args() / pwm_apply_state() when we support HW readout.
>
> The problem is that we make the API clumsy to use. If we don't sync the
> "initial" state (as defined by DT or board files) to hardware at any
> point, then we need to add the pwm_args construct and always stick to
> it. I think it weird to have to use the pwm_args.period instead of the
> current period.
>
> So we're back to square one, really. That's exactly what Mark brought up
> originally.
I had missed the part where you wanted to deprecate pwm_get_period().
Thus my points here aren't really valid.
In my mind the old API was perfectly fine (and actually quite clean /
simple to use) except in the special case of avoiding the PWM
regulator glitches. With that mindset I think my previous email make
sense. However, this is your subsystem to maintain and if you think
moving everyone to a new atomic API makes more sense then you're in
the best position to make that decision. :)
-Doug
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Russell King - ARM Linux @ 2016-02-24 10:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <56CC546A.9070705@ti.com>
On Tue, Feb 23, 2016 at 02:45:30PM +0200, Tomi Valkeinen wrote:
> My opinion is that the bootloader should be responsible for telling the
> kernel what hardware there is on the board. For busses like PCI we have
> proper probing mechanism with global unique identifiers for the devices,
> and nothing is needed from the bootloader.
Exactly, but that is _NOT_ the case here, because we're not talking
about an on-board display.
> In the Versatile case the panels are kind of probeable, but not in the
> same sense as PCI: all that can be probed on Versatile is a board
> specific ID, which in itself doesn't tell what kind of panel there is.
> In addition to the ID we need board specific tables listing the details
> of the panels.
That argument does not stack up. Just because you've plugged in a
network device does not mean that the kernel can drive it: the kernel
needs a device specific driver, which is determined by looking at the
IDs. There is no standard network driver PCI interface.
> I think one of the core questions here is: do we want to start adding
> board specific drivers to the kernel, instead of dealing with it in the
> bootloader when possible? My understanding is that we've been trying to
> reduce board specific code from the kernel.
That's not really the question, because that question assumes that it
isn't already present, which is not true. The code is already present.
The question is how to deal with this from the DT perspective.
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Russell King - ARM Linux @ 2016-02-24 10:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <56CC60C4.6040908@ti.com>
On Tue, Feb 23, 2016 at 03:38:12PM +0200, Tomi Valkeinen wrote:
> Ok. I feel everyone is trying to push the ugly part out of their domain.
> I want the board specific hacks out of fbdev. Bootloader people don't
> want it there. arch/arm/ people don't want it there. =)
I think that is really really unfair. No one is trying to push ugly
bits out of their domain - this is being driven by Linus, who is
trying to convert Versatile to DT. There is no other agenda here.
Remember, I was the one who wrote the CLCD driver, and it was written
to support the boards at the time using the best methods at the time.
Things change, people's ideas of what's acceptable change. What was
acceptable when classes of boards were separate is no longer acceptable
with single zImage.
The board specific parts of CLCD were in arch/arm for a very long time,
but that gets in the way of single zImage, and the solution adopted by
the newly interested parties has been to move them to drivers/video
to keep things working.
That's how we're here: there isn't a conspiracy as you seem to be
thinking.
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Tomi Valkeinen @ 2016-02-24 11:21 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160224104638.GL19428@n2100.arm.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2902 bytes --]
On 24/02/16 12:46, Russell King - ARM Linux wrote:
> On Tue, Feb 23, 2016 at 02:45:30PM +0200, Tomi Valkeinen wrote:
>> My opinion is that the bootloader should be responsible for telling the
>> kernel what hardware there is on the board. For busses like PCI we have
>> proper probing mechanism with global unique identifiers for the devices,
>> and nothing is needed from the bootloader.
>
> Exactly, but that is _NOT_ the case here, because we're not talking
> about an on-board display.
Ok, what is it then? I'm not familiar with the boards in question.
When does a display become an on-board display? All the panels I have
can be disconnected quite easily, but I still consider them as on-board
displays.
>> In the Versatile case the panels are kind of probeable, but not in the
>> same sense as PCI: all that can be probed on Versatile is a board
>> specific ID, which in itself doesn't tell what kind of panel there is.
>> In addition to the ID we need board specific tables listing the details
>> of the panels.
>
> That argument does not stack up. Just because you've plugged in a
> network device does not mean that the kernel can drive it: the kernel
> needs a device specific driver, which is determined by looking at the
> IDs. There is no standard network driver PCI interface.
Yes, but you can connect the network device to any board with a PCI bus
and it works. Here, if I'm not mistaken, the displays are built for this
single board, making them board specific.
So sure, someone could build boards with the same connector, allowing
you to connect the same displays. If that's the case, then CLCD should
be taken out of this picture, as the board could have something else
than CLCD as the display controller.
>> I think one of the core questions here is: do we want to start adding
>> board specific drivers to the kernel, instead of dealing with it in the
>> bootloader when possible? My understanding is that we've been trying to
>> reduce board specific code from the kernel.
>
> That's not really the question, because that question assumes that it
> isn't already present, which is not true. The code is already present.
> The question is how to deal with this from the DT perspective.
Yes. As I commented in this (or the other thread), I'm looking for a
proper generic solution which can be recommended for all new boards.
When we know what that is, we can see if and how that could fit into
Versatile's case. Versatile is not the only board with the exact same
problem.
I wouldn't be at all surprised if the final solution is to just go with
Linus' current patches for Versatile, as everything else would break
compatibility or be overly complex.
But I cannot accept that as a general solution for all similar cases
going forward, especially when moving to DRM world, that's just bad SW
design.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Russell King - ARM Linux @ 2016-02-24 11:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <56CD924F.50108@ti.com>
On Wed, Feb 24, 2016 at 01:21:51PM +0200, Tomi Valkeinen wrote:
> On 24/02/16 12:46, Russell King - ARM Linux wrote:
> > On Tue, Feb 23, 2016 at 02:45:30PM +0200, Tomi Valkeinen wrote:
> >> My opinion is that the bootloader should be responsible for telling the
> >> kernel what hardware there is on the board. For busses like PCI we have
> >> proper probing mechanism with global unique identifiers for the devices,
> >> and nothing is needed from the bootloader.
> >
> > Exactly, but that is _NOT_ the case here, because we're not talking
> > about an on-board display.
>
> Ok, what is it then? I'm not familiar with the boards in question.
>
> When does a display become an on-board display? All the panels I have
> can be disconnected quite easily, but I still consider them as on-board
> displays.
The difference to me is quite clear.
If the connector is a flexi-strip or LVDS connector designed to be
connected directly to a panel, it is not designed as a user connector,
and the display can be regarded as part of the board: the connector
probably isn't rated for a large number of mating cycles.
If the connector is a board-edge external-unit connector, then the
panel is not part of the board.
In the case of Versatile, it's the latter: the connector is situated
at the board edge, next to the serial port connectors, and is designed
to connect to an external box housing the display.
> > That argument does not stack up. Just because you've plugged in a
> > network device does not mean that the kernel can drive it: the kernel
> > needs a device specific driver, which is determined by looking at the
> > IDs. There is no standard network driver PCI interface.
>
> Yes, but you can connect the network device to any board with a PCI bus
> and it works. Here, if I'm not mistaken, the displays are built for this
> single board, making them board specific.
It only works because Linux has a rich array of network drivers supporting
all that hardware, and the appropriate network driver is bound depending
on the hardware ID of the card. If a new PCI network device comes out,
it'll more likely than not require an update to a network driver to make
it work.
The displays are not built for "this single board" but for a family of
boards: not only Versatile PB/AB, but also the Realview family of boards
too.
> But I cannot accept that as a general solution for all similar cases
> going forward, especially when moving to DRM world, that's just bad SW
> design.
I think that's a matter of personal opinion, perspective and situation.
What is good design today is not necessary good design yesterday or
tomorrow. I thought we already ascertained that earlier in this
discussion. :)
--
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Tomi Valkeinen @ 2016-02-24 11:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160224105304.GM19428@n2100.arm.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2464 bytes --]
On 24/02/16 12:53, Russell King - ARM Linux wrote:
> On Tue, Feb 23, 2016 at 03:38:12PM +0200, Tomi Valkeinen wrote:
>> Ok. I feel everyone is trying to push the ugly part out of their domain.
>> I want the board specific hacks out of fbdev. Bootloader people don't
>> want it there. arch/arm/ people don't want it there. =)
>
> I think that is really really unfair. No one is trying to push ugly
> bits out of their domain - this is being driven by Linus, who is
> trying to convert Versatile to DT. There is no other agenda here.
>
> Remember, I was the one who wrote the CLCD driver, and it was written
> to support the boards at the time using the best methods at the time.
> Things change, people's ideas of what's acceptable change. What was
> acceptable when classes of boards were separate is no longer acceptable
> with single zImage.
That's fine. I've done the same. The point here is where should we aim
for with today's kernel? What's the good solution for the future boards?
> The board specific parts of CLCD were in arch/arm for a very long time,
> but that gets in the way of single zImage, and the solution adopted by
> the newly interested parties has been to move them to drivers/video
> to keep things working.
And I'm fine with having board specific parts in drivers/video when
needed. That's the only option when we really need a driver for the
board specific parts, i.e. we need to do something with the board
specific HW at runtime.
But that's not really the case with Versatile. Correct me if I'm wrong,
but there's just one panel connected to the board at a time and the
panel cannot be changed at runtime. We need to do the board specific
panel probing once at boot time, but other than that, there's no
difference to a single on-board panel.
To me it sounds that the cleanest solution to this is that the
bootloader does the detection (it's a trivial detection, isn't it? no
complex busses need to be used?), and just passes the kernel the correct
HW setup with DT.
Again, I understand there are lots of board out there without bootloader
doing that, so we may not get there with Versatile. But if someone comes
with patches for a new board, I'd like to have a good suggestion how to
handle similar cases the best way.
> That's how we're here: there isn't a conspiracy as you seem to be
> thinking.
I wasn't exactly serious there, as the smiley tried to imply...
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Tomi Valkeinen @ 2016-02-24 11:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20160224113549.GN19428@n2100.arm.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 2310 bytes --]
On 24/02/16 13:35, Russell King - ARM Linux wrote:
> If the connector is a flexi-strip or LVDS connector designed to be
> connected directly to a panel, it is not designed as a user connector,
> and the display can be regarded as part of the board: the connector
> probably isn't rated for a large number of mating cycles.
>
> If the connector is a board-edge external-unit connector, then the
> panel is not part of the board.
Ok, I see. I presumed the display-board was attached directly to the
mainboard. Of course, we could still argue about the difference, with
the exact same pins used with an external connector and with an on-board
connector, but lets not go there.
I agree in this case the panels are external devices =).
However, I would still be interested in opinions how to implement the
exact same case, but for boards where the panels were considered
on-board panels.
> The displays are not built for "this single board" but for a family of
> boards: not only Versatile PB/AB, but also the Realview family of boards
> too.
Alright.
So, if this is to be done correctly, we need to disconnect the display
board code from the CLCD code, as they really have nothing to do with
each other.
Perhaps a panel driver which covers the display boards used here, which
does the probing and contains the video timings for the panels in question?
One could then use that panel driver with other display controllers too.
Although the probing part is perhaps difficult to make generic, but that
board specific code should still be part of the panel-board driver, not
CLCD driver.
>> But I cannot accept that as a general solution for all similar cases
>> going forward, especially when moving to DRM world, that's just bad SW
>> design.
>
> I think that's a matter of personal opinion, perspective and situation.
> What is good design today is not necessary good design yesterday or
> tomorrow. I thought we already ascertained that earlier in this
> discussion. :)
Well, I was mostly referring to combining separate devices into single
driver. CLCD and the external displays, in this case. Then again,
"complexity" is part of the SW design, and splitting the devices drivers
into independent pieces often increases complexity, so...
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Tomi Valkeinen @ 2016-02-24 12:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAFEAcA-Y7rE_9CiOphgmae3FDApRznoBS7vsVN_+9hBqvAzEEA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2336 bytes --]
On 23/02/16 15:49, Peter Maydell wrote:
> On 23 February 2016 at 12:45, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> So, true, there's probing going on, but it's all board specific,
>> requiring a board specific driver to support it in the kernel. And I
>> think that makes the bootloader much better place for supporting it.
>
> This doesn't seem to me like a reason to put the requirement
> in the bootloader. A huge part of the purpose of the kernel
> is to support the hardware (whether that's completely generic
> and probeable, like PCI, or generic but not probeable, or
> completely specific to a particular board). The kernel has to
> support the hardware, and just because it happens to be board
> specific hardware rather than generic hardware doesn't seem to
> me to imply that the kernel gets to drop part of its core purpose.
The thing here is, the kernel doesn't have to support the hardware (the
probing method). The kernel _has_ to support the display controller and
the panels, but the probing could as well be done in the bootloader. It
would work fine, and it would be a cleaner solution that what's being
proposed so far.
>> I think one of the core questions here is: do we want to start adding
>> board specific drivers to the kernel, instead of dealing with it in the
>> bootloader when possible? My understanding is that we've been trying to
>> reduce board specific code from the kernel.
>
> I think there's a difference between "reduce board specific code
> in the kernel by replacing it with the combination of generic
> or parameterisable code in the kernel plus a kernel data structure
> (DT) that supplies the parameterisation needed", and "reduce
> board specific code in the kernel by forcing the bootloader to
> do the kernel's job for it".
Perhaps my phone background affects here, but I see the vendor provided
bootloader as the place for board specific custom solutions, and then
the kernel doesn't have to deal with those if at all possible.
With an open source generic bootloader like u-boot that doesn't exactly
hold, though, as the custom solutions will still pile up in a common
project.
Anyway, as discussed in the thread, I'm fine with having a kernel driver
for this, as the display boards for Versatile are an external device.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Pantelis Antoniou @ 2016-02-24 12:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <56CD924F.50108@ti.com>
Hi Tomi,
> On Feb 24, 2016, at 13:21 , Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
[snip]
>>
>> That argument does not stack up. Just because you've plugged in a
>> network device does not mean that the kernel can drive it: the kernel
>> needs a device specific driver, which is determined by looking at the
>> IDs. There is no standard network driver PCI interface.
>
> Yes, but you can connect the network device to any board with a PCI bus
> and it works. Here, if I'm not mistaken, the displays are built for this
> single board, making them board specific.
>
> So sure, someone could build boards with the same connector, allowing
> you to connect the same displays. If that's the case, then CLCD should
> be taken out of this picture, as the board could have something else
> than CLCD as the display controller.
>
>>> I think one of the core questions here is: do we want to start adding
>>> board specific drivers to the kernel, instead of dealing with it in the
>>> bootloader when possible? My understanding is that we've been trying to
>>> reduce board specific code from the kernel.
>>
>> That's not really the question, because that question assumes that it
>> isn't already present, which is not true. The code is already present.
>> The question is how to deal with this from the DT perspective.
>
> Yes. As I commented in this (or the other thread), I'm looking for a
> proper generic solution which can be recommended for all new boards.
> When we know what that is, we can see if and how that could fit into
> Versatile's case. Versatile is not the only board with the exact same
> problem.
>
> I wouldn't be at all surprised if the final solution is to just go with
> Linus' current patches for Versatile, as everything else would break
> compatibility or be overly complex.
>
> But I cannot accept that as a general solution for all similar cases
> going forward, especially when moving to DRM world, that's just bad SW
> design.
>
IMHO DT+overlays handle all your cases just fine.
As far as I see these are the cases which we need to handle:
1) The expansion board in question has some means of identification, whether it’s an
EEPROM or a GPIO keying combination etc. In that case it is the kernel’s job to match this
id value with a dtbo firmware file and apply it. The blob is located via means of request_firmware().
2) The expansion board in question has no means of identification. In that case the bootloader’s
job is to provide user-configured means to the kernel to use the expansion board.
We talked about two basic schemes.
2.1) The kernel command line contains information about the extra overlays to apply upon booting.
Something like “applyoverlays=foo.dtbo,bar.dtbo” is sufficient. The problem with this approach is that
if the firmware files must be present in the initrd/rootfs/builtin-kernel-image. For some this is a
problem.
2.2) The base DTB provided to the kernel from the bootloader is augmented with extra DTBOs that describe
the extra hardware. The concatenation is as simple as performing a ‘$ cat base.dtb foo.dtbo bar.dtbo’, which
can be done either by the bootloader or offline. At the moment this is not being done, but it’s easy enough
to add it and other people expressed interest for it.
Am I missing something here?
> Tomi
>
Regards
— Pantelis
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Linus Walleij @ 2016-02-25 13:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <B8A82546-46C7-4A21-92D8-62418E49FBB6@konsulko.com>
On Wed, Feb 24, 2016 at 1:13 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> IMHO DT+overlays handle all your cases just fine.
>
> As far as I see these are the cases which we need to handle:
>
> 1) The expansion board in question has some means of identification, whether it’s an
> EEPROM or a GPIO keying combination etc. In that case it is the kernel’s job to match this
> id value with a dtbo firmware file and apply it. The blob is located via means of request_firmware().
Since the dawn of time the x86 people used that console to display
the early boot crawl and collect crash data. What you're suggesting
is that we can't get the console up until after the filesystems and mounts
are up so the kernel can read firmware files.
This kills of early boot graphics and getting crash logs on the fbdev
console until that has happened.
It also means there is no way to get the console up without the right
firmware files in the filesystem. I think that is really crap compared
to what we have today where the display will always come up, and
basically a regression.
I understand the stance with respect to things like add-on hardware
like a Bluetooth board or WLAN or whatnot. But the fbdev console
is just too basic, like a serial port IMO.
Sure in the ARM world we usually have a serial console, but this is
seriously breaking current practice.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Tomi Valkeinen @ 2016-02-25 13:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACRpkdZ=PR6O28NQusUEipCYwRZyuhbeAJO_ZK80+4H7Zakt=g@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 1058 bytes --]
On 25/02/16 15:43, Linus Walleij wrote:
> On Wed, Feb 24, 2016 at 1:13 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>
>> IMHO DT+overlays handle all your cases just fine.
>>
>> As far as I see these are the cases which we need to handle:
>>
>> 1) The expansion board in question has some means of identification, whether it’s an
>> EEPROM or a GPIO keying combination etc. In that case it is the kernel’s job to match this
>> id value with a dtbo firmware file and apply it. The blob is located via means of request_firmware().
>
> Since the dawn of time the x86 people used that console to display
> the early boot crawl and collect crash data. What you're suggesting
> is that we can't get the console up until after the filesystems and mounts
> are up so the kernel can read firmware files.
You can build firmware images into the kernel image. No, I don't like
that either.
The 2.2) option in Pantelis' mail can be used for 1) too, although then
the bootloader needs to know which dtbos to add.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 11/11] ARM: versatile: move CLCD configuration to device tree
From: Linus Walleij @ 2016-02-25 14:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <56CC60C4.6040908@ti.com>
On Tue, Feb 23, 2016 at 2:38 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Maybe Versatile is different. If CLCD is only used on that board, or a
> small family of boards, from one vendor, I guess it is maintainable to
> have board specific driver parts for CLCD. But if CLCD can be used by
> many vendors in many different boards, I'd steer clear of board specific
> driver code.
OK I think at this point we would say that CLCD is a legacy driver.
It is a PrimceCell (IP block) made by ARM Ltd for their reference
designs, and intended for demonstration purposes. It is used in
these:
arch/arm/mach-integrator/
arch/arm/mach-versatile/
arch/arm/mach-realview/
arch/arm/mach-vexpress/
At the last iteration of their reference designs, ARM invented a
new display driver called HDLCD, which you can find in
drivers/gpu/drm/arm in linux-next. Thus the CLCD is now legacy.
As part of signing a deal with ARM to synthesize their silicon,
vendors get copies of the IP blocks to jumpstart design work.
Sometimes they will design their own display controller, sometimes
they will take the ARM CLCD and synthesize it right off
and not innovate around it at all. That is why CLCD also
appears in:
arch/arm/configs/axm55xx_defconfig
arch/arm/configs/lpc18xx_defconfig
arch/arm/boot/dts/lpc18xx.dtsi
arch/arm/configs/lpc32xx_defconfig
arch/arm/boot/dts/lpc32xx.dtsi
arch/arm/configs/netx_defconfig
arch/arm/configs/spear3xx_defconfig
Sometimes the vendors will tweak the CLCD. St Microelectronics
did the latter, and that is why I add support for that variant as
well.
HOWEVER: the ARM Versatile is the *only* platform I have
seen of these that have plug'n'play for the display.
*All* the others
will be very happy with *ONE* display defined as panel in the
device tree, and off they go. Usually VGA. And that will look
much like arch/arm/boot/dts/vexpress-v2m.dtsi already look
like today, using "panel-dpi" to define their displays.
They and their displays may need some board-specific or SoC
specific tweaks though, just like the Nomadik. The Vexpress is
happy to be able to go without, because I guess it is hard-coded
to just use the DVI output, so no path for the signal needs to be
set up.
I add support for doing this for the Integrator and RealView in
the patch set, by grabbing a handle to the system controller
where they have a few "misc registers". However if you look at
it:
static void integrator_clcd_enable(struct clcd_fb *fb)
{
struct fb_var_screeninfo *var = &fb->fb.var;
u32 val;
dev_info(&fb->dev->dev, "enable Integrator CLCD connectors\n");
val = INTEGRATOR_CLCD_LCD_STATIC1 | INTEGRATOR_CLCD_LCD_STATIC2 |
INTEGRATOR_CLCD_LCD0_EN | INTEGRATOR_CLCD_LCD1_EN;
if (var->bits_per_pixel <= 8 ||
(var->bits_per_pixel = 16 && var->green.length = 5))
/* Pseudocolor, RGB555, BGR555 */
val |= INTEGRATOR_CLCD_LCDMUX_VGA555;
else if (fb->fb.var.bits_per_pixel <= 16)
/* truecolor RGB565 */
val |= INTEGRATOR_CLCD_LCDMUX_VGA565;
else
val = 0; /* no idea for this, don't trust the docs */
regmap_update_bits(versatile_syscon_map,
INTEGRATOR_HDR_CTRL_OFFSET,
0,
INTEGRATOR_CLCD_MASK);
}
This is stuff that is so closely tied in to the fbdev driver that even
if it is SoC-specific (and reside in arch/arm/mach-integrator etc
today) it would be hard to argument that it should not be part
of the fbdev driver: what it does is connect the lines out of the
CLCD block to the physical VGA encode chip in different ways
depending on how the pixels were set up.
Yours,
Linus Walleij
^ 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