[0/3] Add SFC support for Ingenic SoCs.

1.Use the spi-mem poll status APIs in SPI-NOR to reduce CPU load.
2.Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC from Ingenic.

Liu Jinghui and Aidan MacDonald provided a lot of assistance during the development of this driver.

周琰杰 (Zhou Yanjie) (3):
  mtd: spi-nor: Use the spi-mem poll status APIs.
  dt-bindings: SPI: Add Ingenic SFC bindings.
  SPI: Ingenic: Add SFC support for Ingenic SoCs.

 .../devicetree/bindings/spi/ingenic,sfc.yaml       |  64 ++
 drivers/mtd/spi-nor/core.c                         |  42 +-
 drivers/spi/Kconfig                                |   9 +
 drivers/spi/Makefile                               |   1 +
 drivers/spi/spi-ingenic-sfc.c                      | 662 +++++++++++++++++++++
 5 files changed, 768 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
 create mode 100755 drivers/spi/spi-ingenic-sfc.c

On 22/07/2022 18:48, 周琰杰 (Zhou Yanjie) wrote:
> Add the SFC bindings for the X1000 SoC, the X1600 SoC, the X1830 SoC,
> and the X2000 SoC from Ingenic.
> 
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> ---
>  .../devicetree/bindings/spi/ingenic,sfc.yaml       | 64 ++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
> new file mode 100644
> index 00000000..b7c4cf4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
> @@ -0,0 +1,64 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/ingenic,sfc.yaml#

File name should be rather based on first compatible, so
ingenic,x1000-sfc.yaml

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Bindings for SFC in Ingenic SoCs
> +
> +maintainers:
> +  - 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> +
> +description:
> +  The SPI Flash Controller in Ingenic SoCs.
> +

You miss here allOf referencing spi-controller.

> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - ingenic,x1000-sfc
> +          - ingenic,x1600-sfc
> +          - ingenic,x2000-sfc
> +      - items:
> +          - enum:
> +              - ingenic,x1830-sfc
> +          - const: ingenic,x1000-sfc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: sfc

Remove the clock-names entirely, no benefits.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/ingenic,x1000-cgu.h>
> +
> +	sfc: spi@13440000 {
> +		compatible = "ingenic,x1000-sfc";


Use 4 spaces for example indentation.

> +		reg = <0x13440000 0x1000>;
> +
> +		interrupt-parent = <&intc>;
> +		interrupts = <7>;
> +
> +		clocks = <&cgu X1000_CLK_SFC>;
> +		clock-names = "sfc";
> +
> +		status = "disabled";

No status in example.

> +	};
> +...


Best regards,
Krzysztof

On 22/07/2022 18:48, 周琰杰 (Zhou Yanjie) wrote:
> Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC
> from Ingenic.
> 
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> ---
>  drivers/spi/Kconfig           |   9 +
>  drivers/spi/Makefile          |   1 +
>  drivers/spi/spi-ingenic-sfc.c | 662 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 672 insertions(+)
>  create mode 100644 drivers/spi/spi-ingenic-sfc.c
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 3b1044e..1077bd3 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -437,6 +437,15 @@ config SPI_INGENIC
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called spi-ingenic.
>  
> +config SPI_INGENIC_SFC
> +	tristate "Ingenic SoCs SPI Flash Controller"
> +	depends on MACH_INGENIC || COMPILE_TEST
> +	help
> +	  This enables support for the Ingenic SoCs SPI flash controller.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called ingenic-sfc.
> +
>  config SPI_INTEL
>  	tristate
>  
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 0f44eb6..f3e42c0 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_SPI_HISI_SFC_V3XX)		+= spi-hisi-sfc-v3xx.o
>  obj-$(CONFIG_SPI_IMG_SPFI)		+= spi-img-spfi.o
>  obj-$(CONFIG_SPI_IMX)			+= spi-imx.o
>  obj-$(CONFIG_SPI_INGENIC)		+= spi-ingenic.o
> +obj-$(CONFIG_SPI_INGENIC_SFC)	+= spi-ingenic-sfc.o
>  obj-$(CONFIG_SPI_INTEL)			+= spi-intel.o
>  obj-$(CONFIG_SPI_INTEL_PCI)		+= spi-intel-pci.o
>  obj-$(CONFIG_SPI_INTEL_PLATFORM)	+= spi-intel-platform.o
> diff --git a/drivers/spi/spi-ingenic-sfc.c b/drivers/spi/spi-ingenic-sfc.c
> new file mode 100644
> index 00000000..a565546
> --- /dev/null
> +++ b/drivers/spi/spi-ingenic-sfc.c
> @@ -0,0 +1,662 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Ingenic SoCs SPI Flash Controller Driver
> + * Copyright (c) 2022 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +
> +/* SFC register offsets */
> +#define SFC_REG_GLB						0x0000
> +#define SFC_REG_DEV_CONF				0x0004
> +#define SFC_REG_DEV_STA_EXP				0x0008
> +#define SFC_REG_DEV_STA_RT				0x000c
> +#define SFC_REG_DEV_STA_MSK				0x0010
> +#define SFC_REG_TRAN_CONF(n)			(0x0014 + n * 4)
> +#define SFC_REG_TRAN_CFG0(n)			(0x0014 + n * 4)
> +#define SFC_REG_TRAN_LEN				0x002c
> +#define SFC_REG_DEV_ADDR(n)				(0x0030 + n * 4)
> +#define SFC_REG_DEV_ADDR_PLUS(n)		(0x0048 + n * 4)
> +#define SFC_REG_MEM_ADDR				0x0060
> +#define SFC_REG_TRIG					0x0064
> +#define SFC_REG_SR						0x0068
> +#define SFC_REG_SCR						0x006c
> +#define SFC_REG_INTC					0x0070
> +#define SFC_REG_FSM						0x0074
> +#define SFC_REG_CGE						0x0078
> +#define SFC_REG_TRAN_CFG1(n)			(0x009c + n * 4)
> +#define SFC_REG_DR						0x1000
> +
> +/* bits within the GLB register */
> +#define GLB_TRAN_DIR_MASK				GENMASK(13, 13)
> +#define GLB_TRAN_DIR_WRITE				0x1
> +#define GLB_TRAN_DIR_READ				0x0
> +#define GLB_THRESHOLD_MASK				GENMASK(12, 7)
> +#define GLB_OP_MODE_MASK				GENMASK(6, 6)
> +#define GLB_OP_MODE_DMA					0x1
> +#define GLB_OP_MODE_SLAVE				0x0
> +#define GLB_PHASE_NUM_MASK				GENMASK(5, 3)
> +#define GLB_WP_EN						BIT(2)
> +#define GLB_BURST_MD_MASK				GENMASK(1, 0)
> +#define GLB_BURST_MD_INCR32				0x3
> +#define GLB_BURST_MD_INCR16				0x2
> +#define GLB_BURST_MD_INCR8				0x1
> +#define GLB_BURST_MD_INCR4				0x0
> +
> +/* bits within the DEV_CONF register */
> +#define DEV_CONF_SMP_DELAY_MASK			GENMASK(20, 16)
> +#define DEV_CONF_SMP_DELAY_180DEG		0x4
> +#define DEV_CONF_SMP_DELAY_HALF_CYCLE	0x1
> +#define DEV_CONF_CMD_TYPE_MASK			GENMASK(15, 15)
> +#define DEV_CONF_CMD_TYPE_16BIT			0x1
> +#define DEV_CONF_CMD_TYPE_8BIT			0x0
> +#define DEV_CONF_STA_TYPE_MASK			GENMASK(14, 13)
> +#define DEV_CONF_THOLD_MASK				GENMASK(12, 11)
> +#define DEV_CONF_TSETUP_MASK			GENMASK(10, 9)
> +#define DEV_CONF_TSH_MASK				GENMASK(8, 5)
> +#define DEV_CONF_CPHA					BIT(4)
> +#define DEV_CONF_CPOL					BIT(3)
> +#define DEV_CONF_CE_DL					BIT(2)
> +#define DEV_CONF_HOLD_DL				BIT(1)
> +#define DEV_CONF_WP_DL					BIT(0)
> +
> +/* bits within the TRAN_CONF(n) register */
> +#define TRAN_CONF_TRAN_MODE_MASK		GENMASK(31, 29)
> +#define TRAN_CONF_ADDR_WIDTH_MASK		GENMASK(28, 26)
> +#define TRAN_CONF_POLL_EN				BIT(25)
> +#define TRAN_CONF_CMD_EN				BIT(24)
> +#define TRAN_CONF_PHASE_FORMAT_MASK		GENMASK(23, 23)
> +#define TRAN_CONF_DMY_BITS_MASK			GENMASK(22, 17)
> +#define TRAN_CONF_DATA_EN				BIT(16)
> +#define TRAN_CONF_CMD_MASK				GENMASK(15, 0)
> +
> +/* bits within the TRIG register */
> +#define TRIG_FLUSH						BIT(2)
> +#define TRIG_STOP						BIT(1)
> +#define TRIG_START						BIT(0)
> +
> +/* bits within the SR register */
> +#define SR_FIFO_NUM_MASK				GENMASK(22, 16)
> +#define SR_END							BIT(4)
> +#define SR_TRAN_REQ						BIT(3)
> +#define SR_RECE_REQ						BIT(2)
> +#define SR_OVER							BIT(1)
> +#define SR_UNDER						BIT(0)
> +
> +/* bits within the SCR register */
> +#define SCR_CLR_END						BIT(4)
> +#define SCR_CLR_TREQ					BIT(3)
> +#define SCR_CLR_RREQ					BIT(2)
> +#define SCR_CLR_OVER					BIT(1)
> +#define SCR_CLR_UNDER					BIT(0)
> +
> +/* bits within the INTC register */
> +#define INTC_MASK_END					BIT(4)
> +#define INTC_MASK_TREQ					BIT(3)
> +#define INTC_MASK_RREQ					BIT(2)
> +#define INTC_MASK_OVER					BIT(1)
> +#define INTC_MASK_UNDER					BIT(0)
> +
> +/* bits within the TRAN_CFG1(n) register */
> +#define TRAN_CFG1_TRAN_MODE_MASK		GENMASK(7, 4)
> +
> +#define TRAN_MODE_STANDARD				0
> +#define TRAN_MODE_DUAL_DATA				1
> +#define TRAN_MODE_DUAL_IO				2
> +#define TRAN_MODE_DUAL_FULL				3
> +#define TRAN_MODE_QUAD_DATA				5
> +#define TRAN_MODE_QUAD_IO				6
> +#define TRAN_MODE_QUAD_FULL				7
> +#define TRAN_MODE_OCTAL_DATA			9
> +#define TRAN_MODE_OCTAL_IO				10
> +#define TRAN_MODE_OCTAL_FULL			11
> +
> +#define INGENIC_SFC_FIFO_SIZE			(64 * 4)
> +
> +#define INGENIC_SFC_TRANSFER_TIMEOUT	1000
> +
> +enum ingenic_sfc_version {
> +	ID_X1000,
> +	ID_X1600,
> +	ID_X2000,
> +};
> +
> +struct ingenic_soc_info {
> +	enum ingenic_sfc_version version;
> +

No need for blank line.

> +	unsigned int max_bus_width;
> +

Remove as well.

> +	const u32 tran_mode_mask;
> +};
> +
> +struct ingenic_sfc {
> +	const struct ingenic_soc_info *soc_info;
> +
> +	void __iomem *base;
> +	struct device *dev;
> +	struct clk *clk;
> +	int irq;
> +
> +	struct completion completion;
> +};
> +
> +static irqreturn_t ingenic_sfc_irq_handler(int irq, void *data)
> +{
> +	struct ingenic_sfc *sfc = data;
> +
> +	writel(0x1f, sfc->base + SFC_REG_INTC);
> +
> +	complete(&sfc->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ingenic_sfc_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> +{
> +	uintptr_t addr = (uintptr_t)op->data.buf.in;
> +
> +	if (op->data.nbytes > INGENIC_SFC_FIFO_SIZE && !IS_ALIGNED(addr, 4))
> +		op->data.nbytes = INGENIC_SFC_FIFO_SIZE;
> +
> +	return 0;
> +}
> +
> +static bool ingenic_sfc_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +	struct spi_device *spi = mem->spi;
> +	struct ingenic_sfc *sfc = spi_controller_get_devdata(spi->master);
> +	uintptr_t addr = (uintptr_t)op->data.buf.in;
> +
> +	/* The controller only supports Standard SPI mode, Duall mode, Quad mode and Octal mode. */
> +	if (op->cmd.buswidth > sfc->soc_info->max_bus_width ||
> +		op->addr.buswidth > sfc->soc_info->max_bus_width ||
> +		op->dummy.buswidth > sfc->soc_info->max_bus_width ||
> +		op->data.buswidth > sfc->soc_info->max_bus_width)
> +		return false;
> +
> +	/* Max 32 dummy clock cycles supported */
> +	if (op->dummy.nbytes && op->dummy.nbytes * 8 / op->dummy.buswidth > 32)
> +		return false;
> +
> +	/* Max rx data length, check controller limits and alignment */
> +	if (op->data.dir == SPI_MEM_DATA_IN &&
> +		op->data.nbytes > INGENIC_SFC_FIFO_SIZE && !IS_ALIGNED(addr, 4))
> +		return false;
> +
> +	/* Max 6 bytes address width supported */
> +	if (op->addr.nbytes > 6)
> +		return false;
> +
> +	return spi_mem_default_supports_op(mem, op);
> +}
> +
> +static void ingenic_sfc_set_transfer_mode(struct ingenic_sfc *sfc, const struct spi_mem_op *op)
> +{
> +	int val;
> +
> +	val = readl(sfc->base + (sfc->soc_info->version >= ID_X1600 ?
> +			SFC_REG_TRAN_CFG1(0) : SFC_REG_TRAN_CONF(0)));

This is no really readable.

> +	val &= ~sfc->soc_info->tran_mode_mask;
> +	if (op->cmd.buswidth == 8)
> +		val |= (TRAN_MODE_OCTAL_FULL << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
> +				sfc->soc_info->tran_mode_mask;
> +	else if (op->cmd.buswidth == 4)
> +		val |= (TRAN_MODE_QUAD_FULL << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
> +				sfc->soc_info->tran_mode_mask;
> +	else if (op->cmd.buswidth == 2)
> +		val |= (TRAN_MODE_DUAL_FULL << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
> +				sfc->soc_info->tran_mode_mask;
> +	else if (op->addr.buswidth == 8)
> +		val |= (TRAN_MODE_OCTAL_IO << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
> +				sfc->soc_info->tran_mode_mask;
> +	else if (op->addr.buswidth == 4)
> +		val |= (TRAN_MODE_QUAD_IO << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
> +				sfc->soc_info->tran_mode_mask;
> +	else if (op->addr.buswidth == 2)
> +		val |= (TRAN_MODE_DUAL_IO << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
> +				sfc->soc_info->tran_mode_mask;
> +	else if (op->data.buswidth == 8)
> +		val |= (TRAN_MODE_OCTAL_DATA << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
> +				sfc->soc_info->tran_mode_mask;
> +	else if (op->data.buswidth == 4)
> +		val |= (TRAN_MODE_QUAD_DATA << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
> +				sfc->soc_info->tran_mode_mask;
> +	else if (op->data.buswidth == 2)
> +		val |= (TRAN_MODE_DUAL_DATA << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
> +				sfc->soc_info->tran_mode_mask;
> +	else
> +		val |= (TRAN_MODE_STANDARD << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
> +				sfc->soc_info->tran_mode_mask;
> +	writel(val, sfc->base + (sfc->soc_info->version >= ID_X1600 ?
> +			SFC_REG_TRAN_CFG1(0) : SFC_REG_TRAN_CONF(0)));

Not readable.

> +}
> +
> +/*
> + * We only need PIO mode to handle the SPI_MEM_NO_DATA transfers,
> + * and the unaligned accesses in SPI_MEM_DATA_IN transfers.
> + */

(...)

> +	sfc->dev = &pdev->dev;
> +
> +	platform_set_drvdata(pdev, sfc);
> +
> +	ret = devm_request_irq(&pdev->dev, sfc->irq, ingenic_sfc_irq_handler, 0,
> +			dev_name(&pdev->dev), sfc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request irq%d, ret = %d\n", sfc->irq, ret);
> +		goto err_put_master;
> +	}
> +
> +	ctlr->bus_num = -1;
> +	ctlr->num_chipselect = 1;
> +	ctlr->mem_ops = &ingenic_sfc_mem_ops;
> +	ctlr->dev.of_node = pdev->dev.of_node;
> +	ctlr->setup = ingenic_sfc_setup;
> +	ctlr->mode_bits = SPI_CPHA | SPI_CPOL |
> +			SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD;
> +	if (sfc->soc_info->version >= ID_X2000)
> +		ctlr->mode_bits |= SPI_RX_OCTAL | SPI_TX_OCTAL;
> +
> +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
> +	if (ret)
> +		goto err_put_master;
> +
> +	return 0;
> +
> +err_put_master:
> +	spi_master_put(ctlr);

What about unpreparing clocks? Is it part of spi_master_put()?

> +
> +	return ret;
> +}
> +
> +static const struct ingenic_soc_info x1000_soc_info = {
> +	.version = ID_X1000,
> +

No need for blank lines after each field.


Best regards,
Krzysztof

W dniu 22.07.2022 o 18:48, 周琰杰 (Zhou Yanjie) pisze:
> 1.Use the spi-mem poll status APIs in SPI-NOR to reduce CPU load.
> 2.Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC from Ingenic.
> 
> Liu Jinghui and Aidan MacDonald provided a lot of assistance during the development of this driver.
> 
> 周琰杰 (Zhou Yanjie) (3):
>   mtd: spi-nor: Use the spi-mem poll status APIs.
>   dt-bindings: SPI: Add Ingenic SFC bindings.
>   SPI: Ingenic: Add SFC support for Ingenic SoCs.
> 
>  .../devicetree/bindings/spi/ingenic,sfc.yaml       |  64 ++
>  drivers/mtd/spi-nor/core.c                         |  42 +-
>  drivers/spi/Kconfig                                |   9 +
>  drivers/spi/Makefile                               |   1 +
>  drivers/spi/spi-ingenic-sfc.c                      | 662 +++++++++++++++++++++
>  5 files changed, 768 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>  create mode 100755 drivers/spi/spi-ingenic-sfc.c
> 

Even tough it's still early in revision process, I'll add my
Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>

The test was performed with Damai DM6291A SoC which is a Ingenic X1000 IP
but with 256 MiB RAM. No bugs yet observed on my side.

Le 22/07/2022 à 18:48, 周琰杰 (Zhou Yanjie) a écrit :
> Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC
> from Ingenic.
> 
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> ---
>   drivers/spi/Kconfig           |   9 +
>   drivers/spi/Makefile          |   1 +
>   drivers/spi/spi-ingenic-sfc.c | 662 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 672 insertions(+)
>   create mode 100644 drivers/spi/spi-ingenic-sfc.c
> 

[...]

> +static int ingenic_sfc_probe(struct platform_device *pdev)
> +{
> +	struct ingenic_sfc *sfc;
> +	struct spi_controller *ctlr;
> +	int ret;
> +
> +	ctlr = spi_alloc_master(&pdev->dev, sizeof(*sfc));
> +	if (!ctlr)
> +		return -ENOMEM;

devm_spi_alloc_master()? (+ error handling simplification)
Or there should be a .remove() function.

CJ

> +
> +	sfc = spi_controller_get_devdata(ctlr);
> +
> +	sfc->soc_info = of_device_get_match_data(&pdev->dev);
> +	if (!sfc->soc_info) {
> +		dev_err(&pdev->dev, "No of match data provided\n");
> +		ret = -ENODEV;
> +		goto err_put_master;
> +	}
> +

[...]

Hi Krzysztof,

On 2022/7/23 上午1:46, Krzysztof Kozlowski wrote:
> On 22/07/2022 18:48, 周琰杰 (Zhou Yanjie) wrote:
>> Add the SFC bindings for the X1000 SoC, the X1600 SoC, the X1830 SoC,
>> and the X2000 SoC from Ingenic.
>>
>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> ---
>>   .../devicetree/bindings/spi/ingenic,sfc.yaml       | 64 ++++++++++++++++++++++
>>   1 file changed, 64 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>> new file mode 100644
>> index 00000000..b7c4cf4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>> @@ -0,0 +1,64 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/spi/ingenic,sfc.yaml#
> File name should be rather based on first compatible, so
> ingenic,x1000-sfc.yaml


No offense, does it really need to be named that way?
I can't seem to find documentation with instructions on this :(

The use of "ingenic,sfc.yaml" indicates that this is the documentation
for the SFC module for all Ingenic SoCs, without misleading people into
thinking it's only for a specific model of SoC. And there seem to be many
other yaml documents that use similar names (eg. fsl,spi-fsl-qspi.yaml,
spi-rockchip.yaml, spi-nxp-fspi.yaml, ingenic,spi.yaml, spi-sifive.yaml,
omap-spi.yaml), maybe these yaml files that are not named with first
compatible are also for the same consideration. :)


>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Bindings for SFC in Ingenic SoCs
>> +
>> +maintainers:
>> +  - 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> +
>> +description:
>> +  The SPI Flash Controller in Ingenic SoCs.
>> +
> You miss here allOf referencing spi-controller.


Sure, will add it in the next version.


>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - enum:
>> +          - ingenic,x1000-sfc
>> +          - ingenic,x1600-sfc
>> +          - ingenic,x2000-sfc
>> +      - items:
>> +          - enum:
>> +              - ingenic,x1830-sfc
>> +          - const: ingenic,x1000-sfc
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  clock-names:
>> +    const: sfc
> Remove the clock-names entirely, no benefits.


Sure.


>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/ingenic,x1000-cgu.h>
>> +
>> +	sfc: spi@13440000 {
>> +		compatible = "ingenic,x1000-sfc";
>
> Use 4 spaces for example indentation.


My fault, will fix it in the next version.


>> +		reg = <0x13440000 0x1000>;
>> +
>> +		interrupt-parent = <&intc>;
>> +		interrupts = <7>;
>> +
>> +		clocks = <&cgu X1000_CLK_SFC>;
>> +		clock-names = "sfc";
>> +
>> +		status = "disabled";
> No status in example.


Sure, will remove it in the next version.


Thanks and beset regards!


>
>> +	};
>> +...
>
> Best regards,
> Krzysztof

Hi Krzysztof,

On 2022/7/23 上午2:07, Krzysztof Kozlowski wrote:
> On 22/07/2022 18:48, 周琰杰 (Zhou Yanjie) wrote:
>> Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC
>> from Ingenic.
>>
>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> ---
>>   drivers/spi/Kconfig           |   9 +
>>   drivers/spi/Makefile          |   1 +
>>   drivers/spi/spi-ingenic-sfc.c | 662 ++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 672 insertions(+)
>>   create mode 100644 drivers/spi/spi-ingenic-sfc.c
>>
>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> index 3b1044e..1077bd3 100644
>> --- a/drivers/spi/Kconfig
>> +++ b/drivers/spi/Kconfig
>> @@ -437,6 +437,15 @@ config SPI_INGENIC
>>   	  To compile this driver as a module, choose M here: the module
>>   	  will be called spi-ingenic.
>>   
>> +config SPI_INGENIC_SFC
>> +	tristate "Ingenic SoCs SPI Flash Controller"
>> +	depends on MACH_INGENIC || COMPILE_TEST
>> +	help
>> +	  This enables support for the Ingenic SoCs SPI flash controller.
>> +
>> +	  To compile this driver as a module, choose M here: the module
>> +	  will be called ingenic-sfc.
>> +
>>   config SPI_INTEL
>>   	tristate
>>   
>> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
>> index 0f44eb6..f3e42c0 100644
>> --- a/drivers/spi/Makefile
>> +++ b/drivers/spi/Makefile
>> @@ -62,6 +62,7 @@ obj-$(CONFIG_SPI_HISI_SFC_V3XX)		+= spi-hisi-sfc-v3xx.o
>>   obj-$(CONFIG_SPI_IMG_SPFI)		+= spi-img-spfi.o
>>   obj-$(CONFIG_SPI_IMX)			+= spi-imx.o
>>   obj-$(CONFIG_SPI_INGENIC)		+= spi-ingenic.o
>> +obj-$(CONFIG_SPI_INGENIC_SFC)	+= spi-ingenic-sfc.o
>>   obj-$(CONFIG_SPI_INTEL)			+= spi-intel.o
>>   obj-$(CONFIG_SPI_INTEL_PCI)		+= spi-intel-pci.o
>>   obj-$(CONFIG_SPI_INTEL_PLATFORM)	+= spi-intel-platform.o
>> diff --git a/drivers/spi/spi-ingenic-sfc.c b/drivers/spi/spi-ingenic-sfc.c
>> new file mode 100644
>> index 00000000..a565546
>> --- /dev/null
>> +++ b/drivers/spi/spi-ingenic-sfc.c
>> @@ -0,0 +1,662 @@
>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +/*
>> + * Ingenic SoCs SPI Flash Controller Driver
>> + * Copyright (c) 2022 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/bitops.h>
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/spi/spi-mem.h>
>> +
>> +/* SFC register offsets */
>> +#define SFC_REG_GLB						0x0000
>> +#define SFC_REG_DEV_CONF				0x0004
>> +#define SFC_REG_DEV_STA_EXP				0x0008
>> +#define SFC_REG_DEV_STA_RT				0x000c
>> +#define SFC_REG_DEV_STA_MSK				0x0010
>> +#define SFC_REG_TRAN_CONF(n)			(0x0014 + n * 4)
>> +#define SFC_REG_TRAN_CFG0(n)			(0x0014 + n * 4)
>> +#define SFC_REG_TRAN_LEN				0x002c
>> +#define SFC_REG_DEV_ADDR(n)				(0x0030 + n * 4)
>> +#define SFC_REG_DEV_ADDR_PLUS(n)		(0x0048 + n * 4)
>> +#define SFC_REG_MEM_ADDR				0x0060
>> +#define SFC_REG_TRIG					0x0064
>> +#define SFC_REG_SR						0x0068
>> +#define SFC_REG_SCR						0x006c
>> +#define SFC_REG_INTC					0x0070
>> +#define SFC_REG_FSM						0x0074
>> +#define SFC_REG_CGE						0x0078
>> +#define SFC_REG_TRAN_CFG1(n)			(0x009c + n * 4)
>> +#define SFC_REG_DR						0x1000
>> +
>> +/* bits within the GLB register */
>> +#define GLB_TRAN_DIR_MASK				GENMASK(13, 13)
>> +#define GLB_TRAN_DIR_WRITE				0x1
>> +#define GLB_TRAN_DIR_READ				0x0
>> +#define GLB_THRESHOLD_MASK				GENMASK(12, 7)
>> +#define GLB_OP_MODE_MASK				GENMASK(6, 6)
>> +#define GLB_OP_MODE_DMA					0x1
>> +#define GLB_OP_MODE_SLAVE				0x0
>> +#define GLB_PHASE_NUM_MASK				GENMASK(5, 3)
>> +#define GLB_WP_EN						BIT(2)
>> +#define GLB_BURST_MD_MASK				GENMASK(1, 0)
>> +#define GLB_BURST_MD_INCR32				0x3
>> +#define GLB_BURST_MD_INCR16				0x2
>> +#define GLB_BURST_MD_INCR8				0x1
>> +#define GLB_BURST_MD_INCR4				0x0
>> +
>> +/* bits within the DEV_CONF register */
>> +#define DEV_CONF_SMP_DELAY_MASK			GENMASK(20, 16)
>> +#define DEV_CONF_SMP_DELAY_180DEG		0x4
>> +#define DEV_CONF_SMP_DELAY_HALF_CYCLE	0x1
>> +#define DEV_CONF_CMD_TYPE_MASK			GENMASK(15, 15)
>> +#define DEV_CONF_CMD_TYPE_16BIT			0x1
>> +#define DEV_CONF_CMD_TYPE_8BIT			0x0
>> +#define DEV_CONF_STA_TYPE_MASK			GENMASK(14, 13)
>> +#define DEV_CONF_THOLD_MASK				GENMASK(12, 11)
>> +#define DEV_CONF_TSETUP_MASK			GENMASK(10, 9)
>> +#define DEV_CONF_TSH_MASK				GENMASK(8, 5)
>> +#define DEV_CONF_CPHA					BIT(4)
>> +#define DEV_CONF_CPOL					BIT(3)
>> +#define DEV_CONF_CE_DL					BIT(2)
>> +#define DEV_CONF_HOLD_DL				BIT(1)
>> +#define DEV_CONF_WP_DL					BIT(0)
>> +
>> +/* bits within the TRAN_CONF(n) register */
>> +#define TRAN_CONF_TRAN_MODE_MASK		GENMASK(31, 29)
>> +#define TRAN_CONF_ADDR_WIDTH_MASK		GENMASK(28, 26)
>> +#define TRAN_CONF_POLL_EN				BIT(25)
>> +#define TRAN_CONF_CMD_EN				BIT(24)
>> +#define TRAN_CONF_PHASE_FORMAT_MASK		GENMASK(23, 23)
>> +#define TRAN_CONF_DMY_BITS_MASK			GENMASK(22, 17)
>> +#define TRAN_CONF_DATA_EN				BIT(16)
>> +#define TRAN_CONF_CMD_MASK				GENMASK(15, 0)
>> +
>> +/* bits within the TRIG register */
>> +#define TRIG_FLUSH						BIT(2)
>> +#define TRIG_STOP						BIT(1)
>> +#define TRIG_START						BIT(0)
>> +
>> +/* bits within the SR register */
>> +#define SR_FIFO_NUM_MASK				GENMASK(22, 16)
>> +#define SR_END							BIT(4)
>> +#define SR_TRAN_REQ						BIT(3)
>> +#define SR_RECE_REQ						BIT(2)
>> +#define SR_OVER							BIT(1)
>> +#define SR_UNDER						BIT(0)
>> +
>> +/* bits within the SCR register */
>> +#define SCR_CLR_END						BIT(4)
>> +#define SCR_CLR_TREQ					BIT(3)
>> +#define SCR_CLR_RREQ					BIT(2)
>> +#define SCR_CLR_OVER					BIT(1)
>> +#define SCR_CLR_UNDER					BIT(0)
>> +
>> +/* bits within the INTC register */
>> +#define INTC_MASK_END					BIT(4)
>> +#define INTC_MASK_TREQ					BIT(3)
>> +#define INTC_MASK_RREQ					BIT(2)
>> +#define INTC_MASK_OVER					BIT(1)
>> +#define INTC_MASK_UNDER					BIT(0)
>> +
>> +/* bits within the TRAN_CFG1(n) register */
>> +#define TRAN_CFG1_TRAN_MODE_MASK		GENMASK(7, 4)
>> +
>> +#define TRAN_MODE_STANDARD				0
>> +#define TRAN_MODE_DUAL_DATA				1
>> +#define TRAN_MODE_DUAL_IO				2
>> +#define TRAN_MODE_DUAL_FULL				3
>> +#define TRAN_MODE_QUAD_DATA				5
>> +#define TRAN_MODE_QUAD_IO				6
>> +#define TRAN_MODE_QUAD_FULL				7
>> +#define TRAN_MODE_OCTAL_DATA			9
>> +#define TRAN_MODE_OCTAL_IO				10
>> +#define TRAN_MODE_OCTAL_FULL			11
>> +
>> +#define INGENIC_SFC_FIFO_SIZE			(64 * 4)
>> +
>> +#define INGENIC_SFC_TRANSFER_TIMEOUT	1000
>> +
>> +enum ingenic_sfc_version {
>> +	ID_X1000,
>> +	ID_X1600,
>> +	ID_X2000,
>> +};
>> +
>> +struct ingenic_soc_info {
>> +	enum ingenic_sfc_version version;
>> +
> No need for blank line.
>
>> +	unsigned int max_bus_width;
>> +
> Remove as well.
>
>> +	const u32 tran_mode_mask;
>> +};
>> +
>> +struct ingenic_sfc {
>> +	const struct ingenic_soc_info *soc_info;
>> +
>> +	void __iomem *base;
>> +	struct device *dev;
>> +	struct clk *clk;
>> +	int irq;
>> +
>> +	struct completion completion;
>> +};
>> +
>> +static irqreturn_t ingenic_sfc_irq_handler(int irq, void *data)
>> +{
>> +	struct ingenic_sfc *sfc = data;
>> +
>> +	writel(0x1f, sfc->base + SFC_REG_INTC);
>> +
>> +	complete(&sfc->completion);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int ingenic_sfc_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
>> +{
>> +	uintptr_t addr = (uintptr_t)op->data.buf.in;
>> +
>> +	if (op->data.nbytes > INGENIC_SFC_FIFO_SIZE && !IS_ALIGNED(addr, 4))
>> +		op->data.nbytes = INGENIC_SFC_FIFO_SIZE;
>> +
>> +	return 0;
>> +}
>> +
>> +static bool ingenic_sfc_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
>> +{
>> +	struct spi_device *spi = mem->spi;
>> +	struct ingenic_sfc *sfc = spi_controller_get_devdata(spi->master);
>> +	uintptr_t addr = (uintptr_t)op->data.buf.in;
>> +
>> +	/* The controller only supports Standard SPI mode, Duall mode, Quad mode and Octal mode. */
>> +	if (op->cmd.buswidth > sfc->soc_info->max_bus_width ||
>> +		op->addr.buswidth > sfc->soc_info->max_bus_width ||
>> +		op->dummy.buswidth > sfc->soc_info->max_bus_width ||
>> +		op->data.buswidth > sfc->soc_info->max_bus_width)
>> +		return false;
>> +
>> +	/* Max 32 dummy clock cycles supported */
>> +	if (op->dummy.nbytes && op->dummy.nbytes * 8 / op->dummy.buswidth > 32)
>> +		return false;
>> +
>> +	/* Max rx data length, check controller limits and alignment */
>> +	if (op->data.dir == SPI_MEM_DATA_IN &&
>> +		op->data.nbytes > INGENIC_SFC_FIFO_SIZE && !IS_ALIGNED(addr, 4))
>> +		return false;
>> +
>> +	/* Max 6 bytes address width supported */
>> +	if (op->addr.nbytes > 6)
>> +		return false;
>> +
>> +	return spi_mem_default_supports_op(mem, op);
>> +}
>> +
>> +static void ingenic_sfc_set_transfer_mode(struct ingenic_sfc *sfc, const struct spi_mem_op *op)
>> +{
>> +	int val;
>> +
>> +	val = readl(sfc->base + (sfc->soc_info->version >= ID_X1600 ?
>> +			SFC_REG_TRAN_CFG1(0) : SFC_REG_TRAN_CONF(0)));
> This is no really readable.


Sure, I will change it in the next version.


>> +	val &= ~sfc->soc_info->tran_mode_mask;
>> +	if (op->cmd.buswidth == 8)
>> +		val |= (TRAN_MODE_OCTAL_FULL << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +				sfc->soc_info->tran_mode_mask;
>> +	else if (op->cmd.buswidth == 4)
>> +		val |= (TRAN_MODE_QUAD_FULL << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +				sfc->soc_info->tran_mode_mask;
>> +	else if (op->cmd.buswidth == 2)
>> +		val |= (TRAN_MODE_DUAL_FULL << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +				sfc->soc_info->tran_mode_mask;
>> +	else if (op->addr.buswidth == 8)
>> +		val |= (TRAN_MODE_OCTAL_IO << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +				sfc->soc_info->tran_mode_mask;
>> +	else if (op->addr.buswidth == 4)
>> +		val |= (TRAN_MODE_QUAD_IO << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +				sfc->soc_info->tran_mode_mask;
>> +	else if (op->addr.buswidth == 2)
>> +		val |= (TRAN_MODE_DUAL_IO << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +				sfc->soc_info->tran_mode_mask;
>> +	else if (op->data.buswidth == 8)
>> +		val |= (TRAN_MODE_OCTAL_DATA << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +				sfc->soc_info->tran_mode_mask;
>> +	else if (op->data.buswidth == 4)
>> +		val |= (TRAN_MODE_QUAD_DATA << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +				sfc->soc_info->tran_mode_mask;
>> +	else if (op->data.buswidth == 2)
>> +		val |= (TRAN_MODE_DUAL_DATA << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +				sfc->soc_info->tran_mode_mask;
>> +	else
>> +		val |= (TRAN_MODE_STANDARD << (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +				sfc->soc_info->tran_mode_mask;
>> +	writel(val, sfc->base + (sfc->soc_info->version >= ID_X1600 ?
>> +			SFC_REG_TRAN_CFG1(0) : SFC_REG_TRAN_CONF(0)));
> Not readable.


Will change it.


>
>> +}
>> +
>> +/*
>> + * We only need PIO mode to handle the SPI_MEM_NO_DATA transfers,
>> + * and the unaligned accesses in SPI_MEM_DATA_IN transfers.
>> + */
> (...)
>
>> +	sfc->dev = &pdev->dev;
>> +
>> +	platform_set_drvdata(pdev, sfc);
>> +
>> +	ret = devm_request_irq(&pdev->dev, sfc->irq, ingenic_sfc_irq_handler, 0,
>> +			dev_name(&pdev->dev), sfc);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Failed to request irq%d, ret = %d\n", sfc->irq, ret);
>> +		goto err_put_master;
>> +	}
>> +
>> +	ctlr->bus_num = -1;
>> +	ctlr->num_chipselect = 1;
>> +	ctlr->mem_ops = &ingenic_sfc_mem_ops;
>> +	ctlr->dev.of_node = pdev->dev.of_node;
>> +	ctlr->setup = ingenic_sfc_setup;
>> +	ctlr->mode_bits = SPI_CPHA | SPI_CPOL |
>> +			SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD;
>> +	if (sfc->soc_info->version >= ID_X2000)
>> +		ctlr->mode_bits |= SPI_RX_OCTAL | SPI_TX_OCTAL;
>> +
>> +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
>> +	if (ret)
>> +		goto err_put_master;
>> +
>> +	return 0;
>> +
>> +err_put_master:
>> +	spi_master_put(ctlr);
> What about unpreparing clocks? Is it part of spi_master_put()?


Will add it in the next version.


Thanks and beset regards!


>> +
>> +	return ret;
>> +}
>> +
>> +static const struct ingenic_soc_info x1000_soc_info = {
>> +	.version = ID_X1000,
>> +
> No need for blank lines after each field.
>
>
> Best regards,
> Krzysztof

Hi Paul,

On 2022/7/23 上午4:03, Paul Cercueil wrote:
> Hi Zhou,
>
>
> Le sam., juil. 23 2022 at 00:48:30 +0800, 周琰杰 (Zhou Yanjie) 
> <zhouyanjie@wanyeetech.com> a écrit :
>> Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC
>> from Ingenic.
>>
>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> ---
>>  drivers/spi/Kconfig           |   9 +
>>  drivers/spi/Makefile          |   1 +
>>  drivers/spi/spi-ingenic-sfc.c | 662 
>> ++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 672 insertions(+)
>>  create mode 100644 drivers/spi/spi-ingenic-sfc.c
>>
>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> index 3b1044e..1077bd3 100644
>> --- a/drivers/spi/Kconfig
>> +++ b/drivers/spi/Kconfig
>> @@ -437,6 +437,15 @@ config SPI_INGENIC
>>        To compile this driver as a module, choose M here: the module
>>        will be called spi-ingenic.
>>
>> +config SPI_INGENIC_SFC
>> +    tristate "Ingenic SoCs SPI Flash Controller"
>> +    depends on MACH_INGENIC || COMPILE_TEST
>> +    help
>> +      This enables support for the Ingenic SoCs SPI flash controller.
>> +
>> +      To compile this driver as a module, choose M here: the module
>> +      will be called ingenic-sfc.
>> +
>>  config SPI_INTEL
>>      tristate
>>
>> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
>> index 0f44eb6..f3e42c0 100644
>> --- a/drivers/spi/Makefile
>> +++ b/drivers/spi/Makefile
>> @@ -62,6 +62,7 @@ obj-$(CONFIG_SPI_HISI_SFC_V3XX)        += 
>> spi-hisi-sfc-v3xx.o
>>  obj-$(CONFIG_SPI_IMG_SPFI)        += spi-img-spfi.o
>>  obj-$(CONFIG_SPI_IMX)            += spi-imx.o
>>  obj-$(CONFIG_SPI_INGENIC)        += spi-ingenic.o
>> +obj-$(CONFIG_SPI_INGENIC_SFC)    += spi-ingenic-sfc.o
>>  obj-$(CONFIG_SPI_INTEL)            += spi-intel.o
>>  obj-$(CONFIG_SPI_INTEL_PCI)        += spi-intel-pci.o
>>  obj-$(CONFIG_SPI_INTEL_PLATFORM)    += spi-intel-platform.o
>> diff --git a/drivers/spi/spi-ingenic-sfc.c 
>> b/drivers/spi/spi-ingenic-sfc.c
>> new file mode 100644
>> index 00000000..a565546
>> --- /dev/null
>> +++ b/drivers/spi/spi-ingenic-sfc.c
>> @@ -0,0 +1,662 @@
>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>
> Dual-license driver? That's not what MODULE_LICENSE() says.
>

My fault, forgot to modify MODULE_LICENSE(), will fix it in the next 
version.


>> +/*
>> + * Ingenic SoCs SPI Flash Controller Driver
>> + * Copyright (c) 2022 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/bitops.h>
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/module.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/spi/spi-mem.h>
>> +
>> +/* SFC register offsets */
>> +#define SFC_REG_GLB                        0x0000
>> +#define SFC_REG_DEV_CONF                0x0004
>> +#define SFC_REG_DEV_STA_EXP                0x0008
>> +#define SFC_REG_DEV_STA_RT                0x000c
>> +#define SFC_REG_DEV_STA_MSK                0x0010
>> +#define SFC_REG_TRAN_CONF(n)            (0x0014 + n * 4)
>> +#define SFC_REG_TRAN_CFG0(n)            (0x0014 + n * 4)
>
> You should protect the macro parameter. If you do for instance 
> SFC_REG_TRAN_CONF(x + 1) it would resolve to (0x0014 + x + 1 * 4) 
> which is not what you'd want.
>

Sure, will fix it.


> Also - looks like SFC_REG_TRAN_CONF() and SFC_REG_TRAN_CFG0() are the 
> same thing, that's on purpose?
>
>> +#define SFC_REG_TRAN_LEN 0x002c
>> +#define SFC_REG_DEV_ADDR(n)                (0x0030 + n * 4)
>> +#define SFC_REG_DEV_ADDR_PLUS(n)        (0x0048 + n * 4)
>> +#define SFC_REG_MEM_ADDR                0x0060
>> +#define SFC_REG_TRIG                    0x0064
>> +#define SFC_REG_SR                        0x0068
>> +#define SFC_REG_SCR                        0x006c
>> +#define SFC_REG_INTC                    0x0070
>> +#define SFC_REG_FSM                        0x0074
>> +#define SFC_REG_CGE                        0x0078
>> +#define SFC_REG_TRAN_CFG1(n)            (0x009c + n * 4)
>> +#define SFC_REG_DR                        0x1000
>> +
>> +/* bits within the GLB register */
>> +#define GLB_TRAN_DIR_MASK                GENMASK(13, 13)
>> +#define GLB_TRAN_DIR_WRITE                0x1
>> +#define GLB_TRAN_DIR_READ                0x0
>
> When it's a single bit - just use BIT().


Sure.


>
>> +#define GLB_THRESHOLD_MASK GENMASK(12, 7)
>> +#define GLB_OP_MODE_MASK                GENMASK(6, 6)
>
> Same here, and I see it a few times below as well.
>

Sure.


>> +#define GLB_OP_MODE_DMA                    0x1
>> +#define GLB_OP_MODE_SLAVE                0x0
>> +#define GLB_PHASE_NUM_MASK                GENMASK(5, 3)
>> +#define GLB_WP_EN                        BIT(2)
>> +#define GLB_BURST_MD_MASK                GENMASK(1, 0)
>> +#define GLB_BURST_MD_INCR32                0x3
>> +#define GLB_BURST_MD_INCR16                0x2
>> +#define GLB_BURST_MD_INCR8                0x1
>> +#define GLB_BURST_MD_INCR4                0x0
>> +
>> +/* bits within the DEV_CONF register */
>> +#define DEV_CONF_SMP_DELAY_MASK            GENMASK(20, 16)
>> +#define DEV_CONF_SMP_DELAY_180DEG        0x4
>> +#define DEV_CONF_SMP_DELAY_HALF_CYCLE    0x1
>> +#define DEV_CONF_CMD_TYPE_MASK            GENMASK(15, 15)
>> +#define DEV_CONF_CMD_TYPE_16BIT            0x1
>> +#define DEV_CONF_CMD_TYPE_8BIT            0x0
>> +#define DEV_CONF_STA_TYPE_MASK            GENMASK(14, 13)
>> +#define DEV_CONF_THOLD_MASK                GENMASK(12, 11)
>> +#define DEV_CONF_TSETUP_MASK            GENMASK(10, 9)
>> +#define DEV_CONF_TSH_MASK                GENMASK(8, 5)
>> +#define DEV_CONF_CPHA                    BIT(4)
>> +#define DEV_CONF_CPOL                    BIT(3)
>> +#define DEV_CONF_CE_DL                    BIT(2)
>> +#define DEV_CONF_HOLD_DL                BIT(1)
>> +#define DEV_CONF_WP_DL                    BIT(0)
>> +
>> +/* bits within the TRAN_CONF(n) register */
>> +#define TRAN_CONF_TRAN_MODE_MASK        GENMASK(31, 29)
>> +#define TRAN_CONF_ADDR_WIDTH_MASK        GENMASK(28, 26)
>> +#define TRAN_CONF_POLL_EN                BIT(25)
>> +#define TRAN_CONF_CMD_EN                BIT(24)
>> +#define TRAN_CONF_PHASE_FORMAT_MASK        GENMASK(23, 23)
>> +#define TRAN_CONF_DMY_BITS_MASK            GENMASK(22, 17)
>> +#define TRAN_CONF_DATA_EN                BIT(16)
>> +#define TRAN_CONF_CMD_MASK                GENMASK(15, 0)
>> +
>> +/* bits within the TRIG register */
>> +#define TRIG_FLUSH                        BIT(2)
>> +#define TRIG_STOP                        BIT(1)
>> +#define TRIG_START                        BIT(0)
>> +
>> +/* bits within the SR register */
>> +#define SR_FIFO_NUM_MASK                GENMASK(22, 16)
>> +#define SR_END                            BIT(4)
>> +#define SR_TRAN_REQ                        BIT(3)
>> +#define SR_RECE_REQ                        BIT(2)
>> +#define SR_OVER                            BIT(1)
>> +#define SR_UNDER                        BIT(0)
>> +
>> +/* bits within the SCR register */
>> +#define SCR_CLR_END                        BIT(4)
>> +#define SCR_CLR_TREQ                    BIT(3)
>> +#define SCR_CLR_RREQ                    BIT(2)
>> +#define SCR_CLR_OVER                    BIT(1)
>> +#define SCR_CLR_UNDER                    BIT(0)
>> +
>> +/* bits within the INTC register */
>> +#define INTC_MASK_END                    BIT(4)
>> +#define INTC_MASK_TREQ                    BIT(3)
>> +#define INTC_MASK_RREQ                    BIT(2)
>> +#define INTC_MASK_OVER                    BIT(1)
>> +#define INTC_MASK_UNDER                    BIT(0)
>> +
>> +/* bits within the TRAN_CFG1(n) register */
>> +#define TRAN_CFG1_TRAN_MODE_MASK        GENMASK(7, 4)
>> +
>> +#define TRAN_MODE_STANDARD                0
>> +#define TRAN_MODE_DUAL_DATA                1
>> +#define TRAN_MODE_DUAL_IO                2
>> +#define TRAN_MODE_DUAL_FULL                3
>> +#define TRAN_MODE_QUAD_DATA                5
>> +#define TRAN_MODE_QUAD_IO                6
>> +#define TRAN_MODE_QUAD_FULL                7
>> +#define TRAN_MODE_OCTAL_DATA            9
>> +#define TRAN_MODE_OCTAL_IO                10
>> +#define TRAN_MODE_OCTAL_FULL            11
>> +
>> +#define INGENIC_SFC_FIFO_SIZE            (64 * 4)
>> +
>> +#define INGENIC_SFC_TRANSFER_TIMEOUT    1000
>
> Maybe add the unit name in the macro as well - 
> INGENIC_SFC_TRANSFER_TIMEOUT_MS.


Sure.


>
>> +
>> +enum ingenic_sfc_version {
>> +    ID_X1000,
>> +    ID_X1600,
>> +    ID_X2000,
>> +};
>> +
>> +struct ingenic_soc_info {
>> +    enum ingenic_sfc_version version;
>> +
>> +    unsigned int max_bus_width;
>> +
>> +    const u32 tran_mode_mask;
>> +};
>> +
>> +struct ingenic_sfc {
>> +    const struct ingenic_soc_info *soc_info;
>> +
>> +    void __iomem *base;
>> +    struct device *dev;
>> +    struct clk *clk;
>> +    int irq;
>> +
>> +    struct completion completion;
>> +};
>> +
>> +static irqreturn_t ingenic_sfc_irq_handler(int irq, void *data)
>> +{
>> +    struct ingenic_sfc *sfc = data;
>> +
>> +    writel(0x1f, sfc->base + SFC_REG_INTC);
>> +
>> +    complete(&sfc->completion);
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +static int ingenic_sfc_adjust_op_size(struct spi_mem *mem, struct 
>> spi_mem_op *op)
>> +{
>> +    uintptr_t addr = (uintptr_t)op->data.buf.in;
>> +
>> +    if (op->data.nbytes > INGENIC_SFC_FIFO_SIZE && !IS_ALIGNED(addr, 
>> 4))
>> +        op->data.nbytes = INGENIC_SFC_FIFO_SIZE;
>> +
>> +    return 0;
>> +}
>> +
>> +static bool ingenic_sfc_supports_op(struct spi_mem *mem, const 
>> struct spi_mem_op *op)
>> +{
>> +    struct spi_device *spi = mem->spi;
>> +    struct ingenic_sfc *sfc = spi_controller_get_devdata(spi->master);
>> +    uintptr_t addr = (uintptr_t)op->data.buf.in;
>> +
>> +    /* The controller only supports Standard SPI mode, Duall mode, 
>> Quad mode and Octal mode. */
>
> Dual*


Oops, will fix it.


>
>> +    if (op->cmd.buswidth > sfc->soc_info->max_bus_width ||
>> +        op->addr.buswidth > sfc->soc_info->max_bus_width ||
>> +        op->dummy.buswidth > sfc->soc_info->max_bus_width ||
>> +        op->data.buswidth > sfc->soc_info->max_bus_width)
>> +        return false;
>> +
>> +    /* Max 32 dummy clock cycles supported */
>> +    if (op->dummy.nbytes && op->dummy.nbytes * 8 / 
>> op->dummy.buswidth > 32)
>> +        return false;
>> +
>> +    /* Max rx data length, check controller limits and alignment */
>> +    if (op->data.dir == SPI_MEM_DATA_IN &&
>> +        op->data.nbytes > INGENIC_SFC_FIFO_SIZE && !IS_ALIGNED(addr, 
>> 4))
>
> This does the same check than in ingenic_sfc_adjust_op_size(), maybe 
> move it to a new inline function?
>

Sure.


>> +        return false;
>> +
>> +    /* Max 6 bytes address width supported */
>> +    if (op->addr.nbytes > 6)
>> +        return false;
>> +
>> +    return spi_mem_default_supports_op(mem, op);
>> +}
>> +
>> +static void ingenic_sfc_set_transfer_mode(struct ingenic_sfc *sfc, 
>> const struct spi_mem_op *op)
>> +{
>> +    int val;
>> +
>> +    val = readl(sfc->base + (sfc->soc_info->version >= ID_X1600 ?
>> +            SFC_REG_TRAN_CFG1(0) : SFC_REG_TRAN_CONF(0)));
>
> As Krzysztof said - ugh.
>

Will fix it.


> Also, instead of having a "version" enum in your soc_info, why not 
> just have a "reg_conf" field that gives you directly the right register?
>

We still need a version to distinguish the SFC before X1600 SoC and
the SFC after X1600 SoC, because in addition to the difference in the
cfg register, another difference is that the SFC before X1600 SoC does
not support address unaligned RX DMA transfer, while SFC in X1600 and
later SoCs can support this feature.


>
>> +    val &= ~sfc->soc_info->tran_mode_mask;
>> +    if (op->cmd.buswidth == 8)
>> +        val |= (TRAN_MODE_OCTAL_FULL << 
>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +                sfc->soc_info->tran_mode_mask;
>
> Looks like you're really trying to reinvent the wheel.
>
> val |= FIELD_PREP(sfc->soc_info->tran_mode_mask, TRAN_MODE_OCTAL_FULL);
>
> using <linux/bitfield.h>.
>
> Also, just define a 'mode' variable and set it in your if/else blocks, 
> that would look much better. Then you can set val |= FIELD_PREP(..., 
> mode) at the end.


Sure, will change this in the next version.


>
>> +    else if (op->cmd.buswidth == 4)
>> +        val |= (TRAN_MODE_QUAD_FULL << 
>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +                sfc->soc_info->tran_mode_mask;
>> +    else if (op->cmd.buswidth == 2)
>> +        val |= (TRAN_MODE_DUAL_FULL << 
>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +                sfc->soc_info->tran_mode_mask;
>> +    else if (op->addr.buswidth == 8)
>> +        val |= (TRAN_MODE_OCTAL_IO << 
>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +                sfc->soc_info->tran_mode_mask;
>> +    else if (op->addr.buswidth == 4)
>> +        val |= (TRAN_MODE_QUAD_IO << 
>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +                sfc->soc_info->tran_mode_mask;
>> +    else if (op->addr.buswidth == 2)
>> +        val |= (TRAN_MODE_DUAL_IO << 
>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +                sfc->soc_info->tran_mode_mask;
>> +    else if (op->data.buswidth == 8)
>> +        val |= (TRAN_MODE_OCTAL_DATA << 
>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +                sfc->soc_info->tran_mode_mask;
>> +    else if (op->data.buswidth == 4)
>> +        val |= (TRAN_MODE_QUAD_DATA << 
>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +                sfc->soc_info->tran_mode_mask;
>> +    else if (op->data.buswidth == 2)
>> +        val |= (TRAN_MODE_DUAL_DATA << 
>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +                sfc->soc_info->tran_mode_mask;
>> +    else
>> +        val |= (TRAN_MODE_STANDARD << 
>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>> +                sfc->soc_info->tran_mode_mask;
>> +    writel(val, sfc->base + (sfc->soc_info->version >= ID_X1600 ?
>> +            SFC_REG_TRAN_CFG1(0) : SFC_REG_TRAN_CONF(0)));
>> +}
>> +
>> +/*
>> + * We only need PIO mode to handle the SPI_MEM_NO_DATA transfers,
>> + * and the unaligned accesses in SPI_MEM_DATA_IN transfers.
>> + */
>> +static void ingenic_sfc_read_rxfifo(struct ingenic_sfc *sfc, u8 *to, 
>> unsigned int len)
>> +{
>> +    void __iomem *from;
>> +
>> +    from = sfc->base + SFC_REG_DR;
>> +
>> +    for (; len >= 4; len -= 4, to += 4) {
>> +        u32 val = __raw_readl(from);
>> +        memcpy(to, &val, 4);
>
> No need to use memcpy for 4 bytes. You can do: put_unaligned(val, (u32 
> *)to);
>

Sure.


>> +    }
>> +
>> +    if (len) {
>> +        u32 val = __raw_readl(from);
>> +        memcpy(to, &val, len);
>
> Hmm, I'm not sure that is endian-safe. I would prefer if you copied 
> byte by byte.
>

Sure.


>> +    }
>> +}
>> +
>> +static int ingenic_sfc_exec_op_pio(struct ingenic_sfc *sfc, const 
>> struct spi_mem_op *op)
>> +{
>> +    int ret, val;
>> +
>> +    val = readl(sfc->base + SFC_REG_GLB);
>> +    u32p_replace_bits(&val, GLB_TRAN_DIR_READ, GLB_TRAN_DIR_MASK);
>> +    u32p_replace_bits(&val, GLB_OP_MODE_SLAVE, GLB_OP_MODE_MASK);
>> +    writel(val, sfc->base + SFC_REG_GLB);
>
> By the way, have you considered using regmap?
>
> It would give you things like regmap_update_bits() for this kind of 
> things, and regmap_field() to handle your conf register being at a 
> different address across SoCs.
>

It seems that the overhead of using regmap will be greater than using 
readl and writel,
the purpose of using readl and writel is to minimize overhead and 
maximize performance. :)


>> +
>> +    val = TRAN_CONF_CMD_EN | op->cmd.opcode;
>> +
>> +    if (op->addr.nbytes > 0) {
>> +        val |= FIELD_PREP(TRAN_CONF_ADDR_WIDTH_MASK, op->addr.nbytes);
>> +
>> +        writel(op->addr.val & 0xffffffff, sfc->base + 
>> SFC_REG_DEV_ADDR(0));
>> +        writel(op->addr.val >> 32, sfc->base + 
>> SFC_REG_DEV_ADDR_PLUS(0));
>> +    }
>> +
>> +    if (op->dummy.nbytes > 0)
>> +        val |= FIELD_PREP(TRAN_CONF_DMY_BITS_MASK,
>> +                op->dummy.nbytes * 8 / op->dummy.buswidth);
>> +
>> +    if (op->data.nbytes > 0)
>> +        val |= TRAN_CONF_DATA_EN;
>> +
>> +    writel(val, sfc->base + SFC_REG_TRAN_CONF(0));
>> +    writel(op->data.nbytes, sfc->base + SFC_REG_TRAN_LEN);
>> +
>> +    ingenic_sfc_set_transfer_mode(sfc, op);
>> +
>> +    writel(0x1f, sfc->base + SFC_REG_SCR);
>
> Random 0x1f value here, maybe use a macro?


Sure.


>
>> +    writel(~(INTC_MASK_END | INTC_MASK_RREQ), sfc->base + 
>> SFC_REG_INTC);
>> +
>> +    writel(0, sfc->base + SFC_REG_MEM_ADDR);
>> +
>> +    writel(TRIG_FLUSH, sfc->base + SFC_REG_TRIG);
>> +    writel(TRIG_START, sfc->base + SFC_REG_TRIG);
>> +
>> +    ret = wait_for_completion_timeout(&sfc->completion,
>> +            msecs_to_jiffies(INGENIC_SFC_TRANSFER_TIMEOUT));
>> +    if (!ret) {
>> +        writel(0x1f, sfc->base + SFC_REG_INTC);
>> +        writel(0x1f, sfc->base + SFC_REG_SCR);
>> +        dev_err(sfc->dev, "line:%d Timeout for ACK from SFC 
>> device\n", __LINE__);
>> +        return -ETIMEDOUT;
>> +    }
>> +
>> +    ingenic_sfc_read_rxfifo(sfc, op->data.buf.in, op->data.nbytes);
>> +    readl_poll_timeout(sfc->base + SFC_REG_SR, val, val & SR_END, 
>> 10, 0);
>
> Infinite timeout? Is that very wise?


Will fix it in the next version.


>
>> +
>> +    writel(INTC_MASK_END | INTC_MASK_RREQ, sfc->base + SFC_REG_SCR);
>> +    writel(TRIG_STOP, sfc->base + SFC_REG_TRIG);
>> +
>> +    return 0;
>> +}
>> +
>> +static int ingenic_sfc_exec_op_dma(struct ingenic_sfc *sfc, const 
>> struct spi_mem_op *op)
>> +{
>> +    dma_addr_t addr;
>> +    int ret, val;
>> +
>> +    val = readl(sfc->base + SFC_REG_GLB);
>> +    u32p_replace_bits(&val, op->data.dir == SPI_MEM_DATA_IN ?
>> +            GLB_TRAN_DIR_READ : GLB_TRAN_DIR_WRITE, GLB_TRAN_DIR_MASK);
>> +    u32p_replace_bits(&val, GLB_OP_MODE_DMA, GLB_OP_MODE_MASK);
>> +    writel(val, sfc->base + SFC_REG_GLB);
>> +
>> +    val = TRAN_CONF_CMD_EN | op->cmd.opcode;
>> +
>> +    if (op->addr.nbytes > 0) {
>> +        val |= FIELD_PREP(TRAN_CONF_ADDR_WIDTH_MASK, op->addr.nbytes);
>> +        writel(op->addr.val & 0xffffffff, sfc->base + 
>> SFC_REG_DEV_ADDR(0));
>> +        writel(op->addr.val >> 32, sfc->base + 
>> SFC_REG_DEV_ADDR_PLUS(0));
>> +    }
>> +
>> +    if (op->dummy.nbytes > 0)
>> +        val |= FIELD_PREP(TRAN_CONF_DMY_BITS_MASK,
>> +                op->dummy.nbytes * 8 / op->dummy.buswidth);
>> +
>> +    if (op->data.nbytes > 0)
>> +        val |= TRAN_CONF_DATA_EN;
>
> There's a lot of code duplication here with ingenic_sfc_exec_op_pio(). 
> A lot can be factorized.


Sure, I will try to do it.


>
>> +
>> +    writel(val, sfc->base + SFC_REG_TRAN_CONF(0));
>> +    writel(op->data.nbytes, sfc->base + SFC_REG_TRAN_LEN);
>> +
>> +    ingenic_sfc_set_transfer_mode(sfc, op);
>> +
>> +    writel(0x1f, sfc->base + SFC_REG_SCR);
>> +    writel(~INTC_MASK_END, sfc->base + SFC_REG_INTC);
>> +
>> +    switch (op->data.dir) {
>> +    case SPI_MEM_DATA_IN:
>> +        addr = dma_map_single(sfc->dev, op->data.buf.in, 
>> op->data.nbytes, DMA_FROM_DEVICE);
>> +        if (dma_mapping_error(sfc->dev, addr)) {
>> +            dev_err(sfc->dev, "RX DMA　memory not mapped\n");
>> +            return -ENOMEM;
>> +        }
>> +
>> +        writel(addr, sfc->base + SFC_REG_MEM_ADDR);
>> +        break;
>> +
>> +    case SPI_MEM_DATA_OUT:
>> +        addr = dma_map_single(sfc->dev, (void *)op->data.buf.out,
>> +                op->data.nbytes, DMA_TO_DEVICE);
>> +        if (dma_mapping_error(sfc->dev, addr)) {
>> +            dev_err(sfc->dev, "TX DMA　memory not mapped\n");
>> +            return -ENOMEM;
>> +        }
>> +
>> +        writel(addr, sfc->base + SFC_REG_MEM_ADDR);
>> +        break;
>> +
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    writel(TRIG_START, sfc->base + SFC_REG_TRIG);
>> +
>> +    ret = wait_for_completion_timeout(&sfc->completion,
>> +            msecs_to_jiffies(INGENIC_SFC_TRANSFER_TIMEOUT));
>> +    if (!ret) {
>> +        writel(0x1f, sfc->base + SFC_REG_INTC);
>> +        writel(0x1f, sfc->base + SFC_REG_SCR);
>> +        dev_err(sfc->dev, "line:%d Timeout for ACK from SFC 
>> device\n", __LINE__);
>> +        return -ETIMEDOUT;
>> +    }
>> +
>> +    dma_unmap_single(sfc->dev, addr, op->data.nbytes,
>> +            op->data.dir == SPI_MEM_DATA_IN ? DMA_FROM_DEVICE : 
>> DMA_TO_DEVICE);
>
> Use a small inline function for that too. My personal rule is that ?: 
> is fine if the line fits in 80 characters, but if you have to break, 
> then you really need to move it somewhere else.
>

Sure.


>> +
>> +    writel(INTC_MASK_END, sfc->base + SFC_REG_SCR);
>> +    writel(TRIG_STOP, sfc->base + SFC_REG_TRIG);
>> +
>> +    return 0;
>> +}
>> +
>> +static int ingenic_sfc_exec_op(struct spi_mem *mem, const struct 
>> spi_mem_op *op)
>> +{
>> +    struct spi_device *spi = mem->spi;
>> +    struct ingenic_sfc *sfc = spi_controller_get_devdata(spi->master);
>> +    uintptr_t addr = (uintptr_t)op->data.buf.in;
>> +
>> +    init_completion(&sfc->completion);
>> +
>> +    switch (op->data.dir) {
>> +    case SPI_MEM_DATA_IN:
>> +        if (sfc->soc_info->version >= ID_X1600 || IS_ALIGNED(addr, 4))
>> +            break;
>> +
>> +        fallthrough;
>> +
>> +    case SPI_MEM_NO_DATA:
>> +        return ingenic_sfc_exec_op_pio(sfc, op);
>> +
>> +    default:
>> +        break;
>> +    }
>> +
>> +    return ingenic_sfc_exec_op_dma(sfc, op);
>> +}
>> +
>> +static int ingenic_sfc_poll_status(struct spi_mem *mem, const struct 
>> spi_mem_op *op,
>> +            u16 mask, u16 match, unsigned long initial_delay_us,
>> +            unsigned long polling_delay_us, unsigned long timeout_ms)
>> +{
>> +    struct spi_device *spi = mem->spi;
>> +    struct ingenic_sfc *sfc = spi_controller_get_devdata(spi->master);
>> +    int ret, val;
>> +
>> +    init_completion(&sfc->completion);
>> +
>> +    val = readl(sfc->base + SFC_REG_GLB);
>> +    u32p_replace_bits(&val, GLB_TRAN_DIR_READ, GLB_TRAN_DIR_MASK);
>> +    u32p_replace_bits(&val, GLB_OP_MODE_SLAVE, GLB_OP_MODE_MASK);
>> +    writel(val, sfc->base + SFC_REG_GLB);
>> +
>> +    writel(match, sfc->base + SFC_REG_DEV_STA_EXP);
>> +    writel(mask, sfc->base + SFC_REG_DEV_STA_MSK);
>> +
>> +    val = TRAN_CONF_POLL_EN | TRAN_CONF_CMD_EN | op->cmd.opcode;
>> +
>> +    if (op->addr.nbytes > 0) {
>> +        val |= FIELD_PREP(TRAN_CONF_ADDR_WIDTH_MASK, op->addr.nbytes);
>> +
>> +        writel(op->addr.val & 0xffffffff, sfc->base + 
>> SFC_REG_DEV_ADDR(0));
>> +        writel(op->addr.val >> 32, sfc->base + 
>> SFC_REG_DEV_ADDR_PLUS(0));
>> +    }
>> +
>> +    if (op->dummy.nbytes > 0)
>> +        val |= FIELD_PREP(TRAN_CONF_DMY_BITS_MASK,
>> +                op->dummy.nbytes * 8 / op->dummy.buswidth);
>> +
>> +    if (op->data.nbytes > 0)
>> +        val |= TRAN_CONF_DATA_EN;
>> +
>> +    writel(val, sfc->base + SFC_REG_TRAN_CONF(0));
>> +    writel(op->data.nbytes, sfc->base + SFC_REG_TRAN_LEN);
>> +
>> +    ingenic_sfc_set_transfer_mode(sfc, op);
>> +
>> +    writel(0x1f, sfc->base + SFC_REG_SCR);
>> +    writel(~INTC_MASK_END, sfc->base + SFC_REG_INTC);
>> +
>> +    writel(0, sfc->base + SFC_REG_MEM_ADDR);
>> +
>> +    writel(TRIG_START, sfc->base + SFC_REG_TRIG);
>> +
>> +    ret = wait_for_completion_timeout(&sfc->completion,
>> +            msecs_to_jiffies(INGENIC_SFC_TRANSFER_TIMEOUT));
>> +    if (!ret) {
>> +        writel(0x1f, sfc->base + SFC_REG_INTC);
>> +        writel(0x1f, sfc->base + SFC_REG_SCR);
>> +        dev_err(sfc->dev, "line:%d Timeout for ACK from SFC 
>> device\n", __LINE__);
>> +        return -ETIMEDOUT;
>> +    }
>> +
>> +    writel(SCR_CLR_END, sfc->base + SFC_REG_SCR);
>> +    writel(TRIG_STOP, sfc->base + SFC_REG_TRIG);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct spi_controller_mem_ops ingenic_sfc_mem_ops = {
>> +    .adjust_op_size = ingenic_sfc_adjust_op_size,
>> +    .supports_op = ingenic_sfc_supports_op,
>> +    .exec_op = ingenic_sfc_exec_op,
>> +    .poll_status = ingenic_sfc_poll_status,
>> +};
>> +
>> +static int ingenic_sfc_setup(struct spi_device *spi)
>> +{
>> +    struct ingenic_sfc *sfc = spi_controller_get_devdata(spi->master);
>> +    unsigned long rate;
>> +    int ret, val;
>> +
>> +    if (!spi->max_speed_hz)
>> +        return -EINVAL;
>
> Maybe set a sane default?


Sure.


>
>> +
>> +    ret = clk_set_rate(sfc->clk, spi->max_speed_hz * 2);
>> +    if (ret)
>> +        return -EINVAL;
>> +
>> +    writel(TRIG_STOP, sfc->base + SFC_REG_TRIG);
>> +    writel(0, sfc->base + SFC_REG_DEV_CONF);
>> +    writel(0, sfc->base + SFC_REG_CGE);
>> +
>> +    val = readl(sfc->base + SFC_REG_GLB);
>> +    u32p_replace_bits(&val, 64 - 1, GLB_THRESHOLD_MASK);
>> +    writel(val, sfc->base + SFC_REG_GLB);
>> +
>> +    val = readl(sfc->base + SFC_REG_DEV_CONF);
>> +
>> +    /* cpha bit:0 , cpol bit:0 */
>> +    val &= ~(DEV_CONF_CPHA | DEV_CONF_CPOL);
>> +    val |= spi->mode & SPI_CPHA ? DEV_CONF_CPHA : 0;
>> +    val |= spi->mode & SPI_CPOL ? DEV_CONF_CPOL : 0;
>> +
>> +    /* ce_dl bit:1, hold bit:1, wp bit:1 */
>> +    val |= (DEV_CONF_CE_DL | DEV_CONF_HOLD_DL | DEV_CONF_WP_DL);
>> +
>> +    writel(val, sfc->base + SFC_REG_DEV_CONF);
>> +
>> +    val = readl(sfc->base + SFC_REG_GLB);
>> +    u32p_replace_bits(&val, GLB_OP_MODE_SLAVE, GLB_OP_MODE_MASK);
>> +    writel(val, sfc->base + SFC_REG_GLB);
>> +
>> +    rate = clk_get_rate(sfc->clk);
>
> I'd suggest using clk_round_rate() before clk_set_rate() because then 
> you know what frequency it's going to be, and you don't have to call 
> clk_get_rate() afterwards.
>

Sure, will try.


Thanks and best regards!


> Cheers,
> -Paul
>
>> +    val = readl(sfc->base + SFC_REG_DEV_CONF);
>> +    if (sfc->soc_info->version >= ID_X1600 && rate >= 200000000)
>> +        u32p_replace_bits(&val, DEV_CONF_SMP_DELAY_180DEG, 
>> DEV_CONF_SMP_DELAY_MASK);
>> +    else if (sfc->soc_info->version == ID_X1000 && rate >= 100000000)
>> +        u32p_replace_bits(&val, DEV_CONF_SMP_DELAY_HALF_CYCLE, 
>> DEV_CONF_SMP_DELAY_MASK);
>> +    writel(val, sfc->base + SFC_REG_DEV_CONF);
>> +
>> +    return 0;
>> +}
>> +
>> +static int ingenic_sfc_probe(struct platform_device *pdev)
>> +{
>> +    struct ingenic_sfc *sfc;
>> +    struct spi_controller *ctlr;
>> +    int ret;
>> +
>> +    ctlr = spi_alloc_master(&pdev->dev, sizeof(*sfc));
>> +    if (!ctlr)
>> +        return -ENOMEM;
>> +
>> +    sfc = spi_controller_get_devdata(ctlr);
>> +
>> +    sfc->soc_info = of_device_get_match_data(&pdev->dev);
>> +    if (!sfc->soc_info) {
>> +        dev_err(&pdev->dev, "No of match data provided\n");
>> +        ret = -ENODEV;
>> +        goto err_put_master;
>> +    }
>> +
>> +    sfc->base = devm_platform_ioremap_resource(pdev, 0);
>> +    if (IS_ERR(sfc->base)) {
>> +        ret = PTR_ERR(sfc->base);
>> +        goto err_put_master;
>> +    }
>> +
>> +    sfc->clk = devm_clk_get(&pdev->dev, "sfc");
>> +    if (IS_ERR(sfc->clk)) {
>> +        ret = IS_ERR(sfc->clk);
>> +        goto err_put_master;
>> +    }
>> +
>> +    ret = clk_prepare_enable(sfc->clk);
>> +    if (ret)
>> +        goto err_put_master;
>> +
>> +    sfc->irq = platform_get_irq(pdev, 0);
>> +    if (sfc->irq < 0) {
>> +        ret = sfc->irq;
>> +        goto err_put_master;
>> +    }
>> +
>> +    sfc->dev = &pdev->dev;
>> +
>> +    platform_set_drvdata(pdev, sfc);
>> +
>> +    ret = devm_request_irq(&pdev->dev, sfc->irq, 
>> ingenic_sfc_irq_handler, 0,
>> +            dev_name(&pdev->dev), sfc);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Failed to request irq%d, ret = %d\n", 
>> sfc->irq, ret);
>> +        goto err_put_master;
>> +    }
>> +
>> +    ctlr->bus_num = -1;
>> +    ctlr->num_chipselect = 1;
>> +    ctlr->mem_ops = &ingenic_sfc_mem_ops;
>> +    ctlr->dev.of_node = pdev->dev.of_node;
>> +    ctlr->setup = ingenic_sfc_setup;
>> +    ctlr->mode_bits = SPI_CPHA | SPI_CPOL |
>> +            SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD;
>> +    if (sfc->soc_info->version >= ID_X2000)
>> +        ctlr->mode_bits |= SPI_RX_OCTAL | SPI_TX_OCTAL;
>> +
>> +    ret = devm_spi_register_controller(&pdev->dev, ctlr);
>> +    if (ret)
>> +        goto err_put_master;
>> +
>> +    return 0;
>> +
>> +err_put_master:
>> +    spi_master_put(ctlr);
>> +
>> +    return ret;
>> +}
>> +
>> +static const struct ingenic_soc_info x1000_soc_info = {
>> +    .version = ID_X1000,
>> +
>> +    .max_bus_width = 4,
>> +
>> +    .tran_mode_mask = TRAN_CONF_TRAN_MODE_MASK,
>> +};
>> +
>> +static const struct ingenic_soc_info x1600_soc_info = {
>> +    .version = ID_X1600,
>> +
>> +    .max_bus_width = 4,
>> +
>> +    .tran_mode_mask = TRAN_CONF_TRAN_MODE_MASK,
>> +};
>> +
>> +static const struct ingenic_soc_info x2000_soc_info = {
>> +    .version = ID_X2000,
>> +
>> +    .max_bus_width = 8,
>> +
>> +    .tran_mode_mask = TRAN_CFG1_TRAN_MODE_MASK,
>> +};
>> +
>> +static const struct of_device_id ingenic_sfc_of_matches[] = {
>> +    { .compatible = "ingenic,x1000-sfc", .data = &x1000_soc_info },
>> +    { .compatible = "ingenic,x1600-sfc", .data = &x1600_soc_info },
>> +    { .compatible = "ingenic,x2000-sfc", .data = &x2000_soc_info },
>> +    { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, ingenic_sfc_of_matches);
>> +
>> +static struct platform_driver ingenic_sfc_driver = {
>> +    .driver = {
>> +        .name = "ingenic-sfc",
>> +        .of_match_table = ingenic_sfc_of_matches,
>> +    },
>> +    .probe = ingenic_sfc_probe,
>> +};
>> +module_platform_driver(ingenic_sfc_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>");
>> +MODULE_DESCRIPTION("Ingenic SoCs SPI Flash Controller Driver");
>> -- 
>> 2.7.4
>>
>

On 23/07/2022 18:50, Zhou Yanjie wrote:
> Hi Krzysztof,
> 
> On 2022/7/23 上午1:46, Krzysztof Kozlowski wrote:
>> On 22/07/2022 18:48, 周琰杰 (Zhou Yanjie) wrote:
>>> Add the SFC bindings for the X1000 SoC, the X1600 SoC, the X1830 SoC,
>>> and the X2000 SoC from Ingenic.
>>>
>>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>>> ---
>>>   .../devicetree/bindings/spi/ingenic,sfc.yaml       | 64 ++++++++++++++++++++++
>>>   1 file changed, 64 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>> new file mode 100644
>>> index 00000000..b7c4cf4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>> @@ -0,0 +1,64 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/spi/ingenic,sfc.yaml#
>> File name should be rather based on first compatible, so
>> ingenic,x1000-sfc.yaml
> 
> 
> No offense, does it really need to be named that way?
> I can't seem to find documentation with instructions on this :(
> 
> The use of "ingenic,sfc.yaml" indicates that this is the documentation
> for the SFC module for all Ingenic SoCs, without misleading people into
> thinking it's only for a specific model of SoC. And there seem to be many
> other yaml documents that use similar names (eg. fsl,spi-fsl-qspi.yaml,
> spi-rockchip.yaml, spi-nxp-fspi.yaml, ingenic,spi.yaml, spi-sifive.yaml,
> omap-spi.yaml), maybe these yaml files that are not named with first
> compatible are also for the same consideration. :)

We have many bad examples, many poor patterns and they are never an
argument to add one more bad pattern.

It might never grow to new devices (because they might be different), so
that is not really an argument.

All bindings are to follow this rule, so I don't understand why you
think it is an exception for you?


Best regards,
Krzysztof

On 7/24/22 01:43, Krzysztof Kozlowski wrote:
> On 23/07/2022 18:50, Zhou Yanjie wrote:
>> Hi Krzysztof,
>>
>> On 2022/7/23 上午1:46, Krzysztof Kozlowski wrote:
>>> On 22/07/2022 18:48, 周琰杰 (Zhou Yanjie) wrote:
>>>> Add the SFC bindings for the X1000 SoC, the X1600 SoC, the X1830 SoC,
>>>> and the X2000 SoC from Ingenic.
>>>>
>>>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>>>> ---
>>>>   .../devicetree/bindings/spi/ingenic,sfc.yaml       | 64 ++++++++++++++++++++++
>>>>   1 file changed, 64 insertions(+)
>>>>   create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>> new file mode 100644
>>>> index 00000000..b7c4cf4
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>> @@ -0,0 +1,64 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/spi/ingenic,sfc.yaml#
>>> File name should be rather based on first compatible, so
>>> ingenic,x1000-sfc.yaml
>>
>>
>> No offense, does it really need to be named that way?
>> I can't seem to find documentation with instructions on this :(
>>
>> The use of "ingenic,sfc.yaml" indicates that this is the documentation
>> for the SFC module for all Ingenic SoCs, without misleading people into
>> thinking it's only for a specific model of SoC. And there seem to be many
>> other yaml documents that use similar names (eg. fsl,spi-fsl-qspi.yaml,
>> spi-rockchip.yaml, spi-nxp-fspi.yaml, ingenic,spi.yaml, spi-sifive.yaml,
>> omap-spi.yaml), maybe these yaml files that are not named with first
>> compatible are also for the same consideration. :)
> 
> We have many bad examples, many poor patterns and they are never an
> argument to add one more bad pattern.

Zhou already mentioned he was unable find the naming guidelines of these .yaml files.

Apparently you think it's unacceptable for new contributors of a certain subsystem to use existing code as examples, and/or they're responsible for figuring out what's a good example and what's a bad one in the existing codebase.

> 
> It might never grow to new devices (because they might be different), so
> that is not really an argument.

It is an argument. A very valid one.

"they *might* be different". You may want to get your hands on real hardware and try another word. Or at least read the datasheets instead of believing your imagination.

I would enjoy duplicating the st,stm32-spi.yaml into st,stm32{f,h}{0..7}-spi.yaml if I'm bored at a Sunday afternoon.

> 
> All bindings are to follow this rule, so I don't understand why you
> think it is an exception for you?

Zhou didn't ask you to make an exception. They have a valid point and they're asking why.

You may want to avoid further incidents of this kind by stop being bossy and actually writing a guideline of naming these .yaml files and publish it somewhere online.

> 
> 
> Best regards,
> Krzysztof

Best regards,
Mike Yang

On Sun, Jul 24, 2022 at 02:47:14AM +0800, Mike Yang wrote:
> On 7/24/22 01:43, Krzysztof Kozlowski wrote:
> > On 23/07/2022 18:50, Zhou Yanjie wrote:

> >> No offense, does it really need to be named that way?
> >> I can't seem to find documentation with instructions on this :(

...

> > All bindings are to follow this rule, so I don't understand why you
> > think it is an exception for you?

> Zhou didn't ask you to make an exception. They have a valid
> point and they're asking why.

> You may want to avoid further incidents of this kind by stop
> being bossy and actually writing a guideline of naming these
> .yaml files and publish it somewhere online.

Yeah, I do have to say that I was also completely unaware that
there was any enforced convention here.

On Sun, Jul 24, 2022 at 01:06:16AM +0800, Zhou Yanjie wrote:
> On 2022/7/23 上午2:38, Mark Brown wrote:

> > > +++ b/drivers/spi/spi-ingenic-sfc.c
> > > @@ -0,0 +1,662 @@
> > > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +/*
> > > + * Ingenic SoCs SPI Flash Controller Driver

> > Please make the entire comment a C++ one so things look more
> > intentional.

> I'm sorry, I didn't understand well what you meant :(
> Could you please explain a little more detail?

The above comment block uses both C /* */ and C++ // style comments,
please make it just use the C++ style.

> > > +static irqreturn_t ingenic_sfc_irq_handler(int irq, void *data)
> > > +{
> > > +	struct ingenic_sfc *sfc = data;
> > > +
> > > +	writel(0x1f, sfc->base + SFC_REG_INTC);
> > > +
> > > +	complete(&sfc->completion);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}

> > This doesn't pay any attention to any status registers in the chip so
> > won't work if the interrupt is shared and won't notice any error reports
> > from the device...

> This interrupt is exclusively owned by SFC, do we still
> need to perform the operation you said? I haven't done
> these operations before because I want to minimize the
> overhead and avoid affecting performance.

Even if the device is not shared is there no possibility that the
device can report an unexpected interrupt status?  It's not just
the sharing case, it's also the fact that it looks like there's a
status being reported but we're not checking it so if anything
goes wrong then we're less likely to notice.  I'd worry about
data corruption.

On 23/07/2022 20:47, Mike Yang wrote:
> On 7/24/22 01:43, Krzysztof Kozlowski wrote:
>> On 23/07/2022 18:50, Zhou Yanjie wrote:
>>> Hi Krzysztof,
>>>
>>> On 2022/7/23 上午1:46, Krzysztof Kozlowski wrote:
>>>> On 22/07/2022 18:48, 周琰杰 (Zhou Yanjie) wrote:
>>>>> Add the SFC bindings for the X1000 SoC, the X1600 SoC, the X1830 SoC,
>>>>> and the X2000 SoC from Ingenic.
>>>>>
>>>>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>>>>> ---
>>>>>   .../devicetree/bindings/spi/ingenic,sfc.yaml       | 64 ++++++++++++++++++++++
>>>>>   1 file changed, 64 insertions(+)
>>>>>   create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>>> new file mode 100644
>>>>> index 00000000..b7c4cf4
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>>> @@ -0,0 +1,64 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/spi/ingenic,sfc.yaml#
>>>> File name should be rather based on first compatible, so
>>>> ingenic,x1000-sfc.yaml
>>>
>>>
>>> No offense, does it really need to be named that way?
>>> I can't seem to find documentation with instructions on this :(
>>>
>>> The use of "ingenic,sfc.yaml" indicates that this is the documentation
>>> for the SFC module for all Ingenic SoCs, without misleading people into
>>> thinking it's only for a specific model of SoC. And there seem to be many
>>> other yaml documents that use similar names (eg. fsl,spi-fsl-qspi.yaml,
>>> spi-rockchip.yaml, spi-nxp-fspi.yaml, ingenic,spi.yaml, spi-sifive.yaml,
>>> omap-spi.yaml), maybe these yaml files that are not named with first
>>> compatible are also for the same consideration. :)
>>
>> We have many bad examples, many poor patterns and they are never an
>> argument to add one more bad pattern.
> 
> Zhou already mentioned he was unable find the naming guidelines of these .yaml files.
> 
> Apparently you think it's unacceptable for new contributors of a certain subsystem to use existing code as examples, and/or they're responsible for figuring out what's a good example and what's a bad one in the existing codebase.

It's everywhere in the kernel, what can I say? If you copy existing
code, you might copy poor code...

> 
>>
>> It might never grow to new devices (because they might be different), so
>> that is not really an argument.
> 
> It is an argument. A very valid one.
> 
> "they *might* be different". You may want to get your hands on real hardware and try another word. Or at least read the datasheets instead of believing your imagination.
> 
> I would enjoy duplicating the st,stm32-spi.yaml into st,stm32{f,h}{0..7}-spi.yaml if I'm bored at a Sunday afternoon.
> 
>>
>> All bindings are to follow this rule, so I don't understand why you
>> think it is an exception for you?
> 
> Zhou didn't ask you to make an exception. They have a valid point and they're asking why.

Hm, everyone has the same valid point and such recommendation is to
everyone, although it is nothing serious.

> You may want to avoid further incidents of this kind by stop being bossy and actually writing a guideline of naming these .yaml files and publish it somewhere online.

I did not see any incident here... Process of review includes comments
and there is nothing bad happening when you receive a comment. No
incident...

Best regards,
Krzysztof

On 23/07/2022 21:27, Mark Brown wrote:
> On Sun, Jul 24, 2022 at 02:47:14AM +0800, Mike Yang wrote:
>> On 7/24/22 01:43, Krzysztof Kozlowski wrote:
>>> On 23/07/2022 18:50, Zhou Yanjie wrote:
> 
>>>> No offense, does it really need to be named that way?
>>>> I can't seem to find documentation with instructions on this :(
> 
> ...
> 
>>> All bindings are to follow this rule, so I don't understand why you
>>> think it is an exception for you?
> 
>> Zhou didn't ask you to make an exception. They have a valid
>> point and they're asking why.
> 
>> You may want to avoid further incidents of this kind by stop
>> being bossy and actually writing a guideline of naming these
>> .yaml files and publish it somewhere online.
> 
> Yeah, I do have to say that I was also completely unaware that
> there was any enforced convention here.

Indeed, it's not a enforced pattern. But there are many other
insignificant ones which we also tend to forget during review, like
using words "Device Tree bindings" in title or using unnecessary quotes
around "refs" (also in ID of schema). It's not a big deal, but I ask
when I notice it.


Best regards,
Krzysztof

Hi Zhou,

Le dim., juil. 24 2022 at 01:26:56 +0800, Zhou Yanjie 
<zhouyanjie@wanyeetech.com> a écrit :
> Hi Paul,
> 
> On 2022/7/23 上午4:03, Paul Cercueil wrote:
>> Hi Zhou,
>> 
>> 
>> Le sam., juil. 23 2022 at 00:48:30 +0800, 周琰杰 (Zhou Yanjie) 
>> <zhouyanjie@wanyeetech.com> a écrit :
>>> Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC
>>> from Ingenic.
>>> 
>>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>>> ---
>>>  drivers/spi/Kconfig           |   9 +
>>>  drivers/spi/Makefile          |   1 +
>>>  drivers/spi/spi-ingenic-sfc.c | 662 
>>> ++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 672 insertions(+)
>>>  create mode 100644 drivers/spi/spi-ingenic-sfc.c
>>> 
>>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>>> index 3b1044e..1077bd3 100644
>>> --- a/drivers/spi/Kconfig
>>> +++ b/drivers/spi/Kconfig
>>> @@ -437,6 +437,15 @@ config SPI_INGENIC
>>>        To compile this driver as a module, choose M here: the module
>>>        will be called spi-ingenic.
>>> 
>>> +config SPI_INGENIC_SFC
>>> +    tristate "Ingenic SoCs SPI Flash Controller"
>>> +    depends on MACH_INGENIC || COMPILE_TEST
>>> +    help
>>> +      This enables support for the Ingenic SoCs SPI flash 
>>> controller.
>>> +
>>> +      To compile this driver as a module, choose M here: the module
>>> +      will be called ingenic-sfc.
>>> +
>>>  config SPI_INTEL
>>>      tristate
>>> 
>>> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
>>> index 0f44eb6..f3e42c0 100644
>>> --- a/drivers/spi/Makefile
>>> +++ b/drivers/spi/Makefile
>>> @@ -62,6 +62,7 @@ obj-$(CONFIG_SPI_HISI_SFC_V3XX)        += 
>>> spi-hisi-sfc-v3xx.o
>>>  obj-$(CONFIG_SPI_IMG_SPFI)        += spi-img-spfi.o
>>>  obj-$(CONFIG_SPI_IMX)            += spi-imx.o
>>>  obj-$(CONFIG_SPI_INGENIC)        += spi-ingenic.o
>>> +obj-$(CONFIG_SPI_INGENIC_SFC)    += spi-ingenic-sfc.o
>>>  obj-$(CONFIG_SPI_INTEL)            += spi-intel.o
>>>  obj-$(CONFIG_SPI_INTEL_PCI)        += spi-intel-pci.o
>>>  obj-$(CONFIG_SPI_INTEL_PLATFORM)    += spi-intel-platform.o
>>> diff --git a/drivers/spi/spi-ingenic-sfc.c 
>>> b/drivers/spi/spi-ingenic-sfc.c
>>> new file mode 100644
>>> index 00000000..a565546
>>> --- /dev/null
>>> +++ b/drivers/spi/spi-ingenic-sfc.c
>>> @@ -0,0 +1,662 @@
>>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> 
>> Dual-license driver? That's not what MODULE_LICENSE() says.
>> 
> 
> My fault, forgot to modify MODULE_LICENSE(), will fix it in the next 
> version.
> 
> 
>>> +/*
>>> + * Ingenic SoCs SPI Flash Controller Driver
>>> + * Copyright (c) 2022 周琰杰 (Zhou Yanjie) 
>>> <zhouyanjie@wanyeetech.com>
>>> + */
>>> +
>>> +#include <linux/bitfield.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/completion.h>
>>> +#include <linux/dma-mapping.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/iopoll.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mtd/mtd.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/spi/spi.h>
>>> +#include <linux/spi/spi-mem.h>
>>> +
>>> +/* SFC register offsets */
>>> +#define SFC_REG_GLB                        0x0000
>>> +#define SFC_REG_DEV_CONF                0x0004
>>> +#define SFC_REG_DEV_STA_EXP                0x0008
>>> +#define SFC_REG_DEV_STA_RT                0x000c
>>> +#define SFC_REG_DEV_STA_MSK                0x0010
>>> +#define SFC_REG_TRAN_CONF(n)            (0x0014 + n * 4)
>>> +#define SFC_REG_TRAN_CFG0(n)            (0x0014 + n * 4)
>> 
>> You should protect the macro parameter. If you do for instance 
>> SFC_REG_TRAN_CONF(x + 1) it would resolve to (0x0014 + x + 1 * 4) 
>> which is not what you'd want.
>> 
> 
> Sure, will fix it.
> 
> 
>> Also - looks like SFC_REG_TRAN_CONF() and SFC_REG_TRAN_CFG0() are 
>> the same thing, that's on purpose?
>> 
>>> +#define SFC_REG_TRAN_LEN 0x002c
>>> +#define SFC_REG_DEV_ADDR(n)                (0x0030 + n * 4)
>>> +#define SFC_REG_DEV_ADDR_PLUS(n)        (0x0048 + n * 4)
>>> +#define SFC_REG_MEM_ADDR                0x0060
>>> +#define SFC_REG_TRIG                    0x0064
>>> +#define SFC_REG_SR                        0x0068
>>> +#define SFC_REG_SCR                        0x006c
>>> +#define SFC_REG_INTC                    0x0070
>>> +#define SFC_REG_FSM                        0x0074
>>> +#define SFC_REG_CGE                        0x0078
>>> +#define SFC_REG_TRAN_CFG1(n)            (0x009c + n * 4)
>>> +#define SFC_REG_DR                        0x1000
>>> +
>>> +/* bits within the GLB register */
>>> +#define GLB_TRAN_DIR_MASK                GENMASK(13, 13)
>>> +#define GLB_TRAN_DIR_WRITE                0x1
>>> +#define GLB_TRAN_DIR_READ                0x0
>> 
>> When it's a single bit - just use BIT().
> 
> 
> Sure.
> 
> 
>> 
>>> +#define GLB_THRESHOLD_MASK GENMASK(12, 7)
>>> +#define GLB_OP_MODE_MASK                GENMASK(6, 6)
>> 
>> Same here, and I see it a few times below as well.
>> 
> 
> Sure.
> 
> 
>>> +#define GLB_OP_MODE_DMA                    0x1
>>> +#define GLB_OP_MODE_SLAVE                0x0
>>> +#define GLB_PHASE_NUM_MASK                GENMASK(5, 3)
>>> +#define GLB_WP_EN                        BIT(2)
>>> +#define GLB_BURST_MD_MASK                GENMASK(1, 0)
>>> +#define GLB_BURST_MD_INCR32                0x3
>>> +#define GLB_BURST_MD_INCR16                0x2
>>> +#define GLB_BURST_MD_INCR8                0x1
>>> +#define GLB_BURST_MD_INCR4                0x0
>>> +
>>> +/* bits within the DEV_CONF register */
>>> +#define DEV_CONF_SMP_DELAY_MASK            GENMASK(20, 16)
>>> +#define DEV_CONF_SMP_DELAY_180DEG        0x4
>>> +#define DEV_CONF_SMP_DELAY_HALF_CYCLE    0x1
>>> +#define DEV_CONF_CMD_TYPE_MASK            GENMASK(15, 15)
>>> +#define DEV_CONF_CMD_TYPE_16BIT            0x1
>>> +#define DEV_CONF_CMD_TYPE_8BIT            0x0
>>> +#define DEV_CONF_STA_TYPE_MASK            GENMASK(14, 13)
>>> +#define DEV_CONF_THOLD_MASK                GENMASK(12, 11)
>>> +#define DEV_CONF_TSETUP_MASK            GENMASK(10, 9)
>>> +#define DEV_CONF_TSH_MASK                GENMASK(8, 5)
>>> +#define DEV_CONF_CPHA                    BIT(4)
>>> +#define DEV_CONF_CPOL                    BIT(3)
>>> +#define DEV_CONF_CE_DL                    BIT(2)
>>> +#define DEV_CONF_HOLD_DL                BIT(1)
>>> +#define DEV_CONF_WP_DL                    BIT(0)
>>> +
>>> +/* bits within the TRAN_CONF(n) register */
>>> +#define TRAN_CONF_TRAN_MODE_MASK        GENMASK(31, 29)
>>> +#define TRAN_CONF_ADDR_WIDTH_MASK        GENMASK(28, 26)
>>> +#define TRAN_CONF_POLL_EN                BIT(25)
>>> +#define TRAN_CONF_CMD_EN                BIT(24)
>>> +#define TRAN_CONF_PHASE_FORMAT_MASK        GENMASK(23, 23)
>>> +#define TRAN_CONF_DMY_BITS_MASK            GENMASK(22, 17)
>>> +#define TRAN_CONF_DATA_EN                BIT(16)
>>> +#define TRAN_CONF_CMD_MASK                GENMASK(15, 0)
>>> +
>>> +/* bits within the TRIG register */
>>> +#define TRIG_FLUSH                        BIT(2)
>>> +#define TRIG_STOP                        BIT(1)
>>> +#define TRIG_START                        BIT(0)
>>> +
>>> +/* bits within the SR register */
>>> +#define SR_FIFO_NUM_MASK                GENMASK(22, 16)
>>> +#define SR_END                            BIT(4)
>>> +#define SR_TRAN_REQ                        BIT(3)
>>> +#define SR_RECE_REQ                        BIT(2)
>>> +#define SR_OVER                            BIT(1)
>>> +#define SR_UNDER                        BIT(0)
>>> +
>>> +/* bits within the SCR register */
>>> +#define SCR_CLR_END                        BIT(4)
>>> +#define SCR_CLR_TREQ                    BIT(3)
>>> +#define SCR_CLR_RREQ                    BIT(2)
>>> +#define SCR_CLR_OVER                    BIT(1)
>>> +#define SCR_CLR_UNDER                    BIT(0)
>>> +
>>> +/* bits within the INTC register */
>>> +#define INTC_MASK_END                    BIT(4)
>>> +#define INTC_MASK_TREQ                    BIT(3)
>>> +#define INTC_MASK_RREQ                    BIT(2)
>>> +#define INTC_MASK_OVER                    BIT(1)
>>> +#define INTC_MASK_UNDER                    BIT(0)
>>> +
>>> +/* bits within the TRAN_CFG1(n) register */
>>> +#define TRAN_CFG1_TRAN_MODE_MASK        GENMASK(7, 4)
>>> +
>>> +#define TRAN_MODE_STANDARD                0
>>> +#define TRAN_MODE_DUAL_DATA                1
>>> +#define TRAN_MODE_DUAL_IO                2
>>> +#define TRAN_MODE_DUAL_FULL                3
>>> +#define TRAN_MODE_QUAD_DATA                5
>>> +#define TRAN_MODE_QUAD_IO                6
>>> +#define TRAN_MODE_QUAD_FULL                7
>>> +#define TRAN_MODE_OCTAL_DATA            9
>>> +#define TRAN_MODE_OCTAL_IO                10
>>> +#define TRAN_MODE_OCTAL_FULL            11
>>> +
>>> +#define INGENIC_SFC_FIFO_SIZE            (64 * 4)
>>> +
>>> +#define INGENIC_SFC_TRANSFER_TIMEOUT    1000
>> 
>> Maybe add the unit name in the macro as well - 
>> INGENIC_SFC_TRANSFER_TIMEOUT_MS.
> 
> 
> Sure.
> 
> 
>> 
>>> +
>>> +enum ingenic_sfc_version {
>>> +    ID_X1000,
>>> +    ID_X1600,
>>> +    ID_X2000,
>>> +};
>>> +
>>> +struct ingenic_soc_info {
>>> +    enum ingenic_sfc_version version;
>>> +
>>> +    unsigned int max_bus_width;
>>> +
>>> +    const u32 tran_mode_mask;
>>> +};
>>> +
>>> +struct ingenic_sfc {
>>> +    const struct ingenic_soc_info *soc_info;
>>> +
>>> +    void __iomem *base;
>>> +    struct device *dev;
>>> +    struct clk *clk;
>>> +    int irq;
>>> +
>>> +    struct completion completion;
>>> +};
>>> +
>>> +static irqreturn_t ingenic_sfc_irq_handler(int irq, void *data)
>>> +{
>>> +    struct ingenic_sfc *sfc = data;
>>> +
>>> +    writel(0x1f, sfc->base + SFC_REG_INTC);
>>> +
>>> +    complete(&sfc->completion);
>>> +
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int ingenic_sfc_adjust_op_size(struct spi_mem *mem, struct 
>>> spi_mem_op *op)
>>> +{
>>> +    uintptr_t addr = (uintptr_t)op->data.buf.in;
>>> +
>>> +    if (op->data.nbytes > INGENIC_SFC_FIFO_SIZE && 
>>> !IS_ALIGNED(addr, 4))
>>> +        op->data.nbytes = INGENIC_SFC_FIFO_SIZE;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static bool ingenic_sfc_supports_op(struct spi_mem *mem, const 
>>> struct spi_mem_op *op)
>>> +{
>>> +    struct spi_device *spi = mem->spi;
>>> +    struct ingenic_sfc *sfc = 
>>> spi_controller_get_devdata(spi->master);
>>> +    uintptr_t addr = (uintptr_t)op->data.buf.in;
>>> +
>>> +    /* The controller only supports Standard SPI mode, Duall mode, 
>>> Quad mode and Octal mode. */
>> 
>> Dual*
> 
> 
> Oops, will fix it.
> 
> 
>> 
>>> +    if (op->cmd.buswidth > sfc->soc_info->max_bus_width ||
>>> +        op->addr.buswidth > sfc->soc_info->max_bus_width ||
>>> +        op->dummy.buswidth > sfc->soc_info->max_bus_width ||
>>> +        op->data.buswidth > sfc->soc_info->max_bus_width)
>>> +        return false;
>>> +
>>> +    /* Max 32 dummy clock cycles supported */
>>> +    if (op->dummy.nbytes && op->dummy.nbytes * 8 / 
>>> op->dummy.buswidth > 32)
>>> +        return false;
>>> +
>>> +    /* Max rx data length, check controller limits and alignment */
>>> +    if (op->data.dir == SPI_MEM_DATA_IN &&
>>> +        op->data.nbytes > INGENIC_SFC_FIFO_SIZE && 
>>> !IS_ALIGNED(addr, 4))
>> 
>> This does the same check than in ingenic_sfc_adjust_op_size(), maybe 
>> move it to a new inline function?
>> 
> 
> Sure.
> 
> 
>>> +        return false;
>>> +
>>> +    /* Max 6 bytes address width supported */
>>> +    if (op->addr.nbytes > 6)
>>> +        return false;
>>> +
>>> +    return spi_mem_default_supports_op(mem, op);
>>> +}
>>> +
>>> +static void ingenic_sfc_set_transfer_mode(struct ingenic_sfc *sfc, 
>>> const struct spi_mem_op *op)
>>> +{
>>> +    int val;
>>> +
>>> +    val = readl(sfc->base + (sfc->soc_info->version >= ID_X1600 ?
>>> +            SFC_REG_TRAN_CFG1(0) : SFC_REG_TRAN_CONF(0)));
>> 
>> As Krzysztof said - ugh.
>> 
> 
> Will fix it.
> 
> 
>> Also, instead of having a "version" enum in your soc_info, why not 
>> just have a "reg_conf" field that gives you directly the right 
>> register?
>> 
> 
> We still need a version to distinguish the SFC before X1600 SoC and
> the SFC after X1600 SoC, because in addition to the difference in the
> cfg register, another difference is that the SFC before X1600 SoC does
> not support address unaligned RX DMA transfer, while SFC in X1600 and
> later SoCs can support this feature.

Then add a .dma_supports_unaligned_address field.

> 
> 
>> 
>>> +    val &= ~sfc->soc_info->tran_mode_mask;
>>> +    if (op->cmd.buswidth == 8)
>>> +        val |= (TRAN_MODE_OCTAL_FULL << 
>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>> +                sfc->soc_info->tran_mode_mask;
>> 
>> Looks like you're really trying to reinvent the wheel.
>> 
>> val |= FIELD_PREP(sfc->soc_info->tran_mode_mask, 
>> TRAN_MODE_OCTAL_FULL);
>> 
>> using <linux/bitfield.h>.
>> 
>> Also, just define a 'mode' variable and set it in your if/else 
>> blocks, that would look much better. Then you can set val |= 
>> FIELD_PREP(..., mode) at the end.
> 
> 
> Sure, will change this in the next version.
> 
> 
>> 
>>> +    else if (op->cmd.buswidth == 4)
>>> +        val |= (TRAN_MODE_QUAD_FULL << 
>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>> +                sfc->soc_info->tran_mode_mask;
>>> +    else if (op->cmd.buswidth == 2)
>>> +        val |= (TRAN_MODE_DUAL_FULL << 
>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>> +                sfc->soc_info->tran_mode_mask;
>>> +    else if (op->addr.buswidth == 8)
>>> +        val |= (TRAN_MODE_OCTAL_IO << 
>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>> +                sfc->soc_info->tran_mode_mask;
>>> +    else if (op->addr.buswidth == 4)
>>> +        val |= (TRAN_MODE_QUAD_IO << 
>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>> +                sfc->soc_info->tran_mode_mask;
>>> +    else if (op->addr.buswidth == 2)
>>> +        val |= (TRAN_MODE_DUAL_IO << 
>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>> +                sfc->soc_info->tran_mode_mask;
>>> +    else if (op->data.buswidth == 8)
>>> +        val |= (TRAN_MODE_OCTAL_DATA << 
>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>> +                sfc->soc_info->tran_mode_mask;
>>> +    else if (op->data.buswidth == 4)
>>> +        val |= (TRAN_MODE_QUAD_DATA << 
>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>> +                sfc->soc_info->tran_mode_mask;
>>> +    else if (op->data.buswidth == 2)
>>> +        val |= (TRAN_MODE_DUAL_DATA << 
>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>> +                sfc->soc_info->tran_mode_mask;
>>> +    else
>>> +        val |= (TRAN_MODE_STANDARD << 
>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>> +                sfc->soc_info->tran_mode_mask;
>>> +    writel(val, sfc->base + (sfc->soc_info->version >= ID_X1600 ?
>>> +            SFC_REG_TRAN_CFG1(0) : SFC_REG_TRAN_CONF(0)));
>>> +}
>>> +
>>> +/*
>>> + * We only need PIO mode to handle the SPI_MEM_NO_DATA transfers,
>>> + * and the unaligned accesses in SPI_MEM_DATA_IN transfers.
>>> + */
>>> +static void ingenic_sfc_read_rxfifo(struct ingenic_sfc *sfc, u8 
>>> *to, unsigned int len)
>>> +{
>>> +    void __iomem *from;
>>> +
>>> +    from = sfc->base + SFC_REG_DR;
>>> +
>>> +    for (; len >= 4; len -= 4, to += 4) {
>>> +        u32 val = __raw_readl(from);
>>> +        memcpy(to, &val, 4);
>> 
>> No need to use memcpy for 4 bytes. You can do: put_unaligned(val, 
>> (u32 *)to);
>> 
> 
> Sure.
> 
> 
>>> +    }
>>> +
>>> +    if (len) {
>>> +        u32 val = __raw_readl(from);
>>> +        memcpy(to, &val, len);
>> 
>> Hmm, I'm not sure that is endian-safe. I would prefer if you copied 
>> byte by byte.
>> 
> 
> Sure.
> 
> 
>>> +    }
>>> +}
>>> +
>>> +static int ingenic_sfc_exec_op_pio(struct ingenic_sfc *sfc, const 
>>> struct spi_mem_op *op)
>>> +{
>>> +    int ret, val;
>>> +
>>> +    val = readl(sfc->base + SFC_REG_GLB);
>>> +    u32p_replace_bits(&val, GLB_TRAN_DIR_READ, GLB_TRAN_DIR_MASK);
>>> +    u32p_replace_bits(&val, GLB_OP_MODE_SLAVE, GLB_OP_MODE_MASK);
>>> +    writel(val, sfc->base + SFC_REG_GLB);
>> 
>> By the way, have you considered using regmap?
>> 
>> It would give you things like regmap_update_bits() for this kind of 
>> things, and regmap_field() to handle your conf register being at a 
>> different address across SoCs.
>> 
> 
> It seems that the overhead of using regmap will be greater than using 
> readl and writel,
> the purpose of using readl and writel is to minimize overhead and 
> maximize performance. :)

If you really think that this is a valid reason not to use regmap, then 
prove it with a benchmark, because I'm *very* doubtful about your claim.

Cheers,
-Paul

>>> +
>>> +    val = TRAN_CONF_CMD_EN | op->cmd.opcode;
>>> +
>>> +    if (op->addr.nbytes > 0) {
>>> +        val |= FIELD_PREP(TRAN_CONF_ADDR_WIDTH_MASK, 
>>> op->addr.nbytes);
>>> +
>>> +        writel(op->addr.val & 0xffffffff, sfc->base + 
>>> SFC_REG_DEV_ADDR(0));
>>> +        writel(op->addr.val >> 32, sfc->base + 
>>> SFC_REG_DEV_ADDR_PLUS(0));
>>> +    }
>>> +
>>> +    if (op->dummy.nbytes > 0)
>>> +        val |= FIELD_PREP(TRAN_CONF_DMY_BITS_MASK,
>>> +                op->dummy.nbytes * 8 / op->dummy.buswidth);
>>> +
>>> +    if (op->data.nbytes > 0)
>>> +        val |= TRAN_CONF_DATA_EN;
>>> +
>>> +    writel(val, sfc->base + SFC_REG_TRAN_CONF(0));
>>> +    writel(op->data.nbytes, sfc->base + SFC_REG_TRAN_LEN);
>>> +
>>> +    ingenic_sfc_set_transfer_mode(sfc, op);
>>> +
>>> +    writel(0x1f, sfc->base + SFC_REG_SCR);
>> 
>> Random 0x1f value here, maybe use a macro?
> 
> 
> Sure.
> 
> 
>> 
>>> +    writel(~(INTC_MASK_END | INTC_MASK_RREQ), sfc->base + 
>>> SFC_REG_INTC);
>>> +
>>> +    writel(0, sfc->base + SFC_REG_MEM_ADDR);
>>> +
>>> +    writel(TRIG_FLUSH, sfc->base + SFC_REG_TRIG);
>>> +    writel(TRIG_START, sfc->base + SFC_REG_TRIG);
>>> +
>>> +    ret = wait_for_completion_timeout(&sfc->completion,
>>> +            msecs_to_jiffies(INGENIC_SFC_TRANSFER_TIMEOUT));
>>> +    if (!ret) {
>>> +        writel(0x1f, sfc->base + SFC_REG_INTC);
>>> +        writel(0x1f, sfc->base + SFC_REG_SCR);
>>> +        dev_err(sfc->dev, "line:%d Timeout for ACK from SFC 
>>> device\n", __LINE__);
>>> +        return -ETIMEDOUT;
>>> +    }
>>> +
>>> +    ingenic_sfc_read_rxfifo(sfc, op->data.buf.in, op->data.nbytes);
>>> +    readl_poll_timeout(sfc->base + SFC_REG_SR, val, val & SR_END, 
>>> 10, 0);
>> 
>> Infinite timeout? Is that very wise?
> 
> 
> Will fix it in the next version.
> 
> 
>> 
>>> +
>>> +    writel(INTC_MASK_END | INTC_MASK_RREQ, sfc->base + 
>>> SFC_REG_SCR);
>>> +    writel(TRIG_STOP, sfc->base + SFC_REG_TRIG);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ingenic_sfc_exec_op_dma(struct ingenic_sfc *sfc, const 
>>> struct spi_mem_op *op)
>>> +{
>>> +    dma_addr_t addr;
>>> +    int ret, val;
>>> +
>>> +    val = readl(sfc->base + SFC_REG_GLB);
>>> +    u32p_replace_bits(&val, op->data.dir == SPI_MEM_DATA_IN ?
>>> +            GLB_TRAN_DIR_READ : GLB_TRAN_DIR_WRITE, 
>>> GLB_TRAN_DIR_MASK);
>>> +    u32p_replace_bits(&val, GLB_OP_MODE_DMA, GLB_OP_MODE_MASK);
>>> +    writel(val, sfc->base + SFC_REG_GLB);
>>> +
>>> +    val = TRAN_CONF_CMD_EN | op->cmd.opcode;
>>> +
>>> +    if (op->addr.nbytes > 0) {
>>> +        val |= FIELD_PREP(TRAN_CONF_ADDR_WIDTH_MASK, 
>>> op->addr.nbytes);
>>> +        writel(op->addr.val & 0xffffffff, sfc->base + 
>>> SFC_REG_DEV_ADDR(0));
>>> +        writel(op->addr.val >> 32, sfc->base + 
>>> SFC_REG_DEV_ADDR_PLUS(0));
>>> +    }
>>> +
>>> +    if (op->dummy.nbytes > 0)
>>> +        val |= FIELD_PREP(TRAN_CONF_DMY_BITS_MASK,
>>> +                op->dummy.nbytes * 8 / op->dummy.buswidth);
>>> +
>>> +    if (op->data.nbytes > 0)
>>> +        val |= TRAN_CONF_DATA_EN;
>> 
>> There's a lot of code duplication here with 
>> ingenic_sfc_exec_op_pio(). A lot can be factorized.
> 
> 
> Sure, I will try to do it.
> 
> 
>> 
>>> +
>>> +    writel(val, sfc->base + SFC_REG_TRAN_CONF(0));
>>> +    writel(op->data.nbytes, sfc->base + SFC_REG_TRAN_LEN);
>>> +
>>> +    ingenic_sfc_set_transfer_mode(sfc, op);
>>> +
>>> +    writel(0x1f, sfc->base + SFC_REG_SCR);
>>> +    writel(~INTC_MASK_END, sfc->base + SFC_REG_INTC);
>>> +
>>> +    switch (op->data.dir) {
>>> +    case SPI_MEM_DATA_IN:
>>> +        addr = dma_map_single(sfc->dev, op->data.buf.in, 
>>> op->data.nbytes, DMA_FROM_DEVICE);
>>> +        if (dma_mapping_error(sfc->dev, addr)) {
>>> +            dev_err(sfc->dev, "RX DMA　memory not mapped\n");
>>> +            return -ENOMEM;
>>> +        }
>>> +
>>> +        writel(addr, sfc->base + SFC_REG_MEM_ADDR);
>>> +        break;
>>> +
>>> +    case SPI_MEM_DATA_OUT:
>>> +        addr = dma_map_single(sfc->dev, (void *)op->data.buf.out,
>>> +                op->data.nbytes, DMA_TO_DEVICE);
>>> +        if (dma_mapping_error(sfc->dev, addr)) {
>>> +            dev_err(sfc->dev, "TX DMA　memory not mapped\n");
>>> +            return -ENOMEM;
>>> +        }
>>> +
>>> +        writel(addr, sfc->base + SFC_REG_MEM_ADDR);
>>> +        break;
>>> +
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    writel(TRIG_START, sfc->base + SFC_REG_TRIG);
>>> +
>>> +    ret = wait_for_completion_timeout(&sfc->completion,
>>> +            msecs_to_jiffies(INGENIC_SFC_TRANSFER_TIMEOUT));
>>> +    if (!ret) {
>>> +        writel(0x1f, sfc->base + SFC_REG_INTC);
>>> +        writel(0x1f, sfc->base + SFC_REG_SCR);
>>> +        dev_err(sfc->dev, "line:%d Timeout for ACK from SFC 
>>> device\n", __LINE__);
>>> +        return -ETIMEDOUT;
>>> +    }
>>> +
>>> +    dma_unmap_single(sfc->dev, addr, op->data.nbytes,
>>> +            op->data.dir == SPI_MEM_DATA_IN ? DMA_FROM_DEVICE : 
>>> DMA_TO_DEVICE);
>> 
>> Use a small inline function for that too. My personal rule is that 
>> ?: is fine if the line fits in 80 characters, but if you have to 
>> break, then you really need to move it somewhere else.
>> 
> 
> Sure.
> 
> 
>>> +
>>> +    writel(INTC_MASK_END, sfc->base + SFC_REG_SCR);
>>> +    writel(TRIG_STOP, sfc->base + SFC_REG_TRIG);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ingenic_sfc_exec_op(struct spi_mem *mem, const struct 
>>> spi_mem_op *op)
>>> +{
>>> +    struct spi_device *spi = mem->spi;
>>> +    struct ingenic_sfc *sfc = 
>>> spi_controller_get_devdata(spi->master);
>>> +    uintptr_t addr = (uintptr_t)op->data.buf.in;
>>> +
>>> +    init_completion(&sfc->completion);
>>> +
>>> +    switch (op->data.dir) {
>>> +    case SPI_MEM_DATA_IN:
>>> +        if (sfc->soc_info->version >= ID_X1600 || IS_ALIGNED(addr, 
>>> 4))
>>> +            break;
>>> +
>>> +        fallthrough;
>>> +
>>> +    case SPI_MEM_NO_DATA:
>>> +        return ingenic_sfc_exec_op_pio(sfc, op);
>>> +
>>> +    default:
>>> +        break;
>>> +    }
>>> +
>>> +    return ingenic_sfc_exec_op_dma(sfc, op);
>>> +}
>>> +
>>> +static int ingenic_sfc_poll_status(struct spi_mem *mem, const 
>>> struct spi_mem_op *op,
>>> +            u16 mask, u16 match, unsigned long initial_delay_us,
>>> +            unsigned long polling_delay_us, unsigned long 
>>> timeout_ms)
>>> +{
>>> +    struct spi_device *spi = mem->spi;
>>> +    struct ingenic_sfc *sfc = 
>>> spi_controller_get_devdata(spi->master);
>>> +    int ret, val;
>>> +
>>> +    init_completion(&sfc->completion);
>>> +
>>> +    val = readl(sfc->base + SFC_REG_GLB);
>>> +    u32p_replace_bits(&val, GLB_TRAN_DIR_READ, GLB_TRAN_DIR_MASK);
>>> +    u32p_replace_bits(&val, GLB_OP_MODE_SLAVE, GLB_OP_MODE_MASK);
>>> +    writel(val, sfc->base + SFC_REG_GLB);
>>> +
>>> +    writel(match, sfc->base + SFC_REG_DEV_STA_EXP);
>>> +    writel(mask, sfc->base + SFC_REG_DEV_STA_MSK);
>>> +
>>> +    val = TRAN_CONF_POLL_EN | TRAN_CONF_CMD_EN | op->cmd.opcode;
>>> +
>>> +    if (op->addr.nbytes > 0) {
>>> +        val |= FIELD_PREP(TRAN_CONF_ADDR_WIDTH_MASK, 
>>> op->addr.nbytes);
>>> +
>>> +        writel(op->addr.val & 0xffffffff, sfc->base + 
>>> SFC_REG_DEV_ADDR(0));
>>> +        writel(op->addr.val >> 32, sfc->base + 
>>> SFC_REG_DEV_ADDR_PLUS(0));
>>> +    }
>>> +
>>> +    if (op->dummy.nbytes > 0)
>>> +        val |= FIELD_PREP(TRAN_CONF_DMY_BITS_MASK,
>>> +                op->dummy.nbytes * 8 / op->dummy.buswidth);
>>> +
>>> +    if (op->data.nbytes > 0)
>>> +        val |= TRAN_CONF_DATA_EN;
>>> +
>>> +    writel(val, sfc->base + SFC_REG_TRAN_CONF(0));
>>> +    writel(op->data.nbytes, sfc->base + SFC_REG_TRAN_LEN);
>>> +
>>> +    ingenic_sfc_set_transfer_mode(sfc, op);
>>> +
>>> +    writel(0x1f, sfc->base + SFC_REG_SCR);
>>> +    writel(~INTC_MASK_END, sfc->base + SFC_REG_INTC);
>>> +
>>> +    writel(0, sfc->base + SFC_REG_MEM_ADDR);
>>> +
>>> +    writel(TRIG_START, sfc->base + SFC_REG_TRIG);
>>> +
>>> +    ret = wait_for_completion_timeout(&sfc->completion,
>>> +            msecs_to_jiffies(INGENIC_SFC_TRANSFER_TIMEOUT));
>>> +    if (!ret) {
>>> +        writel(0x1f, sfc->base + SFC_REG_INTC);
>>> +        writel(0x1f, sfc->base + SFC_REG_SCR);
>>> +        dev_err(sfc->dev, "line:%d Timeout for ACK from SFC 
>>> device\n", __LINE__);
>>> +        return -ETIMEDOUT;
>>> +    }
>>> +
>>> +    writel(SCR_CLR_END, sfc->base + SFC_REG_SCR);
>>> +    writel(TRIG_STOP, sfc->base + SFC_REG_TRIG);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct spi_controller_mem_ops ingenic_sfc_mem_ops = {
>>> +    .adjust_op_size = ingenic_sfc_adjust_op_size,
>>> +    .supports_op = ingenic_sfc_supports_op,
>>> +    .exec_op = ingenic_sfc_exec_op,
>>> +    .poll_status = ingenic_sfc_poll_status,
>>> +};
>>> +
>>> +static int ingenic_sfc_setup(struct spi_device *spi)
>>> +{
>>> +    struct ingenic_sfc *sfc = 
>>> spi_controller_get_devdata(spi->master);
>>> +    unsigned long rate;
>>> +    int ret, val;
>>> +
>>> +    if (!spi->max_speed_hz)
>>> +        return -EINVAL;
>> 
>> Maybe set a sane default?
> 
> 
> Sure.
> 
> 
>> 
>>> +
>>> +    ret = clk_set_rate(sfc->clk, spi->max_speed_hz * 2);
>>> +    if (ret)
>>> +        return -EINVAL;
>>> +
>>> +    writel(TRIG_STOP, sfc->base + SFC_REG_TRIG);
>>> +    writel(0, sfc->base + SFC_REG_DEV_CONF);
>>> +    writel(0, sfc->base + SFC_REG_CGE);
>>> +
>>> +    val = readl(sfc->base + SFC_REG_GLB);
>>> +    u32p_replace_bits(&val, 64 - 1, GLB_THRESHOLD_MASK);
>>> +    writel(val, sfc->base + SFC_REG_GLB);
>>> +
>>> +    val = readl(sfc->base + SFC_REG_DEV_CONF);
>>> +
>>> +    /* cpha bit:0 , cpol bit:0 */
>>> +    val &= ~(DEV_CONF_CPHA | DEV_CONF_CPOL);
>>> +    val |= spi->mode & SPI_CPHA ? DEV_CONF_CPHA : 0;
>>> +    val |= spi->mode & SPI_CPOL ? DEV_CONF_CPOL : 0;
>>> +
>>> +    /* ce_dl bit:1, hold bit:1, wp bit:1 */
>>> +    val |= (DEV_CONF_CE_DL | DEV_CONF_HOLD_DL | DEV_CONF_WP_DL);
>>> +
>>> +    writel(val, sfc->base + SFC_REG_DEV_CONF);
>>> +
>>> +    val = readl(sfc->base + SFC_REG_GLB);
>>> +    u32p_replace_bits(&val, GLB_OP_MODE_SLAVE, GLB_OP_MODE_MASK);
>>> +    writel(val, sfc->base + SFC_REG_GLB);
>>> +
>>> +    rate = clk_get_rate(sfc->clk);
>> 
>> I'd suggest using clk_round_rate() before clk_set_rate() because 
>> then you know what frequency it's going to be, and you don't have 
>> to call clk_get_rate() afterwards.
>> 
> 
> Sure, will try.
> 
> 
> Thanks and best regards!
> 
> 
>> Cheers,
>> -Paul
>> 
>>> +    val = readl(sfc->base + SFC_REG_DEV_CONF);
>>> +    if (sfc->soc_info->version >= ID_X1600 && rate >= 200000000)
>>> +        u32p_replace_bits(&val, DEV_CONF_SMP_DELAY_180DEG, 
>>> DEV_CONF_SMP_DELAY_MASK);
>>> +    else if (sfc->soc_info->version == ID_X1000 && rate >= 
>>> 100000000)
>>> +        u32p_replace_bits(&val, DEV_CONF_SMP_DELAY_HALF_CYCLE, 
>>> DEV_CONF_SMP_DELAY_MASK);
>>> +    writel(val, sfc->base + SFC_REG_DEV_CONF);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int ingenic_sfc_probe(struct platform_device *pdev)
>>> +{
>>> +    struct ingenic_sfc *sfc;
>>> +    struct spi_controller *ctlr;
>>> +    int ret;
>>> +
>>> +    ctlr = spi_alloc_master(&pdev->dev, sizeof(*sfc));
>>> +    if (!ctlr)
>>> +        return -ENOMEM;
>>> +
>>> +    sfc = spi_controller_get_devdata(ctlr);
>>> +
>>> +    sfc->soc_info = of_device_get_match_data(&pdev->dev);
>>> +    if (!sfc->soc_info) {
>>> +        dev_err(&pdev->dev, "No of match data provided\n");
>>> +        ret = -ENODEV;
>>> +        goto err_put_master;
>>> +    }
>>> +
>>> +    sfc->base = devm_platform_ioremap_resource(pdev, 0);
>>> +    if (IS_ERR(sfc->base)) {
>>> +        ret = PTR_ERR(sfc->base);
>>> +        goto err_put_master;
>>> +    }
>>> +
>>> +    sfc->clk = devm_clk_get(&pdev->dev, "sfc");
>>> +    if (IS_ERR(sfc->clk)) {
>>> +        ret = IS_ERR(sfc->clk);
>>> +        goto err_put_master;
>>> +    }
>>> +
>>> +    ret = clk_prepare_enable(sfc->clk);
>>> +    if (ret)
>>> +        goto err_put_master;
>>> +
>>> +    sfc->irq = platform_get_irq(pdev, 0);
>>> +    if (sfc->irq < 0) {
>>> +        ret = sfc->irq;
>>> +        goto err_put_master;
>>> +    }
>>> +
>>> +    sfc->dev = &pdev->dev;
>>> +
>>> +    platform_set_drvdata(pdev, sfc);
>>> +
>>> +    ret = devm_request_irq(&pdev->dev, sfc->irq, 
>>> ingenic_sfc_irq_handler, 0,
>>> +            dev_name(&pdev->dev), sfc);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "Failed to request irq%d, ret = %d\n", 
>>> sfc->irq, ret);
>>> +        goto err_put_master;
>>> +    }
>>> +
>>> +    ctlr->bus_num = -1;
>>> +    ctlr->num_chipselect = 1;
>>> +    ctlr->mem_ops = &ingenic_sfc_mem_ops;
>>> +    ctlr->dev.of_node = pdev->dev.of_node;
>>> +    ctlr->setup = ingenic_sfc_setup;
>>> +    ctlr->mode_bits = SPI_CPHA | SPI_CPOL |
>>> +            SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD;
>>> +    if (sfc->soc_info->version >= ID_X2000)
>>> +        ctlr->mode_bits |= SPI_RX_OCTAL | SPI_TX_OCTAL;
>>> +
>>> +    ret = devm_spi_register_controller(&pdev->dev, ctlr);
>>> +    if (ret)
>>> +        goto err_put_master;
>>> +
>>> +    return 0;
>>> +
>>> +err_put_master:
>>> +    spi_master_put(ctlr);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static const struct ingenic_soc_info x1000_soc_info = {
>>> +    .version = ID_X1000,
>>> +
>>> +    .max_bus_width = 4,
>>> +
>>> +    .tran_mode_mask = TRAN_CONF_TRAN_MODE_MASK,
>>> +};
>>> +
>>> +static const struct ingenic_soc_info x1600_soc_info = {
>>> +    .version = ID_X1600,
>>> +
>>> +    .max_bus_width = 4,
>>> +
>>> +    .tran_mode_mask = TRAN_CONF_TRAN_MODE_MASK,
>>> +};
>>> +
>>> +static const struct ingenic_soc_info x2000_soc_info = {
>>> +    .version = ID_X2000,
>>> +
>>> +    .max_bus_width = 8,
>>> +
>>> +    .tran_mode_mask = TRAN_CFG1_TRAN_MODE_MASK,
>>> +};
>>> +
>>> +static const struct of_device_id ingenic_sfc_of_matches[] = {
>>> +    { .compatible = "ingenic,x1000-sfc", .data = &x1000_soc_info },
>>> +    { .compatible = "ingenic,x1600-sfc", .data = &x1600_soc_info },
>>> +    { .compatible = "ingenic,x2000-sfc", .data = &x2000_soc_info },
>>> +    { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ingenic_sfc_of_matches);
>>> +
>>> +static struct platform_driver ingenic_sfc_driver = {
>>> +    .driver = {
>>> +        .name = "ingenic-sfc",
>>> +        .of_match_table = ingenic_sfc_of_matches,
>>> +    },
>>> +    .probe = ingenic_sfc_probe,
>>> +};
>>> +module_platform_driver(ingenic_sfc_driver);
>>> +
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_AUTHOR("周琰杰 (Zhou Yanjie) 
>>> <zhouyanjie@wanyeetech.com>");
>>> +MODULE_DESCRIPTION("Ingenic SoCs SPI Flash Controller Driver");
>>> --
>>> 2.7.4
>>> 
>>

On 7/24/22 04:07, Krzysztof Kozlowski wrote:
> On 23/07/2022 21:27, Mark Brown wrote:
>> On Sun, Jul 24, 2022 at 02:47:14AM +0800, Mike Yang wrote:
>>> On 7/24/22 01:43, Krzysztof Kozlowski wrote:
>>>> On 23/07/2022 18:50, Zhou Yanjie wrote:
>>
>>>>> No offense, does it really need to be named that way?
>>>>> I can't seem to find documentation with instructions on this :(
>>
>> ...
>>
>>>> All bindings are to follow this rule, so I don't understand why you
>>>> think it is an exception for you?
>>
>>> Zhou didn't ask you to make an exception. They have a valid
>>> point and they're asking why.
>>
>>> You may want to avoid further incidents of this kind by stop
>>> being bossy and actually writing a guideline of naming these
>>> .yaml files and publish it somewhere online.
>>
>> Yeah, I do have to say that I was also completely unaware that
>> there was any enforced convention here.
> 
> Indeed, it's not a enforced pattern. But there are many other
> insignificant ones which we also tend to forget during review, like
> using words "Device Tree bindings" in title or using unnecessary quotes
> around "refs" (also in ID of schema). It's not a big deal, but I ask
> when I notice it.

Good. Thanks for paying attention to these details.


>> Zhou already mentioned he was unable find the naming guidelines of these .yaml files.
>>
>> Apparently you think it's unacceptable for new contributors of a certain subsystem to use existing code as examples, and/or they're responsible for figuring out what's a good example and what's a bad one in the existing codebase.
> 
> It's everywhere in the kernel, what can I say? If you copy existing
> code, you might copy poor code...

Still, it shouldn't be a responsibility of new contributors to determine the quality of an existing piece of code, unless there are clear guidelines (i.e. one should use the new "cs-gpios" attribute in SPI controllers).

> 
>>
>>>
>>> It might never grow to new devices (because they might be different), so
>>> that is not really an argument.
>>
>> It is an argument. A very valid one.
>>
>> "they *might* be different". You may want to get your hands on real hardware and try another word. Or at least read the datasheets instead of believing your imagination.
>>
>> I would enjoy duplicating the st,stm32-spi.yaml into st,stm32{f,h}{0..7}-spi.yaml if I'm bored at a Sunday afternoon.
>>
>>>
>>> All bindings are to follow this rule, so I don't understand why you
>>> think it is an exception for you?
>>
>> Zhou didn't ask you to make an exception. They have a valid point and they're asking why.
> 
> Hm, everyone has the same valid point and such recommendation is to
> everyone, although it is nothing serious.
> 
>> You may want to avoid further incidents of this kind by stop being bossy and actually writing a guideline of naming these .yaml files and publish it somewhere online.
> 
> I did not see any incident here... Process of review includes comments
> and there is nothing bad happening when you receive a comment. No
> incident...


Okay. After careful inspection of the Ingenic datasheets, now I have the conclusion: The Ingenic X1000, X1021, X1500, X1501, X1520, X1600, X1800, X1830, X2000, X2100, X2500 have the same SFC controller.

X1600 has a newer version (let's say v2) of the SFC, and X2000-2500 have v3. Others have the original version (let's say v1). Each new version introduced new features such as arbitrary DMA sizes, and the rest features are the same.


So IMO the name "ingenic,sfc.yaml" is perfectly logical.


Regards,
Mike Yang

Hi Christophe,

On 2022/7/23 下午11:15, Christophe JAILLET wrote:
> Le 22/07/2022 à 18:48, 周琰杰 (Zhou Yanjie) a écrit :
>> Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC
>> from Ingenic.
>>
>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>> ---
>>   drivers/spi/Kconfig           |   9 +
>>   drivers/spi/Makefile          |   1 +
>>   drivers/spi/spi-ingenic-sfc.c | 662 
>> ++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 672 insertions(+)
>>   create mode 100644 drivers/spi/spi-ingenic-sfc.c
>>
>
> [...]
>
>> +static int ingenic_sfc_probe(struct platform_device *pdev)
>> +{
>> +    struct ingenic_sfc *sfc;
>> +    struct spi_controller *ctlr;
>> +    int ret;
>> +
>> +    ctlr = spi_alloc_master(&pdev->dev, sizeof(*sfc));
>> +    if (!ctlr)
>> +        return -ENOMEM;
>
> devm_spi_alloc_master()? (+ error handling simplification)
> Or there should be a .remove() function.
>

Sure, will do in the next version.


Thanks and best regards!


> CJ
>
>> +
>> +    sfc = spi_controller_get_devdata(ctlr);
>> +
>> +    sfc->soc_info = of_device_get_match_data(&pdev->dev);
>> +    if (!sfc->soc_info) {
>> +        dev_err(&pdev->dev, "No of match data provided\n");
>> +        ret = -ENODEV;
>> +        goto err_put_master;
>> +    }
>> +
>
> [...]

Hi Mark,

On 2022/7/24 上午3:32, Mark Brown wrote:
> On Sun, Jul 24, 2022 at 01:06:16AM +0800, Zhou Yanjie wrote:
>> On 2022/7/23 上午2:38, Mark Brown wrote:
>>>> +++ b/drivers/spi/spi-ingenic-sfc.c
>>>> @@ -0,0 +1,662 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +/*
>>>> + * Ingenic SoCs SPI Flash Controller Driver
>>> Please make the entire comment a C++ one so things look more
>>> intentional.
>> I'm sorry, I didn't understand well what you meant :(
>> Could you please explain a little more detail?
> The above comment block uses both C /* */ and C++ // style comments,
> please make it just use the C++ style.


Sure, will do in the next version.


>>>> +static irqreturn_t ingenic_sfc_irq_handler(int irq, void *data)
>>>> +{
>>>> +	struct ingenic_sfc *sfc = data;
>>>> +
>>>> +	writel(0x1f, sfc->base + SFC_REG_INTC);
>>>> +
>>>> +	complete(&sfc->completion);
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>> This doesn't pay any attention to any status registers in the chip so
>>> won't work if the interrupt is shared and won't notice any error reports
>>> from the device...
>> This interrupt is exclusively owned by SFC, do we still
>> need to perform the operation you said? I haven't done
>> these operations before because I want to minimize the
>> overhead and avoid affecting performance.
> Even if the device is not shared is there no possibility that the
> device can report an unexpected interrupt status?  It's not just
> the sharing case, it's also the fact that it looks like there's a
> status being reported but we're not checking it so if anything
> goes wrong then we're less likely to notice.  I'd worry about
> data corruption.


Sure, I will change this in the next version.


Thanks and best regards!

Hi Tomasz,

On 2022/7/23 下午10:47, Tomasz Maciej Nowak wrote:
> W dniu 22.07.2022 o 18:48, 周琰杰 (Zhou Yanjie) pisze:
>> 1.Use the spi-mem poll status APIs in SPI-NOR to reduce CPU load.
>> 2.Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC from Ingenic.
>>
>> Liu Jinghui and Aidan MacDonald provided a lot of assistance during the development of this driver.
>>
>> 周琰杰 (Zhou Yanjie) (3):
>>    mtd: spi-nor: Use the spi-mem poll status APIs.
>>    dt-bindings: SPI: Add Ingenic SFC bindings.
>>    SPI: Ingenic: Add SFC support for Ingenic SoCs.
>>
>>   .../devicetree/bindings/spi/ingenic,sfc.yaml       |  64 ++
>>   drivers/mtd/spi-nor/core.c                         |  42 +-
>>   drivers/spi/Kconfig                                |   9 +
>>   drivers/spi/Makefile                               |   1 +
>>   drivers/spi/spi-ingenic-sfc.c                      | 662 +++++++++++++++++++++
>>   5 files changed, 768 insertions(+), 10 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>   create mode 100755 drivers/spi/spi-ingenic-sfc.c
>>
> Even tough it's still early in revision process, I'll add my
> Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
>
> The test was performed with Damai DM6291A SoC which is a Ingenic X1000 IP
> but with 256 MiB RAM. No bugs yet observed on my side.


Thanks for you test!


>

288                    op->dummy.nbytes * 8 / op->dummy.buswidth);
  289    
  290        if (op->data.nbytes > 0)
  291            val |= TRAN_CONF_DATA_EN;
  292    
  293        writel(val, sfc->base + SFC_REG_TRAN_CONF(0));
  294        writel(op->data.nbytes, sfc->base + SFC_REG_TRAN_LEN);
  295    
  296       

Sent from my iPhone

> On Jul 23, 2022, at 9:27 PM, Zhou Yanjie <zhouyanjie@wanyeetech.com> wrote:
> 
> Hi Tomasz,
> 
>> On 2022/7/23 下午10:47, Tomasz Maciej Nowak wrote:
>> W dniu 22.07.2022 o 18:48, 周琰杰 (Zhou Yanjie) pisze:
>>> 1.Use the spi-mem poll status APIs in SPI-NOR to reduce CPU load.
>>> 2.Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC from Ingenic.
>>> 
>>> Liu Jinghui and Aidan MacDonald provided a lot of assistance during the development of this driver.
>>> 
>>> 周琰杰 (Zhou Yanjie) (3):
>>>   mtd: spi-nor: Use the spi-mem poll status APIs.
>>>   dt-bindings: SPI: Add Ingenic SFC bindings.
>>>   SPI: Ingenic: Add SFC support for Ingenic SoCs.
>>> 
>>>  .../devicetree/bindings/spi/ingenic,sfc.yaml       |  64 ++
>>>  drivers/mtd/spi-nor/core.c                         |  42 +-
>>>  drivers/spi/Kconfig                                |   9 +
>>>  drivers/spi/Makefile                               |   1 +
>>>  drivers/spi/spi-ingenic-sfc.c                      | 662 +++++++++++++++++++++
>>>  5 files changed, 768 insertions(+), 10 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>  create mode 100755 drivers/spi/spi-ingenic-sfc.c
>>> 
>> Even tough it's still early in revision process, I'll add my
>> Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
>> 
>> The test was performed with Damai DM6291A SoC which is a Ingenic X1000 IP
>> but with 256 MiB RAM. No bugs yet observed on my side.
> 
> 
> Thanks for you test!
> 
> 
>> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

288                    op->dummy.nbytes * 8 / op->dummy.buswidth);
  289    
  290        if (op->data.nbytes > 0)
  291            val |= TRAN_CONF_DATA_EN;
  292    
  293        writel(val, sfc->base + SFC_REG_TRAN_CONF(0));
  294        writel(op->data.nbytes, sfc->base + SFC_REG_TRAN_LEN);
  295    
  296       

Sent from my iPhone

> On Jul 23, 2022, at 9:27 PM, Zhou Yanjie <zhouyanjie@wanyeetech.com> wrote:
> 
> Hi Tomasz,
> 
>> On 2022/7/23 下午10:47, Tomasz Maciej Nowak wrote:
>> W dniu 22.07.2022 o 18:48, 周琰杰 (Zhou Yanjie) pisze:
>>> 1.Use the spi-mem poll status APIs in SPI-NOR to reduce CPU load.
>>> 2.Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC from Ingenic.
>>> 
>>> Liu Jinghui and Aidan MacDonald provided a lot of assistance during the development of this driver.
>>> 
>>> 周琰杰 (Zhou Yanjie) (3):
>>>   mtd: spi-nor: Use the spi-mem poll status APIs.
>>>   dt-bindings: SPI: Add Ingenic SFC bindings.
>>>   SPI: Ingenic: Add SFC support for Ingenic SoCs.
>>> 
>>>  .../devicetree/bindings/spi/ingenic,sfc.yaml       |  64 ++
>>>  drivers/mtd/spi-nor/core.c                         |  42 +-
>>>  drivers/spi/Kconfig                                |   9 +
>>>  drivers/spi/Makefile                               |   1 +
>>>  drivers/spi/spi-ingenic-sfc.c                      | 662 +++++++++++++++++++++
>>>  5 files changed, 768 insertions(+), 10 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>  create mode 100755 drivers/spi/spi-ingenic-sfc.c
>>> 
>> Even tough it's still early in revision process, I'll add my
>> Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
>> 
>> The test was performed with Damai DM6291A SoC which is a Ingenic X1000 IP
>> but with 256 MiB RAM. No bugs yet observed on my side.
> 
> 
> Thanks for you test!
> 
> 
>> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😚😍😚😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍

Omg thank you Janika!

I’m glad to see everything is okay just tell them next time!

Love ya babes!

> On Jul 23, 2022, at 9:27 PM, Zhou Yanjie <zhouyanjie@wanyeetech.com> wrote:
> 
> Hi Tomasz,
> 
>> On 2022/7/23 下午10:47, Tomasz Maciej Nowak wrote:
>> W dniu 22.07.2022 o 18:48, 周琰杰 (Zhou Yanjie) pisze:
>>> 1.Use the spi-mem poll status APIs in SPI-NOR to reduce CPU load.
>>> 2.Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC from Ingenic.
>>> 
>>> Liu Jinghui and Aidan MacDonald provided a lot of assistance during the development of this driver.
>>> 
>>> 周琰杰 (Zhou Yanjie) (3):
>>>   mtd: spi-nor: Use the spi-mem poll status APIs.
>>>   dt-bindings: SPI: Add Ingenic SFC bindings.
>>>   SPI: Ingenic: Add SFC support for Ingenic SoCs.
>>> 
>>>  .../devicetree/bindings/spi/ingenic,sfc.yaml       |  64 ++
>>>  drivers/mtd/spi-nor/core.c                         |  42 +-
>>>  drivers/spi/Kconfig                                |   9 +
>>>  drivers/spi/Makefile                               |   1 +
>>>  drivers/spi/spi-ingenic-sfc.c                      | 662 +++++++++++++++++++++
>>>  5 files changed, 768 insertions(+), 10 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>  create mode 100755 drivers/spi/spi-ingenic-sfc.c
>>> 
>> Even tough it's still early in revision process, I'll add my
>> Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
>> 
>> The test was performed with Damai DM6291A SoC which is a Ingenic X1000 IP
>> but with 256 MiB RAM. No bugs yet observed on my side.
> 
> 
> Thanks for you test!
> 
> 
>> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

No problem!!!
I forget to tell them. It’s okay though! 👌👍
😚😚😚😚😚😚😚😚😚😚😚😚😚😚☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️☺️
I’m blushing! Last night was fun.

> On Jul 23, 2022, at 9:30 PM, Vanessa Page <Vebpe@outlook.com> wrote:
> 
> 🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰🥰😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😚😍😚😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍😍
> 
> Omg thank you Janika!
> 
> I’m glad to see everything is okay just tell them next time!
> 
> Love ya babes!
> 
>> On Jul 23, 2022, at 9:27 PM, Zhou Yanjie <zhouyanjie@wanyeetech.com> wrote:
>> 
>> Hi Tomasz,
>> 
>>>> On 2022/7/23 下午10:47, Tomasz Maciej Nowak wrote:
>>> W dniu 22.07.2022 o 18:48, 周琰杰 (Zhou Yanjie) pisze:
>>>> 1.Use the spi-mem poll status APIs in SPI-NOR to reduce CPU load.
>>>> 2.Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC from Ingenic.
>>>> 
>>>> Liu Jinghui and Aidan MacDonald provided a lot of assistance during the development of this driver.
>>>> 
>>>> 周琰杰 (Zhou Yanjie) (3):
>>>>  mtd: spi-nor: Use the spi-mem poll status APIs.
>>>>  dt-bindings: SPI: Add Ingenic SFC bindings.
>>>>  SPI: Ingenic: Add SFC support for Ingenic SoCs.
>>>> 
>>>> .../devicetree/bindings/spi/ingenic,sfc.yaml       |  64 ++
>>>> drivers/mtd/spi-nor/core.c                         |  42 +-
>>>> drivers/spi/Kconfig                                |   9 +
>>>> drivers/spi/Makefile                               |   1 +
>>>> drivers/spi/spi-ingenic-sfc.c                      | 662 +++++++++++++++++++++
>>>> 5 files changed, 768 insertions(+), 10 deletions(-)
>>>> create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>> create mode 100755 drivers/spi/spi-ingenic-sfc.c
>>>> 
>>> Even tough it's still early in revision process, I'll add my
>>> Tested-by: Tomasz Maciej Nowak <tmn505@gmail.com>
>>> 
>>> The test was performed with Damai DM6291A SoC which is a Ingenic X1000 IP
>>> but with 256 MiB RAM. No bugs yet observed on my side.
>> 
>> 
>> Thanks for you test!
>> 
>> 
>>> 
>> 
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Hi Krzysztof, Mike & Mark,

On 2022/7/24 上午4:05, Krzysztof Kozlowski wrote:
> On 23/07/2022 20:47, Mike Yang wrote:
>> On 7/24/22 01:43, Krzysztof Kozlowski wrote:
>>> On 23/07/2022 18:50, Zhou Yanjie wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On 2022/7/23 上午1:46, Krzysztof Kozlowski wrote:
>>>>> On 22/07/2022 18:48, 周琰杰 (Zhou Yanjie) wrote:
>>>>>> Add the SFC bindings for the X1000 SoC, the X1600 SoC, the X1830 SoC,
>>>>>> and the X2000 SoC from Ingenic.
>>>>>>
>>>>>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>>>>>> ---
>>>>>>    .../devicetree/bindings/spi/ingenic,sfc.yaml       | 64 ++++++++++++++++++++++
>>>>>>    1 file changed, 64 insertions(+)
>>>>>>    create mode 100644 Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>>>> new file mode 100644
>>>>>> index 00000000..b7c4cf4
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/spi/ingenic,sfc.yaml
>>>>>> @@ -0,0 +1,64 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/spi/ingenic,sfc.yaml#
>>>>> File name should be rather based on first compatible, so
>>>>> ingenic,x1000-sfc.yaml
>>>>
>>>> No offense, does it really need to be named that way?
>>>> I can't seem to find documentation with instructions on this :(
>>>>
>>>> The use of "ingenic,sfc.yaml" indicates that this is the documentation
>>>> for the SFC module for all Ingenic SoCs, without misleading people into
>>>> thinking it's only for a specific model of SoC. And there seem to be many
>>>> other yaml documents that use similar names (eg. fsl,spi-fsl-qspi.yaml,
>>>> spi-rockchip.yaml, spi-nxp-fspi.yaml, ingenic,spi.yaml, spi-sifive.yaml,
>>>> omap-spi.yaml), maybe these yaml files that are not named with first
>>>> compatible are also for the same consideration. :)
>>> We have many bad examples, many poor patterns and they are never an
>>> argument to add one more bad pattern.
>> Zhou already mentioned he was unable find the naming guidelines of these .yaml files.
>>
>> Apparently you think it's unacceptable for new contributors of a certain subsystem to use existing code as examples, and/or they're responsible for figuring out what's a good example and what's a bad one in the existing codebase.
> It's everywhere in the kernel, what can I say? If you copy existing
> code, you might copy poor code...
>
>>> It might never grow to new devices (because they might be different), so
>>> that is not really an argument.
>> It is an argument. A very valid one.
>>
>> "they *might* be different". You may want to get your hands on real hardware and try another word. Or at least read the datasheets instead of believing your imagination.
>>
>> I would enjoy duplicating the st,stm32-spi.yaml into st,stm32{f,h}{0..7}-spi.yaml if I'm bored at a Sunday afternoon.
>>
>>> All bindings are to follow this rule, so I don't understand why you
>>> think it is an exception for you?
>> Zhou didn't ask you to make an exception. They have a valid point and they're asking why.
> Hm, everyone has the same valid point and such recommendation is to
> everyone, although it is nothing serious.
>
>> You may want to avoid further incidents of this kind by stop being bossy and actually writing a guideline of naming these .yaml files and publish it somewhere online.
> I did not see any incident here... Process of review includes comments
> and there is nothing bad happening when you receive a comment. No
> incident...


I have no intention of provoking arguments, I just did *grep -rn "first 
compatible"* in the
Documentation folder after you mentioned the naming rules about "first 
compatible" before,
but just found a "first compatible" in "Documentation/fb/sstfb.rst", but 
It has absolutely
nothing to do with file naming. As Mike and Mark said, my question in 
the previous email
was just out of curiosity about where to look for a detailed explanation 
of the rule.
Since it would be somewhat strange to have two "Ingenic" yaml files with 
different naming
rules in the SPI subsystem, maybe Rob can guide us here?


Thanks and best regards!


>
> Best regards,
> Krzysztof

Hi Paul,

On 2022/7/24 上午4:24, Paul Cercueil wrote:
> Hi Zhou,
>
> Le dim., juil. 24 2022 at 01:26:56 +0800, Zhou Yanjie 
> <zhouyanjie@wanyeetech.com> a écrit :
>> Hi Paul,
>>
>> On 2022/7/23 上午4:03, Paul Cercueil wrote:
>>> Hi Zhou,
>>>
>>>
>>> Le sam., juil. 23 2022 at 00:48:30 +0800, 周琰杰 (Zhou Yanjie) 
>>> <zhouyanjie@wanyeetech.com> a écrit :
>>>> Add SFC support for the X1000 SoC, the X1600 SoC, and the X2000 SoC
>>>> from Ingenic.
>>>>
>>>> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>>>> ---
>>>>  drivers/spi/Kconfig           |   9 +
>>>>  drivers/spi/Makefile          |   1 +
>>>>  drivers/spi/spi-ingenic-sfc.c | 662 
>>>> ++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 672 insertions(+)
>>>>  create mode 100644 drivers/spi/spi-ingenic-sfc.c
>>>>
>>>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>>>> index 3b1044e..1077bd3 100644
>>>> --- a/drivers/spi/Kconfig
>>>> +++ b/drivers/spi/Kconfig
>>>> @@ -437,6 +437,15 @@ config SPI_INGENIC
>>>>        To compile this driver as a module, choose M here: the module
>>>>        will be called spi-ingenic.
>>>>
>>>> +config SPI_INGENIC_SFC
>>>> +    tristate "Ingenic SoCs SPI Flash Controller"
>>>> +    depends on MACH_INGENIC || COMPILE_TEST
>>>> +    help
>>>> +      This enables support for the Ingenic SoCs SPI flash controller.
>>>> +
>>>> +      To compile this driver as a module, choose M here: the module
>>>> +      will be called ingenic-sfc.
>>>> +
>>>>  config SPI_INTEL
>>>>      tristate
>>>>
>>>> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
>>>> index 0f44eb6..f3e42c0 100644
>>>> --- a/drivers/spi/Makefile
>>>> +++ b/drivers/spi/Makefile
>>>> @@ -62,6 +62,7 @@ obj-$(CONFIG_SPI_HISI_SFC_V3XX)        += 
>>>> spi-hisi-sfc-v3xx.o
>>>>  obj-$(CONFIG_SPI_IMG_SPFI)        += spi-img-spfi.o
>>>>  obj-$(CONFIG_SPI_IMX)            += spi-imx.o
>>>>  obj-$(CONFIG_SPI_INGENIC)        += spi-ingenic.o
>>>> +obj-$(CONFIG_SPI_INGENIC_SFC)    += spi-ingenic-sfc.o
>>>>  obj-$(CONFIG_SPI_INTEL)            += spi-intel.o
>>>>  obj-$(CONFIG_SPI_INTEL_PCI)        += spi-intel-pci.o
>>>>  obj-$(CONFIG_SPI_INTEL_PLATFORM)    += spi-intel-platform.o
>>>> diff --git a/drivers/spi/spi-ingenic-sfc.c 
>>>> b/drivers/spi/spi-ingenic-sfc.c
>>>> new file mode 100644
>>>> index 00000000..a565546
>>>> --- /dev/null
>>>> +++ b/drivers/spi/spi-ingenic-sfc.c
>>>> @@ -0,0 +1,662 @@
>>>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>
>>> Dual-license driver? That's not what MODULE_LICENSE() says.
>>>
>>
>> My fault, forgot to modify MODULE_LICENSE(), will fix it in the next 
>> version.
>>
>>
>>>> +/*
>>>> + * Ingenic SoCs SPI Flash Controller Driver
>>>> + * Copyright (c) 2022 周琰杰 (Zhou Yanjie) 
>>>> <zhouyanjie@wanyeetech.com>
>>>> + */
>>>> +
>>>> +#include <linux/bitfield.h>
>>>> +#include <linux/bitops.h>
>>>> +#include <linux/clk.h>
>>>> +#include <linux/completion.h>
>>>> +#include <linux/dma-mapping.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/iopoll.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mtd/mtd.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/spi/spi.h>
>>>> +#include <linux/spi/spi-mem.h>
>>>> +
>>>> +/* SFC register offsets */
>>>> +#define SFC_REG_GLB                        0x0000
>>>> +#define SFC_REG_DEV_CONF                0x0004
>>>> +#define SFC_REG_DEV_STA_EXP                0x0008
>>>> +#define SFC_REG_DEV_STA_RT                0x000c
>>>> +#define SFC_REG_DEV_STA_MSK                0x0010
>>>> +#define SFC_REG_TRAN_CONF(n)            (0x0014 + n * 4)
>>>> +#define SFC_REG_TRAN_CFG0(n)            (0x0014 + n * 4)
>>>
>>> You should protect the macro parameter. If you do for instance 
>>> SFC_REG_TRAN_CONF(x + 1) it would resolve to (0x0014 + x + 1 * 4) 
>>> which is not what you'd want.
>>>
>>
>> Sure, will fix it.
>>
>>
>>> Also - looks like SFC_REG_TRAN_CONF() and SFC_REG_TRAN_CFG0() are 
>>> the same thing, that's on purpose?
>>>
>>>> +#define SFC_REG_TRAN_LEN 0x002c
>>>> +#define SFC_REG_DEV_ADDR(n)                (0x0030 + n * 4)
>>>> +#define SFC_REG_DEV_ADDR_PLUS(n)        (0x0048 + n * 4)
>>>> +#define SFC_REG_MEM_ADDR                0x0060
>>>> +#define SFC_REG_TRIG                    0x0064
>>>> +#define SFC_REG_SR                        0x0068
>>>> +#define SFC_REG_SCR                        0x006c
>>>> +#define SFC_REG_INTC                    0x0070
>>>> +#define SFC_REG_FSM                        0x0074
>>>> +#define SFC_REG_CGE                        0x0078
>>>> +#define SFC_REG_TRAN_CFG1(n)            (0x009c + n * 4)
>>>> +#define SFC_REG_DR                        0x1000
>>>> +
>>>> +/* bits within the GLB register */
>>>> +#define GLB_TRAN_DIR_MASK                GENMASK(13, 13)
>>>> +#define GLB_TRAN_DIR_WRITE                0x1
>>>> +#define GLB_TRAN_DIR_READ                0x0
>>>
>>> When it's a single bit - just use BIT().
>>
>>
>> Sure.
>>
>>
>>>
>>>> +#define GLB_THRESHOLD_MASK GENMASK(12, 7)
>>>> +#define GLB_OP_MODE_MASK                GENMASK(6, 6)
>>>
>>> Same here, and I see it a few times below as well.
>>>
>>
>> Sure.
>>
>>
>>>> +#define GLB_OP_MODE_DMA                    0x1
>>>> +#define GLB_OP_MODE_SLAVE                0x0
>>>> +#define GLB_PHASE_NUM_MASK                GENMASK(5, 3)
>>>> +#define GLB_WP_EN                        BIT(2)
>>>> +#define GLB_BURST_MD_MASK                GENMASK(1, 0)
>>>> +#define GLB_BURST_MD_INCR32                0x3
>>>> +#define GLB_BURST_MD_INCR16                0x2
>>>> +#define GLB_BURST_MD_INCR8                0x1
>>>> +#define GLB_BURST_MD_INCR4                0x0
>>>> +
>>>> +/* bits within the DEV_CONF register */
>>>> +#define DEV_CONF_SMP_DELAY_MASK            GENMASK(20, 16)
>>>> +#define DEV_CONF_SMP_DELAY_180DEG        0x4
>>>> +#define DEV_CONF_SMP_DELAY_HALF_CYCLE    0x1
>>>> +#define DEV_CONF_CMD_TYPE_MASK            GENMASK(15, 15)
>>>> +#define DEV_CONF_CMD_TYPE_16BIT            0x1
>>>> +#define DEV_CONF_CMD_TYPE_8BIT            0x0
>>>> +#define DEV_CONF_STA_TYPE_MASK            GENMASK(14, 13)
>>>> +#define DEV_CONF_THOLD_MASK                GENMASK(12, 11)
>>>> +#define DEV_CONF_TSETUP_MASK            GENMASK(10, 9)
>>>> +#define DEV_CONF_TSH_MASK                GENMASK(8, 5)
>>>> +#define DEV_CONF_CPHA                    BIT(4)
>>>> +#define DEV_CONF_CPOL                    BIT(3)
>>>> +#define DEV_CONF_CE_DL                    BIT(2)
>>>> +#define DEV_CONF_HOLD_DL                BIT(1)
>>>> +#define DEV_CONF_WP_DL                    BIT(0)
>>>> +
>>>> +/* bits within the TRAN_CONF(n) register */
>>>> +#define TRAN_CONF_TRAN_MODE_MASK        GENMASK(31, 29)
>>>> +#define TRAN_CONF_ADDR_WIDTH_MASK        GENMASK(28, 26)
>>>> +#define TRAN_CONF_POLL_EN                BIT(25)
>>>> +#define TRAN_CONF_CMD_EN                BIT(24)
>>>> +#define TRAN_CONF_PHASE_FORMAT_MASK        GENMASK(23, 23)
>>>> +#define TRAN_CONF_DMY_BITS_MASK            GENMASK(22, 17)
>>>> +#define TRAN_CONF_DATA_EN                BIT(16)
>>>> +#define TRAN_CONF_CMD_MASK                GENMASK(15, 0)
>>>> +
>>>> +/* bits within the TRIG register */
>>>> +#define TRIG_FLUSH                        BIT(2)
>>>> +#define TRIG_STOP                        BIT(1)
>>>> +#define TRIG_START                        BIT(0)
>>>> +
>>>> +/* bits within the SR register */
>>>> +#define SR_FIFO_NUM_MASK                GENMASK(22, 16)
>>>> +#define SR_END                            BIT(4)
>>>> +#define SR_TRAN_REQ                        BIT(3)
>>>> +#define SR_RECE_REQ                        BIT(2)
>>>> +#define SR_OVER                            BIT(1)
>>>> +#define SR_UNDER                        BIT(0)
>>>> +
>>>> +/* bits within the SCR register */
>>>> +#define SCR_CLR_END                        BIT(4)
>>>> +#define SCR_CLR_TREQ                    BIT(3)
>>>> +#define SCR_CLR_RREQ                    BIT(2)
>>>> +#define SCR_CLR_OVER                    BIT(1)
>>>> +#define SCR_CLR_UNDER                    BIT(0)
>>>> +
>>>> +/* bits within the INTC register */
>>>> +#define INTC_MASK_END                    BIT(4)
>>>> +#define INTC_MASK_TREQ                    BIT(3)
>>>> +#define INTC_MASK_RREQ                    BIT(2)
>>>> +#define INTC_MASK_OVER                    BIT(1)
>>>> +#define INTC_MASK_UNDER                    BIT(0)
>>>> +
>>>> +/* bits within the TRAN_CFG1(n) register */
>>>> +#define TRAN_CFG1_TRAN_MODE_MASK        GENMASK(7, 4)
>>>> +
>>>> +#define TRAN_MODE_STANDARD                0
>>>> +#define TRAN_MODE_DUAL_DATA                1
>>>> +#define TRAN_MODE_DUAL_IO                2
>>>> +#define TRAN_MODE_DUAL_FULL                3
>>>> +#define TRAN_MODE_QUAD_DATA                5
>>>> +#define TRAN_MODE_QUAD_IO                6
>>>> +#define TRAN_MODE_QUAD_FULL                7
>>>> +#define TRAN_MODE_OCTAL_DATA            9
>>>> +#define TRAN_MODE_OCTAL_IO                10
>>>> +#define TRAN_MODE_OCTAL_FULL            11
>>>> +
>>>> +#define INGENIC_SFC_FIFO_SIZE            (64 * 4)
>>>> +
>>>> +#define INGENIC_SFC_TRANSFER_TIMEOUT    1000
>>>
>>> Maybe add the unit name in the macro as well - 
>>> INGENIC_SFC_TRANSFER_TIMEOUT_MS.
>>
>>
>> Sure.
>>
>>
>>>
>>>> +
>>>> +enum ingenic_sfc_version {
>>>> +    ID_X1000,
>>>> +    ID_X1600,
>>>> +    ID_X2000,
>>>> +};
>>>> +
>>>> +struct ingenic_soc_info {
>>>> +    enum ingenic_sfc_version version;
>>>> +
>>>> +    unsigned int max_bus_width;
>>>> +
>>>> +    const u32 tran_mode_mask;
>>>> +};
>>>> +
>>>> +struct ingenic_sfc {
>>>> +    const struct ingenic_soc_info *soc_info;
>>>> +
>>>> +    void __iomem *base;
>>>> +    struct device *dev;
>>>> +    struct clk *clk;
>>>> +    int irq;
>>>> +
>>>> +    struct completion completion;
>>>> +};
>>>> +
>>>> +static irqreturn_t ingenic_sfc_irq_handler(int irq, void *data)
>>>> +{
>>>> +    struct ingenic_sfc *sfc = data;
>>>> +
>>>> +    writel(0x1f, sfc->base + SFC_REG_INTC);
>>>> +
>>>> +    complete(&sfc->completion);
>>>> +
>>>> +    return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int ingenic_sfc_adjust_op_size(struct spi_mem *mem, struct 
>>>> spi_mem_op *op)
>>>> +{
>>>> +    uintptr_t addr = (uintptr_t)op->data.buf.in;
>>>> +
>>>> +    if (op->data.nbytes > INGENIC_SFC_FIFO_SIZE && 
>>>> !IS_ALIGNED(addr, 4))
>>>> +        op->data.nbytes = INGENIC_SFC_FIFO_SIZE;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static bool ingenic_sfc_supports_op(struct spi_mem *mem, const 
>>>> struct spi_mem_op *op)
>>>> +{
>>>> +    struct spi_device *spi = mem->spi;
>>>> +    struct ingenic_sfc *sfc = 
>>>> spi_controller_get_devdata(spi->master);
>>>> +    uintptr_t addr = (uintptr_t)op->data.buf.in;
>>>> +
>>>> +    /* The controller only supports Standard SPI mode, Duall mode, 
>>>> Quad mode and Octal mode. */
>>>
>>> Dual*
>>
>>
>> Oops, will fix it.
>>
>>
>>>
>>>> +    if (op->cmd.buswidth > sfc->soc_info->max_bus_width ||
>>>> +        op->addr.buswidth > sfc->soc_info->max_bus_width ||
>>>> +        op->dummy.buswidth > sfc->soc_info->max_bus_width ||
>>>> +        op->data.buswidth > sfc->soc_info->max_bus_width)
>>>> +        return false;
>>>> +
>>>> +    /* Max 32 dummy clock cycles supported */
>>>> +    if (op->dummy.nbytes && op->dummy.nbytes * 8 / 
>>>> op->dummy.buswidth > 32)
>>>> +        return false;
>>>> +
>>>> +    /* Max rx data length, check controller limits and alignment */
>>>> +    if (op->data.dir == SPI_MEM_DATA_IN &&
>>>> +        op->data.nbytes > INGENIC_SFC_FIFO_SIZE && 
>>>> !IS_ALIGNED(addr, 4))
>>>
>>> This does the same check than in ingenic_sfc_adjust_op_size(), maybe 
>>> move it to a new inline function?
>>>
>>
>> Sure.
>>
>>
>>>> +        return false;
>>>> +
>>>> +    /* Max 6 bytes address width supported */
>>>> +    if (op->addr.nbytes > 6)
>>>> +        return false;
>>>> +
>>>> +    return spi_mem_default_supports_op(mem, op);
>>>> +}
>>>> +
>>>> +static void ingenic_sfc_set_transfer_mode(struct ingenic_sfc *sfc, 
>>>> const struct spi_mem_op *op)
>>>> +{
>>>> +    int val;
>>>> +
>>>> +    val = readl(sfc->base + (sfc->soc_info->version >= ID_X1600 ?
>>>> +            SFC_REG_TRAN_CFG1(0) : SFC_REG_TRAN_CONF(0)));
>>>
>>> As Krzysztof said - ugh.
>>>
>>
>> Will fix it.
>>
>>
>>> Also, instead of having a "version" enum in your soc_info, why not 
>>> just have a "reg_conf" field that gives you directly the right 
>>> register?
>>>
>>
>> We still need a version to distinguish the SFC before X1600 SoC and
>> the SFC after X1600 SoC, because in addition to the difference in the
>> cfg register, another difference is that the SFC before X1600 SoC does
>> not support address unaligned RX DMA transfer, while SFC in X1600 and
>> later SoCs can support this feature.
>
> Then add a .dma_supports_unaligned_address field.


Sure, I will try.


>
>>
>>
>>>
>>>> +    val &= ~sfc->soc_info->tran_mode_mask;
>>>> +    if (op->cmd.buswidth == 8)
>>>> +        val |= (TRAN_MODE_OCTAL_FULL << 
>>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>>> +                sfc->soc_info->tran_mode_mask;
>>>
>>> Looks like you're really trying to reinvent the wheel.
>>>
>>> val |= FIELD_PREP(sfc->soc_info->tran_mode_mask, TRAN_MODE_OCTAL_FULL);
>>>
>>> using <linux/bitfield.h>.
>>>
>>> Also, just define a 'mode' variable and set it in your if/else 
>>> blocks, that would look much better. Then you can set val |= 
>>> FIELD_PREP(..., mode) at the end.
>>
>>
>> Sure, will change this in the next version.
>>
>>
>>>
>>>> +    else if (op->cmd.buswidth == 4)
>>>> +        val |= (TRAN_MODE_QUAD_FULL << 
>>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>>> +                sfc->soc_info->tran_mode_mask;
>>>> +    else if (op->cmd.buswidth == 2)
>>>> +        val |= (TRAN_MODE_DUAL_FULL << 
>>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>>> +                sfc->soc_info->tran_mode_mask;
>>>> +    else if (op->addr.buswidth == 8)
>>>> +        val |= (TRAN_MODE_OCTAL_IO << 
>>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>>> +                sfc->soc_info->tran_mode_mask;
>>>> +    else if (op->addr.buswidth == 4)
>>>> +        val |= (TRAN_MODE_QUAD_IO << 
>>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>>> +                sfc->soc_info->tran_mode_mask;
>>>> +    else if (op->addr.buswidth == 2)
>>>> +        val |= (TRAN_MODE_DUAL_IO << 
>>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>>> +                sfc->soc_info->tran_mode_mask;
>>>> +    else if (op->data.buswidth == 8)
>>>> +        val |= (TRAN_MODE_OCTAL_DATA << 
>>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>>> +                sfc->soc_info->tran_mode_mask;
>>>> +    else if (op->data.buswidth == 4)
>>>> +        val |= (TRAN_MODE_QUAD_DATA << 
>>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>>> +                sfc->soc_info->tran_mode_mask;
>>>> +    else if (op->data.buswidth == 2)
>>>> +        val |= (TRAN_MODE_DUAL_DATA << 
>>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>>> +                sfc->soc_info->tran_mode_mask;
>>>> +    else
>>>> +        val |= (TRAN_MODE_STANDARD << 
>>>> (ffs(sfc->soc_info->tran_mode_mask) - 1)) &
>>>> +                sfc->soc_info->tran_mode_mask;
>>>> +    writel(val, sfc->base + (sfc->soc_info->version >= ID_X1600 ?
>>>> +            SFC_REG_TRAN_CFG1(0) : SFC_REG_TRAN_CONF(0)));
>>>> +}
>>>> +
>>>> +/*
>>>> + * We only need PIO mode to handle the SPI_MEM_NO_DATA transfers,
>>>> + * and the unaligned accesses in SPI_MEM_DATA_IN transfers.
>>>> + */
>>>> +static void ingenic_sfc_read_rxfifo(struct ingenic_sfc *sfc, u8 
>>>> *to, unsigned int len)
>>>> +{
>>>> +    void __iomem *from;
>>>> +
>>>> +    from = sfc->base + SFC_REG_DR;
>>>> +
>>>> +    for (; len >= 4; len -= 4, to += 4) {
>>>> +        u32 val = __raw_readl(from);
>>>> +        memcpy(to, &val, 4);
>>>
>>> No need to use memcpy for 4 bytes. You can do: put_unaligned(val, 
>>> (u32 *)to);
>>>
>>
>> Sure.
>>
>>
>>>> +    }
>>>> +
>>>> +    if (len) {
>>>> +        u32 val = __raw_readl(from);
>>>> +        memcpy(to, &val, len);
>>>
>>> Hmm, I'm not sure that is endian-safe. I would prefer if you copied 
>>> byte by byte.
>>>
>>
>> Sure.
>>
>>
>>>> +    }
>>>> +}
>>>> +
>>>> +static int ingenic_sfc_exec_op_pio(struct ingenic_sfc *sfc, const 
>>>> struct spi_mem_op *op)
>>>> +{
>>>> +    int ret, val;
>>>> +
>>>> +    val = readl(sfc->base + SFC_REG_GLB);
>>>> +    u32p_replace_bits(&val, GLB_TRAN_DIR_READ, GLB_TRAN_DIR_MASK);
>>>> +    u32p_replace_bits(&val, GLB_OP_MODE_SLAVE, GLB_OP_MODE_MASK);
>>>> +    writel(val, sfc->base + SFC_REG_GLB);
>>>
>>> By the way, have you considered using regmap?
>>>
>>> It would give you things like regmap_update_bits() for this kind of 
>>> things, and regmap_field() to handle your conf register being at a 
>>> different address across SoCs.
>>>
>>
>> It seems that the overhead of using regmap will be greater than using 
>> readl and writel,
>> the purpose of using readl and writel is to minimize overhead and 
>> maximize performance. :)
>
> If you really think that this is a valid reason not to use regmap, 
> then prove it with a benchmark, because I'm *very* doubtful about your 
> claim.
>

I have tested your writel/readl version of the spi driver and the 
current regmap
version of the spi driver, and indeed the measured performance 
differences are
perceivable. I will do another test for the sfc driver to determine if 
it's the
same case. By the way, I'm trying to use interrupts in the spi driver 
instead of
using regmap_read_poll_timeout(), which can indeed improve some performance.


Thanks and best regards!


> Cheers,
> -Paul
>
>>>> +
>>>> +    val = TRAN_CONF_CMD_EN | op->cmd.opcode;
>>>> +
>>>> +    if (op->addr.nbytes > 0) {
>>>> +        val |= FIELD_PREP(TRAN_CONF_ADDR_WIDTH_MASK, 
>>>> op->addr.nbytes);
>>>> +
>>>> +        writel(op->addr.val & 0xffffffff, sfc->base + 
>>>> SFC_REG_DEV_ADDR(0));
>>>> +        writel(op->addr.val >> 32, sfc->base + 
>>>> SFC_REG_DEV_ADDR_PLUS(0));
>>>> +    }
>>>> +
>>>> +    if (op->dummy.nbytes > 0)
>>>> +        val |= FIELD_PREP(TRAN_CONF_DMY_BITS_MASK,
>>>> +                op->dummy.nbytes * 8 / op->dummy.buswidth);
>>>> +
>>>> +    if (op->data.nbytes > 0)
>>>> +        val |= TRAN_CONF_DATA_EN;
>>>> +
>>>> +    writel(val, sfc->base + SFC_REG_TRAN_CONF(0));
>>>> +    writel(op->data.nbytes, sfc->base + SFC_REG_TRAN_LEN);
>>>> +
>>>> +    ingenic_sfc_set_transfer_mode(sfc, op);
>>>> +
>>>> +    writel(0x1f, sfc->base + SFC_REG_SCR);
>>>
>>> Random 0x1f value here, maybe use a macro?
>>
>>
>> Sure.
>>
>>
>>>
>>>> +    writel(~(INTC_MASK_END | INTC_MASK_RREQ), sfc->base + 
>>>> SFC_REG_INTC);
>>>> +
>>>> +    writel(0, sfc->base + SFC_REG_MEM_ADDR);
>>>> +
>>>> +    writel(TRIG_FLUSH, sfc->base + SFC_REG_TRIG);
>>>> +    writel(TRIG_START, sfc->base + SFC_REG_TRIG);
>>>> +
>>>> +    ret = wait_for_completion_timeout(&sfc->completion,
>>>> + msecs_to_jiffies(INGENIC_SFC_TRANSFER_TIMEOUT));
>>>> +    if (!ret) {
>>>> +        writel(0x1f, sfc->base + SFC_REG_INTC);
>>>> +        writel(0x1f, sfc->base + SFC_REG_SCR);
>>>> +        dev_err(sfc->dev, "line:%d Timeout for ACK from SFC 
>>>> device\n", __LINE__);
>>>> +        return -ETIMEDOUT;
>>>> +    }
>>>> +
>>>> +    ingenic_sfc_read_rxfifo(sfc, op->data.buf.in, op->data.nbytes);
>>>> +    readl_poll_timeout(sfc->base + SFC_REG_SR, val, val & SR_END, 
>>>> 10, 0);
>>>
>>> Infinite timeout? Is that very wise?
>>
>>
>> Will fix it in the next version.
>>
>>
>>>
>>>> +
>>>> +    writel(INTC_MASK_END | INTC_MASK_RREQ, sfc->base + SFC_REG_SCR);
>>>> +    writel(TRIG_STOP, sfc->base + SFC_REG_TRIG);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int ingenic_sfc_exec_op_dma(struct ingenic_sfc *sfc, const 
>>>> struct spi_mem_op *op)
>>>> +{
>>>> +    dma_addr_t addr;
>>>> +    int ret, val;
>>>> +
>>>> +    val = readl(sfc->base + SFC_REG_GLB);
>>>> +    u32p_replace_bits(&val, op->data.dir == SPI_MEM_DATA_IN ?
>>>> +            GLB_TRAN_DIR_READ : GLB_TRAN_DIR_WRITE, 
>>>> GLB_TRAN_DIR_MASK);
>>>> +    u32p_replace_bits(&val, GLB_OP_MODE_DMA, GLB_OP_MODE_MASK);
>>>> +    writel(val, sfc->base + SFC_REG_GLB);
>>>> +
>>>> +    val = TRAN_CONF_CMD_EN | op->cmd.opcode;
>>>> +
>>>> +    if (op->addr.nbytes > 0) {
>>>> +        val |= FIELD_PREP(TRAN_CONF_ADDR_WIDTH_MASK, 
>>>> op->addr.nbytes);
>>>> +        writel(op->addr.val & 0xffffffff, sfc->base + 
>>>> SFC_REG_DEV_ADDR(0));
>>>> +        writel(op->addr.val >> 32, sfc->base + 
>>>> SFC_REG_DEV_ADDR_PLUS(0));
>>>> +    }
>>>> +
>>>> +    if (op->dummy.nbytes > 0)
>>>> +        val |= FIELD_PREP(TRAN_CONF_DMY_BITS_MASK,
>>>> +                op->dummy.nbytes * 8 / op->dummy.buswidth);
>>>> +
>>>> +    if (op->data.nbytes > 0)
>>>> +        val |= TRAN_CONF_DATA_EN;
>>>
>>> There's a lot of code duplication here with 
>>> ingenic_sfc_exec_op_pio(). A lot can be factorized.
>>
>>
>> Sure, I will try to do it.
>>
>>
>>>
>>>> +
>>>> +    writel(val, sfc->base + SFC_REG_TRAN_CONF(0));
>>>> +    writel(op->data.nbytes, sfc->base + SFC_REG_TRAN_LEN);
>>>> +
>>>> +    ingenic_sfc_set_transfer_mode(sfc, op);
>>>> +
>>>> +    writel(0x1f, sfc->base + SFC_REG_SCR);
>>>> +    writel(~INTC_MASK_END, sfc->base + SFC_REG_INTC);
>>>> +
>>>> +    switch (op->data.dir) {
>>>> +    case SPI_MEM_DATA_IN:
>>>> +        addr = dma_map_single(sfc->dev, op->data.buf.in, 
>>>> op->data.nbytes, DMA_FROM_DEVICE);
>>>> +        if (dma_mapping_error(sfc->dev, addr)) {
>>>> +            dev_err(sfc->dev, "RX DMA　memory not mapped\n");
>>>> +            return -ENOMEM;
>>>> +        }
>>>> +
>>>> +        writel(addr, sfc->base + SFC_REG_MEM_ADDR);
>>>> +        break;
>>>> +
>>>> +    case SPI_MEM_DATA_OUT:
>>>> +        addr = dma_map_single(sfc->dev, (void *)op->data.buf.out,
>>>> +                op->data.nbytes, DMA_TO_DEVICE);
>>>> +        if (dma_mapping_error(sfc->dev, addr)) {
>>>> +            dev_err(sfc->dev, "TX DMA　memory not mapped\n");
>>>> +            return -ENOMEM;
>>>> +        }
>>>> +
>>>> +        writel(addr, sfc->base + SFC_REG_MEM_ADDR);
>>>> +        break;
>>>> +
>>>> +    default:
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    writel(TRIG_START, sfc->base + SFC_REG_TRIG);
>>>> +
>>>> +    ret = wait_for_completion_timeout(&sfc->completion,
>>>> + msecs_to_jiffies(INGENIC_SFC_TRANSFER_TIMEOUT));
>>>> +    if (!ret) {
>>>> +        writel(0x1f, sfc->base + SFC_REG_INTC);
>>>> +        writel(0x1f, sfc->base + SFC_REG_SCR);
>>>> +        dev_err(sfc->dev, "line:%d Timeout for ACK from SFC 
>>>> device\n", __LINE__);
>>>> +        return -ETIMEDOUT;
>>>> +    }
>>>> +
>>>> +    dma_unmap_single(sfc->dev, addr, op->data.nbytes,
>>>> +            op->data.dir == SPI_MEM_DATA_IN ? DMA_FROM_DEVICE : 
>>>> DMA_TO_DEVICE);
>>>
>>> Use a small inline function for that too. My personal rule is that 
>>> ?: is fine if the line fits in 80 characters, but if you have to 
>>> break, then you really need to move it somewhere else.
>>>
>>
>> Sure.
>>
>>
>>>> +
>>>> +    writel(INTC_MASK_END, sfc->base + SFC_REG_SCR);
>>>> +    writel(TRIG_STOP, sfc->base + SFC_REG_TRIG);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int ingenic_sfc_exec_op(struct spi_mem *mem, const struct 
>>>> spi_mem_op *op)
>>>> +{
>>>> +    struct spi_device *spi = mem->spi;
>>>> +    struct ingenic_sfc *sfc = 
>>>> spi_controller_get_devdata(spi->master);
>>>> +    uintptr_t addr = (uintptr_t)op->data.buf.in;
>>>> +
>>>> +    init_completion(&sfc->completion);
>>>> +
>>>> +    switch (op->data.dir) {
>>>> +    case SPI_MEM_DATA_IN:
>>>> +        if (sfc->soc_info->version >= ID_X1600 || IS_ALIGNED(addr, 
>>>> 4))
>>>> +            break;
>>>> +
>>>> +        fallthrough;
>>>> +
>>>> +    case SPI_MEM_NO_DATA:
>>>> +        return ingenic_sfc_exec_op_pio(sfc, op);
>>>> +
>>>> +    default:
>>>> +        break;
>>>> +    }
>>>> +
>>>> +    return ingenic_sfc_exec_op_dma(sfc, op);
>>>> +}
>>>> +
>>>> +static int ingenic_sfc_poll_status(struct spi_mem *mem, const 
>>>> struct spi_mem_op *op,
>>>> +            u16 mask, u16 match, unsigned long initial_delay_us,
>>>> +            unsigned long polling_delay_us, unsigned long timeout_ms)
>>>> +{
>>>> +    struct spi_device *spi = mem->spi;
>>>> +    struct ingenic_sfc *sfc = 
>>>> spi_controller_get_devdata(spi->master);
>>>> +    int ret, val;
>>>> +
>>>> +    init_completion(&sfc->completion);
>>>> +
>>>> +    val = readl(sfc->base + SFC_REG_GLB);
>>>> +    u32p_replace_bits(&val, GLB_TRAN_DIR_READ, GLB_TRAN_DIR_MASK);
>>>> +    u32p_replace_bits(&val, GLB_OP_MODE_SLAVE, GLB_OP_MODE_MASK);
>>>> +    writel(val, sfc->base + SFC_REG_GLB);
>>>> +
>>>> +    writel(match, sfc->base + SFC_REG_DEV_STA_EXP);
>>>> +    writel(mask, sfc->base + SFC_REG_DEV_STA_MSK);
>>>> +
>>>> +    val = TRAN_CONF_POLL_EN | TRAN_CONF_CMD_EN | op->cmd.opcode;
>>>> +
>>>> +    if (op->addr.nbytes > 0) {
>>>> +        val |= FIELD_PREP(TRAN_CONF_ADDR_WIDTH_MASK, 
>>>> op->addr.nbytes);
>>>> +
>>>> +        writel(op->addr.val & 0xffffffff, sfc->base + 
>>>> SFC_REG_DEV_ADDR(0));
>>>> +        writel(op->addr.val >> 32, sfc->base + 
>>>> SFC_REG_DEV_ADDR_PLUS(0));
>>>> +    }
>>>> +
>>>> +    if (op->dummy.nbytes > 0)
>>>> +        val |= FIELD_PREP(TRAN_CONF_DMY_BITS_MASK,
>>>> +                op->dummy.nbytes * 8 / op->dummy.buswidth);
>>>> +
>>>> +    if (op->data.nbytes > 0)
>>>> +        val |= TRAN_CONF_DATA_EN;
>>>> +
>>>> +    writel(val, sfc->base + SFC_REG_TRAN_CONF(0));
>>>> +    writel(op->data.nbytes, sfc->base + SFC_REG_TRAN_LEN);
>>>> +
>>>> +    ingenic_sfc_set_transfer_mode(sfc, op);
>>>> +
>>>> +    writel(0x1f, sfc->base + SFC_REG_SCR);
>>>> +    writel(~INTC_MASK_END, sfc->base + SFC_REG_INTC);
>>>> +
>>>> +    writel(0, sfc->base + SFC_REG_MEM_ADDR);
>>>> +
>>>> +    writel(TRIG_START, sfc->base + SFC_REG_TRIG);
>>>> +
>>>> +    ret = wait_for_completion_timeout(&sfc->completion,
>>>> + msecs_to_jiffies(INGENIC_SFC_TRANSFER_TIMEOUT));
>>>> +    if (!ret) {
>>>> +        writel(0x1f, sfc->base + SFC_REG_INTC);
>>>> +        writel(0x1f, sfc->base + SFC_REG_SCR);
>>>> +        dev_err(sfc->dev, "line:%d Timeout for ACK from SFC 
>>>> device\n", __LINE__);
>>>> +        return -ETIMEDOUT;
>>>> +    }
>>>> +
>>>> +    writel(SCR_CLR_END, sfc->base + SFC_REG_SCR);
>>>> +    writel(TRIG_STOP, sfc->base + SFC_REG_TRIG);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static const struct spi_controller_mem_ops ingenic_sfc_mem_ops = {
>>>> +    .adjust_op_size = ingenic_sfc_adjust_op_size,
>>>> +    .supports_op = ingenic_sfc_supports_op,
>>>> +    .exec_op = ingenic_sfc_exec_op,
>>>> +    .poll_status = ingenic_sfc_poll_status,
>>>> +};
>>>> +
>>>> +static int ingenic_sfc_setup(struct spi_device *spi)
>>>> +{
>>>> +    struct ingenic_sfc *sfc = 
>>>> spi_controller_get_devdata(spi->master);
>>>> +    unsigned long rate;
>>>> +    int ret, val;
>>>> +
>>>> +    if (!spi->max_speed_hz)
>>>> +        return -EINVAL;
>>>
>>> Maybe set a sane default?
>>
>>
>> Sure.
>>
>>
>>>
>>>> +
>>>> +    ret = clk_set_rate(sfc->clk, spi->max_speed_hz * 2);
>>>> +    if (ret)
>>>> +        return -EINVAL;
>>>> +
>>>> +    writel(TRIG_STOP, sfc->base + SFC_REG_TRIG);
>>>> +    writel(0, sfc->base + SFC_REG_DEV_CONF);
>>>> +    writel(0, sfc->base + SFC_REG_CGE);
>>>> +
>>>> +    val = readl(sfc->base + SFC_REG_GLB);
>>>> +    u32p_replace_bits(&val, 64 - 1, GLB_THRESHOLD_MASK);
>>>> +    writel(val, sfc->base + SFC_REG_GLB);
>>>> +
>>>> +    val = readl(sfc->base + SFC_REG_DEV_CONF);
>>>> +
>>>> +    /* cpha bit:0 , cpol bit:0 */
>>>> +    val &= ~(DEV_CONF_CPHA | DEV_CONF_CPOL);
>>>> +    val |= spi->mode & SPI_CPHA ? DEV_CONF_CPHA : 0;
>>>> +    val |= spi->mode & SPI_CPOL ? DEV_CONF_CPOL : 0;
>>>> +
>>>> +    /* ce_dl bit:1, hold bit:1, wp bit:1 */
>>>> +    val |= (DEV_CONF_CE_DL | DEV_CONF_HOLD_DL | DEV_CONF_WP_DL);
>>>> +
>>>> +    writel(val, sfc->base + SFC_REG_DEV_CONF);
>>>> +
>>>> +    val = readl(sfc->base + SFC_REG_GLB);
>>>> +    u32p_replace_bits(&val, GLB_OP_MODE_SLAVE, GLB_OP_MODE_MASK);
>>>> +    writel(val, sfc->base + SFC_REG_GLB);
>>>> +
>>>> +    rate = clk_get_rate(sfc->clk);
>>>
>>> I'd suggest using clk_round_rate() before clk_set_rate() because 
>>> then you know what frequency it's going to be, and you don't have 
>>> to call clk_get_rate() afterwards.
>>>
>>
>> Sure, will try.
>>
>>
>> Thanks and best regards!
>>
>>
>>> Cheers,
>>> -Paul
>>>
>>>> +    val = readl(sfc->base + SFC_REG_DEV_CONF);
>>>> +    if (sfc->soc_info->version >= ID_X1600 && rate >= 200000000)
>>>> +        u32p_replace_bits(&val, DEV_CONF_SMP_DELAY_180DEG, 
>>>> DEV_CONF_SMP_DELAY_MASK);
>>>> +    else if (sfc->soc_info->version == ID_X1000 && rate >= 100000000)
>>>> +        u32p_replace_bits(&val, DEV_CONF_SMP_DELAY_HALF_CYCLE, 
>>>> DEV_CONF_SMP_DELAY_MASK);
>>>> +    writel(val, sfc->base + SFC_REG_DEV_CONF);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int ingenic_sfc_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct ingenic_sfc *sfc;
>>>> +    struct spi_controller *ctlr;
>>>> +    int ret;
>>>> +
>>>> +    ctlr = spi_alloc_master(&pdev->dev, sizeof(*sfc));
>>>> +    if (!ctlr)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    sfc = spi_controller_get_devdata(ctlr);
>>>> +
>>>> +    sfc->soc_info = of_device_get_match_data(&pdev->dev);
>>>> +    if (!sfc->soc_info) {
>>>> +        dev_err(&pdev->dev, "No of match data provided\n");
>>>> +        ret = -ENODEV;
>>>> +        goto err_put_master;
>>>> +    }
>>>> +
>>>> +    sfc->base = devm_platform_ioremap_resource(pdev, 0);
>>>> +    if (IS_ERR(sfc->base)) {
>>>> +        ret = PTR_ERR(sfc->base);
>>>> +        goto err_put_master;
>>>> +    }
>>>> +
>>>> +    sfc->clk = devm_clk_get(&pdev->dev, "sfc");
>>>> +    if (IS_ERR(sfc->clk)) {
>>>> +        ret = IS_ERR(sfc->clk);
>>>> +        goto err_put_master;
>>>> +    }
>>>> +
>>>> +    ret = clk_prepare_enable(sfc->clk);
>>>> +    if (ret)
>>>> +        goto err_put_master;
>>>> +
>>>> +    sfc->irq = platform_get_irq(pdev, 0);
>>>> +    if (sfc->irq < 0) {
>>>> +        ret = sfc->irq;
>>>> +        goto err_put_master;
>>>> +    }
>>>> +
>>>> +    sfc->dev = &pdev->dev;
>>>> +
>>>> +    platform_set_drvdata(pdev, sfc);
>>>> +
>>>> +    ret = devm_request_irq(&pdev->dev, sfc->irq, 
>>>> ingenic_sfc_irq_handler, 0,
>>>> +            dev_name(&pdev->dev), sfc);
>>>> +    if (ret) {
>>>> +        dev_err(&pdev->dev, "Failed to request irq%d, ret = %d\n", 
>>>> sfc->irq, ret);
>>>> +        goto err_put_master;
>>>> +    }
>>>> +
>>>> +    ctlr->bus_num = -1;
>>>> +    ctlr->num_chipselect = 1;
>>>> +    ctlr->mem_ops = &ingenic_sfc_mem_ops;
>>>> +    ctlr->dev.of_node = pdev->dev.of_node;
>>>> +    ctlr->setup = ingenic_sfc_setup;
>>>> +    ctlr->mode_bits = SPI_CPHA | SPI_CPOL |
>>>> +            SPI_RX_DUAL | SPI_RX_QUAD | SPI_TX_DUAL | SPI_TX_QUAD;
>>>> +    if (sfc->soc_info->version >= ID_X2000)
>>>> +        ctlr->mode_bits |= SPI_RX_OCTAL | SPI_TX_OCTAL;
>>>> +
>>>> +    ret = devm_spi_register_controller(&pdev->dev, ctlr);
>>>> +    if (ret)
>>>> +        goto err_put_master;
>>>> +
>>>> +    return 0;
>>>> +
>>>> +err_put_master:
>>>> +    spi_master_put(ctlr);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static const struct ingenic_soc_info x1000_soc_info = {
>>>> +    .version = ID_X1000,
>>>> +
>>>> +    .max_bus_width = 4,
>>>> +
>>>> +    .tran_mode_mask = TRAN_CONF_TRAN_MODE_MASK,
>>>> +};
>>>> +
>>>> +static const struct ingenic_soc_info x1600_soc_info = {
>>>> +    .version = ID_X1600,
>>>> +
>>>> +    .max_bus_width = 4,
>>>> +
>>>> +    .tran_mode_mask = TRAN_CONF_TRAN_MODE_MASK,
>>>> +};
>>>> +
>>>> +static const struct ingenic_soc_info x2000_soc_info = {
>>>> +    .version = ID_X2000,
>>>> +
>>>> +    .max_bus_width = 8,
>>>> +
>>>> +    .tran_mode_mask = TRAN_CFG1_TRAN_MODE_MASK,
>>>> +};
>>>> +
>>>> +static const struct of_device_id ingenic_sfc_of_matches[] = {
>>>> +    { .compatible = "ingenic,x1000-sfc", .data = &x1000_soc_info },
>>>> +    { .compatible = "ingenic,x1600-sfc", .data = &x1600_soc_info },
>>>> +    { .compatible = "ingenic,x2000-sfc", .data = &x2000_soc_info },
>>>> +    { /* sentinel */ }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, ingenic_sfc_of_matches);
>>>> +
>>>> +static struct platform_driver ingenic_sfc_driver = {
>>>> +    .driver = {
>>>> +        .name = "ingenic-sfc",
>>>> +        .of_match_table = ingenic_sfc_of_matches,
>>>> +    },
>>>> +    .probe = ingenic_sfc_probe,
>>>> +};
>>>> +module_platform_driver(ingenic_sfc_driver);
>>>> +
>>>> +MODULE_LICENSE("GPL");
>>>> +MODULE_AUTHOR("周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>");
>>>> +MODULE_DESCRIPTION("Ingenic SoCs SPI Flash Controller Driver");
>>>> -- 
>>>> 2.7.4
>>>>
>>>
>

Hi Mike,

On 2022/7/24 上午4:49, Mike Yang wrote:
> On 7/24/22 04:07, Krzysztof Kozlowski wrote:
>> On 23/07/2022 21:27, Mark Brown wrote:
>>> On Sun, Jul 24, 2022 at 02:47:14AM +0800, Mike Yang wrote:
>>>> On 7/24/22 01:43, Krzysztof Kozlowski wrote:
>>>>> On 23/07/2022 18:50, Zhou Yanjie wrote:
>>>>>> No offense, does it really need to be named that way?
>>>>>> I can't seem to find documentation with instructions on this :(
>>> ...
>>>
>>>>> All bindings are to follow this rule, so I don't understand why you
>>>>> think it is an exception for you?
>>>> Zhou didn't ask you to make an exception. They have a valid
>>>> point and they're asking why.
>>>> You may want to avoid further incidents of this kind by stop
>>>> being bossy and actually writing a guideline of naming these
>>>> .yaml files and publish it somewhere online.
>>> Yeah, I do have to say that I was also completely unaware that
>>> there was any enforced convention here.
>> Indeed, it's not a enforced pattern. But there are many other
>> insignificant ones which we also tend to forget during review, like
>> using words "Device Tree bindings" in title or using unnecessary quotes
>> around "refs" (also in ID of schema). It's not a big deal, but I ask
>> when I notice it.
> Good. Thanks for paying attention to these details.
>
>
>>> Zhou already mentioned he was unable find the naming guidelines of these .yaml files.
>>>
>>> Apparently you think it's unacceptable for new contributors of a certain subsystem to use existing code as examples, and/or they're responsible for figuring out what's a good example and what's a bad one in the existing codebase.
>> It's everywhere in the kernel, what can I say? If you copy existing
>> code, you might copy poor code...
> Still, it shouldn't be a responsibility of new contributors to determine the quality of an existing piece of code, unless there are clear guidelines (i.e. one should use the new "cs-gpios" attribute in SPI controllers).
>
>>>> It might never grow to new devices (because they might be different), so
>>>> that is not really an argument.
>>> It is an argument. A very valid one.
>>>
>>> "they *might* be different". You may want to get your hands on real hardware and try another word. Or at least read the datasheets instead of believing your imagination.
>>>
>>> I would enjoy duplicating the st,stm32-spi.yaml into st,stm32{f,h}{0..7}-spi.yaml if I'm bored at a Sunday afternoon.
>>>
>>>> All bindings are to follow this rule, so I don't understand why you
>>>> think it is an exception for you?
>>> Zhou didn't ask you to make an exception. They have a valid point and they're asking why.
>> Hm, everyone has the same valid point and such recommendation is to
>> everyone, although it is nothing serious.
>>
>>> You may want to avoid further incidents of this kind by stop being bossy and actually writing a guideline of naming these .yaml files and publish it somewhere online.
>> I did not see any incident here... Process of review includes comments
>> and there is nothing bad happening when you receive a comment. No
>> incident...
>
> Okay. After careful inspection of the Ingenic datasheets, now I have the conclusion: The Ingenic X1000, X1021, X1500, X1501, X1520, X1600, X1800, X1830, X2000, X2100, X2500 have the same SFC controller.


Actually, you are also missing out the X1630 and X1660, and the upcoming 
X2600.


>
> X1600 has a newer version (let's say v2) of the SFC, and X2000-2500 have v3. Others have the original version (let's say v1). Each new version introduced new features such as arbitrary DMA sizes, and the rest features are the same.
>
>
> So IMO the name "ingenic,sfc.yaml" is perfectly logical.
>
>
> Regards,
> Mike Yang

On Sun, Jul 24, 2022 at 04:49:25AM +0800, Mike Yang wrote:
> On 7/24/22 04:07, Krzysztof Kozlowski wrote:
> > On 23/07/2022 21:27, Mark Brown wrote:
> >> On Sun, Jul 24, 2022 at 02:47:14AM +0800, Mike Yang wrote:
> >>> On 7/24/22 01:43, Krzysztof Kozlowski wrote:
> >>>> On 23/07/2022 18:50, Zhou Yanjie wrote:
> >>
> >>>>> No offense, does it really need to be named that way?
> >>>>> I can't seem to find documentation with instructions on this :(
> >>
> >> ...
> >>
> >>>> All bindings are to follow this rule, so I don't understand why you
> >>>> think it is an exception for you?
> >>
> >>> Zhou didn't ask you to make an exception. They have a valid
> >>> point and they're asking why.
> >>
> >>> You may want to avoid further incidents of this kind by stop
> >>> being bossy and actually writing a guideline of naming these
> >>> .yaml files and publish it somewhere online.

I don't like your tone. Patches are welcome to fix deficiencies in 
documentation. Out of the hundreds of bindings a year, I see <5 
documentation patches a year.

The documentation clearly says to run 'make dt_binding_check' and that 
was obviously not followed here. 

> >> Yeah, I do have to say that I was also completely unaware that
> >> there was any enforced convention here.
> > 
> > Indeed, it's not a enforced pattern. But there are many other
> > insignificant ones which we also tend to forget during review, like
> > using words "Device Tree bindings" in title or using unnecessary quotes
> > around "refs" (also in ID of schema). It's not a big deal, but I ask
> > when I notice it.
> 
> Good. Thanks for paying attention to these details.
> 
> 
> >> Zhou already mentioned he was unable find the naming guidelines of these .yaml files.
> >>
> >> Apparently you think it's unacceptable for new contributors of a certain subsystem to use existing code as examples, and/or they're responsible for figuring out what's a good example and what's a bad one in the existing codebase.

Please wrap your lines on replies.


> > 
> > It's everywhere in the kernel, what can I say? If you copy existing
> > code, you might copy poor code...
> 
> Still, it shouldn't be a responsibility of new contributors to 
> determine the quality of an existing piece of code, unless there are 
> clear guidelines (i.e. one should use the new "cs-gpios" attribute in SPI controllers).

Generally the guidance is to look at newer drivers for current best 
practices.


> >>> It might never grow to new devices (because they might be different), so
> >>> that is not really an argument.
> >>
> >> It is an argument. A very valid one.
> >>
> >> "they *might* be different". You may want to get your hands on real hardware and try another word. Or at least read the datasheets instead of believing your imagination.
> >>
> >> I would enjoy duplicating the st,stm32-spi.yaml into st,stm32{f,h}{0..7}-spi.yaml if I'm bored at a Sunday afternoon.
> >>
> >>>
> >>> All bindings are to follow this rule, so I don't understand why you
> >>> think it is an exception for you?
> >>
> >> Zhou didn't ask you to make an exception. They have a valid point and they're asking why.
> > 
> > Hm, everyone has the same valid point and such recommendation is to
> > everyone, although it is nothing serious.
> > 
> >> You may want to avoid further incidents of this kind by stop being bossy and actually writing a guideline of naming these .yaml files and publish it somewhere online.
> > 
> > I did not see any incident here... Process of review includes comments
> > and there is nothing bad happening when you receive a comment. No
> > incident...
> 
> 
> Okay. After careful inspection of the Ingenic datasheets, now I have 
> the conclusion: The Ingenic X1000, X1021, X1500, X1501, X1520, X1600, 
> X1800, X1830, X2000, X2100, X2500 have the same SFC controller.

So if they are all 'the same', then I expect they all have a fallback 
compatible with x1000 and using that for the filename makes sense.


> X1600 has a newer version (let's say v2) of the SFC, and X2000-2500 
> have v3. Others have the original version (let's say v1). Each new 
> version introduced new features such as arbitrary DMA sizes, and the 
> rest features are the same.

So backwards compatible? If so, then they should have x1000 for 
fallback.

> 
> So IMO the name "ingenic,sfc.yaml" is perfectly logical.

Covering all 3 versions? If so and not backwards compatible, then I 
would agree.

Rob