On Tue, Jan 28, 2020 at 01:41:57PM +0100, Konrad Kociolek wrote:
> Enables Xilinx GQSPI controller driver for Zynq UltraScale+ MPSoC.
>
> +config SPI_CADENCE_XSPI
> + tristate "Cadence XSPI controller"
> + depends on (OF || COMPILE_TEST) && HAS_IOMEM
> + help
> + Enable support for the Cadence XSPI Flash controller.
> +
> + Cadence XSPI is a specialized controller for connecting an SPI
> + Flash over upto 8bit wide bus. Enable this option if you have a
> + device with a Cadence XSPI controller and want to access the
> + Flash as an MTD device.
> +
> #
> # Add new SPI master controllers in alphabetical order above this line
> #
Please keep Kconfig and Makefile alphabetically sorted as the comment in
the context from the diff says. :/
> --- /dev/null
> +++ b/drivers/spi/spi-cadence-xspi.c
> @@ -0,0 +1,895 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Cadence XSPI flash controller driver
Please make the entire comment a C++ so things look more intentional.
> + dev_info(cdns_xspi->dev,
> + "Running PHY training for read_dqs_delay parameter\n");
This print is just noise, please remove it.
> +static int cdns_xspi_setup(struct spi_device *spi_dev)
> +{
> + if (spi_dev->chip_select > spi_dev->master->num_chipselect) {
> + dev_err(&spi_dev->dev,
> + "%d chip-select is out of range\n",
> + spi_dev->chip_select);
> + return -EINVAL;
> + }
If this isn't already being validated by the core it should be and this
function can be removed.
> + irq_status = readl(cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG);
> + if (irq_status) {
> + writel(irq_status,
> + cdns_xspi->iobase + CDNS_XSPI_INTR_STATUS_REG);
This unconditionally acknowledges everything, even things we don't
understand. If the hardware is generating unexpected interrupt statuses
we should probably warn.
> +static void cdns_xspi_print_phy_config(struct cdns_xspi_dev *cdns_xspi)
> +{
> + struct device *dev = cdns_xspi->dev;
> +
> + dev_info(dev, "PHY configuration\n");
> + dev_info(dev, " * xspi_dll_phy_ctrl: %08x\n",
> + readl(cdns_xspi->iobase + CDNS_XSPI_DLL_PHY_CTRL));
> + dev_info(dev, " * phy_dq_timing: %08x\n",
> + readl(cdns_xspi->auxbase + CDNS_XSPI_CCP_PHY_DQ_TIMING));
> + dev_info(dev, " * phy_dqs_timing: %08x\n",
> + readl(cdns_xspi->auxbase + CDNS_XSPI_CCP_PHY_DQS_TIMING));
> + dev_info(dev, " * phy_gate_loopback_ctrl: %08x\n",
> + readl(cdns_xspi->auxbase + CDNS_XSPI_CCP_PHY_GATE_LPBCK_CTRL));
> + dev_info(dev, " * phy_dll_slave_ctrl: %08x\n",
> + readl(cdns_xspi->auxbase + CDNS_XSPI_CCP_PHY_DLL_SLAVE_CTRL));
> +}
This seems pretty verbose for an individual device... If this is needed
for diagnostics perhaps put it in sysfs or debugfs where it'll be
accessible even if the log wraps?
> +err_no_mem:
> + dev_err(dev, "Failed to probe Cadence XSPI controller driver\n");
Not sure this is adding anything over the individual error messages on
specific failures.
> +#ifdef CONFIG_OF
> +static const struct of_device_id cdns_xspi_of_match[] = {
> + {
> + .compatible = "cdns,xspi-nor-fpga",
> + },
Why -fpga?
Attachment:
signature.asc
Description: PGP signature
