Re: [PATCH v2 13/19] spi: dw: Move Non-DMA code to the DW PCIe-SPI driver

From: Andy Shevchenko
Date: Fri May 15 2020 - 10:52:02 EST


On Fri, May 15, 2020 at 01:47:52PM +0300, Serge Semin wrote:
> This is a preparation patch before adding the DW DMA support into the
> DW SPI MMIO driver. We need to unpin the Non-DMA-specific code from the
> intended to be generic DW APB SSI DMA code. This isn't that hard,
> since the most part of the spi-dw-mid.c driver in fact implements a
> generic DMA interface for the DW SPI controller driver. The only Intel
> MID specifics concern getting the max frequency from the MRST Clock
> Control Unit and fetching the DMA controller channels from
> corresponding PCIe DMA controller. Since first one is related with the
> SPI interface configuration we moved it' implementation into the
> DW PCIe-SPI driver module. After that former spi-dw-mid.c file
> can be just renamed to be the DW SPI DMA module optionally compiled in to
> the DW APB SSI core driver.

Cc list here is huge!

I think this patch should go immediately after bunch of fixes.

...

> -obj-$(CONFIG_SPI_DESIGNWARE) += spi-dw.o
> +obj-$(CONFIG_SPI_DESIGNWARE) += spi-dw-core.o
> +spi-dw-core-y := spi-dw.o
> +spi-dw-core-$(CONFIG_SPI_DW_DMA) += spi-dw-dma.o

We may leave module name the same, right?

obj-$(CONFIG_SPI_DESIGNWARE) += spi-dw.o
spi-dw-y := spi-dw-core.o
spi-dw-$(CONFIG_SPI_DW_DMA) += spi-dw-dma.o

> obj-$(CONFIG_SPI_DW_MMIO) += spi-dw-mmio.o
> -obj-$(CONFIG_SPI_DW_PCI) += spi-dw-midpci.o
> -spi-dw-midpci-objs := spi-dw-pci.o spi-dw-mid.o
> +obj-$(CONFIG_SPI_DW_PCI) += spi-dw-pci.o

> -/* Some specific info for SPI0 controller on Intel MID */
> -
> -/* HW info for MRST Clk Control Unit, 32b reg per controller */
> -#define MRST_SPI_CLK_BASE 100000000 /* 100m */
> -#define MRST_CLK_SPI_REG 0xff11d86c
> -#define CLK_SPI_BDIV_OFFSET 0
> -#define CLK_SPI_BDIV_MASK 0x00000007
> -#define CLK_SPI_CDIV_OFFSET 9
> -#define CLK_SPI_CDIV_MASK 0x00000e00
> -#define CLK_SPI_DISABLE_OFFSET 8
> -
> -int dw_spi_mid_init_mfld(struct dw_spi *dws)
> -{
> - void __iomem *clk_reg;
> - u32 clk_cdiv;
> -
> - clk_reg = ioremap(MRST_CLK_SPI_REG, 16);
> - if (!clk_reg)
> - return -ENOMEM;
> -
> - /* Get SPI controller operating freq info */
> - clk_cdiv = readl(clk_reg + dws->bus_num * sizeof(u32));
> - clk_cdiv &= CLK_SPI_CDIV_MASK;
> - clk_cdiv >>= CLK_SPI_CDIV_OFFSET;
> - dws->max_freq = MRST_SPI_CLK_BASE / (clk_cdiv + 1);
> -
> - iounmap(clk_reg);
> -
> - /* Register hook to configure CTRLR0 */
> - dws->update_cr0 = dw_spi_update_cr0;
> -
> - dw_spi_mid_setup_dma_mfld(dws);
> - return 0;
> -}
> -
> -int dw_spi_mid_init_generic(struct dw_spi *dws)
> -{
> - /* Register hook to configure CTRLR0 */
> - dws->update_cr0 = dw_spi_update_cr0;
> -
> - dw_spi_mid_setup_dma_generic(dws);
> - return 0;
> -}
> +EXPORT_SYMBOL_GPL(dw_spi_mid_setup_dma_generic);
> diff --git a/drivers/spi/spi-dw-pci.c b/drivers/spi/spi-dw-pci.c
> index dde54a918b5d..c13707b8493e 100644
> --- a/drivers/spi/spi-dw-pci.c
> +++ b/drivers/spi/spi-dw-pci.c
> @@ -15,6 +15,15 @@
>
> #define DRIVER_NAME "dw_spi_pci"
>
> +/* HW info for MRST Clk Control Unit, 32b reg per controller */
> +#define MRST_SPI_CLK_BASE 100000000 /* 100m */
> +#define MRST_CLK_SPI_REG 0xff11d86c
> +#define CLK_SPI_BDIV_OFFSET 0
> +#define CLK_SPI_BDIV_MASK 0x00000007
> +#define CLK_SPI_CDIV_OFFSET 9
> +#define CLK_SPI_CDIV_MASK 0x00000e00
> +#define CLK_SPI_DISABLE_OFFSET 8
> +
> struct spi_pci_desc {
> int (*setup)(struct dw_spi *);
> u16 num_cs;
> @@ -22,20 +31,55 @@ struct spi_pci_desc {
> u32 max_freq;
> };
>
> +static int spi_mid_init(struct dw_spi *dws)
> +{
> + void __iomem *clk_reg;
> + u32 clk_cdiv;
> +
> + clk_reg = ioremap(MRST_CLK_SPI_REG, 16);
> + if (!clk_reg)
> + return -ENOMEM;
> +
> + /* Get SPI controller operating freq info */
> + clk_cdiv = readl(clk_reg + dws->bus_num * sizeof(u32));
> + clk_cdiv &= CLK_SPI_CDIV_MASK;
> + clk_cdiv >>= CLK_SPI_CDIV_OFFSET;
> + dws->max_freq = MRST_SPI_CLK_BASE / (clk_cdiv + 1);
> +
> + iounmap(clk_reg);
> +
> + /* Register hook to configure CTRLR0 */
> + dws->update_cr0 = dw_spi_update_cr0;
> +
> + dw_spi_mid_setup_dma_mfld(dws);
> +
> + return 0;
> +}
> +
> +static int spi_generic_init(struct dw_spi *dws)
> +{
> + /* Register hook to configure CTRLR0 */
> + dws->update_cr0 = dw_spi_update_cr0;
> +
> + dw_spi_mid_setup_dma_generic(dws);
> +
> + return 0;
> +}
> +
> static struct spi_pci_desc spi_pci_mid_desc_1 = {
> - .setup = dw_spi_mid_init_mfld,
> + .setup = spi_mid_init,
> .num_cs = 5,
> .bus_num = 0,
> };
>
> static struct spi_pci_desc spi_pci_mid_desc_2 = {
> - .setup = dw_spi_mid_init_mfld,
> + .setup = spi_mid_init,
> .num_cs = 2,
> .bus_num = 1,
> };
>
> static struct spi_pci_desc spi_pci_ehl_desc = {
> - .setup = dw_spi_mid_init_generic,
> + .setup = spi_generic_init,
> .num_cs = 2,
> .bus_num = -1,
> .max_freq = 100000000,
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index d0c8b7d3a5d2..75fdcc5e7642 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -265,8 +265,16 @@ extern u32 dw_spi_update_cr0_v1_01a(struct spi_controller *master,
> struct spi_device *spi,
> struct spi_transfer *transfer);
>
> -/* platform related setup */
> -extern int dw_spi_mid_init_mfld(struct dw_spi *dws);
> -extern int dw_spi_mid_init_generic(struct dw_spi *dws);
> +#ifdef CONFIG_SPI_DW_DMA
> +
> +extern void dw_spi_mid_setup_dma_mfld(struct dw_spi *dws);
> +extern void dw_spi_mid_setup_dma_generic(struct dw_spi *dws);
> +

I would drop blank lines and extern keywords.

> +#else

> +
> +static inline void dw_spi_mid_setup_dma_mfld(struct dw_spi *dws) {}
> +static inline void dw_spi_mid_setup_dma_generic(struct dw_spi *dws) {}
> +

Ditto for blank lines.

> +#endif /* !CONFIG_SPI_DW_DMA */
>
> #endif /* DW_SPI_HEADER_H */
> --
> 2.25.1
>

--
With Best Regards,
Andy Shevchenko