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

[Mark Brown]

On Tue, Jun 11, 2024 at 09:51:58PM +0000, Witold Sadowski wrote:
> > On Fri, Jun 07, 2024 at 08:18:29AM -0700, Witold Sadowski wrote:

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

They won't actually do anything until we detect the Marvell IP.

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

A cast won't do anything to fix alignment issues.

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

No more than having a bunch of functions which are called a single time
with checks in them will?

Attachment: signature.asc
Description: PGP signature