| Message ID | 20230926113812.30692-1-vaishnav.a@ti.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2d9f4877988f64f0f336983de65c365b6a7debfb |
| Headers | show |
| Series | [v2] spi: omap2-mcspi: Fix hardcoded reference clock | expand |
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>
On Tue, Sep 26, 2023 at 05:25:54PM +0530, Dhruva Gole wrote: > On Sep 26, 2023 at 17:08:12 +0530, Vaishnav Achath wrote: > > + 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? I'll queue this for CI now, please send an incremental fix for this style issue with the constant.
On Tue, 26 Sep 2023 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. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: omap2-mcspi: Fix hardcoded reference clock commit: 2d9f4877988f64f0f336983de65c365b6a7debfb All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
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; }; 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; 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; + pm_runtime_use_autosuspend(&pdev->dev); pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_AUTOSUSPEND_TIMEOUT); pm_runtime_enable(&pdev->dev);
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 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(-)