diff mbox

[10/15] drm/sun4i: tcon: Switch mux on only for composite

Message ID 3f70bcfe2ec03188c06d54a262d49ad91963a510.1488876832.git-series.maxime.ripard@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maxime Ripard March 7, 2017, 8:56 a.m. UTC
Even though that mux is undocumented, it seems like it needs to be set to 1
when using composite, and 0 when using HDMI.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/gpu/drm/sun4i/sun4i_tcon.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Chen-Yu Tsai March 8, 2017, 3:51 a.m. UTC | #1
On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Even though that mux is undocumented, it seems like it needs to be set to 1
> when using composite, and 0 when using HDMI.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/gpu/drm/sun4i/sun4i_tcon.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> index d2335f109601..93249c5ab1e4 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> @@ -268,11 +268,16 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
>                            SUN4I_TCON_GCTL_IOMAP_MASK,
>                            SUN4I_TCON_GCTL_IOMAP_TCON1);
>
> +       if (encoder->encoder_type == DRM_MODE_ENCODER_TVDAC)
> +               val = 1;
> +       else
> +               val = 0;
> +
>         /*
>          * FIXME: Undocumented bits
>          */
>         if (tcon->quirks->has_unknown_mux)
> -               regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, 1);
> +               regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, val);

We might want to do this the other way around, i.e. exporting

    int sun4i_tcon_mux_set(struct drm_device *drm, int encoder_type,
                           int pipeline)

and have downstream encoders call it. For the A31, the mux is not exclusively
used for channel 1; there is a mux setting for MIPI DSI as well, but AFAIK
DSI is connected to channel 0.

Additionally, the mux registers are only valid in the first TCON, meaning
it must available be active in 2 pipeline chips. It's also why we'd pass
"struct drm_device *" instead of "struct sun4i_tcon *".


Regards
ChenYu

>  }
>  EXPORT_SYMBOL(sun4i_tcon1_mode_set);
>
> --
> git-series 0.8.11
Stefan Monnier March 8, 2017, 4:16 a.m. UTC | #2
>> +       if (encoder->encoder_type == DRM_MODE_ENCODER_TVDAC)
>> +               val = 1;
>> +       else
>> +               val = 0;

Isn't this better written as

           val = (encoder->encoder_type == DRM_MODE_ENCODER_TVDAC);


-- Stefan
Maxime Ripard March 9, 2017, 10:58 a.m. UTC | #3
On Wed, Mar 08, 2017 at 11:51:39AM +0800, Chen-Yu Tsai wrote:
> On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Even though that mux is undocumented, it seems like it needs to be set to 1
> > when using composite, and 0 when using HDMI.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun4i_tcon.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > index d2335f109601..93249c5ab1e4 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > @@ -268,11 +268,16 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
> >                            SUN4I_TCON_GCTL_IOMAP_MASK,
> >                            SUN4I_TCON_GCTL_IOMAP_TCON1);
> >
> > +       if (encoder->encoder_type == DRM_MODE_ENCODER_TVDAC)
> > +               val = 1;
> > +       else
> > +               val = 0;
> > +
> >         /*
> >          * FIXME: Undocumented bits
> >          */
> >         if (tcon->quirks->has_unknown_mux)
> > -               regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, 1);
> > +               regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, val);
> 
> We might want to do this the other way around, i.e. exporting
> 
>     int sun4i_tcon_mux_set(struct drm_device *drm, int encoder_type,
>                            int pipeline)
> 
> and have downstream encoders call it. For the A31, the mux is not exclusively
> used for channel 1; there is a mux setting for MIPI DSI as well, but AFAIK
> DSI is connected to channel 0.

We could make it part of sun4i_tcon_channel_enable too, though. What
do you think?

> Additionally, the mux registers are only valid in the first TCON, meaning
> it must available be active in 2 pipeline chips. It's also why we'd pass
> "struct drm_device *" instead of "struct sun4i_tcon *".

Hmmmm. That's going to be tricky to support. Has this been confirmed
somehow? Is the register used for something else on TCON1?

Thanks,
Maxime
Chen-Yu Tsai March 9, 2017, 11:31 a.m. UTC | #4
On Thu, Mar 9, 2017 at 6:58 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Wed, Mar 08, 2017 at 11:51:39AM +0800, Chen-Yu Tsai wrote:
>> On Tue, Mar 7, 2017 at 4:56 PM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > Even though that mux is undocumented, it seems like it needs to be set to 1
>> > when using composite, and 0 when using HDMI.
>> >
>> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> > ---
>> >  drivers/gpu/drm/sun4i/sun4i_tcon.c | 7 ++++++-
>> >  1 file changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> > index d2335f109601..93249c5ab1e4 100644
>> > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> > @@ -268,11 +268,16 @@ void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
>> >                            SUN4I_TCON_GCTL_IOMAP_MASK,
>> >                            SUN4I_TCON_GCTL_IOMAP_TCON1);
>> >
>> > +       if (encoder->encoder_type == DRM_MODE_ENCODER_TVDAC)
>> > +               val = 1;
>> > +       else
>> > +               val = 0;
>> > +
>> >         /*
>> >          * FIXME: Undocumented bits
>> >          */
>> >         if (tcon->quirks->has_unknown_mux)
>> > -               regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, 1);
>> > +               regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, val);
>>
>> We might want to do this the other way around, i.e. exporting
>>
>>     int sun4i_tcon_mux_set(struct drm_device *drm, int encoder_type,
>>                            int pipeline)
>>
>> and have downstream encoders call it. For the A31, the mux is not exclusively
>> used for channel 1; there is a mux setting for MIPI DSI as well, but AFAIK
>> DSI is connected to channel 0.
>
> We could make it part of sun4i_tcon_channel_enable too, though. What
> do you think?

We still need some way of figuring out what mux value to set for those
cases. Let's keep your solution for now. We can work on it later when
we have an actual use case to deal with.

>
>> Additionally, the mux registers are only valid in the first TCON, meaning
>> it must available be active in 2 pipeline chips. It's also why we'd pass
>> "struct drm_device *" instead of "struct sun4i_tcon *".
>
> Hmmmm. That's going to be tricky to support. Has this been confirmed
> somehow? Is the register used for something else on TCON1?

At this point, the only reference is Allwinner's kernel, and the old 3.4
kernel for A10/A20. I could try getting HDMI working on the A31 to get
some real results.

FWIW, the registers do not seem to be aliased across the two TCONs.

Regards
ChenYu
Maxime Ripard March 9, 2017, 2:55 p.m. UTC | #5
On Thu, Mar 09, 2017 at 07:31:27PM +0800, Chen-Yu Tsai wrote:
> >> Additionally, the mux registers are only valid in the first TCON, meaning
> >> it must available be active in 2 pipeline chips. It's also why we'd pass
> >> "struct drm_device *" instead of "struct sun4i_tcon *".
> >
> > Hmmmm. That's going to be tricky to support. Has this been confirmed
> > somehow? Is the register used for something else on TCON1?
> 
> At this point, the only reference is Allwinner's kernel, and the old 3.4
> kernel for A10/A20. I could try getting HDMI working on the A31 to get
> some real results.
> 
> FWIW, the registers do not seem to be aliased across the two TCONs.

Then maybe we don't need to care, and we can just always write to the
mux?

Maxime
diff mbox

Patch

diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
index d2335f109601..93249c5ab1e4 100644
--- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
+++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
@@ -268,11 +268,16 @@  void sun4i_tcon1_mode_set(struct sun4i_tcon *tcon, struct drm_encoder *encoder,
 			   SUN4I_TCON_GCTL_IOMAP_MASK,
 			   SUN4I_TCON_GCTL_IOMAP_TCON1);
 
+	if (encoder->encoder_type == DRM_MODE_ENCODER_TVDAC)
+		val = 1;
+	else
+		val = 0;
+
 	/*
 	 * FIXME: Undocumented bits
 	 */
 	if (tcon->quirks->has_unknown_mux)
-		regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, 1);
+		regmap_write(tcon->regs, SUN4I_TCON_MUX_CTRL_REG, val);
 }
 EXPORT_SYMBOL(sun4i_tcon1_mode_set);