[PATCH v2 0/2] soundwire: set frame shape and divider based on actual clk

[PATCH v2 0/2] soundwire: set frame shape and divider based on actual clk

[Bard Liao]

Set frame shape and clock divider based on actual clock frequency. And
with that change, we can support ckock change in Intel platforms.

v2:
 - Mave the check before writing the divider registers
 - Remove the 'freq' intermediate variable which is multiplied by two
   after divided by two.

Bard Liao (2):
  soundwire: cadence_master: set frame shape and divider based on actual
    clk freq
  Revert "soundwire: intel_auxdevice: start the bus at default
    frequency"

 drivers/soundwire/cadence_master.c  | 22 +++++++++++++++++++---
 drivers/soundwire/intel_auxdevice.c | 21 ---------------------
 2 files changed, 19 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] soundwire: cadence_master: set frame shape and divider based on actual clk freq

[Bard Liao]

Frame shape and curr_dr_freq could be updated by sdw_compute_bus_params().
Peripherals will set curr_dr_freq as their frequency. Managers
should do the same. Then update frame shape according to the actual
bus frequency.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 drivers/soundwire/cadence_master.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index f367670ea991..68be8ff3f02b 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -1341,7 +1341,7 @@ static u32 cdns_set_initial_frame_shape(int n_rows, int n_cols)
 	return val;
 }
 
-static void cdns_init_clock_ctrl(struct sdw_cdns *cdns)
+static int cdns_init_clock_ctrl(struct sdw_cdns *cdns)
 {
 	struct sdw_bus *bus = &cdns->bus;
 	struct sdw_master_prop *prop = &bus->prop;
@@ -1355,14 +1355,25 @@ static void cdns_init_clock_ctrl(struct sdw_cdns *cdns)
 		prop->default_row,
 		prop->default_col);
 
+	if (!prop->default_frame_rate || !prop->default_row) {
+		dev_err(cdns->dev, "Default frame_rate %d or row %d is invalid\n",
+			prop->default_frame_rate, prop->default_row);
+		return -EINVAL;
+	}
+
 	/* Set clock divider */
-	divider	= (prop->mclk_freq / prop->max_clk_freq) - 1;
+	divider	= (prop->mclk_freq * SDW_DOUBLE_RATE_FACTOR /
+		bus->params.curr_dr_freq) - 1;
 
 	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);
 
+	/* Set frame shape base on the actual bus frequency. */
+	prop->default_col = bus->params.curr_dr_freq /
+			    prop->default_frame_rate / prop->default_row;
+
 	/*
 	 * Frame shape changes after initialization have to be done
 	 * with the bank switch mechanism
@@ -1375,6 +1386,8 @@ static void cdns_init_clock_ctrl(struct sdw_cdns *cdns)
 	ssp_interval = prop->default_frame_rate / SDW_CADENCE_GSYNC_HZ;
 	cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, ssp_interval);
 	cdns_writel(cdns, CDNS_MCP_SSP_CTRL1, ssp_interval);
+
+	return 0;
 }
 
 /**
@@ -1408,9 +1421,12 @@ EXPORT_SYMBOL(sdw_cdns_soft_reset);
  */
 int sdw_cdns_init(struct sdw_cdns *cdns)
 {
+	int ret;
 	u32 val;
 
-	cdns_init_clock_ctrl(cdns);
+	ret = cdns_init_clock_ctrl(cdns);
+	if (ret)
+		return ret;
 
 	sdw_cdns_check_self_clearing_bits(cdns, __func__, false, 0);
 
-- 
2.43.0

[PATCH v2 2/2] Revert "soundwire: intel_auxdevice: start the bus at default frequency"

[Bard Liao]

Now, we can support more than 1 soundwire bus clock frequency.

This reverts commit c326356188f1dc2d7a2c55b30dac6a8b76087bc6.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 drivers/soundwire/intel_auxdevice.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/soundwire/intel_auxdevice.c b/drivers/soundwire/intel_auxdevice.c
index 599954d92752..dee126f6d9d5 100644
--- a/drivers/soundwire/intel_auxdevice.c
+++ b/drivers/soundwire/intel_auxdevice.c
@@ -222,30 +222,9 @@ static int sdw_master_read_intel_prop(struct sdw_bus *bus)
 
 static int intel_prop_read(struct sdw_bus *bus)
 {
-	struct sdw_master_prop *prop;
-
 	/* Initialize with default handler to read all DisCo properties */
 	sdw_master_read_prop(bus);
 
-	/*
-	 * Only one bus frequency is supported so far, filter
-	 * frequencies reported in the DSDT
-	 */
-	prop = &bus->prop;
-	if (prop->clk_freq && prop->num_clk_freq > 1) {
-		unsigned int default_bus_frequency;
-
-		default_bus_frequency =
-			prop->default_frame_rate *
-			prop->default_row *
-			prop->default_col /
-			SDW_DOUBLE_RATE_FACTOR;
-
-		prop->num_clk_freq = 1;
-		prop->clk_freq[0] = default_bus_frequency;
-		prop->max_clk_freq = default_bus_frequency;
-	}
-
 	/* read Intel-specific properties */
 	sdw_master_read_intel_prop(bus);
 
-- 
2.43.0

Re: [PATCH v2 0/2] soundwire: set frame shape and divider based on actual clk

[Pierre-Louis Bossart]

On 1/14/25 12:16 AM, Bard Liao wrote:
> Set frame shape and clock divider based on actual clock frequency. And
> with that change, we can support ckock change in Intel platforms.
> 
> v2:
>  - Mave the check before writing the divider registers
>  - Remove the 'freq' intermediate variable which is multiplied by two
>    after divided by two.
> 
> Bard Liao (2):
>   soundwire: cadence_master: set frame shape and divider based on actual
>     clk freq
>   Revert "soundwire: intel_auxdevice: start the bus at default
>     frequency"

This looks fine so 

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>

but one additional question: what is the default start frequency? There is an expectation in SoundWire 1.2 that the bus starts at the slowest speed to allow for potential programming of PHY registers before bumping the speed to a higher rate. I don't recall what is typically listed in the _DSD properties, it'd be a mistake to start blindly with the first listed value if it happens to be the max rate.

>  drivers/soundwire/cadence_master.c  | 22 +++++++++++++++++++---
>  drivers/soundwire/intel_auxdevice.c | 21 ---------------------
>  2 files changed, 19 insertions(+), 24 deletions(-)
> 

RE: [PATCH v2 0/2] soundwire: set frame shape and divider based on actual clk

[Liao, Bard]

> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>
> Sent: Saturday, January 18, 2025 12:11 AM
> To: Bard Liao <yung-chuan.liao@linux.intel.com>; linux-sound@vger.kernel.org;
> vkoul@kernel.org
> Cc: vinod.koul@linaro.org; linux-kernel@vger.kernel.org; Liao, Bard
> <bard.liao@intel.com>
> Subject: Re: [PATCH v2 0/2] soundwire: set frame shape and divider based on
> actual clk
> 
> On 1/14/25 12:16 AM, Bard Liao wrote:
> > Set frame shape and clock divider based on actual clock frequency. And
> > with that change, we can support ckock change in Intel platforms.
> >
> > v2:
> >  - Mave the check before writing the divider registers
> >  - Remove the 'freq' intermediate variable which is multiplied by two
> >    after divided by two.
> >
> > Bard Liao (2):
> >   soundwire: cadence_master: set frame shape and divider based on actual
> >     clk freq
> >   Revert "soundwire: intel_auxdevice: start the bus at default
> >     frequency"
> 
> This looks fine so
> 
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>
> 
> but one additional question: what is the default start frequency? There is an
> expectation in SoundWire 1.2 that the bus starts at the slowest speed to allow
> for potential programming of PHY registers before bumping the speed to a
> higher rate. I don't recall what is typically listed in the _DSD properties, it'd be a
> mistake to start blindly with the first listed value if it happens to be the max
> rate.

Currently, we start with the first listed value. I checked the listed
values, and the first value is the slowest and the latter is faster.
But, yes, I agree that we can't rely on the _DSD properties.
Let's sort it when we get the values.

> 
> >  drivers/soundwire/cadence_master.c  | 22 +++++++++++++++++++---
> >  drivers/soundwire/intel_auxdevice.c | 21 ---------------------
> >  2 files changed, 19 insertions(+), 24 deletions(-)
> >