Received-SPF: pass (google.com: domain of linux-kernel+bounces-211348-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 12 Jun 2024 11:31:40 +0100
From: Mark Brown <broonie@kernel.org>
To: Witold Sadowski <wsadowski@marvell.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"robh@kernel.org" <robh@kernel.org>,
	"krzysztof.kozlowski+dt@linaro.org" <krzysztof.kozlowski+dt@linaro.org>,
	"conor+dt@kernel.org" <conor+dt@kernel.org>,
	"pthombar@cadence.com" <pthombar@cadence.com>
Subject: Re: [EXTERNAL] Re: [PATCH v8 2/4] spi: cadence: Add Marvell xSPI IP
 overlay changes
Message-ID: <Zml5DD5s7OZopVkU@finisterre.sirena.org.uk>
References: <20240607151831.3858304-1-wsadowski@marvell.com>
 <20240607151831.3858304-3-wsadowski@marvell.com>
 <Zmcj3fZ4DF8r_qf0@finisterre.sirena.org.uk>
 <CO6PR18MB4098E3DEF64621FB147BC345B0C72@CO6PR18MB4098.namprd18.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="xvH9bqAg6EKL9q/g"
Content-Disposition: inline
In-Reply-To: <CO6PR18MB4098E3DEF64621FB147BC345B0C72@CO6PR18MB4098.namprd18.prod.outlook.com>


--xvH9bqAg6EKL9q/g
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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?

--xvH9bqAg6EKL9q/g
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmZpeQsACgkQJNaLcl1U
h9ADNAf/WTR0H0Qn2OWdd/BymBjuQCwA8E904iCChNKnS4OQO/0+hIrOvaP17XKx
CC+pD4RcnGfxAo7ZaSt+e/4x+FSwvCrvehnKyf/SyDsdGM9L7xiSA1DkaTUB17nn
2i4VK0aHKc1Dsl6/FBpX557xiWLY0/2Cvp7pSFvrKkserLjBXZQOK/bAKe+91DTg
W1Khh+gxUR2WExITICPU9oK2mPMQLICmv2OESHgG5KORduKk89GtcS0UNHngP+Zt
Rk1R8BwlDkdx9iGZSghCSZxMF+0Uce2z7xDL9iJiR6lCXvFKk2i2jTbX9vxy8v5/
D7z6idrOqBxEOijFsYLWngShvjw+NQ==
=oof7
-----END PGP SIGNATURE-----

--xvH9bqAg6EKL9q/g--