* Re: [PATCH 8/9 v2] video: fbdev: aty: use c99 initializers in structures
From: Josh Triplett @ 2014-08-23 16:03 UTC (permalink / raw)
To: Julia Lawall
Cc: Paul Mackerras, kernel-janitors, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev, linux-kernel
In-Reply-To: <alpine.DEB.2.02.1408231749030.2068@localhost6.localdomain6>
On Sat, Aug 23, 2014 at 05:50:28PM +0200, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> Use c99 initializers for structures.
>
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @decl@
> identifier i1,fld;
> type T;
> field list[n] fs;
> @@
>
> struct i1 {
> fs
> T fld;
> ...};
>
> @bad@
> identifier decl.i1,i2;
> expression e;
> initializer list[decl.n] is;
> @@
>
> struct i1 i2 = { is,
> + .fld = e
> - e
> ,...};
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> v2: Move close braces down to the next line and add trailing commas, as
> suggested by Josh Triplett.
>
> diff --git a/drivers/video/fbdev/aty/aty128fb.c b/drivers/video/fbdev/aty/aty128fb.c
> index ff60701..aedf2fb 100644
> --- a/drivers/video/fbdev/aty/aty128fb.c
> +++ b/drivers/video/fbdev/aty/aty128fb.c
> @@ -324,14 +324,61 @@ struct aty128_meminfo {
> };
>
> /* various memory configurations */
> -static const struct aty128_meminfo sdr_128 > - { 4, 4, 3, 3, 1, 3, 1, 16, 30, 16, "128-bit SDR SGRAM (1:1)" };
> -static const struct aty128_meminfo sdr_64 > - { 4, 8, 3, 3, 1, 3, 1, 17, 46, 17, "64-bit SDR SGRAM (1:1)" };
> -static const struct aty128_meminfo sdr_sgram > - { 4, 4, 1, 2, 1, 2, 1, 16, 24, 16, "64-bit SDR SGRAM (2:1)" };
> -static const struct aty128_meminfo ddr_sgram > - { 4, 4, 3, 3, 2, 3, 1, 16, 31, 16, "64-bit DDR SGRAM" };
> +static const struct aty128_meminfo sdr_128 = {
> + .ML = 4,
> + .MB = 4,
> + .Trcd = 3,
> + .Trp = 3,
> + .Twr = 1,
> + .CL = 3,
> + .Tr2w = 1,
> + .LoopLatency = 16,
> + .DspOn = 30,
> + .Rloop = 16,
> + .name = "128-bit SDR SGRAM (1:1)",
> +};
> +
> +static const struct aty128_meminfo sdr_64 = {
> + .ML = 4,
> + .MB = 8,
> + .Trcd = 3,
> + .Trp = 3,
> + .Twr = 1,
> + .CL = 3,
> + .Tr2w = 1,
> + .LoopLatency = 17,
> + .DspOn = 46,
> + .Rloop = 17,
> + .name = "64-bit SDR SGRAM (1:1)",
> +};
> +
> +static const struct aty128_meminfo sdr_sgram = {
> + .ML = 4,
> + .MB = 4,
> + .Trcd = 1,
> + .Trp = 2,
> + .Twr = 1,
> + .CL = 2,
> + .Tr2w = 1,
> + .LoopLatency = 16,
> + .DspOn = 24,
> + .Rloop = 16,
> + .name = "64-bit SDR SGRAM (2:1)",
> +};
> +
> +static const struct aty128_meminfo ddr_sgram = {
> + .ML = 4,
> + .MB = 4,
> + .Trcd = 3,
> + .Trp = 3,
> + .Twr = 2,
> + .CL = 3,
> + .Tr2w = 1,
> + .LoopLatency = 16,
> + .DspOn = 31,
> + .Rloop = 16,
> + .name = "64-bit DDR SGRAM",
> +};
>
> static struct fb_fix_screeninfo aty128fb_fix = {
> .id = "ATY Rage128",
^ permalink raw reply
* [PATCH 2/7] video: fbdev: intelfb: delete double assignment
From: Julia Lawall @ 2014-08-23 18:33 UTC (permalink / raw)
To: Maik Broemme
Cc: joe, kernel-janitors, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev, linux-kernel
In-Reply-To: <1408818808-18850-1-git-send-email-Julia.Lawall@lip6.fr>
From: Julia Lawall <Julia.Lawall@lip6.fr>
Delete successive assignments to the same location.
In the second case, = is converted to |=, which looks appropriate based on
the values involved.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
@@
expression i;
@@
*i = ...;
i = ...;
// </smpl>
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
The patches in this series do not depend on each other.
This changes the behavior of the code in the second case and is not tested.
drivers/video/fbdev/intelfb/intelfbhw.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/intelfb/intelfbhw.c b/drivers/video/fbdev/intelfb/intelfbhw.c
index fbad61d..d31ed4e 100644
--- a/drivers/video/fbdev/intelfb/intelfbhw.c
+++ b/drivers/video/fbdev/intelfb/intelfbhw.c
@@ -1191,7 +1191,6 @@ int intelfbhw_mode_to_hw(struct intelfb_info *dinfo,
vsync_end = vsync_start + var->vsync_len;
vtotal = vsync_end + var->upper_margin;
vblank_start = vactive;
- vblank_end = vtotal;
vblank_end = vsync_end + 1;
DBG_MSG("V: act %d, ss %d, se %d, tot %d bs %d, be %d\n",
@@ -1859,7 +1858,7 @@ void intelfbhw_cursor_init(struct intelfb_info *dinfo)
tmp = INREG(CURSOR_CONTROL);
tmp &= ~(CURSOR_FORMAT_MASK | CURSOR_GAMMA_ENABLE |
CURSOR_ENABLE | CURSOR_STRIDE_MASK);
- tmp = CURSOR_FORMAT_3C;
+ tmp |= CURSOR_FORMAT_3C;
OUTREG(CURSOR_CONTROL, tmp);
OUTREG(CURSOR_A_BASEADDR, dinfo->cursor.offset << 12);
tmp = (64 << CURSOR_SIZE_H_SHIFT) |
^ permalink raw reply related
* [PATCH 6/7] video: fbdev: sis: delete double assignment
From: Julia Lawall @ 2014-08-23 18:33 UTC (permalink / raw)
To: Thomas Winischhofer
Cc: joe, kernel-janitors, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, linux-fbdev, linux-kernel
In-Reply-To: <1408818808-18850-1-git-send-email-Julia.Lawall@lip6.fr>
From: Julia Lawall <Julia.Lawall@lip6.fr>
Delete successive assignments to the same location. The second assignment
is changed to update a different field, as done in other nearby code.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
@@
expression i;
@@
*i = ...;
i = ...;
// </smpl>
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
The patches in this series do not depend on each other.
This changes the semantics of the code and is not tested.
drivers/video/fbdev/sis/init301.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/sis/init301.c b/drivers/video/fbdev/sis/init301.c
index a89e3ca..295e0de 100644
--- a/drivers/video/fbdev/sis/init301.c
+++ b/drivers/video/fbdev/sis/init301.c
@@ -1714,7 +1714,7 @@ SiS_GetLCDResInfo(struct SiS_Private *SiS_Pr, unsigned short ModeNo, unsigned sh
SiS_Pr->PanelVCLKIdx315 = VCLK81_315; /* ? */
} else {
SiS_Pr->PanelHT = 1688; SiS_Pr->PanelVT = 802;
- SiS_Pr->PanelHRS = 48; SiS_Pr->PanelHRS = 112;
+ SiS_Pr->PanelHRS = 48; SiS_Pr->PanelHRE = 112;
SiS_Pr->PanelVRS = 3; SiS_Pr->PanelVRE = 6;
SiS_Pr->PanelVCLKIdx300 = VCLK81_300;
SiS_Pr->PanelVCLKIdx315 = VCLK81_315;
^ permalink raw reply related
* Re: [PATCH 6/7] video: fbdev: sis: delete double assignment
From: Geert Uytterhoeven @ 2014-08-24 18:21 UTC (permalink / raw)
To: Julia Lawall
Cc: Thomas Winischhofer, Joe Perches, kernel-janitors@vger.kernel.org,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Linux Fbdev development list, linux-kernel@vger.kernel.org
In-Reply-To: <1408818808-18850-7-git-send-email-Julia.Lawall@lip6.fr>
On Sat, Aug 23, 2014 at 8:33 PM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> ---
> The patches in this series do not depend on each other.
>
> This changes the semantics of the code and is not tested.
Hence I think you should change the subject of the patch, so it's obvious
some bug is fixed, e.g. "video: fbdev: sis: Fix double assignment".
> drivers/video/fbdev/sis/init301.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/sis/init301.c b/drivers/video/fbdev/sis/init301.c
> index a89e3ca..295e0de 100644
> --- a/drivers/video/fbdev/sis/init301.c
> +++ b/drivers/video/fbdev/sis/init301.c
> @@ -1714,7 +1714,7 @@ SiS_GetLCDResInfo(struct SiS_Private *SiS_Pr, unsigned short ModeNo, unsigned sh
> SiS_Pr->PanelVCLKIdx315 = VCLK81_315; /* ? */
> } else {
> SiS_Pr->PanelHT = 1688; SiS_Pr->PanelVT = 802;
> - SiS_Pr->PanelHRS = 48; SiS_Pr->PanelHRS = 112;
> + SiS_Pr->PanelHRS = 48; SiS_Pr->PanelHRE = 112;
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 6/7] video: fbdev: sis: delete double assignment
From: Julia Lawall @ 2014-08-24 18:40 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Julia Lawall, Thomas Winischhofer, Joe Perches,
kernel-janitors@vger.kernel.org, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Linux Fbdev development list,
linux-kernel@vger.kernel.org
In-Reply-To: <CAMuHMdVERt9XBN5OW8QqT_PvQVHd6xgnCdjRpiSNp7ZSEgXWSg@mail.gmail.com>
On Sun, 24 Aug 2014, Geert Uytterhoeven wrote:
> On Sat, Aug 23, 2014 at 8:33 PM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
> > ---
> > The patches in this series do not depend on each other.
> >
> > This changes the semantics of the code and is not tested.
>
> Hence I think you should change the subject of the patch, so it's obvious
> some bug is fixed, e.g. "video: fbdev: sis: Fix double assignment".
Do you want me to resend this patch with the new subject, or just keep it
in mind for the future?
thanks,
julia
>
> > drivers/video/fbdev/sis/init301.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/video/fbdev/sis/init301.c b/drivers/video/fbdev/sis/init301.c
> > index a89e3ca..295e0de 100644
> > --- a/drivers/video/fbdev/sis/init301.c
> > +++ b/drivers/video/fbdev/sis/init301.c
> > @@ -1714,7 +1714,7 @@ SiS_GetLCDResInfo(struct SiS_Private *SiS_Pr, unsigned short ModeNo, unsigned sh
> > SiS_Pr->PanelVCLKIdx315 = VCLK81_315; /* ? */
> > } else {
> > SiS_Pr->PanelHT = 1688; SiS_Pr->PanelVT = 802;
> > - SiS_Pr->PanelHRS = 48; SiS_Pr->PanelHRS = 112;
> > + SiS_Pr->PanelHRS = 48; SiS_Pr->PanelHRE = 112;
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH 6/7] video: fbdev: sis: delete double assignment
From: Geert Uytterhoeven @ 2014-08-25 11:36 UTC (permalink / raw)
To: Julia Lawall
Cc: Thomas Winischhofer, Joe Perches, kernel-janitors@vger.kernel.org,
Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
Linux Fbdev development list, linux-kernel@vger.kernel.org
In-Reply-To: <alpine.DEB.2.02.1408242040020.2066@localhost6.localdomain6>
Hi Julia,
On Sun, Aug 24, 2014 at 8:40 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> On Sat, Aug 23, 2014 at 8:33 PM, Julia Lawall <Julia.Lawall@lip6.fr> wrote:
>> > ---
>> > The patches in this series do not depend on each other.
>> >
>> > This changes the semantics of the code and is not tested.
>>
>> Hence I think you should change the subject of the patch, so it's obvious
>> some bug is fixed, e.g. "video: fbdev: sis: Fix double assignment".
>
> Do you want me to resend this patch with the new subject, or just keep it
> in mind for the future?
That's up to the fbdev maintainer.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-25 12:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140813170106.GT15297@lukather>
[-- Attachment #1: Type: text/plain, Size: 735 bytes --]
On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
[...]
> > If not, perhaps the clock driver should force the clock to be
> > enabled (perhaps only if the DRM/KMS driver isn't enabled?).
>
> I'm sorry, but I'm not going to take any code that will do that in our
> clock driver.
>
> I'm not going to have a huge list of ifdef depending on configuration
> options to know which clock to enable, especially when clk_get should
> have the consumer device as an argument.
Are you saying is that you want to solve a platform-specific problem by
pushing code into simple, generic drivers so that your platform code can
stay "clean"?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-25 12:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825121228.GB4163@ulmo.nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]
On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> > On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> [...]
> > > If not, perhaps the clock driver should force the clock to be
> > > enabled (perhaps only if the DRM/KMS driver isn't enabled?).
> >
> > I'm sorry, but I'm not going to take any code that will do that in our
> > clock driver.
> >
> > I'm not going to have a huge list of ifdef depending on configuration
> > options to know which clock to enable, especially when clk_get should
> > have the consumer device as an argument.
>
> Are you saying is that you want to solve a platform-specific problem by
> pushing code into simple, generic drivers so that your platform code can
> stay "clean"?
Are you saying that this driver would become "dirty" with such a patch?
If so, we really have an issue in the kernel.
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-25 13:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825124410.GZ15297@lukather>
[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]
On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> > On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> > > On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> > [...]
> > > > If not, perhaps the clock driver should force the clock to be
> > > > enabled (perhaps only if the DRM/KMS driver isn't enabled?).
> > >
> > > I'm sorry, but I'm not going to take any code that will do that in our
> > > clock driver.
> > >
> > > I'm not going to have a huge list of ifdef depending on configuration
> > > options to know which clock to enable, especially when clk_get should
> > > have the consumer device as an argument.
> >
> > Are you saying is that you want to solve a platform-specific problem by
> > pushing code into simple, generic drivers so that your platform code can
> > stay "clean"?
>
> Are you saying that this driver would become "dirty" with such a patch?
Yes. Others have said the same and even provided alternative solutions
on how to solve what's seemingly a platform-specific problem in a
platform-specific way.
> If so, we really have an issue in the kernel.
Can you elaborate?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Hans de Goede @ 2014-08-25 13:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825133953.GJ4163@ulmo.nvidia.com>
Hi,
On 08/25/2014 03:39 PM, Thierry Reding wrote:
> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>>> [...]
>>>>> If not, perhaps the clock driver should force the clock to be
>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
>>>>
>>>> I'm sorry, but I'm not going to take any code that will do that in our
>>>> clock driver.
>>>>
>>>> I'm not going to have a huge list of ifdef depending on configuration
>>>> options to know which clock to enable, especially when clk_get should
>>>> have the consumer device as an argument.
>>>
>>> Are you saying is that you want to solve a platform-specific problem by
>>> pushing code into simple, generic drivers so that your platform code can
>>> stay "clean"?
>>
>> Are you saying that this driver would become "dirty" with such a patch?
>
> Yes. Others have said the same and even provided alternative solutions
> on how to solve what's seemingly a platform-specific problem in a
> platform-specific way.
This is not platform specific, any platform with a complete clock driver
will suffer from the same problem (the clock driver disabling unclaimed
ahb gates, and thus killing the video output) if it wants to use simplefb
for early console support.
I can only assume that this problem was never hit on tegra because when
kms support (and thus also a clock driver for the video plls) was introduced
simplefb support was dropped at the same time, or the gates are not being
disabled for some other reason.
As for the suggestion to simply never disable the plls / ahb gates by blocking
them from ever being disabled in the sunxi clock driver, that is not really
a solution either, as we want to be able to turn these things off to safe
power on screen blank once control has been turned over to the kms driver.
And while at it let me also tackle the don't use simplefb only use kms argument,
that means that the clocks will be turned off until the kms module loads, which
will cause noticable screen flicker / video output resync, something which we've
been trying to get rid of for years now.
And no, build in the kms driver is not an answer either. That works nicely for
firmware, but not for generic Linux distributions supporting a wide range
of boards.
Regards,
Hans
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-25 14:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53FB3E7F.4000503@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2939 bytes --]
On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
> On 08/25/2014 03:39 PM, Thierry Reding wrote:
> > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> >>> [...]
> >>>>> If not, perhaps the clock driver should force the clock to be
> >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
> >>>>
> >>>> I'm sorry, but I'm not going to take any code that will do that in our
> >>>> clock driver.
> >>>>
> >>>> I'm not going to have a huge list of ifdef depending on configuration
> >>>> options to know which clock to enable, especially when clk_get should
> >>>> have the consumer device as an argument.
> >>>
> >>> Are you saying is that you want to solve a platform-specific problem by
> >>> pushing code into simple, generic drivers so that your platform code can
> >>> stay "clean"?
> >>
> >> Are you saying that this driver would become "dirty" with such a patch?
> >
> > Yes. Others have said the same and even provided alternative solutions
> > on how to solve what's seemingly a platform-specific problem in a
> > platform-specific way.
>
> This is not platform specific, any platform with a complete clock driver
> will suffer from the same problem (the clock driver disabling unclaimed
> ahb gates, and thus killing the video output) if it wants to use simplefb
> for early console support.
It is platform specific in that your platform may require certain clocks
to remain on. The next platform may require power domains to remain on
during boot and yet another one may rely on regulators to stay on during
boot. By your argument simplefb will need to be taught to handle pretty
much every type of resource that the kernel has.
> As for the suggestion to simply never disable the plls / ahb gates by blocking
> them from ever being disabled in the sunxi clock driver, that is not really
> a solution either, as we want to be able to turn these things off to safe
> power on screen blank once control has been turned over to the kms driver.
Then perhaps part of the hand-off procedure between simplefb and DRM/KMS
should involve marking PLLs or "gates" as properly managed.
> And while at it let me also tackle the don't use simplefb only use kms argument,
> that means that the clocks will be turned off until the kms module loads, which
> will cause noticable screen flicker / video output resync, something which we've
> been trying to get rid of for years now.
>
> And no, build in the kms driver is not an answer either. That works nicely for
> firmware, but not for generic Linux distributions supporting a wide range
> of boards.
Odd... I didn't offer any of those two as solutions to the problem.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: jonsmirl @ 2014-08-25 14:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825141600.GA14763@ulmo.nvidia.com>
On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
>> On 08/25/2014 03:39 PM, Thierry Reding wrote:
>> > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
>> >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
>> >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
>> >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>> >>> [...]
>> >>>>> If not, perhaps the clock driver should force the clock to be
>> >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
>> >>>>
>> >>>> I'm sorry, but I'm not going to take any code that will do that in our
>> >>>> clock driver.
>> >>>>
>> >>>> I'm not going to have a huge list of ifdef depending on configuration
>> >>>> options to know which clock to enable, especially when clk_get should
>> >>>> have the consumer device as an argument.
>> >>>
>> >>> Are you saying is that you want to solve a platform-specific problem by
>> >>> pushing code into simple, generic drivers so that your platform code can
>> >>> stay "clean"?
>> >>
>> >> Are you saying that this driver would become "dirty" with such a patch?
>> >
>> > Yes. Others have said the same and even provided alternative solutions
>> > on how to solve what's seemingly a platform-specific problem in a
>> > platform-specific way.
>>
>> This is not platform specific, any platform with a complete clock driver
>> will suffer from the same problem (the clock driver disabling unclaimed
>> ahb gates, and thus killing the video output) if it wants to use simplefb
>> for early console support.
>
> It is platform specific in that your platform may require certain clocks
> to remain on. The next platform may require power domains to remain on
> during boot and yet another one may rely on regulators to stay on during
> boot. By your argument simplefb will need to be taught to handle pretty
> much every type of resource that the kernel has.
Why can't simplefb be a driver library that is called from a device
specific device driver that only claims the clocks (or regulators)?
Then build all of these device specific drivers into the generic ARM
kernel. They will be quite small since all they do is claim the clocks
(or regulator). Maybe we can even figure out some protocol for
removing the unused ones from memory later.
Later during the boot process the device specific driver can load its
KMS code which has also been implemented as a driver library. Maybe
use E_PROBE_DEFER to do this. Match on the device ID, claim the
clocks, defer until the full KMS library can be loaded.
>
>> As for the suggestion to simply never disable the plls / ahb gates by blocking
>> them from ever being disabled in the sunxi clock driver, that is not really
>> a solution either, as we want to be able to turn these things off to safe
>> power on screen blank once control has been turned over to the kms driver.
>
> Then perhaps part of the hand-off procedure between simplefb and DRM/KMS
> should involve marking PLLs or "gates" as properly managed.
>
>> And while at it let me also tackle the don't use simplefb only use kms argument,
>> that means that the clocks will be turned off until the kms module loads, which
>> will cause noticable screen flicker / video output resync, something which we've
>> been trying to get rid of for years now.
>>
>> And no, build in the kms driver is not an answer either. That works nicely for
>> firmware, but not for generic Linux distributions supporting a wide range
>> of boards.
>
> Odd... I didn't offer any of those two as solutions to the problem.
>
> Thierry
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Hans de Goede @ 2014-08-25 14:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825141600.GA14763@ulmo.nvidia.com>
Hi,
On 08/25/2014 04:16 PM, Thierry Reding wrote:
> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
>> On 08/25/2014 03:39 PM, Thierry Reding wrote:
>>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
>>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
>>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
>>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>>>>> [...]
>>>>>>> If not, perhaps the clock driver should force the clock to be
>>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
>>>>>>
>>>>>> I'm sorry, but I'm not going to take any code that will do that in our
>>>>>> clock driver.
>>>>>>
>>>>>> I'm not going to have a huge list of ifdef depending on configuration
>>>>>> options to know which clock to enable, especially when clk_get should
>>>>>> have the consumer device as an argument.
>>>>>
>>>>> Are you saying is that you want to solve a platform-specific problem by
>>>>> pushing code into simple, generic drivers so that your platform code can
>>>>> stay "clean"?
>>>>
>>>> Are you saying that this driver would become "dirty" with such a patch?
>>>
>>> Yes. Others have said the same and even provided alternative solutions
>>> on how to solve what's seemingly a platform-specific problem in a
>>> platform-specific way.
>>
>> This is not platform specific, any platform with a complete clock driver
>> will suffer from the same problem (the clock driver disabling unclaimed
>> ahb gates, and thus killing the video output) if it wants to use simplefb
>> for early console support.
>
> It is platform specific in that your platform may require certain clocks
> to remain on. The next platform may require power domains to remain on
> during boot and yet another one may rely on regulators to stay on during
> boot. By your argument simplefb will need to be taught to handle pretty
> much every type of resource that the kernel has.
>
>> As for the suggestion to simply never disable the plls / ahb gates by blocking
>> them from ever being disabled in the sunxi clock driver, that is not really
>> a solution either, as we want to be able to turn these things off to safe
>> power on screen blank once control has been turned over to the kms driver.
>
> Then perhaps part of the hand-off procedure between simplefb and DRM/KMS
> should involve marking PLLs or "gates" as properly managed.
And by your earlier argument also power domains, regulators, etc. So now we need
to add code to each of the clock core, power-domain core, regulator core, etc. to
have them now about this initially unmanaged state thing you're introducing, as
well as modify all involved clock / regulator / etc. drivers to mark certain
resources as unmanaged.
Or we add a single simple and clean patch to the simplefb driver for dealing
with clocks, and worry about all the other hypothetical problems later...
Regards,
Hans
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Hans de Goede @ 2014-08-25 14:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKON4OyjJ+QcBGOzrUDH9J+DxBEKLmXeOcnUyOHHHV_DtCG0zA@mail.gmail.com>
Hi,
On 08/25/2014 04:23 PM, jonsmirl@gmail.com wrote:
> On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
>>> On 08/25/2014 03:39 PM, Thierry Reding wrote:
>>>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
>>>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
>>>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
>>>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>>>>>> [...]
>>>>>>>> If not, perhaps the clock driver should force the clock to be
>>>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
>>>>>>>
>>>>>>> I'm sorry, but I'm not going to take any code that will do that in our
>>>>>>> clock driver.
>>>>>>>
>>>>>>> I'm not going to have a huge list of ifdef depending on configuration
>>>>>>> options to know which clock to enable, especially when clk_get should
>>>>>>> have the consumer device as an argument.
>>>>>>
>>>>>> Are you saying is that you want to solve a platform-specific problem by
>>>>>> pushing code into simple, generic drivers so that your platform code can
>>>>>> stay "clean"?
>>>>>
>>>>> Are you saying that this driver would become "dirty" with such a patch?
>>>>
>>>> Yes. Others have said the same and even provided alternative solutions
>>>> on how to solve what's seemingly a platform-specific problem in a
>>>> platform-specific way.
>>>
>>> This is not platform specific, any platform with a complete clock driver
>>> will suffer from the same problem (the clock driver disabling unclaimed
>>> ahb gates, and thus killing the video output) if it wants to use simplefb
>>> for early console support.
>>
>> It is platform specific in that your platform may require certain clocks
>> to remain on. The next platform may require power domains to remain on
>> during boot and yet another one may rely on regulators to stay on during
>> boot. By your argument simplefb will need to be taught to handle pretty
>> much every type of resource that the kernel has.
>
> Why can't simplefb be a driver library that is called from a device
> specific device driver that only claims the clocks (or regulators)?
> Then build all of these device specific drivers into the generic ARM
> kernel. They will be quite small since all they do is claim the clocks
> (or regulator). Maybe we can even figure out some protocol for
> removing the unused ones from memory later.
>
> Later during the boot process the device specific driver can load its
> KMS code which has also been implemented as a driver library. Maybe
> use E_PROBE_DEFER to do this. Match on the device ID, claim the
> clocks, defer until the full KMS library can be loaded.
There is no need for all this complexity, all that is needed is for the
simplefb driver to be thought to claim + enable any clocks listed in
its dt node.
Then once we want to do a handover, all is needed is a single simplefb
unregister call, at which point simplefb will disable the clocks and
release them. Note that this will be a nop as they should already be
claimed and enabled by the kms driver at this time.
Regards,
Hans
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-25 14:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53FB46FF.1010208@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 3479 bytes --]
On Mon, Aug 25, 2014 at 04:23:59PM +0200, Hans de Goede wrote:
> Hi,
>
> On 08/25/2014 04:16 PM, Thierry Reding wrote:
> > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
> >> On 08/25/2014 03:39 PM, Thierry Reding wrote:
> >>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> >>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> >>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> >>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> >>>>> [...]
> >>>>>>> If not, perhaps the clock driver should force the clock to be
> >>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
> >>>>>>
> >>>>>> I'm sorry, but I'm not going to take any code that will do that in our
> >>>>>> clock driver.
> >>>>>>
> >>>>>> I'm not going to have a huge list of ifdef depending on configuration
> >>>>>> options to know which clock to enable, especially when clk_get should
> >>>>>> have the consumer device as an argument.
> >>>>>
> >>>>> Are you saying is that you want to solve a platform-specific problem by
> >>>>> pushing code into simple, generic drivers so that your platform code can
> >>>>> stay "clean"?
> >>>>
> >>>> Are you saying that this driver would become "dirty" with such a patch?
> >>>
> >>> Yes. Others have said the same and even provided alternative solutions
> >>> on how to solve what's seemingly a platform-specific problem in a
> >>> platform-specific way.
> >>
> >> This is not platform specific, any platform with a complete clock driver
> >> will suffer from the same problem (the clock driver disabling unclaimed
> >> ahb gates, and thus killing the video output) if it wants to use simplefb
> >> for early console support.
> >
> > It is platform specific in that your platform may require certain clocks
> > to remain on. The next platform may require power domains to remain on
> > during boot and yet another one may rely on regulators to stay on during
> > boot. By your argument simplefb will need to be taught to handle pretty
> > much every type of resource that the kernel has.
> >
> >> As for the suggestion to simply never disable the plls / ahb gates by blocking
> >> them from ever being disabled in the sunxi clock driver, that is not really
> >> a solution either, as we want to be able to turn these things off to safe
> >> power on screen blank once control has been turned over to the kms driver.
> >
> > Then perhaps part of the hand-off procedure between simplefb and DRM/KMS
> > should involve marking PLLs or "gates" as properly managed.
>
> And by your earlier argument also power domains, regulators, etc. So now we need
> to add code to each of the clock core, power-domain core, regulator core, etc. to
> have them now about this initially unmanaged state thing you're introducing, as
> well as modify all involved clock / regulator / etc. drivers to mark certain
> resources as unmanaged.
Hmm... that's true. But we already have a way to deal with exactly this
situation for regulators. There's a property called regulator-boot-on
which a bootloader should set whet it has enabled a given regulator. It
can of course also be set statically in a DTS if it's know upfront that
a bootloader will always enable it. Perhaps what we need is a similar
property for clocks so that the clock framework will not inadvertently
turn off a clock that's still being used.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-25 14:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825141600.GA14763@ulmo.nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 2599 bytes --]
On Mon, Aug 25, 2014 at 04:16:29PM +0200, Thierry Reding wrote:
> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
> > On 08/25/2014 03:39 PM, Thierry Reding wrote:
> > > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> > >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> > >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> > >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> > >>> [...]
> > >>>>> If not, perhaps the clock driver should force the clock to be
> > >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
> > >>>>
> > >>>> I'm sorry, but I'm not going to take any code that will do that in our
> > >>>> clock driver.
> > >>>>
> > >>>> I'm not going to have a huge list of ifdef depending on configuration
> > >>>> options to know which clock to enable, especially when clk_get should
> > >>>> have the consumer device as an argument.
> > >>>
> > >>> Are you saying is that you want to solve a platform-specific problem by
> > >>> pushing code into simple, generic drivers so that your platform code can
> > >>> stay "clean"?
> > >>
> > >> Are you saying that this driver would become "dirty" with such a patch?
> > >
> > > Yes. Others have said the same and even provided alternative solutions
> > > on how to solve what's seemingly a platform-specific problem in a
> > > platform-specific way.
> >
> > This is not platform specific, any platform with a complete clock driver
> > will suffer from the same problem (the clock driver disabling unclaimed
> > ahb gates, and thus killing the video output) if it wants to use simplefb
> > for early console support.
>
> It is platform specific in that your platform may require certain clocks
> to remain on.
The platform doesn't. simplefb does. simplefb is the obvious consumer
for these clocks, and given the current API and abstraction we have,
it should be the one claiming the clocks too.
> The next platform may require power domains to remain on during boot
> and yet another one may rely on regulators to stay on during
> boot. By your argument simplefb will need to be taught to handle
> pretty much every type of resource that the kernel has.
And I wouldn't find anything wrong with that. We're already doing so
for any generic driver in the kernel (AHCI, EHCI comes to my mind
first, there's probably a lot of others). Why wouldn't we do as such
for this one?
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-25 15:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKON4OyjJ+QcBGOzrUDH9J+DxBEKLmXeOcnUyOHHHV_DtCG0zA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3567 bytes --]
On Mon, Aug 25, 2014 at 10:23:26AM -0400, jonsmirl@gmail.com wrote:
> On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
> >> On 08/25/2014 03:39 PM, Thierry Reding wrote:
> >> > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> >> >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> >> >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> >> >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> >> >>> [...]
> >> >>>>> If not, perhaps the clock driver should force the clock to be
> >> >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
> >> >>>>
> >> >>>> I'm sorry, but I'm not going to take any code that will do that in our
> >> >>>> clock driver.
> >> >>>>
> >> >>>> I'm not going to have a huge list of ifdef depending on configuration
> >> >>>> options to know which clock to enable, especially when clk_get should
> >> >>>> have the consumer device as an argument.
> >> >>>
> >> >>> Are you saying is that you want to solve a platform-specific problem by
> >> >>> pushing code into simple, generic drivers so that your platform code can
> >> >>> stay "clean"?
> >> >>
> >> >> Are you saying that this driver would become "dirty" with such a patch?
> >> >
> >> > Yes. Others have said the same and even provided alternative solutions
> >> > on how to solve what's seemingly a platform-specific problem in a
> >> > platform-specific way.
> >>
> >> This is not platform specific, any platform with a complete clock driver
> >> will suffer from the same problem (the clock driver disabling unclaimed
> >> ahb gates, and thus killing the video output) if it wants to use simplefb
> >> for early console support.
> >
> > It is platform specific in that your platform may require certain clocks
> > to remain on. The next platform may require power domains to remain on
> > during boot and yet another one may rely on regulators to stay on during
> > boot. By your argument simplefb will need to be taught to handle pretty
> > much every type of resource that the kernel has.
>
> Why can't simplefb be a driver library that is called from a device
> specific device driver that only claims the clocks (or regulators)?
> Then build all of these device specific drivers into the generic ARM
> kernel. They will be quite small since all they do is claim the clocks
> (or regulator). Maybe we can even figure out some protocol for
> removing the unused ones from memory later.
>
> Later during the boot process the device specific driver can load its
> KMS code which has also been implemented as a driver library. Maybe
> use E_PROBE_DEFER to do this. Match on the device ID, claim the
> clocks, defer until the full KMS library can be loaded.
That sounds like the most scalable solution so far. On the other hand,
as I understand it, the simplefb driver was designed to take over the
framebuffer set up by firmware, so it's somewhat odd that the driver
would have to deal with resources in the first place. If we push the
resource problem into the respective subsystems we keep the simplefb
driver completely hardware agnostic.
And we'll also be solving this problem for other types of drivers at the
same time. Firmware may after all initialize clocks and other resources
for other types of devices too. Handling resources in the drivers would
therefore imply that every driver needs to cope with this.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-25 15:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825145854.GA15297@lukather>
[-- Attachment #1: Type: text/plain, Size: 2924 bytes --]
On Mon, Aug 25, 2014 at 04:58:54PM +0200, Maxime Ripard wrote:
> On Mon, Aug 25, 2014 at 04:16:29PM +0200, Thierry Reding wrote:
> > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
> > > On 08/25/2014 03:39 PM, Thierry Reding wrote:
> > > > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> > > >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> > > >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> > > >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> > > >>> [...]
> > > >>>>> If not, perhaps the clock driver should force the clock to be
> > > >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
> > > >>>>
> > > >>>> I'm sorry, but I'm not going to take any code that will do that in our
> > > >>>> clock driver.
> > > >>>>
> > > >>>> I'm not going to have a huge list of ifdef depending on configuration
> > > >>>> options to know which clock to enable, especially when clk_get should
> > > >>>> have the consumer device as an argument.
> > > >>>
> > > >>> Are you saying is that you want to solve a platform-specific problem by
> > > >>> pushing code into simple, generic drivers so that your platform code can
> > > >>> stay "clean"?
> > > >>
> > > >> Are you saying that this driver would become "dirty" with such a patch?
> > > >
> > > > Yes. Others have said the same and even provided alternative solutions
> > > > on how to solve what's seemingly a platform-specific problem in a
> > > > platform-specific way.
> > >
> > > This is not platform specific, any platform with a complete clock driver
> > > will suffer from the same problem (the clock driver disabling unclaimed
> > > ahb gates, and thus killing the video output) if it wants to use simplefb
> > > for early console support.
> >
> > It is platform specific in that your platform may require certain clocks
> > to remain on.
>
> The platform doesn't. simplefb does. simplefb is the obvious consumer
> for these clocks, and given the current API and abstraction we have,
> it should be the one claiming the clocks too.
No. simplefb just wants to write to some memory that hardware has been
set up to scan out. The platform requires that the clocks be on. Other
platforms may not even allow turning off the clocks.
> > The next platform may require power domains to remain on during boot
> > and yet another one may rely on regulators to stay on during
> > boot. By your argument simplefb will need to be taught to handle
> > pretty much every type of resource that the kernel has.
>
> And I wouldn't find anything wrong with that. We're already doing so
> for any generic driver in the kernel (AHCI, EHCI comes to my mind
> first, there's probably a lot of others). Why wouldn't we do as such
> for this one?
Yes, and we've had similar discussions in those subsystems too.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-25 15:07 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825145303.GC14763@ulmo.nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 867 bytes --]
On Mon, Aug 25, 2014 at 04:53:06PM +0200, Thierry Reding wrote:
> Hmm... that's true. But we already have a way to deal with exactly this
> situation for regulators. There's a property called regulator-boot-on
> which a bootloader should set whet it has enabled a given regulator. It
> can of course also be set statically in a DTS if it's know upfront that
> a bootloader will always enable it. Perhaps what we need is a similar
> property for clocks so that the clock framework will not inadvertently
> turn off a clock that's still being used.
Except that such a property won't work either. Regulators with
regulator-boot-on will still be disabled if there's no one to claim
it. Just like what happens currently for the clocks.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: jonsmirl @ 2014-08-25 15:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825145303.GC14763@ulmo.nvidia.com>
On Mon, Aug 25, 2014 at 10:53 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Aug 25, 2014 at 04:23:59PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 08/25/2014 04:16 PM, Thierry Reding wrote:
>> > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
>> >> On 08/25/2014 03:39 PM, Thierry Reding wrote:
>> >>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
>> >>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
>> >>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
>> >>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>> >>>>> [...]
>> >>>>>>> If not, perhaps the clock driver should force the clock to be
>> >>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
>> >>>>>>
>> >>>>>> I'm sorry, but I'm not going to take any code that will do that in our
>> >>>>>> clock driver.
>> >>>>>>
>> >>>>>> I'm not going to have a huge list of ifdef depending on configuration
>> >>>>>> options to know which clock to enable, especially when clk_get should
>> >>>>>> have the consumer device as an argument.
>> >>>>>
>> >>>>> Are you saying is that you want to solve a platform-specific problem by
>> >>>>> pushing code into simple, generic drivers so that your platform code can
>> >>>>> stay "clean"?
>> >>>>
>> >>>> Are you saying that this driver would become "dirty" with such a patch?
>> >>>
>> >>> Yes. Others have said the same and even provided alternative solutions
>> >>> on how to solve what's seemingly a platform-specific problem in a
>> >>> platform-specific way.
>> >>
>> >> This is not platform specific, any platform with a complete clock driver
>> >> will suffer from the same problem (the clock driver disabling unclaimed
>> >> ahb gates, and thus killing the video output) if it wants to use simplefb
>> >> for early console support.
>> >
>> > It is platform specific in that your platform may require certain clocks
>> > to remain on. The next platform may require power domains to remain on
>> > during boot and yet another one may rely on regulators to stay on during
>> > boot. By your argument simplefb will need to be taught to handle pretty
>> > much every type of resource that the kernel has.
>> >
>> >> As for the suggestion to simply never disable the plls / ahb gates by blocking
>> >> them from ever being disabled in the sunxi clock driver, that is not really
>> >> a solution either, as we want to be able to turn these things off to safe
>> >> power on screen blank once control has been turned over to the kms driver.
>> >
>> > Then perhaps part of the hand-off procedure between simplefb and DRM/KMS
>> > should involve marking PLLs or "gates" as properly managed.
>>
>> And by your earlier argument also power domains, regulators, etc. So now we need
>> to add code to each of the clock core, power-domain core, regulator core, etc. to
>> have them now about this initially unmanaged state thing you're introducing, as
>> well as modify all involved clock / regulator / etc. drivers to mark certain
>> resources as unmanaged.
>
> Hmm... that's true. But we already have a way to deal with exactly this
> situation for regulators. There's a property called regulator-boot-on
> which a bootloader should set whet it has enabled a given regulator. It
> can of course also be set statically in a DTS if it's know upfront that
> a bootloader will always enable it. Perhaps what we need is a similar
> property for clocks so that the clock framework will not inadvertently
> turn off a clock that's still being used.
There should probably be a generic 'boot-initialized;' property that
can be added to any DT device node. Then uboot can add that property
to the device node for anything it has turned on.
You could even use it to add more info 'boot-initialized = <"9600 8 N 1">;'
That passes the info in an OS agnostic manner.
>
> Thierry
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Luc Verhaegen @ 2014-08-25 15:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825150501.GE14763@ulmo.nvidia.com>
On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote:
>
> No. simplefb just wants to write to some memory that hardware has been
> set up to scan out. The platform requires that the clocks be on.
Simplefb also requires that the memory is there and is persistent. Fine
for discrete graphics cards, fine for rpi where most things are hidden
from the ARM core anyway, not so fine for anybody else.
Luc Verhaegen.
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Thierry Reding @ 2014-08-25 15:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53FB47B8.2090104@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 3384 bytes --]
On Mon, Aug 25, 2014 at 04:27:04PM +0200, Hans de Goede wrote:
> Hi,
>
> On 08/25/2014 04:23 PM, jonsmirl@gmail.com wrote:
> > On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding
> > <thierry.reding@gmail.com> wrote:
> >> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
> >>> On 08/25/2014 03:39 PM, Thierry Reding wrote:
> >>>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> >>>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> >>>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> >>>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> >>>>>> [...]
> >>>>>>>> If not, perhaps the clock driver should force the clock to be
> >>>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
> >>>>>>>
> >>>>>>> I'm sorry, but I'm not going to take any code that will do that in our
> >>>>>>> clock driver.
> >>>>>>>
> >>>>>>> I'm not going to have a huge list of ifdef depending on configuration
> >>>>>>> options to know which clock to enable, especially when clk_get should
> >>>>>>> have the consumer device as an argument.
> >>>>>>
> >>>>>> Are you saying is that you want to solve a platform-specific problem by
> >>>>>> pushing code into simple, generic drivers so that your platform code can
> >>>>>> stay "clean"?
> >>>>>
> >>>>> Are you saying that this driver would become "dirty" with such a patch?
> >>>>
> >>>> Yes. Others have said the same and even provided alternative solutions
> >>>> on how to solve what's seemingly a platform-specific problem in a
> >>>> platform-specific way.
> >>>
> >>> This is not platform specific, any platform with a complete clock driver
> >>> will suffer from the same problem (the clock driver disabling unclaimed
> >>> ahb gates, and thus killing the video output) if it wants to use simplefb
> >>> for early console support.
> >>
> >> It is platform specific in that your platform may require certain clocks
> >> to remain on. The next platform may require power domains to remain on
> >> during boot and yet another one may rely on regulators to stay on during
> >> boot. By your argument simplefb will need to be taught to handle pretty
> >> much every type of resource that the kernel has.
> >
> > Why can't simplefb be a driver library that is called from a device
> > specific device driver that only claims the clocks (or regulators)?
> > Then build all of these device specific drivers into the generic ARM
> > kernel. They will be quite small since all they do is claim the clocks
> > (or regulator). Maybe we can even figure out some protocol for
> > removing the unused ones from memory later.
> >
> > Later during the boot process the device specific driver can load its
> > KMS code which has also been implemented as a driver library. Maybe
> > use E_PROBE_DEFER to do this. Match on the device ID, claim the
> > clocks, defer until the full KMS library can be loaded.
>
> There is no need for all this complexity, all that is needed is for the
> simplefb driver to be thought to claim + enable any clocks listed in
> its dt node.
Out of curiosity, how does this work in practice? How does the
bootloader create this entry? Does it scan the DT to see which clocks
the real hardware device references and then simply copies them to the
simplefb node?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Luc Verhaegen @ 2014-08-25 15:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825151255.GA14967@ulmo.nvidia.com>
On Mon, Aug 25, 2014 at 05:12:58PM +0200, Thierry Reding wrote:
>
> Out of curiosity, how does this work in practice? How does the
> bootloader create this entry? Does it scan the DT to see which clocks
> the real hardware device references and then simply copies them to the
> simplefb node?
>
> Thierry
https://www.mail-archive.com/linux-sunxi@googlegroups.com/msg06619.html
Luc Verhaegen.
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Maxime Ripard @ 2014-08-25 15:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140825150501.GE14763@ulmo.nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 3587 bytes --]
On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote:
> On Mon, Aug 25, 2014 at 04:58:54PM +0200, Maxime Ripard wrote:
> > On Mon, Aug 25, 2014 at 04:16:29PM +0200, Thierry Reding wrote:
> > > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
> > > > On 08/25/2014 03:39 PM, Thierry Reding wrote:
> > > > > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
> > > > >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
> > > > >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
> > > > >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
> > > > >>> [...]
> > > > >>>>> If not, perhaps the clock driver should force the clock to be
> > > > >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
> > > > >>>>
> > > > >>>> I'm sorry, but I'm not going to take any code that will do that in our
> > > > >>>> clock driver.
> > > > >>>>
> > > > >>>> I'm not going to have a huge list of ifdef depending on configuration
> > > > >>>> options to know which clock to enable, especially when clk_get should
> > > > >>>> have the consumer device as an argument.
> > > > >>>
> > > > >>> Are you saying is that you want to solve a platform-specific problem by
> > > > >>> pushing code into simple, generic drivers so that your platform code can
> > > > >>> stay "clean"?
> > > > >>
> > > > >> Are you saying that this driver would become "dirty" with such a patch?
> > > > >
> > > > > Yes. Others have said the same and even provided alternative solutions
> > > > > on how to solve what's seemingly a platform-specific problem in a
> > > > > platform-specific way.
> > > >
> > > > This is not platform specific, any platform with a complete clock driver
> > > > will suffer from the same problem (the clock driver disabling unclaimed
> > > > ahb gates, and thus killing the video output) if it wants to use simplefb
> > > > for early console support.
> > >
> > > It is platform specific in that your platform may require certain clocks
> > > to remain on.
> >
> > The platform doesn't. simplefb does. simplefb is the obvious consumer
> > for these clocks, and given the current API and abstraction we have,
> > it should be the one claiming the clocks too.
>
> No. simplefb just wants to write to some memory that hardware has been
> set up to scan out. The platform requires that the clocks be on. Other
> platforms may not even allow turning off the clocks.
Like what? the rpi? Come on. Just because the videocore is some black
box we know nothing about doesn't mean we should use it as an example.
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...
> > > The next platform may require power domains to remain on during boot
> > > and yet another one may rely on regulators to stay on during
> > > boot. By your argument simplefb will need to be taught to handle
> > > pretty much every type of resource that the kernel has.
> >
> > And I wouldn't find anything wrong with that. We're already doing so
> > for any generic driver in the kernel (AHCI, EHCI comes to my mind
> > first, there's probably a lot of others). Why wouldn't we do as such
> > for this one?
>
> Yes, and we've had similar discussions in those subsystems too.
Similar discussion, with different outcomes it seems.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
From: Andreas Färber @ 2014-08-25 15:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53FB3E7F.4000503@redhat.com>
Hi,
Am 25.08.2014 15:47, schrieb Hans de Goede:
> On 08/25/2014 03:39 PM, Thierry Reding wrote:
>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>>>>>> If not, perhaps the clock driver should force the clock to be
>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
[...]
> As for the suggestion to simply never disable the plls / ahb gates by blocking
> them from ever being disabled in the sunxi clock driver, that is not really
> a solution either, as we want to be able to turn these things off to safe
> power on screen blank once control has been turned over to the kms driver.
Without wanting to take sides on the simplefb matter, can't you just use
clk_ignore_unused in bootargs to work around the issue at hand?
That's what I do on the Spring Chromebook.
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox