Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Tue, 26 Sep 2023 17:25:54 +0530
From:   Dhruva Gole <d-gole@ti.com>
To:     Vaishnav Achath <vaishnav.a@ti.com>
CC:     <broonie@kernel.org>, <linux-spi@vger.kernel.org>,
        <vigneshr@ti.com>, <linux-kernel@vger.kernel.org>,
        <u-kumar1@ti.com>
Subject: Re: [PATCH v2] spi: omap2-mcspi: Fix hardcoded reference clock
Message-ID: <20230926115554.4hpcgnu6wn4uurpy@dhruva>
References: <20230926113812.30692-1-vaishnav.a@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230926113812.30692-1-vaishnav.a@ti.com>
Precedence: bulk

On Sep 26, 2023 at 17:08:12 +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>
> ---
> 
> V1->V2:
>  * Use u32 instead of unsigned int
>  * rename clk to ref_clk and rebase for latest struct ctlr rename changes.
> 
> Blamed commit is the first device where the default reference clock was
> different from the hardcoded value.
> Tested on TDA4VM SK (6.6.0-rc3-next-20230926)
> https://gist.github.com/vaishnavachath/7bbc6dd6e02294aed3e8c547cfa039c2

Superb, the logic analyzer snapshot in the end makes it so much cooler
hehe :)

> 
> V1 - https://lore.kernel.org/all/20230912100328.31813-1-vaishnav.a@ti.com/
> 
>  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 79888e6c54c2..ddf1c684bcc7 100644
> --- a/drivers/spi/spi-omap2-mcspi.c
> +++ b/drivers/spi/spi-omap2-mcspi.c
> @@ -125,10 +125,12 @@ struct omap2_mcspi {
>  	struct omap2_mcspi_dma	*dma_channels;
>  	struct device		*dev;
>  	struct omap2_mcspi_regs ctx;
> +	struct clk		*ref_clk;
>  	int			fifo_depth;
>  	bool			target_aborted;
>  	unsigned int		pin_dir:1;
>  	size_t			max_xfer_len;
> +	u32			ref_clk_hz;

Looks good now.

>  };
>  
>  struct omap2_mcspi_cs {
> @@ -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;

Looks good.

>  		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)
>  	ctlr->cleanup = omap2_mcspi_cleanup;
>  	ctlr->target_abort = omap2_mcspi_target_abort;
>  	ctlr->dev.of_node = node;
> -	ctlr->max_speed_hz = OMAP2_MCSPI_MAX_FREQ;
> -	ctlr->min_speed_hz = OMAP2_MCSPI_MAX_FREQ >> 15;
>  	ctlr->use_gpio_descriptors = true;
>  
>  	platform_set_drvdata(pdev, ctlr);
> @@ -1519,6 +1520,14 @@ static int omap2_mcspi_probe(struct platform_device *pdev)
>  		goto free_ctlr;
>  	}
>  
> +	mcspi->ref_clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
> +	if (mcspi->ref_clk)
> +		mcspi->ref_clk_hz = clk_get_rate(mcspi->ref_clk);
> +	else
> +		mcspi->ref_clk_hz = OMAP2_MCSPI_MAX_FREQ;
> +	ctlr->max_speed_hz = mcspi->ref_clk_hz;
> +	ctlr->min_speed_hz = mcspi->ref_clk_hz >> 15;

nit: Can we improve on previous code by making 15 into a macro?

> +
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_AUTOSUSPEND_TIMEOUT);
>  	pm_runtime_enable(&pdev->dev);
> -- 
> 2.17.1
> 

Thanks for addressing comments,
Reviewed-by: Dhruva Gole <d-gole@ti.com>

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