lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ