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

[Witold Sadowski <wsadowski@xxxxxxxxxxx>]

> ----------------------------------------------------------------------
> 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