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:
signature.asc
Description: PGP signature
