[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<CO6PR18MB4098FAFE58A705F6809D8727B01B2@CO6PR18MB4098.namprd18.prod.outlook.com>
Date: Mon, 29 Apr 2024 15:27:23 +0000
From: Witold Sadowski <wsadowski@...vell.com>
To: Mark Brown <broonie@...nel.org>
CC: "linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"jpawar@...ence.com" <jpawar@...ence.com>,
"pthombar@...ence.com"
<pthombar@...ence.com>,
"konrad@...ence.com" <konrad@...ence.com>,
Wojciech
Bartczak <wbartczak@...vell.com>,
Wojciech Zmuda <wzmuda@...vell.com>
Subject: RE: [EXT] Re: [PATCH 6/7] spi: cadence: Add Marvell IP modification
changes
> ----------------------------------------------------------------------
> 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.
>
> 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.
>
> > +config SPI_CADENCE_MRVL_XSPI
> > + tristate "Marvell mods for XSPI controller"
> > + depends on SPI_CADENCE_XSPI
> > +
> > + help
>
> 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.
>
> > + /*Reset DLL*/
>
> Please follow the kernel coding style.
>
> > @@ -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);
> > +
>
> 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.
>
> > +#if IS_ENABLED(CONFIG_SPI_CADENCE_MRVL_XSPI)
> > + writel(CDNS_MSIX_CLEAR_IRQ, cdns_xspi->auxbase +
> > +CDNS_XSPI_SPIX_INTR_AUX); #endif
>
> 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.
>
> > +#if IS_ENABLED(CONFIG_SPI_CADENCE_MRVL_XSPI)
> > +static int cdns_xspi_setup(struct spi_device *spi_dev) {
> > + struct cdns_xspi_dev *cdns_xspi =
> > +spi_master_get_devdata(spi_dev->master);
> > +
> > + cdns_xspi_setup_clock(cdns_xspi, spi_dev->max_speed_hz);
> > +
> > + return 0;
> > +}
> > +#endif
>
> 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
Powered by blists - more mailing lists