On Mon, Feb 24, 2025 at 01:21:15PM +0000, Yixun Lan wrote:
> Hi Longbin:
>
> On 18:12 Mon 24 Feb , Longbin Li wrote:
> > Add support for Sophgo SPI NOR controller in Sophgo SoC.
> >
> > Signed-off-by: Longbin Li <looong.bin@xxxxxxxxx>
> > ---
> > drivers/spi/Kconfig | 9 +
> > drivers/spi/Makefile | 1 +
> > drivers/spi/spi-sophgo-nor.c | 501 +++++++++++++++++++++++++++++++++++
> > 3 files changed, 511 insertions(+)
> > create mode 100644 drivers/spi/spi-sophgo-nor.c
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index ea8a31032927..6b6d7b348485 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -1021,6 +1021,15 @@ config SPI_SN_F_OSPI
> > for connecting an SPI Flash memory over up to 8-bit wide bus.
> > It supports indirect access mode only.
> >
> > +config SPI_SOPHGO_NOR
> > + tristate "Sophgo SPI NOR Controller"
> > + depends on ARCH_SOPHGO || COMPILE_TEST
> > + help
> > + This enables support for the Sophgo SPI NOR controller,
> > + which supports Dual/Qual read and write operations while
> > + also supporting 3Byte address devices and 4Byte address
> > + devices.
> > +
> > config SPI_SPRD
> > tristate "Spreadtrum SPI controller"
> > depends on ARCH_SPRD || COMPILE_TEST
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> > index 9db7554c1864..9ded1de4b2fd 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -134,6 +134,7 @@ obj-$(CONFIG_SPI_SH_SCI) += spi-sh-sci.o
> > obj-$(CONFIG_SPI_SIFIVE) += spi-sifive.o
> > obj-$(CONFIG_SPI_SLAVE_MT27XX) += spi-slave-mt27xx.o
> > obj-$(CONFIG_SPI_SN_F_OSPI) += spi-sn-f-ospi.o
> > +obj-$(CONFIG_SPI_SOPHGO_NOR) += spi-sophgo-nor.o
> > obj-$(CONFIG_SPI_SPRD) += spi-sprd.o
> > obj-$(CONFIG_SPI_SPRD_ADI) += spi-sprd-adi.o
> > obj-$(CONFIG_SPI_STM32) += spi-stm32.o
> > diff --git a/drivers/spi/spi-sophgo-nor.c b/drivers/spi/spi-sophgo-nor.c
> > new file mode 100644
> > index 000000000000..1139deeac327
> > --- /dev/null
> > +++ b/drivers/spi/spi-sophgo-nor.c
> > @@ -0,0 +1,501 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Sophgo SPI NOR controller driver
> > + *
> > + * Copyright (c) 2025 Longbin Li <looong.bin@xxxxxxxxx>
> > + */
> > +
> > [...]
> > +struct sophgo_spifmc {
> > + struct spi_controller *ctrl;
> > + void __iomem *io_base;
> > + struct device *dev;
> > + struct mutex lock;
> it will be great to document the lock
Thanks, but I don't think the function of this lock is complicated,
adding comments may be useless.
> > + struct clk *clk;
> > +};
> > +
> > +static int sophgo_spifmc_wait_int(struct sophgo_spifmc *spifmc, u8 int_type)
> > +{
> > + u32 stat;
> > +
> > + return readl_poll_timeout(spifmc->io_base + SPIFMC_INT_STS, stat,
> > + (stat & int_type), 0, 1000000);
> > +}
> > +
> > +
> > [...]
> > +static ssize_t sophgo_spifmc_trans_reg(struct sophgo_spifmc *spifmc,
> > + const struct spi_mem_op *op)
> > +{
> > + const u8 *dout = NULL;
> > + u8 *din = NULL;
> > + size_t len = op->data.nbytes;
> > + u32 reg;
> > + int ret;
> > + int i;
> squash them which save one line:
> int i, ret;
>
Thanks, I will modify it.
> > +
> > + if (op->data.dir == SPI_MEM_DATA_IN)
> > + din = op->data.buf.in;
> > + else
> > + dout = op->data.buf.out;
> > +
> > + reg = sophgo_spifmc_init_reg(spifmc);
> > + reg |= SPIFMC_TRAN_CSR_FIFO_TRG_LVL_1_BYTE;
> > + reg |= SPIFMC_TRAN_CSR_WITH_CMD;
> > +
> > + if (din) {
> > + reg |= SPIFMC_TRAN_CSR_BUS_WIDTH_1_BIT;
> > + reg |= SPIFMC_TRAN_CSR_TRAN_MODE_RX;
> > + reg |= SPIFMC_TRAN_CSR_TRAN_MODE_TX;
> > +
> > + writel(SPIFMC_OPT_DISABLE_FIFO_FLUSH, spifmc->io_base + SPIFMC_OPT);
> > + } else {
> > + /*
> > + * If write values to the Status Register,
> > + * configure TRAN_CSR register as the same as
> > + * sophgo_spifmc_read_reg.
> > + */
> > + if (op->cmd.opcode == 0x01) {
> > + reg |= SPIFMC_TRAN_CSR_TRAN_MODE_RX;
> > + reg |= SPIFMC_TRAN_CSR_TRAN_MODE_TX;
> > + writel(len, spifmc->io_base + SPIFMC_TRAN_NUM);
> > + }
> > + }
> > +
> > +
> > [...]
> > +static const struct spi_controller_mem_ops sophgo_spifmc_mem_ops = {
> > + .exec_op = sophgo_spifmc_exec_op,
> > +};
> > +
> > +static void sophgo_spifmc_init(struct sophgo_spifmc *spifmc)
> > +{
> > + u32 tran_csr;
> > + u32 reg;
> > +
> > + writel(0, spifmc->io_base + SPIFMC_DMMR);
> > +
> > + reg = readl(spifmc->io_base + SPIFMC_CTRL);
> > + reg |= SPIFMC_CTRL_SRST;
> ..
> > + reg &= ~((1 << 11) - 1);
> so this is a mask? use macro to define, instead of using magic number
Yes, this is a mask to init SPI Clock Divider, I will add a macro, thanks.
> > + reg |= 1;
> > + writel(reg, spifmc->io_base + SPIFMC_CTRL);
> > +
> > + writel(0, spifmc->io_base + SPIFMC_CE_CTRL);
> > +
> > + tran_csr = readl(spifmc->io_base + SPIFMC_TRAN_CSR);
> > + tran_csr |= (0 << SPIFMC_TRAN_CSR_ADDR_BYTES_SHIFT);
> > + tran_csr |= SPIFMC_TRAN_CSR_FIFO_TRG_LVL_4_BYTE;
> > + tran_csr |= SPIFMC_TRAN_CSR_WITH_CMD;
> > + writel(tran_csr, spifmc->io_base + SPIFMC_TRAN_CSR);
> > +}
> > +
> > +static int sophgo_spifmc_probe(struct platform_device *pdev)
> > +{
> > + struct spi_controller *ctrl;
> > + struct sophgo_spifmc *spifmc;
> > + void __iomem *base;
> > + int ret;
> > +
> > + ctrl = devm_spi_alloc_host(&pdev->dev, sizeof(*spifmc));
> > + if (!ctrl)
> > + return -ENOMEM;
> > +
> > + spifmc = spi_controller_get_devdata(ctrl);
> > + dev_set_drvdata(&pdev->dev, ctrl);
> > +
> ..
> > + spifmc->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(spifmc->clk)) {
> > + dev_err(&pdev->dev, "AHB clock not found.\n");
> > + return PTR_ERR(spifmc->clk);
> > + }
> > +
> > + ret = clk_prepare_enable(spifmc->clk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Unable to enable AHB clock.\n");
> > + return ret;
> > + }
> you can combine above with devm_clk_get_enabled(), and simplify
> return routine by using "return dev_err_probe(..)"
>
Thanks, I will modify it.
> > +
> > + spifmc->dev = &pdev->dev;
> > + spifmc->ctrl = ctrl;
> > +
> > + spifmc->io_base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(base))
> > + return PTR_ERR(base);
> > +
> > + ctrl->num_chipselect = 1;
> > + ctrl->dev.of_node = pdev->dev.of_node;
> > + ctrl->bits_per_word_mask = SPI_BPW_MASK(8);
> > + ctrl->auto_runtime_pm = false;
> > + ctrl->mem_ops = &sophgo_spifmc_mem_ops;
> > + ctrl->mode_bits = SPI_RX_DUAL | SPI_TX_DUAL | SPI_RX_QUAD | SPI_TX_QUAD;
> > +
> > + mutex_init(&spifmc->lock);
> strictly, you still need to do error handler, e.g, destroy mutex if probe fail
Thanks, I will add it.
> > +
> > + sophgo_spifmc_init(spifmc);
> > + sophgo_spifmc_init_reg(spifmc);
> > +
> > + return devm_spi_register_controller(&pdev->dev, ctrl);
> > +}
> > +
> > [...]
>
> --
> Yixun Lan (dlan)
> Gentoo Linux Developer
> GPG Key ID AABEFD55
