Re: [PATCH v2 20/24] spi: zx: Add ZTE ZX family SPI flash controller driver

["Stefan Dösinger" <stefandoesinger@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2145 bytes --]

Hi Navid,

I had some time to review the changes you made to my SPI driver.

I'll grab the code style changes, they seem in line with kernel guidelines. Of 
course it'll only live in my gitlab repo for the time being.

Am Mittwoch, 14. Oktober 2026, 09:21:00 Mitteleuropäische Sommerzeit schrieb 
Navid Ghahremani:
> +static int zx_qspi_set_speed(struct zx_qspi *qspi, unsigned int max_hz)
> +{
> +	unsigned long rate;
> +	long rounded;
> +	int ret;
> +
> +	if (!max_hz)
> +		return -EINVAL;
> +	if (qspi->speed_hz == max_hz)
> +		return 0;
> +
> +	rounded = clk_round_rate(qspi->wclk, max_hz);
> +	if (rounded < 0)
> +		return rounded;
> +	if (!rounded || rounded > max_hz)
> +		return -EINVAL;
> +
> +	ret = clk_set_rate(qspi->wclk, rounded);
> +	if (ret)
> +		return ret;
> +	rate = clk_get_rate(qspi->wclk);
> +	if (!rate || rate > max_hz)
> +		return -EINVAL;
> +	qspi->speed_hz = rate;
> +
> +	return 0;
> +}

I think the idea of op->max_freq is that it is the SPI bus clock, not the 
controller clock. Existing drivers aren't quite consistent on it though (or 
there's HW without a divider register). spi-ma35d1-qspi.c for example sets the 
controller's clock divider rather than the clock.

So IOW, this function should primarily configure ZX_QSPI_TIMING_REG. If the 
target clock can't be achieved with the controller input clock (e.g. divider 
is larger than ZX_QSPI_TIMING_REG supports or clock rate is lower than 
requested SPI speed), then a change to the wclk is in order. 

> +	ctrl->min_speed_hz = clk_round_rate(qspi->wclk, 1);
> +	ctrl->max_speed_hz = 50000000;

This looks wrong - the max speed is probably the highest possible wclk rate, 
the lowest speed the lowest possible wclk rate divided by the highest possible 
divider.

> +	/* Ensure proper data sampling point */
> +	reg = readl_relaxed(qspi->io_base + ZX_QSPI_TIMING_REG);
> +	if (!FIELD_GET(GENMASK(17, 16), reg)) {
> +		reg |= FIELD_PREP(GENMASK(17, 16), 1);
> +		writel_relaxed(reg, qspi->io_base + ZX_QSPI_TIMING_REG);
> +	}

I don't know what this is doing. Can you elaborate?

Cheers,
Stefan

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 870 bytes --]