All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] spi: omap2-mcspi: Fix hardcoded reference clock
@ 2023-09-12 10:03 Vaishnav Achath
  2023-09-12 13:40 ` Dhruva Gole
  0 siblings, 1 reply; 2+ messages in thread
From: Vaishnav Achath @ 2023-09-12 10:03 UTC (permalink / raw)
  To: broonie, linux-spi
  Cc: t-kristo, d-gole, vigneshr, linux-kernel, vaishnav.a, u-kumar1

A hardcoded reference clock of 48 MHz is used to calculate the
clock divisor values, but the reference clock frequency can be
different across devices and can be configured which can cause
a mismatch between the reported frequency and actual SPI clock
frequency observed. Fix this by fetching the clock rate from
the clock provider and falling back to hardcoded reference only
if the clock is not supplied.

Fixes: 2cd7d393f461 ("arm64: dts: ti: k3-am654: Add McSPI DT nodes")

Signed-off-by: Vaishnav Achath <vaishnav.a@ti.com>
---

Blamed commit is the first device where the default reference clock was
different from the hardcoded value.
Tested on TDA4VM SK (6.6.0-rc1-next-20230911)

 drivers/spi/spi-omap2-mcspi.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index e5cd82eb9e54..6ec45fd0e904 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -125,8 +125,10 @@ struct omap2_mcspi {
 	struct omap2_mcspi_dma	*dma_channels;
 	struct device		*dev;
 	struct omap2_mcspi_regs ctx;
+	struct clk		*clk;
 	int			fifo_depth;
 	bool			slave_aborted;
+	unsigned int		ref_clk_hz;
 	unsigned int		pin_dir:1;
 	size_t			max_xfer_len;
 };
@@ -880,12 +882,12 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct spi_transfer *xfer)
 	return count - c;
 }
 
-static u32 omap2_mcspi_calc_divisor(u32 speed_hz)
+static u32 omap2_mcspi_calc_divisor(u32 speed_hz, u32 ref_clk_hz)
 {
 	u32 div;
 
 	for (div = 0; div < 15; div++)
-		if (speed_hz >= (OMAP2_MCSPI_MAX_FREQ >> div))
+		if (speed_hz >= (ref_clk_hz >> div))
 			return div;
 
 	return 15;
@@ -897,7 +899,7 @@ static int omap2_mcspi_setup_transfer(struct spi_device *spi,
 {
 	struct omap2_mcspi_cs *cs = spi->controller_state;
 	struct omap2_mcspi *mcspi;
-	u32 l = 0, clkd = 0, div, extclk = 0, clkg = 0;
+	u32 ref_clk_hz, l = 0, clkd = 0, div, extclk = 0, clkg = 0;
 	u8 word_len = spi->bits_per_word;
 	u32 speed_hz = spi->max_speed_hz;
 
@@ -911,14 +913,15 @@ static int omap2_mcspi_setup_transfer(struct spi_device *spi,
 	if (t && t->speed_hz)
 		speed_hz = t->speed_hz;
 
-	speed_hz = min_t(u32, speed_hz, OMAP2_MCSPI_MAX_FREQ);
-	if (speed_hz < (OMAP2_MCSPI_MAX_FREQ / OMAP2_MCSPI_MAX_DIVIDER)) {
-		clkd = omap2_mcspi_calc_divisor(speed_hz);
-		speed_hz = OMAP2_MCSPI_MAX_FREQ >> clkd;
+	ref_clk_hz = mcspi->ref_clk_hz;
+	speed_hz = min_t(u32, speed_hz, ref_clk_hz);
+	if (speed_hz < (ref_clk_hz / OMAP2_MCSPI_MAX_DIVIDER)) {
+		clkd = omap2_mcspi_calc_divisor(speed_hz, ref_clk_hz);
+		speed_hz = ref_clk_hz >> clkd;
 		clkg = 0;
 	} else {
-		div = (OMAP2_MCSPI_MAX_FREQ + speed_hz - 1) / speed_hz;
-		speed_hz = OMAP2_MCSPI_MAX_FREQ / div;
+		div = (ref_clk_hz + speed_hz - 1) / speed_hz;
+		speed_hz = ref_clk_hz / div;
 		clkd = (div - 1) & 0xf;
 		extclk = (div - 1) >> 4;
 		clkg = OMAP2_MCSPI_CHCONF_CLKG;
@@ -1448,8 +1451,6 @@ static int omap2_mcspi_probe(struct platform_device *pdev)
 	master->cleanup = omap2_mcspi_cleanup;
 	master->slave_abort = omap2_mcspi_slave_abort;
 	master->dev.of_node = node;
-	master->max_speed_hz = OMAP2_MCSPI_MAX_FREQ;
-	master->min_speed_hz = OMAP2_MCSPI_MAX_FREQ >> 15;
 	master->use_gpio_descriptors = true;
 
 	platform_set_drvdata(pdev, master);
@@ -1519,6 +1520,14 @@ static int omap2_mcspi_probe(struct platform_device *pdev)
 		goto free_master;
 	}
 
+	mcspi->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
+	if (mcspi->clk)
+		mcspi->ref_clk_hz = clk_get_rate(mcspi->clk);
+	else
+		mcspi->ref_clk_hz = OMAP2_MCSPI_MAX_FREQ;
+	master->max_speed_hz = mcspi->ref_clk_hz;
+	master->min_speed_hz = mcspi->ref_clk_hz >> 15;
+
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_AUTOSUSPEND_TIMEOUT);
 	pm_runtime_enable(&pdev->dev);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH] spi: omap2-mcspi: Fix hardcoded reference clock
  2023-09-12 10:03 [PATCH] spi: omap2-mcspi: Fix hardcoded reference clock Vaishnav Achath
@ 2023-09-12 13:40 ` Dhruva Gole
  0 siblings, 0 replies; 2+ messages in thread
From: Dhruva Gole @ 2023-09-12 13:40 UTC (permalink / raw)
  To: Vaishnav Achath
  Cc: broonie, linux-spi, t-kristo, vigneshr, linux-kernel, u-kumar1

On Sep 12, 2023 at 15:33:28 +0530, Vaishnav Achath wrote:
> A hardcoded reference clock of 48 MHz is used to calculate the
> clock divisor values, but the reference clock frequency can be
> different across devices and can be configured which can cause
> a mismatch between the reported frequency and actual SPI clock
> frequency observed. Fix this by fetching the clock rate from
> the clock provider and falling back to hardcoded reference only
> if the clock is not supplied.
> 
> Fixes: 2cd7d393f461 ("arm64: dts: ti: k3-am654: Add McSPI DT nodes")
> 
> Signed-off-by: Vaishnav Achath <vaishnav.a@ti.com>
> ---
> 
> Blamed commit is the first device where the default reference clock was
> different from the hardcoded value.
> Tested on TDA4VM SK (6.6.0-rc1-next-20230911)

Would be good to have some logs too next time, to share how it was tested.

> 
>  drivers/spi/spi-omap2-mcspi.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
> index e5cd82eb9e54..6ec45fd0e904 100644
> --- a/drivers/spi/spi-omap2-mcspi.c
> +++ b/drivers/spi/spi-omap2-mcspi.c
> @@ -125,8 +125,10 @@ struct omap2_mcspi {
>  	struct omap2_mcspi_dma	*dma_channels;
>  	struct device		*dev;
>  	struct omap2_mcspi_regs ctx;
> +	struct clk		*clk;

I dislike naming an obj after it's struct, can we come up with some
other name?

>  	int			fifo_depth;
>  	bool			slave_aborted;
> +	unsigned int		ref_clk_hz;

Do you want to make it u32 here? or...

>  	unsigned int		pin_dir:1;
>  	size_t			max_xfer_len;
>  };
> @@ -880,12 +882,12 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct spi_transfer *xfer)
>  	return count - c;
>  }
>  
> -static u32 omap2_mcspi_calc_divisor(u32 speed_hz)
> +static u32 omap2_mcspi_calc_divisor(u32 speed_hz, u32 ref_clk_hz)

unsigned int ref_clk_hz here?

>  {
>  	u32 div;
>  
>  	for (div = 0; div < 15; div++)
> -		if (speed_hz >= (OMAP2_MCSPI_MAX_FREQ >> div))
> +		if (speed_hz >= (ref_clk_hz >> div))
>  			return div;
>  
>  	return 15;
> @@ -897,7 +899,7 @@ static int omap2_mcspi_setup_transfer(struct spi_device *spi,
>  {
>  	struct omap2_mcspi_cs *cs = spi->controller_state;
>  	struct omap2_mcspi *mcspi;
> -	u32 l = 0, clkd = 0, div, extclk = 0, clkg = 0;
> +	u32 ref_clk_hz, l = 0, clkd = 0, div, extclk = 0, clkg = 0;

here as well, your ref_clk_hz is u32 but below you're making it mcspi->ref_clk_hz
which is an unsigned int?

I know it's not a deal breaker, however not a big fan of mixing fixed
types and types that the compiler may decide.

>  	u8 word_len = spi->bits_per_word;
>  	u32 speed_hz = spi->max_speed_hz;
>  
> @@ -911,14 +913,15 @@ static int omap2_mcspi_setup_transfer(struct spi_device *spi,
>  	if (t && t->speed_hz)
>  		speed_hz = t->speed_hz;
>  
> -	speed_hz = min_t(u32, speed_hz, OMAP2_MCSPI_MAX_FREQ);
> -	if (speed_hz < (OMAP2_MCSPI_MAX_FREQ / OMAP2_MCSPI_MAX_DIVIDER)) {
> -		clkd = omap2_mcspi_calc_divisor(speed_hz);
> -		speed_hz = OMAP2_MCSPI_MAX_FREQ >> clkd;
> +	ref_clk_hz = mcspi->ref_clk_hz;
> +	speed_hz = min_t(u32, speed_hz, ref_clk_hz);
> +	if (speed_hz < (ref_clk_hz / OMAP2_MCSPI_MAX_DIVIDER)) {
> +		clkd = omap2_mcspi_calc_divisor(speed_hz, ref_clk_hz);
> +		speed_hz = ref_clk_hz >> clkd;
>  		clkg = 0;
>  	} else {
> -		div = (OMAP2_MCSPI_MAX_FREQ + speed_hz - 1) / speed_hz;
> -		speed_hz = OMAP2_MCSPI_MAX_FREQ / div;
> +		div = (ref_clk_hz + speed_hz - 1) / speed_hz;
> +		speed_hz = ref_clk_hz / div;
>  		clkd = (div - 1) & 0xf;
>  		extclk = (div - 1) >> 4;
>  		clkg = OMAP2_MCSPI_CHCONF_CLKG;
> @@ -1448,8 +1451,6 @@ static int omap2_mcspi_probe(struct platform_device *pdev)
>  	master->cleanup = omap2_mcspi_cleanup;
>  	master->slave_abort = omap2_mcspi_slave_abort;
>  	master->dev.of_node = node;
> -	master->max_speed_hz = OMAP2_MCSPI_MAX_FREQ;
> -	master->min_speed_hz = OMAP2_MCSPI_MAX_FREQ >> 15;
>  	master->use_gpio_descriptors = true;
>  
>  	platform_set_drvdata(pdev, master);
> @@ -1519,6 +1520,14 @@ static int omap2_mcspi_probe(struct platform_device *pdev)
>  		goto free_master;
>  	}
>  
> +	mcspi->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
> +	if (mcspi->clk)
> +		mcspi->ref_clk_hz = clk_get_rate(mcspi->clk);
> +	else
> +		mcspi->ref_clk_hz = OMAP2_MCSPI_MAX_FREQ;
> +	master->max_speed_hz = mcspi->ref_clk_hz;
> +	master->min_speed_hz = mcspi->ref_clk_hz >> 15;
> +
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_AUTOSUSPEND_TIMEOUT);
>  	pm_runtime_enable(&pdev->dev);
> -- 
> 2.17.1
> 

-- 
Best regards,
Dhruva Gole <d-gole@ti.com>

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-09-12 13:40 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-12 10:03 [PATCH] spi: omap2-mcspi: Fix hardcoded reference clock Vaishnav Achath
2023-09-12 13:40 ` Dhruva Gole

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.