[06/17] soundwire: cadence_master: use firmware defaults for frame shape
diff mbox series

Message ID 20190806005522.22642-7-pierre-louis.bossart@linux.intel.com
State New
Headers show
Series
  • soundwire: fixes for 5.4
Related show

Commit Message

Pierre-Louis Bossart Aug. 6, 2019, 12:55 a.m. UTC
Remove hard-coding and use firmware (BIOS/DT) values. If they are
wrong use default 48x2 frame shape.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/cadence_master.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

Comments

Cezary Rojewski Aug. 6, 2019, 3:27 p.m. UTC | #1
On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index 5d9729b4d634..89c55e4bb72c 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -48,6 +48,8 @@
>   #define CDNS_MCP_SSPSTAT			0xC
>   #define CDNS_MCP_FRAME_SHAPE			0x10
>   #define CDNS_MCP_FRAME_SHAPE_INIT		0x14
> +#define CDNS_MCP_FRAME_SHAPE_COL_MASK		GENMASK(2, 0)
> +#define CDNS_MCP_FRAME_SHAPE_ROW_OFFSET		3
>   
>   #define CDNS_MCP_CONFIG_UPDATE			0x18
>   #define CDNS_MCP_CONFIG_UPDATE_BIT		BIT(0)
> @@ -175,7 +177,6 @@
>   /* Driver defaults */
>   
>   #define CDNS_DEFAULT_CLK_DIVIDER		0
> -#define CDNS_DEFAULT_FRAME_SHAPE		0x30
>   #define CDNS_DEFAULT_SSP_INTERVAL		0x18
>   #define CDNS_TX_TIMEOUT				2000
>   
> @@ -901,6 +902,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
>   }
>   EXPORT_SYMBOL(sdw_cdns_pdi_init);
>   
> +static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols)
> +{
> +	u32 val;
> +	int c;
> +	int r;
> +
> +	r = sdw_find_row_index(n_rows);
> +	c = sdw_find_col_index(n_cols) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
> +
> +	val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
> +
> +	return val;
> +}
> +

Guess this have been said already, but this function could be simplified 
- unless you really want to keep explicit variable declaration. Both "c" 
and "r" declarations could be merged into single line while "val" is not 
needed at all.

One more thing - is AND bitwise op really needed for cols explicitly? We 
know all col values upfront - these are static and declared in global 
table nearby. Static declaration takes care of "initial range-check". Is 
another one necessary?

Moreover, this is a _get_ and certainly not a _set_ type of function. 
I'd even consider renaming it to: "cdns_get_frame_shape" as this is 
neither a _set_ nor an explicit initial frame shape setter.

It might be even helpful to split two usages:

#define sdw_frame_shape(col_idx, row_idx) \
	((row_idx << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | col_idx)

u32 cdns_get_frame_shape(u16 rows, u16 cols)
{
	u16 c, r;

	r = sdw_find_row_index(rows);
	c = sdw_find_col_index(cols);

	return sdw_frame_shape(c, r);
}

The above may even be simplified into one-liner.
Pierre-Louis Bossart Aug. 6, 2019, 3:36 p.m. UTC | #2
On 8/6/19 10:27 AM, Cezary Rojewski wrote:
> On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
>> diff --git a/drivers/soundwire/cadence_master.c 
>> b/drivers/soundwire/cadence_master.c
>> index 5d9729b4d634..89c55e4bb72c 100644
>> --- a/drivers/soundwire/cadence_master.c
>> +++ b/drivers/soundwire/cadence_master.c
>> @@ -48,6 +48,8 @@
>>   #define CDNS_MCP_SSPSTAT            0xC
>>   #define CDNS_MCP_FRAME_SHAPE            0x10
>>   #define CDNS_MCP_FRAME_SHAPE_INIT        0x14
>> +#define CDNS_MCP_FRAME_SHAPE_COL_MASK        GENMASK(2, 0)
>> +#define CDNS_MCP_FRAME_SHAPE_ROW_OFFSET        3
>>   #define CDNS_MCP_CONFIG_UPDATE            0x18
>>   #define CDNS_MCP_CONFIG_UPDATE_BIT        BIT(0)
>> @@ -175,7 +177,6 @@
>>   /* Driver defaults */
>>   #define CDNS_DEFAULT_CLK_DIVIDER        0
>> -#define CDNS_DEFAULT_FRAME_SHAPE        0x30
>>   #define CDNS_DEFAULT_SSP_INTERVAL        0x18
>>   #define CDNS_TX_TIMEOUT                2000
>> @@ -901,6 +902,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
>>   }
>>   EXPORT_SYMBOL(sdw_cdns_pdi_init);
>> +static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols)
>> +{
>> +    u32 val;
>> +    int c;
>> +    int r;
>> +
>> +    r = sdw_find_row_index(n_rows);
>> +    c = sdw_find_col_index(n_cols) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
>> +
>> +    val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
>> +
>> +    return val;
>> +}
>> +
> 
> Guess this have been said already, but this function could be simplified 
> - unless you really want to keep explicit variable declaration. Both "c" 
> and "r" declarations could be merged into single line while "val" is not 
> needed at all.
> 
> One more thing - is AND bitwise op really needed for cols explicitly? We 
> know all col values upfront - these are static and declared in global 
> table nearby. Static declaration takes care of "initial range-check". Is 
> another one necessary?
> 
> Moreover, this is a _get_ and certainly not a _set_ type of function. 
> I'd even consider renaming it to: "cdns_get_frame_shape" as this is 
> neither a _set_ nor an explicit initial frame shape setter.
> 
> It might be even helpful to split two usages:
> 
> #define sdw_frame_shape(col_idx, row_idx) \
>      ((row_idx << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | col_idx)
> 
> u32 cdns_get_frame_shape(u16 rows, u16 cols)
> {
>      u16 c, r;
> 
>      r = sdw_find_row_index(rows);
>      c = sdw_find_col_index(cols);
> 
>      return sdw_frame_shape(c, r);
> }
> 
> The above may even be simplified into one-liner.

This is a function used once on startup, there is no real need to 
simplify further. The separate variables help add debug traces as needed 
and keep the code readable while showing how the values are encoded into 
a register.
Cezary Rojewski Aug. 6, 2019, 4:06 p.m. UTC | #3
On 2019-08-06 17:36, Pierre-Louis Bossart wrote:
> 
> 
> On 8/6/19 10:27 AM, Cezary Rojewski wrote:
>> On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
>>> diff --git a/drivers/soundwire/cadence_master.c 
>>> b/drivers/soundwire/cadence_master.c
>>> index 5d9729b4d634..89c55e4bb72c 100644
>>> --- a/drivers/soundwire/cadence_master.c
>>> +++ b/drivers/soundwire/cadence_master.c
>>> @@ -48,6 +48,8 @@
>>>   #define CDNS_MCP_SSPSTAT            0xC
>>>   #define CDNS_MCP_FRAME_SHAPE            0x10
>>>   #define CDNS_MCP_FRAME_SHAPE_INIT        0x14
>>> +#define CDNS_MCP_FRAME_SHAPE_COL_MASK        GENMASK(2, 0)
>>> +#define CDNS_MCP_FRAME_SHAPE_ROW_OFFSET        3
>>>   #define CDNS_MCP_CONFIG_UPDATE            0x18
>>>   #define CDNS_MCP_CONFIG_UPDATE_BIT        BIT(0)
>>> @@ -175,7 +177,6 @@
>>>   /* Driver defaults */
>>>   #define CDNS_DEFAULT_CLK_DIVIDER        0
>>> -#define CDNS_DEFAULT_FRAME_SHAPE        0x30
>>>   #define CDNS_DEFAULT_SSP_INTERVAL        0x18
>>>   #define CDNS_TX_TIMEOUT                2000
>>> @@ -901,6 +902,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
>>>   }
>>>   EXPORT_SYMBOL(sdw_cdns_pdi_init);
>>> +static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols)
>>> +{
>>> +    u32 val;
>>> +    int c;
>>> +    int r;
>>> +
>>> +    r = sdw_find_row_index(n_rows);
>>> +    c = sdw_find_col_index(n_cols) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
>>> +
>>> +    val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
>>> +
>>> +    return val;
>>> +}
>>> +
>>
>> Guess this have been said already, but this function could be 
>> simplified - unless you really want to keep explicit variable 
>> declaration. Both "c" and "r" declarations could be merged into single 
>> line while "val" is not needed at all.
>>
>> One more thing - is AND bitwise op really needed for cols explicitly? 
>> We know all col values upfront - these are static and declared in 
>> global table nearby. Static declaration takes care of "initial 
>> range-check". Is another one necessary?
>>
>> Moreover, this is a _get_ and certainly not a _set_ type of function. 
>> I'd even consider renaming it to: "cdns_get_frame_shape" as this is 
>> neither a _set_ nor an explicit initial frame shape setter.
>>
>> It might be even helpful to split two usages:
>>
>> #define sdw_frame_shape(col_idx, row_idx) \
>>      ((row_idx << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | col_idx)
>>
>> u32 cdns_get_frame_shape(u16 rows, u16 cols)
>> {
>>      u16 c, r;
>>
>>      r = sdw_find_row_index(rows);
>>      c = sdw_find_col_index(cols);
>>
>>      return sdw_frame_shape(c, r);
>> }
>>
>> The above may even be simplified into one-liner.
> 
> This is a function used once on startup, there is no real need to 
> simplify further. The separate variables help add debug traces as needed 
> and keep the code readable while showing how the values are encoded into 
> a register.

Eh, I've thought it's gonna be exposed to userspace (via uapi) so it can 
be fetched by tests or tools.

In such case - if there is a single usage only - guess function is fine 
as is.
Vinod Koul Aug. 14, 2019, 4:31 a.m. UTC | #4
On 06-08-19, 18:06, Cezary Rojewski wrote:
> On 2019-08-06 17:36, Pierre-Louis Bossart wrote:
> > 
> > 
> > On 8/6/19 10:27 AM, Cezary Rojewski wrote:
> > > On 2019-08-06 02:55, Pierre-Louis Bossart wrote:
> > > > diff --git a/drivers/soundwire/cadence_master.c
> > > > b/drivers/soundwire/cadence_master.c
> > > > index 5d9729b4d634..89c55e4bb72c 100644
> > > > --- a/drivers/soundwire/cadence_master.c
> > > > +++ b/drivers/soundwire/cadence_master.c
> > > > @@ -48,6 +48,8 @@
> > > >   #define CDNS_MCP_SSPSTAT            0xC
> > > >   #define CDNS_MCP_FRAME_SHAPE            0x10
> > > >   #define CDNS_MCP_FRAME_SHAPE_INIT        0x14
> > > > +#define CDNS_MCP_FRAME_SHAPE_COL_MASK        GENMASK(2, 0)
> > > > +#define CDNS_MCP_FRAME_SHAPE_ROW_OFFSET        3
> > > >   #define CDNS_MCP_CONFIG_UPDATE            0x18
> > > >   #define CDNS_MCP_CONFIG_UPDATE_BIT        BIT(0)
> > > > @@ -175,7 +177,6 @@
> > > >   /* Driver defaults */
> > > >   #define CDNS_DEFAULT_CLK_DIVIDER        0
> > > > -#define CDNS_DEFAULT_FRAME_SHAPE        0x30
> > > >   #define CDNS_DEFAULT_SSP_INTERVAL        0x18
> > > >   #define CDNS_TX_TIMEOUT                2000
> > > > @@ -901,6 +902,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
> > > >   }
> > > >   EXPORT_SYMBOL(sdw_cdns_pdi_init);
> > > > +static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols)
> > > > +{
> > > > +    u32 val;
> > > > +    int c;
> > > > +    int r;
> > > > +
> > > > +    r = sdw_find_row_index(n_rows);
> > > > +    c = sdw_find_col_index(n_cols) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
> > > > +
> > > > +    val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
> > > > +
> > > > +    return val;
> > > > +}
> > > > +
> > > 
> > > Guess this have been said already, but this function could be
> > > simplified - unless you really want to keep explicit variable
> > > declaration. Both "c" and "r" declarations could be merged into
> > > single line while "val" is not needed at all.
> > > 
> > > One more thing - is AND bitwise op really needed for cols
> > > explicitly? We know all col values upfront - these are static and
> > > declared in global table nearby. Static declaration takes care of
> > > "initial range-check". Is another one necessary?
> > > 
> > > Moreover, this is a _get_ and certainly not a _set_ type of
> > > function. I'd even consider renaming it to: "cdns_get_frame_shape"
> > > as this is neither a _set_ nor an explicit initial frame shape
> > > setter.
> > > 
> > > It might be even helpful to split two usages:
> > > 
> > > #define sdw_frame_shape(col_idx, row_idx) \
> > >      ((row_idx << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | col_idx)
> > > 
> > > u32 cdns_get_frame_shape(u16 rows, u16 cols)
> > > {
> > >      u16 c, r;
> > > 
> > >      r = sdw_find_row_index(rows);
> > >      c = sdw_find_col_index(cols);
> > > 
> > >      return sdw_frame_shape(c, r);
> > > }
> > > 
> > > The above may even be simplified into one-liner.
> > 
> > This is a function used once on startup, there is no real need to
> > simplify further. The separate variables help add debug traces as needed
> > and keep the code readable while showing how the values are encoded into
> > a register.
> 
> Eh, I've thought it's gonna be exposed to userspace (via uapi) so it can be
> fetched by tests or tools.

Uapi? I dont see anything in this or other series posted, did I miss
something? Also I am not sure I like the idea of exposing these to
userland!

> 
> In such case - if there is a single usage only - guess function is fine as
> is.
Pierre-Louis Bossart Aug. 14, 2019, 2:03 p.m. UTC | #5
>>>>> +static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols)
>>>>> +{
>>>>> +    u32 val;
>>>>> +    int c;
>>>>> +    int r;
>>>>> +
>>>>> +    r = sdw_find_row_index(n_rows);
>>>>> +    c = sdw_find_col_index(n_cols) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
>>>>> +
>>>>> +    val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
>>>>> +
>>>>> +    return val;
>>>>> +}
>>>>> +
>>>>
>>>> Guess this have been said already, but this function could be
>>>> simplified - unless you really want to keep explicit variable
>>>> declaration. Both "c" and "r" declarations could be merged into
>>>> single line while "val" is not needed at all.
>>>>
>>>> One more thing - is AND bitwise op really needed for cols
>>>> explicitly? We know all col values upfront - these are static and
>>>> declared in global table nearby. Static declaration takes care of
>>>> "initial range-check". Is another one necessary?
>>>>
>>>> Moreover, this is a _get_ and certainly not a _set_ type of
>>>> function. I'd even consider renaming it to: "cdns_get_frame_shape"
>>>> as this is neither a _set_ nor an explicit initial frame shape
>>>> setter.
>>>>
>>>> It might be even helpful to split two usages:
>>>>
>>>> #define sdw_frame_shape(col_idx, row_idx) \
>>>>       ((row_idx << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | col_idx)
>>>>
>>>> u32 cdns_get_frame_shape(u16 rows, u16 cols)
>>>> {
>>>>       u16 c, r;
>>>>
>>>>       r = sdw_find_row_index(rows);
>>>>       c = sdw_find_col_index(cols);
>>>>
>>>>       return sdw_frame_shape(c, r);
>>>> }
>>>>
>>>> The above may even be simplified into one-liner.
>>>
>>> This is a function used once on startup, there is no real need to
>>> simplify further. The separate variables help add debug traces as needed
>>> and keep the code readable while showing how the values are encoded into
>>> a register.
>>
>> Eh, I've thought it's gonna be exposed to userspace (via uapi) so it can be
>> fetched by tests or tools.
> 
> Uapi? I dont see anything in this or other series posted, did I miss
> something? Also I am not sure I like the idea of exposing these to
> userland!

Vinod, that was never the intent, and Cezary agreed, see following line

> 
>>
>> In such case - if there is a single usage only - guess function is fine as
>> is.
>

Patch
diff mbox series

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index 5d9729b4d634..89c55e4bb72c 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -48,6 +48,8 @@ 
 #define CDNS_MCP_SSPSTAT			0xC
 #define CDNS_MCP_FRAME_SHAPE			0x10
 #define CDNS_MCP_FRAME_SHAPE_INIT		0x14
+#define CDNS_MCP_FRAME_SHAPE_COL_MASK		GENMASK(2, 0)
+#define CDNS_MCP_FRAME_SHAPE_ROW_OFFSET		3
 
 #define CDNS_MCP_CONFIG_UPDATE			0x18
 #define CDNS_MCP_CONFIG_UPDATE_BIT		BIT(0)
@@ -175,7 +177,6 @@ 
 /* Driver defaults */
 
 #define CDNS_DEFAULT_CLK_DIVIDER		0
-#define CDNS_DEFAULT_FRAME_SHAPE		0x30
 #define CDNS_DEFAULT_SSP_INTERVAL		0x18
 #define CDNS_TX_TIMEOUT				2000
 
@@ -901,6 +902,20 @@  int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
 }
 EXPORT_SYMBOL(sdw_cdns_pdi_init);
 
+static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols)
+{
+	u32 val;
+	int c;
+	int r;
+
+	r = sdw_find_row_index(n_rows);
+	c = sdw_find_col_index(n_cols) & CDNS_MCP_FRAME_SHAPE_COL_MASK;
+
+	val = (r << CDNS_MCP_FRAME_SHAPE_ROW_OFFSET) | c;
+
+	return val;
+}
+
 /**
  * sdw_cdns_init() - Cadence initialization
  * @cdns: Cadence instance
@@ -923,8 +938,13 @@  int sdw_cdns_init(struct sdw_cdns *cdns)
 	val |= CDNS_DEFAULT_CLK_DIVIDER;
 	cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
 
-	/* Set the default frame shape */
-	cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, CDNS_DEFAULT_FRAME_SHAPE);
+	/*
+	 * Frame shape changes after initialization have to be done
+	 * with the bank switch mechanism
+	 */
+	val = cdns_set_initial_frame_shape(prop->default_row,
+					   prop->default_col);
+	cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, val);
 
 	/* Set SSP interval to default value */
 	cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, CDNS_DEFAULT_SSP_INTERVAL);