Re: [PATCH v2 1/2] Add Cadence XSPI driver

[Konrad Kociolek]

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