RE: [EXTERNAL] Re: [PATCH v8 2/4] spi: cadence: Add Marvell xSPI IP overlay changes

[Witold Sadowski <wsadowski@xxxxxxxxxxx>]

Hi

> On Fri, Jun 07, 2024 at 08:18:29AM -0700, Witold Sadowski wrote:
> > This commit adds support for the basic v2 Marvell overlay block. Key
> > features included are:
> >     - Clock configuration
> >     - PHY configuration
> >     - Interrupt configuration (enabling)
> 
> This feels like it could usefully be split up so these three bits are
> separate, and there appear to be other changes buried in here as well.
> I can't tell what changes either the PHY or interrupt configuration might
> be referencing.

That changes are in single commit as, using not all of them will result in
total xSPI failure. Configuring PHY makes no sense if clock is not enabled.
But I can try to split that into 3 separate commits.

> 
> > @@ -295,6 +450,10 @@ static void cdns_xspi_set_interrupts(struct
> cdns_xspi_dev *cdns_xspi,
> >  				     bool enabled)
> >  {
> >  	u32 intr_enable;
> > +	u32 irq_status;
> > +
> > +	irq_status = readl(cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG);
> > +	writel(irq_status, cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG);
> >
> >  	intr_enable = readl(cdns_xspi->iobase + CDNS_XSPI_INTR_ENABLE_REG);
> >  	if (enabled)
> 
> This seems like a separate change which applies to everything, not just
> Marvell versions of the IP, and should be split out and explained.

I will move that to separate commit too. It is possible that previous
stage will not clear that register correctly, and that will lead to
interrupt fail - at least in Marvell implementation.

> 
> > @@ -319,6 +478,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 wasn't clearly mentioned in the changelog and is again being done
> unconditionally for all instances of the IP, probably best to split out
> and explain.

Ok, I can move to separate commit too. 

> 
> > +static void mrvl_ioreadq(void __iomem  *addr, void *buf, int len) {
> > +	int i = 0;
> > +	int rcount = len / 8;
> > +	int rcount_nf = len % 8;
> > +	uint64_t tmp;
> > +	uint64_t *buf64 = (uint64_t *)buf;
> 
> Any need to cast away from void * indicates a problem.

I will check that, but code is checking alignment of that pointer.

> 
> > @@ -337,13 +563,11 @@ static void cdns_xspi_sdma_handle(struct
> > cdns_xspi_dev *cdns_xspi)
> >
> >  	switch (sdma_dir) {
> >  	case CDNS_XSPI_SDMA_DIR_READ:
> > -		ioread8_rep(cdns_xspi->sdmabase,
> > -			    cdns_xspi->in_buffer, sdma_size);
> > +		cdns_xspi_sdma_memread(cdns_xspi, sdma_size);
> >  		break;
> 
> It's feeling like it might make sense to have an ops structure rather than
> sprinkling checks for the Marvell overlay everywhere.

Won't it cause big code duplication? There are some differences, but whole
Part of SPI stig mode configuration is the same.

Regards
Witek