> ----------------------------------------------------------------------
> 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?
In that particular driver, we have to set control registers to predefined
Values, that depends on feeding clock/internal architecture etc.
>
> > +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.
Kconfig is removed now.
>
> > + /*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?
You are referring to mode switch? That will affect only one IP, different
Will not be affected.
>
> > +#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.
Ok, that was reworked to base on device-tree compatible property.
Also, that part was changed in v2 overlay.
>
> > +#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.
Clock set function will act only when actual change is needed. And it seems
Changing the clock is not affecting xSPI block
Regards
Witek
