| Message ID | 20170604160149.30230-8-icenowy@aosc.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@aosc.io> > --- > 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.
在 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@aosc.io> >> --- >> 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.) > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sun, Jun 11, 2017 at 02:43:42PM +0800, icenowy@aosc.io 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@aosc.io> > > > --- > > > 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. Maxime
于 2017年6月13日 GMT+08:00 下午3:44:32, Maxime Ripard <maxime.ripard@free-electrons.com> 写到: >On Sun, Jun 11, 2017 at 02:43:42PM +0800, icenowy@aosc.io 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@aosc.io> >> > > --- >> > > 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? > >Maxime
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@free-electrons.com> 写到: > >On Sun, Jun 11, 2017 at 02:43:42PM +0800, icenowy@aosc.io 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@aosc.io> > >> > > --- > >> > > 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
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; + } + sun4i_tcon1_mode_set(tcon, mode); sun4i_tcon_set_mux(tcon, 1, encoder); @@ -579,6 +593,10 @@ static int sun4i_tv_bind(struct device *dev, struct device *master, tv->drv = drv; dev_set_drvdata(dev, tv); + tv->quirks = of_device_get_match_data(dev); + if (!tv->quirks) + return -EINVAL; + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); regs = devm_ioremap_resource(dev, res); if (IS_ERR(regs)) { @@ -684,8 +702,23 @@ static int sun4i_tv_remove(struct platform_device *pdev) return 0; } +static const struct sun4i_tv_quirks sun4i_a10_tv_quirks = { + /* Nothing special */ +}; + +static const struct sun4i_tv_quirks sun8i_h3_tv_quirks = { + .fixed_divider = 16, +}; + static const struct of_device_id sun4i_tv_of_table[] = { - { .compatible = "allwinner,sun4i-a10-tv-encoder" }, + { + .compatible = "allwinner,sun4i-a10-tv-encoder", + .data = &sun4i_a10_tv_quirks, + }, + { + .compatible = "allwinner,sun8i-h3-tv-encoder", + .data = &sun8i_h3_tv_quirks, + }, { } }; MODULE_DEVICE_TABLE(of, sun4i_tv_of_table);
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@aosc.io> --- Changes in v2: - Quirk part rewritten. drivers/gpu/drm/sun4i/sun4i_tv.c | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)