On Tue, Jun 13, 2017 at 05:51:42PM +0800, Icenowy Zheng wrote: > > > 于 2017年6月13日 GMT+08:00 下午3:44:32, Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx> 写到: > >On Sun, Jun 11, 2017 at 02:43:42PM +0800, icenowy@xxxxxxx wrote: > >> 在 2017-06-07 17:38,Maxime Ripard 写道: > >> > On Mon, Jun 05, 2017 at 12:01:45AM +0800, Icenowy Zheng wrote: > >> > > Allwinner H3 features a TV encoder similar to the one in earlier > >SoCs, > >> > > but has a internal fixed clock divider that divides the TCON1 > >clock > >> > > (called TVE clock in datasheet) by 11. > >> > > > >> > > Add support for it. > >> > > > >> > > Signed-off-by: Icenowy Zheng <icenowy@xxxxxxx> > >> > > --- > >> > > Changes in v2: > >> > > - Quirk part rewritten. > >> > > > >> > > drivers/gpu/drm/sun4i/sun4i_tv.c | 35 > >> > > ++++++++++++++++++++++++++++++++++- > >> > > 1 file changed, 34 insertions(+), 1 deletion(-) > >> > > > >> > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c > >> > > b/drivers/gpu/drm/sun4i/sun4i_tv.c > >> > > index 338b9e5bb2a3..b9ff6d5ea67a 100644 > >> > > --- a/drivers/gpu/drm/sun4i/sun4i_tv.c > >> > > +++ b/drivers/gpu/drm/sun4i/sun4i_tv.c > >> > > @@ -13,6 +13,7 @@ > >> > > #include <linux/clk.h> > >> > > #include <linux/component.h> > >> > > #include <linux/of_address.h> > >> > > +#include <linux/of_device.h> > >> > > #include <linux/regmap.h> > >> > > #include <linux/reset.h> > >> > > > >> > > @@ -169,14 +170,21 @@ struct tv_mode { > >> > > const struct resync_parameters *resync_params; > >> > > }; > >> > > > >> > > +struct sun4i_tv_quirks { > >> > > + int fixed_divider; > >> > > +}; > >> > > + > >> > > struct sun4i_tv { > >> > > struct drm_connector connector; > >> > > struct drm_encoder encoder; > >> > > > >> > > struct clk *clk; > >> > > + struct clk *mod_clk; > >> > > struct regmap *regs; > >> > > struct reset_control *reset; > >> > > > >> > > + const struct sun4i_tv_quirks *quirks; > >> > > + > >> > > struct sun4i_drv *drv; > >> > > }; > >> > > > >> > > @@ -391,6 +399,12 @@ static void sun4i_tv_mode_set(struct > >> > > drm_encoder *encoder, > >> > > struct sun4i_tcon *tcon = crtc->tcon; > >> > > const struct tv_mode *tv_mode = sun4i_tv_find_tv_by_mode(mode); > >> > > > >> > > + if (tv->quirks->fixed_divider) { > >> > > + DRM_DEBUG_DRIVER("Applying fixed divider %d on TVE clock\n", > >> > > + tv->quirks->fixed_divider); > >> > > + mode->crtc_clock *= tv->quirks->fixed_divider; > >> > > + } > >> > > + > >> > > >> > You're not allowed to change the mode in mode_set, and you > >shouldn't > >> > even change it. The output pixel clock is still 27MHz. > >> > > >> > You should implement that using the states, as we discussed > >already. > >> > >> After reading the comments at > >include/drm/drm_modeset_helper_vtables.h, > >> I think the atomic_check function is allowed to directly change > >> the adjust_mode of crtc_state. > >> > >> And according to other comments at include/drm/drm_modes.h, the > >> crtc_clock in adjust_mode should be the clock to be really feed > >> to the hardware. > >> > >> So I think I can just change this in atomic_check. > >> > >> However, the mode_set function of sun4i_tv seems to be not > >> regarding adjusted_mode and still use original mode. > >> > >> So my current design is: > >> - Multiply adjusted_mode in atomic_check. > >> - Feed adjust_mode to TCON code in mode_set function. > >> > >> Is this proper? > >> > >> (From my own understanding to the comments I think so.) > > > >No, it's not. The pixel clock in the mode is the pixel clock output > >physically by the connector. You want to use it for an intermediate > >clock in your pipeline. > > > >This is not the same clock, and not the same frequency. > > So should a multiplier parameter be passed via > tcon1_mode_set function? No, and I've explained a couple of times already what you needed to do. Please read again the previous messages. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Attachment:
signature.asc
Description: PGP signature