Re: [PATCH 6/7] spi: cadence: Add Marvell IP modification changes

Mark Brown
2022-12-19T18:27:50Z

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

On Mon, Dec 19, 2022 at 06:42:53AM -0800, Witold Sadowski wrote:
> Add support for Marvell IP modification - clock divider,
> and PHY config, and IRQ clearing.
> Clock divider block is build into Cadence XSPI controller
> and is connected directly to 800MHz clock.
> As PHY config is not set directly in IP block, driver can
> load custom PHY configuration values.

What is a PHY in the context of a SPI controller?

> +config SPI_CADENCE_MRVL_XSPI
> + tristate "Marvell mods for XSPI controller"
> + depends on SPI_CADENCE_XSPI
> +
> + help

Extra blank line (does this work?). It's not clear to me that there's
enough code here to justify a Kconfig.

> + /*Reset DLL*/

Please follow the kernel coding style.

> @@ -328,6 +468,9 @@ static int cdns_xspi_controller_init(struct cdns_xspi_dev *cdns_xspi)
> return -EIO;
> }
>
> + writel(FIELD_PREP(CDNS_XSPI_CTRL_WORK_MODE, CDNS_XSPI_WORK_MODE_STIG),
> + cdns_xspi->iobase + CDNS_XSPI_CTRL_CONFIG_REG);
> +

This is done unconditionally, will other instances in the IP be OK with
it? Should it be a separate commit since it's affecting everything?

> +#if IS_ENABLED(CONFIG_SPI_CADENCE_MRVL_XSPI)
> + writel(CDNS_MSIX_CLEAR_IRQ, cdns_xspi->auxbase + CDNS_XSPI_SPIX_INTR_AUX);
> +#endif

This is not how we do support for variants of an IP, we need to support
a single kernel image for many different systems so variant handling
needs to be done with runtime selection not build time selection.
Please handle this in a similar way to how other drivers handle support
for multiple devices.

> +#if IS_ENABLED(CONFIG_SPI_CADENCE_MRVL_XSPI)
> +static int cdns_xspi_setup(struct spi_device *spi_dev)
> +{
> + struct cdns_xspi_dev *cdns_xspi = spi_master_get_devdata(spi_dev->master);
> +
> + cdns_xspi_setup_clock(cdns_xspi, spi_dev->max_speed_hz);
> +
> + return 0;
> +}
> +#endif

Note that setup() might be called while other transfers are in progress
and should not affect them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]