diff mbox

[v2,04/11] drm: sun4i: add support for H3's TCON0/1

Message ID 20170604160149.30230-5-icenowy@aosc.io (mailing list archive)
State Not Applicable
Headers show

Commit Message

Icenowy Zheng June 4, 2017, 4:01 p.m. UTC 
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(-)

Comments

Jernej Škrabec June 4, 2017, 6:46 p.m. UTC | #1 
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.

>  };
>  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)".

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.

Best regards,
Jernej


--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Icenowy Zheng June 4, 2017, 7:03 p.m. UTC | #2 
于 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
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
kernel test robot June 4, 2017, 10:43 p.m. UTC | #3 
Hi Icenowy,

[auto build test ERROR on next-20170602]
[cannot apply to mripard/sunxi/for-next robh/for-next clk/clk-next v4.9-rc8 v4.9-rc7 v4.9-rc6 v4.12-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Icenowy-Zheng/Support-for-H3-Composite-Output-support/20170605-003002
config: arm-multi_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/sun4i/sun4i-tcon: struct of_device_id is 196 bytes.  The last of 6 is:
   0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x61 0x6c 0x6c 0x77 0x69 0x6e 0x6e 0x65 0x72 0x2c 0x73 0x75 0x6e 0x38 0x69 0x2d 0x68 0x33 0x2d 0x74 0x63 0x6f 0x6e 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0xa0 0x04 0x00 0x00 
>> FATAL: drivers/gpu/drm/sun4i/sun4i-tcon: struct of_device_id is not terminated with a NULL entry!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Maxime Ripard June 7, 2017, 9:43 a.m. UTC | #4 
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
Icenowy Zheng June 7, 2017, 9:44 a.m. UTC | #5 
于 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
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Icenowy Zheng June 7, 2017, 10 a.m. UTC | #6 
于 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
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Ripard June 7, 2017, 2:19 p.m. UTC | #7 
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
Icenowy Zheng June 7, 2017, 2:21 p.m. UTC | #8 
于 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
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Ripard June 9, 2017, 2:48 p.m. UTC | #9 
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 mbox

Patch

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;