linux-kernel - Re: [PATCH 6/7] spi: cadence: Add Marvell IP modification changes

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]

Message-ID: <Y6CtGEmi0wZUKaxT@sirena.org.uk>
Date:   Mon, 19 Dec 2022 18:27:36 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Witold Sadowski <wsadowski@...vell.com>
Cc:     linux-spi@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, jpawar@...ence.com,
        pthombar@...ence.com, konrad@...ence.com, wbartczak@...vell.com,
        wzmuda@...vell.com
Subject: 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?

> +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.

> +	/*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?

> +#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.

> +#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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)