diff mbox series

[RFC,26/40] soundwire: cadence_master: fix divider setting in clock register

Message ID 20190725234032.21152-27-pierre-louis.bossart@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series soundwire: updates for 5.4 | expand

Commit Message

Pierre-Louis Bossart July 25, 2019, 11:40 p.m. UTC 
From: Rander Wang <rander.wang@linux.intel.com>

The existing code uses an OR operation which would mix the original
divider setting with the new one, resulting in an invalid
configuration that can make codecs hang.

Add the mask definition and use cdns_updatel to update divider

Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/cadence_master.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Bard Liao July 26, 2019, 5:19 a.m. UTC | #1 
On 7/26/2019 7:40 AM, Pierre-Louis Bossart wrote:
> From: Rander Wang <rander.wang@linux.intel.com>
>
> The existing code uses an OR operation which would mix the original
> divider setting with the new one, resulting in an invalid
> configuration that can make codecs hang.
>
> Add the mask definition and use cdns_updatel to update divider
>
> Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>   drivers/soundwire/cadence_master.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index 10ebcef2e84e..18c6ac026e85 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -57,6 +57,7 @@
>   #define CDNS_MCP_SSP_CTRL1			0x28
>   #define CDNS_MCP_CLK_CTRL0			0x30
>   #define CDNS_MCP_CLK_CTRL1			0x38
> +#define CDNS_MCP_CLK_MCLKD_MASK		GENMASK(7, 0)
>   
>   #define CDNS_MCP_STAT				0x40
>   
> @@ -988,9 +989,11 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
>   	/* Set clock divider */
>   	divider	= (prop->mclk_freq / prop->max_clk_freq) - 1;
>   	val = cdns_readl(cdns, CDNS_MCP_CLK_CTRL0);


Do we still need to read cdns_readl(cdns, CDNS_MCP_CLK_CTRL0)

after this change?


> -	val |= divider;
> -	cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
> -	cdns_writel(cdns, CDNS_MCP_CLK_CTRL1, val);
> +
> +	cdns_updatel(cdns, CDNS_MCP_CLK_CTRL0,
> +		     CDNS_MCP_CLK_MCLKD_MASK, divider);
> +	cdns_updatel(cdns, CDNS_MCP_CLK_CTRL1,
> +		     CDNS_MCP_CLK_MCLKD_MASK, divider);
>   
>   	pr_err("plb: mclk %d max_freq %d divider %d register %x\n",
>   	       prop->mclk_freq,
> @@ -1064,8 +1067,7 @@ int cdns_bus_conf(struct sdw_bus *bus, struct sdw_bus_params *params)
>   		mcp_clkctrl_off = CDNS_MCP_CLK_CTRL0;
>   
>   	mcp_clkctrl = cdns_readl(cdns, mcp_clkctrl_off);


Same here.


> -	mcp_clkctrl |= divider;
> -	cdns_writel(cdns, mcp_clkctrl_off, mcp_clkctrl);
> +	cdns_updatel(cdns, mcp_clkctrl_off, CDNS_MCP_CLK_MCLKD_MASK, divider);
>   
>   	pr_err("plb: mclk * 2 %d curr_dr_freq %d divider %d register %x\n",
>   	       prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR,
rander.wang July 26, 2019, 5:56 a.m. UTC | #2 
在 7/26/2019 1:19 PM, Bard liao 写道:
>
> On 7/26/2019 7:40 AM, Pierre-Louis Bossart wrote:
>> From: Rander Wang <rander.wang@linux.intel.com>
>>
>> The existing code uses an OR operation which would mix the original
>> divider setting with the new one, resulting in an invalid
>> configuration that can make codecs hang.
>>
>> Add the mask definition and use cdns_updatel to update divider
>>
>> Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
>> Signed-off-by: Pierre-Louis Bossart 
>> <pierre-louis.bossart@linux.intel.com>
>> ---
>>   drivers/soundwire/cadence_master.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soundwire/cadence_master.c 
>> b/drivers/soundwire/cadence_master.c
>> index 10ebcef2e84e..18c6ac026e85 100644
>> --- a/drivers/soundwire/cadence_master.c
>> +++ b/drivers/soundwire/cadence_master.c
>> @@ -57,6 +57,7 @@
>>   #define CDNS_MCP_SSP_CTRL1            0x28
>>   #define CDNS_MCP_CLK_CTRL0            0x30
>>   #define CDNS_MCP_CLK_CTRL1            0x38
>> +#define CDNS_MCP_CLK_MCLKD_MASK        GENMASK(7, 0)
>>     #define CDNS_MCP_STAT                0x40
>>   @@ -988,9 +989,11 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
>>       /* Set clock divider */
>>       divider    = (prop->mclk_freq / prop->max_clk_freq) - 1;
>>       val = cdns_readl(cdns, CDNS_MCP_CLK_CTRL0);
>
>
> Do we still need to read cdns_readl(cdns, CDNS_MCP_CLK_CTRL0)
>
> after this change?
>
The val is used to print debug message,  and my opinion is to change the log

from pr_err("plb: ........") to dev_dbg(bus->dev, "........") to follow 
the dev_dbg

usage in this file.

>
>> -    val |= divider;
>> -    cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
>> -    cdns_writel(cdns, CDNS_MCP_CLK_CTRL1, val);
>> +
>> +    cdns_updatel(cdns, CDNS_MCP_CLK_CTRL0,
>> +             CDNS_MCP_CLK_MCLKD_MASK, divider);
>> +    cdns_updatel(cdns, CDNS_MCP_CLK_CTRL1,
>> +             CDNS_MCP_CLK_MCLKD_MASK, divider);
>>         pr_err("plb: mclk %d max_freq %d divider %d register %x\n",
>>              prop->mclk_freq,
>> @@ -1064,8 +1067,7 @@ int cdns_bus_conf(struct sdw_bus *bus, struct 
>> sdw_bus_params *params)
>>           mcp_clkctrl_off = CDNS_MCP_CLK_CTRL0;
>>         mcp_clkctrl = cdns_readl(cdns, mcp_clkctrl_off);
>
>
> Same here.
>
Also refine the debug log.
>
>> -    mcp_clkctrl |= divider;
>> -    cdns_writel(cdns, mcp_clkctrl_off, mcp_clkctrl);
>> +    cdns_updatel(cdns, mcp_clkctrl_off, CDNS_MCP_CLK_MCLKD_MASK, 
>> divider);
>>         pr_err("plb: mclk * 2 %d curr_dr_freq %d divider %d register 
>> %x\n",
>>              prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR,
Pierre-Louis Bossart July 26, 2019, 2:24 p.m. UTC | #3 
On 7/26/19 12:19 AM, Bard liao wrote:
> 
> On 7/26/2019 7:40 AM, Pierre-Louis Bossart wrote:
>> From: Rander Wang <rander.wang@linux.intel.com>
>>
>> The existing code uses an OR operation which would mix the original
>> divider setting with the new one, resulting in an invalid
>> configuration that can make codecs hang.
>>
>> Add the mask definition and use cdns_updatel to update divider
>>
>> Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
>> Signed-off-by: Pierre-Louis Bossart 
>> <pierre-louis.bossart@linux.intel.com>
>> ---
>>   drivers/soundwire/cadence_master.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/soundwire/cadence_master.c 
>> b/drivers/soundwire/cadence_master.c
>> index 10ebcef2e84e..18c6ac026e85 100644
>> --- a/drivers/soundwire/cadence_master.c
>> +++ b/drivers/soundwire/cadence_master.c
>> @@ -57,6 +57,7 @@
>>   #define CDNS_MCP_SSP_CTRL1            0x28
>>   #define CDNS_MCP_CLK_CTRL0            0x30
>>   #define CDNS_MCP_CLK_CTRL1            0x38
>> +#define CDNS_MCP_CLK_MCLKD_MASK        GENMASK(7, 0)
>>   #define CDNS_MCP_STAT                0x40
>> @@ -988,9 +989,11 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
>>       /* Set clock divider */
>>       divider    = (prop->mclk_freq / prop->max_clk_freq) - 1;
>>       val = cdns_readl(cdns, CDNS_MCP_CLK_CTRL0);
> 
> 
> Do we still need to read cdns_readl(cdns, CDNS_MCP_CLK_CTRL0)
> 
> after this change?

no I don't think we do indeed.
> 
> 
>> -    val |= divider;
>> -    cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
>> -    cdns_writel(cdns, CDNS_MCP_CLK_CTRL1, val);
>> +
>> +    cdns_updatel(cdns, CDNS_MCP_CLK_CTRL0,
>> +             CDNS_MCP_CLK_MCLKD_MASK, divider);
>> +    cdns_updatel(cdns, CDNS_MCP_CLK_CTRL1,
>> +             CDNS_MCP_CLK_MCLKD_MASK, divider);
>>       pr_err("plb: mclk %d max_freq %d divider %d register %x\n",
>>              prop->mclk_freq,

and this log needs to go away or done in a better way, I missed this.

>> @@ -1064,8 +1067,7 @@ int cdns_bus_conf(struct sdw_bus *bus, struct 
>> sdw_bus_params *params)
>>           mcp_clkctrl_off = CDNS_MCP_CLK_CTRL0;
>>       mcp_clkctrl = cdns_readl(cdns, mcp_clkctrl_off);
> 
> 
> Same here.

yep.

> 
> 
>> -    mcp_clkctrl |= divider;
>> -    cdns_writel(cdns, mcp_clkctrl_off, mcp_clkctrl);
>> +    cdns_updatel(cdns, mcp_clkctrl_off, CDNS_MCP_CLK_MCLKD_MASK, 
>> divider);
>>       pr_err("plb: mclk * 2 %d curr_dr_freq %d divider %d register %x\n",
>>              prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR,
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Vinod Koul Aug. 2, 2019, 5:19 p.m. UTC | #4 
On 25-07-19, 18:40, Pierre-Louis Bossart wrote:
> From: Rander Wang <rander.wang@linux.intel.com>
> 
> The existing code uses an OR operation which would mix the original
> divider setting with the new one, resulting in an invalid
> configuration that can make codecs hang.

This looked fine but fails to apply, feel free to send as a fix
Pierre-Louis Bossart Aug. 2, 2019, 5:30 p.m. UTC | #5 
On 8/2/19 12:19 PM, Vinod Koul wrote:
> On 25-07-19, 18:40, Pierre-Louis Bossart wrote:
>> From: Rander Wang <rander.wang@linux.intel.com>
>>
>> The existing code uses an OR operation which would mix the original
>> divider setting with the new one, resulting in an invalid
>> configuration that can make codecs hang.
> 
> This looked fine but fails to apply, feel free to send as a fix

likely because it's on top of patch 25
Sanyog Kale Aug. 5, 2019, 10:40 a.m. UTC | #6 
On Thu, Jul 25, 2019 at 06:40:18PM -0500, Pierre-Louis Bossart wrote:
> From: Rander Wang <rander.wang@linux.intel.com>
> 
> The existing code uses an OR operation which would mix the original
> divider setting with the new one, resulting in an invalid
> configuration that can make codecs hang.
> 
> Add the mask definition and use cdns_updatel to update divider
> 
> Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/cadence_master.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index 10ebcef2e84e..18c6ac026e85 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -57,6 +57,7 @@
>  #define CDNS_MCP_SSP_CTRL1			0x28
>  #define CDNS_MCP_CLK_CTRL0			0x30
>  #define CDNS_MCP_CLK_CTRL1			0x38
> +#define CDNS_MCP_CLK_MCLKD_MASK		GENMASK(7, 0)
>  
>  #define CDNS_MCP_STAT				0x40
>  
> @@ -988,9 +989,11 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
>  	/* Set clock divider */
>  	divider	= (prop->mclk_freq / prop->max_clk_freq) - 1;
>  	val = cdns_readl(cdns, CDNS_MCP_CLK_CTRL0);

reg read of CLK_CTRL0 can be removed.

> -	val |= divider;
> -	cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
> -	cdns_writel(cdns, CDNS_MCP_CLK_CTRL1, val);
> +
> +	cdns_updatel(cdns, CDNS_MCP_CLK_CTRL0,
> +		     CDNS_MCP_CLK_MCLKD_MASK, divider);
> +	cdns_updatel(cdns, CDNS_MCP_CLK_CTRL1,
> +		     CDNS_MCP_CLK_MCLKD_MASK, divider);
>  
>  	pr_err("plb: mclk %d max_freq %d divider %d register %x\n",
>  	       prop->mclk_freq,
> @@ -1064,8 +1067,7 @@ int cdns_bus_conf(struct sdw_bus *bus, struct sdw_bus_params *params)
>  		mcp_clkctrl_off = CDNS_MCP_CLK_CTRL0;
>  
>  	mcp_clkctrl = cdns_readl(cdns, mcp_clkctrl_off);

same as above.

> -	mcp_clkctrl |= divider;
> -	cdns_writel(cdns, mcp_clkctrl_off, mcp_clkctrl);
> +	cdns_updatel(cdns, mcp_clkctrl_off, CDNS_MCP_CLK_MCLKD_MASK, divider);
>  
>  	pr_err("plb: mclk * 2 %d curr_dr_freq %d divider %d register %x\n",
>  	       prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR,
> -- 
> 2.20.1
>
Pierre-Louis Bossart Aug. 5, 2019, 3:41 p.m. UTC | #7 
>> @@ -988,9 +989,11 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
>>   	/* Set clock divider */
>>   	divider	= (prop->mclk_freq / prop->max_clk_freq) - 1;
>>   	val = cdns_readl(cdns, CDNS_MCP_CLK_CTRL0);
> 
> reg read of CLK_CTRL0 can be removed.

yes for both comments. Thanks for the review Sanyog, appreciate it.

> 
>> -	val |= divider;
>> -	cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
>> -	cdns_writel(cdns, CDNS_MCP_CLK_CTRL1, val);
>> +
>> +	cdns_updatel(cdns, CDNS_MCP_CLK_CTRL0,
>> +		     CDNS_MCP_CLK_MCLKD_MASK, divider);
>> +	cdns_updatel(cdns, CDNS_MCP_CLK_CTRL1,
>> +		     CDNS_MCP_CLK_MCLKD_MASK, divider);
>>   
>>   	pr_err("plb: mclk %d max_freq %d divider %d register %x\n",
>>   	       prop->mclk_freq,
>> @@ -1064,8 +1067,7 @@ int cdns_bus_conf(struct sdw_bus *bus, struct sdw_bus_params *params)
>>   		mcp_clkctrl_off = CDNS_MCP_CLK_CTRL0;
>>   
>>   	mcp_clkctrl = cdns_readl(cdns, mcp_clkctrl_off);
> 
> same as above.
> 
>> -	mcp_clkctrl |= divider;
>> -	cdns_writel(cdns, mcp_clkctrl_off, mcp_clkctrl);
>> +	cdns_updatel(cdns, mcp_clkctrl_off, CDNS_MCP_CLK_MCLKD_MASK, divider);
>>   
>>   	pr_err("plb: mclk * 2 %d curr_dr_freq %d divider %d register %x\n",
>>   	       prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR,
>> -- 
>> 2.20.1
>>
>
diff mbox series

Patch

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index 10ebcef2e84e..18c6ac026e85 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -57,6 +57,7 @@ 
 #define CDNS_MCP_SSP_CTRL1			0x28
 #define CDNS_MCP_CLK_CTRL0			0x30
 #define CDNS_MCP_CLK_CTRL1			0x38
+#define CDNS_MCP_CLK_MCLKD_MASK		GENMASK(7, 0)
 
 #define CDNS_MCP_STAT				0x40
 
@@ -988,9 +989,11 @@  int sdw_cdns_init(struct sdw_cdns *cdns)
 	/* Set clock divider */
 	divider	= (prop->mclk_freq / prop->max_clk_freq) - 1;
 	val = cdns_readl(cdns, CDNS_MCP_CLK_CTRL0);
-	val |= divider;
-	cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
-	cdns_writel(cdns, CDNS_MCP_CLK_CTRL1, val);
+
+	cdns_updatel(cdns, CDNS_MCP_CLK_CTRL0,
+		     CDNS_MCP_CLK_MCLKD_MASK, divider);
+	cdns_updatel(cdns, CDNS_MCP_CLK_CTRL1,
+		     CDNS_MCP_CLK_MCLKD_MASK, divider);
 
 	pr_err("plb: mclk %d max_freq %d divider %d register %x\n",
 	       prop->mclk_freq,
@@ -1064,8 +1067,7 @@  int cdns_bus_conf(struct sdw_bus *bus, struct sdw_bus_params *params)
 		mcp_clkctrl_off = CDNS_MCP_CLK_CTRL0;
 
 	mcp_clkctrl = cdns_readl(cdns, mcp_clkctrl_off);
-	mcp_clkctrl |= divider;
-	cdns_writel(cdns, mcp_clkctrl_off, mcp_clkctrl);
+	cdns_updatel(cdns, mcp_clkctrl_off, CDNS_MCP_CLK_MCLKD_MASK, divider);
 
 	pr_err("plb: mclk * 2 %d curr_dr_freq %d divider %d register %x\n",
 	       prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR,