lkml.org 
[lkml]   [2017]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 06/18] drm/sun4i: tcon: Don't rely on encoders to enable the TCON
    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
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2017-07-20 23:22    [W:4.139 / U:0.180 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site