Received-SPF: pass (google.com: domain of linux-kernel+bounces-210630-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
From: Witold Sadowski <wsadowski@marvell.com>
To: Mark Brown <broonie@kernel.org>
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
Thread-Topic: [EXTERNAL] Re: [PATCH v8 2/4] spi: cadence: Add Marvell xSPI IP
 overlay changes
Thread-Index: AQHauO35xUH3Zn/k1UOJaBN7iNN9L7HBLaqAgAHtIRA=
Date: Tue, 11 Jun 2024 21:51:58 +0000
Message-ID: 
 <CO6PR18MB4098E3DEF64621FB147BC345B0C72@CO6PR18MB4098.namprd18.prod.outlook.com>
References: <20240607151831.3858304-1-wsadowski@marvell.com>
 <20240607151831.3858304-3-wsadowski@marvell.com>
 <Zmcj3fZ4DF8r_qf0@finisterre.sirena.org.uk>
In-Reply-To: <Zmcj3fZ4DF8r_qf0@finisterre.sirena.org.uk>
Accept-Language: en-US, pl-PL
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?us-ascii?Q?O/3iPUWvSIw+Q9ZucugWBPTYqkm/TgU+n1LMLCjBFrN4yjCBIC/irk77NZIo?=
 =?us-ascii?Q?7SSGvsORgXe6ussfb2WoDuhKgoFcma+NyUa8RrX+GiNirBX8DfNKDg8V8Ods?=
 =?us-ascii?Q?s4pIKLP62+TvYfANx1o1kecm6SVeS1CtzbhAWvAFBztcn+/i8yKVjGydnVRf?=
 =?us-ascii?Q?hfoi4ER6XIQgNMWC9Gg2EL7xRz4Q+dCeynDxgv4V27ZT20X5MRE/1B/EO0Ve?=
 =?us-ascii?Q?OHXWCzeR/c1F4Qpz7Hi3ralCRZ5KSMcG6HJmYZAnNnt7a3AMfgWAYlf7xium?=
 =?us-ascii?Q?be0mwNLhXwknCoF5xvPOOPw9uisIRV55jr9B2B9W52Sdcztgo06NB3Nyh4X4?=
 =?us-ascii?Q?xbrB1QpMsAG0obO1LUFAqbU/R0D7WO6lTKZ1a0PdmxqsTOXUL4Rot/UZgjJi?=
 =?us-ascii?Q?8sdR1q8OtUkMMST5UzXRjwZORHXdr1FxQ0rJsBPuKKAXV7hzSrI2i/wAbiZb?=
 =?us-ascii?Q?zECSMzw7ldVMOtvbx4mxn3bKJnbypYS4touFpxG8b8hk1DQyFHVwFq0h2p0J?=
 =?us-ascii?Q?yxPXQcdbE1xnO5xpb2gkAuAWBT2hQ9Zi/tK2UI5nLtCkDe8MxFw9zDP3Ekz9?=
 =?us-ascii?Q?SRDzK/GHDohkSoxek01nig2TjXweXwdP9N/YhhZCXxgQrqJ9kcNJgFCKpAns?=
 =?us-ascii?Q?rTtX9KS0oi6HPe9zHiYsaUH7DZdsF9L7aN3HyrTdSD8mbKXWmPhl8a5JjSBa?=
 =?us-ascii?Q?UnDltGBc99iWAW4QIaPLdRTw8Ap6CIAW9dnUdpz9t9y+ha9KGO1mHwiM6rrI?=
 =?us-ascii?Q?j3b79aIwfsazC8+jMLv8/ZprYZyqvWW+CMkUJ6ALCqtQLp+60t08IER21aow?=
 =?us-ascii?Q?bp/vId53MJtOaLBRgEQXNGSAB2yMHGH5gzQg8X/Ejc8N2/FZZD9+RIcF2JSa?=
 =?us-ascii?Q?06gEgxzhU0N0g0pOuAC8sNPafinD0vyuspsuehMufYT/dvpVgcgr+pW/O5NA?=
 =?us-ascii?Q?PIMlpVe0sgNg1wKpEBk7CsPOee/hSS52O/1XcuRFsMd5mMneriq3yzLfvsX9?=
 =?us-ascii?Q?mYymJSXdhXn1T8AgZSfZGLPeSbyEA8f35PILkpZ6ZsH52lG7znGzecpDvUQv?=
 =?us-ascii?Q?VX6mlPhFM8j7TfrMGCI6yIFLQ18KCtQg8lzoNZWkwsxesbe8/Yy3W2b/qWpN?=
 =?us-ascii?Q?2WaXosJ5k09ZV7oRcDZ2PZBwTnC18Dv+6NReVAHwM/89heQgssab0ZxC4Di2?=
 =?us-ascii?Q?Yewa0TsGIiXdgFCkufBYA/sFrNJpa4NNrjQLdMD7/G2gR7QdYZSK/8nblsJt?=
 =?us-ascii?Q?+Rvkl6QfvFzNSXVkMLiN5BmLH0gdzho1A4X+t1Pywxj/vRIlgYkLHWH5IsJ0?=
 =?us-ascii?Q?cbgpGPFRWTGZ1Vtvb3XiPliCWtlxNNYE5RaL/MHw6shBxjk7g/VoPaefzPJE?=
 =?us-ascii?Q?M1D79pQHohIUSq/MSNx1HAzItY3CWoUTszxn9VUBxegpsdvR56MHF0ULuZ7U?=
 =?us-ascii?Q?LHO2GbnD1sEuG83YvHjc3aq3DAHZFDq2vicT0VcMUG3rieEFLxC7nAZY/XD+?=
 =?us-ascii?Q?g5wJQaO1xbehEH9O/XyhUqZ6vLAgcBZ1KpjsjeLQuME+ysksSd3NgvZz+Ggy?=
 =?us-ascii?Q?qR40qlReEerIxEicTHJo71vTWtMTRA/dAKqBbr2x?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CO6PR18MB4098.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 41807f58-c6d4-45d7-6d4c-08dc8a60b7ca
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Jun 2024 21:51:58.4226
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: PHMoJZ/sP2CTWgNjDuvaFMdBuR2Q9CYXLuR8mMyhkkZj4QHhv6rRub8rL9kQB5OOGg0qy2aY9ab38B8KOOciXg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR18MB4457
X-Proofpoint-GUID: gFL96voD7RpI_4zrm8fWXWSa_Pi9JdF4
X-Proofpoint-ORIG-GUID: gFL96voD7RpI_4zrm8fWXWSa_Pi9JdF4
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16
 definitions=2024-06-11_11,2024-06-11_01,2024-05-17_01

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)
>=20
> 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.

>=20
> > @@ -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 =3D readl(cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG);
> > +	writel(irq_status, cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG);
> >
> >  	intr_enable =3D readl(cdns_xspi->iobase + CDNS_XSPI_INTR_ENABLE_REG);
> >  	if (enabled)
>=20
> 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.

>=20
> > @@ -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);
> > +
>=20
> 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.=20

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

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

>=20
> > @@ -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;
>=20
> It's feeling like it might make sense to have an ops structure rather tha=
n
> 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