* [PATCH v4 0/3] support for amlogic the new SPI IP
@ 2025-07-04 2:59 Xianwei Zhao via B4 Relay
2025-07-04 2:59 ` [PATCH v4 1/3] spi: dt-bindings: Add binding document of Amlogic SPISG controller Xianwei Zhao via B4 Relay
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Xianwei Zhao via B4 Relay @ 2025-07-04 2:59 UTC (permalink / raw)
To: Sunny Luo, Mark Brown, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: linux-amlogic, linux-spi, devicetree, linux-kernel, Xianwei Zhao,
Conor Dooley
Introduced support for the new SPI IP (SPISG). The SPISG is
a communication-oriented SPI controller from Amlogic,supporting
three operation modes: PIO, block DMA, and scatter-gather DMA.
Add the drivers and device tree bindings corresponding to the SPISG.
Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
Changes in v4:
- Add resets prop and modify some formats for bindings.
- Remove irrelevant headers files and fix some issues.
- Link to v3: https://lore.kernel.org/r/20250623-spisg-v3-0-c731f57e289c@amlogic.com
Changes in v3:
- Rename of bit definition and fix some issues.
- Enable runtime_suspend function.
- Link to v2: https://lore.kernel.org/r/20250617-spisg-v2-0-51a605a84bd5@amlogic.com
Changes in v2:
- Use regmap to operation register and drop bitfied define.
- Use "SPISG" prefix intead of "SPICC", and declare clock div table in the spisg_device.
- Delete other power operation functions except for runtime_supspend and runtime_resume.
- Fix some format corrections.
- Link to v1: https://lore.kernel.org/r/20250604-spisg-v1-0-5893dbe9d953@amlogic.com
---
Sunny Luo (2):
spi: dt-bindings: Add binding document of Amlogic SPISG controller
spi: Add Amlogic SPISG driver
Xianwei Zhao (1):
MAINTAINERS: Add an entry for Amlogic spi driver
.../devicetree/bindings/spi/amlogic,a4-spisg.yaml | 59 ++
MAINTAINERS | 9 +
drivers/spi/Kconfig | 9 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-amlogic-spisg.c | 871 +++++++++++++++++++++
5 files changed, 949 insertions(+)
---
base-commit: bd30b995df8fd053e13d10f78dbc7b2fa5ed1aae
change-id: 20250603-spisg-78f21682ebac
Best regards,
--
Xianwei Zhao <xianwei.zhao@amlogic.com>
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v4 1/3] spi: dt-bindings: Add binding document of Amlogic SPISG controller 2025-07-04 2:59 [PATCH v4 0/3] support for amlogic the new SPI IP Xianwei Zhao via B4 Relay @ 2025-07-04 2:59 ` Xianwei Zhao via B4 Relay 2025-07-04 2:59 ` [PATCH v4 2/3] spi: Add Amlogic SPISG driver Xianwei Zhao via B4 Relay 2025-07-04 2:59 ` [PATCH v4 3/3] MAINTAINERS: Add an entry for Amlogic spi driver Xianwei Zhao via B4 Relay 2 siblings, 0 replies; 14+ messages in thread From: Xianwei Zhao via B4 Relay @ 2025-07-04 2:59 UTC (permalink / raw) To: Sunny Luo, Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-amlogic, linux-spi, devicetree, linux-kernel, Xianwei Zhao, Conor Dooley From: Sunny Luo <sunny.luo@amlogic.com> The SPISG is a new communication oriented SPI controller of Amlogic, which supports PIO, block DMA and scatter-gather DMA three operation modes. Signed-off-by: Sunny Luo <sunny.luo@amlogic.com> Acked-by: Conor Dooley <conor.dooley@microchip.com> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> --- .../devicetree/bindings/spi/amlogic,a4-spisg.yaml | 59 ++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml b/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml new file mode 100644 index 000000000000..9bfb8089f7ea --- /dev/null +++ b/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml @@ -0,0 +1,59 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright (C) 2025 Amlogic, Inc. All rights reserved +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/spi/amlogic,a4-spisg.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Amlogic SPI Scatter-Gather Controller + +maintainers: + - Xianwei Zhao <xianwei.zhao@amlogic.com> + - Sunny Luo <sunny.luo@amlogic.com> + +allOf: + - $ref: spi-controller.yaml# + +properties: + compatible: + const: amlogic,a4-spisg + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + maxItems: 2 + + clock-names: + items: + - const: core + - const: pclk + + resets: + maxItems: 1 + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + spi@50000 { + compatible = "amlogic,a4-spisg"; + reg = <0x50000 0x38>; + interrupts = <GIC_SPI 183 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&clkc 37>, + <&clkc 93>; + clock-names = "core", "pclk"; + #address-cells = <1>; + #size-cells = <0>; + }; -- 2.37.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v4 2/3] spi: Add Amlogic SPISG driver 2025-07-04 2:59 [PATCH v4 0/3] support for amlogic the new SPI IP Xianwei Zhao via B4 Relay 2025-07-04 2:59 ` [PATCH v4 1/3] spi: dt-bindings: Add binding document of Amlogic SPISG controller Xianwei Zhao via B4 Relay @ 2025-07-04 2:59 ` Xianwei Zhao via B4 Relay 2025-07-07 13:05 ` Mark Brown 2025-07-08 16:01 ` Martin Blumenstingl 2025-07-04 2:59 ` [PATCH v4 3/3] MAINTAINERS: Add an entry for Amlogic spi driver Xianwei Zhao via B4 Relay 2 siblings, 2 replies; 14+ messages in thread From: Xianwei Zhao via B4 Relay @ 2025-07-04 2:59 UTC (permalink / raw) To: Sunny Luo, Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-amlogic, linux-spi, devicetree, linux-kernel, Xianwei Zhao From: Sunny Luo <sunny.luo@amlogic.com> Introduced support for the new SPI IP (SPISG) driver. The SPISG is a communication-oriented SPI controller from Amlogic,supporting three operation modes: PIO, block DMA, and scatter-gather DMA. Signed-off-by: Sunny Luo <sunny.luo@amlogic.com> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> --- drivers/spi/Kconfig | 9 + drivers/spi/Makefile | 1 + drivers/spi/spi-amlogic-spisg.c | 871 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 881 insertions(+) diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index c51da3fc3604..e11341df2ecf 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -99,6 +99,15 @@ config SPI_AMLOGIC_SPIFC_A1 This enables master mode support for the SPIFC (SPI flash controller) available in Amlogic A1 (A113L SoC). +config SPI_AMLOGIC_SPISG + tristate "Amlogic SPISG controller" + depends on COMMON_CLK + depends on ARCH_MESON || COMPILE_TEST + help + This enables master mode support for the SPISG (SPI scatter-gather + communication controller), which is available on platforms such as + Amlogic A4 SoCs. + config SPI_APPLE tristate "Apple SoC SPI Controller platform driver" depends on ARCH_APPLE || COMPILE_TEST diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index 4ea89f6fc531..b74e3104d71f 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_SPI_ALTERA) += spi-altera-platform.o obj-$(CONFIG_SPI_ALTERA_CORE) += spi-altera-core.o obj-$(CONFIG_SPI_ALTERA_DFL) += spi-altera-dfl.o obj-$(CONFIG_SPI_AMLOGIC_SPIFC_A1) += spi-amlogic-spifc-a1.o +obj-$(CONFIG_SPI_AMLOGIC_SPISG) += spi-amlogic-spisg.o obj-$(CONFIG_SPI_APPLE) += spi-apple.o obj-$(CONFIG_SPI_AR934X) += spi-ar934x.o obj-$(CONFIG_SPI_ARMADA_3700) += spi-armada-3700.o diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c new file mode 100644 index 000000000000..068fea095039 --- /dev/null +++ b/drivers/spi/spi-amlogic-spisg.c @@ -0,0 +1,871 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Driver for Amlogic SPI communication Scatter-Gather Controller + * + * Copyright (C) 2025 Amlogic, Inc. All rights reserved + * + * Author: Sunny Luo <sunny.luo@amlogic.com> + * Author: Xianwei Zhao <xianwei.zhao@amlogic.com> + */ + +#include <linux/bitfield.h> +#include <linux/device.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/dma-mapping.h> +#include <linux/platform_device.h> +#include <linux/pinctrl/consumer.h> +#include <linux/pm_runtime.h> +#include <linux/spi/spi.h> +#include <linux/types.h> +#include <linux/interrupt.h> +#include <linux/reset.h> +#include <linux/regmap.h> + +/* Register Map */ +#define SPISG_REG_CFG_READY 0x00 + +#define SPISG_REG_CFG_SPI 0x04 +#define CFG_BUS64_EN BIT(0) +#define CFG_SLAVE_EN BIT(1) +#define CFG_SLAVE_SELECT GENMASK(3, 2) +#define CFG_SFLASH_WP BIT(4) +#define CFG_SFLASH_HD BIT(5) +/* start on vsync rising */ +#define CFG_HW_POS BIT(6) +/* start on vsync falling */ +#define CFG_HW_NEG BIT(7) + +#define SPISG_REG_CFG_START 0x08 +#define CFG_BLOCK_NUM GENMASK(19, 0) +#define CFG_BLOCK_SIZE GENMASK(22, 20) +#define CFG_DATA_COMMAND BIT(23) +#define CFG_OP_MODE GENMASK(25, 24) +#define CFG_RXD_MODE GENMASK(27, 26) +#define CFG_TXD_MODE GENMASK(29, 28) +#define CFG_EOC BIT(30) +#define CFG_PEND BIT(31) + +#define SPISG_REG_CFG_BUS 0x0C +#define CFG_CLK_DIV GENMASK(7, 0) +#define CLK_DIV_WIDTH 8 +#define CFG_RX_TUNING GENMASK(11, 8) +#define CFG_TX_TUNING GENMASK(15, 12) +#define CFG_CS_SETUP GENMASK(19, 16) +#define CFG_LANE GENMASK(21, 20) +#define CFG_HALF_DUPLEX BIT(22) +#define CFG_B_L_ENDIAN BIT(23) +#define CFG_DC_MODE BIT(24) +#define CFG_NULL_CTL BIT(25) +#define CFG_DUMMY_CTL BIT(26) +#define CFG_READ_TURN GENMASK(28, 27) +#define CFG_KEEP_SS BIT(29) +#define CFG_CPHA BIT(30) +#define CFG_CPOL BIT(31) + +#define SPISG_REG_PIO_TX_DATA_L 0x10 +#define SPISG_REG_PIO_TX_DATA_H 0x14 +#define SPISG_REG_PIO_RX_DATA_L 0x18 +#define SPISG_REG_PIO_RX_DATA_H 0x1C +#define SPISG_REG_MEM_TX_ADDR_L 0x10 +#define SPISG_REG_MEM_TX_ADDR_H 0x14 +#define SPISG_REG_MEM_RX_ADDR_L 0x18 +#define SPISG_REG_MEM_RX_ADDR_H 0x1C +#define SPISG_REG_DESC_LIST_L 0x20 +#define SPISG_REG_DESC_LIST_H 0x24 +#define LIST_DESC_PENDING BIT(31) +#define SPISG_REG_DESC_CURRENT_L 0x28 +#define SPISG_REG_DESC_CURRENT_H 0x2c +#define SPISG_REG_IRQ_STS 0x30 +#define SPISG_REG_IRQ_ENABLE 0x34 +#define IRQ_RCH_DESC_EOC BIT(0) +#define IRQ_RCH_DESC_INVALID BIT(1) +#define IRQ_RCH_DESC_RESP BIT(2) +#define IRQ_RCH_DATA_RESP BIT(3) +#define IRQ_WCH_DESC_EOC BIT(4) +#define IRQ_WCH_DESC_INVALID BIT(5) +#define IRQ_WCH_DESC_RESP BIT(6) +#define IRQ_WCH_DATA_RESP BIT(7) +#define IRQ_DESC_ERR BIT(8) +#define IRQ_SPI_READY BIT(9) +#define IRQ_DESC_DONE BIT(10) +#define IRQ_DESC_CHAIN_DONE BIT(11) + +#define SPISG_MAX_REG 0x40 + +#define SPISG_BLOCK_MAX 0x100000 + +#define SPISG_OP_MODE_WRITE_CMD 0 +#define SPISG_OP_MODE_READ_STS 1 +#define SPISG_OP_MODE_WRITE 2 +#define SPISG_OP_MODE_READ 3 + +#define SPISG_DATA_MODE_NONE 0 +#define SPISG_DATA_MODE_PIO 1 +#define SPISG_DATA_MODE_MEM 2 +#define SPISG_DATA_MODE_SG 3 + +#define SPISG_CLK_DIV_MAX 256 +/* recommended by specification */ +#define SPISG_CLK_DIV_MIN 4 +#define DIV_NUM (SPISG_CLK_DIV_MAX - SPISG_CLK_DIV_MIN + 1) + +#define SPISG_PCLK_RATE_MIN 24000000 + +#define SPISG_SINGLE_SPI 0 +#define SPISG_DUAL_SPI 1 +#define SPISG_QUAD_SPI 2 + +struct spisg_sg_link { +#define LINK_ADDR_VALID BIT(0) +#define LINK_ADDR_EOC BIT(1) +#define LINK_ADDR_IRQ BIT(2) +#define LINK_ADDR_ACT GENMASK(5, 3) +#define LINK_ADDR_RING BIT(6) +#define LINK_ADDR_LEN GENMASK(31, 8) + u32 addr; + u32 addr1; +}; + +struct spisg_descriptor { + u32 cfg_start; + u32 cfg_bus; + u64 tx_paddr; + u64 rx_paddr; +}; + +struct spisg_descriptor_extra { + struct spisg_sg_link *tx_ccsg; + struct spisg_sg_link *rx_ccsg; + int tx_ccsg_len; + int rx_ccsg_len; +}; + +struct spisg_device { + struct spi_controller *controller; + struct platform_device *pdev; + struct regmap *map; + struct clk *core; + struct clk *pclk; + struct clk *sclk; + struct clk_div_table *tbl; + struct completion completion; + u32 status; + u32 speed_hz; + u32 effective_speed_hz; + u32 bytes_per_word; + u32 cfg_spi; + u32 cfg_start; + u32 cfg_bus; +}; + +static int spi_delay_to_sclk(u32 slck_speed_hz, struct spi_delay *delay) +{ + u32 ns; + + if (!delay) + return 0; + + if (delay->unit == SPI_DELAY_UNIT_SCK) + return delay->value; + + ns = spi_delay_to_ns(delay, NULL); + if (ns < 0) + return 0; + + return DIV_ROUND_UP_ULL(slck_speed_hz * ns, NSEC_PER_SEC); +} + +static inline u32 aml_spisg_sem_down_read(struct spisg_device *spisg) +{ + u32 ret; + + regmap_read(spisg->map, SPISG_REG_CFG_READY, &ret); + if (ret) + regmap_write(spisg->map, SPISG_REG_CFG_READY, 0); + + return ret; +} + +static inline void aml_spisg_sem_up_write(struct spisg_device *spisg) +{ + regmap_write(spisg->map, SPISG_REG_CFG_READY, 1); +} + +static int aml_spisg_set_speed(struct spisg_device *spisg, uint speed_hz) +{ + u32 cfg_bus; + + if (!speed_hz || speed_hz == spisg->speed_hz) + return 0; + + spisg->speed_hz = speed_hz; + clk_set_rate(spisg->sclk, speed_hz); + /* Store the div for the descriptor mode */ + regmap_read(spisg->map, SPISG_REG_CFG_BUS, &cfg_bus); + spisg->cfg_bus &= ~CFG_CLK_DIV; + spisg->cfg_bus |= cfg_bus & CFG_CLK_DIV; + spisg->effective_speed_hz = clk_get_rate(spisg->sclk); + dev_dbg(&spisg->pdev->dev, + "desired speed %dHz, effective speed %dHz\n", + speed_hz, spisg->effective_speed_hz); + + return 0; +} + +static bool aml_spisg_can_dma(struct spi_controller *ctlr, + struct spi_device *spi, + struct spi_transfer *xfer) +{ + return true; +} + +static void aml_spisg_sg_xlate(struct sg_table *sgt, struct spisg_sg_link *ccsg) +{ + struct scatterlist *sg; + int i; + + for_each_sg(sgt->sgl, sg, sgt->nents, i) { + ccsg->addr = FIELD_PREP(LINK_ADDR_VALID, 1) | + FIELD_PREP(LINK_ADDR_RING, 0) | + FIELD_PREP(LINK_ADDR_EOC, sg_is_last(sg)) | + FIELD_PREP(LINK_ADDR_LEN, sg_dma_len(sg)); + ccsg->addr1 = (u32)sg_dma_address(sg); + ccsg++; + } +} + +static int nbits_to_lane[] = { + SPISG_SINGLE_SPI, + SPISG_SINGLE_SPI, + SPISG_DUAL_SPI, + -EINVAL, + SPISG_QUAD_SPI +}; + +static int aml_spisg_setup_transfer(struct spisg_device *spisg, + struct spi_transfer *xfer, + struct spisg_descriptor *desc, + struct spisg_descriptor_extra *exdesc) +{ + int block_size, blocks; + struct device *dev = &spisg->pdev->dev; + struct spisg_sg_link *ccsg; + int ccsg_len; + dma_addr_t paddr; + int ret; + + memset(desc, 0, sizeof(*desc)); + if (exdesc) + memset(exdesc, 0, sizeof(*exdesc)); + aml_spisg_set_speed(spisg, xfer->speed_hz); + xfer->effective_speed_hz = spisg->effective_speed_hz; + + desc->cfg_start = spisg->cfg_start; + desc->cfg_bus = spisg->cfg_bus; + + block_size = xfer->bits_per_word >> 3; + blocks = xfer->len / block_size; + + desc->cfg_start |= FIELD_PREP(CFG_EOC, 0); + desc->cfg_bus |= FIELD_PREP(CFG_KEEP_SS, !xfer->cs_change); + desc->cfg_bus |= FIELD_PREP(CFG_NULL_CTL, 0); + + if (xfer->tx_buf || xfer->tx_dma) { + desc->cfg_bus |= FIELD_PREP(CFG_LANE, nbits_to_lane[xfer->tx_nbits]); + desc->cfg_start |= FIELD_PREP(CFG_OP_MODE, SPISG_OP_MODE_WRITE); + } + if (xfer->rx_buf || xfer->rx_dma) { + desc->cfg_bus |= FIELD_PREP(CFG_LANE, nbits_to_lane[xfer->rx_nbits]); + desc->cfg_start |= FIELD_PREP(CFG_OP_MODE, SPISG_OP_MODE_READ); + } + + if (FIELD_GET(CFG_OP_MODE, desc->cfg_start) == SPISG_OP_MODE_READ_STS) { + desc->cfg_start |= FIELD_PREP(CFG_BLOCK_SIZE, blocks) | + FIELD_PREP(CFG_BLOCK_NUM, 1); + } else { + blocks = min_t(int, blocks, SPISG_BLOCK_MAX); + desc->cfg_start |= FIELD_PREP(CFG_BLOCK_SIZE, block_size & 0x7) | + FIELD_PREP(CFG_BLOCK_NUM, blocks); + } + + if (xfer->tx_sg.nents && xfer->tx_sg.sgl) { + ccsg_len = xfer->tx_sg.nents * sizeof(struct spisg_sg_link); + ccsg = kzalloc(ccsg_len, GFP_KERNEL | GFP_DMA); + if (!ccsg) { + dev_err(dev, "alloc tx_ccsg failed\n"); + return -ENOMEM; + } + + aml_spisg_sg_xlate(&xfer->tx_sg, ccsg); + paddr = dma_map_single(dev, (void *)ccsg, + ccsg_len, DMA_TO_DEVICE); + ret = dma_mapping_error(dev, paddr); + if (ret) { + kfree(ccsg); + dev_err(dev, "tx ccsg map failed\n"); + return ret; + } + + desc->tx_paddr = paddr; + desc->cfg_start |= FIELD_PREP(CFG_TXD_MODE, SPISG_DATA_MODE_SG); + exdesc->tx_ccsg = ccsg; + exdesc->tx_ccsg_len = ccsg_len; + dma_sync_sgtable_for_device(spisg->controller->cur_tx_dma_dev, + &xfer->tx_sg, DMA_TO_DEVICE); + } else if (xfer->tx_buf || xfer->tx_dma) { + paddr = xfer->tx_dma; + if (!paddr) { + paddr = dma_map_single(dev, (void *)xfer->tx_buf, + xfer->len, DMA_TO_DEVICE); + ret = dma_mapping_error(dev, paddr); + if (ret) { + dev_err(dev, "tx buf map failed\n"); + return ret; + } + } + desc->tx_paddr = paddr; + desc->cfg_start |= FIELD_PREP(CFG_TXD_MODE, SPISG_DATA_MODE_MEM); + } + + if (xfer->rx_sg.nents && xfer->rx_sg.sgl) { + ccsg_len = xfer->rx_sg.nents * sizeof(struct spisg_sg_link); + ccsg = kzalloc(ccsg_len, GFP_KERNEL | GFP_DMA); + if (!ccsg) { + dev_err(dev, "alloc rx_ccsg failed\n"); + return -ENOMEM; + } + + aml_spisg_sg_xlate(&xfer->rx_sg, ccsg); + paddr = dma_map_single(dev, (void *)ccsg, + ccsg_len, DMA_TO_DEVICE); + ret = dma_mapping_error(dev, paddr); + if (ret) { + kfree(ccsg); + dev_err(dev, "rx ccsg map failed\n"); + return ret; + } + + desc->rx_paddr = paddr; + desc->cfg_start |= FIELD_PREP(CFG_RXD_MODE, SPISG_DATA_MODE_SG); + exdesc->rx_ccsg = ccsg; + exdesc->rx_ccsg_len = ccsg_len; + dma_sync_sgtable_for_device(spisg->controller->cur_rx_dma_dev, + &xfer->rx_sg, DMA_FROM_DEVICE); + } else if (xfer->rx_buf || xfer->rx_dma) { + paddr = xfer->rx_dma; + if (!paddr) { + paddr = dma_map_single(dev, xfer->rx_buf, + xfer->len, DMA_FROM_DEVICE); + ret = dma_mapping_error(dev, paddr); + if (ret) { + dev_err(dev, "rx buf map failed\n"); + return ret; + } + } + + desc->rx_paddr = paddr; + desc->cfg_start |= FIELD_PREP(CFG_RXD_MODE, SPISG_DATA_MODE_MEM); + } + + return 0; +} + +static void aml_spisg_cleanup_transfer(struct spisg_device *spisg, + struct spi_transfer *xfer, + struct spisg_descriptor *desc, + struct spisg_descriptor_extra *exdesc) +{ + struct device *dev = &spisg->pdev->dev; + + if (desc->tx_paddr) { + if (FIELD_GET(CFG_TXD_MODE, desc->cfg_start) == SPISG_DATA_MODE_SG) { + dma_unmap_single(dev, (dma_addr_t)desc->tx_paddr, + exdesc->tx_ccsg_len, DMA_TO_DEVICE); + kfree(exdesc->tx_ccsg); + dma_sync_sgtable_for_cpu(spisg->controller->cur_tx_dma_dev, + &xfer->tx_sg, DMA_TO_DEVICE); + } else if (!xfer->tx_dma) { + dma_unmap_single(dev, (dma_addr_t)desc->tx_paddr, + xfer->len, DMA_TO_DEVICE); + } + } + + if (desc->rx_paddr) { + if (FIELD_GET(CFG_RXD_MODE, desc->cfg_start) == SPISG_DATA_MODE_SG) { + dma_unmap_single(dev, (dma_addr_t)desc->rx_paddr, + exdesc->rx_ccsg_len, DMA_TO_DEVICE); + kfree(exdesc->rx_ccsg); + dma_sync_sgtable_for_cpu(spisg->controller->cur_rx_dma_dev, + &xfer->rx_sg, DMA_FROM_DEVICE); + } else if (!xfer->rx_dma) { + dma_unmap_single(dev, (dma_addr_t)desc->rx_paddr, + xfer->len, DMA_FROM_DEVICE); + } + } +} + +static void aml_spisg_setup_null_desc(struct spisg_device *spisg, + struct spisg_descriptor *desc, + u32 n_sclk) +{ + /* unit is the last xfer sclk */ + desc->cfg_start = spisg->cfg_start; + desc->cfg_bus = spisg->cfg_bus; + + desc->cfg_start |= FIELD_PREP(CFG_OP_MODE, SPISG_OP_MODE_WRITE) | + FIELD_PREP(CFG_BLOCK_SIZE, 1) | + FIELD_PREP(CFG_BLOCK_NUM, DIV_ROUND_UP(n_sclk, 8)); + + desc->cfg_bus |= FIELD_PREP(CFG_NULL_CTL, 1); +} + +static void aml_spisg_pending(struct spisg_device *spisg, + dma_addr_t desc_paddr, + bool trig, + bool irq_en) +{ + u32 desc_l, desc_h, cfg_spi; + +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT + desc_l = (u64)desc_paddr & 0xffffffff; + desc_h = (u64)desc_paddr >> 32; +#else + desc_l = desc_paddr & 0xffffffff; + desc_h = 0; +#endif + + cfg_spi = spisg->cfg_spi; + if (trig) + cfg_spi |= CFG_HW_POS; + else + desc_h |= LIST_DESC_PENDING; + + regmap_write(spisg->map, SPISG_REG_IRQ_ENABLE, irq_en ? IRQ_DESC_CHAIN_DONE : 0); + regmap_write(spisg->map, SPISG_REG_CFG_SPI, cfg_spi); + regmap_write(spisg->map, SPISG_REG_DESC_LIST_L, desc_l); + regmap_write(spisg->map, SPISG_REG_DESC_LIST_H, desc_h); +} + +static irqreturn_t aml_spisg_irq(int irq, void *data) +{ + struct spisg_device *spisg = (void *)data; + u32 sts; + + spisg->status = 0; + regmap_read(spisg->map, SPISG_REG_IRQ_STS, &sts); + regmap_write(spisg->map, SPISG_REG_IRQ_STS, sts); + if (sts & (IRQ_RCH_DESC_INVALID | + IRQ_RCH_DESC_RESP | + IRQ_RCH_DATA_RESP | + IRQ_WCH_DESC_INVALID | + IRQ_WCH_DESC_RESP | + IRQ_WCH_DATA_RESP | + IRQ_DESC_ERR)) + spisg->status = sts; + + complete(&spisg->completion); + + return IRQ_HANDLED; +} + +static int aml_spisg_transfer_one_message(struct spi_controller *ctlr, + struct spi_message *msg) +{ + struct spisg_device *spisg = spi_controller_get_devdata(ctlr); + struct device *dev = &spisg->pdev->dev; + unsigned long long ms = 0; + struct spi_transfer *xfer; + struct spisg_descriptor *descs, *desc; + struct spisg_descriptor_extra *exdescs, *exdesc; + dma_addr_t descs_paddr; + int desc_num = 1, descs_len; + u32 cs_hold_in_sclk = 0; + int ret = -EIO; + + if (!aml_spisg_sem_down_read(spisg)) { + spi_finalize_current_message(ctlr); + dev_err(dev, "controller busy\n"); + return -EBUSY; + } + + /* calculate the desc num for all xfer */ + list_for_each_entry(xfer, &msg->transfers, transfer_list) + desc_num++; + + /* alloc descriptor/extra-descriptor table */ + descs = kcalloc(desc_num, sizeof(*desc) + sizeof(*exdesc), + GFP_KERNEL | GFP_DMA); + if (!descs) { + spi_finalize_current_message(ctlr); + aml_spisg_sem_up_write(spisg); + return -ENOMEM; + } + descs_len = sizeof(*desc) * desc_num; + exdescs = (struct spisg_descriptor_extra *)(descs + desc_num); + + /* config descriptor for each xfer */ + desc = descs; + exdesc = exdescs; + list_for_each_entry(xfer, &msg->transfers, transfer_list) { + ret = aml_spisg_setup_transfer(spisg, xfer, desc, exdesc); + if (ret) { + dev_err(dev, "config descriptor failed\n"); + goto end; + } + + /* calculate cs-setup delay with the first xfer speed */ + if (list_is_first(&xfer->transfer_list, &msg->transfers)) + desc->cfg_bus |= FIELD_PREP(CFG_CS_SETUP, + spi_delay_to_sclk(xfer->effective_speed_hz, &msg->spi->cs_setup)); + + /* calculate cs-hold delay with the last xfer speed */ + if (list_is_last(&xfer->transfer_list, &msg->transfers)) + cs_hold_in_sclk = + spi_delay_to_sclk(xfer->effective_speed_hz, &msg->spi->cs_hold); + + desc++; + exdesc++; + ms += DIV_ROUND_UP_ULL(8LL * MSEC_PER_SEC * xfer->len, + xfer->effective_speed_hz); + } + + if (cs_hold_in_sclk) + /* additional null-descriptor to achieve the cs-hold delay */ + aml_spisg_setup_null_desc(spisg, desc, cs_hold_in_sclk); + else + desc--; + + desc->cfg_bus |= FIELD_PREP(CFG_KEEP_SS, 0); + desc->cfg_start |= FIELD_PREP(CFG_EOC, 1); + + /* some tolerances */ + ms += ms + 20; + if (ms > UINT_MAX) + ms = UINT_MAX; + + descs_paddr = dma_map_single(dev, (void *)descs, + descs_len, DMA_TO_DEVICE); + ret = dma_mapping_error(dev, descs_paddr); + if (ret) { + dev_err(dev, "desc table map failed\n"); + goto end; + } + + reinit_completion(&spisg->completion); + aml_spisg_pending(spisg, descs_paddr, false, true); + if (wait_for_completion_timeout(&spisg->completion, + spi_controller_is_target(spisg->controller) ? + MAX_SCHEDULE_TIMEOUT : msecs_to_jiffies(ms))) + ret = spisg->status ? -EIO : 0; + else + ret = -ETIMEDOUT; + + dma_unmap_single(dev, descs_paddr, descs_len, DMA_TO_DEVICE); +end: + desc = descs; + exdesc = exdescs; + list_for_each_entry(xfer, &msg->transfers, transfer_list) + aml_spisg_cleanup_transfer(spisg, xfer, desc++, exdesc++); + kfree(descs); + + if (!ret) + msg->actual_length = msg->frame_length; + msg->status = ret; + spi_finalize_current_message(ctlr); + aml_spisg_sem_up_write(spisg); + + return ret; +} + +static int aml_spisg_prepare_message(struct spi_controller *ctlr, + struct spi_message *message) +{ + struct spisg_device *spisg = spi_controller_get_devdata(ctlr); + struct spi_device *spi = message->spi; + + if (!spi->bits_per_word || spi->bits_per_word % 8) { + dev_err(&spisg->pdev->dev, "invalid wordlen %d\n", spi->bits_per_word); + return -EINVAL; + } + + spisg->bytes_per_word = spi->bits_per_word >> 3; + + spisg->cfg_spi &= ~CFG_SLAVE_SELECT; + spisg->cfg_spi |= FIELD_PREP(CFG_SLAVE_SELECT, spi_get_chipselect(spi, 0)); + + spisg->cfg_bus &= ~(CFG_CPOL | CFG_CPHA | CFG_B_L_ENDIAN | CFG_HALF_DUPLEX); + spisg->cfg_bus |= FIELD_PREP(CFG_CPOL, !!(spi->mode & SPI_CPOL)) | + FIELD_PREP(CFG_CPHA, !!(spi->mode & SPI_CPHA)) | + FIELD_PREP(CFG_B_L_ENDIAN, !!(spi->mode & SPI_LSB_FIRST)) | + FIELD_PREP(CFG_HALF_DUPLEX, !!(spi->mode & SPI_3WIRE)); + + return 0; +} + +static int aml_spisg_setup(struct spi_device *spi) +{ + if (!spi->controller_state) + spi->controller_state = spi_controller_get_devdata(spi->controller); + + return 0; +} + +static void aml_spisg_cleanup(struct spi_device *spi) +{ + spi->controller_state = NULL; +} + +static int aml_spisg_target_abort(struct spi_controller *ctlr) +{ + struct spisg_device *spisg = spi_controller_get_devdata(ctlr); + + spisg->status = 0; + regmap_write(spisg->map, SPISG_REG_DESC_LIST_H, 0); + complete(&spisg->completion); + + return 0; +} + +static int aml_spisg_clk_init(struct spisg_device *spisg, void __iomem *base) +{ + struct device *dev = &spisg->pdev->dev; + struct clk_init_data init; + struct clk_divider *div; + struct clk_div_table *tbl; + const char *parent_names[1]; + char name[32]; + int i; + + spisg->core = devm_clk_get_enabled(dev, "core"); + if (IS_ERR_OR_NULL(spisg->core)) { + dev_err(dev, "core clock request failed\n"); + return PTR_ERR(spisg->core); + } + + spisg->pclk = devm_clk_get_enabled(dev, "pclk"); + if (IS_ERR_OR_NULL(spisg->pclk)) { + dev_err(dev, "pclk clock request failed\n"); + return PTR_ERR(spisg->pclk); + } + + clk_set_min_rate(spisg->pclk, SPISG_PCLK_RATE_MIN); + + clk_disable_unprepare(spisg->pclk); + + tbl = devm_kzalloc(dev, sizeof(struct clk_div_table) * (DIV_NUM + 1), GFP_KERNEL); + if (!tbl) + return -ENOMEM; + + for (i = 0; i < DIV_NUM; i++) { + tbl[i].val = i + SPISG_CLK_DIV_MIN - 1; + tbl[i].div = i + SPISG_CLK_DIV_MIN; + } + spisg->tbl = tbl; + + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL); + if (!div) + return -ENOMEM; + + div->flags = CLK_DIVIDER_ROUND_CLOSEST; + div->reg = base + SPISG_REG_CFG_BUS; + div->shift = __bf_shf(CFG_CLK_DIV); + div->width = CLK_DIV_WIDTH; + div->table = tbl; + + /* Register value should not be outside of the table */ + regmap_update_bits(spisg->map, SPISG_REG_CFG_BUS, CFG_CLK_DIV, + FIELD_PREP(CFG_CLK_DIV, SPISG_CLK_DIV_MIN - 1)); + + /* Register clk-divider */ + parent_names[0] = __clk_get_name(spisg->pclk); + snprintf(name, sizeof(name), "%s_div", dev_name(dev)); + init.name = name; + init.ops = &clk_divider_ops; + init.flags = CLK_SET_RATE_PARENT; + init.parent_names = parent_names; + init.num_parents = 1; + div->hw.init = &init; + + spisg->sclk = devm_clk_register(dev, &div->hw); + if (IS_ERR_OR_NULL(spisg->sclk)) { + dev_err(dev, "clock registration failed\n"); + return PTR_ERR(spisg->sclk); + } + + clk_prepare_enable(spisg->sclk); + + return 0; +} + +static int aml_spisg_probe(struct platform_device *pdev) +{ + struct spi_controller *ctlr; + struct spisg_device *spisg; + struct device *dev = &pdev->dev; + void __iomem *base; + int ret, irq; + + const struct regmap_config aml_regmap_config = { + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, + .max_register = SPISG_MAX_REG, + }; + + if (of_property_read_bool(dev->of_node, "spi-slave")) + ctlr = spi_alloc_target(dev, sizeof(*spisg)); + else + ctlr = spi_alloc_host(dev, sizeof(*spisg)); + if (!ctlr) + return dev_err_probe(dev, -ENOMEM, "controller allocation failed\n"); + + spisg = spi_controller_get_devdata(ctlr); + spisg->controller = ctlr; + + spisg->pdev = pdev; + platform_set_drvdata(pdev, spisg); + + base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(base)) + return dev_err_probe(dev, PTR_ERR(base), "resource ioremap failed\n"); + + spisg->map = devm_regmap_init_mmio(dev, base, &aml_regmap_config); + if (IS_ERR(spisg->map)) + return dev_err_probe(dev, PTR_ERR(spisg->map), "regmap init failed\n"); + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + ret = irq; + goto out_controller; + } + + ret = devm_request_irq(&pdev->dev, irq, aml_spisg_irq, 0, NULL, spisg); + if (ret) { + dev_err(&pdev->dev, "irq request failed\n"); + goto out_controller; + } + + ret = aml_spisg_clk_init(spisg, base); + if (ret) + goto out_controller; + + spisg->cfg_spi = 0; + spisg->cfg_start = 0; + spisg->cfg_bus = 0; + + spisg->cfg_spi = FIELD_PREP(CFG_SFLASH_WP, 1) | + FIELD_PREP(CFG_SFLASH_HD, 1); + if (spi_controller_is_target(ctlr)) { + spisg->cfg_spi |= FIELD_PREP(CFG_SLAVE_EN, 1); + spisg->cfg_bus = FIELD_PREP(CFG_TX_TUNING, 0xf); + } + /* default pending */ + spisg->cfg_start = FIELD_PREP(CFG_PEND, 1); + + pm_runtime_set_active(&spisg->pdev->dev); + pm_runtime_enable(&spisg->pdev->dev); + pm_runtime_resume_and_get(&spisg->pdev->dev); + + device_reset_optional(dev); + ctlr->num_chipselect = 4; + ctlr->dev.of_node = pdev->dev.of_node; + ctlr->mode_bits = SPI_CPHA | SPI_CPOL | SPI_LSB_FIRST | + SPI_3WIRE | SPI_TX_QUAD | SPI_RX_QUAD; + ctlr->max_speed_hz = 1000 * 1000 * 100; + ctlr->min_speed_hz = 1000 * 10; + ctlr->setup = aml_spisg_setup; + ctlr->cleanup = aml_spisg_cleanup; + ctlr->prepare_message = aml_spisg_prepare_message; + ctlr->transfer_one_message = aml_spisg_transfer_one_message; + ctlr->target_abort = aml_spisg_target_abort; + ctlr->can_dma = aml_spisg_can_dma; + ctlr->max_dma_len = SPISG_BLOCK_MAX; + ctlr->auto_runtime_pm = true; + + dma_set_max_seg_size(&pdev->dev, SPISG_BLOCK_MAX); + ret = devm_spi_register_controller(dev, ctlr); + if (ret) { + dev_err(&pdev->dev, "spi controller registration failed\n"); + goto out_clk; + } + + init_completion(&spisg->completion); + + pm_runtime_put(&spisg->pdev->dev); + + return 0; +out_clk: + if (spisg->core) + clk_disable_unprepare(spisg->core); + clk_disable_unprepare(spisg->pclk); +out_controller: + spi_controller_put(ctlr); + + return ret; +} + +static void aml_spisg_remove(struct platform_device *pdev) +{ + struct spisg_device *spisg = platform_get_drvdata(pdev); + + if (!pm_runtime_suspended(&pdev->dev)) { + pinctrl_pm_select_sleep_state(&spisg->pdev->dev); + clk_disable_unprepare(spisg->core); + clk_disable_unprepare(spisg->pclk); + } +} + +static int spisg_suspend_runtime(struct device *dev) +{ + struct spisg_device *spisg = dev_get_drvdata(dev); + + pinctrl_pm_select_sleep_state(&spisg->pdev->dev); + clk_disable_unprepare(spisg->sclk); + clk_disable_unprepare(spisg->core); + + return 0; +} + +static int spisg_resume_runtime(struct device *dev) +{ + struct spisg_device *spisg = dev_get_drvdata(dev); + + clk_prepare_enable(spisg->core); + clk_prepare_enable(spisg->sclk); + pinctrl_pm_select_default_state(&spisg->pdev->dev); + + return 0; +} + +static const struct dev_pm_ops amlogic_spisg_pm_ops = { + .runtime_suspend = spisg_suspend_runtime, + .runtime_resume = spisg_resume_runtime, +}; + +static const struct of_device_id amlogic_spisg_of_match[] = { + { + .compatible = "amlogic,a4-spisg", + }, + + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, amlogic_spisg_of_match); + +static struct platform_driver amlogic_spisg_driver = { + .probe = aml_spisg_probe, + .remove = aml_spisg_remove, + .driver = { + .name = "amlogic-spisg", + .of_match_table = amlogic_spisg_of_match, + .pm = &amlogic_spisg_pm_ops, + }, +}; + +module_platform_driver(amlogic_spisg_driver); + +MODULE_DESCRIPTION("Amlogic SPI Scatter-Gather Controller driver"); +MODULE_AUTHOR("Sunny Luo <sunny.luo@amlogic.com>"); +MODULE_LICENSE("GPL"); -- 2.37.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver 2025-07-04 2:59 ` [PATCH v4 2/3] spi: Add Amlogic SPISG driver Xianwei Zhao via B4 Relay @ 2025-07-07 13:05 ` Mark Brown 2025-07-08 10:34 ` Xianwei Zhao 2025-07-08 16:01 ` Martin Blumenstingl 1 sibling, 1 reply; 14+ messages in thread From: Mark Brown @ 2025-07-07 13:05 UTC (permalink / raw) To: xianwei.zhao Cc: Sunny Luo, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-amlogic, linux-spi, devicetree, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 2008 bytes --] On Fri, Jul 04, 2025 at 10:59:33AM +0800, Xianwei Zhao via B4 Relay wrote: > Introduced support for the new SPI IP (SPISG) driver. The SPISG is > a communication-oriented SPI controller from Amlogic,supporting > three operation modes: PIO, block DMA, and scatter-gather DMA. This looks good, a few small things below but nothing major. > +static bool aml_spisg_can_dma(struct spi_controller *ctlr, > + struct spi_device *spi, > + struct spi_transfer *xfer) > +{ > + return true; > +} Is it worth having a copybreak such that smaller transfers are done using PIO? With a lot of controllers that increases performance due to the extra overhead of setting up DMA, talking to the DMA and interrupt controllers can be as expensive as directly accessing the FIFOs. > +static irqreturn_t aml_spisg_irq(int irq, void *data) > +{ > + struct spisg_device *spisg = (void *)data; > + u32 sts; > + > + spisg->status = 0; > + regmap_read(spisg->map, SPISG_REG_IRQ_STS, &sts); > + regmap_write(spisg->map, SPISG_REG_IRQ_STS, sts); > + if (sts & (IRQ_RCH_DESC_INVALID | > + IRQ_RCH_DESC_RESP | > + IRQ_RCH_DATA_RESP | > + IRQ_WCH_DESC_INVALID | > + IRQ_WCH_DESC_RESP | > + IRQ_WCH_DATA_RESP | > + IRQ_DESC_ERR)) > + spisg->status = sts; > + > + complete(&spisg->completion); > + > + return IRQ_HANDLED; It'd be better to check if there's an interrupt actually flagged and return IRQ_NONE if not, as well as supporting sharing that means that the interrupt core can handle any errors that cause the interrupt to latch on. > + ret = devm_request_irq(&pdev->dev, irq, aml_spisg_irq, 0, NULL, spisg); > + if (ret) { > + dev_err(&pdev->dev, "irq request failed\n"); > + goto out_controller; > + } > + > + ret = aml_spisg_clk_init(spisg, base); > + if (ret) > + goto out_controller; Do we need the clocks for register access - if so what happens if the interrupt fires as soon as it is registered? I'd have expected requesting the interrupt to be one of the last things done. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 167 bytes --] _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver 2025-07-07 13:05 ` Mark Brown @ 2025-07-08 10:34 ` Xianwei Zhao 2025-07-08 13:50 ` Mark Brown 0 siblings, 1 reply; 14+ messages in thread From: Xianwei Zhao @ 2025-07-08 10:34 UTC (permalink / raw) To: Mark Brown Cc: Sunny Luo, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-amlogic, linux-spi, devicetree, linux-kernel Hi Mark, Thanks for your advice. On 2025/7/7 21:05, Mark Brown wrote: > Subject: > Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver > From: > Mark Brown <broonie@kernel.org> > Date: > 2025/7/7 21:05 > > To: > xianwei.zhao@amlogic.com > CC: > Sunny Luo <sunny.luo@amlogic.com>, Rob Herring <robh@kernel.org>, > Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley > <conor+dt@kernel.org>, linux-amlogic@lists.infradead.org, > linux-spi@vger.kernel.org, devicetree@vger.kernel.org, > linux-kernel@vger.kernel.org > > > > On Fri, Jul 04, 2025 at 10:59:33AM +0800, Xianwei Zhao via B4 Relay wrote: > >> Introduced support for the new SPI IP (SPISG) driver. The SPISG is >> a communication-oriented SPI controller from Amlogic,supporting >> three operation modes: PIO, block DMA, and scatter-gather DMA. > This looks good, a few small things below but nothing major. > >> +static bool aml_spisg_can_dma(struct spi_controller *ctlr, >> + struct spi_device *spi, >> + struct spi_transfer *xfer) >> +{ >> + return true; >> +} > Is it worth having a copybreak such that smaller transfers are done > using PIO? With a lot of controllers that increases performance due to > the extra overhead of setting up DMA, talking to the DMA and interrupt > controllers can be as expensive as directly accessing the FIFOs. > If the data volume of a single transfer (xfer) is small, PIO mode does offer some advantages. However, since PIO requires the CPU to wait in a busy loop for the transfer to complete, it continuously occupies CPU resources. As a result, its advantages are not particularly significant. If PIO is to be implemented, it can only handle one transfer at a time (via transfer_one), and not entire messages (which consist of multiple transfers). In contrast, when processing messages, the SPI controller can handle the entire sequence in one go, which also provides certain benefits. Taking all factors into account, it may be better not to add PIO support. >> +static irqreturn_t aml_spisg_irq(int irq, void *data) >> +{ >> + struct spisg_device *spisg = (void *)data; >> + u32 sts; >> + >> + spisg->status = 0; >> + regmap_read(spisg->map, SPISG_REG_IRQ_STS, &sts); >> + regmap_write(spisg->map, SPISG_REG_IRQ_STS, sts); >> + if (sts & (IRQ_RCH_DESC_INVALID | >> + IRQ_RCH_DESC_RESP | >> + IRQ_RCH_DATA_RESP | >> + IRQ_WCH_DESC_INVALID | >> + IRQ_WCH_DESC_RESP | >> + IRQ_WCH_DATA_RESP | >> + IRQ_DESC_ERR)) >> + spisg->status = sts; >> + >> + complete(&spisg->completion); >> + >> + return IRQ_HANDLED; > It'd be better to check if there's an interrupt actually flagged and > return IRQ_NONE if not, as well as supporting sharing that means that > the interrupt core can handle any errors that cause the interrupt to > latch on. > Will do,unreasonable values will be returned IRQ_NONE. >> + ret = devm_request_irq(&pdev->dev, irq, aml_spisg_irq, 0, NULL, spisg); >> + if (ret) { >> + dev_err(&pdev->dev, "irq request failed\n"); >> + goto out_controller; >> + } >> + >> + ret = aml_spisg_clk_init(spisg, base); >> + if (ret) >> + goto out_controller; > Do we need the clocks for register access - if so what happens if the > interrupt fires as soon as it is registered? I'd have expected > requesting the interrupt to be one of the last things done. Will move irq request of the processing to a bit further back. _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver 2025-07-08 10:34 ` Xianwei Zhao @ 2025-07-08 13:50 ` Mark Brown 2025-07-09 7:02 ` Xianwei Zhao 0 siblings, 1 reply; 14+ messages in thread From: Mark Brown @ 2025-07-08 13:50 UTC (permalink / raw) To: Xianwei Zhao Cc: Sunny Luo, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-amlogic, linux-spi, devicetree, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 1554 bytes --] On Tue, Jul 08, 2025 at 06:34:02PM +0800, Xianwei Zhao wrote: > On 2025/7/7 21:05, Mark Brown wrote: > > Is it worth having a copybreak such that smaller transfers are done > > using PIO? With a lot of controllers that increases performance due to > > the extra overhead of setting up DMA, talking to the DMA and interrupt > > controllers can be as expensive as directly accessing the FIFOs. > If the data volume of a single transfer (xfer) is small, PIO mode does offer > some advantages. However, since PIO requires the CPU to wait in a busy loop > for the transfer to complete, it continuously occupies CPU resources. As a > result, its advantages are not particularly significant. The CPU overhead tends to be higher (you can avoid some of it with a dead reckoning sleep), but the latency vastly improved which for many applications is a worthwhile advantage. It tends to be things like accesses to one or two registers on a device with registers where this wins, 16 bytes or lower would be a common number off the top of my head. > If PIO is to be implemented, it can only handle one transfer at a time (via > transfer_one), and not entire messages (which consist of multiple > transfers). In contrast, when processing messages, the SPI controller can > handle the entire sequence in one go, which also provides certain benefits. It's probably worth adding something to the framework to be able to take a decision at the message level, for writes this tends to all fall out naturally since the write will tend to be a single transfer anyway. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 167 bytes --] _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver 2025-07-08 13:50 ` Mark Brown @ 2025-07-09 7:02 ` Xianwei Zhao 2025-07-16 9:30 ` Xianwei Zhao 0 siblings, 1 reply; 14+ messages in thread From: Xianwei Zhao @ 2025-07-09 7:02 UTC (permalink / raw) To: Mark Brown Cc: Sunny Luo, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-amlogic, linux-spi, devicetree, linux-kernel Hi Mark, Thanks for your advice. On 2025/7/8 21:50, Mark Brown wrote: > Subject: > Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver > From: > Mark Brown <broonie@kernel.org> > Date: > 2025/7/8 21:50 > > To: > Xianwei Zhao <xianwei.zhao@amlogic.com> > CC: > Sunny Luo <sunny.luo@amlogic.com>, Rob Herring <robh@kernel.org>, > Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley > <conor+dt@kernel.org>, linux-amlogic@lists.infradead.org, > linux-spi@vger.kernel.org, devicetree@vger.kernel.org, > linux-kernel@vger.kernel.org > > > > On Tue, Jul 08, 2025 at 06:34:02PM +0800, Xianwei Zhao wrote: >> On 2025/7/7 21:05, Mark Brown wrote: >>> Is it worth having a copybreak such that smaller transfers are done >>> using PIO? With a lot of controllers that increases performance due to >>> the extra overhead of setting up DMA, talking to the DMA and interrupt >>> controllers can be as expensive as directly accessing the FIFOs. >> If the data volume of a single transfer (xfer) is small, PIO mode does offer >> some advantages. However, since PIO requires the CPU to wait in a busy loop >> for the transfer to complete, it continuously occupies CPU resources. As a >> result, its advantages are not particularly significant. > The CPU overhead tends to be higher (you can avoid some of it with a > dead reckoning sleep), but the latency vastly improved which for many > applications is a worthwhile advantage. It tends to be things like > accesses to one or two registers on a device with registers where this > wins, 16 bytes or lower would be a common number off the top of my head. > >> If PIO is to be implemented, it can only handle one transfer at a time (via >> transfer_one), and not entire messages (which consist of multiple >> transfers). In contrast, when processing messages, the SPI controller can >> handle the entire sequence in one go, which also provides certain benefits. > It's probably worth adding something to the framework to be able to take > a decision at the message level, for writes this tends to all fall out > naturally since the write will tend to be a single transfer anyway. I will try to add new API message_can_dma for framework, and implement PIO for message. _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver 2025-07-09 7:02 ` Xianwei Zhao @ 2025-07-16 9:30 ` Xianwei Zhao 2025-07-16 16:25 ` Da Xue 0 siblings, 1 reply; 14+ messages in thread From: Xianwei Zhao @ 2025-07-16 9:30 UTC (permalink / raw) To: Mark Brown Cc: Sunny Luo, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-amlogic, linux-spi, devicetree, linux-kernel Hi Mark, On 2025/7/9 15:02, Xianwei Zhao wrote: > Hi Mark, > Thanks for your advice. > > On 2025/7/8 21:50, Mark Brown wrote: >> Subject: >> Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver >> From: >> Mark Brown <broonie@kernel.org> >> Date: >> 2025/7/8 21:50 >> >> To: >> Xianwei Zhao <xianwei.zhao@amlogic.com> >> CC: >> Sunny Luo <sunny.luo@amlogic.com>, Rob Herring <robh@kernel.org>, >> Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley >> <conor+dt@kernel.org>, linux-amlogic@lists.infradead.org, >> linux-spi@vger.kernel.org, devicetree@vger.kernel.org, >> linux-kernel@vger.kernel.org >> >> >> >> On Tue, Jul 08, 2025 at 06:34:02PM +0800, Xianwei Zhao wrote: >>> On 2025/7/7 21:05, Mark Brown wrote: >>>> Is it worth having a copybreak such that smaller transfers are done >>>> using PIO? With a lot of controllers that increases performance due to >>>> the extra overhead of setting up DMA, talking to the DMA and interrupt >>>> controllers can be as expensive as directly accessing the FIFOs. >>> If the data volume of a single transfer (xfer) is small, PIO mode >>> does offer >>> some advantages. However, since PIO requires the CPU to wait in a >>> busy loop >>> for the transfer to complete, it continuously occupies CPU resources. >>> As a >>> result, its advantages are not particularly significant. >> The CPU overhead tends to be higher (you can avoid some of it with a >> dead reckoning sleep), but the latency vastly improved which for many >> applications is a worthwhile advantage. It tends to be things like >> accesses to one or two registers on a device with registers where this >> wins, 16 bytes or lower would be a common number off the top of my head. >> >>> If PIO is to be implemented, it can only handle one transfer at a >>> time (via >>> transfer_one), and not entire messages (which consist of multiple >>> transfers). In contrast, when processing messages, the SPI controller >>> can >>> handle the entire sequence in one go, which also provides certain >>> benefits. >> It's probably worth adding something to the framework to be able to take >> a decision at the message level, for writes this tends to all fall out >> naturally since the write will tend to be a single transfer anyway. > > I will try to add new API message_can_dma for framework, and implement > PIO for message. > I tried to implement PIO mode in the driver, but it turned out to be too slow. Due to the lack of an internal FIFO, data could only be transmitted one word at a time, and each transmission required reconfiguring the corresponding registers. As a result, the efficiency was quite low. The only simplifications provided by PIO mode were in two areas: 1. The allocation and release of the transfer descriptor 2. The DMA mapping and unmapping process Therefore, I suggest not implementing PIO mode in this driver. I will document clearly in the code that PIO mode is not supported and explain the reasons behind this decision. _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver 2025-07-16 9:30 ` Xianwei Zhao @ 2025-07-16 16:25 ` Da Xue 2025-07-17 3:06 ` Xianwei Zhao 0 siblings, 1 reply; 14+ messages in thread From: Da Xue @ 2025-07-16 16:25 UTC (permalink / raw) To: Xianwei Zhao Cc: Mark Brown, Sunny Luo, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-amlogic, linux-spi, devicetree, linux-kernel On Wed, Jul 16, 2025 at 5:30 AM Xianwei Zhao <xianwei.zhao@amlogic.com> wrote: > > Hi Mark, > > On 2025/7/9 15:02, Xianwei Zhao wrote: > > Hi Mark, > > Thanks for your advice. > > > > On 2025/7/8 21:50, Mark Brown wrote: > >> Subject: > >> Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver > >> From: > >> Mark Brown <broonie@kernel.org> > >> Date: > >> 2025/7/8 21:50 > >> > >> To: > >> Xianwei Zhao <xianwei.zhao@amlogic.com> > >> CC: > >> Sunny Luo <sunny.luo@amlogic.com>, Rob Herring <robh@kernel.org>, > >> Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley > >> <conor+dt@kernel.org>, linux-amlogic@lists.infradead.org, > >> linux-spi@vger.kernel.org, devicetree@vger.kernel.org, > >> linux-kernel@vger.kernel.org > >> > >> > >> > >> On Tue, Jul 08, 2025 at 06:34:02PM +0800, Xianwei Zhao wrote: > >>> On 2025/7/7 21:05, Mark Brown wrote: > >>>> Is it worth having a copybreak such that smaller transfers are done > >>>> using PIO? With a lot of controllers that increases performance due to > >>>> the extra overhead of setting up DMA, talking to the DMA and interrupt > >>>> controllers can be as expensive as directly accessing the FIFOs. > >>> If the data volume of a single transfer (xfer) is small, PIO mode > >>> does offer > >>> some advantages. However, since PIO requires the CPU to wait in a > >>> busy loop > >>> for the transfer to complete, it continuously occupies CPU resources. > >>> As a > >>> result, its advantages are not particularly significant. > >> The CPU overhead tends to be higher (you can avoid some of it with a > >> dead reckoning sleep), but the latency vastly improved which for many > >> applications is a worthwhile advantage. It tends to be things like > >> accesses to one or two registers on a device with registers where this > >> wins, 16 bytes or lower would be a common number off the top of my head. > >> > >>> If PIO is to be implemented, it can only handle one transfer at a > >>> time (via > >>> transfer_one), and not entire messages (which consist of multiple > >>> transfers). In contrast, when processing messages, the SPI controller > >>> can > >>> handle the entire sequence in one go, which also provides certain > >>> benefits. > >> It's probably worth adding something to the framework to be able to take > >> a decision at the message level, for writes this tends to all fall out > >> naturally since the write will tend to be a single transfer anyway. > > > > I will try to add new API message_can_dma for framework, and implement > > PIO for message. > > > > I tried to implement PIO mode in the driver, but it turned out to be too > slow. Due to the lack of an internal FIFO, data could only be > transmitted one word at a time, and each transmission required > reconfiguring the corresponding registers. As a result, the efficiency > was quite low. One of the use cases is for SPI-based displays and it transfers one word via PIO and then a lot of words via DMA. I see PIO as beneficial for this common use case for SPI. The efficiency does not need to be high for this one word but reducing the latency for DMA setup is a significant gain. > > The only simplifications provided by PIO mode were in two areas: > > 1. The allocation and release of the transfer descriptor > 2. The DMA mapping and unmapping process > > Therefore, I suggest not implementing PIO mode in this driver. I will > document clearly in the code that PIO mode is not supported and explain > the reasons behind this decision. > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver 2025-07-16 16:25 ` Da Xue @ 2025-07-17 3:06 ` Xianwei Zhao 0 siblings, 0 replies; 14+ messages in thread From: Xianwei Zhao @ 2025-07-17 3:06 UTC (permalink / raw) To: Da Xue Cc: Mark Brown, Sunny Luo, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-amlogic, linux-spi, devicetree, linux-kernel On 2025/7/17 00:25, Da Xue wrote: > [ EXTERNAL EMAIL ] > > On Wed, Jul 16, 2025 at 5:30 AM Xianwei Zhao <xianwei.zhao@amlogic.com> wrote: >> >> Hi Mark, >> >> On 2025/7/9 15:02, Xianwei Zhao wrote: >>> Hi Mark, >>> Thanks for your advice. >>> >>> On 2025/7/8 21:50, Mark Brown wrote: >>>> Subject: >>>> Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver >>>> From: >>>> Mark Brown <broonie@kernel.org> >>>> Date: >>>> 2025/7/8 21:50 >>>> >>>> To: >>>> Xianwei Zhao <xianwei.zhao@amlogic.com> >>>> CC: >>>> Sunny Luo <sunny.luo@amlogic.com>, Rob Herring <robh@kernel.org>, >>>> Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley >>>> <conor+dt@kernel.org>, linux-amlogic@lists.infradead.org, >>>> linux-spi@vger.kernel.org, devicetree@vger.kernel.org, >>>> linux-kernel@vger.kernel.org >>>> >>>> >>>> >>>> On Tue, Jul 08, 2025 at 06:34:02PM +0800, Xianwei Zhao wrote: >>>>> On 2025/7/7 21:05, Mark Brown wrote: >>>>>> Is it worth having a copybreak such that smaller transfers are done >>>>>> using PIO? With a lot of controllers that increases performance due to >>>>>> the extra overhead of setting up DMA, talking to the DMA and interrupt >>>>>> controllers can be as expensive as directly accessing the FIFOs. >>>>> If the data volume of a single transfer (xfer) is small, PIO mode >>>>> does offer >>>>> some advantages. However, since PIO requires the CPU to wait in a >>>>> busy loop >>>>> for the transfer to complete, it continuously occupies CPU resources. >>>>> As a >>>>> result, its advantages are not particularly significant. >>>> The CPU overhead tends to be higher (you can avoid some of it with a >>>> dead reckoning sleep), but the latency vastly improved which for many >>>> applications is a worthwhile advantage. It tends to be things like >>>> accesses to one or two registers on a device with registers where this >>>> wins, 16 bytes or lower would be a common number off the top of my head. >>>> >>>>> If PIO is to be implemented, it can only handle one transfer at a >>>>> time (via >>>>> transfer_one), and not entire messages (which consist of multiple >>>>> transfers). In contrast, when processing messages, the SPI controller >>>>> can >>>>> handle the entire sequence in one go, which also provides certain >>>>> benefits. >>>> It's probably worth adding something to the framework to be able to take >>>> a decision at the message level, for writes this tends to all fall out >>>> naturally since the write will tend to be a single transfer anyway. >>> >>> I will try to add new API message_can_dma for framework, and implement >>> PIO for message. >>> >> >> I tried to implement PIO mode in the driver, but it turned out to be too >> slow. Due to the lack of an internal FIFO, data could only be >> transmitted one word at a time, and each transmission required >> reconfiguring the corresponding registers. As a result, the efficiency >> was quite low. > > One of the use cases is for SPI-based displays and it transfers one > word via PIO and then a lot of words via DMA. I see PIO as beneficial > for this common use case for SPI. > The efficiency does not need to be high for this one word but reducing > the latency for DMA setup is a significant gain. > For the new SPISG driver, regardless of the mode(PIO, DMA(MEM or sg), the configuration registers are the same; the only difference value in the mode selection. see reg cfg_start. >> >> The only simplifications provided by PIO mode were in two areas: >> >> 1. The allocation and release of the transfer descriptor >> 2. The DMA mapping and unmapping process >> >> Therefore, I suggest not implementing PIO mode in this driver. I will >> document clearly in the code that PIO mode is not supported and explain >> the reasons behind this decision. >> >> _______________________________________________ >> linux-amlogic mailing list >> linux-amlogic@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-amlogic _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver 2025-07-04 2:59 ` [PATCH v4 2/3] spi: Add Amlogic SPISG driver Xianwei Zhao via B4 Relay 2025-07-07 13:05 ` Mark Brown @ 2025-07-08 16:01 ` Martin Blumenstingl 2025-07-09 6:29 ` Xianwei Zhao 1 sibling, 1 reply; 14+ messages in thread From: Martin Blumenstingl @ 2025-07-08 16:01 UTC (permalink / raw) To: xianwei.zhao Cc: Sunny Luo, Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-amlogic, linux-spi, devicetree, linux-kernel Hi, On Fri, Jul 4, 2025 at 5:07 AM Xianwei Zhao via B4 Relay <devnull+xianwei.zhao.amlogic.com@kernel.org> wrote: [...] > + div->table = tbl; > + > + /* Register value should not be outside of the table */ > + regmap_update_bits(spisg->map, SPISG_REG_CFG_BUS, CFG_CLK_DIV, > + FIELD_PREP(CFG_CLK_DIV, SPISG_CLK_DIV_MIN - 1)); Are you doing this to prevent errors for value zero? If so, is CLK_DIVIDER_MAX_AT_ZERO applicable instead (it has been discussed for the t7 clock controller recently: [0])? > + /* Register clk-divider */ > + parent_names[0] = __clk_get_name(spisg->pclk); Instead of using __clk_get_name my suggestion is to use struct clk_parent_data with .fw_name set. If you want to simplify the code further you can use helper macros like CLK_HW_INIT_FW_NAME > + snprintf(name, sizeof(name), "%s_div", dev_name(dev)); > + init.name = name; > + init.ops = &clk_divider_ops; > + init.flags = CLK_SET_RATE_PARENT; > + init.parent_names = parent_names; > + init.num_parents = 1; > + div->hw.init = &init; > + > + spisg->sclk = devm_clk_register(dev, &div->hw); My understanding is that devm_clk_register() is not recommended for new drivers. The replacement is to use devm_clk_hw_register() first, then devm_clk_hw_get_clk(). drivers/pwm/pwm-meson.c implements this in case you're looking for an example [...] > +static int aml_spisg_probe(struct platform_device *pdev) > +{ > + struct spi_controller *ctlr; > + struct spisg_device *spisg; > + struct device *dev = &pdev->dev; > + void __iomem *base; > + int ret, irq; > + > + const struct regmap_config aml_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = SPISG_MAX_REG, > + }; Most regmap_configs in Amlogic drivers are static const. If you make it a static const then I suggest renaming the variable to aml_spisg_regmap_config for consistency. [...] > + device_reset_optional(dev); I haven't checked the reset code but I think the return value still needs to be checked (drivers/mmc/host/meson-gx-mmc.c does so). Even though the reset is optional there's conditions where we must act for example on -EPROBE_DEFER (which must be propagated). Best regards, Martin [0] https://lore.kernel.org/linux-amlogic/bd68352f-7f8c-4cbc-9f4f-f83645cc1f70@amlogic.com/ _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver 2025-07-08 16:01 ` Martin Blumenstingl @ 2025-07-09 6:29 ` Xianwei Zhao 2025-07-09 9:36 ` Martin Blumenstingl 0 siblings, 1 reply; 14+ messages in thread From: Xianwei Zhao @ 2025-07-09 6:29 UTC (permalink / raw) To: Martin Blumenstingl Cc: Sunny Luo, Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-amlogic, linux-spi, devicetree, linux-kernel Hi Martin, Thanks for your reply. On 2025/7/9 00:01, Martin Blumenstingl wrote: > [ EXTERNAL EMAIL ] > > Hi, > > On Fri, Jul 4, 2025 at 5:07 AM Xianwei Zhao via B4 Relay > <devnull+xianwei.zhao.amlogic.com@kernel.org> wrote: > [...] >> + div->table = tbl; >> + >> + /* Register value should not be outside of the table */ >> + regmap_update_bits(spisg->map, SPISG_REG_CFG_BUS, CFG_CLK_DIV, >> + FIELD_PREP(CFG_CLK_DIV, SPISG_CLK_DIV_MIN - 1)); > Are you doing this to prevent errors for value zero? > If so, is CLK_DIVIDER_MAX_AT_ZERO applicable instead (it has been > discussed for the t7 clock controller recently: [0])? > No, if add add flag CLK_DIVIDER_MAX_AT_ZERO, reg value will equals divider, see https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/clk/clk-divider.c?h=next-20250708 As follow in function _get_div. "if (flags & CLK_DIVIDER_MAX_AT_ZERO) return val ? val : clk_div_mask(width) + 1;" But here reg value adding one equals divider. >> + /* Register clk-divider */ >> + parent_names[0] = __clk_get_name(spisg->pclk); > Instead of using __clk_get_name my suggestion is to use struct > clk_parent_data with .fw_name set. > If you want to simplify the code further you can use helper macros > like CLK_HW_INIT_FW_NAME here I don't know parent fw_name, The system does not have a corresponding interface to obtain parent fw_name, only name (clk-core->name) > >> + snprintf(name, sizeof(name), "%s_div", dev_name(dev)); >> + init.name = name; >> + init.ops = &clk_divider_ops; >> + init.flags = CLK_SET_RATE_PARENT; >> + init.parent_names = parent_names; >> + init.num_parents = 1; >> + div->hw.init = &init; >> + >> + spisg->sclk = devm_clk_register(dev, &div->hw); > My understanding is that devm_clk_register() is not recommended for new drivers. > The replacement is to use devm_clk_hw_register() first, then > devm_clk_hw_get_clk(). drivers/pwm/pwm-meson.c implements this in case > you're looking for an example > Will do. > > [...] >> +static int aml_spisg_probe(struct platform_device *pdev) >> +{ >> + struct spi_controller *ctlr; >> + struct spisg_device *spisg; >> + struct device *dev = &pdev->dev; >> + void __iomem *base; >> + int ret, irq; >> + >> + const struct regmap_config aml_regmap_config = { >> + .reg_bits = 32, >> + .val_bits = 32, >> + .reg_stride = 4, >> + .max_register = SPISG_MAX_REG, >> + }; > Most regmap_configs in Amlogic drivers are static const. > If you make it a static const then I suggest renaming the variable to > aml_spisg_regmap_config for consistency. > regmap_config as a local variable, it can save space. > [...] >> + device_reset_optional(dev); > I haven't checked the reset code but I think the return value still > needs to be checked (drivers/mmc/host/meson-gx-mmc.c does so). > Even though the reset is optional there's conditions where we must act > for example on -EPROBE_DEFER (which must be propagated). > The reset might not exist for this node, see relevant yaml file. > > Best regards, > Martin > > > [0] https://lore.kernel.org/linux-amlogic/bd68352f-7f8c-4cbc-9f4f-f83645cc1f70@amlogic.com/ _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/3] spi: Add Amlogic SPISG driver 2025-07-09 6:29 ` Xianwei Zhao @ 2025-07-09 9:36 ` Martin Blumenstingl 0 siblings, 0 replies; 14+ messages in thread From: Martin Blumenstingl @ 2025-07-09 9:36 UTC (permalink / raw) To: Xianwei Zhao Cc: Sunny Luo, Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-amlogic, linux-spi, devicetree, linux-kernel Hi, On Wed, Jul 9, 2025 at 8:29 AM Xianwei Zhao <xianwei.zhao@amlogic.com> wrote: > > Hi Martin, > Thanks for your reply. > > On 2025/7/9 00:01, Martin Blumenstingl wrote: > > [ EXTERNAL EMAIL ] > > > > Hi, > > > > On Fri, Jul 4, 2025 at 5:07 AM Xianwei Zhao via B4 Relay > > <devnull+xianwei.zhao.amlogic.com@kernel.org> wrote: > > [...] > >> + div->table = tbl; > >> + > >> + /* Register value should not be outside of the table */ > >> + regmap_update_bits(spisg->map, SPISG_REG_CFG_BUS, CFG_CLK_DIV, > >> + FIELD_PREP(CFG_CLK_DIV, SPISG_CLK_DIV_MIN - 1)); > > Are you doing this to prevent errors for value zero? > > If so, is CLK_DIVIDER_MAX_AT_ZERO applicable instead (it has been > > discussed for the t7 clock controller recently: [0])? > > > > No, if add add flag CLK_DIVIDER_MAX_AT_ZERO, reg value will equals > divider, see > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/clk/clk-divider.c?h=next-20250708 > As follow in function _get_div. > "if (flags & CLK_DIVIDER_MAX_AT_ZERO) > return val ? val : clk_div_mask(width) + 1;" > > But here reg value adding one equals divider. I see, thanks. > >> + /* Register clk-divider */ > >> + parent_names[0] = __clk_get_name(spisg->pclk); > > Instead of using __clk_get_name my suggestion is to use struct > > clk_parent_data with .fw_name set. > > If you want to simplify the code further you can use helper macros > > like CLK_HW_INIT_FW_NAME > > here I don't know parent fw_name, The system does not have a > corresponding interface to obtain parent fw_name, only name (clk-core->name) The .fw_name is simply "pclk" in this case. That clock is then internally looked up (by the common clock framework) using the "clock-names" and "clocks" properties from your device-tree. [...] > >> +static int aml_spisg_probe(struct platform_device *pdev) > >> +{ > >> + struct spi_controller *ctlr; > >> + struct spisg_device *spisg; > >> + struct device *dev = &pdev->dev; > >> + void __iomem *base; > >> + int ret, irq; > >> + > >> + const struct regmap_config aml_regmap_config = { > >> + .reg_bits = 32, > >> + .val_bits = 32, > >> + .reg_stride = 4, > >> + .max_register = SPISG_MAX_REG, > >> + }; > > Most regmap_configs in Amlogic drivers are static const. > > If you make it a static const then I suggest renaming the variable to > > aml_spisg_regmap_config for consistency. > > > > regmap_config as a local variable, it can save space. Thanks, I was not aware of that. For documentation purposes, with the original approach: $ aarch64-linux-gnu-size -d drivers/spi/spi-amlogic-spisg.ko text data bss dec hex filename 7504 1377 0 8881 22b1 drivers/spi/spi-amlogic-spisg.ko and making the regmap_config static const: $ aarch64-linux-gnu-size -d drivers/spi/spi-amlogic-spisg.ko text data bss dec hex filename 7716 1377 0 9093 2385 drivers/spi/spi-amlogic-spisg.ko [...] > >> + device_reset_optional(dev); > > I haven't checked the reset code but I think the return value still > > needs to be checked (drivers/mmc/host/meson-gx-mmc.c does so). > > Even though the reset is optional there's conditions where we must act > > for example on -EPROBE_DEFER (which must be propagated). > > > > The reset might not exist for this node, see relevant yaml file. This part I understand. Optional however doesn't mean that we can simply ignore all errors. Let's consider the following three scenarios: 1) reset not provided in .dtb -> we don't expect any error here 2) reset is provided in .dtb but the reset-controller has not been registered -> spisg driver must propagate -EPROBE_DEFER 3) reset is provided in .dtb but reset_control_reset (which is used internally in device_reset_optional) returns an error -> spisg must propagate this error Your code works for the first case but it doesn't consider the other two. That said, I'm not sure if the third case is realistic given the T7 reset controller uses MMIO access. This may change in future though (if SPISG IP is the same but the reset controller changes). It still leaves the second case where the spisg driver should be probed only after the reset controller is available. Best regards, Martin _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4 3/3] MAINTAINERS: Add an entry for Amlogic spi driver 2025-07-04 2:59 [PATCH v4 0/3] support for amlogic the new SPI IP Xianwei Zhao via B4 Relay 2025-07-04 2:59 ` [PATCH v4 1/3] spi: dt-bindings: Add binding document of Amlogic SPISG controller Xianwei Zhao via B4 Relay 2025-07-04 2:59 ` [PATCH v4 2/3] spi: Add Amlogic SPISG driver Xianwei Zhao via B4 Relay @ 2025-07-04 2:59 ` Xianwei Zhao via B4 Relay 2 siblings, 0 replies; 14+ messages in thread From: Xianwei Zhao via B4 Relay @ 2025-07-04 2:59 UTC (permalink / raw) To: Sunny Luo, Mark Brown, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-amlogic, linux-spi, devicetree, linux-kernel, Xianwei Zhao From: Xianwei Zhao <xianwei.zhao@amlogic.com> Add Amlogic spi entry to MAINTAINERS to clarify the maintainers. Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com> --- MAINTAINERS | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index a92290fffa16..8225df5ede74 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1307,6 +1307,15 @@ S: Maintained F: Documentation/devicetree/bindings/rtc/amlogic,a4-rtc.yaml F: drivers/rtc/rtc-amlogic-a4.c +AMLOGIC SPISG DRIVER +M: Sunny Luo <sunny.luo@amlogic.com> +M: Xianwei Zhao <xianwei.zhao@amlogic.com> +L: linux-amlogic@lists.infradead.org +L: linux-spi@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml +F: drivers/spi/spi-amlogic-spisg.c + AMPHENOL CHIPCAP 2 DRIVER M: Javier Carrasco <javier.carrasco.cruz@gmail.com> L: linux-hwmon@vger.kernel.org -- 2.37.1 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2025-07-17 3:07 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-07-04 2:59 [PATCH v4 0/3] support for amlogic the new SPI IP Xianwei Zhao via B4 Relay 2025-07-04 2:59 ` [PATCH v4 1/3] spi: dt-bindings: Add binding document of Amlogic SPISG controller Xianwei Zhao via B4 Relay 2025-07-04 2:59 ` [PATCH v4 2/3] spi: Add Amlogic SPISG driver Xianwei Zhao via B4 Relay 2025-07-07 13:05 ` Mark Brown 2025-07-08 10:34 ` Xianwei Zhao 2025-07-08 13:50 ` Mark Brown 2025-07-09 7:02 ` Xianwei Zhao 2025-07-16 9:30 ` Xianwei Zhao 2025-07-16 16:25 ` Da Xue 2025-07-17 3:06 ` Xianwei Zhao 2025-07-08 16:01 ` Martin Blumenstingl 2025-07-09 6:29 ` Xianwei Zhao 2025-07-09 9:36 ` Martin Blumenstingl 2025-07-04 2:59 ` [PATCH v4 3/3] MAINTAINERS: Add an entry for Amlogic spi driver Xianwei Zhao via B4 Relay
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).