The 02/10/2020 19:16, Mark Brown wrote:
EXTERNAL MAIL
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. :/
What I see is Kconfig is first and Makefile is second file in diff,
according to:
drivers/spi/Kconfig | 11 +
drivers/spi/Makefile | 1 +
Is that wrong?
--- /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?
This is because this driver was tested only on FPGA board.
This driver was not tested for ASIC version as PHY
initialization algorithm is differ.
--
--
Konrad Kociolek
