* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-27 20:20 ` Maxime Ripard
0 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-03-27 20:20 UTC (permalink / raw)
To: Frank Oltmanns
Cc: Roman Beranek, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
Hi,
On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
> On 2023-03-20 at 17:16:36 +0100, Roman Beranek <romanberanek@icloud.com> wrote:
> > In the case of DSI output, the value of SUN4I_TCON0_DCLK_DIV (4) does
> > not represent the actual dotclock divider, PLL_MIPI instead runs at
> > (bpp / lanes )-multiple [1] of the dotclock. [2] Setting 4 as dotclock
> > divder thus leads to reduced frame rate, specifically by 1/3 on 4-lane
> > panels, and by 2/3 on 2-lane panels respectively.
> >
> > As sun4i_dotclock driver stores its calculated divider directly in
> > the register, conditional handling of the DSI output scenario is needed.
> > Instead of reading the divider from SUN4I_TCON0_DCLK_REG, retrieve
> > the value from tcon->dclk_min_div.
> >
> > [1] bits per pixel / number of DSI lanes
> > [2] <https://github.com/BPI-SINOVOIP/BPI-M64-bsp-4.4/blob/66bef0f2f30b367eb93b1cbad21ce85e0361f7ae/linux-sunxi/drivers/video/fbdev/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L322>
> >
> > Signed-off-by: Roman Beranek <romanberanek@icloud.com>
> > —
> > drivers/gpu/drm/sun4i/sun4i_dotclock.c | 6 +++++-
> > drivers/gpu/drm/sun4i/sun4i_tcon.c | 5 +++–
> > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
> > 3 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff –git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> > index 417ade3d2565..26fa99aff590 100644
> > — a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> > @@ -11,6 +11,7 @@
> >
> > #include “sun4i_tcon.h”
> > #include “sun4i_dotclock.h”
> > +#include “sun6i_mipi_dsi.h”
> >
> > struct sun4i_dclk {
> > struct clk_hw hw;
> > @@ -56,6 +57,9 @@ static unsigned long sun4i_dclk_recalc_rate(struct clk_hw *hw,
> > struct sun4i_dclk *dclk = hw_to_dclk(hw);
> > u32 val;
> >
> > + if (dclk->tcon->is_dsi)
> > + return parent_rate / dclk->tcon->dclk_min_div;
> > +
> > regmap_read(dclk->regmap, SUN4I_TCON0_DCLK_REG, &val);
> >
> > val >>= SUN4I_TCON0_DCLK_DIV_SHIFT;
> > @@ -116,7 +120,7 @@ static int sun4i_dclk_set_rate(struct clk_hw *hw, unsigned long rate,
> > unsigned long parent_rate)
> > {
> > struct sun4i_dclk *dclk = hw_to_dclk(hw);
> > - u8 div = parent_rate / rate;
> > + u8 div = dclk->tcon->is_dsi ? SUN6I_DSI_TCON_DIV : parent_rate / rate;
> >
> > return regmap_update_bits(dclk->regmap, SUN4I_TCON0_DCLK_REG,
> > GENMASK(6, 0), div);
> > diff –git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > index 523a6d787921..7f5d3c135058 100644
> > — a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > @@ -367,8 +367,9 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon *tcon,
> > u32 block_space, start_delay;
> > u32 tcon_div;
> >
> > - tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
> > - tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
> > + tcon->is_dsi = true;
> > + tcon->dclk_min_div = bpp / lanes;
> > + tcon->dclk_max_div = bpp / lanes;
>
> Claiming to set the divider to a different value (bpp / lanes) than what we’re actually using in
> the end (SUN6I_DSIO_TCON_DIV) is somehow bugging me. I feel like the proposal that I submitted is
> more direct: <https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/>
Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it
works with his testing, I'll be happy to merge it.
> Actually, I had the following third patch prepared that adjusted the dotclock rate so that the
> required PLL rate is set. But again, this seems very indirect, so that’s why I refrained from
> submitting it and I submitted the linked patch above instead.
>
> Anyway, here is the third proposal:
>
> — a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
> regulator_disable(dsi->regulator);
> }
>
> +static bool sun6i_dsi_encoder_mode_fixup(
> ⁃ struct drm_encoder *encoder,
> ⁃ const struct drm_display_mode *mode,
> ⁃ struct drm_display_mode *adjusted_mode)
> +{
> ⁃ if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
> ⁃ /*
> ⁃ * For DSI the PLL rate has to respect the bits per pixel and
> ⁃ * number of lanes.
> ⁃ *
> ⁃ * According to the BSP code:
> ⁃ * PLL rate = DOTCLOCK * bpp / lanes
> ⁃ *
> ⁃ * Therefore, the clock has to be adjusted in order to set the
> ⁃ * correct PLL rate when actually setting the clock.
> ⁃ */
> ⁃ struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
> ⁃ struct mipi_dsi_device *device = dsi->device;
> ⁃ u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> ⁃ u8 lanes = device->lanes;
> ⁃
>
> ⁃ adjusted_mode->crtc_clock = mode->crtc_clock
> ⁃ * bpp / (lanes * SUN6I_DSI_TCON_DIV);
> ⁃ }
> ⁃
>
> ⁃ return true;
> +}
> ⁃ static int sun6i_dsi_get_modes(struct drm_connector *connector)
> {
> struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
> @@ -851,6 +879,7 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
> static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
> .disable = sun6i_dsi_encoder_disable,
> .enable = sun6i_dsi_encoder_enable,
> ⁃ .mode_fixup = sun6i_dsi_encoder_mode_fixup,
> };
It's not clear to me what this patch is supposed to be doing, there's no mode_fixup implementation
upstream?
Maxime
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-27 20:20 ` Maxime Ripard
0 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-03-27 20:20 UTC (permalink / raw)
To: Frank Oltmanns
Cc: Samuel Holland, linux-kernel, Jernej Skrabec, Roman Beranek,
Chen-Yu Tsai, dri-devel, linux-sunxi, linux-arm-kernel
Hi,
On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
> On 2023-03-20 at 17:16:36 +0100, Roman Beranek <romanberanek@icloud.com> wrote:
> > In the case of DSI output, the value of SUN4I_TCON0_DCLK_DIV (4) does
> > not represent the actual dotclock divider, PLL_MIPI instead runs at
> > (bpp / lanes )-multiple [1] of the dotclock. [2] Setting 4 as dotclock
> > divder thus leads to reduced frame rate, specifically by 1/3 on 4-lane
> > panels, and by 2/3 on 2-lane panels respectively.
> >
> > As sun4i_dotclock driver stores its calculated divider directly in
> > the register, conditional handling of the DSI output scenario is needed.
> > Instead of reading the divider from SUN4I_TCON0_DCLK_REG, retrieve
> > the value from tcon->dclk_min_div.
> >
> > [1] bits per pixel / number of DSI lanes
> > [2] <https://github.com/BPI-SINOVOIP/BPI-M64-bsp-4.4/blob/66bef0f2f30b367eb93b1cbad21ce85e0361f7ae/linux-sunxi/drivers/video/fbdev/sunxi/disp2/disp/de/lowlevel_sun50iw1/disp_al.c#L322>
> >
> > Signed-off-by: Roman Beranek <romanberanek@icloud.com>
> > —
> > drivers/gpu/drm/sun4i/sun4i_dotclock.c | 6 +++++-
> > drivers/gpu/drm/sun4i/sun4i_tcon.c | 5 +++–
> > drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 +
> > 3 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff –git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> > index 417ade3d2565..26fa99aff590 100644
> > — a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> > @@ -11,6 +11,7 @@
> >
> > #include “sun4i_tcon.h”
> > #include “sun4i_dotclock.h”
> > +#include “sun6i_mipi_dsi.h”
> >
> > struct sun4i_dclk {
> > struct clk_hw hw;
> > @@ -56,6 +57,9 @@ static unsigned long sun4i_dclk_recalc_rate(struct clk_hw *hw,
> > struct sun4i_dclk *dclk = hw_to_dclk(hw);
> > u32 val;
> >
> > + if (dclk->tcon->is_dsi)
> > + return parent_rate / dclk->tcon->dclk_min_div;
> > +
> > regmap_read(dclk->regmap, SUN4I_TCON0_DCLK_REG, &val);
> >
> > val >>= SUN4I_TCON0_DCLK_DIV_SHIFT;
> > @@ -116,7 +120,7 @@ static int sun4i_dclk_set_rate(struct clk_hw *hw, unsigned long rate,
> > unsigned long parent_rate)
> > {
> > struct sun4i_dclk *dclk = hw_to_dclk(hw);
> > - u8 div = parent_rate / rate;
> > + u8 div = dclk->tcon->is_dsi ? SUN6I_DSI_TCON_DIV : parent_rate / rate;
> >
> > return regmap_update_bits(dclk->regmap, SUN4I_TCON0_DCLK_REG,
> > GENMASK(6, 0), div);
> > diff –git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > index 523a6d787921..7f5d3c135058 100644
> > — a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > @@ -367,8 +367,9 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon *tcon,
> > u32 block_space, start_delay;
> > u32 tcon_div;
> >
> > - tcon->dclk_min_div = SUN6I_DSI_TCON_DIV;
> > - tcon->dclk_max_div = SUN6I_DSI_TCON_DIV;
> > + tcon->is_dsi = true;
> > + tcon->dclk_min_div = bpp / lanes;
> > + tcon->dclk_max_div = bpp / lanes;
>
> Claiming to set the divider to a different value (bpp / lanes) than what we’re actually using in
> the end (SUN6I_DSIO_TCON_DIV) is somehow bugging me. I feel like the proposal that I submitted is
> more direct: <https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/>
Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it
works with his testing, I'll be happy to merge it.
> Actually, I had the following third patch prepared that adjusted the dotclock rate so that the
> required PLL rate is set. But again, this seems very indirect, so that’s why I refrained from
> submitting it and I submitted the linked patch above instead.
>
> Anyway, here is the third proposal:
>
> — a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
> regulator_disable(dsi->regulator);
> }
>
> +static bool sun6i_dsi_encoder_mode_fixup(
> ⁃ struct drm_encoder *encoder,
> ⁃ const struct drm_display_mode *mode,
> ⁃ struct drm_display_mode *adjusted_mode)
> +{
> ⁃ if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
> ⁃ /*
> ⁃ * For DSI the PLL rate has to respect the bits per pixel and
> ⁃ * number of lanes.
> ⁃ *
> ⁃ * According to the BSP code:
> ⁃ * PLL rate = DOTCLOCK * bpp / lanes
> ⁃ *
> ⁃ * Therefore, the clock has to be adjusted in order to set the
> ⁃ * correct PLL rate when actually setting the clock.
> ⁃ */
> ⁃ struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
> ⁃ struct mipi_dsi_device *device = dsi->device;
> ⁃ u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> ⁃ u8 lanes = device->lanes;
> ⁃
>
> ⁃ adjusted_mode->crtc_clock = mode->crtc_clock
> ⁃ * bpp / (lanes * SUN6I_DSI_TCON_DIV);
> ⁃ }
> ⁃
>
> ⁃ return true;
> +}
> ⁃ static int sun6i_dsi_get_modes(struct drm_connector *connector)
> {
> struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
> @@ -851,6 +879,7 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
> static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
> .disable = sun6i_dsi_encoder_disable,
> .enable = sun6i_dsi_encoder_enable,
> ⁃ .mode_fixup = sun6i_dsi_encoder_mode_fixup,
> };
It's not clear to me what this patch is supposed to be doing, there's no mode_fixup implementation
upstream?
Maxime
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
2023-03-27 20:20 ` Maxime Ripard
(?)
@ 2023-03-27 23:48 ` Roman Beranek
-1 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-03-27 23:48 UTC (permalink / raw)
To: Maxime Ripard, Frank Oltmanns
Cc: Chen-Yu Tsai, David Airlie, Daniel Vetter, Jernej Skrabec,
Samuel Holland, dri-devel, linux-arm-kernel, linux-sunxi,
linux-kernel
On Mon Mar 27, 2023 at 10:20 PM CEST, Maxime Ripard wrote:
>
> On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
> > Claiming to set the divider to a different value (bpp / lanes) than what we’re actually using in
> > the end (SUN6I_DSIO_TCON_DIV) is somehow bugging me. I feel like the proposal that I submitted is
> > more direct: <https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/>
>
> Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it
> works with his testing, I'll be happy to merge it.
>
So I've just found out that my understanding of what sun4i_dotclock is
was wrong the whole time. I treated it as a virtual clock representing
the true CRTC pixel clock and only coincidentally also matching what
A64 Reference Manual labels as TCON0 data clock (a coincidence to which
DSI is an exception).
Now that I finally see dotclock as 'what could dclk be an abbreviation
to', I to agree that it's not only straightforward but also correct to
keep the divider at 4 and adjust the rate as is done it the patch Frank
submitted.
In order to preserve semantic correctness however, I propose to preface
the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
such that dot/pixel is replaced with d/data. What do you think?
Roman
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-27 23:48 ` Roman Beranek
0 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-03-27 23:48 UTC (permalink / raw)
To: Maxime Ripard, Frank Oltmanns
Cc: Samuel Holland, linux-kernel, dri-devel, Chen-Yu Tsai,
Jernej Skrabec, linux-sunxi, linux-arm-kernel
On Mon Mar 27, 2023 at 10:20 PM CEST, Maxime Ripard wrote:
>
> On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
> > Claiming to set the divider to a different value (bpp / lanes) than what we’re actually using in
> > the end (SUN6I_DSIO_TCON_DIV) is somehow bugging me. I feel like the proposal that I submitted is
> > more direct: <https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/>
>
> Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it
> works with his testing, I'll be happy to merge it.
>
So I've just found out that my understanding of what sun4i_dotclock is
was wrong the whole time. I treated it as a virtual clock representing
the true CRTC pixel clock and only coincidentally also matching what
A64 Reference Manual labels as TCON0 data clock (a coincidence to which
DSI is an exception).
Now that I finally see dotclock as 'what could dclk be an abbreviation
to', I to agree that it's not only straightforward but also correct to
keep the divider at 4 and adjust the rate as is done it the patch Frank
submitted.
In order to preserve semantic correctness however, I propose to preface
the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
such that dot/pixel is replaced with d/data. What do you think?
Roman
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-27 23:48 ` Roman Beranek
0 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-03-27 23:48 UTC (permalink / raw)
To: Maxime Ripard, Frank Oltmanns
Cc: Chen-Yu Tsai, David Airlie, Daniel Vetter, Jernej Skrabec,
Samuel Holland, dri-devel, linux-arm-kernel, linux-sunxi,
linux-kernel
On Mon Mar 27, 2023 at 10:20 PM CEST, Maxime Ripard wrote:
>
> On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
> > Claiming to set the divider to a different value (bpp / lanes) than what we’re actually using in
> > the end (SUN6I_DSIO_TCON_DIV) is somehow bugging me. I feel like the proposal that I submitted is
> > more direct: <https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/>
>
> Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it
> works with his testing, I'll be happy to merge it.
>
So I've just found out that my understanding of what sun4i_dotclock is
was wrong the whole time. I treated it as a virtual clock representing
the true CRTC pixel clock and only coincidentally also matching what
A64 Reference Manual labels as TCON0 data clock (a coincidence to which
DSI is an exception).
Now that I finally see dotclock as 'what could dclk be an abbreviation
to', I to agree that it's not only straightforward but also correct to
keep the divider at 4 and adjust the rate as is done it the patch Frank
submitted.
In order to preserve semantic correctness however, I propose to preface
the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
such that dot/pixel is replaced with d/data. What do you think?
Roman
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
2023-03-27 23:48 ` Roman Beranek
(?)
@ 2023-03-29 19:58 ` Maxime Ripard
-1 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-03-29 19:58 UTC (permalink / raw)
To: Roman Beranek
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1554 bytes --]
On Tue, Mar 28, 2023 at 01:48:33AM +0200, Roman Beranek wrote:
> On Mon Mar 27, 2023 at 10:20 PM CEST, Maxime Ripard wrote:
> >
> > On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
> > > Claiming to set the divider to a different value (bpp / lanes) than what we’re actually using in
> > > the end (SUN6I_DSIO_TCON_DIV) is somehow bugging me. I feel like the proposal that I submitted is
> > > more direct: <https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/>
> >
> > Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it
> > works with his testing, I'll be happy to merge it.
> >
>
> So I've just found out that my understanding of what sun4i_dotclock is
> was wrong the whole time. I treated it as a virtual clock representing
> the true CRTC pixel clock and only coincidentally also matching what
> A64 Reference Manual labels as TCON0 data clock (a coincidence to which
> DSI is an exception).
>
> Now that I finally see dotclock as 'what could dclk be an abbreviation
> to', I to agree that it's not only straightforward but also correct to
> keep the divider at 4 and adjust the rate as is done it the patch Frank
> submitted.
>
> In order to preserve semantic correctness however, I propose to preface
> the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
> such that dot/pixel is replaced with d/data. What do you think?
I don't think it's exposed to the userspace in any way so it makes sense to me
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-29 19:58 ` Maxime Ripard
0 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-03-29 19:58 UTC (permalink / raw)
To: Roman Beranek
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1554 bytes --]
On Tue, Mar 28, 2023 at 01:48:33AM +0200, Roman Beranek wrote:
> On Mon Mar 27, 2023 at 10:20 PM CEST, Maxime Ripard wrote:
> >
> > On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
> > > Claiming to set the divider to a different value (bpp / lanes) than what we’re actually using in
> > > the end (SUN6I_DSIO_TCON_DIV) is somehow bugging me. I feel like the proposal that I submitted is
> > > more direct: <https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/>
> >
> > Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it
> > works with his testing, I'll be happy to merge it.
> >
>
> So I've just found out that my understanding of what sun4i_dotclock is
> was wrong the whole time. I treated it as a virtual clock representing
> the true CRTC pixel clock and only coincidentally also matching what
> A64 Reference Manual labels as TCON0 data clock (a coincidence to which
> DSI is an exception).
>
> Now that I finally see dotclock as 'what could dclk be an abbreviation
> to', I to agree that it's not only straightforward but also correct to
> keep the divider at 4 and adjust the rate as is done it the patch Frank
> submitted.
>
> In order to preserve semantic correctness however, I propose to preface
> the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
> such that dot/pixel is replaced with d/data. What do you think?
I don't think it's exposed to the userspace in any way so it makes sense to me
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-29 19:58 ` Maxime Ripard
0 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-03-29 19:58 UTC (permalink / raw)
To: Roman Beranek
Cc: Samuel Holland, Frank Oltmanns, Jernej Skrabec, linux-kernel,
Chen-Yu Tsai, dri-devel, linux-sunxi, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1554 bytes --]
On Tue, Mar 28, 2023 at 01:48:33AM +0200, Roman Beranek wrote:
> On Mon Mar 27, 2023 at 10:20 PM CEST, Maxime Ripard wrote:
> >
> > On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
> > > Claiming to set the divider to a different value (bpp / lanes) than what we’re actually using in
> > > the end (SUN6I_DSIO_TCON_DIV) is somehow bugging me. I feel like the proposal that I submitted is
> > > more direct: <https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/>
> >
> > Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it
> > works with his testing, I'll be happy to merge it.
> >
>
> So I've just found out that my understanding of what sun4i_dotclock is
> was wrong the whole time. I treated it as a virtual clock representing
> the true CRTC pixel clock and only coincidentally also matching what
> A64 Reference Manual labels as TCON0 data clock (a coincidence to which
> DSI is an exception).
>
> Now that I finally see dotclock as 'what could dclk be an abbreviation
> to', I to agree that it's not only straightforward but also correct to
> keep the divider at 4 and adjust the rate as is done it the patch Frank
> submitted.
>
> In order to preserve semantic correctness however, I propose to preface
> the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
> such that dot/pixel is replaced with d/data. What do you think?
I don't think it's exposed to the userspace in any way so it makes sense to me
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
2023-03-29 19:58 ` Maxime Ripard
(?)
@ 2023-03-30 4:45 ` Frank Oltmanns
-1 siblings, 0 replies; 60+ messages in thread
From: Frank Oltmanns @ 2023-03-30 4:45 UTC (permalink / raw)
To: Roman Beranek
Cc: Maxime Ripard, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
Hi Roman,
On 2023-03-29 at 21:58:02 +0200, Maxime Ripard <maxime@cerno.tech> wrote:
> On Tue, Mar 28, 2023 at 01:48:33AM +0200, Roman Beranek wrote:
>> On Mon Mar 27, 2023 at 10:20 PM CEST, Maxime Ripard wrote:
>> >
>> > On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
>> > > Claiming to set the divider to a different value (bpp / lanes) than what we’re actually using in
>> > > the end (SUN6I_DSIO_TCON_DIV) is somehow bugging me. I feel like the proposal that I submitted is
>> > > more direct: <https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/>
>> >
>> > Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it
>> > works with his testing, I'll be happy to merge it.
>> >
>>
>> So I've just found out that my understanding of what sun4i_dotclock is
>> was wrong the whole time. I treated it as a virtual clock representing
>> the true CRTC pixel clock and only coincidentally also matching what
>> A64 Reference Manual labels as TCON0 data clock (a coincidence to which
>> DSI is an exception).
>>
>> Now that I finally see dotclock as 'what could dclk be an abbreviation
>> to', I to agree that it's not only straightforward but also correct to
>> keep the divider at 4 and adjust the rate as is done it the patch Frank
>> submitted.
>>
>> In order to preserve semantic correctness however, I propose to preface
>> the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
>> such that dot/pixel is replaced with d/data. What do you think?
>
> I don't think it's exposed to the userspace in any way so it makes sense to me
Roman, will you please submit a V2 of the patch I submitted then? Or do
you want me to do it?
Thanks,
Frank
>
> Maxime
>
--
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-30 4:45 ` Frank Oltmanns
0 siblings, 0 replies; 60+ messages in thread
From: Frank Oltmanns @ 2023-03-30 4:45 UTC (permalink / raw)
To: Roman Beranek
Cc: Maxime Ripard, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
Hi Roman,
On 2023-03-29 at 21:58:02 +0200, Maxime Ripard <maxime@cerno.tech> wrote:
> On Tue, Mar 28, 2023 at 01:48:33AM +0200, Roman Beranek wrote:
>> On Mon Mar 27, 2023 at 10:20 PM CEST, Maxime Ripard wrote:
>> >
>> > On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
>> > > Claiming to set the divider to a different value (bpp / lanes) than what we’re actually using in
>> > > the end (SUN6I_DSIO_TCON_DIV) is somehow bugging me. I feel like the proposal that I submitted is
>> > > more direct: <https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/>
>> >
>> > Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it
>> > works with his testing, I'll be happy to merge it.
>> >
>>
>> So I've just found out that my understanding of what sun4i_dotclock is
>> was wrong the whole time. I treated it as a virtual clock representing
>> the true CRTC pixel clock and only coincidentally also matching what
>> A64 Reference Manual labels as TCON0 data clock (a coincidence to which
>> DSI is an exception).
>>
>> Now that I finally see dotclock as 'what could dclk be an abbreviation
>> to', I to agree that it's not only straightforward but also correct to
>> keep the divider at 4 and adjust the rate as is done it the patch Frank
>> submitted.
>>
>> In order to preserve semantic correctness however, I propose to preface
>> the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
>> such that dot/pixel is replaced with d/data. What do you think?
>
> I don't think it's exposed to the userspace in any way so it makes sense to me
Roman, will you please submit a V2 of the patch I submitted then? Or do
you want me to do it?
Thanks,
Frank
>
> Maxime
>
--
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-30 4:45 ` Frank Oltmanns
0 siblings, 0 replies; 60+ messages in thread
From: Frank Oltmanns @ 2023-03-30 4:45 UTC (permalink / raw)
To: Roman Beranek
Cc: Samuel Holland, dri-devel, linux-kernel, Jernej Skrabec,
Chen-Yu Tsai, Maxime Ripard, linux-sunxi, linux-arm-kernel
Hi Roman,
On 2023-03-29 at 21:58:02 +0200, Maxime Ripard <maxime@cerno.tech> wrote:
> On Tue, Mar 28, 2023 at 01:48:33AM +0200, Roman Beranek wrote:
>> On Mon Mar 27, 2023 at 10:20 PM CEST, Maxime Ripard wrote:
>> >
>> > On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
>> > > Claiming to set the divider to a different value (bpp / lanes) than what we’re actually using in
>> > > the end (SUN6I_DSIO_TCON_DIV) is somehow bugging me. I feel like the proposal that I submitted is
>> > > more direct: <https://lore.kernel.org/all/20230319160704.9858-2-frank@oltmanns.dev/>
>> >
>> > Yeah, this patch looks better to me too: it's simpler, more straightforward. If Roman can confirm it
>> > works with his testing, I'll be happy to merge it.
>> >
>>
>> So I've just found out that my understanding of what sun4i_dotclock is
>> was wrong the whole time. I treated it as a virtual clock representing
>> the true CRTC pixel clock and only coincidentally also matching what
>> A64 Reference Manual labels as TCON0 data clock (a coincidence to which
>> DSI is an exception).
>>
>> Now that I finally see dotclock as 'what could dclk be an abbreviation
>> to', I to agree that it's not only straightforward but also correct to
>> keep the divider at 4 and adjust the rate as is done it the patch Frank
>> submitted.
>>
>> In order to preserve semantic correctness however, I propose to preface
>> the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
>> such that dot/pixel is replaced with d/data. What do you think?
>
> I don't think it's exposed to the userspace in any way so it makes sense to me
Roman, will you please submit a V2 of the patch I submitted then? Or do
you want me to do it?
Thanks,
Frank
>
> Maxime
>
--
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
2023-03-30 4:45 ` Frank Oltmanns
(?)
@ 2023-03-31 5:36 ` Roman Beranek
-1 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-03-31 5:36 UTC (permalink / raw)
To: Frank Oltmanns
Cc: Maxime Ripard, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
Hello Frank,
On Thu Mar 30, 2023 at 6:45 AM CEST, Frank Oltmanns wrote:
> Roman, will you please submit a V2 of the patch I submitted then? Or do
> you want me to do it?
Yes, I'm already on it, only missing a cover letter.
Roman
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-31 5:36 ` Roman Beranek
0 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-03-31 5:36 UTC (permalink / raw)
To: Frank Oltmanns
Cc: Maxime Ripard, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
Hello Frank,
On Thu Mar 30, 2023 at 6:45 AM CEST, Frank Oltmanns wrote:
> Roman, will you please submit a V2 of the patch I submitted then? Or do
> you want me to do it?
Yes, I'm already on it, only missing a cover letter.
Roman
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-31 5:36 ` Roman Beranek
0 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-03-31 5:36 UTC (permalink / raw)
To: Frank Oltmanns
Cc: Samuel Holland, dri-devel, linux-kernel, Jernej Skrabec,
Chen-Yu Tsai, Maxime Ripard, linux-sunxi, linux-arm-kernel
Hello Frank,
On Thu Mar 30, 2023 at 6:45 AM CEST, Frank Oltmanns wrote:
> Roman, will you please submit a V2 of the patch I submitted then? Or do
> you want me to do it?
Yes, I'm already on it, only missing a cover letter.
Roman
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
2023-03-29 19:58 ` Maxime Ripard
(?)
@ 2023-04-05 12:34 ` Roman Beranek
-1 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-04-05 12:34 UTC (permalink / raw)
To: Maxime Ripard
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
Hello Maxime,
On Wed Mar 29, 2023 at 9:58 PM CEST, Maxime Ripard wrote:
> > In order to preserve semantic correctness however, I propose to preface
> > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
> > such that dot/pixel is replaced with d/data. What do you think?
>
> I don't think it's exposed to the userspace in any way so it makes sense to me
>
Here's a new series that includes those renames:
<https://lore.kernel.org/all/20230331110245.43527-1-me@crly.cz/>
It turns out however that the new dclk rates can't be set exactly as
requested without touching pll-video0*, tcon0 now therefore gets
reparented from pll-mipi to pll-video0-2x which, as it further turns
out, breaks DSI. While simply forbidding the video0-2x mux option seems
to me as the right way to go because there's not much use for it with
non-DSI interfaces either besides the opportunity to power pll-mipi
down, I'd like to run by you first.
Kind regards,
Roman
* As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
294 MHz though because it limits itself to 6 MHz steps.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-04-05 12:34 ` Roman Beranek
0 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-04-05 12:34 UTC (permalink / raw)
To: Maxime Ripard
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
Hello Maxime,
On Wed Mar 29, 2023 at 9:58 PM CEST, Maxime Ripard wrote:
> > In order to preserve semantic correctness however, I propose to preface
> > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
> > such that dot/pixel is replaced with d/data. What do you think?
>
> I don't think it's exposed to the userspace in any way so it makes sense to me
>
Here's a new series that includes those renames:
<https://lore.kernel.org/all/20230331110245.43527-1-me@crly.cz/>
It turns out however that the new dclk rates can't be set exactly as
requested without touching pll-video0*, tcon0 now therefore gets
reparented from pll-mipi to pll-video0-2x which, as it further turns
out, breaks DSI. While simply forbidding the video0-2x mux option seems
to me as the right way to go because there's not much use for it with
non-DSI interfaces either besides the opportunity to power pll-mipi
down, I'd like to run by you first.
Kind regards,
Roman
* As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
294 MHz though because it limits itself to 6 MHz steps.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-04-05 12:34 ` Roman Beranek
0 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-04-05 12:34 UTC (permalink / raw)
To: Maxime Ripard
Cc: Samuel Holland, Frank Oltmanns, Jernej Skrabec, linux-kernel,
Chen-Yu Tsai, dri-devel, linux-sunxi, linux-arm-kernel
Hello Maxime,
On Wed Mar 29, 2023 at 9:58 PM CEST, Maxime Ripard wrote:
> > In order to preserve semantic correctness however, I propose to preface
> > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
> > such that dot/pixel is replaced with d/data. What do you think?
>
> I don't think it's exposed to the userspace in any way so it makes sense to me
>
Here's a new series that includes those renames:
<https://lore.kernel.org/all/20230331110245.43527-1-me@crly.cz/>
It turns out however that the new dclk rates can't be set exactly as
requested without touching pll-video0*, tcon0 now therefore gets
reparented from pll-mipi to pll-video0-2x which, as it further turns
out, breaks DSI. While simply forbidding the video0-2x mux option seems
to me as the right way to go because there's not much use for it with
non-DSI interfaces either besides the opportunity to power pll-mipi
down, I'd like to run by you first.
Kind regards,
Roman
* As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
294 MHz though because it limits itself to 6 MHz steps.
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
2023-04-05 12:34 ` Roman Beranek
(?)
@ 2023-04-05 15:03 ` Maxime Ripard
-1 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-04-05 15:03 UTC (permalink / raw)
To: Roman Beranek
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1485 bytes --]
On Wed, Apr 05, 2023 at 02:34:11PM +0200, Roman Beranek wrote:
> Hello Maxime,
>
> On Wed Mar 29, 2023 at 9:58 PM CEST, Maxime Ripard wrote:
> > > In order to preserve semantic correctness however, I propose to preface
> > > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
> > > such that dot/pixel is replaced with d/data. What do you think?
> >
> > I don't think it's exposed to the userspace in any way so it makes sense to me
> >
>
> Here's a new series that includes those renames:
> <https://lore.kernel.org/all/20230331110245.43527-1-me@crly.cz/>
>
> It turns out however that the new dclk rates can't be set exactly as
> requested without touching pll-video0*, tcon0 now therefore gets
> reparented from pll-mipi to pll-video0-2x which, as it further turns
> out, breaks DSI. While simply forbidding the video0-2x mux option seems
> to me as the right way to go because there's not much use for it with
> non-DSI interfaces either besides the opportunity to power pll-mipi
> down, I'd like to run by you first.
Sounds reasonable
> * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
> retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
> in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
> The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
> 294 MHz though because it limits itself to 6 MHz steps.
We could also address that though
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-04-05 15:03 ` Maxime Ripard
0 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-04-05 15:03 UTC (permalink / raw)
To: Roman Beranek
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1485 bytes --]
On Wed, Apr 05, 2023 at 02:34:11PM +0200, Roman Beranek wrote:
> Hello Maxime,
>
> On Wed Mar 29, 2023 at 9:58 PM CEST, Maxime Ripard wrote:
> > > In order to preserve semantic correctness however, I propose to preface
> > > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
> > > such that dot/pixel is replaced with d/data. What do you think?
> >
> > I don't think it's exposed to the userspace in any way so it makes sense to me
> >
>
> Here's a new series that includes those renames:
> <https://lore.kernel.org/all/20230331110245.43527-1-me@crly.cz/>
>
> It turns out however that the new dclk rates can't be set exactly as
> requested without touching pll-video0*, tcon0 now therefore gets
> reparented from pll-mipi to pll-video0-2x which, as it further turns
> out, breaks DSI. While simply forbidding the video0-2x mux option seems
> to me as the right way to go because there's not much use for it with
> non-DSI interfaces either besides the opportunity to power pll-mipi
> down, I'd like to run by you first.
Sounds reasonable
> * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
> retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
> in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
> The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
> 294 MHz though because it limits itself to 6 MHz steps.
We could also address that though
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-04-05 15:03 ` Maxime Ripard
0 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-04-05 15:03 UTC (permalink / raw)
To: Roman Beranek
Cc: Samuel Holland, Frank Oltmanns, Jernej Skrabec, linux-kernel,
Chen-Yu Tsai, dri-devel, linux-sunxi, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1485 bytes --]
On Wed, Apr 05, 2023 at 02:34:11PM +0200, Roman Beranek wrote:
> Hello Maxime,
>
> On Wed Mar 29, 2023 at 9:58 PM CEST, Maxime Ripard wrote:
> > > In order to preserve semantic correctness however, I propose to preface
> > > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
> > > such that dot/pixel is replaced with d/data. What do you think?
> >
> > I don't think it's exposed to the userspace in any way so it makes sense to me
> >
>
> Here's a new series that includes those renames:
> <https://lore.kernel.org/all/20230331110245.43527-1-me@crly.cz/>
>
> It turns out however that the new dclk rates can't be set exactly as
> requested without touching pll-video0*, tcon0 now therefore gets
> reparented from pll-mipi to pll-video0-2x which, as it further turns
> out, breaks DSI. While simply forbidding the video0-2x mux option seems
> to me as the right way to go because there's not much use for it with
> non-DSI interfaces either besides the opportunity to power pll-mipi
> down, I'd like to run by you first.
Sounds reasonable
> * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
> retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
> in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
> The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
> 294 MHz though because it limits itself to 6 MHz steps.
We could also address that though
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
2023-04-05 15:03 ` Maxime Ripard
(?)
@ 2023-04-12 7:14 ` Roman Beranek
-1 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-04-12 7:14 UTC (permalink / raw)
To: Maxime Ripard
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
On Wed Apr 5, 2023 at 5:03 PM CEST, Maxime Ripard wrote:
> On Wed, Apr 05, 2023 at 02:34:11PM +0200, Roman Beranek wrote:
> > It turns out however that the new dclk rates can't be set exactly as
> > requested without touching pll-video0*, tcon0 now therefore gets
> > reparented from pll-mipi to pll-video0-2x which, as it further turns
> > out, breaks DSI. While simply forbidding the video0-2x mux option seems
> > to me as the right way to go because there's not much use for it with
> > non-DSI interfaces either besides the opportunity to power pll-mipi
> > down, I'd like to run by you first.
>
> Sounds reasonable
Okay, I'm unsure of how to denote that in the code however. Should I
just comment the parent out of the table and put an explanation in
a comment nearby? Or just erase it? I couldn't find an applicable
precedent.
> > * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
> > retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
> > in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
> > The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
> > 294 MHz though because it limits itself to 6 MHz steps.
>
> We could also address that though
Should I include it in v2 of the series, or leave it for later?
Thanks,
Roman
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-04-12 7:14 ` Roman Beranek
0 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-04-12 7:14 UTC (permalink / raw)
To: Maxime Ripard
Cc: Samuel Holland, Frank Oltmanns, Jernej Skrabec, linux-kernel,
Chen-Yu Tsai, dri-devel, linux-sunxi, linux-arm-kernel
On Wed Apr 5, 2023 at 5:03 PM CEST, Maxime Ripard wrote:
> On Wed, Apr 05, 2023 at 02:34:11PM +0200, Roman Beranek wrote:
> > It turns out however that the new dclk rates can't be set exactly as
> > requested without touching pll-video0*, tcon0 now therefore gets
> > reparented from pll-mipi to pll-video0-2x which, as it further turns
> > out, breaks DSI. While simply forbidding the video0-2x mux option seems
> > to me as the right way to go because there's not much use for it with
> > non-DSI interfaces either besides the opportunity to power pll-mipi
> > down, I'd like to run by you first.
>
> Sounds reasonable
Okay, I'm unsure of how to denote that in the code however. Should I
just comment the parent out of the table and put an explanation in
a comment nearby? Or just erase it? I couldn't find an applicable
precedent.
> > * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
> > retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
> > in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
> > The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
> > 294 MHz though because it limits itself to 6 MHz steps.
>
> We could also address that though
Should I include it in v2 of the series, or leave it for later?
Thanks,
Roman
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-04-12 7:14 ` Roman Beranek
0 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-04-12 7:14 UTC (permalink / raw)
To: Maxime Ripard
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
On Wed Apr 5, 2023 at 5:03 PM CEST, Maxime Ripard wrote:
> On Wed, Apr 05, 2023 at 02:34:11PM +0200, Roman Beranek wrote:
> > It turns out however that the new dclk rates can't be set exactly as
> > requested without touching pll-video0*, tcon0 now therefore gets
> > reparented from pll-mipi to pll-video0-2x which, as it further turns
> > out, breaks DSI. While simply forbidding the video0-2x mux option seems
> > to me as the right way to go because there's not much use for it with
> > non-DSI interfaces either besides the opportunity to power pll-mipi
> > down, I'd like to run by you first.
>
> Sounds reasonable
Okay, I'm unsure of how to denote that in the code however. Should I
just comment the parent out of the table and put an explanation in
a comment nearby? Or just erase it? I couldn't find an applicable
precedent.
> > * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
> > retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
> > in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
> > The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
> > 294 MHz though because it limits itself to 6 MHz steps.
>
> We could also address that though
Should I include it in v2 of the series, or leave it for later?
Thanks,
Roman
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
2023-04-12 7:14 ` Roman Beranek
(?)
@ 2023-04-12 14:09 ` Maxime Ripard
-1 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-04-12 14:09 UTC (permalink / raw)
To: Roman Beranek
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1596 bytes --]
On Wed, Apr 12, 2023 at 09:14:59AM +0200, Roman Beranek wrote:
> On Wed Apr 5, 2023 at 5:03 PM CEST, Maxime Ripard wrote:
> > On Wed, Apr 05, 2023 at 02:34:11PM +0200, Roman Beranek wrote:
> > > It turns out however that the new dclk rates can't be set exactly as
> > > requested without touching pll-video0*, tcon0 now therefore gets
> > > reparented from pll-mipi to pll-video0-2x which, as it further turns
> > > out, breaks DSI. While simply forbidding the video0-2x mux option seems
> > > to me as the right way to go because there's not much use for it with
> > > non-DSI interfaces either besides the opportunity to power pll-mipi
> > > down, I'd like to run by you first.
> >
> > Sounds reasonable
>
> Okay, I'm unsure of how to denote that in the code however. Should I
> just comment the parent out of the table and put an explanation in
> a comment nearby? Or just erase it? I couldn't find an applicable
> precedent.
I think that forcing the parent at boot, and adding the
CLK_SET_RATE_NOREPARENT flag should be enough.
> > > * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
> > > retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
> > > in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
> > > The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
> > > 294 MHz though because it limits itself to 6 MHz steps.
> >
> > We could also address that though
>
> Should I include it in v2 of the series, or leave it for later?
I guess you can include it into this one too
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-04-12 14:09 ` Maxime Ripard
0 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-04-12 14:09 UTC (permalink / raw)
To: Roman Beranek
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1596 bytes --]
On Wed, Apr 12, 2023 at 09:14:59AM +0200, Roman Beranek wrote:
> On Wed Apr 5, 2023 at 5:03 PM CEST, Maxime Ripard wrote:
> > On Wed, Apr 05, 2023 at 02:34:11PM +0200, Roman Beranek wrote:
> > > It turns out however that the new dclk rates can't be set exactly as
> > > requested without touching pll-video0*, tcon0 now therefore gets
> > > reparented from pll-mipi to pll-video0-2x which, as it further turns
> > > out, breaks DSI. While simply forbidding the video0-2x mux option seems
> > > to me as the right way to go because there's not much use for it with
> > > non-DSI interfaces either besides the opportunity to power pll-mipi
> > > down, I'd like to run by you first.
> >
> > Sounds reasonable
>
> Okay, I'm unsure of how to denote that in the code however. Should I
> just comment the parent out of the table and put an explanation in
> a comment nearby? Or just erase it? I couldn't find an applicable
> precedent.
I think that forcing the parent at boot, and adding the
CLK_SET_RATE_NOREPARENT flag should be enough.
> > > * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
> > > retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
> > > in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
> > > The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
> > > 294 MHz though because it limits itself to 6 MHz steps.
> >
> > We could also address that though
>
> Should I include it in v2 of the series, or leave it for later?
I guess you can include it into this one too
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-04-12 14:09 ` Maxime Ripard
0 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-04-12 14:09 UTC (permalink / raw)
To: Roman Beranek
Cc: Samuel Holland, Frank Oltmanns, Jernej Skrabec, linux-kernel,
Chen-Yu Tsai, dri-devel, linux-sunxi, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1596 bytes --]
On Wed, Apr 12, 2023 at 09:14:59AM +0200, Roman Beranek wrote:
> On Wed Apr 5, 2023 at 5:03 PM CEST, Maxime Ripard wrote:
> > On Wed, Apr 05, 2023 at 02:34:11PM +0200, Roman Beranek wrote:
> > > It turns out however that the new dclk rates can't be set exactly as
> > > requested without touching pll-video0*, tcon0 now therefore gets
> > > reparented from pll-mipi to pll-video0-2x which, as it further turns
> > > out, breaks DSI. While simply forbidding the video0-2x mux option seems
> > > to me as the right way to go because there's not much use for it with
> > > non-DSI interfaces either besides the opportunity to power pll-mipi
> > > down, I'd like to run by you first.
> >
> > Sounds reasonable
>
> Okay, I'm unsure of how to denote that in the code however. Should I
> just comment the parent out of the table and put an explanation in
> a comment nearby? Or just erase it? I couldn't find an applicable
> precedent.
I think that forcing the parent at boot, and adding the
CLK_SET_RATE_NOREPARENT flag should be enough.
> > > * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
> > > retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
> > > in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
> > > The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
> > > 294 MHz though because it limits itself to 6 MHz steps.
> >
> > We could also address that though
>
> Should I include it in v2 of the series, or leave it for later?
I guess you can include it into this one too
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
2023-04-05 12:34 ` Roman Beranek
(?)
@ 2023-04-08 7:07 ` Jernej Škrabec
-1 siblings, 0 replies; 60+ messages in thread
From: Jernej Škrabec @ 2023-04-08 7:07 UTC (permalink / raw)
To: Maxime Ripard, Roman Beranek
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Samuel Holland, dri-devel, linux-arm-kernel, linux-sunxi,
linux-kernel
Dne sreda, 05. april 2023 ob 14:34:11 CEST je Roman Beranek napisal(a):
> Hello Maxime,
>
> On Wed Mar 29, 2023 at 9:58 PM CEST, Maxime Ripard wrote:
> > > In order to preserve semantic correctness however, I propose to preface
> > > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
> > > such that dot/pixel is replaced with d/data. What do you think?
> >
> > I don't think it's exposed to the userspace in any way so it makes sense
> > to me
> Here's a new series that includes those renames:
> <https://lore.kernel.org/all/20230331110245.43527-1-me@crly.cz/>
>
> It turns out however that the new dclk rates can't be set exactly as
> requested without touching pll-video0*, tcon0 now therefore gets
> reparented from pll-mipi to pll-video0-2x which, as it further turns
> out, breaks DSI. While simply forbidding the video0-2x mux option seems
> to me as the right way to go because there's not much use for it with
> non-DSI interfaces either besides the opportunity to power pll-mipi
> down, I'd like to run by you first.
It's been a long time since I looked at A64 HDMI clocks, but IIRC, pll-video0
is the only useful source for HDMI PHY (as opposed to HDMI controller.)
So question remains how to properly support both displays at the same time.
Have you ever tried to make HDMI and DSI work at the same time? This is one of
issues of the PinePhone IIUC.
>
> Kind regards,
> Roman
>
> * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
> retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
> in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
> The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
> 294 MHz though because it limits itself to 6 MHz steps.
Yeah, we added CLK_SET_RATE_PARENT flag to several clocks after initial driver
was merged. Adding this flag sounds completely reasonable.
Best regards,
Jernej
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-04-08 7:07 ` Jernej Škrabec
0 siblings, 0 replies; 60+ messages in thread
From: Jernej Škrabec @ 2023-04-08 7:07 UTC (permalink / raw)
To: Maxime Ripard, Roman Beranek
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Samuel Holland, dri-devel, linux-arm-kernel, linux-sunxi,
linux-kernel
Dne sreda, 05. april 2023 ob 14:34:11 CEST je Roman Beranek napisal(a):
> Hello Maxime,
>
> On Wed Mar 29, 2023 at 9:58 PM CEST, Maxime Ripard wrote:
> > > In order to preserve semantic correctness however, I propose to preface
> > > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
> > > such that dot/pixel is replaced with d/data. What do you think?
> >
> > I don't think it's exposed to the userspace in any way so it makes sense
> > to me
> Here's a new series that includes those renames:
> <https://lore.kernel.org/all/20230331110245.43527-1-me@crly.cz/>
>
> It turns out however that the new dclk rates can't be set exactly as
> requested without touching pll-video0*, tcon0 now therefore gets
> reparented from pll-mipi to pll-video0-2x which, as it further turns
> out, breaks DSI. While simply forbidding the video0-2x mux option seems
> to me as the right way to go because there's not much use for it with
> non-DSI interfaces either besides the opportunity to power pll-mipi
> down, I'd like to run by you first.
It's been a long time since I looked at A64 HDMI clocks, but IIRC, pll-video0
is the only useful source for HDMI PHY (as opposed to HDMI controller.)
So question remains how to properly support both displays at the same time.
Have you ever tried to make HDMI and DSI work at the same time? This is one of
issues of the PinePhone IIUC.
>
> Kind regards,
> Roman
>
> * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
> retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
> in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
> The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
> 294 MHz though because it limits itself to 6 MHz steps.
Yeah, we added CLK_SET_RATE_PARENT flag to several clocks after initial driver
was merged. Adding this flag sounds completely reasonable.
Best regards,
Jernej
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-04-08 7:07 ` Jernej Škrabec
0 siblings, 0 replies; 60+ messages in thread
From: Jernej Škrabec @ 2023-04-08 7:07 UTC (permalink / raw)
To: Maxime Ripard, Roman Beranek
Cc: Samuel Holland, Frank Oltmanns, dri-devel, linux-kernel,
Chen-Yu Tsai, linux-sunxi, linux-arm-kernel
Dne sreda, 05. april 2023 ob 14:34:11 CEST je Roman Beranek napisal(a):
> Hello Maxime,
>
> On Wed Mar 29, 2023 at 9:58 PM CEST, Maxime Ripard wrote:
> > > In order to preserve semantic correctness however, I propose to preface
> > > the change with a patch that renames sun4i_dotclock and tcon-pixel-clock
> > > such that dot/pixel is replaced with d/data. What do you think?
> >
> > I don't think it's exposed to the userspace in any way so it makes sense
> > to me
> Here's a new series that includes those renames:
> <https://lore.kernel.org/all/20230331110245.43527-1-me@crly.cz/>
>
> It turns out however that the new dclk rates can't be set exactly as
> requested without touching pll-video0*, tcon0 now therefore gets
> reparented from pll-mipi to pll-video0-2x which, as it further turns
> out, breaks DSI. While simply forbidding the video0-2x mux option seems
> to me as the right way to go because there's not much use for it with
> non-DSI interfaces either besides the opportunity to power pll-mipi
> down, I'd like to run by you first.
It's been a long time since I looked at A64 HDMI clocks, but IIRC, pll-video0
is the only useful source for HDMI PHY (as opposed to HDMI controller.)
So question remains how to properly support both displays at the same time.
Have you ever tried to make HDMI and DSI work at the same time? This is one of
issues of the PinePhone IIUC.
>
> Kind regards,
> Roman
>
> * As pll-mipi doesn't have CLK_SET_RATE_PARENT flag set, pll-video0
> retains its boot-time rate of 294 MHz set by sunxi-dw-hdmi driver
> in u-boot. Why 294 MHz (as opposed to the default rate of 297 MHz)?
> The driver actually asks for 297 MHz, clock_set_pll3 rounds it to
> 294 MHz though because it limits itself to 6 MHz steps.
Yeah, we added CLK_SET_RATE_PARENT flag to several clocks after initial driver
was merged. Adding this flag sounds completely reasonable.
Best regards,
Jernej
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
2023-04-08 7:07 ` Jernej Škrabec
(?)
@ 2023-04-12 1:22 ` Roman Beranek
-1 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-04-12 1:22 UTC (permalink / raw)
To: Jernej Škrabec, Maxime Ripard
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Samuel Holland, dri-devel, linux-arm-kernel, linux-sunxi,
linux-kernel, icenowy
On Sat Apr 8, 2023 at 9:07 AM CEST, Jernej Škrabec wrote:
> Dne sreda, 05. april 2023 ob 14:34:11 CEST je Roman Beranek napisal(a):
> > While simply forbidding the video0-2x mux option seems
> > to me as the right way to go because there's not much use for it with
> > non-DSI interfaces either besides the opportunity to power pll-mipi
> > down, I'd like to run by you first.
>
> It's been a long time since I looked at A64 HDMI clocks, but IIRC, pll-video0
> is the only useful source for HDMI PHY (as opposed to HDMI controller.)
> So question remains how to properly support both displays at the same time.
>
Correct.
> Have you ever tried to make HDMI and DSI work at the same time? This is one of
> issues of the PinePhone IIUC.
>
Yes, I have. Prusa3D's SL1 printer, on which I previously worked on,
uses both outputs simultaneously. I had encountered the same reparenting
problem back then but since I hadn't been able to identify it,
I resorted to fiddling with the DSI pixelclock until it worked.
DSI & HDMI co-existence is yet another reasoni though for forbidding
the pll-video-2x parent. megi's kernel includes Mr. Zheng's commit which
does the same.
<https://github.com/megous/linux/commit/7374d57>
Best wishes
Roman Beranek
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-04-12 1:22 ` Roman Beranek
0 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-04-12 1:22 UTC (permalink / raw)
To: Jernej Škrabec, Maxime Ripard
Cc: Samuel Holland, Frank Oltmanns, dri-devel, linux-kernel,
Chen-Yu Tsai, linux-sunxi, linux-arm-kernel, icenowy
On Sat Apr 8, 2023 at 9:07 AM CEST, Jernej Škrabec wrote:
> Dne sreda, 05. april 2023 ob 14:34:11 CEST je Roman Beranek napisal(a):
> > While simply forbidding the video0-2x mux option seems
> > to me as the right way to go because there's not much use for it with
> > non-DSI interfaces either besides the opportunity to power pll-mipi
> > down, I'd like to run by you first.
>
> It's been a long time since I looked at A64 HDMI clocks, but IIRC, pll-video0
> is the only useful source for HDMI PHY (as opposed to HDMI controller.)
> So question remains how to properly support both displays at the same time.
>
Correct.
> Have you ever tried to make HDMI and DSI work at the same time? This is one of
> issues of the PinePhone IIUC.
>
Yes, I have. Prusa3D's SL1 printer, on which I previously worked on,
uses both outputs simultaneously. I had encountered the same reparenting
problem back then but since I hadn't been able to identify it,
I resorted to fiddling with the DSI pixelclock until it worked.
DSI & HDMI co-existence is yet another reasoni though for forbidding
the pll-video-2x parent. megi's kernel includes Mr. Zheng's commit which
does the same.
<https://github.com/megous/linux/commit/7374d57>
Best wishes
Roman Beranek
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-04-12 1:22 ` Roman Beranek
0 siblings, 0 replies; 60+ messages in thread
From: Roman Beranek @ 2023-04-12 1:22 UTC (permalink / raw)
To: Jernej Škrabec, Maxime Ripard
Cc: Frank Oltmanns, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Samuel Holland, dri-devel, linux-arm-kernel, linux-sunxi,
linux-kernel, icenowy
On Sat Apr 8, 2023 at 9:07 AM CEST, Jernej Škrabec wrote:
> Dne sreda, 05. april 2023 ob 14:34:11 CEST je Roman Beranek napisal(a):
> > While simply forbidding the video0-2x mux option seems
> > to me as the right way to go because there's not much use for it with
> > non-DSI interfaces either besides the opportunity to power pll-mipi
> > down, I'd like to run by you first.
>
> It's been a long time since I looked at A64 HDMI clocks, but IIRC, pll-video0
> is the only useful source for HDMI PHY (as opposed to HDMI controller.)
> So question remains how to properly support both displays at the same time.
>
Correct.
> Have you ever tried to make HDMI and DSI work at the same time? This is one of
> issues of the PinePhone IIUC.
>
Yes, I have. Prusa3D's SL1 printer, on which I previously worked on,
uses both outputs simultaneously. I had encountered the same reparenting
problem back then but since I hadn't been able to identify it,
I resorted to fiddling with the DSI pixelclock until it worked.
DSI & HDMI co-existence is yet another reasoni though for forbidding
the pll-video-2x parent. megi's kernel includes Mr. Zheng's commit which
does the same.
<https://github.com/megous/linux/commit/7374d57>
Best wishes
Roman Beranek
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread
* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
2023-03-27 20:20 ` Maxime Ripard
(?)
@ 2023-03-28 19:28 ` Frank Oltmanns
-1 siblings, 0 replies; 60+ messages in thread
From: Frank Oltmanns @ 2023-03-28 19:28 UTC (permalink / raw)
To: Maxime Ripard
Cc: Roman Beranek, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
Hi,
On 2023-03-27 at 22:20:45 +0200, Maxime Ripard <maxime@cerno.tech> wrote:
> Hi,
>
> On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
[...]
>> Actually, I had the following third patch prepared that adjusted the dotclock rate so that the
>> required PLL rate is set. But again, this seems very indirect, so that’s why I refrained from
>> submitting it and I submitted the linked patch above instead.
>>
>> Anyway, here is the third proposal:
>>
>> — a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
>> regulator_disable(dsi->regulator);
>> }
>>
>> +static bool sun6i_dsi_encoder_mode_fixup(
>> ⁃ struct drm_encoder *encoder,
>> ⁃ const struct drm_display_mode *mode,
>> ⁃ struct drm_display_mode *adjusted_mode)
>> +{
>> ⁃ if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
>> ⁃ /*
>> ⁃ * For DSI the PLL rate has to respect the bits per pixel and
>> ⁃ * number of lanes.
>> ⁃ *
>> ⁃ * According to the BSP code:
>> ⁃ * PLL rate = DOTCLOCK * bpp / lanes
>> ⁃ *
>> ⁃ * Therefore, the clock has to be adjusted in order to set the
>> ⁃ * correct PLL rate when actually setting the clock.
>> ⁃ */
>> ⁃ struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
>> ⁃ struct mipi_dsi_device *device = dsi->device;
>> ⁃ u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
>> ⁃ u8 lanes = device->lanes;
>> ⁃
>>
>> ⁃ adjusted_mode->crtc_clock = mode->crtc_clock
>> ⁃ * bpp / (lanes * SUN6I_DSI_TCON_DIV);
>> ⁃ }
>> ⁃
>>
>> ⁃ return true;
>> +}
>> ⁃ static int sun6i_dsi_get_modes(struct drm_connector *connector)
>> {
>> struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
>> @@ -851,6 +879,7 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
>> static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
>> .disable = sun6i_dsi_encoder_disable,
>> .enable = sun6i_dsi_encoder_enable,
>> ⁃ .mode_fixup = sun6i_dsi_encoder_mode_fixup,
>> };
>
> It's not clear to me what this patch is supposed to be doing, there's no mode_fixup implementation
> upstream?
>
Sorry, my mail client tried some fancy formatting. :(
This is the patch again.
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
regulator_disable(dsi->regulator);
}
+static bool sun6i_dsi_encoder_mode_fixup(
+ struct drm_encoder *encoder,
+ const struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode)
+{
+ if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
+ /*
+ * For DSI the PLL rate has to respect the bits per pixel and
+ * number of lanes.
+ *
+ * According to the BSP code:
+ * PLL rate = DOTCLOCK * bpp / lanes
+ *
+ * Therefore, the clock has to be adjusted in order to set the
+ * correct PLL rate when actually setting the clock.
+ */
+ struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
+ struct mipi_dsi_device *device = dsi->device;
+ u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
+ u8 lanes = device->lanes;
+
+ adjusted_mode->crtc_clock = mode->crtc_clock
+ * bpp / (lanes * SUN6I_DSI_TCON_DIV);
+ }
+
+ return true;
+}
+
static int sun6i_dsi_get_modes(struct drm_connector *connector)
{
struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
@@ -851,6 +879,7 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
.disable = sun6i_dsi_encoder_disable,
.enable = sun6i_dsi_encoder_enable,
+ .mode_fixup = sun6i_dsi_encoder_mode_fixup,
};
static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi,
I still like the original patch better, but I'd be happy to submit this
as a proper patch, if this is more to your liking.
Thanks,
Frank
> Maxime
>
--
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-28 19:28 ` Frank Oltmanns
0 siblings, 0 replies; 60+ messages in thread
From: Frank Oltmanns @ 2023-03-28 19:28 UTC (permalink / raw)
To: Maxime Ripard
Cc: Roman Beranek, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
Hi,
On 2023-03-27 at 22:20:45 +0200, Maxime Ripard <maxime@cerno.tech> wrote:
> Hi,
>
> On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
[...]
>> Actually, I had the following third patch prepared that adjusted the dotclock rate so that the
>> required PLL rate is set. But again, this seems very indirect, so that’s why I refrained from
>> submitting it and I submitted the linked patch above instead.
>>
>> Anyway, here is the third proposal:
>>
>> — a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
>> regulator_disable(dsi->regulator);
>> }
>>
>> +static bool sun6i_dsi_encoder_mode_fixup(
>> ⁃ struct drm_encoder *encoder,
>> ⁃ const struct drm_display_mode *mode,
>> ⁃ struct drm_display_mode *adjusted_mode)
>> +{
>> ⁃ if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
>> ⁃ /*
>> ⁃ * For DSI the PLL rate has to respect the bits per pixel and
>> ⁃ * number of lanes.
>> ⁃ *
>> ⁃ * According to the BSP code:
>> ⁃ * PLL rate = DOTCLOCK * bpp / lanes
>> ⁃ *
>> ⁃ * Therefore, the clock has to be adjusted in order to set the
>> ⁃ * correct PLL rate when actually setting the clock.
>> ⁃ */
>> ⁃ struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
>> ⁃ struct mipi_dsi_device *device = dsi->device;
>> ⁃ u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
>> ⁃ u8 lanes = device->lanes;
>> ⁃
>>
>> ⁃ adjusted_mode->crtc_clock = mode->crtc_clock
>> ⁃ * bpp / (lanes * SUN6I_DSI_TCON_DIV);
>> ⁃ }
>> ⁃
>>
>> ⁃ return true;
>> +}
>> ⁃ static int sun6i_dsi_get_modes(struct drm_connector *connector)
>> {
>> struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
>> @@ -851,6 +879,7 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
>> static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
>> .disable = sun6i_dsi_encoder_disable,
>> .enable = sun6i_dsi_encoder_enable,
>> ⁃ .mode_fixup = sun6i_dsi_encoder_mode_fixup,
>> };
>
> It's not clear to me what this patch is supposed to be doing, there's no mode_fixup implementation
> upstream?
>
Sorry, my mail client tried some fancy formatting. :(
This is the patch again.
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
regulator_disable(dsi->regulator);
}
+static bool sun6i_dsi_encoder_mode_fixup(
+ struct drm_encoder *encoder,
+ const struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode)
+{
+ if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
+ /*
+ * For DSI the PLL rate has to respect the bits per pixel and
+ * number of lanes.
+ *
+ * According to the BSP code:
+ * PLL rate = DOTCLOCK * bpp / lanes
+ *
+ * Therefore, the clock has to be adjusted in order to set the
+ * correct PLL rate when actually setting the clock.
+ */
+ struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
+ struct mipi_dsi_device *device = dsi->device;
+ u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
+ u8 lanes = device->lanes;
+
+ adjusted_mode->crtc_clock = mode->crtc_clock
+ * bpp / (lanes * SUN6I_DSI_TCON_DIV);
+ }
+
+ return true;
+}
+
static int sun6i_dsi_get_modes(struct drm_connector *connector)
{
struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
@@ -851,6 +879,7 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
.disable = sun6i_dsi_encoder_disable,
.enable = sun6i_dsi_encoder_enable,
+ .mode_fixup = sun6i_dsi_encoder_mode_fixup,
};
static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi,
I still like the original patch better, but I'd be happy to submit this
as a proper patch, if this is more to your liking.
Thanks,
Frank
> Maxime
>
--
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-28 19:28 ` Frank Oltmanns
0 siblings, 0 replies; 60+ messages in thread
From: Frank Oltmanns @ 2023-03-28 19:28 UTC (permalink / raw)
To: Maxime Ripard
Cc: Samuel Holland, linux-kernel, Jernej Skrabec, Roman Beranek,
Chen-Yu Tsai, dri-devel, linux-sunxi, linux-arm-kernel
Hi,
On 2023-03-27 at 22:20:45 +0200, Maxime Ripard <maxime@cerno.tech> wrote:
> Hi,
>
> On Sat, Mar 25, 2023 at 12:40:04PM +0100, Frank Oltmanns wrote:
[...]
>> Actually, I had the following third patch prepared that adjusted the dotclock rate so that the
>> required PLL rate is set. But again, this seems very indirect, so that’s why I refrained from
>> submitting it and I submitted the linked patch above instead.
>>
>> Anyway, here is the third proposal:
>>
>> — a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
>> regulator_disable(dsi->regulator);
>> }
>>
>> +static bool sun6i_dsi_encoder_mode_fixup(
>> ⁃ struct drm_encoder *encoder,
>> ⁃ const struct drm_display_mode *mode,
>> ⁃ struct drm_display_mode *adjusted_mode)
>> +{
>> ⁃ if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
>> ⁃ /*
>> ⁃ * For DSI the PLL rate has to respect the bits per pixel and
>> ⁃ * number of lanes.
>> ⁃ *
>> ⁃ * According to the BSP code:
>> ⁃ * PLL rate = DOTCLOCK * bpp / lanes
>> ⁃ *
>> ⁃ * Therefore, the clock has to be adjusted in order to set the
>> ⁃ * correct PLL rate when actually setting the clock.
>> ⁃ */
>> ⁃ struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
>> ⁃ struct mipi_dsi_device *device = dsi->device;
>> ⁃ u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
>> ⁃ u8 lanes = device->lanes;
>> ⁃
>>
>> ⁃ adjusted_mode->crtc_clock = mode->crtc_clock
>> ⁃ * bpp / (lanes * SUN6I_DSI_TCON_DIV);
>> ⁃ }
>> ⁃
>>
>> ⁃ return true;
>> +}
>> ⁃ static int sun6i_dsi_get_modes(struct drm_connector *connector)
>> {
>> struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
>> @@ -851,6 +879,7 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
>> static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
>> .disable = sun6i_dsi_encoder_disable,
>> .enable = sun6i_dsi_encoder_enable,
>> ⁃ .mode_fixup = sun6i_dsi_encoder_mode_fixup,
>> };
>
> It's not clear to me what this patch is supposed to be doing, there's no mode_fixup implementation
> upstream?
>
Sorry, my mail client tried some fancy formatting. :(
This is the patch again.
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
regulator_disable(dsi->regulator);
}
+static bool sun6i_dsi_encoder_mode_fixup(
+ struct drm_encoder *encoder,
+ const struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode)
+{
+ if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
+ /*
+ * For DSI the PLL rate has to respect the bits per pixel and
+ * number of lanes.
+ *
+ * According to the BSP code:
+ * PLL rate = DOTCLOCK * bpp / lanes
+ *
+ * Therefore, the clock has to be adjusted in order to set the
+ * correct PLL rate when actually setting the clock.
+ */
+ struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
+ struct mipi_dsi_device *device = dsi->device;
+ u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
+ u8 lanes = device->lanes;
+
+ adjusted_mode->crtc_clock = mode->crtc_clock
+ * bpp / (lanes * SUN6I_DSI_TCON_DIV);
+ }
+
+ return true;
+}
+
static int sun6i_dsi_get_modes(struct drm_connector *connector)
{
struct sun6i_dsi *dsi = connector_to_sun6i_dsi(connector);
@@ -851,6 +879,7 @@ static const struct drm_connector_funcs sun6i_dsi_connector_funcs = {
static const struct drm_encoder_helper_funcs sun6i_dsi_enc_helper_funcs = {
.disable = sun6i_dsi_encoder_disable,
.enable = sun6i_dsi_encoder_enable,
+ .mode_fixup = sun6i_dsi_encoder_mode_fixup,
};
static u32 sun6i_dsi_dcs_build_pkt_hdr(struct sun6i_dsi *dsi,
I still like the original patch better, but I'd be happy to submit this
as a proper patch, if this is more to your liking.
Thanks,
Frank
> Maxime
>
--
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
2023-03-28 19:28 ` Frank Oltmanns
(?)
@ 2023-03-29 19:56 ` Maxime Ripard
-1 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-03-29 19:56 UTC (permalink / raw)
To: Frank Oltmanns
Cc: Roman Beranek, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]
Hi,
On Tue, Mar 28, 2023 at 09:28:19PM +0200, Frank Oltmanns wrote:
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
> regulator_disable(dsi->regulator);
> }
>
> +static bool sun6i_dsi_encoder_mode_fixup(
> + struct drm_encoder *encoder,
> + const struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
So, mode_fixup is kind of deprecated in favour of atomic_check
> +{
> + if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
> + /*
> + * For DSI the PLL rate has to respect the bits per pixel and
> + * number of lanes.
> + *
> + * According to the BSP code:
> + * PLL rate = DOTCLOCK * bpp / lanes
> + *
> + * Therefore, the clock has to be adjusted in order to set the
> + * correct PLL rate when actually setting the clock.
> + */
> + struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
> + struct mipi_dsi_device *device = dsi->device;
> + u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> + u8 lanes = device->lanes;
> +
> + adjusted_mode->crtc_clock = mode->crtc_clock
> + * bpp / (lanes * SUN6I_DSI_TCON_DIV);
And that's visible to the userspace, so it's not where we should store
that value. I guess the best way to do something similar would be to
store it into crtc_state, and then reuse it there. But it starts to make
a lot of rather complicated code compared to your previous patch.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-29 19:56 ` Maxime Ripard
0 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-03-29 19:56 UTC (permalink / raw)
To: Frank Oltmanns
Cc: Roman Beranek, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1568 bytes --]
Hi,
On Tue, Mar 28, 2023 at 09:28:19PM +0200, Frank Oltmanns wrote:
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
> regulator_disable(dsi->regulator);
> }
>
> +static bool sun6i_dsi_encoder_mode_fixup(
> + struct drm_encoder *encoder,
> + const struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
So, mode_fixup is kind of deprecated in favour of atomic_check
> +{
> + if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
> + /*
> + * For DSI the PLL rate has to respect the bits per pixel and
> + * number of lanes.
> + *
> + * According to the BSP code:
> + * PLL rate = DOTCLOCK * bpp / lanes
> + *
> + * Therefore, the clock has to be adjusted in order to set the
> + * correct PLL rate when actually setting the clock.
> + */
> + struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
> + struct mipi_dsi_device *device = dsi->device;
> + u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> + u8 lanes = device->lanes;
> +
> + adjusted_mode->crtc_clock = mode->crtc_clock
> + * bpp / (lanes * SUN6I_DSI_TCON_DIV);
And that's visible to the userspace, so it's not where we should store
that value. I guess the best way to do something similar would be to
store it into crtc_state, and then reuse it there. But it starts to make
a lot of rather complicated code compared to your previous patch.
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-29 19:56 ` Maxime Ripard
0 siblings, 0 replies; 60+ messages in thread
From: Maxime Ripard @ 2023-03-29 19:56 UTC (permalink / raw)
To: Frank Oltmanns
Cc: Samuel Holland, linux-kernel, Jernej Skrabec, Roman Beranek,
Chen-Yu Tsai, dri-devel, linux-sunxi, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]
Hi,
On Tue, Mar 28, 2023 at 09:28:19PM +0200, Frank Oltmanns wrote:
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
> regulator_disable(dsi->regulator);
> }
>
> +static bool sun6i_dsi_encoder_mode_fixup(
> + struct drm_encoder *encoder,
> + const struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
So, mode_fixup is kind of deprecated in favour of atomic_check
> +{
> + if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
> + /*
> + * For DSI the PLL rate has to respect the bits per pixel and
> + * number of lanes.
> + *
> + * According to the BSP code:
> + * PLL rate = DOTCLOCK * bpp / lanes
> + *
> + * Therefore, the clock has to be adjusted in order to set the
> + * correct PLL rate when actually setting the clock.
> + */
> + struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
> + struct mipi_dsi_device *device = dsi->device;
> + u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
> + u8 lanes = device->lanes;
> +
> + adjusted_mode->crtc_clock = mode->crtc_clock
> + * bpp / (lanes * SUN6I_DSI_TCON_DIV);
And that's visible to the userspace, so it's not where we should store
that value. I guess the best way to do something similar would be to
store it into crtc_state, and then reuse it there. But it starts to make
a lot of rather complicated code compared to your previous patch.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
2023-03-29 19:56 ` Maxime Ripard
(?)
@ 2023-03-30 4:41 ` Frank Oltmanns
-1 siblings, 0 replies; 60+ messages in thread
From: Frank Oltmanns @ 2023-03-30 4:41 UTC (permalink / raw)
To: Maxime Ripard
Cc: Roman Beranek, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
Hi,
On 2023-03-29 at 21:56:39 +0200, Maxime Ripard <maxime@cerno.tech> wrote:
> Hi,
>
> On Tue, Mar 28, 2023 at 09:28:19PM +0200, Frank Oltmanns wrote:
>> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
>> regulator_disable(dsi->regulator);
>> }
>>
>> +static bool sun6i_dsi_encoder_mode_fixup(
>> + struct drm_encoder *encoder,
>> + const struct drm_display_mode *mode,
>> + struct drm_display_mode *adjusted_mode)
>
> So, mode_fixup is kind of deprecated in favour of atomic_check
I see. Thanks for pointing that out.
>> +{
>> + if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
>> + /*
>> + * For DSI the PLL rate has to respect the bits per pixel and
>> + * number of lanes.
>> + *
>> + * According to the BSP code:
>> + * PLL rate = DOTCLOCK * bpp / lanes
>> + *
>> + * Therefore, the clock has to be adjusted in order to set the
>> + * correct PLL rate when actually setting the clock.
>> + */
>> + struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
>> + struct mipi_dsi_device *device = dsi->device;
>> + u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
>> + u8 lanes = device->lanes;
>> +
>> + adjusted_mode->crtc_clock = mode->crtc_clock
>> + * bpp / (lanes * SUN6I_DSI_TCON_DIV);
>
> And that's visible to the userspace, so it's not where we should store
> that value. I guess the best way to do something similar would be to
> store it into crtc_state, and then reuse it there. But it starts to make
> a lot of rather complicated code compared to your previous patch.
Ah, interesting. But I agree, let's stick to the simpler aproach.
Thanks,
Frank
>
> Maxime
>
> [[End of PGP Signed Part]]
--
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-30 4:41 ` Frank Oltmanns
0 siblings, 0 replies; 60+ messages in thread
From: Frank Oltmanns @ 2023-03-30 4:41 UTC (permalink / raw)
To: Maxime Ripard
Cc: Roman Beranek, Chen-Yu Tsai, David Airlie, Daniel Vetter,
Jernej Skrabec, Samuel Holland, dri-devel, linux-arm-kernel,
linux-sunxi, linux-kernel
Hi,
On 2023-03-29 at 21:56:39 +0200, Maxime Ripard <maxime@cerno.tech> wrote:
> Hi,
>
> On Tue, Mar 28, 2023 at 09:28:19PM +0200, Frank Oltmanns wrote:
>> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
>> regulator_disable(dsi->regulator);
>> }
>>
>> +static bool sun6i_dsi_encoder_mode_fixup(
>> + struct drm_encoder *encoder,
>> + const struct drm_display_mode *mode,
>> + struct drm_display_mode *adjusted_mode)
>
> So, mode_fixup is kind of deprecated in favour of atomic_check
I see. Thanks for pointing that out.
>> +{
>> + if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
>> + /*
>> + * For DSI the PLL rate has to respect the bits per pixel and
>> + * number of lanes.
>> + *
>> + * According to the BSP code:
>> + * PLL rate = DOTCLOCK * bpp / lanes
>> + *
>> + * Therefore, the clock has to be adjusted in order to set the
>> + * correct PLL rate when actually setting the clock.
>> + */
>> + struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
>> + struct mipi_dsi_device *device = dsi->device;
>> + u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
>> + u8 lanes = device->lanes;
>> +
>> + adjusted_mode->crtc_clock = mode->crtc_clock
>> + * bpp / (lanes * SUN6I_DSI_TCON_DIV);
>
> And that's visible to the userspace, so it's not where we should store
> that value. I guess the best way to do something similar would be to
> store it into crtc_state, and then reuse it there. But it starts to make
> a lot of rather complicated code compared to your previous patch.
Ah, interesting. But I agree, let's stick to the simpler aproach.
Thanks,
Frank
>
> Maxime
>
> [[End of PGP Signed Part]]
--
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 60+ messages in thread* Re: [PATCH] drm/sun4i: uncouple DSI dotclock divider from TCON0_DCLK_REG
@ 2023-03-30 4:41 ` Frank Oltmanns
0 siblings, 0 replies; 60+ messages in thread
From: Frank Oltmanns @ 2023-03-30 4:41 UTC (permalink / raw)
To: Maxime Ripard
Cc: Samuel Holland, linux-kernel, Jernej Skrabec, Roman Beranek,
Chen-Yu Tsai, dri-devel, linux-sunxi, linux-arm-kernel
Hi,
On 2023-03-29 at 21:56:39 +0200, Maxime Ripard <maxime@cerno.tech> wrote:
> Hi,
>
> On Tue, Mar 28, 2023 at 09:28:19PM +0200, Frank Oltmanns wrote:
>> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> @@ -819,6 +819,34 @@ static void sun6i_dsi_encoder_disable(struct drm_encoder *encoder)
>> regulator_disable(dsi->regulator);
>> }
>>
>> +static bool sun6i_dsi_encoder_mode_fixup(
>> + struct drm_encoder *encoder,
>> + const struct drm_display_mode *mode,
>> + struct drm_display_mode *adjusted_mode)
>
> So, mode_fixup is kind of deprecated in favour of atomic_check
I see. Thanks for pointing that out.
>> +{
>> + if (encoder->encoder_type == DRM_MODE_ENCODER_DSI) {
>> + /*
>> + * For DSI the PLL rate has to respect the bits per pixel and
>> + * number of lanes.
>> + *
>> + * According to the BSP code:
>> + * PLL rate = DOTCLOCK * bpp / lanes
>> + *
>> + * Therefore, the clock has to be adjusted in order to set the
>> + * correct PLL rate when actually setting the clock.
>> + */
>> + struct sun6i_dsi *dsi = encoder_to_sun6i_dsi(encoder);
>> + struct mipi_dsi_device *device = dsi->device;
>> + u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format);
>> + u8 lanes = device->lanes;
>> +
>> + adjusted_mode->crtc_clock = mode->crtc_clock
>> + * bpp / (lanes * SUN6I_DSI_TCON_DIV);
>
> And that's visible to the userspace, so it's not where we should store
> that value. I guess the best way to do something similar would be to
> store it into crtc_state, and then reuse it there. But it starts to make
> a lot of rather complicated code compared to your previous patch.
Ah, interesting. But I agree, let's stick to the simpler aproach.
Thanks,
Frank
>
> Maxime
>
> [[End of PGP Signed Part]]
--
^ permalink raw reply [flat|nested] 60+ messages in thread