Re: [PATCH v8 03/18] spi: parse spi-max-post-config-frequency-hz into post_config_max_speed_hz

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] `spi->post_config_max_speed_hz` is not clamped against the controller's maximum supported speed (`spi->controller->max_speed_hz`).
- [Medium] `spi_new_ancillary_device()` does not copy `post_config_max_speed_hz` from the primary SPI device, breaking the feature for secondary chip selects.
--

--- Patch [3]: [PATCH v8 03/18] spi: parse spi-max-post-config-frequency-hz into post_config_max_speed_hz ---
commit c2f1be0c2debf6f375fd36183b8ae8c58dccdc03
Author: Santhosh Kumar K <[email protected]>

spi: parse spi-max-post-config-frequency-hz into post_config_max_speed_hz

Add post_config_max_speed_hz to struct spi_device and parse it from
the spi-max-post-config-frequency-hz DT property. This supports SPI
devices that operate at two distinct clock rates.

> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 5b5b3bc5f0d8c..6b01ddb7cc324 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2800,6 +2800,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
>  	/* Device speed */
>  	if (!of_property_read_u32(nc, "spi-max-frequency", &value))
>  		spi->max_speed_hz = value;
> +	if (!of_property_read_u32(nc, "spi-max-post-config-frequency-hz", &value))
> +		spi->post_config_max_speed_hz = value;

[Severity: Medium]
Does this new property also need to be clamped to the controller's maximum
supported speed?

In drivers/spi/spi.c:__spi_setup(), the pre-configuration speed is clamped:

__spi_setup() {
    ...
    if (spi->controller->max_speed_hz &&
        (!spi->max_speed_hz ||
         spi->max_speed_hz > spi->controller->max_speed_hz))
        spi->max_speed_hz = spi->controller->max_speed_hz;
    ...
}

Since post_config_max_speed_hz is not clamped here, could SPI-MEM request
an invalid post-configuration speed that the SPI controller physically
cannot support?

[ ... ]

> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 88d17fce02dcb..02fd602f29e8b 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h

[ ... ]

> @@ -195,6 +197,7 @@ struct spi_device {
>  	struct device		dev;
>  	struct spi_controller	*controller;
>  	u32			max_speed_hz;
> +	u32			post_config_max_speed_hz;

[Severity: Medium]
Should this new field be copied to ancillary devices?

When creating secondary chip selects for multi-die packages,
drivers/spi/spi.c:spi_new_ancillary_device() clones the main device properties:

spi_new_ancillary_device() {
    ...
    /* Take over SPI mode/speed from SPI main device */
    ancillary->max_speed_hz = spi->max_speed_hz;
    ancillary->mode = spi->mode;
    ...
}

If post_config_max_speed_hz is not copied here, will ancillary devices
have a value of 0 and silently fail to apply the higher speed during
PHY tuning?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3