| Message ID | 20170604160149.30230-5-icenowy@aosc.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
于 2017年6月5日 GMT+08:00 上午2:46:24, "Jernej Škrabec" <jernej.skrabec@siol.net> 写到: >Hi, > >Dne nedelja, 04. junij 2017 ob 18:01:42 CEST je Icenowy Zheng >napisal(a): >> From: Icenowy Zheng <icenowy@aosc.xyz> >> >> Allwinner H3 has two special TCONs, both come without channel0. And >the >> TCON1 of H3 has no special clocks even for the channel1. >> >> Add support for these kinds of TCON. >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> --- >> Changes in v2: >> - Merged TCON0 and TCON1 quirks and compatibles. >> >> drivers/gpu/drm/sun4i/sun4i_tcon.c | 52 >> +++++++++++++++++++++++++------------- >drivers/gpu/drm/sun4i/sun4i_tcon.h | >> 1 + >> 2 files changed, 36 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c >> b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 568cea0e5f8f..62ba4fc19f18 >> 100644 >> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c >> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c >> @@ -59,6 +59,7 @@ void sun4i_tcon_channel_disable(struct sun4i_tcon >*tcon, >> int channel) >> >> /* Disable the TCON's channel */ >> if (channel == 0) { >> + WARN_ON(!tcon->quirks->has_channel_0); >> regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG, >> SUN4I_TCON0_CTL_TCON_ENABLE, 0); >> clk_disable_unprepare(tcon->dclk); >> @@ -78,6 +79,7 @@ void sun4i_tcon_channel_enable(struct sun4i_tcon >*tcon, >> int channel) >> >> /* Enable the TCON's channel */ >> if (channel == 0) { >> + WARN_ON(!tcon->quirks->has_channel_0); >> regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG, >> SUN4I_TCON0_CTL_TCON_ENABLE, >> SUN4I_TCON0_CTL_TCON_ENABLE); >> @@ -159,6 +161,7 @@ void sun4i_tcon0_mode_set(struct sun4i_tcon >*tcon, >> >> /* Configure the dot clock */ >> clk_set_rate(tcon->dclk, mode->crtc_clock * 1000); >> + WARN_ON(!tcon->quirks->has_channel_0); >> >> /* Adjust clock delay */ >> clk_delay = sun4i_tcon_get_clk_delay(mode, 0); >> @@ -366,10 +369,12 @@ static int sun4i_tcon_init_clocks(struct device >*dev, >> } >> clk_prepare_enable(tcon->clk); >> >> - tcon->sclk0 = devm_clk_get(dev, "tcon-ch0"); >> - if (IS_ERR(tcon->sclk0)) { >> - dev_err(dev, "Couldn't get the TCON channel 0 clock\n"); >> - return PTR_ERR(tcon->sclk0); >> + if (tcon->quirks->has_channel_0) { >> + tcon->sclk0 = devm_clk_get(dev, "tcon-ch0"); >> + if (IS_ERR(tcon->sclk0)) { >> + dev_err(dev, "Couldn't get the TCON channel 0 clock\n"); >> + return PTR_ERR(tcon->sclk0); >> + } >> } >> >> if (tcon->quirks->has_channel_1) { >> @@ -594,10 +599,12 @@ static int sun4i_tcon_bind(struct device *dev, >struct >> device *master, goto err_free_clocks; >> } >> >> - ret = sun4i_dclk_create(dev, tcon); >> - if (ret) { >> - dev_err(dev, "Couldn't create our TCON dot clock\n"); >> - goto err_free_clocks; >> + if (tcon->quirks->has_channel_0) { >> + ret = sun4i_dclk_create(dev, tcon); >> + if (ret) { >> + dev_err(dev, "Couldn't create our TCON dot clock\n"); >> + goto err_free_clocks; >> + } >> } >> >> ret = sun4i_tcon_init_irq(dev, tcon); >> @@ -622,7 +629,8 @@ static int sun4i_tcon_bind(struct device *dev, >struct >> device *master, return 0; >> >> err_free_dotclock: >> - sun4i_dclk_free(tcon); >> + if (tcon->quirks->has_channel_0) >> + sun4i_dclk_free(tcon); >> err_free_clocks: >> sun4i_tcon_free_clocks(tcon); >> err_assert_reset: >> @@ -636,7 +644,9 @@ static void sun4i_tcon_unbind(struct device *dev, >struct >> device *master, struct sun4i_tcon *tcon = dev_get_drvdata(dev); >> >> list_del(&tcon->list); >> - sun4i_dclk_free(tcon); >> + >> + if (tcon->quirks->has_channel_0) >> + sun4i_dclk_free(tcon); >> sun4i_tcon_free_clocks(tcon); >> } >> >> @@ -667,24 +677,32 @@ static int sun4i_tcon_remove(struct >platform_device >> *pdev) } >> >> static const struct sun4i_tcon_quirks sun5i_a13_quirks = { >> - .has_unknown_mux = true, >> - .has_channel_1 = true, >> + .has_unknown_mux = true, >> + .has_channel_0 = true, >> + .has_channel_1 = true, >> }; >> >> static const struct sun4i_tcon_quirks sun6i_a31_quirks = { >> - .has_channel_1 = true, >> + .has_channel_0 = true, >> + .has_channel_1 = true, >> }; >> >> static const struct sun4i_tcon_quirks sun6i_a31s_quirks = { >> - .has_channel_1 = true, >> + .has_channel_0 = true, >> + .has_channel_1 = true, >> }; >> >> static const struct sun4i_tcon_quirks sun8i_a33_quirks = { >> - /* nothing is supported */ >> + .has_channel_0 = true, >> }; >> >> static const struct sun4i_tcon_quirks sun8i_v3s_quirks = { >> - /* nothing is supported */ >> + .has_channel_0 = true, >> +}; >> + >> +static const struct sun4i_tcon_quirks sun8i_h3_quirks = { >> + .has_channel_1 = true, >> + .swappable_input = true, >> }; >> >> static const struct of_device_id sun4i_tcon_of_table[] = { >> @@ -693,7 +711,7 @@ static const struct of_device_id >sun4i_tcon_of_table[] = >> { { .compatible = "allwinner,sun6i-a31s-tcon", .data = >&sun6i_a31s_quirks >> }, { .compatible = "allwinner,sun8i-a33-tcon", .data = >&sun8i_a33_quirks }, >> { .compatible = "allwinner,sun8i-v3s-tcon", .data = &sun8i_v3s_quirks >}, >> - { } >> + { .compatible = "allwinner,sun8i-h3-tcon", .data = &sun8i_h3_quirks >}, > >I think you need to leave empty entry as a sentinel. Sorry. > >> }; >> MODULE_DEVICE_TABLE(of, sun4i_tcon_of_table); >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h >> b/drivers/gpu/drm/sun4i/sun4i_tcon.h index c3e01c06e9a0..9c706a0bd478 >> 100644 >> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h >> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h >> @@ -145,6 +145,7 @@ >> >> struct sun4i_tcon_quirks { >> bool has_unknown_mux; /* sun5i has undocumented mux */ >> + bool has_channel_0; /* some A83T+ TCONs don't have channel 0*/ >> bool has_channel_1; /* a33 does not have channel 1 */ >> /* Some DE2 can swap the mixer<->TCON connection */ >> bool swappable_input; >> -- >> 2.12.2 > >You missed sun4i_rgb_init() function which should also be guarded with >"if >(tcon->quirks->has_channel_0)". > Yes... >You should also expand function sun4i_drv_node_is_tcon() at sun4i_drv.c >with >new entries, but I'm not sure if this fits in this patch. Instead I think it should be renamed to something like "sun4i_drv_node_is_tcon_with_ch0". > >Best regards, >Jernej
On Mon, Jun 05, 2017 at 03:03:47AM +0800, Icenowy Zheng wrote: > >You should also expand function sun4i_drv_node_is_tcon() at sun4i_drv.c > >with > >new entries, but I'm not sure if this fits in this patch. > > Instead I think it should be renamed to something like > "sun4i_drv_node_is_tcon_with_ch0". I'm not sure, or at least, it shouldn't make any difference, since TCON without a channel 0 will not have an endpoint 0, so this will be dealt with already. Maxime
于 2017年6月7日 GMT+08:00 下午5:43:43, Maxime Ripard <maxime.ripard@free-electrons.com> 写到: >On Mon, Jun 05, 2017 at 03:03:47AM +0800, Icenowy Zheng wrote: >> >You should also expand function sun4i_drv_node_is_tcon() at >sun4i_drv.c >> >with >> >new entries, but I'm not sure if this fits in this patch. >> >> Instead I think it should be renamed to something like >> "sun4i_drv_node_is_tcon_with_ch0". > >I'm not sure, or at least, it shouldn't make any difference, since >TCON without a channel 0 will not have an endpoint 0, so this will be >dealt with already. But that will prevent new coders from add CH1-less TCON compatibles to this function. > >Maxime
于 2017年6月7日 GMT+08:00 下午5:43:43, Maxime Ripard <maxime.ripard@free-electrons.com> 写到: >On Mon, Jun 05, 2017 at 03:03:47AM +0800, Icenowy Zheng wrote: >> >You should also expand function sun4i_drv_node_is_tcon() at >sun4i_drv.c >> >with >> >new entries, but I'm not sure if this fits in this patch. >> >> Instead I think it should be renamed to something like >> "sun4i_drv_node_is_tcon_with_ch0". > >I'm not sure, or at least, it shouldn't make any difference, since >TCON without a channel 0 will not have an endpoint 0, so this will be >dealt with already. But that will prevent new coders from add CH1-less TCON compatibles to this function. > >Maxime
On Wed, Jun 07, 2017 at 05:44:56PM +0800, Icenowy Zheng wrote: > 于 2017年6月7日 GMT+08:00 下午5:43:43, Maxime Ripard <maxime.ripard@free-electrons.com> 写到: > >On Mon, Jun 05, 2017 at 03:03:47AM +0800, Icenowy Zheng wrote: > >> >You should also expand function sun4i_drv_node_is_tcon() at > >sun4i_drv.c > >> >with > >> >new entries, but I'm not sure if this fits in this patch. > >> > >> Instead I think it should be renamed to something like > >> "sun4i_drv_node_is_tcon_with_ch0". > > > >I'm not sure, or at least, it shouldn't make any difference, since > >TCON without a channel 0 will not have an endpoint 0, so this will be > >dealt with already. > > But that will prevent new coders from add CH1-less TCON > compatibles to this function. Why? We already have such TCONs (like the A33's, or V3S') in that function. Maxime
于 2017年6月7日 GMT+08:00 下午10:19:57, Maxime Ripard <maxime.ripard@free-electrons.com> 写到: >On Wed, Jun 07, 2017 at 05:44:56PM +0800, Icenowy Zheng wrote: >> 于 2017年6月7日 GMT+08:00 下午5:43:43, Maxime Ripard ><maxime.ripard@free-electrons.com> 写到: >> >On Mon, Jun 05, 2017 at 03:03:47AM +0800, Icenowy Zheng wrote: >> >> >You should also expand function sun4i_drv_node_is_tcon() at >> >sun4i_drv.c >> >> >with >> >> >new entries, but I'm not sure if this fits in this patch. >> >> >> >> Instead I think it should be renamed to something like >> >> "sun4i_drv_node_is_tcon_with_ch0". >> > >> >I'm not sure, or at least, it shouldn't make any difference, since >> >TCON without a channel 0 will not have an endpoint 0, so this will >be >> >dealt with already. >> >> But that will prevent new coders from add CH1-less TCON >> compatibles to this function. > >Why? We already have such TCONs (like the A33's, or V3S') in that >function. Sorry, CH0-less. > >Maxime
On Wed, Jun 07, 2017 at 10:21:02PM +0800, Icenowy Zheng wrote: > > > 于 2017年6月7日 GMT+08:00 下午10:19:57, Maxime Ripard <maxime.ripard@free-electrons.com> 写到: > >On Wed, Jun 07, 2017 at 05:44:56PM +0800, Icenowy Zheng wrote: > >> 于 2017年6月7日 GMT+08:00 下午5:43:43, Maxime Ripard > ><maxime.ripard@free-electrons.com> 写到: > >> >On Mon, Jun 05, 2017 at 03:03:47AM +0800, Icenowy Zheng wrote: > >> >> >You should also expand function sun4i_drv_node_is_tcon() at > >> >sun4i_drv.c > >> >> >with > >> >> >new entries, but I'm not sure if this fits in this patch. > >> >> > >> >> Instead I think it should be renamed to something like > >> >> "sun4i_drv_node_is_tcon_with_ch0". > >> > > >> >I'm not sure, or at least, it shouldn't make any difference, since > >> >TCON without a channel 0 will not have an endpoint 0, so this will > >be > >> >dealt with already. > >> > >> But that will prevent new coders from add CH1-less TCON > >> compatibles to this function. > > > >Why? We already have such TCONs (like the A33's, or V3S') in that > >function. > > Sorry, CH0-less. That's not really an issue I think, since the endpoint 0 will not be there on those TCONs, the code will bail out. Maxime
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 568cea0e5f8f..62ba4fc19f18 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -59,6 +59,7 @@ void sun4i_tcon_channel_disable(struct sun4i_tcon *tcon, int channel) /* Disable the TCON's channel */ if (channel == 0) { + WARN_ON(!tcon->quirks->has_channel_0); regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG, SUN4I_TCON0_CTL_TCON_ENABLE, 0); clk_disable_unprepare(tcon->dclk); @@ -78,6 +79,7 @@ void sun4i_tcon_channel_enable(struct sun4i_tcon *tcon, int channel) /* Enable the TCON's channel */ if (channel == 0) { + WARN_ON(!tcon->quirks->has_channel_0); regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG, SUN4I_TCON0_CTL_TCON_ENABLE, SUN4I_TCON0_CTL_TCON_ENABLE); @@ -159,6 +161,7 @@ void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon, /* Configure the dot clock */ clk_set_rate(tcon->dclk, mode->crtc_clock * 1000); + WARN_ON(!tcon->quirks->has_channel_0); /* Adjust clock delay */ clk_delay = sun4i_tcon_get_clk_delay(mode, 0); @@ -366,10 +369,12 @@ static int sun4i_tcon_init_clocks(struct device *dev, } clk_prepare_enable(tcon->clk); - tcon->sclk0 = devm_clk_get(dev, "tcon-ch0"); - if (IS_ERR(tcon->sclk0)) { - dev_err(dev, "Couldn't get the TCON channel 0 clock\n"); - return PTR_ERR(tcon->sclk0); + if (tcon->quirks->has_channel_0) { + tcon->sclk0 = devm_clk_get(dev, "tcon-ch0"); + if (IS_ERR(tcon->sclk0)) { + dev_err(dev, "Couldn't get the TCON channel 0 clock\n"); + return PTR_ERR(tcon->sclk0); + } } if (tcon->quirks->has_channel_1) { @@ -594,10 +599,12 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master, goto err_free_clocks; } - ret = sun4i_dclk_create(dev, tcon); - if (ret) { - dev_err(dev, "Couldn't create our TCON dot clock\n"); - goto err_free_clocks; + if (tcon->quirks->has_channel_0) { + ret = sun4i_dclk_create(dev, tcon); + if (ret) { + dev_err(dev, "Couldn't create our TCON dot clock\n"); + goto err_free_clocks; + } } ret = sun4i_tcon_init_irq(dev, tcon); @@ -622,7 +629,8 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master, return 0; err_free_dotclock: - sun4i_dclk_free(tcon); + if (tcon->quirks->has_channel_0) + sun4i_dclk_free(tcon); err_free_clocks: sun4i_tcon_free_clocks(tcon); err_assert_reset: @@ -636,7 +644,9 @@ static void sun4i_tcon_unbind(struct device *dev, struct device *master, struct sun4i_tcon *tcon = dev_get_drvdata(dev); list_del(&tcon->list); - sun4i_dclk_free(tcon); + + if (tcon->quirks->has_channel_0) + sun4i_dclk_free(tcon); sun4i_tcon_free_clocks(tcon); } @@ -667,24 +677,32 @@ static int sun4i_tcon_remove(struct platform_device *pdev) } static const struct sun4i_tcon_quirks sun5i_a13_quirks = { - .has_unknown_mux = true, - .has_channel_1 = true, + .has_unknown_mux = true, + .has_channel_0 = true, + .has_channel_1 = true, }; static const struct sun4i_tcon_quirks sun6i_a31_quirks = { - .has_channel_1 = true, + .has_channel_0 = true, + .has_channel_1 = true, }; static const struct sun4i_tcon_quirks sun6i_a31s_quirks = { - .has_channel_1 = true, + .has_channel_0 = true, + .has_channel_1 = true, }; static const struct sun4i_tcon_quirks sun8i_a33_quirks = { - /* nothing is supported */ + .has_channel_0 = true, }; static const struct sun4i_tcon_quirks sun8i_v3s_quirks = { - /* nothing is supported */ + .has_channel_0 = true, +}; + +static const struct sun4i_tcon_quirks sun8i_h3_quirks = { + .has_channel_1 = true, + .swappable_input = true, }; static const struct of_device_id sun4i_tcon_of_table[] = { @@ -693,7 +711,7 @@ static const struct of_device_id sun4i_tcon_of_table[] = { { .compatible = "allwinner,sun6i-a31s-tcon", .data = &sun6i_a31s_quirks }, { .compatible = "allwinner,sun8i-a33-tcon", .data = &sun8i_a33_quirks }, { .compatible = "allwinner,sun8i-v3s-tcon", .data = &sun8i_v3s_quirks }, - { } + { .compatible = "allwinner,sun8i-h3-tcon", .data = &sun8i_h3_quirks }, }; MODULE_DEVICE_TABLE(of, sun4i_tcon_of_table); diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h index c3e01c06e9a0..9c706a0bd478 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h @@ -145,6 +145,7 @@ struct sun4i_tcon_quirks { bool has_unknown_mux; /* sun5i has undocumented mux */ + bool has_channel_0; /* some A83T+ TCONs don't have channel 0*/ bool has_channel_1; /* a33 does not have channel 1 */ /* Some DE2 can swap the mixer<->TCON connection */ bool swappable_input;