Received-SPF: pass (google.com: domain of linux-kernel+bounces-162588-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Witold Sadowski <wsadowski@marvell.com>
To: Mark Brown <broonie@kernel.org>
CC: "linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "jpawar@cadence.com" <jpawar@cadence.com>,
        "pthombar@cadence.com"
	<pthombar@cadence.com>,
        "konrad@cadence.com" <konrad@cadence.com>,
        Wojciech
 Bartczak <wbartczak@marvell.com>,
        Wojciech Zmuda <wzmuda@marvell.com>
Subject: RE: [EXT] Re: [PATCH 6/7] spi: cadence: Add Marvell IP modification
 changes
Thread-Topic: [EXT] Re: [PATCH 6/7] spi: cadence: Add Marvell IP modification
 changes
Thread-Index: AQHZE7g77QR4QGOigEOdIGFr7/+Gq651h7IAgwzhMCA=
Date: Mon, 29 Apr 2024 15:27:23 +0000
Message-ID: 
 <CO6PR18MB4098FAFE58A705F6809D8727B01B2@CO6PR18MB4098.namprd18.prod.outlook.com>
References: <20221219144254.20883-1-wsadowski@marvell.com>
 <20221219144254.20883-7-wsadowski@marvell.com>
 <Y6CtGEmi0wZUKaxT@sirena.org.uk>
In-Reply-To: <Y6CtGEmi0wZUKaxT@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?dCI/Avv151wLDm+/9iAum4d+zxE8Cm0Fw4C1pFb0TF0uLEfJFIDCFwr0ntJp?=
 =?us-ascii?Q?39d7gUZyG2inAC+EDU8fPaMK0vfO4PadOBAy2cNU0sqOVgvo7XZYA/zS4t51?=
 =?us-ascii?Q?Z07BqWlwUdNvpJWogihGc03aPK/a8C0dn+V/rUoRNkB6qhLMXOl1O4qXs6/2?=
 =?us-ascii?Q?q2bdDrpi+SC7HGw4B4nX1gTKiM6OzLHngwydOnUPO4IMnH69bxQvXhefsD3R?=
 =?us-ascii?Q?NRLMT35kwvY+dSfyCs+uWkplk3xAdLzu1ttHwL/pm1Pub18zxV/M0eAlz1p1?=
 =?us-ascii?Q?wR1YeVHmZIky7f15SU470TdKWXd59QBByeiiBrXIZoNxvxCjf/iEWgJA4O2p?=
 =?us-ascii?Q?wbA+kptqZ4hzBUn419qW8EGyT4qGrOVPMPki/pj9xZbmSBkqP7dofqVuqxoY?=
 =?us-ascii?Q?s1KAICoM9PSY6v1mAOtBq66osZhK3E7xuHeXT8H/t1kcPOj2JE5GOkAJoLRx?=
 =?us-ascii?Q?/wBnGXkTkM1n2SIQ/AtMN9oz4/RuLnwgpGx5urJGQM4t8w1ugeJeBlhYOeew?=
 =?us-ascii?Q?ivwInLDNawFnhmErctlz2yLO9qmuB6lWxXwgAPA8HagWFYp2GQlgQmwMGlH9?=
 =?us-ascii?Q?Oiy5AWmthaQhrBrzCXyZ3sGEQoRAkTGKMHwK1WezH+9auy0O/QZaWrqV70XA?=
 =?us-ascii?Q?qjHHmVH9/QWl24xLrJC5LFfc6SIvgqA7WoRim3Dp2kwqMqvzTeSxZ8FjS6jq?=
 =?us-ascii?Q?OfhVibysEixiR431ACnNLObrnRyhBzIXo9udt8hp44TUTpSLKLhtgcXec0m+?=
 =?us-ascii?Q?z16HcbPJXuMTDCeaZl4n0pN/zaMUZifgvQyBESLWVHarqF8uLiawf0U9H/Wh?=
 =?us-ascii?Q?3hSRhv8CXsQPsviYQqys5MrfibV0SJt8oRy8i1/933vY/XsQBpfjy85qGpk9?=
 =?us-ascii?Q?ZcE347DKMIHsYqPrq3jU/z4Y3TIk/EOgHZc+GiNt63ztj5ERYjiHoOzavvUh?=
 =?us-ascii?Q?I0Fge1i59UDTTO6kWuk/JoQPn7J0SLIfBJ+M5tiR47peka+Cq6BGQioDL1J8?=
 =?us-ascii?Q?odgbPxzCg8raEFdkroWmHd26EY/zZA1NktOSBGH6Z+CiylKwUUSlZyuZCqrf?=
 =?us-ascii?Q?deXRCVLBB5cfOrlRvT4ArftgYQi/mDzOnord7tBDmiCnd5QSSKsQNGxWoh6p?=
 =?us-ascii?Q?b1fpHNip4mwlLdlU9MVtLIU9O1R8884o0vom2Fe9lNVqH9YLUw/NKmgl41G5?=
 =?us-ascii?Q?Hv/MwERBHP4gsFAsbj8GueScHWrsoGGsCc/VzdG+DBpn28yj9oS8ahFlf4OA?=
 =?us-ascii?Q?zANJ8t8wQ9vygmmdcecSmzPOzO1fooVTbx0ks2K84U1fNaBgmMDiIYaCgszP?=
 =?us-ascii?Q?sdmJvCovi24idsc6sIYpF9MVxi468fdvt7kEMdtXIZP3fwXYbZUJ/1EjN+LF?=
 =?us-ascii?Q?QU1AIc9PAjj9NldtCiWxorI+hWua14SKBfxwkZz9+88Wxt7nLY+U1Ek27jB3?=
 =?us-ascii?Q?Fo5awKY+zML14IfVVAAeStS4wuHuPMv16CuRdjSJ5zBaD1oVvjoJX0bo08bp?=
 =?us-ascii?Q?mvQfm151+3emB9Co3aVW+cN6LTf0yXgQBBBBvT9i+VjelsICCpW9pxSkTczi?=
 =?us-ascii?Q?9ij16nSDkU7tMS54G8GMLsLoxDi4JSWzsfrIia+S?=
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: f2f06a27-668b-4188-ae88-08dc6860de34
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Apr 2024 15:27:23.3656
 (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: 1PaO7H+qT+Q/ii7T/ma+wiYtUr8yQ64CwR23HUVdmTTe8i0ACKPA1OBd39bEx/njgdrfBvCvhDyxm9pkoVBtyw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR18MB3469
X-Proofpoint-ORIG-GUID: DnHPGZ_ZmIdfJN8PQuAqrHw5GsB2b7gn
X-Proofpoint-GUID: DnHPGZ_ZmIdfJN8PQuAqrHw5GsB2b7gn
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1011,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-04-29_12,2024-04-29_01,2023-05-22_02

> ----------------------------------------------------------------------
> On Mon, Dec 19, 2022 at 06:42:53AM -0800, 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.
>=20
> What is a PHY in the context of a SPI controller?

In that particular driver, we have to set control registers to predefined
Values, that depends on feeding clock/internal architecture etc.

>=20
> > +config SPI_CADENCE_MRVL_XSPI
> > +	tristate "Marvell mods for XSPI controller"
> > +	depends on SPI_CADENCE_XSPI
> > +
> > +	help
>=20
> Extra blank line (does this work?).  It's not clear to me that there's
> enough code here to justify a Kconfig.

Kconfig is removed now.

>=20
> > +	/*Reset DLL*/
>=20
> Please follow the kernel coding style.
>=20
> > @@ -328,6 +468,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 is done unconditionally, will other instances in the IP be OK with
> it?  Should it be a separate commit since it's affecting everything?

You are referring to mode switch? That will affect only one IP, different
Will not be affected.

>=20
> > +#if IS_ENABLED(CONFIG_SPI_CADENCE_MRVL_XSPI)
> > +	writel(CDNS_MSIX_CLEAR_IRQ, cdns_xspi->auxbase +
> > +CDNS_XSPI_SPIX_INTR_AUX); #endif
>=20
> This is not how we do support for variants of an IP, we need to support a
> single kernel image for many different systems so variant handling needs
> to be done with runtime selection not build time selection.
> Please handle this in a similar way to how other drivers handle support
> for multiple devices.

Ok, that was reworked to base on device-tree compatible property.
Also, that part was changed in v2 overlay.

>=20
> > +#if IS_ENABLED(CONFIG_SPI_CADENCE_MRVL_XSPI)
> > +static int cdns_xspi_setup(struct spi_device *spi_dev) {
> > +	struct cdns_xspi_dev *cdns_xspi =3D
> > +spi_master_get_devdata(spi_dev->master);
> > +
> > +	cdns_xspi_setup_clock(cdns_xspi, spi_dev->max_speed_hz);
> > +
> > +	return 0;
> > +}
> > +#endif
>=20
> Note that setup() might be called while other transfers are in progress
> and should not affect them.

Clock set function will act only when actual change is needed. And it seems
Changing the clock is not affecting xSPI block

Regards
Witek