Re: [PATCH 06/18] drm/sun4i: tcon: Don't rely on encoders to enable the TCON

[Maxime Ripard]

Hi Chen-Yu,

On Fri, Jul 14, 2017 at 11:40:07AM +0800, Chen-Yu Tsai wrote:
> >  static void sun4i_rgb_encoder_mode_set(struct drm_encoder *encoder,
> > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > index d9791292553e..dc70bc2a42a5 100644
> > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
> > @@ -14,6 +14,7 @@
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_encoder.h>
> >  #include <drm/drm_modes.h>
> >  #include <drm/drm_of.h>
> >
> > @@ -32,66 +33,62 @@
> >  #include "sun4i_tcon.h"
> >  #include "sunxi_engine.h"
> >
> > -void sun4i_tcon_disable(struct sun4i_tcon *tcon)
> > +static void sun4i_tcon_channel_set_status(struct sun4i_tcon *tcon, int channel,
> > +                                         bool enabled)
> >  {
> > -       DRM_DEBUG_DRIVER("Disabling TCON\n");
> > +       struct clk *clk;
> >
> > -       /* Disable the TCON */
> > -       regmap_update_bits(tcon->regs, SUN4I_TCON_GCTL_REG,
> > -                          SUN4I_TCON_GCTL_TCON_ENABLE, 0);
> > -}
> > -EXPORT_SYMBOL(sun4i_tcon_disable);
> > -
> > -void sun4i_tcon_enable(struct sun4i_tcon *tcon)
> > -{
> > -       DRM_DEBUG_DRIVER("Enabling TCON\n");
> > -
> > -       /* Enable the TCON */
> > -       regmap_update_bits(tcon->regs, SUN4I_TCON_GCTL_REG,
> > -                          SUN4I_TCON_GCTL_TCON_ENABLE,
> > -                          SUN4I_TCON_GCTL_TCON_ENABLE);
> > -}
> > -EXPORT_SYMBOL(sun4i_tcon_enable);
> > -
> > -void sun4i_tcon_channel_disable(struct sun4i_tcon *tcon, int channel)
> > -{
> > -       DRM_DEBUG_DRIVER("Disabling TCON channel %d\n", channel);
> > -
> > -       /* Disable the TCON's channel */
> > -       if (channel == 0) {
> > +       switch (channel) {
> > +       case 0:
> >                 regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
> > -                                  SUN4I_TCON0_CTL_TCON_ENABLE, 0);
> > -               clk_disable_unprepare(tcon->dclk);
> > +                                  SUN4I_TCON0_CTL_TCON_ENABLE,
> > +                                  enabled ? SUN4I_TCON0_CTL_TCON_ENABLE : 0);
> > +               clk = tcon->dclk;
> > +               break;
> > +       case 1:
> > +               WARN_ON(!tcon->quirks->has_channel_1);
> > +               regmap_update_bits(tcon->regs, SUN4I_TCON1_CTL_REG,
> > +                                  SUN4I_TCON1_CTL_TCON_ENABLE,
> > +                                  enabled ? SUN4I_TCON1_CTL_TCON_ENABLE : 0);
> > +               clk = tcon->sclk1;
> > +               break;
> > +       default:
> > +               DRM_DEBUG_DRIVER("Unknown channel... doing nothing\n");
> >                 return;
> >         }
> >
> > -       WARN_ON(!tcon->quirks->has_channel_1);
> > -       regmap_update_bits(tcon->regs, SUN4I_TCON1_CTL_REG,
> > -                          SUN4I_TCON1_CTL_TCON_ENABLE, 0);
> > -       clk_disable_unprepare(tcon->sclk1);
> > +       if (enabled)
> > +               clk_prepare_enable(clk);
> 
> I wonder if it's better to enable the clk before the TCON?

I think I kept the current behaviour, which seemed to work fine with
that regard.

> 
> > +       else
> > +               clk_disable_unprepare(clk);
> >  }
> > -EXPORT_SYMBOL(sun4i_tcon_channel_disable);
> >
> > -void sun4i_tcon_channel_enable(struct sun4i_tcon *tcon, int channel)
> > +void sun4i_tcon_set_status(struct sun4i_tcon *tcon,
> > +                          struct drm_encoder *encoder,
> > +                          bool enabled)
> >  {
> > -       DRM_DEBUG_DRIVER("Enabling TCON channel %d\n", channel);
> > -
> > -       /* Enable the TCON's channel */
> > -       if (channel == 0) {
> > -               regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
> > -                                  SUN4I_TCON0_CTL_TCON_ENABLE,
> > -                                  SUN4I_TCON0_CTL_TCON_ENABLE);
> > -               clk_prepare_enable(tcon->dclk);
> > +       int channel;
> > +
> > +       switch (encoder->encoder_type) {
> > +       case DRM_MODE_ENCODER_NONE:
> > +               channel = 0;
> > +               break;
> > +       case DRM_MODE_ENCODER_TMDS:
> > +       case DRM_MODE_ENCODER_TVDAC:
> > +               channel = 1;
> > +               break;
> > +       default:
> > +               DRM_DEBUG_DRIVER("Unknown encoder type, doing nothing...\n");
> 
> We could simply add all the possible types, and print a big warning
> if someone does something unexpected. IMHO this is better than having
> the user enable some hidden debug flag to figure why the display isn't
> working properly.

I'm not sure about all types of encoders, but you're right, it should
be a warning.

> >                 return;
> >         }
> >
> > -       WARN_ON(!tcon->quirks->has_channel_1);
> > -       regmap_update_bits(tcon->regs, SUN4I_TCON1_CTL_REG,
> > -                          SUN4I_TCON1_CTL_TCON_ENABLE,
> > -                          SUN4I_TCON1_CTL_TCON_ENABLE);
> > -       clk_prepare_enable(tcon->sclk1);
> > +       sun4i_tcon_channel_set_status(tcon, channel, enabled);
> > +
> > +       regmap_update_bits(tcon->regs, SUN4I_TCON_GCTL_REG,
> > +                          SUN4I_TCON_GCTL_TCON_ENABLE,
> > +                          enabled ? SUN4I_TCON_GCTL_TCON_ENABLE : 0);
> 
> The global enable bit should be set first.

ACK


> Also the manual says "When itâs disabled, the module will be reset to
> idle state."
> so you might get away with just disabling the global enable bit and returning
> directly after disabling the clock?

I'd rather keep an explicit disable, just in case one SoC is broken,
just like the DE is...

> >  }
> > -EXPORT_SYMBOL(sun4i_tcon_channel_enable);
> > +EXPORT_SYMBOL(sun4i_tcon_set_status);
> 
> The TCON and CRTC code are part of the same module.
> There is no need to export this function.

Ah, right. I'll remove it. Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachment: signature.asc
Description: PGP signature