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

[Witold Sadowski <wsadowski@xxxxxxxxxxx>]

> ----------------------------------------------------------------------
> On 19/12/2022 15:42, 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.
> > To correctly clear interrupt in Marvell implementation MSI-X must be
> > cleared too.
> >
> > Signed-off-by: Witold Sadowski <wsadowski@xxxxxxxxxxx>
> > Reviewed-by: Chandrakala Chavva <cchavva@xxxxxxxxxxx>
> > Tested-by: Sunil Kovvuri Goutham <sgoutham@xxxxxxxxxxx>
> > ---
> >  drivers/spi/Kconfig            |  12 +++
> >  drivers/spi/spi-cadence-xspi.c | 172
> > ++++++++++++++++++++++++++++++++-
> >  2 files changed, 183 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index
> > 3b1c0878bb85..42af5943d00a 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -251,6 +251,18 @@ config SPI_CADENCE_XSPI
> >  	  device with a Cadence XSPI controller and want to access the
> >  	  Flash as an MTD device.
> >
> > +config SPI_CADENCE_MRVL_XSPI
> > +	tristate "Marvell mods for XSPI controller"
> > +	depends on SPI_CADENCE_XSPI
> > +
> > +	help
> > +	  Enable support for Marvell XSPI modifications
> > +
> > +	  During implementation of Cadence XSPI core Marvell
> > +	  has added some additional features like clock divider,
> > +	  PHY config support or non-memory SPI capabilities.
> > +	  Enable that option if you want to enable these features.
> > +
> >  config SPI_CLPS711X
> >  	tristate "CLPS711X host SPI controller"
> >  	depends on ARCH_CLPS711X || COMPILE_TEST diff --git
> > a/drivers/spi/spi-cadence-xspi.c b/drivers/spi/spi-cadence-xspi.c
> > index 719c2f3b4771..c73faf6b0546 100644
> > --- a/drivers/spi/spi-cadence-xspi.c
> > +++ b/drivers/spi/spi-cadence-xspi.c
> > @@ -193,6 +193,46 @@
> >  #define CDNS_XSPI_POLL_TIMEOUT_US	1000
> >  #define CDNS_XSPI_POLL_DELAY_US	10
> >
> > +#if IS_ENABLED(CONFIG_SPI_CADENCE_MRVL_XSPI)
> > +/* clock config register */
> > +#define CDNS_XSPI_CLK_CTRL_AUX_REG	0x2020
> > +#define CDNS_XSPI_CLK_ENABLE		BIT(0)
> > +#define CDNS_XSPI_CLK_DIV		GENMASK(4, 1)
> > +
> > +/* Clock macros */
> > +#define CDNS_XSPI_CLOCK_IO_HZ 800000000 #define
> > +CDNS_XSPI_CLOCK_DIVIDED(div) ((CDNS_XSPI_CLOCK_IO_HZ) / (div))
> > +
> > +/*PHY default values*/
> 
> Keep consistent style.
> 
> > +#define REGS_DLL_PHY_CTRL		0x00000707
> > +#define CTB_RFILE_PHY_CTRL		0x00004000
> > +#define RFILE_PHY_TSEL			0x00000000
> > +#define RFILE_PHY_DQ_TIMING		0x00000101
> > +#define RFILE_PHY_DQS_TIMING		0x00700404
> > +#define RFILE_PHY_GATE_LPBK_CTRL	0x00200030
> > +#define RFILE_PHY_DLL_MASTER_CTRL	0x00800000
> > +#define RFILE_PHY_DLL_SLAVE_CTRL	0x0000ff01
> > +
> > +/*PHY config rtegisters*/
> > +#define CDNS_XSPI_RF_MINICTRL_REGS_DLL_PHY_CTRL		0x1034
> > +#define CDNS_XSPI_PHY_CTB_RFILE_PHY_CTRL			0x0080
> > +#define CDNS_XSPI_PHY_CTB_RFILE_PHY_TSEL			0x0084
> > +#define CDNS_XSPI_PHY_DATASLICE_RFILE_PHY_DQ_TIMING		0x0000
> > +#define CDNS_XSPI_PHY_DATASLICE_RFILE_PHY_DQS_TIMING		0x0004
> > +#define CDNS_XSPI_PHY_DATASLICE_RFILE_PHY_GATE_LPBK_CTRL	0x0008
> > +#define CDNS_XSPI_PHY_DATASLICE_RFILE_PHY_DLL_MASTER_CTRL	0x000c
> > +#define CDNS_XSPI_PHY_DATASLICE_RFILE_PHY_DLL_SLAVE_CTRL	0x0010
> > +#define CDNS_XSPI_DATASLICE_RFILE_PHY_DLL_OBS_REG_0		0x001c
> > +
> > +#define CDNS_XSPI_DLL_RST_N BIT(24)
> > +#define CDNS_XSPI_DLL_LOCK  BIT(0)
> > +
> > +/* MSI-X clear interrupt register */
> > +#define CDNS_XSPI_SPIX_INTR_AUX				0x2000
> > +#define CDNS_MSIX_CLEAR_IRQ					0x01
> > +
> > +#endif
> > +
> >  enum cdns_xspi_stig_instr_type {
> >  	CDNS_XSPI_STIG_INSTR_TYPE_0,
> >  	CDNS_XSPI_STIG_INSTR_TYPE_1,
> > @@ -238,6 +278,106 @@ struct cdns_xspi_dev {
> >  	enum cdns_xspi_sdma_size read_size;
> >  };
> >
> > +#if IS_ENABLED(CONFIG_SPI_CADENCE_MRVL_XSPI)
> 
> Why this is under #if? Different devices have always built-in support and
> behavior is determined by binding method (e.g. match data).

That is already addressed(matching with compatible property)

> 
> > +
> > +#define MRVL_DEFAULT_CLK 25000000
> > +
> > +const int cdns_xspi_clk_div_list[] = {
> > +	4,	//0x0 = Divide by 4.   SPI clock is 200 MHz.
> > +	6,	//0x1 = Divide by 6.   SPI clock is 133.33 MHz.
> > +	8,	//0x2 = Divide by 8.   SPI clock is 100 MHz.
> > +	10,	//0x3 = Divide by 10.  SPI clock is 80 MHz.
> > +	12,	//0x4 = Divide by 12.  SPI clock is 66.666 MHz.
> > +	16,	//0x5 = Divide by 16.  SPI clock is 50 MHz.
> > +	18,	//0x6 = Divide by 18.  SPI clock is 44.44 MHz.
> > +	20,	//0x7 = Divide by 20.  SPI clock is 40 MHz.
> > +	24,	//0x8 = Divide by 24.  SPI clock is 33.33 MHz.
> > +	32,	//0x9 = Divide by 32.  SPI clock is 25 MHz.
> > +	40,	//0xA = Divide by 40.  SPI clock is 20 MHz.
> > +	50,	//0xB = Divide by 50.  SPI clock is 16 MHz.
> > +	64,	//0xC = Divide by 64.  SPI clock is 12.5 MHz.
> > +	128,	//0xD = Divide by 128. SPI clock is 6.25 MHz.
> > +	-1	//End of list
> 
> Why? This is a static list so size is known.

Ok.

> 
> > +};
> > +
> > +static bool cdns_xspi_reset_dll(struct cdns_xspi_dev *cdns_xspi) {
> > +	u32 dll_cntrl = readl(cdns_xspi->iobase +
> CDNS_XSPI_RF_MINICTRL_REGS_DLL_PHY_CTRL);
> > +	u32 dll_lock;
> > +
> > +	/*Reset DLL*/
> > +	dll_cntrl |= CDNS_XSPI_DLL_RST_N;
> > +	writel(dll_cntrl, cdns_xspi->iobase +
> > +CDNS_XSPI_RF_MINICTRL_REGS_DLL_PHY_CTRL);
> > +
> > +	/*Wait for DLL lock*/
> 
> All these comments miss spaces around.
> 
> > +	return readl_relaxed_poll_timeout(cdns_xspi->iobase +
> > +		CDNS_XSPI_INTR_STATUS_REG,
> > +		dll_lock, ((dll_lock & CDNS_XSPI_DLL_LOCK) == 1), 10, 10000);
> }
> > +
> > +//Static confiuration of PHY
> 
> Keep consistent style.
> 
> > +static bool cdns_xspi_configure_phy(struct cdns_xspi_dev *cdns_xspi)
> > +{
> > +	writel(REGS_DLL_PHY_CTRL,
> > +		cdns_xspi->iobase + CDNS_XSPI_RF_MINICTRL_REGS_DLL_PHY_CTRL);
> 
> Don't you need a phy driver?

That is one time configuration, that won't change during run.
Is the phy driver needed for such a simple case?

> 
> > +	writel(CTB_RFILE_PHY_CTRL,
> > +		cdns_xspi->auxbase + CDNS_XSPI_PHY_CTB_RFILE_PHY_CTRL);
> > +	writel(RFILE_PHY_TSEL,
> > +		cdns_xspi->auxbase + CDNS_XSPI_PHY_CTB_RFILE_PHY_TSEL);
> > +	writel(RFILE_PHY_DQ_TIMING,
> > +		cdns_xspi->auxbase +
> CDNS_XSPI_PHY_DATASLICE_RFILE_PHY_DQ_TIMING);
> > +	writel(RFILE_PHY_DQS_TIMING,
> > +		cdns_xspi->auxbase +
> CDNS_XSPI_PHY_DATASLICE_RFILE_PHY_DQS_TIMING);
> > +	writel(RFILE_PHY_GATE_LPBK_CTRL,
> > +		cdns_xspi->auxbase +
> CDNS_XSPI_PHY_DATASLICE_RFILE_PHY_GATE_LPBK_CTRL);
> > +	writel(RFILE_PHY_DLL_MASTER_CTRL,
> > +		cdns_xspi->auxbase +
> CDNS_XSPI_PHY_DATASLICE_RFILE_PHY_DLL_MASTER_CTRL);
> > +	writel(RFILE_PHY_DLL_SLAVE_CTRL,
> > +		cdns_xspi->auxbase +
> > +CDNS_XSPI_PHY_DATASLICE_RFILE_PHY_DLL_SLAVE_CTRL);
> > +
> > +	return cdns_xspi_reset_dll(cdns_xspi); }
> > +
> > +// Find max avalible clock
> 
> Run spell-check.
> 
> 
> Best regards,
> Krzysztof

Regards
Witek