[linux-sunxi] Re: [PATCH] drm/panel-simple: Add Vivax TPC-9150 panel

[Thierry Reding]

On Tue, Aug 17, 2021 at 10:32:01AM +0200, Nikola Pavlica wrote:
> The model and make of the LCD panel of the Vivax TPC-9150 is unknown,
> hence the panel settings that were retrieved with a FEX dump are named
> after the device NOT the actual panel.
> 
> The LCD in question is a 50 pin MISO TFT LCD panel of the resolution
> 1024x600 used by the aforementioned device.
> 
> Signed-off-by: Nikola Pavlica 
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 26 ++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c 
> b/drivers/gpu/drm/panel/panel-simple.c
> index 4e2dad314c79..97fc3c5740bb 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -4090,6 +4090,29 @@ static const struct panel_desc arm_rtsm = {
>   .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
>  };
>  
> +static const struct drm_display_mode vivax_tpc9150_panel_mode = {
> + .clock = 6,
> + .hdisplay = 1024,
> + .hsync_start = 1024 + 160,
> + .hsync_end = 1024 + 160 + 100,
> + .htotal = 1024 + 160 + 100 + 60,
> + .vdisplay = 600,
> + .vsync_start = 600 + 12,
> + .vsync_end = 600 + 12 + 10,
> + .vtotal = 600 + 12 + 10 + 13,
> +};
> +
> +static const struct panel_desc vivax_tpc9150_panel = {
> + .modes = _tpc9150_panel_mode,
> + .num_modes = 1,
> + .size = {
> + .width = 223,
> + .height = 125,
> + },
> + .bus_format = MEDIA_BUS_FMT_RGB888_1X24,
> + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> +};
> +

These were originally supposed to be alphabetically ordered by
compatible string, though it looks like at least the data structures for
ARM RTSM weren't properly ordered.

Perhaps you can prepend a patch correcting that order and then make sure
your patch keeps the alphabetical ordering, too?

>  static const struct of_device_id platform_of_match[] = {
>   {
>   .compatible = "ampire,am-1280800n3tzqw-t00h",
> @@ -4103,6 +4126,9 @@ static const struct of_device_id platform_of_match[] = {
>   }, {
>   .compatible = "arm,rtsm-display",
>   .data = _rtsm,
> + }, {
> + .compatible = "vivax,tpc9150-panel",
> + .data = _tpc9150_panel,

Also make sure to keep the alphabetical order here.

Thierry

-- 
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+unsubscr...@googlegroups.com.
To view this discussion on the web, visit 
https://groups.google.com/d/msgid/linux-sunxi/YRvJWKwfCWLSdroy%40orome.fritz.box.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH] pwm: sun4i: Avoid waiting until the next period

[Thierry Reding]

On Wed, May 12, 2021 at 11:18:24AM +0200, Emil Lenngren wrote:
> Hi Uwe,
> 
> Den ons 12 maj 2021 kl 06:41 skrev Uwe Kleine-König
> :
> >
> > Hello Emil,
> >
> > On Wed, May 12, 2021 at 02:55:26AM +0200, Emil Lenngren wrote:
> > > Well that's one way of "solving it" ;)
> > >
> > > But on what hardware do you really need to wait until one full pulse
> > > cycle ends, before a disable command takes effect?
> > >
> > > On the hardware I've tested on (GR8 and V3s), it's enough to wait at
> > > most two clock cycles in order for it to take effect before we can
> > > close the gate. And with clock cycle I mean 24 MHz divided by the
> > > prescaler. With prescaler 1, that's 84 nanoseconds. By closing the
> > > gate when the pwm should be disabled, I guess we could save some
> > > nanoampere or microampere (is this important?)
> >
> > If I understood correctly you really have to wait longer to achieve that
> > the output is inactive in the disabled state. Do you talk about the same
> > thing?
> 
> Exactly, i.e. after writing 0 to the EN bit, we don't have to wait
> until the current period ends before we can observe that the output
> signal goes to the inactive state.
> 
> Simple test:
> 
> 1. Set pwm interval to a long time like 2 seconds, and duty to 50%.
> 2. Enable clock gating.
> 3. Enable the pwm by writing 1 to the EN bit.
> 4. Observe the LED blink once per second.
> 5. Now at a random time write 0 to the EN bit in order to disable the
> pwm. Don't turn off the clock gating.
> 6. If you just look with the eye it appears the LED turns off
> immediately, regardless of when in the pulse cycle we disabled it.
> 
> Just tested the above using "devmem" on a V3s.
> 
> By using a large prescaler and testing some different prescalers, I've
> concluded that it takes at least 1 and at most 2 clock cycles before
> we can safely turn off the gate and be certain that the output pin has
> changed to disabled.
> 
> It would be good if people having other hardware could confirm this is
> correct there as well.
> 
> Please take a look at some previous material I wrote:
> https://lkml.org/lkml/2020/3/17/1158
> https://linux-sunxi.org/PWM_Controller_Register_Guide (Observed
> behaviour on GR8 from NextThing)
> https://pastebin.com/GWrhWzPJ

I'm pretty sure Alexandre at the time reported that the instantiation of
the controller that he was using required waiting for the period to
complete before the output went to the disabled state. It's possible
that this was changed in subsequent versions of the IP, so perhaps we
need to distinguish based on compatible string?

Thierry

-- 
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+unsubscr...@googlegroups.com.
To view this discussion on the web, visit 
https://groups.google.com/d/msgid/linux-sunxi/YK0onNy2r30aNw2g%40orome.fritz.box.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [RFC PATCH 1/4] pwm: sun4i: Remove redundant needs_delay

[Thierry Reding]

On Tue, Mar 17, 2020 at 04:59:03PM +0100, Pascal Roeleven wrote:
> 'needs_delay' does now always evaluate to true, so remove all
> occurrences.
> 
> Signed-off-by: Pascal Roeleven 
> ---
>  drivers/pwm/pwm-sun4i.c | 13 ++---
>  1 file changed, 2 insertions(+), 11 deletions(-)

I've applied this one since it's obviously correct. I'll hold off on the
others until it can be more broadly tested. Hopefully Maxime or Chen-Yu
can help review the remainder of this series as well.

Thierry

-- 
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+unsubscr...@googlegroups.com.
To view this discussion on the web, visit 
https://groups.google.com/d/msgid/linux-sunxi/20200330141657.GH2431644%40ulmo.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v9 0/6] Add support for H6 PWM

[Thierry Reding]

On Sun, Nov 24, 2019 at 06:29:02PM +0100, Clément Péron wrote:
> Hi,
> 
> This is a rework of Jernej's previous work[1] taking account all the
> previous remarks.
> 
> Bindings is still strict but probe in the driver are now optionnals.
> 
> If someone could confirm that the PWM is not broken, as my board
> doesn't output it.
> 
> Thanks,
> Clément
> 
> Jernej's cover:
> Allwinner H6 SoC has PWM core which is basically the same as that found
> in A20, it's just depends on additional bus clock and reset line.
> 
> This series adds support for it and extends PWM driver functionality in
> a way that it's now possible to bypass whole core and output PWM source
> clock directly as a PWM signal. This functionality is needed by AC200
> chip, which is bundled in same physical package as H6 SoC, to serve as a
> clock source of 24 MHz. AC200 clock input pin is bonded internally to
> the second PWM channel.
> 
> I would be grateful if anyone can test this patch series for any kind of
> regression on other SoCs.
> 
> [1]: https://patchwork.kernel.org/cover/11061737/
> 
> Changes in v9:
>  - print error code in error message
>  - no capital letter to keep coherency
> 
> Changes in v8:
>  - Display error return code
>  - split commit
>  - bypass is false if unsupported
>  - return instead of goto
> 
> Changes in v7:
>  - Fix indent in Yaml bindings
> 
> Changes in v6:
>  - Update git commit log
>  - Distinguish error message
> 
> Changes in v5:
>  - Move bypass calculation to pwm_calculate
>  - Split mod_clock fallback from bus_clk probe   
>  - Update comment
>  - Move my SoB after acked-by/reviewed-by
> 
> Changes in v4:
>  - item description in correct order and add a blank line
>  - use %pe for printing PTR_ERR
>  - don't print error when it's an EPROBE_DEFER
>  - change output clock bypass formula to match PWM policy
> 
> Changes in v3:
>  - Documentation update to allow one clock without name
>  - Change reset optional to shared
>  - If reset probe failed return an error
>  - Remove old clock probe
>  - Update bypass enabled formula
> 
> Changes in v2:
>  - Remove allOf in Documentation
>  - Add H6 example in Documentation
>  - Change clock name from "pwm" to "mod"
>  - Change reset quirk to optional probe
>  - Change bus_clock quirk to optional probe
>  - Add limitation comment about mod_clk_output
>  - Add quirk for mod_clk_output
>  - Change bypass formula
> 
> Clément Péron (2):
>   pwm: sun4i: Prefer "mod" clock to unnamed
>   pwm: sun4i: Always calculate params when applying new parameters
> 
> Jernej Skrabec (4):
>   pwm: sun4i: Add an optional probe for reset line
>   pwm: sun4i: Add an optional probe for bus clock
>   pwm: sun4i: Add support to output source clock directly
>   pwm: sun4i: Add support for H6 PWM
> 
>  drivers/pwm/pwm-sun4i.c | 187 +---
>  1 file changed, 156 insertions(+), 31 deletions(-)

Applied, thanks.

Thierry

-- 
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+unsubscr...@googlegroups.com.
To view this discussion on the web, visit 
https://groups.google.com/d/msgid/linux-sunxi/20200108124225.GD1993114%40ulmo.


signature.asc
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH RESEND v2 06/12] drm/sun4i: rgb: Add 1% tolerance to dclk frequency check when bridge is connected

[Thierry Reding]

On Wed, Feb 06, 2019 at 10:16:08AM +0100, Maxime Ripard wrote:
> On Tue, Feb 05, 2019 at 09:49:17AM -0800, Vasily Khoruzhick wrote:
> > On Tue, Feb 5, 2019 at 7:42 AM Maxime Ripard  
> > wrote:
> > >
> > > On Mon, Feb 04, 2019 at 10:50:17AM -0800, Vasily Khoruzhick wrote:
> > > > On Mon, Feb 4, 2019 at 8:29 AM Icenowy Zheng  wrote:
> > > > > >> IIRC, from the previous discussion, HDMI had a tolerancy 
> > > > > >> requirement
> > > > > >> in the standard. Do you know if there's such a thing for eDP? That
> > > > > >> would solve the issue for all the eDP displays at once.
> > > > > >
> > > > > >I don't have access to eDP standard - vesa.org says it's available to
> > > > > >members only.
> > > > >
> > > > > Try out to grab an old version?
> > > > >
> > > > > I remember 1.0 is open.
> > > >
> > > > I can't find anything regarding dot clock tolerance in DisplayPort
> > > > specification.
> > >
> > > I guess since the DP is a VESA spec, it's probably .5%, just like on
> > > the EDID (well, CVT).
> > 
> > Unfortunately that's not enough for Pinebook. It needs 1% for 768p
> > panel.
> 
> And that mode is stored in the EDID as a standard (or established)
> timing, or a detailed timing?
> 
> If the latter, then it should also provide the tolerancies as part of
> the panel timing description.

The simple-panel driver can, in addition to a struct drm_display_mode
take a struct display_timings to specify the modes. These allow to
define  triplets for each parameter, which
are usually found in panel datasheets.

Of course that's not going to help you much if all you have is EDID and
if that doesn't provide tolerances.

Thierry

> If the former, then what would be the advertised pixel clock and the
> one we can compute? Maybe we have a bug somewhere.
> 
> Maxime
> 
> -- 
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com


-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH RESEND v2 08/12] dt-bindings: add binding for generic eDP panel

[Thierry Reding]

On Tue, Feb 05, 2019 at 09:57:37AM +0100, Daniel Vetter wrote:
> On Mon, Feb 04, 2019 at 05:22:58PM +0100, Thierry Reding wrote:
> > On Mon, Feb 04, 2019 at 04:59:09PM +0100, Daniel Vetter wrote:
> > > On Mon, Feb 04, 2019 at 12:22:18PM +0100, Thierry Reding wrote:
> > > > On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> > > > > On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > > > > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding 
> > > > > > >  wrote:
> > > > > > > >
> > > > > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick 
> > > > > > > > wrote:
> > > > > > > > > eDP panels usually have EDID EEPROM, so there's no need to 
> > > > > > > > > define panel
> > > > > > > > > width/height or any modes/timings in dts. But this panel 
> > > > > > > > > still may have
> > > > > > > > > regulator and/or backlight.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Vasily Khoruzhick 
> > > > > > > > > ---
> > > > > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt| 
> > > > > > > > > 7 +++
> > > > > > > > >  1 file changed, 7 insertions(+)
> > > > > > > > >  create mode 100644 
> > > > > > > > > Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > > > > >
> > > > > > > > Please don't try to make panels look more generic than they 
> > > > > > > > really are.
> > > > > > > > You're going to have to provide a compatible string for your 
> > > > > > > > device that
> > > > > > > > is more specific than "panel-edp". You claim that you don't 
> > > > > > > > need any
> > > > > > > > extra information that is panel specific, but you don't know 
> > > > > > > > that now.
> > > > > > > > We have in the past thought that we didn't need things like 
> > > > > > > > prepare
> > > > > > > > delay, but then we ran into situations where we did need them.
> > > > > > > >
> > > > > > > > Just do what everybody else does. Provide a specific compatible 
> > > > > > > > string
> > > > > > > > and match on that in the panel-simple driver. Even if you can 
> > > > > > > > read all
> > > > > > > > the video timings from an EDID EEPROM, you can still provide a 
> > > > > > > > mode in
> > > > > > > > the panel descriptor to serve as a fallback if for example the 
> > > > > > > > EEPROM
> > > > > > > > is faulty on some device.
> > > > > > > 
> > > > > > > Pinebook used several 768p panels that have slightly different 
> > > > > > > timings
> > > > > > > and recent batch uses 1080p panel.
> > > > > > > 
> > > > > > > What panel descriptor should I use as fallback?
> > > > > > 
> > > > > > You don't use panel descriptors as fallback. The simple-panel driver
> > > > > > will bind to a panel device and use the corresponding descriptor. If
> > > > > > your device tree contains the correct information, the descriptor is
> > > > > > correct for the panel you have.
> > > > > > 
> > > > > > In other words you need to ensure that you have the correct panel in
> > > > > > device tree for the board that you're using. This is exactly the 
> > > > > > same
> > > > > > thing as for other devices.
> > > > > > 
> > > > > > One way to to this is to have separate device trees for each variant
> > > > > > of the board that you want to support. Another variant may be to 
> > > > > > have
> > > > > > a common device tree and then have some early firmware update the 
> > > > > > DTB
> > > > > > with the correc

[linux-sunxi] Re: [PATCH RESEND v2 08/12] dt-bindings: add binding for generic eDP panel

[Thierry Reding]

On Mon, Feb 04, 2019 at 10:27:09AM -0600, Rob Herring wrote:
> On Mon, Feb 4, 2019 at 2:24 AM Thierry Reding  
> wrote:
> >
> > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding  
> > > wrote:
> > > >
> > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > eDP panels usually have EDID EEPROM, so there's no need to define 
> > > > > panel
> > > > > width/height or any modes/timings in dts. But this panel still may 
> > > > > have
> > > > > regulator and/or backlight.
> > > > >
> > > > > Signed-off-by: Vasily Khoruzhick 
> > > > > ---
> > > > >  .../devicetree/bindings/display/panel/panel-edp.txt| 7 
> > > > > +++
> > > > >  1 file changed, 7 insertions(+)
> > > > >  create mode 100644 
> > > > > Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > >
> > > > Please don't try to make panels look more generic than they really are.
> > > > You're going to have to provide a compatible string for your device that
> > > > is more specific than "panel-edp". You claim that you don't need any
> > > > extra information that is panel specific, but you don't know that now.
> > > > We have in the past thought that we didn't need things like prepare
> > > > delay, but then we ran into situations where we did need them.
> > > >
> > > > Just do what everybody else does. Provide a specific compatible string
> > > > and match on that in the panel-simple driver. Even if you can read all
> > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > is faulty on some device.
> > >
> > > Pinebook used several 768p panels that have slightly different timings
> > > and recent batch uses 1080p panel.
> > >
> > > What panel descriptor should I use as fallback?
> >
> > You don't use panel descriptors as fallback. The simple-panel driver
> > will bind to a panel device and use the corresponding descriptor. If
> > your device tree contains the correct information, the descriptor is
> > correct for the panel you have.
> >
> > In other words you need to ensure that you have the correct panel in
> > device tree for the board that you're using. This is exactly the same
> > thing as for other devices.
> >
> > One way to to this is to have separate device trees for each variant
> > of the board that you want to support. Another variant may be to have
> > a common device tree and then have some early firmware update the DTB
> > with the correct panel information.
> 
> That can be a pain to manage especially if panels are swapped run to
> run with 2nd sources.
> 
> I think it is perfectly fine to have a generic-ish fallback as long as
> it is just that, a fallback. If the panel has quirks, then you'd
> better make sure the firmware is stuffing in the right compatibles or
> that you can update the firmware.

simple-panel would probably work if you stuck in some mostly compatible
string and provided a ddc-i2c-bus property in the device tree node. The
generic-ish fallback case could be implemented by providing a fallback
compatible string (we used to have "simple-panel", which I think would
still be adequate for this I suppose) and adding a dummy descriptor in
the driver, perhaps one with pre-defined delays that could be adjusted
to work for all cases, or they could just be 0. At least that way we'd
be explicitly documenting that we support this as a fallback.

I'm not sure how you'd want to enforce that people provide the right
compatible strings that way. Currently there's no way to make your panel
work without adding a panel driver, so people are forced to write the
DT bindings and a driver, or add support to an existing one. If we open
this backdoor, I suspect many people will just take the easy route and
rely on the fallback. And then what do we do when we get bug reports
about panels not working, or requiring some quirks. How do we go and
find out what the right compatible strings are for each board, or how to
fix things for something we don't have access to.

This all sounds like a Pandora's box to me.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH RESEND v2 08/12] dt-bindings: add binding for generic eDP panel

[Thierry Reding]

On Mon, Feb 04, 2019 at 08:11:05AM -0800, Vasily Khoruzhick wrote:
> On Mon, Feb 4, 2019 at 12:24 AM Thierry Reding  
> wrote:
> > >
> > > Pinebook used several 768p panels that have slightly different timings
> > > and recent batch uses 1080p panel.
> > >
> > > What panel descriptor should I use as fallback?
> >
> > You don't use panel descriptors as fallback. The simple-panel driver
> > will bind to a panel device and use the corresponding descriptor. If
> > your device tree contains the correct information, the descriptor is
> > correct for the panel you have.
> >
> > In other words you need to ensure that you have the correct panel in
> > device tree for the board that you're using. This is exactly the same
> > thing as for other devices.
> >
> > One way to to this is to have separate device trees for each variant
> > of the board that you want to support. Another variant may be to have
> > a common device tree and then have some early firmware update the DTB
> > with the correct panel information.
> 
> That defeats the purpose of using eDP panels. Panel can identify
> itself and report what timings it supports.
> 
> If we use separate DTBs then users will have to figure out what panel
> is installed in their hardware and use appropriate software image -
> that's something I'd like to avoid.
> 
> I can add a descriptor for something like "pinebook eDP panel" if it
> works for you, but it won't have any display timings in it.

I'd like to point you to this:


http://sietch-tagr.blogspot.com/2016/04/display-panels-are-not-special.html

it addresses some of your questions. Hopefully that will also help you
understand that this isn't only about display timings.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH RESEND v2 08/12] dt-bindings: add binding for generic eDP panel

[Thierry Reding]

On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding  
> > > wrote:
> > > >
> > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > eDP panels usually have EDID EEPROM, so there's no need to define 
> > > > > panel
> > > > > width/height or any modes/timings in dts. But this panel still may 
> > > > > have
> > > > > regulator and/or backlight.
> > > > >
> > > > > Signed-off-by: Vasily Khoruzhick 
> > > > > ---
> > > > >  .../devicetree/bindings/display/panel/panel-edp.txt| 7 
> > > > > +++
> > > > >  1 file changed, 7 insertions(+)
> > > > >  create mode 100644 
> > > > > Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > >
> > > > Please don't try to make panels look more generic than they really are.
> > > > You're going to have to provide a compatible string for your device that
> > > > is more specific than "panel-edp". You claim that you don't need any
> > > > extra information that is panel specific, but you don't know that now.
> > > > We have in the past thought that we didn't need things like prepare
> > > > delay, but then we ran into situations where we did need them.
> > > >
> > > > Just do what everybody else does. Provide a specific compatible string
> > > > and match on that in the panel-simple driver. Even if you can read all
> > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > is faulty on some device.
> > > 
> > > Pinebook used several 768p panels that have slightly different timings
> > > and recent batch uses 1080p panel.
> > > 
> > > What panel descriptor should I use as fallback?
> > 
> > You don't use panel descriptors as fallback. The simple-panel driver
> > will bind to a panel device and use the corresponding descriptor. If
> > your device tree contains the correct information, the descriptor is
> > correct for the panel you have.
> > 
> > In other words you need to ensure that you have the correct panel in
> > device tree for the board that you're using. This is exactly the same
> > thing as for other devices.
> > 
> > One way to to this is to have separate device trees for each variant
> > of the board that you want to support. Another variant may be to have
> > a common device tree and then have some early firmware update the DTB
> > with the correct panel information.
> 
> This would defeat the point of edp, which is to standardize the mess of
> panels (at least somewhat) and avoid having to change the DT/ACPI
> tables/firmware for every board you ship. Also, we do have DP quirking
> infrastructure already (using the OUI), I think if there's something that
> doesn't work then we should quirk it there.

The problem is that while the attempt may have been to standardize, it
failed. It doesn't take into account any of the details such as timing
between things like powering up the display and enabling the backlight
or similar. I don't know how you'd want to "quirk" those kinds of
requirements because they are highly panel specific.

> What does make sense though imo is if we try not to stuff the edp panel
> into panel-simple, because it's anything like a simple dumb panel. There's
> also some integration awkwardness since with this panel you need to do dp
> aux/i2c transactions to get at the information (edid alone isn't good
> enough for edp), and I'm not sure how exactly that's supposed to be
> instantiated. Maybe a special function to instantiate an edp panel, which
> takes both a DT node and the dp_aux controller would be much better,
> instead of trying to auto-match against a DT compatible string and load a
> panel driver which is almost all fake.
> 
> Or we teach dp_aux to register itself and somehow teach panel-edp how it
> can get hold of the dp_aux channel it needs.

We already do that. drm_dp_aux registers as an I2C adapter that can be
used to read EDID EEPROMs using I2C-over-AUX transactions. We already
use that on some platforms.

Also note that simple-panel already supports getting video timings from
EDID. If a DDC link is present in DT, the driver will load the modes
from EDID and use them.

> But fake mo

[linux-sunxi] Re: [PATCH RESEND v2 08/12] dt-bindings: add binding for generic eDP panel

[Thierry Reding]

On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding  
> wrote:
> >
> > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > width/height or any modes/timings in dts. But this panel still may have
> > > regulator and/or backlight.
> > >
> > > Signed-off-by: Vasily Khoruzhick 
> > > ---
> > >  .../devicetree/bindings/display/panel/panel-edp.txt| 7 +++
> > >  1 file changed, 7 insertions(+)
> > >  create mode 100644 
> > > Documentation/devicetree/bindings/display/panel/panel-edp.txt
> >
> > Please don't try to make panels look more generic than they really are.
> > You're going to have to provide a compatible string for your device that
> > is more specific than "panel-edp". You claim that you don't need any
> > extra information that is panel specific, but you don't know that now.
> > We have in the past thought that we didn't need things like prepare
> > delay, but then we ran into situations where we did need them.
> >
> > Just do what everybody else does. Provide a specific compatible string
> > and match on that in the panel-simple driver. Even if you can read all
> > the video timings from an EDID EEPROM, you can still provide a mode in
> > the panel descriptor to serve as a fallback if for example the EEPROM
> > is faulty on some device.
> 
> Pinebook used several 768p panels that have slightly different timings
> and recent batch uses 1080p panel.
> 
> What panel descriptor should I use as fallback?

You don't use panel descriptors as fallback. The simple-panel driver
will bind to a panel device and use the corresponding descriptor. If
your device tree contains the correct information, the descriptor is
correct for the panel you have.

In other words you need to ensure that you have the correct panel in
device tree for the board that you're using. This is exactly the same
thing as for other devices.

One way to to this is to have separate device trees for each variant
of the board that you want to support. Another variant may be to have
a common device tree and then have some early firmware update the DTB
with the correct panel information.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH RESEND v2 08/12] dt-bindings: add binding for generic eDP panel

[Thierry Reding]

On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> eDP panels usually have EDID EEPROM, so there's no need to define panel
> width/height or any modes/timings in dts. But this panel still may have
> regulator and/or backlight.
> 
> Signed-off-by: Vasily Khoruzhick 
> ---
>  .../devicetree/bindings/display/panel/panel-edp.txt| 7 +++
>  1 file changed, 7 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/panel/panel-edp.txt

Please don't try to make panels look more generic than they really are.
You're going to have to provide a compatible string for your device that
is more specific than "panel-edp". You claim that you don't need any
extra information that is panel specific, but you don't know that now.
We have in the past thought that we didn't need things like prepare
delay, but then we ran into situations where we did need them.

Just do what everybody else does. Provide a specific compatible string
and match on that in the panel-simple driver. Even if you can read all
the video timings from an EDID EEPROM, you can still provide a mode in
the panel descriptor to serve as a fallback if for example the EEPROM
is faulty on some device.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v3 6/8] drm/panel: simple: Add support for the LeMaker BL035-RGB-002 3.5" LCD

[Thierry Reding]

On Wed, Nov 07, 2018 at 07:18:41PM +0100, Paul Kocialkowski wrote:
> This adds support for the 3.5" LCD panel from LeMaker, sold for use with
> BananaPi boards. It comes with a 24-bit RGB888 parallel interface and
> requires an active-low DE signal
> 
> Signed-off-by: Paul Kocialkowski 
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 27 +++
>  1 file changed, 27 insertions(+)

Applied to drm-misc-next, thanks.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v3 5/8] dt-bindings: Add bindings for the LeMaker BL035-RGB-002 LCD panel

[Thierry Reding]

On Wed, Nov 07, 2018 at 07:18:40PM +0100, Paul Kocialkowski wrote:
> This adds the device-tree bindings for the LeMaker BL035-RGB-002 3.5"
> QVGA TFT LCD panel, compatible with simple-panel.
> 
> Signed-off-by: Paul Kocialkowski 
> ---
>  .../bindings/display/panel/lemaker,bl035-rgb-002.txt | 12 
>  1 file changed, 12 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/panel/lemaker,bl035-rgb-002.txt

Applied to drm-misc-next, thanks.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v3 4/8] dt-bindings: Add vendor prefix for LeMaker

[Thierry Reding]

On Wed, Nov 07, 2018 at 07:18:39PM +0100, Paul Kocialkowski wrote:
> This introduces a new device-tree binding vendor prefix for Shenzhen
> LeMaker Technology Co., Ltd.
> 
> This vendor was already in use but it was not documented until now.
> 
> Signed-off-by: Paul Kocialkowski 
> Reviewed-by: Rob Hering 
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
>  1 file changed, 1 insertion(+)

Applied to drm-misc-next, thanks.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v3 6/6] ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support.

[Thierry Reding]

On Mon, Nov 26, 2018 at 12:23:19AM +0800, Hao Zhang wrote:
> The sun8i R40/T3/V40 PWM has 8 PWM channals and divides to 4 PWM pairs,
> each PWM pair built-in 1 clock module, 2 timer logic module and 1
> programmable dead-time generator, it also support waveform capture.
> It has 2 clock sources OSC24M and APB1, it is different with the
> sun4i-pwm driver, Therefore add a new driver for it.
> 
> Signed-off-by: Hao Zhang 
> ---
>  drivers/pwm/Kconfig |  12 +-
>  drivers/pwm/Makefile|   1 +
>  drivers/pwm/pwm-sun8i.c | 418 
> 
>  3 files changed, 430 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/pwm/pwm-sun8i.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 504d252..6105ac8 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -426,7 +426,7 @@ config PWM_STMPE
> expanders.
>  
>  config PWM_SUN4I
> - tristate "Allwinner PWM support"
> + tristate "Allwinner SUN4I PWM support"
>   depends on ARCH_SUNXI || COMPILE_TEST
>   depends on HAS_IOMEM && COMMON_CLK
>   help
> @@ -435,6 +435,16 @@ config PWM_SUN4I
> To compile this driver as a module, choose M here: the module
> will be called pwm-sun4i.
>  
> +config PWM_SUN8I
> + tristate "Allwinner SUN8I (R40/V40/T3) PWM support"
> + depends on ARCH_SUNXI || COMPILE_TEST
> + depends on HAS_IOMEM && COMMON_CLK
> + help
> +   Generic PWM framework driver for Allwinner R40/V40/T3 SoCs.
> +
> +   To compile this driver as a module, choose M here: the module
> +   will be called pwm-sun8i.
> +
>  config PWM_TEGRA
>   tristate "NVIDIA Tegra PWM support"
>   depends on ARCH_TEGRA
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9c676a0..32c8d2d 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
>  obj-$(CONFIG_PWM_STM32_LP)   += pwm-stm32-lp.o
>  obj-$(CONFIG_PWM_STMPE)  += pwm-stmpe.o
>  obj-$(CONFIG_PWM_SUN4I)  += pwm-sun4i.o
> +obj-$(CONFIG_PWM_SUN8I)  += pwm-sun8i.o
>  obj-$(CONFIG_PWM_TEGRA)  += pwm-tegra.o
>  obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
>  obj-$(CONFIG_PWM_TIEHRPWM)   += pwm-tiehrpwm.o
> diff --git a/drivers/pwm/pwm-sun8i.c b/drivers/pwm/pwm-sun8i.c
> new file mode 100644
> index 000..d8597e4
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun8i.c
> @@ -0,0 +1,418 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 Hao Zhang 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define PWM_IRQ_ENABLE_REG   0x
> +#define PCIE(ch) BIT(ch)
> +
> +#define PWM_IRQ_STATUS_REG   0x0004
> +#define PIS(ch)  BIT(ch)
> +
> +#define CAPTURE_IRQ_ENABLE_REG   0x0010
> +#define CRIE(ch) BIT((ch) * 2)
> +#define CFIE(ch) BIT((ch) * 2 + 1)
> +
> +#define CAPTURE_IRQ_STATUS_REG   0x0014
> +#define CRIS(ch) BIT((ch) * 2)
> +#define CFIS(ch) BIT((ch) * 2 + 1)
> +
> +#define CLK_CFG_REG(ch)  (0x0020 + ((ch) >> 1) * 4)
> +#define CLK_SRC_SEL  GENMASK(8, 7)
> +#define CLK_SRC_BYPASS_SEC   BIT(6)
> +#define CLK_SRC_BYPASS_FIR   BIT(5)
> +#define CLK_GATING   BIT(4)
> +#define CLK_DIV_MGENMASK(3, 0)
> +
> +#define PWM_DZ_CTR_REG(ch)   (0x0030 + ((ch) >> 1) * 4)
> +#define PWM_DZ_INTV  GENMASK(15, 8)
> +#define PWM_DZ_ENBIT(0)
> +
> +#define PWM_ENABLE_REG   0x0040
> +#define PWM_EN(ch)   BIT(ch)
> +
> +#define CAPTURE_ENABLE_REG   0x0044
> +#define CAP_EN(ch)   BIT(ch)
> +
> +#define PWM_CTR_REG(ch)  (0x0060 + (ch) * 0x20)
> +#define PWM_PERIOD_RDY   BIT(11)
> +#define PWM_PUL_STARTBIT(10)
> +#define PWM_MODE BIT(9)
> +#define PWM_ACT_STA  BIT(8)
> +#define PWM_PRESCAL_KGENMASK(7, 0)
> +
> +#define PWM_PERIOD_REG(ch)   (0x0064 + (ch) * 0x20)
> +#define PWM_ENTIRE_CYCLE GENMASK(31, 16)
> +#define PWM_ACT_CYCLEGENMASK(15, 0)
> +
> +#define PWM_CNT_REG(ch)  (0x0068 + (ch) * 0x20)
> +#define PWM_CNT_VAL  GENMASK(15, 0)
> +
> +#define CAPTURE_CTR_REG(ch)  (0x006c + (ch) * 0x20)
> +#define CAPTURE_CRLF BIT(2)
> +#define CAPTURE_CFLF BIT(1)
> +#define CAPINV   BIT(0)
> +
> +#define CAPTURE_RISE_REG(ch) (0x0070 + (ch) * 0x20)
> +#define CAPTURE_CRLR GENMASK(15, 0)
> +
> +#define CAPTURE_FALL_REG(ch) (0x0074 + (ch) * 0x20)
> +#define CAPTURE_CFLR GENMASK(15, 0)
> +
> +struct sun8i_pwm_chip {
> + struct pwm_chip chip;
> + struct clk *clk;
> + void __iomem *base;
> + const struct sun8i_pwm_data *data;
> + struct regmap *regmap;
> +};
> 

[linux-sunxi] Re: [PATCH v3 1/6] Documentation: ARM: sunxi: pwm: add Allwinner sun8i.

[Thierry Reding]

On Mon, Nov 26, 2018 at 12:18:59AM +0800, Hao Zhang wrote:
> This patch adds Allwinner sun8i pwm binding document.
> 
> Signed-off-by: Hao Zhang 
> ---
>  .../devicetree/bindings/pwm/pwm-sun8i.txt  | 24 
> ++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt 
> b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
> new file mode 100644
> index 000..7531d85
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sun8i.txt
> @@ -0,0 +1,24 @@
> +Allwinner sun8i R40/V40/T3 SoC PWM controller
> +
> +Required properties:
> +  - compatible: Should be one of:
> +- "allwinner,sun8i-r40-pwm"
> +  - reg: Physical base address and length of the controller's registers
> +  - interrupts: Should contain interrupt.
> +  - clocks: From common clock binding, handle to the parent clock.
> +  - clock-names: Must contain the clock names described just above.
> +  - pwm-channels: PWM channels of the controller.

Why do you need this? In the cover letter you say:

"The sun8i R40/T3/V40 PWM has 8 PWM channals ..."

Why does this need to be specified in the DT?

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v3 4/6] DEV: CLK: add function to check the using clock name of driver.

[Thierry Reding]

On Mon, Nov 26, 2018 at 12:21:18AM +0800, Hao Zhang wrote:
> In some situation we want to check clock whether is we want
> and after the driver been probed use to change different clock source.
> 
> Signed-off-by: Hao Zhang 
> ---
>  drivers/clk/clk.c| 6 ++
>  include/linux/clk-provider.h | 1 +
>  2 files changed, 7 insertions(+)

This doesn't seem right. If you're going to change to to a different
clock source anyway, why would you even want to know what it was set
before that? Isn't that completely irrelevant?

But if you really need this, perhaps a combination of clk_get() and
clk_is_match() would be a better approach? Perhaps best to let Mike
and Stephen share their feelings about this kind of API.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index d31055a..3d2c2cd 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3466,6 +3466,12 @@ static int devm_clk_hw_match(struct device *dev, void 
> *res, void *data)
>   return hw == data;
>  }
>  
> +bool devm_clk_name_match(struct clk *clk, const char *string)
> +{
> + return match_string(>con_id, 1, string) == 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_clk_name_match);

Also, there's nothing about this that would be device-managed, so the
devm_ prefix on the function name is misleading.

>  /**
>   * devm_clk_unregister - resource managed clk_unregister()
>   * @clk: clock to unregister
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 08b1aa7..5cd2eed 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -764,6 +764,7 @@ struct clk *devm_clk_register(struct device *dev, struct 
> clk_hw *hw);
>  int __must_check clk_hw_register(struct device *dev, struct clk_hw *hw);
>  int __must_check devm_clk_hw_register(struct device *dev, struct clk_hw *hw);
>  
> +bool devm_clk_name_match(struct clk *clk, const char *string);
>  void clk_unregister(struct clk *clk);
>  void devm_clk_unregister(struct device *dev, struct clk *clk);

You use this from clock consumers, so clk-provider.h is not the right
place for it. Also, you'll want to add a dummy implementation for the
case where CONFIG_COMMON_CLK is not set.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v2 4/6] drm/panel: simple: Add support for Banana Pi 7" S070WV20-CT16 panel

[Thierry Reding]

On Fri, Sep 07, 2018 at 12:19:46PM +0800, Chen-Yu Tsai wrote:
> This panel is marketed as Banana Pi 7" LCD display. On the back is
> a sticker denoting the model name S070WV20-CT16.
> 
> This is a 7" 800x480 panel connected through a 24-bit RGB interface.
> However the panel only does 262k colors.
> 
> Depending on the variant, the PCB attached to the panel module either
> supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via
> an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel
> itself.
> 
> Signed-off-by: Chen-Yu Tsai 
> ---
>  .../display/panel/bananapi,s070wv20-ct16.txt  | 12 +
>  drivers/gpu/drm/panel/panel-simple.c  | 25 +++
>  2 files changed, 37 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt

Next time, please make the bindings and driver changes separate patches.

Applied, thanks.
Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v2 0/4] drivers: pwm: sun4i: Improve support for A64 SoCs

[Thierry Reding]

On Sun, Mar 18, 2018 at 11:28:43PM +, Andre Przywara wrote:
> A rework addressing the comments. I dropped the H6 and the reset support
> for now, to simplify merging this series.
> 
> This series adds PWM support for new Allwinner SoCs. Actually the A64 PWM 
> is fully compatible with the A13 and H3 PWM IP, so the driver does not
> need any additional code. But I use this opportunity to provide some
> cleanup.
> Patch 1 removes a no longer used parameter from our per-SoC data structure,
> to simplify patch 2, which groups SoCs with a compatible PWM controller.
> Patch 3 adds the new compatible strings to the binding documentation
> (and just there, we expect to use "allwinner,sun5i-a13-pwm" as a fallback
> compatible string).
> The final patch 4 adds the respective PWM nodes to the A64 .dtsi.
> This eventually does not enable the PWM on any new board at the moment, as
> the PWM pins are either not usable (muxed with Ethernet) or exposed on
> a header pin not dedicated to PWM. But the Pinebook (and Teres I) should be
> able to use the PWM for the LCD backlights, plus users can enable the
> R_PWM on their Pine64 boards, if they like.
> 
> Tested by manually enabling r_pwm on a Pine64-LTS.
> 
> Cheers,
> Andre.
> 
> Andre Przywara (4):
>   pwm: sun4i: drop unused .has_rdy member
>   pwm: sun4i: simplify controller mapping
>   dt-bindings: pwm: sunxi: add new compatible strings
>   dts: sunxi: A64: Add PWM controllers
> 
>  .../devicetree/bindings/pwm/pwm-sun4i.txt  |  2 ++
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi  | 28 +++
>  drivers/pwm/pwm-sun4i.c| 32 
> ++
>  3 files changed, 38 insertions(+), 24 deletions(-)

Applied patches 1-3, thanks.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Thierry Reding]

On Tue, Jan 30, 2018 at 10:59:16AM +0100, Thierry Reding wrote:
> On Tue, Jan 30, 2018 at 10:24:48AM +0100, Arnd Bergmann wrote:
> > On Tue, Jan 30, 2018 at 8:54 AM, Maxime Ripard
> > <maxime.rip...@free-electrons.com> wrote:
> > > On Mon, Jan 29, 2018 at 03:34:02PM +0100, Arnd Bergmann wrote:
> > >> On Mon, Jan 29, 2018 at 10:25 AM, Linus Walleij
> > >> <linus.wall...@linaro.org> wrote:
> > >> > On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
> > >> > <maxime.rip...@free-electrons.com> wrote:
> > >> >> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
> > 
> > >>
> > >> At one point we had discussed adding a 'dma-masters' property that
> > >> lists all the buses on which a device can be a dma master, and
> > >> the respective properties of those masters (iommu, coherency,
> > >> offset, ...).
> > >>
> > >> IIRC at the time we decided that we could live without that complexity,
> > >> but perhaps we cannot.
> > >
> > > Are you talking about this ?
> > > https://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/dma/dma.txt#L41
> > >
> > > It doesn't seem to be related to that issue to me. And in our
> > > particular cases, all the devices are DMA masters, the RAM is just
> > > mapped to another address.
> > 
> > No, that's not the one I was thinking of. The idea at the time was much
> > more generic, and not limited to dma engines. I don't recall the details,
> > but I think that Thierry was either involved or made the proposal at the
> > time.
> 
> Yeah, I vaguely remember discussing something like this before. A quick
> search through my inbox yielded these two threads, mostly related to
> IOMMU but I think there were some mentions about dma-ranges and so on as
> well. I'll have to dig deeper into those threads to refresh my memories,
> but I won't get around to it until later today.
> 
> If someone wants to read up on this in the meantime, here are the links:
> 
>   https://lkml.org/lkml/2014/4/27/346
>   
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/257200.html
> 
> From a quick glance the issue of dma-ranges was something that we hand-
> waved at the time.
> 
> Thierry

Also found this, which seems to be relevant as well:


http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/252715.html

Adding Dave.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v6 2/2] media: V3s: Add support for Allwinner CSI.

[Thierry Reding]

On Tue, Jan 30, 2018 at 10:24:48AM +0100, Arnd Bergmann wrote:
> On Tue, Jan 30, 2018 at 8:54 AM, Maxime Ripard
>  wrote:
> > On Mon, Jan 29, 2018 at 03:34:02PM +0100, Arnd Bergmann wrote:
> >> On Mon, Jan 29, 2018 at 10:25 AM, Linus Walleij
> >>  wrote:
> >> > On Mon, Jan 29, 2018 at 9:25 AM, Maxime Ripard
> >> >  wrote:
> >> >> On Sat, Jan 27, 2018 at 05:14:26PM +0100, Linus Walleij wrote:
> 
> >>
> >> At one point we had discussed adding a 'dma-masters' property that
> >> lists all the buses on which a device can be a dma master, and
> >> the respective properties of those masters (iommu, coherency,
> >> offset, ...).
> >>
> >> IIRC at the time we decided that we could live without that complexity,
> >> but perhaps we cannot.
> >
> > Are you talking about this ?
> > https://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/dma/dma.txt#L41
> >
> > It doesn't seem to be related to that issue to me. And in our
> > particular cases, all the devices are DMA masters, the RAM is just
> > mapped to another address.
> 
> No, that's not the one I was thinking of. The idea at the time was much
> more generic, and not limited to dma engines. I don't recall the details,
> but I think that Thierry was either involved or made the proposal at the
> time.

Yeah, I vaguely remember discussing something like this before. A quick
search through my inbox yielded these two threads, mostly related to
IOMMU but I think there were some mentions about dma-ranges and so on as
well. I'll have to dig deeper into those threads to refresh my memories,
but I won't get around to it until later today.

If someone wants to read up on this in the meantime, here are the links:

https://lkml.org/lkml/2014/4/27/346

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/257200.html

>From a quick glance the issue of dma-ranges was something that we hand-
waved at the time.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Thierry Reding]

On Fri, Nov 24, 2017 at 12:04:12PM +, Andre Przywara wrote:
> Hi,
> 
> (adding linux-sunxi, which I forgot at the initial post).
> 
> On 24/11/17 10:52, Thierry Reding wrote:
> > On Fri, Nov 24, 2017 at 11:19:52AM +0100, Linus Walleij wrote:
> >> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przyw...@arm.com> 
> >> wrote:
> >>
> >>> So far all the Allwinner pinctrl drivers provided a table in the
> >>> kernel to describe all the pins and the link between the pinctrl functions
> >>> names (strings) and their respective mux values (register values).
> >>>
> >>> Extend the binding to put those mappings in the DT, so that any SoC can
> >>> describe its pinctrl and GPIO data fully there instead of relying on
> >>> tables.
> >>> This uses a generic compatible name, to be prepended with an SoC
> >>> specific name in the node.
> > 
> > This seems backwards to me. I'm not sure if Rob has any hard rules on
> > this, but in the past I've seen a lot of drivers stick this kind of data
> > into drivers. I personally also prefer that approach because the data is
> > completely static and there's no way for any specific board to customize
> > it. So the tables are in fact implied completely by the SoC compatible
> > string.
> 
> But this is just *data*, and I believe it is actually package specific.
> We need the DT to describe the relation between devices and pins anyway,
> it's just that we use arbitrary strings today instead of the actual
> register value. This is what the generic pinmux property fixes.

Register values don't belong in a device tree. And it's totally fine to
have data in the kernel, too. We do it all the time.

> > Moving all of this data into device tree has a number of disadvantages:
> > 
> >   * Existing boards already use the static tables in the driver, and the
> > device trees don't contain any data, so you can't get rid of any of
> > the existing tables because it would break ABI.
> 
> Yes, my DeLorean is in the garage, so I can't really change this anymore
> ;-) But that doesn't mean we have to go on with this forever, I think.

ABI stability means that, yes, you have to keep maintaining the existing
tables forever, else old DTBs will stop working if you rip out the
static tables that drivers depend on.

> >   * Moving the table into the DT doesn't actually solve anything because
> > the driver would have to validate the DT description to make sure it
> > contains valid data. And in order to validate DT content, the driver
> > would need a copy of the table anyway.
> 
> I don't get what the driver would need to validate? We rely on DT
> information to be correct anyway, otherwise your board just won't work.
> If the DT is wrong, you have much bigger problems.

Given that DT is an ABI you should treat it the same way as other ABIs.
You can't rely on the DT being correct. Rather you have to make sure to
validate it before you hand the content to hardware. If you allow direct
register access to your hardware via DT and don't validate, it becomes
really easy for people to exploit it.

This is not the same as saying we need to be able to fully validate all
aspects of device tree. We can't, because some information simply does
not exist outside of DT. However, I think it's a big mistake to trust a
user to fully know about all intricacies of a pinmux and not make any
mistake when writing the device tree. What if one of the settings causes
the board to go up in flames?

> Actually we gain something, because we only commit information that can
> actually be tested. Right now we have a lot of information which is
> copied from the manual, and nobody knows if pin H24 on the A10 is really
> PATA-CS1 or not. Plus we have bugs when creating the table, plus
> copy bugs. I found some while grep-ing for patterns - will send
> fixes ASAP.

That's a different matter. If you've got bugs in the tables, then go fix
the tables. However the assumption here is that you've done at least a
minimum of testing and your driver didn't cause your board to go up in
flames. When patches were posted, people had the opportunity to review
the tables for correctness. However, if you put all of the flexibility
into DT, you also put all of the risk there. People may just make some
stupid mistake and cause physical damage to their hardware.

> In the moment all the table gives us is a mapping between a *string* and
> the respective mux register value (per pin), plus the number of pins in
> each bank. This can *easily* be put in the DT and should belong there.

Why? This is data that is implied by the compatible string and static
per SoC. There is no way you can change the mapping i

[linux-sunxi] Re: [PATCH] drm/panel: simple: Fix width and height for Olimex LCD-OLinuXino-4.3TS

[Thierry Reding]

On Thu, Jul 20, 2017 at 08:29:43PM +1000, Jonathan Liu wrote:
> The physical size of the panel is 105.5 (W) x 67.2 (H) x 4.05 (D) mm
> but the active display area is 95.04 (W) x 53.856 (H) mm.
> 
> The width and height should be set to the active display area.
> 
> Signed-off-by: Jonathan Liu 
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied to drm-misc-next, thanks.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH RFC] drm/sun4i: rgb: Add 5% tolerance to dot clock frequency check

[Thierry Reding]

On Fri, Feb 24, 2017 at 10:51:12AM +0100, Lucas Stach wrote:
> +CC Thierry, as the drm_panel maintainer.
> 
> Am Donnerstag, den 23.02.2017, 10:54 -0500 schrieb Sean Paul:
> > On Wed, Dec 07, 2016 at 11:48:55AM +0200, Laurent Pinchart wrote:
> > > Hello,
> > > 
> > > On Wednesday 07 Dec 2016 10:26:25 Chen-Yu Tsai wrote:
> > > > On Wed, Dec 7, 2016 at 1:29 AM, Maxime Ripard wrote:
> > > > > On Thu, Nov 24, 2016 at 07:22:31PM +0800, Chen-Yu Tsai wrote:
> > > > >> The panels shipped with Allwinner devices are very "generic", i.e.
> > > > >> they do not have model numbers or reliable sources of information
> > > > >> for the timings (that we know of) other than the fex files shipped
> > > > >> on them. The dot clock frequency provided in the fex files have all
> > > > >> been rounded to the nearest MHz, as that is the unit used in them.
> > > > >> 
> > > > >> We were using the simple panel "urt,umsh-8596md-t" as a substitute
> > > > >> for the A13 Q8 tablets in the absence of a specific model for what
> > > > >> may be many different but otherwise timing compatible panels. This
> > > > >> was usable without any visual artifacts or side effects, until the
> > > > >> dot clock rate check was added in commit bb43d40d7c83 ("drm/sun4i:
> > > > >> rgb: Validate the clock rate").
> > > > >> 
> > > > >> The reason this check fails is because the dotclock frequency for
> > > > >> this model is 33.26 MHz, which is not achievable with our dot clock
> > > > >> hardware, and the rate returned by clk_round_rate deviates slightly,
> > > > >> causing the driver to reject the display mode.
> > > > >> 
> > > > >> The LCD panels have some tolerance on the dot clock frequency, even
> > > > >> if it's not specified in their datasheets.
> > > > >> 
> > > > >> This patch adds a 5% tolerence to the dot clock check.
> > > > > 
> > > > > As we discussed already, I really believe this is just as arbitrary as
> > > > > the current behaviour.
> > > > 
> > > > Yes. I agree. This patch is mainly to give something that works for
> > > > people who don't care about the details, and to get some feedback
> > > > from people that do.
> > > > 
> > > > > Some panels require an exact frequency,
> > > 
> > > There's no such thing as an exact frequency, there will always be some 
> > > tolerance (and if your display controller can really generate an exact 
> > > frequency I'd be very interested in that hardware :-)).
> > 
> > There is such thing as exact frequency when you have to worry about panel
> > warranty. We are fortunate, since we can talk to the panel manufacturer and
> > discuss which frequencies are tolerable inside the warranty. We usually hand
> > pick a rate/mode within these constraints.
> > 
> > Also, even though things *look* ok on the panel at a certain clock rate, 
> > running
> > outside the specified clock could damage the hardware.
> > 
> > I don't think it's unreasonable to add tolerances to drm_panel, since they 
> > will
> > differ in what is acceptable. The tricky part is teasing out what the 
> > tolerances
> > are.
> 
> The drm_panel already allows to specify all panel timings in pairs of
> min/typical/max. Just use this instead of some arbitrary tolerance.
> 
> I know most panels currently only expose a fixed mode, but it's not hard
> to convert this into a display_timing if you can get hold of the display
> datasheet. A lot of the panel datasheets I had to deal with lately
> actually specify all the required information. Most of the time you need
> to sanity check things, as most vendors seem to produce them by "copy
> the last datasheet we made and change only necessary fields", but that
> really isn't a hard job.

Agreed. Back at the time support for min/typ/max timings was introduced
specifically to address such issues. Conversion to those timings from a
fixed mode is as simple as making min = max = typ, where typ is the DRM
display mode value.

There's a helper that will return a DRM display mode based on the
typical values for drivers that don't care (i.e. most) and drivers that
do care can simply use the timings directly and adjust as necessary. It
would be rather backwards to add tolerances to display drivers, because
that's not where the tolerances are defined. Panels impose the
restrictions, so it should be panel drivers that contain the data.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v2 6/9] drm/panel: Add Netron DY E231732

[Thierry Reding]

On Tue, Sep 06, 2016 at 04:46:17PM +0200, Maxime Ripard wrote:
> The E231732 is a 7" panel with a resolution of 800x480.
> 
> Signed-off-by: Maxime Ripard 
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 26 ++
>  1 file changed, 26 insertions(+)

This is missing a device tree binding, but since this is a simple one I
added it. The commit message also says the resolution is 800x480, which
conflicts with what's in the display mode definition, so I've also
fixed that up while at it.

Thanks,
Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v2 5/9] of: Add vendor prefix for Netron DY

[Thierry Reding]

On Tue, Sep 06, 2016 at 04:46:16PM +0200, Maxime Ripard wrote:
> Netron DY is a brand of LCD panels found on SBCs and tablets.
> 
> Signed-off-by: Maxime Ripard 
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH 4/7] drm/panel: Add Sinlinx SinA33 7" panel

[Thierry Reding]

On Mon, Sep 05, 2016 at 10:02:48PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Sep 05, 2016 at 01:03:03AM +0800, Icenowy Zheng wrote:
> > Hi Everyone,
> > 
> > 01.09.2016, 23:40, "Maxime Ripard" :
> > >  The SinA33 has an unidentified panel. Add the timings for it under a new
> > >  compatible.
> > 
> > 
> > 
> > Excuse me...
> > I will ask a question which is not fully related to the patch here...
> > If I want to add a generic panel for Q8 tablets, what should it be called?
> > "allwinner,q8-lcd-panel-800x480"?
> 
> I guess it's more of a question for Thierry, but it seems like the
> trend is to put the diagonal rather than the resolution in the
> compatibles.

Compatible strings should contain the model number of the panel. There
is no such thing as a "generic panel for Q8 tablets".

Also, how is it that these panels are unidentified? Has nobody tried to
open them up and look at the panel to find a model number?

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v3] pwm: sun4i: fix a possible NULL dereference

[Thierry Reding]

On Wed, Aug 24, 2016 at 01:43:58PM +0200, LABBE Corentin wrote:
> of_match_device could return NULL, and so cause a NULL pointer
> dereference later.
> 
> For fixing this problem, we use of_device_get_match_data(), this will
> simplify the code a little by using a standard function for
> getting the match data.
> 
> Testing the return value of of_device_get_match_data is also necessary
> for avoiding a second NULL deref on pwm->data.
> 
> Reported-by: coverity (CID 1324139)
> Signed-off-by: LABBE Corentin 
> ---
> Changes since v2:
>  - Add a test on pwm->data for avoiding a second NULL deref.
> Changes since v1:
>  - Use of_device_get_match_data()
> 
>  drivers/pwm/pwm-sun4i.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 03a99a5..fb76c55 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -309,9 +309,6 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
>   struct resource *res;
>   u32 val;
>   int i, ret;
> - const struct of_device_id *match;
> -
> - match = of_match_device(sun4i_pwm_dt_ids, >dev);
>  
>   pwm = devm_kzalloc(>dev, sizeof(*pwm), GFP_KERNEL);
>   if (!pwm)
> @@ -326,7 +323,9 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
>   if (IS_ERR(pwm->clk))
>   return PTR_ERR(pwm->clk);
>  
> - pwm->data = match->data;
> + pwm->data = of_device_get_match_data(>dev);
> + if (!pwm->data)
> + return -EINVAL;

I don't see how this would ever be NULL. You'd have to add an entry to
the table that has a NULL data field, which is very unlikely to happen.
If you really want to check for this, which I think isn't necessary,
please wrap this in a WARN_ON() or something to make more noise when
this completely unexpected case happens.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v3 11/19] drm/panel: simple: Add timings for the Olimex LCD-OLinuXino-4.3TS

[Thierry Reding]

On Wed, Mar 23, 2016 at 05:38:34PM +0100, Maxime Ripard wrote:
> Add support for the Olimex LCD-OLinuXino-4.3TS panel to the DRM simple
> panel driver.
> 
> It is a 480x272 panel connected through a 24-bits RGB interface.
> 
> Signed-off-by: Maxime Ripard 
> Acked-by: Rob Herring 
> ---
>  .../display/panel/olimex,lcd-olinuxino-43-ts.txt   |  7 ++
>  drivers/gpu/drm/panel/panel-simple.c   | 26 
> ++
>  2 files changed, 33 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/panel/olimex,lcd-olinuxino-43-ts.txt

Applied, thanks.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 03/46] backlight: lm3630a_bl: stop messing with the pwm->period field

[Thierry Reding]

On Tue, Apr 12, 2016 at 03:16:13PM +0100, Lee Jones wrote:
> On Tue, 12 Apr 2016, Thierry Reding wrote:
> 
> > On Wed, Mar 30, 2016 at 10:03:26PM +0200, Boris Brezillon wrote:
> > > pwm->period field is not supposed to be changed by PWM users. The only
> > > ones authorized to change it are the PWM core and PWM drivers.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezil...@free-electrons.com>
> > > ---
> > >  drivers/video/backlight/lm3630a_bl.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > Applied, thanks.
> 
> Applied?

You didn't specifically Ack this one, but I presumed that since the
change is essentially the same as for pwm-backlight, and this is another
prerequisite for the remainder of the series it should go in through the
PWM tree as well.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 02/46] backlight: pwm_bl: remove useless call to pwm_set_period()

[Thierry Reding]

On Tue, Apr 12, 2016 at 03:16:53PM +0100, Lee Jones wrote:
> On Tue, 12 Apr 2016, Thierry Reding wrote:
> 
> > On Wed, Mar 30, 2016 at 10:03:25PM +0200, Boris Brezillon wrote:
> > > The PWM period will be set when calling pwm_config. Remove this useless
> > > call to pwm_set_period(), which might mess up with the internal PWM state.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezil...@free-electrons.com>
> > > Acked-by: Lee Jones <lee.jo...@linaro.org>
> > > ---
> > >  drivers/video/backlight/pwm_bl.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > Applied, thanks.
> 
> Applied?

You Acked it, so I assumed you were fine with me taking it through the
PWM tree to take care of the dependencies.

Did I assume wrongly?

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 15/46] pwm: introduce the pwm_state concept

[Thierry Reding]

On Tue, Apr 12, 2016 at 03:26:44PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 15:11:18 +0200
> Thierry Reding <thierry.red...@gmail.com> wrote:
> 
> > On Tue, Apr 12, 2016 at 02:45:08PM +0200, Boris Brezillon wrote:
> > > On Tue, 12 Apr 2016 14:21:41 +0200
> > > Thierry Reding <thierry.red...@gmail.com> wrote:
> > > 
> > > > On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> > > > > On Tue, 12 Apr 2016 13:49:04 +0200
> > > > > Thierry Reding <thierry.red...@gmail.com> wrote:
> > > > > 
> > > > > > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > > > > > The PWM state, represented by its period, duty_cycle and polarity,
> > > > > > > is currently directly stored in the PWM device.
> > > > > > > Declare a pwm_state structure embedding those field so that we 
> > > > > > > can later
> > > > > > > use this struct to atomically update all the PWM parameters at 
> > > > > > > once.
> > > > > > > 
> > > > > > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > > > > > pwm_get_state().
> > > > > > > 
> > > > > > > Signed-off-by: Boris Brezillon 
> > > > > > > <boris.brezil...@free-electrons.com>
> > > > > > > ---
> > > > > > >  drivers/pwm/core.c  |  8 
> > > > > > >  include/linux/pwm.h | 54 
> > > > > > > +
> > > > > > >  2 files changed, 46 insertions(+), 16 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > > > > index 6433059..f3f91e7 100644
> > > > > > > --- a/drivers/pwm/core.c
> > > > > > > +++ b/drivers/pwm/core.c
> > > > > > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip 
> > > > > > > *chip,
> > > > > > >   pwm->chip = chip;
> > > > > > >   pwm->pwm = chip->base + i;
> > > > > > >   pwm->hwpwm = i;
> > > > > > > - pwm->polarity = polarity;
> > > > > > > + pwm->state.polarity = polarity;
> > > > > > 
> > > > > > Would this not more correctly be assigned to pwm->args.polarity? 
> > > > > > After
> > > > > > all this is setting up the "initial" state, much like DT or the 
> > > > > > lookup
> > > > > > tables would for duty cycle and period.
> > > > > 
> > > > > Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> > > > > all the reference info should be extracted from DT, PWM lookup table 
> > > > > or
> > > > > driver specific ->request() implementation, but I can definitely
> > > > > initialize the args.polarity here too.
> > > > > 
> > > > > Should I keep the pwm->state.polarity assignment (to set the initial
> > > > > polarity when the driver does not support hardware readout)?
> > > > 
> > > > Wouldn't this work automatically as part of the pwm_apply_args() helper
> > > > if we extended it with this setting?
> > > 
> > > Well, as you explained in you answer to patch 5, pwm_apply_args()
> > > should be called on a per-request basis (each time a PWM device is
> > > requested), while the initial polarity setting should only be applied
> > > when registering the PWM chip (and its devices). After that, the
> > > framework takes care of keeping the PWM state in sync with the hardware
> > > state.
> > > 
> > > Let's take a real (though a bit unusual) example. Say you have a single
> > > PWM device referenced by two different users. Only one user can be
> > > enabled at a time, but each of them has its own reference config
> > > (different polarity, different period).
> > > 
> > > User1 calls pwm_get() and applies its own polarity and period. Then
> > > user1 is unregistered and release the PWM device, leaving the polarity
> > > and period untouched.
> > > 
> > > User2 is registered and request the same PWM device, but user2 is
> > > sma

[linux-sunxi] Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept

[Thierry Reding]

On Tue, Apr 12, 2016 at 03:06:27PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 13:39:12 +0200
> Thierry Reding <thierry.red...@gmail.com> wrote:
> 
> > On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> > > Currently the PWM core mixes the current PWM state with the per-platform
> > > reference config (specified through the PWM lookup table, DT definition or
> > > directly hardcoded in PWM drivers).
> > > 
> > > Create a pwm_args struct to store this reference config, so that PWM users
> > > can differentiate the current config from the reference one.
> > > 
> > > Patch all places where pwm->args should be initialized. We keep the
> > > pwm_set_polarity/period() calls until all PWM users are patched to
> > > use pwm_args instead of pwm_get_period/polarity().
> > 
> > Perhaps a helper would be useful? Something like:
> > 
> > static inline void
> > pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
> > {
> > pwm_set_duty_cycle(pwm, args->duty_cycle);
> > pwm_set_period(pwm, args->period);
> > }
> > 
> > ? That would make it slightly easier to get rid of it again after all
> > clients have been converted.
> > 
> > With the exception of pwm-clps711x all of these args are set at of_xlate
> > time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
> > might even be possible to move this call to the core, so that removal of
> > it will be a one-liner.
> 
> Okay, I think I misunderstood your suggestion. I thought you wanted
> this helper to set the reference config, but you actually want to apply
> a new state based on the PWM reference values.
> 
> Except that pwm_args does not contain all the required information to
> apply a full config (args->duty_cycle and args->enable do not exist).
> 
> This being said, in my v6 I moved the content of
> pwm_regulator_adjust_pwm_config() (patch 27) into a generic helper
> (pwm_adjust_config()). This helper is doing pretty much what you're
> suggesting here (but again, I'm not sure I correctly understood your
> suggestion :-/).

I'm not suggesting that pwm_apply_args() apply any state. I think we
both agreed earlier that the initial state (represented by pwm_args) was
never to be automatically applied.

What I was suggesting is that we move all the calls to pwm_set_period()
and pwm_set_duty_cycle() into a central location to make it easier to
remove them later in the series. This is really only temporary, so I
don't mind if we leave the calls sprinkled all over the place. At least
that way I hope we'll avoid confusion about what we're talking about =)

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 15/46] pwm: introduce the pwm_state concept

[Thierry Reding]

On Tue, Apr 12, 2016 at 02:45:08PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 14:21:41 +0200
> Thierry Reding <thierry.red...@gmail.com> wrote:
> 
> > On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> > > On Tue, 12 Apr 2016 13:49:04 +0200
> > > Thierry Reding <thierry.red...@gmail.com> wrote:
> > > 
> > > > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > > > The PWM state, represented by its period, duty_cycle and polarity,
> > > > > is currently directly stored in the PWM device.
> > > > > Declare a pwm_state structure embedding those field so that we can 
> > > > > later
> > > > > use this struct to atomically update all the PWM parameters at once.
> > > > > 
> > > > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > > > pwm_get_state().
> > > > > 
> > > > > Signed-off-by: Boris Brezillon <boris.brezil...@free-electrons.com>
> > > > > ---
> > > > >  drivers/pwm/core.c  |  8 
> > > > >  include/linux/pwm.h | 54 
> > > > > +
> > > > >  2 files changed, 46 insertions(+), 16 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > > > index 6433059..f3f91e7 100644
> > > > > --- a/drivers/pwm/core.c
> > > > > +++ b/drivers/pwm/core.c
> > > > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip 
> > > > > *chip,
> > > > >   pwm->chip = chip;
> > > > >   pwm->pwm = chip->base + i;
> > > > >   pwm->hwpwm = i;
> > > > > - pwm->polarity = polarity;
> > > > > + pwm->state.polarity = polarity;
> > > > 
> > > > Would this not more correctly be assigned to pwm->args.polarity? After
> > > > all this is setting up the "initial" state, much like DT or the lookup
> > > > tables would for duty cycle and period.
> > > 
> > > Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> > > all the reference info should be extracted from DT, PWM lookup table or
> > > driver specific ->request() implementation, but I can definitely
> > > initialize the args.polarity here too.
> > > 
> > > Should I keep the pwm->state.polarity assignment (to set the initial
> > > polarity when the driver does not support hardware readout)?
> > 
> > Wouldn't this work automatically as part of the pwm_apply_args() helper
> > if we extended it with this setting?
> 
> Well, as you explained in you answer to patch 5, pwm_apply_args()
> should be called on a per-request basis (each time a PWM device is
> requested), while the initial polarity setting should only be applied
> when registering the PWM chip (and its devices). After that, the
> framework takes care of keeping the PWM state in sync with the hardware
> state.
> 
> Let's take a real (though a bit unusual) example. Say you have a single
> PWM device referenced by two different users. Only one user can be
> enabled at a time, but each of them has its own reference config
> (different polarity, different period).
> 
> User1 calls pwm_get() and applies its own polarity and period. Then
> user1 is unregistered and release the PWM device, leaving the polarity
> and period untouched.
> 
> User2 is registered and request the same PWM device, but user2 is
> smarter and tries to extract the current PWM state before adapting the
> config according to pwm_args. If you just reset pwm->state.polarity
> each time pwm_apply_args() is called (and you suggested to call it as
> part of the request procedure), then this means the PWM state is no
> longer in sync with the hardware state.

In that case neither will be the period or duty cycle. Essentially this
gets us back to square one where we need to decide how to handle current
state vs. initial arguments.

But I don't think this is really going to be an issue because this is
all moot until we've moved over to the atomic API, at which point this
is all going to go away anyway.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 15/46] pwm: introduce the pwm_state concept

[Thierry Reding]

On Tue, Apr 12, 2016 at 02:17:18PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 13:49:04 +0200
> Thierry Reding <thierry.red...@gmail.com> wrote:
> 
> > On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> > > The PWM state, represented by its period, duty_cycle and polarity,
> > > is currently directly stored in the PWM device.
> > > Declare a pwm_state structure embedding those field so that we can later
> > > use this struct to atomically update all the PWM parameters at once.
> > > 
> > > All pwm_get_xxx() helpers are now implemented as wrappers around
> > > pwm_get_state().
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezil...@free-electrons.com>
> > > ---
> > >  drivers/pwm/core.c  |  8 
> > >  include/linux/pwm.h | 54 
> > > +
> > >  2 files changed, 46 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > index 6433059..f3f91e7 100644
> > > --- a/drivers/pwm/core.c
> > > +++ b/drivers/pwm/core.c
> > > @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> > >   pwm->chip = chip;
> > >   pwm->pwm = chip->base + i;
> > >   pwm->hwpwm = i;
> > > - pwm->polarity = polarity;
> > > + pwm->state.polarity = polarity;
> > 
> > Would this not more correctly be assigned to pwm->args.polarity? After
> > all this is setting up the "initial" state, much like DT or the lookup
> > tables would for duty cycle and period.
> 
> Yes, I wasn't sure about the pwm_add_with_polarity() meaning. To me,
> all the reference info should be extracted from DT, PWM lookup table or
> driver specific ->request() implementation, but I can definitely
> initialize the args.polarity here too.
> 
> Should I keep the pwm->state.polarity assignment (to set the initial
> polarity when the driver does not support hardware readout)?

Wouldn't this work automatically as part of the pwm_apply_args() helper
if we extended it with this setting?

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept

[Thierry Reding]

On Tue, Apr 12, 2016 at 02:04:12PM +0200, Boris Brezillon wrote:
> On Tue, 12 Apr 2016 13:39:12 +0200
> Thierry Reding <thierry.red...@gmail.com> wrote:
> 
> > On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> > > Currently the PWM core mixes the current PWM state with the per-platform
> > > reference config (specified through the PWM lookup table, DT definition or
> > > directly hardcoded in PWM drivers).
> > > 
> > > Create a pwm_args struct to store this reference config, so that PWM users
> > > can differentiate the current config from the reference one.
> > > 
> > > Patch all places where pwm->args should be initialized. We keep the
> > > pwm_set_polarity/period() calls until all PWM users are patched to
> > > use pwm_args instead of pwm_get_period/polarity().
> > 
> > Perhaps a helper would be useful? Something like:
> > 
> > static inline void
> > pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
> > {
> > pwm_set_duty_cycle(pwm, args->duty_cycle);
> > pwm_set_period(pwm, args->period);
> > }
> > 
> > ? That would make it slightly easier to get rid of it again after all
> > clients have been converted.
> 
> Sure. I'll add this helper.
> 
> > 
> > With the exception of pwm-clps711x all of these args are set at of_xlate
> > time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
> > might even be possible to move this call to the core, so that removal of
> > it will be a one-liner.
> 
> Not sure I get that one. Some drivers are implementing their own
> ->of_xlate() method, how would you get rid of this pwm_apply_args() in
> those custom implementations?

I was proposing to have pwm_apply_args() called from the core.
of_pwm_get() is where ->of_xlate() is called from, and the lookup table
arguments would be applied in pwm_get(). Taking into account clps711x,
which sets the arguments in ->request() it might be possible to simply
call pwm_apply_args() from pwm_device_request(), since that's also
called by all other request functions, even the legacy ones.

That said, the amount of code to modify isn't that large, so I'm fine if
you want to keep sprinkling the calls across multiple files, especially
since it's temporary.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 16/46] pwm: move the enabled/disabled info into pwm_state

[Thierry Reding]

On Wed, Mar 30, 2016 at 10:03:39PM +0200, Boris Brezillon wrote:
[...]
> @@ -145,7 +146,11 @@ static inline void pwm_get_state(const struct pwm_device 
> *pwm,
>  
>  static inline bool pwm_is_enabled(const struct pwm_device *pwm)
>  {
> - return test_bit(PWMF_ENABLED, >flags);
> + struct pwm_state pstate;

No need for the p prefix, in my opinion.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 15/46] pwm: introduce the pwm_state concept

[Thierry Reding]

On Wed, Mar 30, 2016 at 10:03:38PM +0200, Boris Brezillon wrote:
> The PWM state, represented by its period, duty_cycle and polarity,
> is currently directly stored in the PWM device.
> Declare a pwm_state structure embedding those field so that we can later
> use this struct to atomically update all the PWM parameters at once.
> 
> All pwm_get_xxx() helpers are now implemented as wrappers around
> pwm_get_state().
> 
> Signed-off-by: Boris Brezillon 
> ---
>  drivers/pwm/core.c  |  8 
>  include/linux/pwm.h | 54 
> +
>  2 files changed, 46 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 6433059..f3f91e7 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -268,7 +268,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>   pwm->chip = chip;
>   pwm->pwm = chip->base + i;
>   pwm->hwpwm = i;
> - pwm->polarity = polarity;
> + pwm->state.polarity = polarity;

Would this not more correctly be assigned to pwm->args.polarity? After
all this is setting up the "initial" state, much like DT or the lookup
tables would for duty cycle and period.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 04/46] pwm: get rid of pwm->lock

[Thierry Reding]

On Tue, Apr 12, 2016 at 01:32:55PM +0200, Boris Brezillon wrote:
> Hi Thierry,
> 
> On Tue, 12 Apr 2016 13:22:46 +0200
> Thierry Reding <thierry.red...@gmail.com> wrote:
> 
> > On Wed, Mar 30, 2016 at 10:03:27PM +0200, Boris Brezillon wrote:
> > > PWM devices are not protected against concurrent accesses. The lock in
> > > pwm_device might let PWM users think it is, but it's actually only
> > > protecting the enabled state.
> > > 
> > > Removing this lock should be fine as long as all PWM users are aware that
> > > accesses to the PWM device have to be serialized, which seems to be the
> > > case for all of them except the sysfs interface.
> > > Patch the sysfs code by adding a lock to the pwm_export struct and making
> > > sure it's taken for all accesses to the exported PWM device.
> > > 
> > > Signed-off-by: Boris Brezillon <boris.brezil...@free-electrons.com>
> > > ---
> > >  drivers/pwm/core.c  | 19 --
> > >  drivers/pwm/sysfs.c | 57 
> > > ++---
> > >  include/linux/pwm.h |  2 --
> > >  3 files changed, 50 insertions(+), 28 deletions(-)
> > 
> > This is a little overzealous. Only accesses that can cause races need to
> > be protected by the lock. All of the *_show() callbacks don't modify the
> > PWM device in any way, so there is no need to protect them against
> > concurrent accesses.
> 
> This is probably true for this set of changes, but what will happen
> when we'll switch to the atomic API? There's no guarantee that
> pwm->state = *newstate is done atomically, and you may see a partially
> updated state when calling pwm_get_state() while another thread is
> calling pwm_apply_state().

I'd argue that for sysfs it doesn't matter since the userspace API is
non-atomic anyway. As such it doesn't really matter which part of the
state you're getting because only one field from the query is exposed
to userspace, hence the coherence of the state is irrelevant.

All other users should be taking care of the serialization themselves
already.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 05/46] pwm: introduce the pwm_args concept

[Thierry Reding]

On Wed, Mar 30, 2016 at 10:03:28PM +0200, Boris Brezillon wrote:
> Currently the PWM core mixes the current PWM state with the per-platform
> reference config (specified through the PWM lookup table, DT definition or
> directly hardcoded in PWM drivers).
> 
> Create a pwm_args struct to store this reference config, so that PWM users
> can differentiate the current config from the reference one.
> 
> Patch all places where pwm->args should be initialized. We keep the
> pwm_set_polarity/period() calls until all PWM users are patched to
> use pwm_args instead of pwm_get_period/polarity().

Perhaps a helper would be useful? Something like:

static inline void
pwm_apply_args(struct pwm_device *pwm, const struct pwm_args *args)
{
pwm_set_duty_cycle(pwm, args->duty_cycle);
pwm_set_period(pwm, args->period);
}

? That would make it slightly easier to get rid of it again after all
clients have been converted.

With the exception of pwm-clps711x all of these args are set at of_xlate
time (for DT) or from the lookup table in pwm_get() (for non-DT), so it
might even be possible to move this call to the core, so that removal of
it will be a one-liner.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 04/46] pwm: get rid of pwm->lock

[Thierry Reding]

On Wed, Mar 30, 2016 at 10:03:27PM +0200, Boris Brezillon wrote:
> PWM devices are not protected against concurrent accesses. The lock in
> pwm_device might let PWM users think it is, but it's actually only
> protecting the enabled state.
> 
> Removing this lock should be fine as long as all PWM users are aware that
> accesses to the PWM device have to be serialized, which seems to be the
> case for all of them except the sysfs interface.
> Patch the sysfs code by adding a lock to the pwm_export struct and making
> sure it's taken for all accesses to the exported PWM device.
> 
> Signed-off-by: Boris Brezillon 
> ---
>  drivers/pwm/core.c  | 19 --
>  drivers/pwm/sysfs.c | 57 
> ++---
>  include/linux/pwm.h |  2 --
>  3 files changed, 50 insertions(+), 28 deletions(-)

This is a little overzealous. Only accesses that can cause races need to
be protected by the lock. All of the *_show() callbacks don't modify the
PWM device in any way, so there is no need to protect them against
concurrent accesses.

I've dropped the changes when applying.

Thanks,
Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 03/46] backlight: lm3630a_bl: stop messing with the pwm->period field

[Thierry Reding]

On Wed, Mar 30, 2016 at 10:03:26PM +0200, Boris Brezillon wrote:
> pwm->period field is not supposed to be changed by PWM users. The only
> ones authorized to change it are the PWM core and PWM drivers.
> 
> Signed-off-by: Boris Brezillon 
> ---
>  drivers/video/backlight/lm3630a_bl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Applied, thanks.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 02/46] backlight: pwm_bl: remove useless call to pwm_set_period()

[Thierry Reding]

On Wed, Mar 30, 2016 at 10:03:25PM +0200, Boris Brezillon wrote:
> The PWM period will be set when calling pwm_config. Remove this useless
> call to pwm_set_period(), which might mess up with the internal PWM state.
> 
> Signed-off-by: Boris Brezillon 
> Acked-by: Lee Jones 
> ---
>  drivers/video/backlight/pwm_bl.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Applied, thanks.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 01/46] pwm: rcar: make use of pwm_is_enabled()

[Thierry Reding]

On Wed, Mar 30, 2016 at 10:03:24PM +0200, Boris Brezillon wrote:
> Commit 5c31252c4a86 ("pwm: Add the pwm_is_enabled() helper") introduced a
> new function to test whether a PWM device is enabled or not without
> manipulating PWM internal fields.
> Hiding this is necessary if we want to smoothly move to the atomic PWM
> config approach without impacting PWM drivers.
> Fix this driver to use pwm_is_enabled() instead of directly accessing the
> ->flags field.
> 
> Signed-off-by: Boris Brezillon 
> ---
>  drivers/pwm/pwm-rcar.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 36/46] input: misc: max77693: switch to the atomic API

[Thierry Reding]

On Thu, Mar 31, 2016 at 08:57:18PM +0200, Boris Brezillon wrote:
> On Thu, 31 Mar 2016 10:48:01 -0700
> Dmitry Torokhov  wrote:
> 
> > Hi Boris,
> > 
> > On Wed, Mar 30, 2016 at 10:03:59PM +0200, Boris Brezillon wrote:
> > > pwm_config/enable/disable() have been deprecated and should be replaced
> > > by pwm_apply_state().
> > > 
> > > Signed-off-by: Boris Brezillon 
> > > ---
> > >  drivers/input/misc/max77693-haptic.c | 23 +--
> > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/input/misc/max77693-haptic.c 
> > > b/drivers/input/misc/max77693-haptic.c
> > > index cf6aac0..aef7dc4 100644
> > > --- a/drivers/input/misc/max77693-haptic.c
> > > +++ b/drivers/input/misc/max77693-haptic.c
> > > @@ -70,13 +70,16 @@ struct max77693_haptic {
> > >  
> > >  static int max77693_haptic_set_duty_cycle(struct max77693_haptic *haptic)
> > >  {
> > > + struct pwm_state pstate;
> > >   struct pwm_args pargs = { };
> > > - int delta;
> > >   int error;
> > >  
> > >   pwm_get_args(haptic->pwm_dev, );
> > > - delta = (pargs.period + haptic->pwm_duty) / 2;
> > > - error = pwm_config(haptic->pwm_dev, delta, pargs.period);
> > > + pwm_get_state(haptic->pwm_dev, );
> > > +
> > > + pstate.period = pargs.period;
> > > + pstate.duty_cycle = (pargs.period + haptic->pwm_duty) / 2;
> > > + error = pwm_apply_state(haptic->pwm_dev, );
> > 
> > This does not make sense with regard to the atomic API. If you look in
> > max77693_haptic_play_work(), right after calling
> > max77693_haptic_set_duty_cycle() we either try to enable or disable the
> > pwm. When switching to this new API we should combine both actions.
> 
> True. I'll address that, unless Thierry is fine keeping the non-atomic
> API, in which case I'll just drop patches 32 to 46.

I'm fine with keeping the pwm_enable(), pwm_disable() and pwm_config()
APIs, but they should only be used as shortcuts. Where possible the new
atomic API should be used to combine multiple operations into one.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 32/46] pwm: deprecate pwm_config(), pwm_enable() and pwm_disable()

[Thierry Reding]

On Thu, Mar 31, 2016 at 08:54:54PM +0200, Boris Brezillon wrote:
> Hi Dmitry,
> 
> On Thu, 31 Mar 2016 10:38:58 -0700
> Dmitry Torokhov  wrote:
> 
> > Hi Boris,
> > 
> > On Wed, Mar 30, 2016 at 10:03:55PM +0200, Boris Brezillon wrote:
> > > Prefix those function as deprecated to encourage all existing users to
> > > switch to pwm_apply_state().
> > 
> > Why not keep at least some of them as wrappers where we do not need to
> > chnage several parameters at once? It is much easier to have a driver
> > do:
> > 
> > error = pwm_enable(pwm);
> > if (error)
> > ...
> > 
> > rather than declaring the state variable, fectch it, adjust and then
> > apply.
> 
> True. Actually deprecating the non-atomic API was not my primary goal.
> Thierry would you mind if we keep both APIs around?

I'm fine with keeping these around, though purely as shortcuts. If users
need to modify two parameters at once (e.g. duty cycle and enable) then
they should be converted to use the atomic API, otherwise there'd be
little point in introduce it.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate

[Thierry Reding]

On Thu, Mar 31, 2016 at 09:07:09AM +0200, Boris Brezillon wrote:
> Hi Guenter,
> 
> On Wed, 30 Mar 2016 15:52:44 -0700
> Guenter Roeck  wrote:
> 
> > On Wed, Mar 30, 2016 at 10:03:31PM +0200, Boris Brezillon wrote:
> > > The PWM framework has clarified the concept of reference PWM config
> > > (the platform dependent config retrieved from the DT or the PWM
> > > lookup table) and real PWM state.
> > > 
> > > Use pwm_get_args() when the PWM user wants to retrieve this reference
> > > config and not the current state.
> > > 
> > > This is part of the rework allowing the PWM framework to support
> > > hardware readout and expose real PWM state even when the PWM has
> > > just been requested (before the user calls pwm_config/enable/disable()).
> > > 
> > > Signed-off-by: Boris Brezillon 
> > > ---
> > >  drivers/hwmon/pwm-fan.c | 19 +--
> > >  1 file changed, 13 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > > index 3e23003..82c5656 100644
> > > --- a/drivers/hwmon/pwm-fan.c
> > > +++ b/drivers/hwmon/pwm-fan.c
> > > @@ -40,15 +40,18 @@ struct pwm_fan_ctx {
> > >  
> > >  static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
> > >  {
> > > + struct pwm_args pargs = { };
> > 
> > Hi Boris,
> > 
> > I guess I am missing some context; sorry for that. Unfortunately,
> > I did not easily find an explanation, so please bear with me.
> > 
> > Two questions: Why do we need a local copy of struct pwm_args instead
> > of a pointer to it ? If it can change while being used, isn't it
> > inconsistent anyway ?
> 
> It cannot change after pwm_get() is called. For the reason behind
> prototype: I just followed the Thierry's proposal, but I'm perfectly
> fine returning a const struct pwm_args pointer intead of passing
> pwm_args as a parameter.
> 
> Thierry, what's your opinion?

I do prefer the current variant because it is more consistent with the
new atomic API, even if not strictly necessary because of the immutable
data.

> > Also, assuming the local copy is necessary, why initialize pargs ? 
> > After all, pwm_get_args() just overwrites it.
> 
> It's a leftover from a previous version where pwm_get_args was
> implemented this way:
> 
> static inline void pwm_get_args(pwm, args)
> {
>   if (pwm)
>   *args = pwm->args
> }
> 
> and this implementation was generating a lot of 'uninitialized
> variable' warnings.
> 
> I just decided to drop the 'if (pwm)' test, because, IMO, this
> should be checked way before calling pwm_get_args() is called.

Sounds fine to me.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

[Thierry Reding]

On Mon, Mar 07, 2016 at 08:34:19AM -0800, Doug Anderson wrote:
> Thierry,
> 
> On Thu, Feb 25, 2016 at 3:14 PM, Doug Anderson  wrote:
> > So just to summarize:
> >
> > * Add pwm_get_state(), pwm_apply_state(), pwm_get_args().
> > pwm_get_state() initially returns 0 for duty cycle if driver doesn't
> > support readout.
> >
> > * Re-implement pwm_get_period() (and maybe other similar functions)
> > atop pwm_get_state() as you describe earlier in the thread.
> >
> > * Document pwm_get_period() (and maybe other similar functions) as 
> > deprecated.
> >
> > * Fix drivers for all current 2 users of PWM regulator to support
> > hardware readout.
> >
> > * Update PWM regulator as you described earlier in the thread (Feb 23).
> >
> > * If PWM regulator is ever used on a new board whose PWM doesn't
> > support hardware readout, the voltage will change at probe time.
> >
> >
> > Did I get all that right?  Thanks!
> 
> Can you provide a "yes, you got that right" or a "no, you didn't
> understand"?  That will unblock Boris, I think.

Sounds about right. Hopefully this will eliminate any objections that
others had about the series.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

[Thierry Reding]

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.red...@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 5 ns and the firmware set
> > the period to 25000 ns. pwm_get_period() for this PWM device will return
> > 5 ns. If you reconfigure the PWM to generate a PWM signal with a
> > period of 3 ns, pwm_get_period() would still return 5 ns.
> >
> > A driver that doesn't support hardware readout, on the contrary, would
> > return 5 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, );

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 

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

[Thierry Reding]

On Thu, Feb 04, 2016 at 03:01:50PM +0100, Boris Brezillon wrote:
> Hi Mark, Thierry,
> 
> On Thu, 4 Feb 2016 11:02:03 +
> Mark Brown  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

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

[Thierry Reding]

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.red...@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 5 ns and the firmware set
the period to 25000 ns. pwm_get_period() for this PWM device will return
5 ns. If you reconfigure the PWM to generate a PWM signal with a
period of 3 ns, pwm_get_period() would still return 5 ns.

A driver that doesn't support hardware readout, on the contrary, would
return 5 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_appl

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

[Thierry Reding]

On Wed, Feb 03, 2016 at 11:04:20AM -0800, Doug Anderson wrote:
> Thierry
> 
> On Wed, Feb 3, 2016 at 6:53 AM, Thierry Reding <thierry.red...@gmail.com> 
> wrote:
> >> A) The software state here is the period and flags (AKA "inverted),
> >> right?  It does seem possible that you could apply the period and
> >> flags while keeping the calculated bootup duty cycle percentage
> >> (presuming that the PWM was actually enabled at probe time and there
> >> was a bootup duty cycle at all).  That would basically say that
> >> whenever you set the period of a PWM then the duty cycle of the PWM
> >> should remain the same percentage.  That actually seems quite sane
> >> IMHO.  It seems much saner than trying to keep the duty cycle "ns"
> >> when the period changes or resetting the PWM to some default when the
> >> period changes.
> >
> > That really depends on the use-case. If you're interested in the output
> > power of the PWM then, yes, this is sane. But it might not be the right
> > answer in other cases.
> 
> Ah, I see.  You're envisioning a device where active time in "ns" is
> more important than the percentage of active time.  Perhaps an LED
> where it's more important to have it on for no more than .1 seconds
> (so we don't drive it too long and burn it out?).  If we slowed down
> the duty cycle and adjusted the period to match, we could get into a
> bad state.

Yes. Granted, the vast majority of users is not in this category, but
it's something that has been brought to my attention in the past and it
works fine with the current API.

> >> B) Alternatively, I'd also say that setting a period without a duty
> >> cycle doesn't make a lot of sense.  ...so you could just apply the
> >> period at the same time that you apply the duty cycle the first time.
> >> Presumably you'd want to "lie" to the callers of the PWM subsystem and
> >> tell them that you already changed the period even though the change
> >> won't really take effect until they actually set the duty cycle.  If
> >> anyone cared to find out the true hardware period we could add a new
> >> pwm_get_hw_period().  ...or since the only reason you'd want to know
> >> the hardware period would be if you're trying to read the current duty
> >> cycle percentage, you could instead add "pwm_get_hw_state()" and have
> >> that return both the hardware period ns and duty cycle ns (which is
> >> the most accurate way to return the "percentage" without using fix or
> >> floating point math).
> >
> > But then you get into a situation where behaviour is dependent on the
> > PWM driver, whereas this is really very specific to one specific use-
> > case.
> 
> This is because only some drivers would be able to read the hardware
> state?  I'm not sure how we can get away from that.  In all proposals
> we've talked about (including what you propose below, right?) the PWM
> regulator will need a PWM driver that can read hardware state.  Only
> PWM drivers that have been upgraded to support reading hardware state
> can use the PWM regulator (or at least only those drivers will be able
> to use the PWM regulator glitch-free).

Yes, the key here is glitch-free. There's no reason whatsoever that the
rugaltor-pwm driver should be limited to usage with a hardware readout-
capable PWM driver. If you don't care about glitches, likely because no
critical components depend on the regulator, you can simply force what
state you choose on boot.

As a matter of fact, I think that's how regulators work already. If the
current output voltage doesn't match the specified constraints, then a
valid value will be forced by the regulator core. If the voltage lies
within the constraints the core won't touch the regulator. Is this not
going to "just work" with the PWM regulator?

The problem is somewhat simplified if that's the case. An implementation
could then fail the regulator_get_voltage() if hardware readout is not
supported and return the current voltage when readout is possible.

> 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 fe

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

[Thierry Reding]

On Mon, Jan 25, 2016 at 10:51:20AM -0800, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jan 25, 2016 at 9:08 AM, Thierry Reding
> <thierry.red...@gmail.com> wrote:
> > I really don't understand this design decision. I presume that the PWM
> > controlling this system-critical logic is driven by the SoC? So if the
> > regulator is system-critical, doesn't that make it a chicken and egg
> > problem? How can the SoC turn the PWM on if it doesn't have power? But
> > perhaps I'm completely misunderstanding what you're saying. Perhaps if
> > somebody could summarize how exactly this works, it would help better
> > understand the requirements or what's the correct thing to do.
> 
> Sure, here's how the dang thing works, as I understand it.
> 
> First, an overview of PWM regulator in general (maybe you know this,
> but to get us on the same page).  There's an external regulator on the
> system.  Looking on at least one board I see a TLV62565 specifically.
> 
> From the docs of TLV62565, I see it describe the situation as the chip
> being able to provide an adjustable output voltage configurable via an
> external resistor divider.  In simplified terms words you can adjust
> the output voltage of the regulator by tweaking the inputs to one of
> its pins.  I'm just a software guy so I can't explain all the details
> of it, but the net-net of the situation is is that you can hook this
> configuration pin up to the output of a PWM (with a bunch of well
> balanced resistors and capacitors) and then you can set the voltage
> based on the output of the PWM.
> 
> 
> OK, so what happens at bootup?  At bootup most of the pins of the
> rk3288 (including the PWM) are configured as inputs with a pull.  The
> particular pin hooked up to this PWM has a pulldown.  Remember that
> we've got this nicely balanced set of resistors and capacitors hooked
> up to the output of our PWM pin?  So what happens when we have this
> pin configured as an input?  As I understand it / remember it:
> 
> * input w/ no pull: equivalent to 50% duty cycle on the PWM
> * input w/ pull down: equivalent to slightly higher voltage than 50%
> duty cycle on the PWM
> * input w/ pull up: equivalent to slightly lower voltage than 50% duty
> cycle on the PWM
> 
> On our particular board that means that the rail comes up with roughly
> 1.1V.  If you drive the PWM at 100% (or set the pin to output high)
> you get .86V and if you drive the PWM at 0% (or set the pin to output
> low) you get 1.36V.
> 
> Now, 1.1V is plenty of voltage to boot the system.  In fact most of
> the logic within the SoC can run as low as 0.95V I think.  ...but 0.86
> V is not enough to run the logic parts of the system (even at their
> default bootup frequencies) 1.1V is _definitely_ not enough to run the
> SDRAM memory controller at full speed.
> 
> 
> So the bootloader wants to run the system fast so it can boot fast.
> It increases the CPU rails (as is typical for a bootloader) and moves
> the ARM CPU to 1.8GHz (from the relatively slow boot frequency) and
> also raises the logic rail to 1.2V (or I think 1.15 V on systems w/
> different memory configs) and inits the SDRAM controller to run at
> full speed.  Then it boots Linux.
> 
> Note: apparently in U-Boot they actually boot system slower (this was
> at least true 1.5 years ago with some reference U-Boot Rockchip
> provided).  If I understand correctly they _didn't_ init the SDRAM
> controller as full speed in the bootloader and just left the logic
> rail at its bootup default.  If everyone had done that then our job
> would be "easier" because we wouldn't need to read in the voltage
> provided by the bootloader (by reading the PWM and cros-referencing
> with our table), though even in that case we'd have to be very careful
> not to glitch the line (since .86 V is too low).  Of course all of
> those systems are stuck running at a very slow memory speed until
> Linux gets DDR Frequency support for Rockchip whereas systems with our
> bootloader not only boot faster but also get to use the full memory
> speed even without any Linux DDRFreq drivers.
> 
> 
> In any case: I think I've demonstrated how a critical system rail can
> be using a PWM regulator and how glitching that PWM regulator at boot
> time can be catastrophic.  Possibly it's not critical to be able to
> "read" the voltage that that bootloader left things configured at
> (it's mostly nice for debugging purposes), but it's definitely
> important to make sure we don't set it to some default and important
> to never glitch it.  Said another way, presumably a DDR Freq driver
> would be able to switch the memory controller frequency sanely by
> reading the memory controller

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

[Thierry Reding]

On Mon, Jan 25, 2016 at 08:28:31AM -0800, Doug Anderson wrote:
> Thierry and Boris,
> 
> On Tue, Nov 10, 2015 at 9:34 AM, Thierry Reding
> <thierry.red...@gmail.com> wrote:
> > On Mon, Oct 19, 2015 at 12:12:12PM +0200, Heiko Stübner wrote:
> >> Hi Thierry,
> >>
> >> Am Montag, 21. September 2015, 11:33:17 schrieb Boris Brezillon:
> >> > Hello,
> >> >
> >> > This series adds support for atomic PWM update, or IOW, the capability
> >> > to update all the parameters of a PWM device (enabled/disabled, period,
> >> > duty and polarity) in one go.
> >>
> >> is anything more blocking this series? It's now sitting on the lists for
> >> nearly a month and everybody seems happy with it, so it would be really 
> >> nice
> >> to have in mainline :-) .
> >>
> >> Especially as this also makes it possible for Rockchip Chromebooks to 
> >> actually
> >> control the logic-regulator that is implemented as pwm-regulator there.
> >
> > Last time I tried to put this into linux-next I got immediately
> > bombarded by a number of build failures, so I backed things out. The
> > current plan is to give this another try after v4.4-rc1.
> 
> We're now into the 4.5 timeframe.  Does anyone have a concrete set of
> things that need to happen before this patch series makes it into
> mainline?

I think the current status is that we're more or less blocked on the
decision on what the reset state of the PWM should be. The question is
what to do if the PWM hardware readout differs from the settings found
in DT.

> From searching I see that the latest version of this series is v4 and
> there are a smattering of comments on the 24-patch series.  Presumably
> a v5 needs to be posted to address those things.
> 
> ...but it looks like the big sticking point is that Boris is waiting
> for a response to his questions in
> <https://patchwork.kernel.org/patch/7622881/>.  Thierry: can you give
> Boris some direction for what else he needs to do?  We need to come up
> with _some_ solution since this series gets us much better support for
> PWM regulators.  Without this series or some other solution, PWM
> regulators aren't usable in mainline on any system that uses them for
> system-critical rails.  Nearly all Rockchip reference boards and
> shipping devices uses a PWM regulator for the system-critidal "logic"
> rail.  That means any patches which need to change this rail in Linux
> are blocked.

I really don't understand this design decision. I presume that the PWM
controlling this system-critical logic is driven by the SoC? So if the
regulator is system-critical, doesn't that make it a chicken and egg
problem? How can the SoC turn the PWM on if it doesn't have power? But
perhaps I'm completely misunderstanding what you're saying. Perhaps if
somebody could summarize how exactly this works, it would help better
understand the requirements or what's the correct thing to do.

> If there's already been off-list discussion and Boris already knows
> what the next steps are then my apologies and I'll wait patiently for
> the next series.  ;)

I don't think we reached a conclusion on this. And to be honest, I'm not
sure what the right way forward is in this situation. So in order to
make some forward progress I suggest we start a discussion, hopefully
that will clarify the situation and help lead to the conclusion. Let me
recap where we are:

Boris' series has two goals: 1) allow seamless hand-off from firmware to
kernel of a PWM channel and 2) apply changes to a regulator in a single
atomic operation. To achieve this the concept of PWM state is introduced
which encapsulates the settings of a PWM channel. On driver probe the
current state will be read from hardware and when one or more parameters
are to be changed, the current state is duplicated, the new values set
in the state and the new state applied.

The problem that we've encountered is that since the PWM parameters are
specified in DT (or board files), there is the possibility of the PWM
hardware state and the board parameters disagreeing. To resolve such
situations there must be a point in time where both hardware state and
software state must be synchronized. Now the most straightforward way to
do that would be to simply apply the software state and be done with it.
However the software state initially lacks the duty cycle because it is
a parameter that usually depends on the use-case (for backlight for
instance it controls the brightness, for regulators it controls the
output voltage, ...).

Applying the software state as-is also means that there's no reason at
all to read out the hardware state in the first place, because it will
simply be discarded.

An alternative would be to discard the software sta

[linux-sunxi] Re: [PATCH v3 00/12] pwm: add support for atomic update

[Thierry Reding]

On Mon, Oct 19, 2015 at 12:12:12PM +0200, Heiko Stübner wrote:
> Hi Thierry,
> 
> Am Montag, 21. September 2015, 11:33:17 schrieb Boris Brezillon:
> > Hello,
> > 
> > This series adds support for atomic PWM update, or IOW, the capability
> > to update all the parameters of a PWM device (enabled/disabled, period,
> > duty and polarity) in one go.
> 
> is anything more blocking this series? It's now sitting on the lists for 
> nearly a month and everybody seems happy with it, so it would be really nice 
> to have in mainline :-) .
> 
> Especially as this also makes it possible for Rockchip Chromebooks to 
> actually 
> control the logic-regulator that is implemented as pwm-regulator there.

Last time I tried to put this into linux-next I got immediately
bombarded by a number of build failures, so I backed things out. The
current plan is to give this another try after v4.4-rc1.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH 09/19] drm: sun4i: Add DT bindings documentation

[Thierry Reding]

On Fri, Oct 30, 2015 at 11:40:03AM -0500, Rob Herring wrote:
> On Fri, Oct 30, 2015 at 9:20 AM, Maxime Ripard
[...]
> > +Optional properties:
> > +  - allwinner,tv-encoder: phandle to the TV Encoder in our pipeline
> > +  - allwinner,panel: phandle to the panel used in our RGB interface
> 
> Use of-graph please.

Why? Panels are a really simple resource and a simple phandle is fully
capable of describing the relationship.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH 07/19] drm/panel: simple: Add timings for the Olimex LCD-OLinuXino-4.3TS

[Thierry Reding]

On Fri, Oct 30, 2015 at 03:20:53PM +0100, Maxime Ripard wrote:
> Add support for the Olimex LCD-OLinuXino-4.3TS panel to the DRM simple
> panel driver.
> 
> It is a 480x272 panel connected through a 24-bits RGB interface.
> 
> Signed-off-by: Maxime Ripard 
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 26 ++
>  1 file changed, 26 insertions(+)

I don't see a patch adding the DT binding documentation for this panel.

Also, the olimex vendor prefix isn't defined.

> diff --git a/drivers/gpu/drm/panel/panel-simple.c 
> b/drivers/gpu/drm/panel/panel-simple.c
> index f97b73ec4713..3a9ecb64d1e6 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1096,6 +1096,29 @@ static const struct panel_desc shelly_sca07010_bfn_lnn 
> = {
>   .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
>  };
>  
> +static const struct drm_display_mode olimex_lcd_olinuxino_43ts_mode = {
> + .clock = 9000,
> + .hdisplay = 480,
> + .hsync_start = 480 + 5,
> + .hsync_end = 480 + 5 + 30,
> + .htotal = 480 + 5 + 30 + 10,
> + .vdisplay = 272,
> + .vsync_start = 272 + 8,
> + .vsync_end = 272 + 8 + 5,
> + .vtotal = 272 + 8 + 5 + 3,
> + .vrefresh = 60,
> +};
> +
> +static const struct panel_desc olimex_lcd_olinuxino_43ts = {
> + .modes = _lcd_olinuxino_43ts_mode,
> + .num_modes = 1,
> + .size = {
> + .width = 105,
> + .height = 67,
> + },
> + .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
> +};
> +

These mode and panel descriptors are all sorted alphabetically (by
vendor, by model), please keep it so.

>  static const struct of_device_id platform_of_match[] = {
>   {
>   .compatible = "ampire,am800480r3tmqwa1h",
> @@ -1191,6 +1214,9 @@ static const struct of_device_id platform_of_match[] = {
>   .compatible = "shelly,sca07010-bfn-lnn",
>   .data = _sca07010_bfn_lnn,
>   }, {
> + .compatible = "olimex,lcd-olinuxino-43-ts",
> + .data = _lcd_olinuxino_43ts,
> + }, {
>   /* sentinel */
>   }

Here as well.

Thierry

-- 
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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Thierry Reding]

On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
 Hi Maxime,
 
 On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
 maxime.rip...@free-electrons.com wrote:
  -module_init(simplefb_init);
  +/*
  + * While this can be a module, if builtin it's most likely the console
  + * So let's leave module_exit but move module_init to an earlier place
  + */
 
  Not really related to this patch itself, but do we want to support
  simplefb as a module? It seems like it's going to be most of the time
  broken.
 
 If it depends on clocks, it won't work as a module, as CCF will have disabled
 all unused clocks at that point.

If it does depend on anything beyond clocks it won't work at all. Clocks
are special because they get set up very early at boot time. If it turns
out that a simplefb ever needs a regulator to remain on, and that's even
quite likely to happen eventually, it's going to fail miserably, because
those regulators will typically be provided by a PMIC on an I2C bus. The
regulator won't be registered until very late into the boot process and
a regulator_get() call will almost certainly cause the simplefb driver
to defer probing.

Now deferring probing is a real showstopper for simplefb, because not
only does it make the framebuffer useless as early boot console, once
probing is attempted again the clocks that it would have needed to
acquire to keep going will already have been switched off, too.

Thierry


pgpt70wiqnhCn.pgp
Description: PGP signature

[linux-sunxi] Re: [PATCH 3/4] simplefb: Change simplefb_init from module_init to fs_initcall

[Thierry Reding]

On Tue, Nov 18, 2014 at 12:01:08PM +0100, Hans de Goede wrote:
 Hi,
 
 On 11/18/2014 11:19 AM, Thierry Reding wrote:
  On Thu, Nov 13, 2014 at 09:58:41AM +0100, Geert Uytterhoeven wrote:
  Hi Maxime,
 
  On Thu, Nov 13, 2014 at 9:52 AM, Maxime Ripard
  maxime.rip...@free-electrons.com wrote:
  -module_init(simplefb_init);
  +/*
  + * While this can be a module, if builtin it's most likely the console
  + * So let's leave module_exit but move module_init to an earlier place
  + */
 
  Not really related to this patch itself, but do we want to support
  simplefb as a module? It seems like it's going to be most of the time
  broken.
 
  If it depends on clocks, it won't work as a module, as CCF will have 
  disabled
  all unused clocks at that point.
  
  If it does depend on anything beyond clocks it won't work at all. Clocks
  are special because they get set up very early at boot time. If it turns
  out that a simplefb ever needs a regulator to remain on, and that's even
  quite likely to happen eventually, it's going to fail miserably, because
  those regulators will typically be provided by a PMIC on an I2C bus. The
  regulator won't be registered until very late into the boot process and
  a regulator_get() call will almost certainly cause the simplefb driver
  to defer probing.
 
 Right, this has been discussed already and the plan is to have simplefb
 continue its probe function and return success from it if it encounters
 any -eprobe_defer errors, while tracking which resources it misses.
 
 And then have a late_initcall which will claim any resources which failed
 with -eprobe beforehand.

How do you ensure that the late_initcall gets run before any of the
other late_initcalls that disable the resources? Also my recollection is
that deferred probing will first be triggered the first time from a
late_initcall, so chances aren't very high that all resources have shown
up by that time.

  Now deferring probing is a real showstopper for simplefb, because not
  only does it make the framebuffer useless as early boot console, once
  probing is attempted again the clocks that it would have needed to
  acquire to keep going will already have been switched off, too.
 
 That is not true, even with the current implementation, if all necessary
 drivers are built in, then simplefb will come up later, but it will still
 come up before the late_initcall which disables the clocks.

Yes, in the current implementation because clocks typically are
registered very early and thus you don't hit the deferred probe. The
same is not true for other types of resources where it's actually quite
common to hit deferred probing (regulators is a very notorious one).

It doesn't matter whether a driver is built-in or not, once you hit
deferred probing you lose.

 Once we do the split probing described above (which is something which
 we plan to do when it becomes necessary), then simplefb will still come
 up early.

It will come up early but won't have acquired all the resources that it
needs, so unless you somehow manage to order late_initcalls in exactly
the way that you need them, the frameworks will still turn off what you
haven't managed to claim yet.

Thierry


pgpJOIzb02efC.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Wed, Oct 01, 2014 at 11:17:23AM -0700, Mike Turquette wrote:
 On Wed, Oct 1, 2014 at 12:30 AM, Thierry Reding
 thierry.red...@gmail.com wrote:
  On Tue, Sep 30, 2014 at 02:37:53PM -0700, Mike Turquette wrote:
  Quoting Thierry Reding (2014-09-29 06:54:00)
   On Mon, Sep 29, 2014 at 01:34:36PM +0200, Maxime Ripard wrote:
On Mon, Sep 29, 2014 at 12:44:57PM +0200, Thierry Reding wrote:
   Plus, speaking more specifically about the clocks, that won't 
   prevent
   your clock to be shut down as a side effect of a later 
   clk_disable
   call from another driver.
 
   Furthermore isn't it a bug for a driver to call clk_disable() 
   before a
   preceding clk_enable()? There are patches being worked on that 
   will
   enable per-user clocks and as I understand it they will 
   specifically
   disallow drivers to disable the hardware clock if other drivers 
   are
   still keeping them on via their own referenc.
 
  Calling clk_disable() preceding clk_enable() is a bug.
 
  Calling clk_disable() after clk_enable() will disable the clock 
  (and
  its parents)
  if the clock subsystem thinks there are no other users, which is 
  what will
  happen here.

 Right. I'm not sure this is really applicable to this situation, 
 though.
   
It's actually very easy to do. Have a driver that probes, enables its
clock, fails to probe for any reason, call clk_disable in its exit
path. If there's no other user at that time of this particular clock
tree, it will be shut down. Bam. You just lost your framebuffer.
   
Really, it's just that simple, and relying on the fact that some other
user of the same clock tree will always be their is beyond fragile.
  
   Perhaps the meaning clk_ignore_unused should be revised, then. What you
   describe isn't at all what I'd expect from such an option. And it does
   not match the description in Documentation/kernel-parameters.txt either.
 
  From e156ee56cbe26c9e8df6619dac1a993245afc1d5 Mon Sep 17 00:00:00 2001
  From: Mike Turquette mturque...@linaro.org
  Date: Tue, 30 Sep 2014 14:24:38 -0700
  Subject: [PATCH] doc/kernel-parameters.txt: clarify clk_ignore_unused
 
  Refine the definition around clk_ignore_unused, which caused some
  confusion recently on the linux-fbdev and linux-arm-kernel mailing
  lists[0].
 
  [0] http://lkml.kernel.org/r/20140929135358.GC30998@ulmo
 
  Signed-off-by: Mike Turquette mturque...@linaro.org
  ---
  Thierry,
 
  Please let me know if this wording makes the feature more clear.
 
  I think that's better than before, but I don't think it's accurate yet.
  As pointed out by Maxime unused clock may still be disabled if it's part
  of a tree and that tree is being disabled because there are no users
  left.
 
 It is entirely accurate. This feature does in fact prevent the clock
 framework from *automatically* gating clock 

According to what Maxime said if an unused clock is a sibling (has the
same parent) of a clock that is used and then gets disabled, then if the
parent has no other clocks that are enabled, the unused clock will still
be disabled.

That's still counts as automatically to me. Not automatically would
mean that the clock needs to be disabled explicitly for it to become
disabled. Disabling it as a side-effect of its parent getting disabled
is still automatic.

 And it was merged by Olof so that he could use simplefb with the
 Chromebook!

And presumably it does work for that specific Chromebook. It seems,
though that for hardware with a somewhat whackier clock tree it doesn't
work so well. As far as I can tell that's the reason for this patch and
the ensuing discussion in the first place.

Although, perhaps nobody ever really tested whether or not the above
scenario was actually a problem for sunxi and maybe clk_ignore_unused
would work for them. But as I understand they don't want to use it, so
this whole debate about this kernel parameter is a bit moot.

  What I had argued is that it's unexpected behavior, because the clock
  is still unused (or becomes unused again), therefore shouldn't be
  disabled at that point either.
 
 Leaving clocks enabled because nobody claimed them is not an option.

But that's exactly what clk_ignore_unused is, isn't it? I'm now totally
confused.

  So if you want to keep the current behaviour where an unused clock can
  still be disabled depending on what other users do, then I think it'd be
  good to mention that as a potential caveat.
 
 Do you have a suggestion on the wording?

Perhaps something like this:

Note that if an unused clock shares a parent with clocks that
are used, the unused clock may still become disabled as a side-
effect of the parent clock being disabled when none of the
children that are used remain enabled.

?

Thierry


pgpdRD2TSuRb3.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Wed, Oct 01, 2014 at 06:17:04PM +0100, Mark Brown wrote:
 On Wed, Oct 01, 2014 at 02:48:53PM +0200, Thierry Reding wrote:
  On Wed, Oct 01, 2014 at 01:20:08PM +0100, Mark Brown wrote:
[...]
  and that the DT must not contain any hint of simplefb as
   shipped separately.
 
  Well, I don't think it should because it describes the same resources
  that the device tree node for the real device already describes. But
  perhaps this is one of the cases where duplication isn't all that bad?
 
 If we were worried about this wecould also do it by referring to
 those nodes and saying get all the resources these things need rather
 than duplicating the references (this might make it easier to work out
 when the system is ready to hand off to the real drivers).

That's problematic to some degree because not all resource types may
have a binding that allows them to be automatically probed, so it could
be difficult to implement get all the resources this thing needs. But
perhaps we can come up with good enough heuristics to make that work
reliably.

One downside of that is that there may be a lot of components involved
in getting display to work and not all resources may be needed to keep
the current state running, so we may be claiming too many. But given
that we'd eventually release all of them anyway this shouldn't be too
much of an issue.

   That's never going to work well as far as I can see
   but doesn't seem like an ABI stability issue, or at least not a
   reasonable one.
 
  It would work well under the assumption that the kernel wouldn't be
  touching any of the resources that simplefb depends on. If that's not a
  reasonable assumption then I think we can't make simplefb work the way
  it's currently written.
 
 I can't see how that's reasonable unless the kernel has some way of
 figuring out what it shouldn't be touching.

Agreed. It's become clear in this discussion that we can't do this in
the way x86 and other more firmware-oriented architectures do it. They
get away with it because they in fact hide all of this in the firmware
or don't provide a way to control the resources in such a fine-grained
manner to begin with.

Thierry


pgpg3RGNrnKTX.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Wed, Oct 01, 2014 at 08:43:27PM +0200, Geert Uytterhoeven wrote:
 On Wed, Oct 1, 2014 at 7:17 PM, Mark Brown broo...@kernel.org wrote:
  Well, I don't think it should because it describes the same resources
  that the device tree node for the real device already describes. But
  perhaps this is one of the cases where duplication isn't all that bad?
 
  If we were worried about this wecould also do it by referring to
  those nodes and saying get all the resources these things need rather
  than duplicating the references (this might make it easier to work out
  when the system is ready to hand off to the real drivers).
 
 You can have a single node for both simplefb and the later real driver.
 DT describes the hardware, not the software ecosystem running on the
 hardware. Clock, regulators, etc. don't change from a hardware point of
 view.
 
 If the firmware initialized a suitable graphics mode, it just has to add
 linux,simplefb to the compatible property (and perhaps a few other
 simplefb-specific properties).

Unfortunately I don't think that's going to work. Especially on ARM SoCs
there is no single node for a display device. The display device is
typically composed of several subdevices.

Thierry


pgpuH2r96AUtv.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Tue, Sep 30, 2014 at 02:37:53PM -0700, Mike Turquette wrote:
 Quoting Thierry Reding (2014-09-29 06:54:00)
  On Mon, Sep 29, 2014 at 01:34:36PM +0200, Maxime Ripard wrote:
   On Mon, Sep 29, 2014 at 12:44:57PM +0200, Thierry Reding wrote:
  Plus, speaking more specifically about the clocks, that won't 
  prevent
  your clock to be shut down as a side effect of a later clk_disable
  call from another driver.
 
  Furthermore isn't it a bug for a driver to call clk_disable() 
  before a
  preceding clk_enable()? There are patches being worked on that will
  enable per-user clocks and as I understand it they will specifically
  disallow drivers to disable the hardware clock if other drivers are
  still keeping them on via their own referenc.
 
 Calling clk_disable() preceding clk_enable() is a bug.
 
 Calling clk_disable() after clk_enable() will disable the clock (and
 its parents)
 if the clock subsystem thinks there are no other users, which is what 
 will
 happen here.

Right. I'm not sure this is really applicable to this situation, though.
   
   It's actually very easy to do. Have a driver that probes, enables its
   clock, fails to probe for any reason, call clk_disable in its exit
   path. If there's no other user at that time of this particular clock
   tree, it will be shut down. Bam. You just lost your framebuffer.
   
   Really, it's just that simple, and relying on the fact that some other
   user of the same clock tree will always be their is beyond fragile.
  
  Perhaps the meaning clk_ignore_unused should be revised, then. What you
  describe isn't at all what I'd expect from such an option. And it does
  not match the description in Documentation/kernel-parameters.txt either.
 
 From e156ee56cbe26c9e8df6619dac1a993245afc1d5 Mon Sep 17 00:00:00 2001
 From: Mike Turquette mturque...@linaro.org
 Date: Tue, 30 Sep 2014 14:24:38 -0700
 Subject: [PATCH] doc/kernel-parameters.txt: clarify clk_ignore_unused
 
 Refine the definition around clk_ignore_unused, which caused some
 confusion recently on the linux-fbdev and linux-arm-kernel mailing
 lists[0].
 
 [0] http://lkml.kernel.org/r/20140929135358.GC30998@ulmo
 
 Signed-off-by: Mike Turquette mturque...@linaro.org
 ---
 Thierry,
 
 Please let me know if this wording makes the feature more clear.

I think that's better than before, but I don't think it's accurate yet.
As pointed out by Maxime unused clock may still be disabled if it's part
of a tree and that tree is being disabled because there are no users
left.

What I had argued is that it's unexpected behaviour, because the clock
is still unused (or becomes unused again), therefore shouldn't be
disabled at that point either.

So if you want to keep the current behaviour where an unused clock can
still be disabled depending on what other users do, then I think it'd be
good to mention that as a potential caveat.

Thierry


pgpC8LuOkv3Rr.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Tue, Sep 30, 2014 at 07:00:36PM +0100, Mark Brown wrote:
 On Tue, Sep 30, 2014 at 08:03:14AM +0200, Thierry Reding wrote:
  On Mon, Sep 29, 2014 at 05:11:01PM +0100, Mark Brown wrote:
 
   Not really thought this through fully yet but would having phandles to
   the relevant devices do the job?  Kind of lines up with Grant's idea of
   having dummy drivers.
 
  One of the arguments that came up during the discussion of the sunxi
  patches is that simplefb is going to be used precisely because there is
  no real driver for the display part of the SoC yet and nobody knows what
  the binding will look like. So there's nothing to point at currently and
  for the same reason having a dummy driver won't work. There's simply no
  definition of what resources are needed.
 
 You may well need to extend the binding in future for an actual driver
 but from the point of view of what's going into the block it really
 should just be a case of reading the datasheet and mechanically typing
 that in.  If we can work out how to say where the framebuffer is we
 really ought to be able to work this stuff out.

I agree from a technical point of view. However given the dynamically
generated nature of the node the problem is more of a logistical nature.
As we've seen U-Boot is being enabled to add clocks to the simplefb node
but I'm fairly certain that there's a regulator somewhere that needs to
be enabled too, be it for powering the display controller, the panel or
the backlight. I wouldn't even be surprised if there were one for each
of those. If so simplefb on this board will break when the regulators
are described in the kernel's DTS.

If we don't consider this a problem then the whole DT ABI stability
business is a farce.

There may be also resets involved. Fortunately the reset framework is
minimalistic enough not to care about asserting all unused resets at
late_initcall. And other things like power domains may also need to be
kept on.
 
   We might want to do that in the future, though it's not always the case
   that reset is the lowest power state.
 
  That proves my point. If we ever decide to assert resets by default
  we'll have yet another subsystem that can potentially break existing
  DTs.
 
 OTOH given the level of breakage that's like to introduce we might just
 decide not to do that...

It might be the sensible thing to do in most cases. I think there's a
legitimate reason not to trust firmware. However in case of simplefb we
already do, so I think having a sort of flag to signal that we do trust
firmware would allow us to cope with these situation much better.

  In the end it brings us back to the very fundamental principles of DT
  that's been causing so much pain. For things to work properly and in a
  stable way you have to get the bindings right and complete from the
  start. That is, it needs to describe every aspect of the hardware block
  and all links to other components.
 
 Or we ned to introduce things in a conservative fashion which does cope
 with backwards compatibility; it's definitely more work but it is
 doable.

Is it? I thought the only way to keep backwards compatibility was by
making any new properties optional. But if those properties add vital
information for the device to work you have to come up with a sensible
default to keep existing setups working that lack the new properties.
Doing that is not going to scale very well. It has a chance of working
for hardware-specific drivers because we may be able to derive the
default from the SoC generation or even the machine compatible. But I
don't see how it could work for something that's supposed to be generic
like simplefb.

I'm hoping that there's a better way that I don't know about, because it
would certainly make a lot of things much easier.

   One thing that makes me a bit nervous about this approach in the context
   of the regulator API is the frequency with which one finds shared
   supplies.  I'm not sure if it's actually a big problem in practice but
   it makes me worry a bit.  We could probably just do something like make
   refcounting down to zero not actually disable anything for standard
   regulators to deal with it which might be an idea anyway in the context
   of this sort of dodge.
 
  Yes, that's sort of how I expected clk_ignore_unused to work. The way I
  understood it, it would cause all unused clocks to be ignored (that is
  stay enabled if they are).
 
  Of course as Geert pointed out in another subthread, taking this all the
  way means that we have to disable all power management because the
  firmware device may be sharing resources with other devices and which
  therefore are not unused. That's a pretty strong argument and I don't
  have a solution for that. It is only really a problem for cases where
  the firmware virtual device isn't taken over by a proper driver at some
  point, though.
 
 Indeed, and we also run into trouble for things where we actually need
 to really turn off

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Tue, Sep 30, 2014 at 07:00:36PM +0100, Mark Brown wrote:
 On Tue, Sep 30, 2014 at 08:03:14AM +0200, Thierry Reding wrote:
[...]
  Of course as Geert pointed out in another subthread, taking this all the
  way means that we have to disable all power management because the
  firmware device may be sharing resources with other devices and which
  therefore are not unused. That's a pretty strong argument and I don't
  have a solution for that. It is only really a problem for cases where
  the firmware virtual device isn't taken over by a proper driver at some
  point, though.
 
 Indeed, and we also run into trouble for things where we actually need
 to really turn off the resource for some reason (MMC has some needs here
 for example).

Perhaps an alternative would be to just keep power management going and
hope for the best. This may turn out not to be as much of a problem for
many SoCs or boards as people make it out to be.

Thierry


pgpGUTF5zy5fp.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Wed, Oct 01, 2014 at 01:32:50PM +0100, Mark Brown wrote:
 On Wed, Oct 01, 2014 at 01:10:46PM +0200, Javier Martinez Canillas wrote:
[...]
  Adding your resources (clock, regulators, etc) incrementally and only
  when the driver for the device that use these resources is available,
  will only make adding support for a new platform slower IMHO since
  there will be more patches to be posted, reviewed and merged.
 
 So don't do that if you're worried about it then, provide the bits of DT
 that hook everything up from the start or otherwise describe things as
 being in use.

Otherwise describe things as being in use doesn't work for clocks for
example. And Mike already said he wasn't willing to add something like
an always-on DT property for clocks.

Thierry


pgpB5ykJ9w8oI.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Wed, Oct 01, 2014 at 01:20:08PM +0100, Mark Brown wrote:
 On Wed, Oct 01, 2014 at 10:14:44AM +0200, Thierry Reding wrote:
  On Tue, Sep 30, 2014 at 07:00:36PM +0100, Mark Brown wrote:
 
   You may well need to extend the binding in future for an actual driver
   but from the point of view of what's going into the block it really
   should just be a case of reading the datasheet and mechanically typing
   that in.  If we can work out how to say where the framebuffer is we
   really ought to be able to work this stuff out.
 
  I agree from a technical point of view. However given the dynamically
  generated nature of the node the problem is more of a logistical nature.
  As we've seen U-Boot is being enabled to add clocks to the simplefb node
  but I'm fairly certain that there's a regulator somewhere that needs to
  be enabled too, be it for powering the display controller, the panel or
  the backlight. I wouldn't even be surprised if there were one for each
  of those. If so simplefb on this board will break when the regulators
  are described in the kernel's DTS.
 
  If we don't consider this a problem then the whole DT ABI stability
  business is a farce.
 
 I think you're setting constraints on the implementation you want to see
 which make it unworkable but I don't think those constraints are needed.
 You're starting from the position that the DT needs to be updated without
 the bootloader

No, what I'm saying is that what the simplefb driver expects and what
the bootloader sets up may diverge as resource drivers are added to the
kernel. The DT /could/ be updated without the bootloader. You may only
be able to replace the DTB but not the bootloader on a given platform.

and that the DT must not contain any hint of simplefb as
 shipped separately.

Well, I don't think it should because it describes the same resources
that the device tree node for the real device already describes. But
perhaps this is one of the cases where duplication isn't all that bad?

 That's never going to work well as far as I can see
 but doesn't seem like an ABI stability issue, or at least not a
 reasonable one.

It would work well under the assumption that the kernel wouldn't be
touching any of the resources that simplefb depends on. If that's not a
reasonable assumption then I think we can't make simplefb work the way
it's currently written.

 Either the bootloader needs to be updated along with the DT

I thought we had decided that this was one of the big no-nos. But
perhaps I'm misremembering.

 or the DT
 needs to offer the bootloader a stable interface of its own for adding
 the description of what it has set up (like a default disabled node
 with the FB description, I'm sure other ideas are possible).  Obviously
 the goal with the stable ABI is that the DT will be distributed along
 with the platform.

So instead of pretending that this is in any way generic, maybe a better
idea would be to provide code in DRM/KMS drivers that is called early,
grabs all the resources as defined in the binding for the device and
then instantiates simplefb using the parsed information. Which is kind
of the stub driver that Grant had suggested.

Of course that means duplicating most of the resource handling from the
real driver into this stub driver. And it means that this part of the
driver would have to be built into the kernel and bloat it some more.

Thierry


pgpvcvXAotbWP.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 05:11:01PM +0100, Mark Brown wrote:
 On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
  On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
 
   With that said I think that Luc's approach is very sensible. I'm not
   sure what purpose in the universe DT is supposed to serve if not for
   _this_exact_case_. We have a nice abstracted driver, usable by many
   people. The hardware details of how it is hooked up at the board level
   can all be hidden behind stable DT bindings that everyone already uses.
 
  simplefb doesn't deal at all with hardware details. It simply uses what
  firmware has set up, which is the only reason why it will work for many
  people. What is passed in via its device tree node is the minimum amount
  of information needed to draw something into the framebuffer. Also note
  that the simplefb device tree node is not statically added to a DTS file
  but needs to be dynamically generated by firmware at runtime.
 
  If we start extending the binding with board-level details we end up
  duplicating the device tree node for the proper video device. Also note
  that it won't stop at clocks. Other setups will require regulators to be
  listed in this device tree node as well so that they don't get disabled
  at late_initcall. And the regulator bindings don't provide a method to
  list an arbitrary number of clocks in a single property in the way that
  the clocks property works.
 
 Not really thought this through fully yet but would having phandles to
 the relevant devices do the job?  Kind of lines up with Grant's idea of
 having dummy drivers.

One of the arguments that came up during the discussion of the sunxi
patches is that simplefb is going to be used precisely because there is
no real driver for the display part of the SoC yet and nobody knows what
the binding will look like. So there's nothing to point at currently and
for the same reason having a dummy driver won't work. There's simply no
definition of what resources are needed.

  There may be also resets involved. Fortunately the reset framework is
  minimalistic enough not to care about asserting all unused resets at
  late_initcall. And other things like power domains may also need to be
  kept on.
 
 We might want to do that in the future, though it's not always the case
 that reset is the lowest power state.

That proves my point. If we ever decide to assert resets by default
we'll have yet another subsystem that can potentially break existing
DTs.

In the end it brings us back to the very fundamental principles of DT
that's been causing so much pain. For things to work properly and in a
stable way you have to get the bindings right and complete from the
start. That is, it needs to describe every aspect of the hardware block
and all links to other components.

But there is no way you can do that for a virtual device like simplefb
because it is a generic abstraction for hardware that varies wildly. The
only way to make it work is to abstract away all the resource management
and consider it to be done elsewhere.

  So how about instead of requiring resources to be explicitly claimed we
  introduce something like the below patch? The intention being to give
  firmware device drivers a way of signalling to the clock framework
  that they need rely on clocks set up by firmware and when they no longer
  need them. This implements essentially what Mark (CC'ing again on this
  subthread) suggested earlier in this thread. Basically, it will allow
  drivers to determine the time when unused clocks are really unused. It
  will of course only work when used correctly by drivers. For the case of
  simplefb I'd expect its .probe() implementation to call the new
  clk_ignore_unused() function and once it has handed over control of the
  display hardware to the real driver it can call clk_unignore_unused() to
  signal that all unused clocks that it cares about have now been claimed.
  This is reference counted and can therefore be used by more than a
  single driver if necessary. Similar functionality could be added for
  other resource subsystems as needed.
 
 One thing that makes me a bit nervous about this approach in the context
 of the regulator API is the frequency with which one finds shared
 supplies.  I'm not sure if it's actually a big problem in practice but
 it makes me worry a bit.  We could probably just do something like make
 refcounting down to zero not actually disable anything for standard
 regulators to deal with it which might be an idea anyway in the context
 of this sort of dodge.

Yes, that's sort of how I expected clk_ignore_unused to work. The way I
understood it, it would cause all unused clocks to be ignored (that is
stay enabled if they are).

Of course as Geert pointed out in another subthread, taking this all the
way means that we have to disable all power management because the
firmware device may be sharing resources with other devices and which
therefore

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Tue, Sep 30, 2014 at 09:52:58AM +0200, Maxime Ripard wrote:
 On Tue, Sep 30, 2014 at 07:21:11AM +0200, Thierry Reding wrote:
  On Mon, Sep 29, 2014 at 06:28:14PM +0200, Maxime Ripard wrote:
   On Mon, Sep 29, 2014 at 03:47:15PM +0200, Thierry Reding wrote:
[...]
What happened in the Snow example is that regulators that were
previously on would all of a sudden be automatically disabled on boot
because there was now a driver that registered them with a generic
framework.

The same thing is going to happen with simplefb for your device. If you
later realize that you need a regulator to keep the panel going, you'll
have to add code to your firmware to populate the corresponding
properties, otherwise the regulator will end up unused and will be
automatically disabled. At the same time you're going to break upstream
for all users of your old firmware because it doesn't add that property
yet.
   
And the same will continue to happen for every new type of resource
you're going to add.
   
   Sure, we can add any resources we will need. Regulators, reset lines,
   pm domains, allocated memory, but I'm not really sure this is what you
   want, right?
  
  No it's not what I want. *You* want to add resource management to this
  driver. What I'm saying is that if we start adding clocks then we can no
  longer say no to resets or regulators or power domains either.
 
 Yes, because resource management can be more than just keep the thing
 enabled. It might also be about not modifying anything, just like we
 saw for the clocks, but that might also apply to regulators voltage.

We've already determined that simplefb can't do anything meaningful with
the resources other than keep them in the status quo. It simply doesn't
have enough knowledge to do so. It doesn't know the exact pixel clock or
what voltage the attached panel needs.

 And the only way I can think of to deal with that properly is to have
 resources management in the driver.

My point is that if we had a proper way to tell the kernel not to do
anything with resources owned by firmware, then the driver wouldn't
have to do anything with the resources.

   I really start to consider adding a sunxi-uboot-fb, that would just
   duplicate the source code of simplefb but also taking the
   clocks. Somehow, I feel like it will be easier (and definitely less of
   a hack than using the generic common clock API).
  
  You're not getting it are you? What makes you think sunxi-uboot-fb is
  going to be any different? This isn't about a name.
 
 At least, we would get to do any binding and resource management we
 want. And that's a big win.

So instead of trying to understand the concerns that I've expressed and
constructively contribute to finding a solution that works for everybody
you'd rather go and write a driver from scratch. Way to go.

I've already said that I'm not saying strictly no to these patches, but
what I want to see happen is some constructive discussion about whether
we can find better ways to do it. If we can't then I'm all for merging
these patches. Fortunately other (sub)threads have been somewhat more
constructive and actually come up with alternatives that should make
everyone happier.

If you're going to do SoC-specific bindings and resource management you
are in fact implementing what Grant suggested in a subthread. You're
implementing a dummy driver only for resource management, which isn't
really a bad thing. It can serve as a placeholder for now until you add
the real driver. And you can also use the simplefb driver to provide
the framebuffer.

Thierry


pgpwc7ByQwhIp.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Tue, Sep 30, 2014 at 10:03:54AM +0200, Maxime Ripard wrote:
 On Tue, Sep 30, 2014 at 07:39:02AM +0200, Thierry Reding wrote:
  You keep bringing up the Raspberry Pi for some reason and suggest that
  it is somehow inferior to sunxi. What makes you think it's less entitled
  to be supported on Linux than sunxi? I don't care about the Raspberry Pi
  and I equally don't care about sunxi. I don't own a Raspberry Pi and I
  don't own any Allwinner hardware. What I do care about is Linux and I
  want it to work well for all SoCs equally.
  
  Perhaps if you could put aside your crusade against the Raspberry Pi for
  just a second you'll realize that we're all on the same team. This isn't
  a competition and I'm not trying to put a spoke in your wheel. On the
  contrary, I'm actually trying to help you.
 
 We've been over this already, and I'll tell you again that you're
 getting this wrong.
 
 No platform is more entitled to get merged than another one. I do care
 about the Allwinner SoCs, and I care just as much about the broader
 Linux support for all the other SoCs, be it from nvidia, samsung or
 whatever vendor you can come up with.

Okay, I'm glad our goals aren't that far apart then. It would be helpful
to stop dragging the Raspberry Pi into this, though, since it isn't at
all relevant.

 But you can't hide the fact that the bcm2835 still has a very limited
 clock support, and I really don't know about its clock tree, but I
 guess that if the times come when they add a more complete clock
 support, they will face the same issue.

This isn't at all relevant. And that's exactly why I think it's good to
hide all the resource management behind firmware. That way it becomes
easy to support any SoC with any firmware.

 If the driver would have been developped initially to create a
 framebuffer on the Allwinner SoCs, at a time when we didn't have any
 clock support too, calling it only usable on sunxi wouldn't have
 shocked me tbh.

The functionality that it provides is still very generic. And the
firmware interface is generic too. It is this abstraction that allows
it to be generic. You on the other hand seem to be arguing that by
making it abstract we've made it less generic.

Abstraction is about hiding details to capture commonality.

Thierry


pgpAFZSGpCS4j.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Tue, Sep 30, 2014 at 11:38:50AM +0200, Michal Suchanek wrote:
 On 30 September 2014 10:54, Thierry Reding thierry.red...@gmail.com wrote:
  On Tue, Sep 30, 2014 at 09:52:58AM +0200, Maxime Ripard wrote:
  On Tue, Sep 30, 2014 at 07:21:11AM +0200, Thierry Reding wrote:
   On Mon, Sep 29, 2014 at 06:28:14PM +0200, Maxime Ripard wrote:
On Mon, Sep 29, 2014 at 03:47:15PM +0200, Thierry Reding wrote:
  [...]
 What happened in the Snow example is that regulators that were
 previously on would all of a sudden be automatically disabled on boot
 because there was now a driver that registered them with a generic
 framework.

 The same thing is going to happen with simplefb for your device. If 
 you
 later realize that you need a regulator to keep the panel going, 
 you'll
 have to add code to your firmware to populate the corresponding
 properties, otherwise the regulator will end up unused and will be
 automatically disabled. At the same time you're going to break 
 upstream
 for all users of your old firmware because it doesn't add that 
 property
 yet.

 And the same will continue to happen for every new type of resource
 you're going to add.
   
Sure, we can add any resources we will need. Regulators, reset lines,
pm domains, allocated memory, but I'm not really sure this is what you
want, right?
  
   No it's not what I want. *You* want to add resource management to this
   driver. What I'm saying is that if we start adding clocks then we can no
   longer say no to resets or regulators or power domains either.
 
  Yes, because resource management can be more than just keep the thing
  enabled. It might also be about not modifying anything, just like we
  saw for the clocks, but that might also apply to regulators voltage.
 
  We've already determined that simplefb can't do anything meaningful with
  the resources other than keep them in the status quo. It simply doesn't
  have enough knowledge to do so. It doesn't know the exact pixel clock or
  what voltage the attached panel needs.
 
  And the only way I can think of to deal with that properly is to have
  resources management in the driver.
 
  My point is that if we had a proper way to tell the kernel not to do
  anything with resources owned by firmware, then the driver wouldn't
  have to do anything with the resources.
 
 The firmware on sunxi does not own any resources whatsoever. It ceases
 running once it executes the kernel. This is different from ACPI and
 UEFI where you have pieces of the firmware lingering indefinitely and
 potentially getting invoked by user pressing some button or some other
 hardware event. It is also different from rpi where the Linux kernel
 effectively runs in a virtual environment created by the firmware
 hypervisor.

You know all that because you of course wrote every single firmware
implementation that does and will ever exist for sunxi. There's nothing
keeping anyone from running UEFI on a sunxi SoC.

 So on sunxi and many other ARM machines the Linux kernel is the sole
 owner of any resources that might happen to be available on the
 machine. There is no firmware owning them when the Linux kernel is
 running, ever.

Of course this is part of the abstraction. The idea is that the device
is a virtual one created by firmware. Therefore firmware owns the
resources until the virtual device has been handed over to the kernel.

If you're into splitting hairs, then the simplefb device shouldn't exist
in the first place.

 And we do have a proper way to tell to the kernel what these resources
 are used for - inserting description of them into the simplefb DT
 node. Sure the simplefb cannot manage the resources in any way and but
 it does own them. When it is running they are in use, when it stops
 they are free to be reclaimed by the platform driver.

Yes. And again I'm not saying anything different. What I'm saying is
that we shouldn't need to know about the resources and instead hide that
within the firmware, for the same reason that we're already hiding the
register programming in hardware, namely to create an abstraction that
works irrespective of the underlying hardware.

 But you refuse to marge the kernel change for this proper management
 to happen.

I have no authority to merge these changes nor have I any way of vetoing
them. All I'm saying is that I think it's a bad idea. If nobody else
thinks it is then it will eventually get merged irrespective of what I'm
saying. And when that happens I'll shut up and live happily ever after.
But it doesn't magically convince me that it's a good idea.

 The alternate proposal to stop the kernel from managing resources
 while simplefb is running and refernce count drivers that cannot
 manage resources is indeed a workable, general alternative.
 
 It does however switch the kernel into special mode when resources are
 not managed and will needlessly hinder eg. development and testing

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Tue, Sep 30, 2014 at 01:43:45PM +0200, Hans de Goede wrote:
 Hi,
 
 On 09/30/2014 06:59 AM, Thierry Reding wrote:
  On Mon, Sep 29, 2014 at 05:57:18PM +0200, Maxime Ripard wrote:
 
 snip
 
  But sure, you can still try to point new issues, get an obvious and
  robust solution, and then discard the issue when the solution doesn't
  go your way...
  
  And you've already proven that you're completely unwilling to even
  consider any other solution than what was originally proposed, so I
  really don't see how discussing this further with you is going to be
  productive.
 
 That is not true, we have seriously considered various other alternatives,
 as you know since you've participated in the discussion about them.
 
 And we've found them all lacking, mostly because they are 10 times as
 complicated.
 
 You've made your point that you don't like this solution quite loudly
 already, and we've all heard you. However you seem to be mostly alone in
 this. Even the clk maintainer has said that what we want to do is
 exactly how clocks are supposed to be used in dt.
 
 If you don't like this no-one is forcing you to use the clocks property
 in your own code. If it is not there, simplefb will behave exactly as
 before.
 
 Now since you're the only one very vocally against this, and a lot
 of people are in favor of this and have a need for this, can we
 please just get this merged and get this over with ?

Whatever. I no longer care.

Thierry


pgpUQ_XRIPqPR.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
 Quoting Maxime Ripard (2014-09-02 02:25:08)
  On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote:
   On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
 I would think the memory should still be reserved anyway to make sure
 nothing else is writing over it. And it's in the device tree anyway
 because the driver needs to know where to put framebuffer content. So
 the point I was trying to make is that we can't treat the memory in 
 the
 same way as clocks because it needs to be explicitly managed. Whereas
 clocks don't. The driver is simply too generic to know what to do with
 the clocks.

You agreed on the fact that the only thing we need to do with the
clocks is claim them. Really, I don't find what's complicated there
(or not generic).
   
   That's not what I agreed on. What I said is that the only thing we need
   to do with the clocks is nothing. They are already in the state that
   they need to be.
  
  Claim was probably a poor choice of words, but still. We have to keep
  the clock running, and both the solution you've been giving and this
  patch do so in a generic way.
  
 It doesn't know what frequency they should be running at

We don't care about that. Just like we don't care about which
frequency is the memory bus running at. It will just run at whatever
frequency is appropriate.
   
   Exactly. And you shouldn't have to care about them at all. Firmware has
   already configured the clocks to run at the correct frequencies, and it
   has made sure that they are enabled.
   
 or what they're used for

And we don't care about that either. You're not interested in what
output the framebuffer is setup to use, which is pretty much the same
here, this is the same thing here.
   
   That's precisely what I've been saying. The only thing that simplefb
   cares about is the memory it should be using and the format of the
   pixels (and how many of them) it writes into that memory. Everything
   else is assumed to have been set up.
   
   Including clocks.
  
  We're really discussing in circles here.
  
  Mike?
  
 
 -EHIGHLATENCYRESPONSE
 
 I forgot about this thread. Sorry.
 
 In an attempt to provide the least helpful answer possible, I will stay
 clear of all of the stuff relating to how simple should simplefb be
 and the related reserved memory discussion.
 
 A few times in this thread it is stated that we can't prevent unused
 clocks from being disabled. That is only partially true.
 
 The clock framework DOES provide a flag to prevent UNUSED clocks from
 being disabled at late_initcall-time by a clock garbage collector
 mechanism. Maxime and others familiar with the clock framework are aware
 of this.
 
 What the framework doesn't do is to allow for a magic flag in DT, in
 platform_data or elsewhere that says, don't let me get turned off until
 the right driver claims me. That would be an external or alternative
 method for preventing a clock from being disabled. We have a method for
 preventing clocks from being disabled. It is as follows:
 
 struct clk *my_clk = clk_get(...);
 clk_prepare_enable(my_clk);
 
 That is how it should be done. Period.
 
 With that said I think that Luc's approach is very sensible. I'm not
 sure what purpose in the universe DT is supposed to serve if not for
 _this_exact_case_. We have a nice abstracted driver, usable by many
 people. The hardware details of how it is hooked up at the board level
 can all be hidden behind stable DT bindings that everyone already uses.

simplefb doesn't deal at all with hardware details. It simply uses what
firmware has set up, which is the only reason why it will work for many
people. What is passed in via its device tree node is the minimum amount
of information needed to draw something into the framebuffer. Also note
that the simplefb device tree node is not statically added to a DTS file
but needs to be dynamically generated by firmware at runtime.

If we start extending the binding with board-level details we end up
duplicating the device tree node for the proper video device. Also note
that it won't stop at clocks. Other setups will require regulators to be
listed in this device tree node as well so that they don't get disabled
at late_initcall. And the regulator bindings don't provide a method to
list an arbitrary number of clocks in a single property in the way that
the clocks property works.

There may be also resets involved. Fortunately the reset framework is
minimalistic enough not to care about asserting all unused resets at
late_initcall. And other things like power domains may also need to be
kept on.

Passing in clock information via the device tree already requires a non-
trivial amount of code in the firmware. A similar amount of code would
be necessary for each type

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 10:27:41AM +0200, Geert Uytterhoeven wrote:
 Hi Thierry,
 
 (CC linux-pm, as PM is the real reason behind disabling unused clocks)
 (CC gregkh and lkml, for driver core)
 
 On Mon, Sep 29, 2014 at 10:06 AM, Thierry Reding
 thierry.red...@gmail.com wrote:
  On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
  Quoting Maxime Ripard (2014-09-02 02:25:08)
   On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote:
On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
 On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
  I would think the memory should still be reserved anyway to make 
  sure
  nothing else is writing over it. And it's in the device tree anyway
  because the driver needs to know where to put framebuffer content. 
  So
  the point I was trying to make is that we can't treat the memory 
  in the
  same way as clocks because it needs to be explicitly managed. 
  Whereas
  clocks don't. The driver is simply too generic to know what to do 
  with
  the clocks.

 You agreed on the fact that the only thing we need to do with the
 clocks is claim them. Really, I don't find what's complicated there
 (or not generic).
   
That's not what I agreed on. What I said is that the only thing we need
to do with the clocks is nothing. They are already in the state that
they need to be.
  
   Claim was probably a poor choice of words, but still. We have to keep
   the clock running, and both the solution you've been giving and this
   patch do so in a generic way.
  
  It doesn't know what frequency they should be running at

 We don't care about that. Just like we don't care about which
 frequency is the memory bus running at. It will just run at whatever
 frequency is appropriate.
   
Exactly. And you shouldn't have to care about them at all. Firmware has
already configured the clocks to run at the correct frequencies, and it
has made sure that they are enabled.
   
  or what they're used for

 And we don't care about that either. You're not interested in what
 output the framebuffer is setup to use, which is pretty much the same
 here, this is the same thing here.
   
That's precisely what I've been saying. The only thing that simplefb
cares about is the memory it should be using and the format of the
pixels (and how many of them) it writes into that memory. Everything
else is assumed to have been set up.
   
Including clocks.
  
   We're really discussing in circles here.
  
   Mike?
  
 
  -EHIGHLATENCYRESPONSE
 
  I forgot about this thread. Sorry.
 
  In an attempt to provide the least helpful answer possible, I will stay
  clear of all of the stuff relating to how simple should simplefb be
  and the related reserved memory discussion.
 
  A few times in this thread it is stated that we can't prevent unused
  clocks from being disabled. That is only partially true.
 
  The clock framework DOES provide a flag to prevent UNUSED clocks from
  being disabled at late_initcall-time by a clock garbage collector
  mechanism. Maxime and others familiar with the clock framework are aware
  of this.
 
  What the framework doesn't do is to allow for a magic flag in DT, in
  platform_data or elsewhere that says, don't let me get turned off until
  the right driver claims me. That would be an external or alternative
  method for preventing a clock from being disabled. We have a method for
  preventing clocks from being disabled. It is as follows:
 
  struct clk *my_clk = clk_get(...);
  clk_prepare_enable(my_clk);
 
  That is how it should be done. Period.
 
  With that said I think that Luc's approach is very sensible. I'm not
  sure what purpose in the universe DT is supposed to serve if not for
  _this_exact_case_. We have a nice abstracted driver, usable by many
  people. The hardware details of how it is hooked up at the board level
  can all be hidden behind stable DT bindings that everyone already uses.
 
  simplefb doesn't deal at all with hardware details. It simply uses what
  firmware has set up, which is the only reason why it will work for many
 
 It doesn't deal with hardware details for hardware components for which
 no driver is available (yet). That's why the hardware is still in a usable
 state, after the firmware has set it up.
 
 Clocks, regulators, PM domains typically have system-wide implications,
 and thus need system-wide drivers (in the absence of such drivers,
 things would work as-is).
 
 Note that the driver still requests resources (ioremap the frame buffer),
 so it needs to know about that tiny piece of hardware detail.

That's not a hardware detail. Or at least it isn't hardware specific. It
is needed and the same irrespective of display hardware. Clocks, power
domains, regulators and all those are not always the same.

  people. What is passed in via its device tree

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 11:10:53AM +0200, Geert Uytterhoeven wrote:
 Hi Thierry,
 
 On Mon, Sep 29, 2014 at 10:54 AM, Thierry Reding
 thierry.red...@gmail.com wrote:
  On Mon, Sep 29, 2014 at 10:27:41AM +0200, Geert Uytterhoeven wrote:
  (CC linux-pm, as PM is the real reason behind disabling unused clocks)
  (CC gregkh and lkml, for driver core)
 
  On Mon, Sep 29, 2014 at 10:06 AM, Thierry Reding
  thierry.red...@gmail.com wrote:
   If we start extending the binding with board-level details we end up
   duplicating the device tree node for the proper video device. Also note
   that it won't stop at clocks. Other setups will require regulators to be
   listed in this device tree node as well so that they don't get disabled
   at late_initcall. And the regulator bindings don't provide a method to
   list an arbitrary number of clocks in a single property in the way that
   the clocks property works.
 
  Then (optional) regulator support needs to be added.
 
  Can you elaborate?
 
 I'm not so familiar with regulators, but I guess it's similar to clocks?

The bindings are different. Essentially what you use is a *-supply
property per regulator. There is no way to specify more than one
regulator in a single property.

So if you want to keep that generic you have to do crazy things like:

simplefb {
enable-0-supply = reg1;
enable-1-supply = reg2;
...
};

I suppose a more generic supplies property could be created to support
this use-case, but I think this kind of proves my point. The only way
that the original proposal is going to work for other resources is if
they follow the same kind of binding. I don't think it makes sense to
introduce such a prerequisite merely because it would make life easy
for some exotic driver with a very specific application.

  And then all of a sudden something that was supposed to be simple and
  generic needs to know the specifics of some hardware device.
 
 And suddenly we wish we could write a real driver and put the stuff in
 the DTS, not DTB...

Oh, there's no doubt a real driver would be preferrable. Note that
simplefb is only meant to be a shim to pass a framebuffer from firmware
to kernel. In some cases it can be used with longer lifetime, like for
example if you really want to have graphical output but the real driver
isn't there yet.

Being a shim driver is precisely the reason why I think the binding
shouldn't be extended to cover all possible types of resources. That
should all go into the binding for the real device.

   The only reasonable thing for simplefb to do is not deal with any kind
   of resource at all (except perhaps area that contains the framebuffer
   memory).
  
   So how about instead of requiring resources to be explicitly claimed we
   introduce something like the below patch? The intention being to give
   firmware device drivers a way of signalling to the clock framework
   that they need rely on clocks set up by firmware and when they no longer
   need them. This implements essentially what Mark (CC'ing again on this
   subthread) suggested earlier in this thread. Basically, it will allow
   drivers to determine the time when unused clocks are really unused. It
   will of course only work when used correctly by drivers. For the case of
   simplefb I'd expect its .probe() implementation to call the new
   clk_ignore_unused() function and once it has handed over control of the
   display hardware to the real driver it can call clk_unignore_unused() to
   signal that all unused clocks that it cares about have now been claimed.
   This is reference counted and can therefore be used by more than a
   single driver if necessary. Similar functionality could be added for
   other resource subsystems as needed.
 
  This still won't work for modules, right? Or am I missing something?
  With modules you will never know in advance what will be used and what
  won't be used, so you need to keep all clocks, regulators, PM domains, ...
  up and running?
 
  No. The way this works is that your firmware shim driver, simplefb in
  this case, will call clk_ignore_unused() to tell the clock framework
  that it uses clocks set up by the firmware, and therefore requests that
  no clocks should be considered unused (for now). Later on when the
  proper driver has successfully taken over from the shim driver, the shim
  driver can unregister itself and call clk_unignore_unused(), which will
  drop its reference on the unused clocks. When all references have been
  dropped the clock framework will then disable all remaining unused
  clocks.
 
 So the shim must be built-in, not modular.

Correct. Making it a module isn't very useful in my opinion. You'd loose
all the advantages.

Thierry


pgpKAyGHybPqz.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:
 On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
[...]
  simplefb doesn't deal at all with hardware details. It simply uses what
  firmware has set up, which is the only reason why it will work for many
  people. What is passed in via its device tree node is the minimum amount
  of information needed to draw something into the framebuffer. Also note
  that the simplefb device tree node is not statically added to a DTS file
  but needs to be dynamically generated by firmware at runtime.
 
 Which makes the whole even simpler, since the firmware already knows
 all about which clocks it had to enable.

It makes things very complicated in the firmware because it now needs to
be able to generate DTB content that we would otherwise be able to do
much easier with a text editor.

  If we start extending the binding with board-level details we end up
  duplicating the device tree node for the proper video device. Also note
  that it won't stop at clocks. Other setups will require regulators to be
  listed in this device tree node as well so that they don't get disabled
  at late_initcall. And the regulator bindings don't provide a method to
  list an arbitrary number of clocks in a single property in the way that
  the clocks property works.
  
  There may be also resets involved. Fortunately the reset framework is
  minimalistic enough not to care about asserting all unused resets at
  late_initcall. And other things like power domains may also need to be
  kept on.
  
  Passing in clock information via the device tree already requires a non-
  trivial amount of code in the firmware. A similar amount of code would
  be necessary for each type of resource that needs to be kept enabled. In
  addition to the above some devices may also require resources that have
  no generic bindings. That just doesn't scale.
  
  The only reasonable thing for simplefb to do is not deal with any kind
  of resource at all (except perhaps area that contains the framebuffer
  memory).
 
 You should really read that thread:
 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-September/284726.html
 
 It's quite interesting, because you'll see that:
 A) Your approach, even on the platform you're working on, doesn't
work. Or at least, isn't reliable.

What platform exactly do you think I'm working on? Why do you think what
I proposed isn't going to work or be reliable? I don't see any arguments
in the thread that would imply that.

 B) Other maintainers, precisely like Mark, came to the same conclusion
than Mike.

Well, and others didn't.

Also I think if you read that thread and look at my proposal it matches
exactly what was discussed as one of the solutions at one point in the
thread.

  So how about instead of requiring resources to be explicitly claimed we
  introduce something like the below patch? The intention being to give
  firmware device drivers a way of signalling to the clock framework
  that they need rely on clocks set up by firmware and when they no longer
  need them. This implements essentially what Mark (CC'ing again on this
  subthread) suggested earlier in this thread. Basically, it will allow
  drivers to determine the time when unused clocks are really unused. It
  will of course only work when used correctly by drivers. For the case of
  simplefb I'd expect its .probe() implementation to call the new
  clk_ignore_unused() function and once it has handed over control of the
  display hardware to the real driver it can call clk_unignore_unused() to
  signal that all unused clocks that it cares about have now been claimed.
  This is reference counted and can therefore be used by more than a
  single driver if necessary. Similar functionality could be added for
  other resource subsystems as needed.
 
 So, just to be clear, instead of doing a generic clk_get and
 clk_prepare_enable, you're willing to do a just as much generic
 clk_ignore_unused call?

Yes.

 How is that less generic?

It's more generic. That's the whole point.

The difference is that with the solution I proposed we don't have to
keep track of all the resources. We know that firmware has set them up
and we know that a real driver will properly take them over at some
point, so duplicating what the real driver does within the simplefb
driver is just that, duplication. We don't allow duplication anywhere
else in the kernel, why should simplefb be an exception?

 You know that you are going to call that for regulator, reset, power
 domains, just as you would have needed to with the proper API, unless
 that with this kind of solution, you would have to modify *every*
 framework that might interact with any resource involved in getting
 simplefb running?

We have to add handling for every kind of resource either way. Also if
this evolves into a common pattern we can easily wrap it up in a single
function call.

 Plus, speaking more specifically about the clocks

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 11:44:19AM +0200, Michal Suchanek wrote:
 On 29 September 2014 10:54, Thierry Reding thierry.red...@gmail.com wrote:
  On Mon, Sep 29, 2014 at 10:27:41AM +0200, Geert Uytterhoeven wrote:
  Hi Thierry,
 
  (CC linux-pm, as PM is the real reason behind disabling unused clocks)
  (CC gregkh and lkml, for driver core)
 
  On Mon, Sep 29, 2014 at 10:06 AM, Thierry Reding
  thierry.red...@gmail.com wrote:
   On Sat, Sep 27, 2014 at 04:56:01PM -0700, Mike Turquette wrote:
   Quoting Maxime Ripard (2014-09-02 02:25:08)
On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote:
 On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
  On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
   I would think the memory should still be reserved anyway to 
   make sure
   nothing else is writing over it. And it's in the device tree 
   anyway
   because the driver needs to know where to put framebuffer 
   content. So
   the point I was trying to make is that we can't treat the 
   memory in the
   same way as clocks because it needs to be explicitly managed. 
   Whereas
   clocks don't. The driver is simply too generic to know what to 
   do with
   the clocks.
 
  You agreed on the fact that the only thing we need to do with the
  clocks is claim them. Really, I don't find what's complicated 
  there
  (or not generic).

 That's not what I agreed on. What I said is that the only thing we 
 need
 to do with the clocks is nothing. They are already in the state that
 they need to be.
   
Claim was probably a poor choice of words, but still. We have to keep
the clock running, and both the solution you've been giving and this
patch do so in a generic way.
   
   It doesn't know what frequency they should be running at
 
  We don't care about that. Just like we don't care about which
  frequency is the memory bus running at. It will just run at 
  whatever
  frequency is appropriate.

 Exactly. And you shouldn't have to care about them at all. Firmware 
 has
 already configured the clocks to run at the correct frequencies, 
 and it
 has made sure that they are enabled.

   or what they're used for
 
  And we don't care about that either. You're not interested in what
  output the framebuffer is setup to use, which is pretty much the 
  same
  here, this is the same thing here.

 That's precisely what I've been saying. The only thing that simplefb
 cares about is the memory it should be using and the format of the
 pixels (and how many of them) it writes into that memory. Everything
 else is assumed to have been set up.

 Including clocks.
   
We're really discussing in circles here.
   
Mike?
   
  
   -EHIGHLATENCYRESPONSE
  
   I forgot about this thread. Sorry.
  
   In an attempt to provide the least helpful answer possible, I will stay
   clear of all of the stuff relating to how simple should simplefb be
   and the related reserved memory discussion.
  
   A few times in this thread it is stated that we can't prevent unused
   clocks from being disabled. That is only partially true.
  
   The clock framework DOES provide a flag to prevent UNUSED clocks from
   being disabled at late_initcall-time by a clock garbage collector
   mechanism. Maxime and others familiar with the clock framework are aware
   of this.
  
   What the framework doesn't do is to allow for a magic flag in DT, in
   platform_data or elsewhere that says, don't let me get turned off until
   the right driver claims me. That would be an external or alternative
   method for preventing a clock from being disabled. We have a method for
   preventing clocks from being disabled. It is as follows:
  
   struct clk *my_clk = clk_get(...);
   clk_prepare_enable(my_clk);
  
   That is how it should be done. Period.
  
   With that said I think that Luc's approach is very sensible. I'm not
   sure what purpose in the universe DT is supposed to serve if not for
   _this_exact_case_. We have a nice abstracted driver, usable by many
   people. The hardware details of how it is hooked up at the board level
   can all be hidden behind stable DT bindings that everyone already uses.
  
   simplefb doesn't deal at all with hardware details. It simply uses what
   firmware has set up, which is the only reason why it will work for many
 
  It doesn't deal with hardware details for hardware components for which
  no driver is available (yet). That's why the hardware is still in a usable
  state, after the firmware has set it up.
 
  Clocks, regulators, PM domains typically have system-wide implications,
  and thus need system-wide drivers (in the absence of such drivers,
  things would work as-is).
 
  Note that the driver still requests resources (ioremap the frame buffer),
  so it needs to know

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 12:35:17PM +0200, Geert Uytterhoeven wrote:
 Hi Thierry,
 
 On Mon, Sep 29, 2014 at 12:18 PM, Thierry Reding
 thierry.red...@gmail.com wrote:
  How is that less generic?
 
  It's more generic. That's the whole point.
 
  The difference is that with the solution I proposed we don't have to
  keep track of all the resources. We know that firmware has set them up
  and we know that a real driver will properly take them over at some
  point, so duplicating what the real driver does within the simplefb
  driver is just that, duplication. We don't allow duplication anywhere
  else in the kernel, why should simplefb be an exception?
 
  You know that you are going to call that for regulator, reset, power
  domains, just as you would have needed to with the proper API, unless
  that with this kind of solution, you would have to modify *every*
  framework that might interact with any resource involved in getting
  simplefb running?
 
  We have to add handling for every kind of resource either way. Also if
  this evolves into a common pattern we can easily wrap it up in a single
  function call.
 
 disable_all_power_management(), as this is not limited to clocks.

Right. But it isn't all power management either. It just shouldn't turn
everything unused off. Clocks, regulators, power domains and so on which
are used can very well be power managed.

  Plus, speaking more specifically about the clocks, that won't prevent
  your clock to be shut down as a side effect of a later clk_disable
  call from another driver.
 
  Furthermore isn't it a bug for a driver to call clk_disable() before a
  preceding clk_enable()? There are patches being worked on that will
  enable per-user clocks and as I understand it they will specifically
  disallow drivers to disable the hardware clock if other drivers are
  still keeping them on via their own referenc.
 
 Calling clk_disable() preceding clk_enable() is a bug.
 
 Calling clk_disable() after clk_enable() will disable the clock (and
 its parents)
 if the clock subsystem thinks there are no other users, which is what will
 happen here.

Right. I'm not sure this is really applicable to this situation, though.
Either way, if there are other users of a clock then they will just as
likely want to modify the rate at which point simplefb will break just
as badly.

Thierry


pgpxfSBHORkJm.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 09:00:01PM +1000, Julian Calaby wrote:
 Hi Thierry,

If you address people directly please make sure they are in the To:
line. Or at least Cc.

 On Mon, Sep 29, 2014 at 8:18 PM, Thierry Reding
 thierry.red...@gmail.com wrote:
  On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:
  On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
  [...]
   simplefb doesn't deal at all with hardware details. It simply uses what
   firmware has set up, which is the only reason why it will work for many
   people. What is passed in via its device tree node is the minimum amount
   of information needed to draw something into the framebuffer. Also note
   that the simplefb device tree node is not statically added to a DTS file
   but needs to be dynamically generated by firmware at runtime.
 
  Which makes the whole even simpler, since the firmware already knows
  all about which clocks it had to enable.
 
  It makes things very complicated in the firmware because it now needs to
  be able to generate DTB content that we would otherwise be able to do
  much easier with a text editor.
 
 As far as the kernel is concerned, this is a solved problem.

It's not completely solved. There's still the issue of no generic way to
specify regulators like you can do for clocks, resets or power domains.

But the kernel isn't the real issue here. The issue is the firmware that
now has to go out of its way not only to initialize display hardware but
also create device tree content just to make Linux not turn everything
off.

 Firmware is going to be doing some dark magic to set up the hardware
 to be a dumb frame buffer and some other stuff to add the simplefb
 device node - so by this point, adding the clocks (or whatever)
 required by the hardware should be fairly uncomplicated - the firmware
 already knows the hardware intimately. As for the actual device tree
 manipulations, U-boot (or whatever) will probably just grow some
 helper functions to make this simple.

Have you looked at the code needed to do this? It's not at all trivial.
And the point is really that all this information is there already, so
we're completely duplicating this into a dynamically created device tree
node and for what reason? Only to have one driver request all these
resources and have them forcefully released a few seconds later.

 Alternatively, it could simply add the relevant data to an existing
 device node and munge it's compatible property so simplefb picks it
 up.

Yes, I think that'd be a much better solution. Of course it's going to
be very difficult to make that work with a generic driver because now
that generic driver needs to parse the DT binding for any number of
compatible devices.

Thierry


pgpXeaVvCkSNz.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 01:46:43PM +0200, Maxime Ripard wrote:
 On Mon, Sep 29, 2014 at 12:18:08PM +0200, Thierry Reding wrote:
  On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:
   On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
  [...]
simplefb doesn't deal at all with hardware details. It simply uses what
firmware has set up, which is the only reason why it will work for many
people. What is passed in via its device tree node is the minimum amount
of information needed to draw something into the framebuffer. Also note
that the simplefb device tree node is not statically added to a DTS file
but needs to be dynamically generated by firmware at runtime.
   
   Which makes the whole even simpler, since the firmware already knows
   all about which clocks it had to enable.
  
  It makes things very complicated in the firmware because it now needs to
  be able to generate DTB content that we would otherwise be able to do
  much easier with a text editor.
 
 Didn't you just say that it was dynamically generated at runtime? So
 we can just ignore the text editor case.

Perhaps read the sentence again. I said that we would *otherwise* be
able to do much easier with a text editor..

My point remains that there shouldn't be a need to generate DTB content
of this complexity at all.

  Why do you think what I proposed isn't going to work or be reliable?
  I don't see any arguments in the thread that would imply that.
 
 The fact that it broke in the first place?

That's exactly the point. And it's going to break again and again as
simplefb is extended with new things. Generally DT bindings should be
backwards compatible. When extended they should provide a way to fall
back to a reasonable default. There's simply no way you can do that
with simplefb.

What happened in the Snow example is that regulators that were
previously on would all of a sudden be automatically disabled on boot
because there was now a driver that registered them with a generic
framework.

The same thing is going to happen with simplefb for your device. If you
later realize that you need a regulator to keep the panel going, you'll
have to add code to your firmware to populate the corresponding
properties, otherwise the regulator will end up unused and will be
automatically disabled. At the same time you're going to break upstream
for all users of your old firmware because it doesn't add that property
yet.

And the same will continue to happen for every new type of resource
you're going to add.

So how about instead of requiring resources to be explicitly claimed we
introduce something like the below patch? The intention being to give
firmware device drivers a way of signalling to the clock framework
that they need rely on clocks set up by firmware and when they no longer
need them. This implements essentially what Mark (CC'ing again on this
subthread) suggested earlier in this thread. Basically, it will allow
drivers to determine the time when unused clocks are really unused. It
will of course only work when used correctly by drivers. For the case of
simplefb I'd expect its .probe() implementation to call the new
clk_ignore_unused() function and once it has handed over control of the
display hardware to the real driver it can call clk_unignore_unused() to
signal that all unused clocks that it cares about have now been claimed.
This is reference counted and can therefore be used by more than a
single driver if necessary. Similar functionality could be added for
other resource subsystems as needed.
   
   So, just to be clear, instead of doing a generic clk_get and
   clk_prepare_enable, you're willing to do a just as much generic
   clk_ignore_unused call?
  
  Yes.
  
   How is that less generic?
  
  It's more generic. That's the whole point.
  
  The difference is that with the solution I proposed we don't have to
  keep track of all the resources. We know that firmware has set them up
  and we know that a real driver will properly take them over at some
  point
 
 You keep saying that... and you know that you can't make this
 assumption.

Why not? Are you really expecting to keep running with simplefb forever?
Nobody is going to seriously use an upstream kernel in any product with
only simplefb as a framebuffer. I've said before that this is a hack to
get you working display. And that's all it is. If you want to do it
properly go and write a DRM/KMS driver.

   You know that you are going to call that for regulator, reset, power
   domains, just as you would have needed to with the proper API, unless
   that with this kind of solution, you would have to modify *every*
   framework that might interact with any resource involved in getting
   simplefb running?
  
  We have to add handling for every kind of resource either way. Also if
  this evolves into a common pattern we can easily wrap it up in a single
  function call.
 
 Unless

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 01:34:36PM +0200, Maxime Ripard wrote:
 On Mon, Sep 29, 2014 at 12:44:57PM +0200, Thierry Reding wrote:
Plus, speaking more specifically about the clocks, that won't prevent
your clock to be shut down as a side effect of a later clk_disable
call from another driver.
   
Furthermore isn't it a bug for a driver to call clk_disable() before a
preceding clk_enable()? There are patches being worked on that will
enable per-user clocks and as I understand it they will specifically
disallow drivers to disable the hardware clock if other drivers are
still keeping them on via their own referenc.
   
   Calling clk_disable() preceding clk_enable() is a bug.
   
   Calling clk_disable() after clk_enable() will disable the clock (and
   its parents)
   if the clock subsystem thinks there are no other users, which is what will
   happen here.
  
  Right. I'm not sure this is really applicable to this situation, though.
 
 It's actually very easy to do. Have a driver that probes, enables its
 clock, fails to probe for any reason, call clk_disable in its exit
 path. If there's no other user at that time of this particular clock
 tree, it will be shut down. Bam. You just lost your framebuffer.
 
 Really, it's just that simple, and relying on the fact that some other
 user of the same clock tree will always be their is beyond fragile.

Perhaps the meaning clk_ignore_unused should be revised, then. What you
describe isn't at all what I'd expect from such an option. And it does
not match the description in Documentation/kernel-parameters.txt either.

  Either way, if there are other users of a clock then they will just as
  likely want to modify the rate at which point simplefb will break just
  as badly.
 
 And this can be handled just as well. Register a clock notifier,
 refuse any rate change, done. But of course, that would require having
 a clock handle.
 
 Now, how would *you* prevent such a change?

Like I said in the other thread. If you have two drivers that use the
same clock but need different frequencies you've lost anyway.

Thierry


pgpKPih4ESVw2.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 01:32:44PM +0200, Geert Uytterhoeven wrote:
 Hi Thierry,
 
 On Mon, Sep 29, 2014 at 12:44 PM, Thierry Reding
 thierry.red...@gmail.com wrote:
   You know that you are going to call that for regulator, reset, power
   domains, just as you would have needed to with the proper API, unless
   that with this kind of solution, you would have to modify *every*
   framework that might interact with any resource involved in getting
   simplefb running?
  
   We have to add handling for every kind of resource either way. Also if
   this evolves into a common pattern we can easily wrap it up in a single
   function call.
 
  disable_all_power_management(), as this is not limited to clocks.
 
  Right. But it isn't all power management either. It just shouldn't turn
  everything unused off. Clocks, regulators, power domains and so on which
  are used can very well be power managed.
 
 No they can't, as the clock/regulator/PM domain core cannot know if any
 of the used ones are also used by a shim driver like simplefb.
 Clocks and regulators may be shared. PM domains can contain multiple
 hardware blocks. Without more knowledge, the only safe thing is not
 disabling anything.

Indeed. That's a shame. In the most common case that probably won't
matter all that much, given that the real driver can be expected to load
within a reasonable amount of time.

   Plus, speaking more specifically about the clocks, that won't prevent
   your clock to be shut down as a side effect of a later clk_disable
   call from another driver.
 
   Furthermore isn't it a bug for a driver to call clk_disable() before a
   preceding clk_enable()? There are patches being worked on that will
   enable per-user clocks and as I understand it they will specifically
   disallow drivers to disable the hardware clock if other drivers are
   still keeping them on via their own referenc.
 
  Calling clk_disable() preceding clk_enable() is a bug.
 
  Calling clk_disable() after clk_enable() will disable the clock (and
  its parents)
  if the clock subsystem thinks there are no other users, which is what will
  happen here.
 
  Right. I'm not sure this is really applicable to this situation, though.
 
 Yes it is: if all users of a clock/regulator/PM domain are gone, it will
 be disabled. Bad luck for simplefb still needing them.

Hmm... if all users are gone, then aren't the resources unused again and
should therefore be ignored?

Thierry


pgpIoQc_lLPua.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Tue, Sep 30, 2014 at 12:46:11AM +1000, Julian Calaby wrote:
 Hi Thierry,
 
 On Mon, Sep 29, 2014 at 11:21 PM, Thierry Reding
 thierry.red...@gmail.com wrote:
  On Mon, Sep 29, 2014 at 09:00:01PM +1000, Julian Calaby wrote:
  Hi Thierry,
 
  If you address people directly please make sure they are in the To:
  line. Or at least Cc.
 
 Sorry about that, the mailing list I received this through (Google
 Groups based) generally strips to: and CC: lines, so my mail client
 (Gmail) doesn't do it automatically. I'm still getting used to it.

Yeah, I wish mailing lists would stop doing that.

  On Mon, Sep 29, 2014 at 8:18 PM, Thierry Reding
  thierry.red...@gmail.com wrote:
   On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:
   On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
   [...]
simplefb doesn't deal at all with hardware details. It simply uses 
what
firmware has set up, which is the only reason why it will work for 
many
people. What is passed in via its device tree node is the minimum 
amount
of information needed to draw something into the framebuffer. Also 
note
that the simplefb device tree node is not statically added to a DTS 
file
but needs to be dynamically generated by firmware at runtime.
  
   Which makes the whole even simpler, since the firmware already knows
   all about which clocks it had to enable.
  
   It makes things very complicated in the firmware because it now needs to
   be able to generate DTB content that we would otherwise be able to do
   much easier with a text editor.
 
  As far as the kernel is concerned, this is a solved problem.
 
  It's not completely solved. There's still the issue of no generic way to
  specify regulators like you can do for clocks, resets or power domains.
  But the kernel isn't the real issue here. The issue is the firmware that
  now has to go out of its way not only to initialize display hardware but
  also create device tree content just to make Linux not turn everything
  off.
 
 My point is that the firmware is going to be doing complicated stuff
 already, adding and using some helpers to configure a device tree node
 is relatively simple in comparison to dealing with the actual
 hardware. It wouldn't surprise me if u-boot, for example, ended up
 with a set of functions to handle this exact case as more graphics
 hardware gets brought up.

Not all firmware is based on U-Boot. Essentially whatever binding
changes we make will need to be implemented in all firmware. And the
complexity isn't so much about writing the actual DT data, but more
about figuring out which data to write. Every firmware image needs to
know exactly which clocks and other resources to transcribe for a given
board. It'll essentially need to contain some sort of driver for each
device that parses a DTB, correlates the data to what it knows of the
device internals and write a subset of that data back into the DTB in a
slightly different format. That's just whacky.

DT was meant to simplify things.

  Firmware is going to be doing some dark magic to set up the hardware
  to be a dumb frame buffer and some other stuff to add the simplefb
  device node - so by this point, adding the clocks (or whatever)
  required by the hardware should be fairly uncomplicated - the firmware
  already knows the hardware intimately. As for the actual device tree
  manipulations, U-boot (or whatever) will probably just grow some
  helper functions to make this simple.
 
  Have you looked at the code needed to do this? It's not at all trivial.
  And the point is really that all this information is there already, so
  we're completely duplicating this into a dynamically created device tree
  node and for what reason? Only to have one driver request all these
  resources and have them forcefully released a few seconds later.
 
  Alternatively, it could simply add the relevant data to an existing
  device node and munge it's compatible property so simplefb picks it
  up.
 
  Yes, I think that'd be a much better solution. Of course it's going to
  be very difficult to make that work with a generic driver because now
  that generic driver needs to parse the DT binding for any number of
  compatible devices.
 
 Not necessarily.
 
 The patch that started this discussion can work with any number of
 clocks specified in a clocks property. Therefore all that needs to
 happen is that the final hardware binding specifies it's clocks that
 way.

Are you suggesting that we should be modeling the hardware specific
binding to match what the simplefb binding does?

 I'm sure that as hardware diversifies, the other subsystems will grow
 in similar directions and eventually be dealt with using similarly
 generic code.

For regulators this already works very differently. As opposed to the
clocks/clock-names type of binding it uses one where the consumer name
of the regulator comes from the prefix of a -supply property. That is
you'd get

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 05:04:32PM +0200, Michal Suchanek wrote:
 On 29 September 2014 15:47, Thierry Reding thierry.red...@gmail.com wrote:
  On Mon, Sep 29, 2014 at 01:46:43PM +0200, Maxime Ripard wrote:
  On Mon, Sep 29, 2014 at 12:18:08PM +0200, Thierry Reding wrote:
   On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:
On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
   [...]
 simplefb doesn't deal at all with hardware details. It simply uses 
 what
 firmware has set up, which is the only reason why it will work for 
 many
 people. What is passed in via its device tree node is the minimum 
 amount
 of information needed to draw something into the framebuffer. Also 
 note
 that the simplefb device tree node is not statically added to a DTS 
 file
 but needs to be dynamically generated by firmware at runtime.
   
Which makes the whole even simpler, since the firmware already knows
all about which clocks it had to enable.
  
   It makes things very complicated in the firmware because it now needs to
   be able to generate DTB content that we would otherwise be able to do
   much easier with a text editor.
 
  Didn't you just say that it was dynamically generated at runtime? So
  we can just ignore the text editor case.
 
  Perhaps read the sentence again. I said that we would *otherwise* be
  able to do much easier with a text editor..
 
  My point remains that there shouldn't be a need to generate DTB content
  of this complexity at all.
 
   Why do you think what I proposed isn't going to work or be reliable?
   I don't see any arguments in the thread that would imply that.
 
  The fact that it broke in the first place?
 
  That's exactly the point. And it's going to break again and again as
  simplefb is extended with new things. Generally DT bindings should be
  backwards compatible. When extended they should provide a way to fall
  back to a reasonable default. There's simply no way you can do that
  with simplefb.
 
  What happened in the Snow example is that regulators that were
  previously on would all of a sudden be automatically disabled on boot
  because there was now a driver that registered them with a generic
  framework.
 
 So what? You get a driver for regulators and suddenly find that
 nothing registered regulators because they were on all the time anyway
 and everything breaks? What a surprise!

I agree, this is not a surprise at all. But like I pointed out this is
bound to happen again and again. So how about we learn from it and add
the functionality needed to prevent it from happening?

  The same thing is going to happen with simplefb for your device. If you
  later realize that you need a regulator to keep the panel going, you'll
  have to add code to your firmware to populate the corresponding
  properties, otherwise the regulator will end up unused and will be
  automatically disabled. At the same time you're going to break upstream
  for all users of your old firmware because it doesn't add that property
  yet.
 
 Sure. And what can you do about that? It's not like the original Snow
 firmware writes anything of use to the DT at all. So to run a
 development kernel you need a development firmware. If you add new
 features to the kernel that involve intefacing to the firmware you
 need to update the firmware as well. Once support for Snow platform is
 stable you can expect that users can use a stable release of firmware
 indefinitely.

Feel free to take that up with the DT ABI stability committee.

  And the same will continue to happen for every new type of resource
  you're going to add.
 
 The like 5 resource types, yes. Some of which may not even apply to simplefb.

Today it's 5, tomorrow 6, next year, who knows.

 So how about instead of requiring resources to be explicitly claimed 
 we
 introduce something like the below patch? The intention being to give
 firmware device drivers a way of signalling to the clock framework
 that they need rely on clocks set up by firmware and when they no 
 longer
 need them. This implements essentially what Mark (CC'ing again on 
 this
 subthread) suggested earlier in this thread. Basically, it will allow
 drivers to determine the time when unused clocks are really unused. 
 It
 will of course only work when used correctly by drivers. For the 
 case of
 simplefb I'd expect its .probe() implementation to call the new
 clk_ignore_unused() function and once it has handed over control of 
 the
 display hardware to the real driver it can call 
 clk_unignore_unused() to
 signal that all unused clocks that it cares about have now been 
 claimed.
 This is reference counted and can therefore be used by more than a
 single driver if necessary. Similar functionality could be added for
 other resource subsystems as needed.
   
So, just to be clear, instead of doing a generic

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 05:57:18PM +0200, Maxime Ripard wrote:
 On Mon, Sep 29, 2014 at 03:54:00PM +0200, Thierry Reding wrote:
  On Mon, Sep 29, 2014 at 01:34:36PM +0200, Maxime Ripard wrote:
   On Mon, Sep 29, 2014 at 12:44:57PM +0200, Thierry Reding wrote:
  Plus, speaking more specifically about the clocks, that won't 
  prevent
  your clock to be shut down as a side effect of a later clk_disable
  call from another driver.
 
  Furthermore isn't it a bug for a driver to call clk_disable() 
  before a
  preceding clk_enable()? There are patches being worked on that will
  enable per-user clocks and as I understand it they will specifically
  disallow drivers to disable the hardware clock if other drivers are
  still keeping them on via their own referenc.
 
 Calling clk_disable() preceding clk_enable() is a bug.
 
 Calling clk_disable() after clk_enable() will disable the clock (and
 its parents)
 if the clock subsystem thinks there are no other users, which is what 
 will
 happen here.

Right. I'm not sure this is really applicable to this situation, though.
   
   It's actually very easy to do. Have a driver that probes, enables its
   clock, fails to probe for any reason, call clk_disable in its exit
   path. If there's no other user at that time of this particular clock
   tree, it will be shut down. Bam. You just lost your framebuffer.
   
   Really, it's just that simple, and relying on the fact that some other
   user of the same clock tree will always be their is beyond fragile.
  
  Perhaps the meaning clk_ignore_unused should be revised, then. What you
  describe isn't at all what I'd expect from such an option. And it does
  not match the description in Documentation/kernel-parameters.txt either.
 
 Well, it never says that it also prevent them from being disabled
 later on. But it's true it's not very explicit.

It says:

Keep all clocks already enabled by bootloader on,
even if no driver has claimed them. ...

There's no until or anything there, so I interpret that as
indefinitely.

Either way, if there are other users of a clock then they will just as
likely want to modify the rate at which point simplefb will break just
as badly.
   
   And this can be handled just as well. Register a clock notifier,
   refuse any rate change, done. But of course, that would require having
   a clock handle.
   
   Now, how would *you* prevent such a change?
  
  Like I said in the other thread. If you have two drivers that use the
  same clock but need different frequencies you've lost anyway.
 
 Except that the driver that has the most logic (ie not simplefb) will
 have a way to recover and deal with that.

You're making an assumption here. Why would the driver have such logic
if nothing ever prevented it from setting the rate properly before.

 But sure, you can still try to point new issues, get an obvious and
 robust solution, and then discard the issue when the solution doesn't
 go your way...

And you've already proven that you're completely unwilling to even
consider any other solution than what was originally proposed, so I
really don't see how discussing this further with you is going to be
productive.

Thierry


pgpHQW_Zr8ZNU.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 04:55:17PM +0100, Mark Brown wrote:
 On Mon, Sep 29, 2014 at 03:47:15PM +0200, Thierry Reding wrote:
[...]
  The same thing is going to happen with simplefb for your device. If you
  later realize that you need a regulator to keep the panel going, you'll
  have to add code to your firmware to populate the corresponding
  properties, otherwise the regulator will end up unused and will be
  automatically disabled. At the same time you're going to break upstream
  for all users of your old firmware because it doesn't add that property
  yet.
 
  And the same will continue to happen for every new type of resource
  you're going to add.
 
 So long as we're ensuring that when we don't start supporting resources
 without DT additions or at least require DT additions to actively manage
 them (which can then include simplefb hookup) we should be fine.

I'm not sure I understand what you mean. If we add a driver for the PMIC
that exposes these regulators and then add a DT node for the PMIC, we'd
still need to fix the firmware to generate the appropriate DT properties
to allow simplefb to enable the regulators.

So unless firmware is updated at the same time, regulators will get
disabled because they are unused.

Thierry


pgp7VvxO9FEQu.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Mon, Sep 29, 2014 at 06:28:14PM +0200, Maxime Ripard wrote:
 On Mon, Sep 29, 2014 at 03:47:15PM +0200, Thierry Reding wrote:
  On Mon, Sep 29, 2014 at 01:46:43PM +0200, Maxime Ripard wrote:
   On Mon, Sep 29, 2014 at 12:18:08PM +0200, Thierry Reding wrote:
On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:
 On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
[...]
  simplefb doesn't deal at all with hardware details. It simply uses 
  what
  firmware has set up, which is the only reason why it will work for 
  many
  people. What is passed in via its device tree node is the minimum 
  amount
  of information needed to draw something into the framebuffer. Also 
  note
  that the simplefb device tree node is not statically added to a DTS 
  file
  but needs to be dynamically generated by firmware at runtime.
 
 Which makes the whole even simpler, since the firmware already knows
 all about which clocks it had to enable.

It makes things very complicated in the firmware because it now needs to
be able to generate DTB content that we would otherwise be able to do
much easier with a text editor.
   
   Didn't you just say that it was dynamically generated at runtime? So
   we can just ignore the text editor case.
  
  Perhaps read the sentence again. I said that we would *otherwise* be
  able to do much easier with a text editor..
  
  My point remains that there shouldn't be a need to generate DTB content
  of this complexity at all.
  
Why do you think what I proposed isn't going to work or be reliable?
I don't see any arguments in the thread that would imply that.
   
   The fact that it broke in the first place?
  
  That's exactly the point. And it's going to break again and again as
  simplefb is extended with new things. Generally DT bindings should be
  backwards compatible. When extended they should provide a way to fall
  back to a reasonable default. There's simply no way you can do that
  with simplefb.
 
 This one *is* backward compatible. It doesn't even change the simplefb
 behaviour, compared to what it was doing before, if you don't have
 this clocks property. What can be a more reasonable default that the
 way it used to behave?

My point is that if we decide not to consider all resources handled by
firmware then we need to go all the way. At that point you start having
problems as evidenced by the Snow example.

  What happened in the Snow example is that regulators that were
  previously on would all of a sudden be automatically disabled on boot
  because there was now a driver that registered them with a generic
  framework.
  
  The same thing is going to happen with simplefb for your device. If you
  later realize that you need a regulator to keep the panel going, you'll
  have to add code to your firmware to populate the corresponding
  properties, otherwise the regulator will end up unused and will be
  automatically disabled. At the same time you're going to break upstream
  for all users of your old firmware because it doesn't add that property
  yet.
 
  And the same will continue to happen for every new type of resource
  you're going to add.
 
 Sure, we can add any resources we will need. Regulators, reset lines,
 pm domains, allocated memory, but I'm not really sure this is what you
 want, right?

No it's not what I want. *You* want to add resource management to this
driver. What I'm saying is that if we start adding clocks then we can no
longer say no to resets or regulators or power domains either.

 You know that you are going to call that for regulator, reset, power
 domains, just as you would have needed to with the proper API, unless
 that with this kind of solution, you would have to modify *every*
 framework that might interact with any resource involved in getting
 simplefb running?

We have to add handling for every kind of resource either way. Also if
this evolves into a common pattern we can easily wrap it up in a single
function call.
   
   Unless that in one case, we already have everything needed to handle
   everything properly, and in another, you keep hacking more and more
   into the involved frameworks.
  
  This is a fundamental issue that we are facing and I'm trying to come up
  with a solution that is future-proof and will work for drivers other
  than simplefb.
  
  Just because we currently lack this functionality doesn't make it a hack
  trying to add it.
 
 Or maybe it's just showing that the trend to rely more and more on the
 firmware is a bad idea?

If you think it's a bad idea to rely on firmware then you shouldn't be
using simplefb in the first place.

 I really start to consider adding a sunxi-uboot-fb, that would just
 duplicate the source code of simplefb but also taking the
 clocks. Somehow, I feel like it will be easier (and definitely less of
 a hack than using the generic common clock

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Tue, Sep 30, 2014 at 12:02:50AM +0200, Luc Verhaegen wrote:
 On Mon, Sep 29, 2014 at 06:58:42PM +0200, Luc Verhaegen wrote:
  
  In the 2nd round of this discussion, i stated that another fb or even a 
  drm driver altogether seemed to be the sensible way out of this mess.
  
  I suggest drm_rescue.
  
  Very early on, now almost two months back, i used the word denialfb.
  rpifb is the real name of this thing though, but then the dt binding 
  names would have to change and whatnot.
  
  I don't get the resistance, at least not from a technical point of view. 
  And i do not care enough to get properly involved in this pointless and 
  endless discussion. drm_rescue it is.
  
  Luc Verhaegen.
 
 So Thierry, let's review what we have achieved here.
 
 1) you keep simplefb absolutely true to the name. Congratulations.
 2) Simplefb will only have a single user: the rpi. As the only other 
 users i can think of, which does not have a full driver and which does 
 not have clocks automatically disabled, are discrete cards. And they do 
 not really tend to happen with dt or platform devices.
 3) a competing driver will be created, which will do these dt-ishy 
 things.

You clearly haven't bothered to even try and understand what I wanted to
achieve. If you had you'd realize that creating a competing driver isn't
going to change anything.

 4) it's just a matter of time before the rpi either gets a full driver, 
 or switches over to the driver that everyone else is actually using. And 
 then the misnomer gets deprecated.
 
 Was that the outcome you were looking for? I think not.

I had, perhaps naively, expected that you guys would be willing to look
beyond your own nose. You clearly aren't. You've been outright
dismissing any proposals beyond what you originally posted.

You keep bringing up the Raspberry Pi for some reason and suggest that
it is somehow inferior to sunxi. What makes you think it's less entitled
to be supported on Linux than sunxi? I don't care about the Raspberry Pi
and I equally don't care about sunxi. I don't own a Raspberry Pi and I
don't own any Allwinner hardware. What I do care about is Linux and I
want it to work well for all SoCs equally.

Perhaps if you could put aside your crusade against the Raspberry Pi for
just a second you'll realize that we're all on the same team. This isn't
a competition and I'm not trying to put a spoke in your wheel. On the
contrary, I'm actually trying to help you.

Thierry


pgpqfdQJ8nL83.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Fri, Aug 29, 2014 at 07:13:22AM +0200, Michal Suchanek wrote:
 On 28 August 2014 12:08, Thierry Reding thierry.red...@gmail.com wrote:
  On Wed, Aug 27, 2014 at 10:57:29PM +0200, Michal Suchanek wrote:
  Hello,
 
  On 27 August 2014 17:42, Maxime Ripard maxime.rip...@free-electrons.com 
  wrote:
   On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote:
   On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote:
On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote:
 On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote:
  On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote:
   [...]
Mike Turquette repeatedly said that he was against such a DT 
property:
https://lkml.org/lkml/2014/5/12/693
  
   Mike says in that email that he's opposing the addition of a 
   property
   for clocks that is the equivalent of regulator-always-on. 
   That's not
   what this is about. If at all it'd be a property to mark a 
   clock that
   should not be disabled by default because it's essential.
 
  It's just semantic. How is a clock that should not be disabled by
  default because it's essential not a clock that stays always on?

 Because a clock that should not be disabled by default can be 
 turned off
 when appropriate. A clock that is always on can't be turned off.
   
If a clock is essential, then it should never be disabled. Or we don't
share the same meaning of essential.
  
   Essential for the particular use-case.
  
   So, how would the clock driver would know about which use case we're
   in? How would it know about which display engine is currently running?
   How would it know about which video output is being set?
  
   Currently, we have two separate display engines, which can each output
   either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every
   one of these combinations would require different clocks. What clocks
   will we put in the driver? All of them?
  
 
  since simplefb cannot be extended how about adding, say, dtfb which
  claims the resources from dt and then instantiates a simplefb once the
  resources are claimed? That is have a dtfb which has the clocks
  assigned and has simplefb as child dt node.
 
  I don't see how that changes anything. All you do is add another layer
  of indirection. The fundamental problem remains the same and isn't
  solved.
 
 It keeps clock code out of simplefb and provides driver for the kind
 of framebuffer set up by firmware that exists on sunxi, exynos, and
 probably many other SoCs. That is a framebuffer that needs some clocks
 and possibly regulators enabled to keep running because the reality of
 the platform is that it has clocks and regulators unlike some other
 platforms that do not.
 
 These clocks and regulators are used but not configured by the
 framebuffer and should be reclaimed when firmware framebuffer is
 disabled. This is the same as the chunk of  memory used by simplefb
 which is currently lost but should be reclaimed when simplefb is
 disabled.
 
 This memory is not 'reserved for firmware' and unusable but reserved
 for framebuffer and the regulators are not 'always on' or 'should
 never be disabled' but should be enabled while framebuffer is used.
 
 As far as I can tell in DT this is expressed by creating a DT node
 associated with the framebuffer driver that tells the kernel that this
 memory, clocks and regulators are associated with the framebuffer
 driver and can be reclaimed if this driver is stopped or not enabled
 in the kernel at all. If you are going to be asinine about simplefb
 not getting support for managing any resource other than the memory
 chunk then another layer of indirection is required for platforms that
 have more resources to manage.
 
 If there is another way to associate resources with a driver in DT
 then please enlighten me.
 
 AFAICT simplefb is the framebuffer driver already in kernel closest to
 the driver that is required for sunxi - simplefb also relies on
 firmware to set up the framebuffer but unlike vesafb or efifb it
 already has DT integration. So the most efficient way to implement
 framebuffer for sunxi is to extend simplefb or if necessary add
 another layer of indirection under simplefb. If there is a better
 fitting driver in the kernel then please enlighten me and the
 developer that wrote this patch what driver it would be.

I think simplefb is exactly the right driver to use for this case. But I
don't think making it manage all the resources is the right thing to do.
While it's fairly easy to do it, it's also something that needs to be
done for every other driver with similar requirements. Consider for
example what happens if you want to play some welcome sound in the
bootloader and keep it playing while the kernel is booting. You'd need
to repeat exactly what this driver does to keep clocks for audio
hardware running

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Thu, Aug 28, 2014 at 02:15:40PM +0200, Hans de Goede wrote:
 On 08/27/2014 04:16 PM, Thierry Reding wrote:
[...]
  Hmm I see, in my mind the problem is not that the clk framework disables
  unused clocks, but that no one is marking the clocks in question as used.
  Someone should mark these clocks as used so that they do not get disabled.
 
  We could add a clk_mark_in_use function and have the simplefb patch call
  that instead of clk_prepare_and_enable, or maybe even better just only
  claim the clks and teach the clk framework to not disable claimed clk
  in its cleanup run. That would make it clear that simplefb is not enabling
  anything, just claiming resource its need to avoid them getting removed
  from underneath simplefb, would that work for you ?
  
  That would be more accurate, but it boils down to the same problem where
  we still need a driver to claim the resources in some way whereas the
  problem is fundamentally one where the bootloader should be telling the
  kernel not to disable it. It's in fact the bootloader that's claiming
  the resources.
 
 Yes, but those resources do not belong to the bootloader in a sense
 that traditional bootloader / firmware claimed resources (e.g. acpi
 reserved resources) do. These traditional resources are claimed for ever.

I thought that at least on x86 there was a way for the kernel to reclaim
memory set apart for an early framebuffer.

 Where as these resources are claimed by the bootloader to keep the simplefb
 it provides working, as soon as the simplefb is no longer used, they become
 unused.

Right. And when simplefb goes away it is because a real driver is taking
over, in which case it will claim the resources explicitly.

  The copy and paste is for code that's platform specific. The clocks have
  different names, resets are different, supplies are different. The fact
  that many can currently use the same driver is likely just coincidence
  rather than design and it's entirely possible that at some point they'll
  add support for a more advanced feature that makes them incompatible
  with the rest of the generic drivers. And then you have a big mess
  because you need to add quirks all over the place.
  
  And this isn't all that far off-topic, since simplefb also needs to deal
  with this kind of situation. And what I've been arguing is that in order
  to really be generic it has to make assumptions, one of which is that it
  uses only resources that it doesn't need to explicitly handle.
 
 I can understand that you're worried about generic ?hci drivers dealing with
 clocks / resets / etc. As there may be strict ordering requirements there,
 but for simplefb that is not the case.
 
 All we're asking for is for a way to communicate 2 things to the kernel:
 
 1) These resources are in use (we are not asking the kernel to do anything
 with them, rather the opposite, we're asking to leave them alone so no
 ordering issues)

Right. That's the issue that needs to be solved. We still only disagree
on how it should be solved.

 2) Tie these resources to simplefb so that the kernel can know when they
 are no longer in use, and it may e.g. re-use the memory

For the memory there shouldn't be a problem because it's already in the
DT node anyway. It has to be there so that simplefb knows where to write
to.

For all the other resources, if 1) is solved properly then 2) becomes a
non-issue.

 To me the most logical and also most correct way of modelling this is to
 put the resources inside the simplefb dt node.

Except that simplefb isn't a real device, so there's a hard time
justifying its existence in DT as it is. Claiming resources from a
virtual device doesn't sound correct to me at all.

  The key word here is the used resources to me this is not about simlefb
  managing resources, but marking them as used as long as it needs them, like
  it will need to do for the reserved mem chunk.
  
  The difference between the clocks and the memory resource is that the
  driver needs to directly access the memory (it needs to map it and
  provide a userspace mapping for it) whereas it doesn't need to touch the
  clocks (except to workaround a Linux-specific implementation detail).
 
 Erm, no, the need to map the memory and the memory being a resource
 which may be released are an orthogonal problem. E.g. a system with
 dedicated framebuffer memory won't need to use a reserved main-memory
 chunk, nor need to worry about returning that mem when simplefb is no
 longer in use.

I would think the memory should still be reserved anyway to make sure
nothing else is writing over it. And it's in the device tree anyway
because the driver needs to know where to put framebuffer content. So
the point I was trying to make is that we can't treat the memory in the
same way as clocks because it needs to be explicitly managed. Whereas
clocks don't. The driver is simply too generic to know what to do with
the clocks. It doesn't know what frequency they should be running

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Fri, Aug 29, 2014 at 08:48:42AM +0200, Michal Suchanek wrote:
 On 29 August 2014 08:19, Thierry Reding thierry.red...@gmail.com wrote:
  On Fri, Aug 29, 2014 at 07:13:22AM +0200, Michal Suchanek wrote:
  On 28 August 2014 12:08, Thierry Reding thierry.red...@gmail.com wrote:
   On Wed, Aug 27, 2014 at 10:57:29PM +0200, Michal Suchanek wrote:
   Hello,
  
   On 27 August 2014 17:42, Maxime Ripard 
   maxime.rip...@free-electrons.com wrote:
On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote:
On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote:
 On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote:
  On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote:
   On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote:
[...]
 Mike Turquette repeatedly said that he was against such a 
 DT property:
 https://lkml.org/lkml/2014/5/12/693
   
Mike says in that email that he's opposing the addition of a 
property
for clocks that is the equivalent of regulator-always-on. 
That's not
what this is about. If at all it'd be a property to mark a 
clock that
should not be disabled by default because it's essential.
  
   It's just semantic. How is a clock that should not be 
   disabled by
   default because it's essential not a clock that stays always 
   on?
 
  Because a clock that should not be disabled by default can be 
  turned off
  when appropriate. A clock that is always on can't be turned off.

 If a clock is essential, then it should never be disabled. Or we 
 don't
 share the same meaning of essential.
   
Essential for the particular use-case.
   
So, how would the clock driver would know about which use case we're
in? How would it know about which display engine is currently running?
How would it know about which video output is being set?
   
Currently, we have two separate display engines, which can each output
either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every
one of these combinations would require different clocks. What clocks
will we put in the driver? All of them?
   
  
   since simplefb cannot be extended how about adding, say, dtfb which
   claims the resources from dt and then instantiates a simplefb once the
   resources are claimed? That is have a dtfb which has the clocks
   assigned and has simplefb as child dt node.
  
   I don't see how that changes anything. All you do is add another layer
   of indirection. The fundamental problem remains the same and isn't
   solved.
 
  It keeps clock code out of simplefb and provides driver for the kind
  of framebuffer set up by firmware that exists on sunxi, exynos, and
  probably many other SoCs. That is a framebuffer that needs some clocks
  and possibly regulators enabled to keep running because the reality of
  the platform is that it has clocks and regulators unlike some other
  platforms that do not.
 
  These clocks and regulators are used but not configured by the
  framebuffer and should be reclaimed when firmware framebuffer is
  disabled. This is the same as the chunk of  memory used by simplefb
  which is currently lost but should be reclaimed when simplefb is
  disabled.
 
  This memory is not 'reserved for firmware' and unusable but reserved
  for framebuffer and the regulators are not 'always on' or 'should
  never be disabled' but should be enabled while framebuffer is used.
 
  As far as I can tell in DT this is expressed by creating a DT node
  associated with the framebuffer driver that tells the kernel that this
  memory, clocks and regulators are associated with the framebuffer
  driver and can be reclaimed if this driver is stopped or not enabled
  in the kernel at all. If you are going to be asinine about simplefb
  not getting support for managing any resource other than the memory
  chunk then another layer of indirection is required for platforms that
  have more resources to manage.
 
  If there is another way to associate resources with a driver in DT
  then please enlighten me.
 
  AFAICT simplefb is the framebuffer driver already in kernel closest to
  the driver that is required for sunxi - simplefb also relies on
  firmware to set up the framebuffer but unlike vesafb or efifb it
  already has DT integration. So the most efficient way to implement
  framebuffer for sunxi is to extend simplefb or if necessary add
  another layer of indirection under simplefb. If there is a better
  fitting driver in the kernel then please enlighten me and the
  developer that wrote this patch what driver it would be.
 
  I think simplefb is exactly the right driver to use for this case. But I
  don't think making it manage all the resources is the right thing to do.
 
 And what is supposed to manage resources used by simplefb when not simplefb?

Nothing should be managing them. There's

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Thu, Aug 28, 2014 at 12:34:58PM -0400, jonsm...@gmail.com wrote:
 On Thu, Aug 28, 2014 at 12:25 PM, Michal Suchanek hramr...@gmail.com wrote:
  On 28 August 2014 16:33, jonsm...@gmail.com jonsm...@gmail.com wrote:
  On Thu, Aug 28, 2014 at 10:20 AM, Geert Uytterhoeven
  ge...@linux-m68k.org wrote:
  On Thu, Aug 28, 2014 at 3:22 PM, jonsm...@gmail.com jonsm...@gmail.com 
  wrote:
  2) We don't want to hardcode these clocks into the kernel (sunxi) clk
  driver, instead the bootloader should tell the kernel about these 
  clocks.
 
  So the only point of discussion left seems to be how to do 2...
 
  Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and
  whip together a device specific driver that claims the proper
  resources? And just implement the minimal about of fbdev possible?
  fbdev already is a driver library.
 
  Like... drivers/video/fbdev/offb.c?
 
  I'd probably reclassify drivers/video/fbdev/simplefb.c as a skeleton
  and use it as a template for making device specific versions of it.
 
  I don't see why there is so much resistance to just making device
  specific fb drivers.  Whenever the KMS driver gets written just
  disable the device specific fb driver in the build.
 
  Except that is not the goal here. The simplefb or whatever replacement
  is supposed to stay as a  generic driver compiled into kernel whereas
 
 There is no generic solution to this problem as this entire thread has
 illustrated. The clocks/regulators needed by each SOC vary.

There is a generic solution. Genericity only works if you define a well
defined set of assumptions. In this case the assumptions are that some
firmware sets up display hardware to scan out from a memory region and
communicates the location, layout and format of that memory region so
that a generic driver can write into that framebuffer.

The generic solution here works because we've eliminated the platform
specificities and let firmware take care of it.

 So there are two solutions..
 1) modify simplefb to have some kind of heuristic that tries to guess
 what needs to be protected. A heuristic that is probably going to fail
 on every new SOC.
 
 2) Spend a day implementing a device specific fbdev driver that does
 the correct thing all of the time. These drivers would sit in initrd
 and load before the clock/regulator clean up shuts everything off. Use
 the existing simplefb code as a template for doing this.

There's a third possibility: find a way to prevent the clock framework
(and any other resource framework for that matter) from forcefully
disabling things that they shouldn't.

Thierry


pgpOAhLeVorot.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Fri, Aug 29, 2014 at 09:23:41AM +0200, Hans de Goede wrote:
 On 08/29/2014 09:01 AM, Thierry Reding wrote:
  On Thu, Aug 28, 2014 at 02:15:40PM +0200, Hans de Goede wrote:
[...]
  2) Tie these resources to simplefb so that the kernel can know when they
  are no longer in use, and it may e.g. re-use the memory
  
  For the memory there shouldn't be a problem because it's already in the
  DT node anyway. It has to be there so that simplefb knows where to write
  to.
 
 The memory information currently in the dt node is not complete enough to
 reclaim memory, e.g. the sunxi driver always reserves 8M, but the simplefb
 reg entry only describes the part actually used for the framebuffer.

You can easily fix that.

  To me the most logical and also most correct way of modelling this is to
  put the resources inside the simplefb dt node.
  
  Except that simplefb isn't a real device, so there's a hard time
  justifying its existence in DT as it is. Claiming resources from a
  virtual device doesn't sound correct to me at all.
 
 Yet you want it to claim memory that does not seem consistent to me.

Because it needs to explicitly access that memory. It does not need to
explicitly access the clocks.

  The key word here is the used resources to me this is not about simlefb
  managing resources, but marking them as used as long as it needs them, 
  like
  it will need to do for the reserved mem chunk.
 
  The difference between the clocks and the memory resource is that the
  driver needs to directly access the memory (it needs to map it and
  provide a userspace mapping for it) whereas it doesn't need to touch the
  clocks (except to workaround a Linux-specific implementation detail).
 
  Erm, no, the need to map the memory and the memory being a resource
  which may be released are an orthogonal problem. E.g. a system with
  dedicated framebuffer memory won't need to use a reserved main-memory
  chunk, nor need to worry about returning that mem when simplefb is no
  longer in use.
  
  I would think the memory should still be reserved anyway to make sure
  nothing else is writing over it. And it's in the device tree anyway
  because the driver needs to know where to put framebuffer content.
 
 No, the address were to write framebuffer contents currently is in the
 simplefb node, not the actually backing memory info. I can fully see
 some crazy hardware where a chunk of main memory is reserved for some
 funky video engine, and then that chunk of memory gets accessed through
 io-mem addresses for framebuffer access (e.g. the hardware may need this
 to know when the fb is dirtied).

We're drifting off into the realm of hypotheses here. Of course if you
can't make the hardware work with these simple assumptions because it's
too whacky then that's just tough luck.

  So
  the point I was trying to make is that we can't treat the memory in the
  same way as clocks because it needs to be explicitly managed. Whereas
  clocks don't. The driver is simply too generic to know what to do with
  the clocks. It doesn't know what frequency they should be running at or
  what they're used for, so by any definition of what DT should describe
  they're useless for this virtual device.
 
 I don't see why you keep insisting that the memory is so special, it is
 just another resource which we need to claim,

It's the only resource which we need to claim. Because it is the only
resource that needs to be explicitly accessed. simplefb is useless if
you don't write to that memory.

   and tie to the simplefb node
 so that it can be re-used (or disabled) when simplefb is no longer used
 (it could e.g. be unbound explicitly on headless systems).

On headless systems the firmware shouldn't be setting up a framebuffer
in the first place.

  Furthermore it's fairly likely that as your kernel support progresses
  you'll find that the driver all of a sudden needs to manage some other
  type of resource that you just haven't needed until now because it may
  default to being always on. Then you'll have a hard time keeping
  backwards-compatibility and will have to resort to the kinds of hacks
  that you don't want to see in the kernel.
 
 kernel support? This is about u-boot's video code and the simplefb bindings.

And the kernel doesn't need to use the simplefb bindings? Why are we
even having this conversation, then?

  So it really boils down to the DT needing to describe a complete device
  with all the dependencies. And just clocks aren't the complete
  description. But if you want the complete description then you're going
  to need a complete driver as well and simplefb is out of the picture
  anyway.
 
 Yes we eventually need a full kernel driver, with its own bindings, that
 is not what this is about.

It is to some degree. The whole point of simplefb is to provide a useful
graphical output until a full driver is in place. The way to achieve
that is by taking advantage of the firmware setting

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Fri, Aug 29, 2014 at 03:57:08PM +0200, Maxime Ripard wrote:
 On Fri, Aug 29, 2014 at 08:29:33AM +0200, Thierry Reding wrote:
  On Thu, Aug 28, 2014 at 10:46:03PM +0200, Maxime Ripard wrote:
   On Thu, Aug 28, 2014 at 12:11:41PM +0200, Thierry Reding wrote:
  [...]
Then since firmware already knows what it set up it can tell the
kernel to not touch those.
   
   Somehow, I've been raised kernel-wise into thinking that you can never
   fully trust your firmware. Or at least that you should have a way to
   recover from any bug/bad behaviour from it.
  
  If you don't trust your firmware then you shouldn't be taking over a
  device that it's set up for you. Rather you should write a proper driver
  that sets things up from the start.
  
  This whole we don't trust firmware isn't going to work if we want to
  have hand-off from firmware to kernel. And it's already been decided in
  other threads that moving more code out of the kernel and into firmware
  is a requirement (c.f. ARM64).
 
 Except that in ARM64 case, it has been asked before having any
 SoC/boards out. We're definitely not in this situation there.

You're still in the situation where you need to trust your firmware, so
my point remains valid.

  Also in this case you wrote the firmware, so why wouldn't you trust it?
 
 We partly wrote the firmware, on some of the available SoCs. Some
 other just have allwinner's own. But yeah, in this case they wouldn't
 even set up the framebuffer in the first place.

Even if you didn't write it, you could still trust it provided that you
had the source code for it. The whole argument of never trust firmware
is based on the assumption that you can't fix it. And even if that's the
case, it's still perfectly acceptable to not trust firmware. But if you
don't trust firmware then you really shouldn't be using simplefb in the
first place and do everything in the kernel.

   Moreover, the way I see it, there's a major flaw in having an
   attribute in the clock node: you don't even know if the clock is ever
   going to be used.
   
   If simplefb is not compiled in, you won't claim the clocks, and they
   will be disabled, which is imho a good thing. This case wouldn't be
   covered with an attribute at the clock node, because you don't have a
   link to what device/feature actually uses it in the system, and so you
   have to make the assumption that it will be used. And you will end up
   with clocks with a rather high rate running for nothing.
  
  That's completely hypothetical. If simplefb isn't enabled and the clock
  isn't claimed there's probably still going to be some other driver
  taking over eventually. If there isn't, what's the point of firmware
  setting up display in the first case?
 
 And now, here is a new requirement for the firmware: that in addition
 to being reliable, that it must not be dumb, or take shortcuts, like
 setting up the framebuffer in every case, no matter wether there's a
 screen or not.

Firmware should never do that anyway. The whole point of firmware is to
be device-specific and therefore it already knows when there's a need to
activate a framebuffer or not.

Thierry


pgpqnooWhMVrK.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote:
 On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote:
  I would think the memory should still be reserved anyway to make sure
  nothing else is writing over it. And it's in the device tree anyway
  because the driver needs to know where to put framebuffer content. So
  the point I was trying to make is that we can't treat the memory in the
  same way as clocks because it needs to be explicitly managed. Whereas
  clocks don't. The driver is simply too generic to know what to do with
  the clocks.
 
 You agreed on the fact that the only thing we need to do with the
 clocks is claim them. Really, I don't find what's complicated there
 (or not generic).

That's not what I agreed on. What I said is that the only thing we need
to do with the clocks is nothing. They are already in the state that
they need to be.

  It doesn't know what frequency they should be running at
 
 We don't care about that. Just like we don't care about which
 frequency is the memory bus running at. It will just run at whatever
 frequency is appropriate.

Exactly. And you shouldn't have to care about them at all. Firmware has
already configured the clocks to run at the correct frequencies, and it
has made sure that they are enabled.

  or what they're used for
 
 And we don't care about that either. You're not interested in what
 output the framebuffer is setup to use, which is pretty much the same
 here, this is the same thing here.

That's precisely what I've been saying. The only thing that simplefb
cares about is the memory it should be using and the format of the
pixels (and how many of them) it writes into that memory. Everything
else is assumed to have been set up.

Including clocks.

  so by any definition of what DT should describe they're useless for
  this virtual device.
 
  Furthermore it's fairly likely that as your kernel support progresses
  you'll find that the driver all of a sudden needs to manage some other
  type of resource that you just haven't needed until now because it may
  default to being always on. Then you'll have a hard time keeping
  backwards-compatibility and will have to resort to the kinds of hacks
  that you don't want to see in the kernel.
 
 Not such a hard time. An older DT wouldn't define the new requirements
 anyway, so they wouldn't be used, and we would end up in pretty much
 the current case.

Except that you have firmware in the wild that sets up an incomplete
simplefb node and if you don't want to break compatibility you need to
provide fallbacks for the resources that aren't listed in the DT node.
And given that those fallbacks are all very board specific you'll need
to find ways to keep them enabled if you want to keep existing setups
working.

Thierry


pgp9ez2aUTRO0.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Wed, Aug 27, 2014 at 10:57:29PM +0200, Michal Suchanek wrote:
 Hello,
 
 On 27 August 2014 17:42, Maxime Ripard maxime.rip...@free-electrons.com 
 wrote:
  On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote:
  On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote:
   On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote:
On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote:
 On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote:
  [...]
   Mike Turquette repeatedly said that he was against such a DT 
   property:
   https://lkml.org/lkml/2014/5/12/693
 
  Mike says in that email that he's opposing the addition of a 
  property
  for clocks that is the equivalent of regulator-always-on. That's 
  not
  what this is about. If at all it'd be a property to mark a clock 
  that
  should not be disabled by default because it's essential.

 It's just semantic. How is a clock that should not be disabled by
 default because it's essential not a clock that stays always on?
   
Because a clock that should not be disabled by default can be turned 
off
when appropriate. A clock that is always on can't be turned off.
  
   If a clock is essential, then it should never be disabled. Or we don't
   share the same meaning of essential.
 
  Essential for the particular use-case.
 
  So, how would the clock driver would know about which use case we're
  in? How would it know about which display engine is currently running?
  How would it know about which video output is being set?
 
  Currently, we have two separate display engines, which can each output
  either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every
  one of these combinations would require different clocks. What clocks
  will we put in the driver? All of them?
 
 
 since simplefb cannot be extended how about adding, say, dtfb which
 claims the resources from dt and then instantiates a simplefb once the
 resources are claimed? That is have a dtfb which has the clocks
 assigned and has simplefb as child dt node.

I don't see how that changes anything. All you do is add another layer
of indirection. The fundamental problem remains the same and isn't
solved.

Thierry


pgppJQBEaKSr2.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Wed, Aug 27, 2014 at 05:42:21PM +0200, Maxime Ripard wrote:
 On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote:
  On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote:
   On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote:
On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote:
 On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote:
  [...]
   Mike Turquette repeatedly said that he was against such a DT 
   property:
   https://lkml.org/lkml/2014/5/12/693
  
  Mike says in that email that he's opposing the addition of a 
  property
  for clocks that is the equivalent of regulator-always-on. That's not
  what this is about. If at all it'd be a property to mark a clock 
  that
  should not be disabled by default because it's essential.
 
 It's just semantic. How is a clock that should not be disabled by
 default because it's essential not a clock that stays always on?

Because a clock that should not be disabled by default can be turned off
when appropriate. A clock that is always on can't be turned off.
   
   If a clock is essential, then it should never be disabled. Or we don't
   share the same meaning of essential.
  
  Essential for the particular use-case.
 
 So, how would the clock driver would know about which use case we're
 in? How would it know about which display engine is currently running?
 How would it know about which video output is being set?
 
 Currently, we have two separate display engines, which can each output
 either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every
 one of these combinations would require different clocks. What clocks
 will we put in the driver? All of them?

Ideally the solution wouldn't involve hard-coding this into the clock
driver at all. There should be a way for firmware to communicate to the
kernel that a given clock shouldn't be disabled. Then since firmware
already knows what it set up it can tell the kernel to not touch those.

Thierry


pgpxH2eOBYQ2e.pgp
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote:
 On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote:
 Any decent enough SoC, with a decent support in the kernel will have
 clocks for this, and I really wonder how simplefb will behave once its
 clocks will be turned off...

There are other devices besides ARM SoCs that may want to use this
driver and that don't have clock support.
   
   And in this case, with this patch, simplefb will not claim any clock,
   nor will fail probing.
   
But you're missing my point. What I'm saying is that the simplefb driver
is meant to serve as a way to take over whatever framebuffer a firmware
set up. Therefore I think it makes the most sense to assume that nothing
needs to be controlled in any way since already been set up by firmware.
Eventually there should be a driver that takes over from simplefb that
knows how to properly handle the device's specifics, but that's not
simplefb.
   
   I guess such a hand over if it were to happen in the kernel would
   involve the second driver claiming the resources before the first one
   release them. How is that different in this case?
  
  It's different in that that driver will be hardware specific and know
  exactly what clock and other resources are required. It will have a
  device-specific binding.
 
 Except that you made simplefb a generic driver. So we have two choices
 here: either we don't anything but the trivial case, and no one with a
 rather more advanced hardware will be able to use it, or we try to
 grab any resource that might be of use, which means clocks,
 regulators, reserved memory region, or whatever, so that we pretty
 much cover all cases. It really is as simple as that.

No, it should be even simpler. simplefb shouldn't have to know any of
this. It's just taking what firmware has set up and uses that. It's a
stop-gap solution to provide information on the display until a real
driver can be loaded and initializes the display hardware properly.

The goal of this patch series is to keep clocks from being turned off.
But that's not what it does. What it does is turn clocks on to prevent
them from being turned off. In my opinion that's a workaround for a
deficiency in the kernel (and the firmware/kernel interface) and I think
it should be fixed at the root. So a much better solution would be to
establish a way for firmware to communicate to the kernel that a given
resource has been enabled by firmware and shouldn't be disabled. Such a
solution can be implement for all types of resources and can be reused
by all drivers since they don't have to worry about these details.
   
   Mike Turquette repeatedly said that he was against such a DT property:
   https://lkml.org/lkml/2014/5/12/693
  
  Mike says in that email that he's opposing the addition of a property
  for clocks that is the equivalent of regulator-always-on. That's not
  what this is about. If at all it'd be a property to mark a clock that
  should not be disabled by default because it's essential.
 
 It's just semantic. How is a clock that should not be disabled by
 default because it's essential not a clock that stays always on?

Because a clock that should not be disabled by default can be turned off
when appropriate. A clock that is always on can't be turned off.

 Plus, you should read the mail further. It's clearly said that
 consumer drivers should call clk_prepare_enable themselves, and that
 the only exception to that is the case where we don't have such a
 driver (and only valid as a temporary exception, which I guess you'll
 soon turn into temporary-until-a-drm-kms-driver-is-merged).

Exactly. simplefb is only a temporary measure. It's not meant to be used
as a full-blown framebuffer driver. There are two use-cases where it is
an appropriate solution:

  1) As a stop-gap solution for when the platform doesn't support a full
 display driver yet. In that case you will want simplefb to stay
 around forever.

  2) To get early boot output before a full display driver can be loaded
 in which case simplefb should go away after the display driver has
 taken over.

In case of 2) the most straightforward solution is to not disable any
clocks until all drivers have had a chance to claim them. When the full
driver has been probed it should have claimed the clocks so they would
no longer get disabled.

For 1) I think it's fair to say that it's only a temporary solution to
get something on the screen. There won't be any kind of acceleration at
all, no power saving. Nothing but a dumb framebuffer. In that case it
should be a simple matter of keeping a select number of clocks always on
in the clock driver until support is more complete and it can be
properly handled. It's not like it's going to make a difference anyway
since simplefb won't ever disable them either.

  Adding Mike on this subthread too.
  
  Either way, Mark

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

Can you please fix you mail setup. You're addressing me directly in this
email, yet I'm neither in To: nor Cc: headers. That's really annoying.

On Tue, Aug 26, 2014 at 09:59:00PM +0200, Hans de Goede wrote:
 Hi,
 
 On 08/26/2014 04:35 PM, Thierry Reding wrote:
 On Tue, Aug 26, 2014 at 03:53:41PM +0200, Maxime Ripard wrote:
 On Tue, Aug 26, 2014 at 10:04:33AM +0200, Thierry Reding wrote:
 No. simplefb just wants to write to some memory that hardware has been
 set up to scan out. The platform requires that the clocks be on. Other
 platforms may not even allow turning off the clocks.
 
 Like what? the rpi? Come on. Just because the videocore is some black
 box we know nothing about doesn't mean we should use it as an example.
 
 You make it sound like the Raspberry Pi is somehow less important than
 sunxi.
 
 No. What I mean is that it seems like we are somehow punished, or at
 least blamed, for having a better and more complete kernel support.
 
 This isn't a competition. Nobody's punishing or blaming anyone. This is
 about finding the best solution for the problem at hand.
 
 Any decent enough SoC, with a decent support in the kernel will have
 clocks for this, and I really wonder how simplefb will behave once its
 clocks will be turned off...
 
 There are other devices besides ARM SoCs that may want to use this
 driver and that don't have clock support.
 
 And in this case, with this patch, simplefb will not claim any clock,
 nor will fail probing.
 
 But you're missing my point. What I'm saying is that the simplefb driver
 is meant to serve as a way to take over whatever framebuffer a firmware
 set up. Therefore I think it makes the most sense to assume that nothing
 needs to be controlled in any way since already been set up by firmware.
 Eventually there should be a driver that takes over from simplefb that
 knows how to properly handle the device's specifics, but that's not
 simplefb.
 
 I guess such a hand over if it were to happen in the kernel would
 involve the second driver claiming the resources before the first one
 release them. How is that different in this case?
 
 It's different in that that driver will be hardware specific and know
 exactly what clock and other resources are required. It will have a
 device-specific binding.
 
 The goal of this patch series is to keep clocks from being turned off.
 But that's not what it does. What it does is turn clocks on to prevent
 them from being turned off. In my opinion that's a workaround for a
 deficiency in the kernel (and the firmware/kernel interface) and I think
 it should be fixed at the root. So a much better solution would be to
 establish a way for firmware to communicate to the kernel that a given
 resource has been enabled by firmware and shouldn't be disabled. Such a
 solution can be implement for all types of resources and can be reused
 by all drivers since they don't have to worry about these details.
 
 Mike Turquette repeatedly said that he was against such a DT property:
 https://lkml.org/lkml/2014/5/12/693
 
 Mike says in that email that he's opposing the addition of a property
 for clocks that is the equivalent of regulator-always-on. That's not
 what this is about. If at all it'd be a property to mark a clock that
 should not be disabled by default because it's essential.
 
 Adding Mike on this subthread too.
 
 Either way, Mark already suggested a different alternative in another
 subthread, namely to add a new kind of checkpoint at which subsystems
 can call a disable unused function that's called later than a late
 initcall. This is not going to fix things for you immediately because
 the clocks will still be switched off (only later) if you don't have
 a real driver that's claiming the clocks. But you can work around that
 for now by making the relevant clocks always on and remove that
 workaround once a real driver is loaded that knows how to handle them
 properly.
 
 This will simply not work, and is a ton more complicated then
 simply teaching simplefb about a clocks property, which is a really simple
 and clean patch.
 
 First of all let me explain why this won't work. When should those
 subsystems call this later then a late initcall call ? the kms driver
 may very well be a kernel module, which will be loaded by userspace,
 so we would need this to be triggered by userspace, but when ?
 
 When the initrd is done? What then if the kms driver is not in the initrd,
 so maybe when udev is done enumerating devices, but what then if the kernel
 is still enumerating hardware at this time?

Usually an initrd knows how to wait for all or a given set of devices to
be probed. udev is pretty good for that.

 Will we just put a sleep 30
 in our initscripts to make sure the kernel is done scanning all busses,
 will that be long enough? Maybe we need to re-introduce the scsi_wait_done
 kernel module which was added as an ugly hack to allow userspace to wait
 for the kernel to have finished scanning scsi (and ata / sata) busses

Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[Thierry Reding]

On Wed, Aug 27, 2014 at 10:00:23AM +0200, Hans de Goede wrote:
 Hi,
 
 On 08/27/2014 08:54 AM, Thierry Reding wrote:
  On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote:
  On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote:
  Any decent enough SoC, with a decent support in the kernel will have
  clocks for this, and I really wonder how simplefb will behave once its
  clocks will be turned off...
 
  There are other devices besides ARM SoCs that may want to use this
  driver and that don't have clock support.
 
  And in this case, with this patch, simplefb will not claim any clock,
  nor will fail probing.
 
  But you're missing my point. What I'm saying is that the simplefb driver
  is meant to serve as a way to take over whatever framebuffer a firmware
  set up. Therefore I think it makes the most sense to assume that nothing
  needs to be controlled in any way since already been set up by firmware.
  Eventually there should be a driver that takes over from simplefb that
  knows how to properly handle the device's specifics, but that's not
  simplefb.
 
  I guess such a hand over if it were to happen in the kernel would
  involve the second driver claiming the resources before the first one
  release them. How is that different in this case?
 
  It's different in that that driver will be hardware specific and know
  exactly what clock and other resources are required. It will have a
  device-specific binding.
 
  Except that you made simplefb a generic driver. So we have two choices
  here: either we don't anything but the trivial case, and no one with a
  rather more advanced hardware will be able to use it, or we try to
  grab any resource that might be of use, which means clocks,
  regulators, reserved memory region, or whatever, so that we pretty
  much cover all cases. It really is as simple as that.
  
  No, it should be even simpler. simplefb shouldn't have to know any of
  this. It's just taking what firmware has set up and uses that. It's a
  stop-gap solution to provide information on the display until a real
  driver can be loaded and initializes the display hardware properly.
  
  The goal of this patch series is to keep clocks from being turned off.
  But that's not what it does. What it does is turn clocks on to prevent
  them from being turned off. In my opinion that's a workaround for a
  deficiency in the kernel (and the firmware/kernel interface) and I think
  it should be fixed at the root. So a much better solution would be to
  establish a way for firmware to communicate to the kernel that a given
  resource has been enabled by firmware and shouldn't be disabled. Such a
  solution can be implement for all types of resources and can be reused
  by all drivers since they don't have to worry about these details.
 
  Mike Turquette repeatedly said that he was against such a DT property:
  https://lkml.org/lkml/2014/5/12/693
 
  Mike says in that email that he's opposing the addition of a property
  for clocks that is the equivalent of regulator-always-on. That's not
  what this is about. If at all it'd be a property to mark a clock that
  should not be disabled by default because it's essential.
 
  It's just semantic. How is a clock that should not be disabled by
  default because it's essential not a clock that stays always on?
  
  Because a clock that should not be disabled by default can be turned off
  when appropriate. A clock that is always on can't be turned off.
  
  Plus, you should read the mail further. It's clearly said that
  consumer drivers should call clk_prepare_enable themselves, and that
  the only exception to that is the case where we don't have such a
  driver (and only valid as a temporary exception, which I guess you'll
  soon turn into temporary-until-a-drm-kms-driver-is-merged).
  
  Exactly. simplefb is only a temporary measure. It's not meant to be used
  as a full-blown framebuffer driver. There are two use-cases where it is
  an appropriate solution:
  
1) As a stop-gap solution for when the platform doesn't support a full
   display driver yet. In that case you will want simplefb to stay
   around forever.
  
2) To get early boot output before a full display driver can be loaded
   in which case simplefb should go away after the display driver has
   taken over.
  
  In case of 2) the most straightforward solution is to not disable any
  clocks until all drivers have had a chance to claim them. When the full
  driver has been probed it should have claimed the clocks so they would
  no longer get disabled.
 
 Please read my long reply from yesterday evening why this will never
 work, as there is not a well defined moment when all drivers have had a
 chance to claim them
 
 I've been doing low level Linux development for 10 years now and I've
 worked a lot on storage (raid, iscsi, fcoe support, etc.) in the past.

Ah, so this is now officially a pissing contest, is it?

Thierry


pgplLXTGz353G.pgp
Description