[PATCHv2,06/22] drm/bridge: tc358767: cleanup aux_link_setup

Message ID 20190326103146.24795-7-tomi.valkeinen@ti.com
State Superseded
Headers show
Series
  • drm/bridge: tc358767: DP support
Related show

Commit Message

Tomi Valkeinen March 26, 2019, 10:31 a.m.
Modify aux_link_setup so that it does not use tc->link, and thus makes
aux setup independent of the link probing.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Andrzej Hajda April 15, 2019, 7:38 a.m. | #1
On 26.03.2019 11:31, Tomi Valkeinen wrote:
> Modify aux_link_setup so that it does not use tc->link, and thus makes
> aux setup independent of the link probing.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 7ef8d754b4ff..f5c232a9064e 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -542,7 +542,6 @@ static int tc_aux_link_setup(struct tc_data *tc)
>  	unsigned long rate;
>  	u32 value;
>  	int ret;
> -	u32 dp_phy_ctrl;
>  
>  	rate = clk_get_rate(tc->refclk);
>  	switch (rate) {
> @@ -567,10 +566,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
>  	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
>  	tc_write(SYS_PLLPARAM, value);
>  
> -	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN;
> -	if (tc->link.base.num_lanes == 2)
> -		dp_phy_ctrl |= PHY_2LANE;
> -	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
> +	tc_write(DP_PHY_CTRL, BGREN | PWR_SW_EN | PHY_A0_EN);


Description does not explain why removal of PHY_2LANE is OK.

I guess because it is set anyway before training but that needs lurking
into the code.

With description fixed:

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej


>  
>  	/*
>  	 * Initially PLLs are in bypass. Force PLL parameter update,
> @@ -587,8 +583,9 @@ static int tc_aux_link_setup(struct tc_data *tc)
>  	if (ret == -ETIMEDOUT) {
>  		dev_err(tc->dev, "Timeout waiting for PHY to become ready");
>  		return ret;
> -	} else if (ret)
> +	} else if (ret) {
>  		goto err;
> +	}
>  
>  	/* Setup AUX link */
>  	tc_write(DP0_AUXCFG1, AUX_RX_FILTER_EN |
Tomi Valkeinen April 15, 2019, 7:52 a.m. | #2
On 15/04/2019 10:38, Andrzej Hajda wrote:
> On 26.03.2019 11:31, Tomi Valkeinen wrote:
>> Modify aux_link_setup so that it does not use tc->link, and thus makes
>> aux setup independent of the link probing.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  drivers/gpu/drm/bridge/tc358767.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index 7ef8d754b4ff..f5c232a9064e 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -542,7 +542,6 @@ static int tc_aux_link_setup(struct tc_data *tc)
>>  	unsigned long rate;
>>  	u32 value;
>>  	int ret;
>> -	u32 dp_phy_ctrl;
>>  
>>  	rate = clk_get_rate(tc->refclk);
>>  	switch (rate) {
>> @@ -567,10 +566,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
>>  	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
>>  	tc_write(SYS_PLLPARAM, value);
>>  
>> -	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN;
>> -	if (tc->link.base.num_lanes == 2)
>> -		dp_phy_ctrl |= PHY_2LANE;
>> -	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
>> +	tc_write(DP_PHY_CTRL, BGREN | PWR_SW_EN | PHY_A0_EN);
> 
> 
> Description does not explain why removal of PHY_2LANE is OK.
> 
> I guess because it is set anyway before training but that needs lurking
> into the code.

Good point, the desc is a bit unclear. The reason is that we're setting
up only AUX here, so anything related to the data-lanes is don't-care.

 Tomi
Laurent Pinchart April 20, 2019, 9:20 p.m. | #3
Hi Tomi,

Thank you for the patch.

On Mon, Apr 15, 2019 at 10:52:38AM +0300, Tomi Valkeinen wrote:
> On 15/04/2019 10:38, Andrzej Hajda wrote:
> > On 26.03.2019 11:31, Tomi Valkeinen wrote:
> >> Modify aux_link_setup so that it does not use tc->link, and thus makes
> >> aux setup independent of the link probing.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> ---
> >>  drivers/gpu/drm/bridge/tc358767.c | 9 +++------
> >>  1 file changed, 3 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> >> index 7ef8d754b4ff..f5c232a9064e 100644
> >> --- a/drivers/gpu/drm/bridge/tc358767.c
> >> +++ b/drivers/gpu/drm/bridge/tc358767.c
> >> @@ -542,7 +542,6 @@ static int tc_aux_link_setup(struct tc_data *tc)
> >>  	unsigned long rate;
> >>  	u32 value;
> >>  	int ret;
> >> -	u32 dp_phy_ctrl;
> >>  
> >>  	rate = clk_get_rate(tc->refclk);
> >>  	switch (rate) {
> >> @@ -567,10 +566,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
> >>  	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
> >>  	tc_write(SYS_PLLPARAM, value);
> >>  
> >> -	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN;
> >> -	if (tc->link.base.num_lanes == 2)
> >> -		dp_phy_ctrl |= PHY_2LANE;
> >> -	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
> >> +	tc_write(DP_PHY_CTRL, BGREN | PWR_SW_EN | PHY_A0_EN);
> > 
> > 
> > Description does not explain why removal of PHY_2LANE is OK.
> > 
> > I guess because it is set anyway before training but that needs lurking
> > into the code.
> 
> Good point, the desc is a bit unclear. The reason is that we're setting
> up only AUX here, so anything related to the data-lanes is don't-care.

And also because the tc_aux_link_setup() function is called at probe
time, while the tc_main_link_setup() function is called later and will
overwrite the DP_PHY_CTRL register with bits related to the main link.

With the description fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Patch

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 7ef8d754b4ff..f5c232a9064e 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -542,7 +542,6 @@  static int tc_aux_link_setup(struct tc_data *tc)
 	unsigned long rate;
 	u32 value;
 	int ret;
-	u32 dp_phy_ctrl;
 
 	rate = clk_get_rate(tc->refclk);
 	switch (rate) {
@@ -567,10 +566,7 @@  static int tc_aux_link_setup(struct tc_data *tc)
 	value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
 	tc_write(SYS_PLLPARAM, value);
 
-	dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN;
-	if (tc->link.base.num_lanes == 2)
-		dp_phy_ctrl |= PHY_2LANE;
-	tc_write(DP_PHY_CTRL, dp_phy_ctrl);
+	tc_write(DP_PHY_CTRL, BGREN | PWR_SW_EN | PHY_A0_EN);
 
 	/*
 	 * Initially PLLs are in bypass. Force PLL parameter update,
@@ -587,8 +583,9 @@  static int tc_aux_link_setup(struct tc_data *tc)
 	if (ret == -ETIMEDOUT) {
 		dev_err(tc->dev, "Timeout waiting for PHY to become ready");
 		return ret;
-	} else if (ret)
+	} else if (ret) {
 		goto err;
+	}
 
 	/* Setup AUX link */
 	tc_write(DP0_AUXCFG1, AUX_RX_FILTER_EN |