linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/6] spi: rockchip: Stop spi slave dma receiver when cs inactive
@ 2022-02-11  3:43 Jon Lin
  2022-02-11  3:43 ` [PATCH 2/6] spi: rockchip: Preset cs-high and clk polarity in setup progress Jon Lin
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Jon Lin @ 2022-02-11  3:43 UTC (permalink / raw)
  To: broonie
  Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel,
	Jon Lin

The spi which's version is higher than ver 2 will automatically
enable this feature.

If the length of master transmission is uncertain, the RK spi slave
is better to automatically stop after cs inactive instead of waiting
for xfer_completion forever.

Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

 drivers/spi/spi-rockchip.c | 85 ++++++++++++++++++++++++++++++++++----
 1 file changed, 77 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 553b6b9d0222..7ac07569e103 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -133,7 +133,8 @@
 #define INT_TF_OVERFLOW				(1 << 1)
 #define INT_RF_UNDERFLOW			(1 << 2)
 #define INT_RF_OVERFLOW				(1 << 3)
-#define INT_RF_FULL					(1 << 4)
+#define INT_RF_FULL				(1 << 4)
+#define INT_CS_INACTIVE				(1 << 6)
 
 /* Bit fields in ICR, 4bit */
 #define ICR_MASK					0x0f
@@ -194,6 +195,8 @@ struct rockchip_spi {
 	bool cs_asserted[ROCKCHIP_SPI_MAX_CS_NUM];
 
 	bool slave_abort;
+	bool cs_inactive; /* spi slave tansmition stop when cs inactive */
+	struct spi_transfer *xfer; /* Store xfer temporarily */
 };
 
 static inline void spi_enable_chip(struct rockchip_spi *rs, bool enable)
@@ -343,6 +346,15 @@ static irqreturn_t rockchip_spi_isr(int irq, void *dev_id)
 	struct spi_controller *ctlr = dev_id;
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
+	/* When int_cs_inactive comes, spi slave abort */
+	if (rs->cs_inactive && readl_relaxed(rs->regs + ROCKCHIP_SPI_IMR) & INT_CS_INACTIVE) {
+		ctlr->slave_abort(ctlr);
+		writel_relaxed(0, rs->regs + ROCKCHIP_SPI_IMR);
+		writel_relaxed(0xffffffff, rs->regs + ROCKCHIP_SPI_ICR);
+
+		return IRQ_HANDLED;
+	}
+
 	if (rs->tx_left)
 		rockchip_spi_pio_writer(rs);
 
@@ -350,6 +362,7 @@ static irqreturn_t rockchip_spi_isr(int irq, void *dev_id)
 	if (!rs->rx_left) {
 		spi_enable_chip(rs, false);
 		writel_relaxed(0, rs->regs + ROCKCHIP_SPI_IMR);
+		writel_relaxed(0xffffffff, rs->regs + ROCKCHIP_SPI_ICR);
 		spi_finalize_current_transfer(ctlr);
 	}
 
@@ -357,14 +370,18 @@ static irqreturn_t rockchip_spi_isr(int irq, void *dev_id)
 }
 
 static int rockchip_spi_prepare_irq(struct rockchip_spi *rs,
-		struct spi_transfer *xfer)
+				    struct spi_controller *ctlr,
+				    struct spi_transfer *xfer)
 {
 	rs->tx = xfer->tx_buf;
 	rs->rx = xfer->rx_buf;
 	rs->tx_left = rs->tx ? xfer->len / rs->n_bytes : 0;
 	rs->rx_left = xfer->len / rs->n_bytes;
 
-	writel_relaxed(INT_RF_FULL, rs->regs + ROCKCHIP_SPI_IMR);
+	if (rs->cs_inactive)
+		writel_relaxed(INT_RF_FULL | INT_CS_INACTIVE, rs->regs + ROCKCHIP_SPI_IMR);
+	else
+		writel_relaxed(INT_RF_FULL, rs->regs + ROCKCHIP_SPI_IMR);
 	spi_enable_chip(rs, true);
 
 	if (rs->tx_left)
@@ -383,6 +400,9 @@ static void rockchip_spi_dma_rxcb(void *data)
 	if (state & TXDMA && !rs->slave_abort)
 		return;
 
+	if (rs->cs_inactive)
+		writel_relaxed(0, rs->regs + ROCKCHIP_SPI_IMR);
+
 	spi_enable_chip(rs, false);
 	spi_finalize_current_transfer(ctlr);
 }
@@ -423,14 +443,16 @@ static int rockchip_spi_prepare_dma(struct rockchip_spi *rs,
 
 	atomic_set(&rs->state, 0);
 
+	rs->tx = xfer->tx_buf;
+	rs->rx = xfer->rx_buf;
+
 	rxdesc = NULL;
 	if (xfer->rx_buf) {
 		struct dma_slave_config rxconf = {
 			.direction = DMA_DEV_TO_MEM,
 			.src_addr = rs->dma_addr_rx,
 			.src_addr_width = rs->n_bytes,
-			.src_maxburst = rockchip_spi_calc_burst_size(xfer->len /
-								     rs->n_bytes),
+			.src_maxburst = rockchip_spi_calc_burst_size(xfer->len / rs->n_bytes),
 		};
 
 		dmaengine_slave_config(ctlr->dma_rx, &rxconf);
@@ -474,10 +496,13 @@ static int rockchip_spi_prepare_dma(struct rockchip_spi *rs,
 	/* rx must be started before tx due to spi instinct */
 	if (rxdesc) {
 		atomic_or(RXDMA, &rs->state);
-		dmaengine_submit(rxdesc);
+		ctlr->dma_rx->cookie = dmaengine_submit(rxdesc);
 		dma_async_issue_pending(ctlr->dma_rx);
 	}
 
+	if (rs->cs_inactive)
+		writel_relaxed(INT_CS_INACTIVE, rs->regs + ROCKCHIP_SPI_IMR);
+
 	spi_enable_chip(rs, true);
 
 	if (txdesc) {
@@ -584,7 +609,46 @@ static size_t rockchip_spi_max_transfer_size(struct spi_device *spi)
 static int rockchip_spi_slave_abort(struct spi_controller *ctlr)
 {
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
+	u32 rx_fifo_left;
+	struct dma_tx_state state;
+	enum dma_status status;
+
+	/* Get current dma rx point */
+	if (atomic_read(&rs->state) & RXDMA) {
+		dmaengine_pause(ctlr->dma_rx);
+		status = dmaengine_tx_status(ctlr->dma_rx, ctlr->dma_rx->cookie, &state);
+		dmaengine_terminate_sync(ctlr->dma_rx);
+		atomic_set(&rs->state, 0);
+		if (status == DMA_ERROR) {
+			rs->rx = rs->xfer->rx_buf;
+			rs->xfer->len = 0;
+			rx_fifo_left = readl_relaxed(rs->regs + ROCKCHIP_SPI_RXFLR);
+			for (; rx_fifo_left; rx_fifo_left--)
+				readl_relaxed(rs->regs + ROCKCHIP_SPI_RXDR);
+			goto out;
+		} else {
+			rs->rx += rs->xfer->len - rs->n_bytes * state.residue;
+		}
+	}
+
+	/* Get the valid data left in rx fifo and set rs->xfer->len real rx size */
+	if (rs->rx) {
+		rx_fifo_left = readl_relaxed(rs->regs + ROCKCHIP_SPI_RXFLR);
+		for (; rx_fifo_left; rx_fifo_left--) {
+			u32 rxw = readl_relaxed(rs->regs + ROCKCHIP_SPI_RXDR);
+
+			if (rs->n_bytes == 1)
+				*(u8 *)rs->rx = (u8)rxw;
+			else
+				*(u16 *)rs->rx = (u16)rxw;
+			rs->rx += rs->n_bytes;
+		}
 
+		rs->xfer->len = (unsigned int)(rs->rx - rs->xfer->rx_buf);
+	}
+
+out:
+	spi_enable_chip(rs, false);
 	rs->slave_abort = true;
 	spi_finalize_current_transfer(ctlr);
 
@@ -620,7 +684,7 @@ static int rockchip_spi_transfer_one(
 	}
 
 	rs->n_bytes = xfer->bits_per_word <= 8 ? 1 : 2;
-
+	rs->xfer = xfer;
 	use_dma = ctlr->can_dma ? ctlr->can_dma(ctlr, spi, xfer) : false;
 
 	ret = rockchip_spi_config(rs, spi, xfer, use_dma, ctlr->slave);
@@ -630,7 +694,7 @@ static int rockchip_spi_transfer_one(
 	if (use_dma)
 		return rockchip_spi_prepare_dma(rs, ctlr, xfer);
 
-	return rockchip_spi_prepare_irq(rs, xfer);
+	return rockchip_spi_prepare_irq(rs, ctlr, xfer);
 }
 
 static bool rockchip_spi_can_dma(struct spi_controller *ctlr,
@@ -808,8 +872,13 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	switch (readl_relaxed(rs->regs + ROCKCHIP_SPI_VERSION)) {
 	case ROCKCHIP_SPI_VER2_TYPE2:
 		ctlr->mode_bits |= SPI_CS_HIGH;
+		if (ctlr->can_dma && slave_mode)
+			rs->cs_inactive = true;
+		else
+			rs->cs_inactive = false;
 		break;
 	default:
+		rs->cs_inactive = false;
 		break;
 	}
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/6] spi: rockchip: Preset cs-high and clk polarity in setup progress
  2022-02-11  3:43 [PATCH 1/6] spi: rockchip: Stop spi slave dma receiver when cs inactive Jon Lin
@ 2022-02-11  3:43 ` Jon Lin
  2022-02-11 11:24   ` Mark Brown
  2022-02-11  3:43 ` [PATCH 3/6] spi: rockchip: Fix error in getting num-cs property Jon Lin
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Jon Lin @ 2022-02-11  3:43 UTC (permalink / raw)
  To: broonie
  Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel,
	Jon Lin

After power up, the cs and clock is in default status, and the cs-high
and clock polarity dts property configuration will take no effect until
the calling of rockchip_spi_config in the first transmission.
So preset them to make sure a correct voltage before the first
transmission coming.

Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

 drivers/spi/spi-rockchip.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 7ac07569e103..1738a2611a2b 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -711,6 +711,26 @@ static bool rockchip_spi_can_dma(struct spi_controller *ctlr,
 	return xfer->len / bytes_per_word >= rs->fifo_len;
 }
 
+static int rockchip_spi_setup(struct spi_device *spi)
+{
+	struct rockchip_spi *rs = spi_controller_get_devdata(spi->controller);
+	u32 cr0;
+
+	pm_runtime_get_sync(rs->dev);
+
+	cr0 = readl_relaxed(rs->regs + ROCKCHIP_SPI_CTRLR0);
+
+	cr0 |= ((spi->mode & 0x3) << CR0_SCPH_OFFSET);
+	if (spi->mode & SPI_CS_HIGH)
+		cr0 |= BIT(spi->chip_select) << CR0_SOI_OFFSET;
+
+	writel_relaxed(cr0, rs->regs + ROCKCHIP_SPI_CTRLR0);
+
+	pm_runtime_put(rs->dev);
+
+	return 0;
+}
+
 static int rockchip_spi_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -837,6 +857,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	ctlr->min_speed_hz = rs->freq / BAUDR_SCKDV_MAX;
 	ctlr->max_speed_hz = min(rs->freq / BAUDR_SCKDV_MIN, MAX_SCLK_OUT);
 
+	ctlr->setup = rockchip_spi_setup;
 	ctlr->set_cs = rockchip_spi_set_cs;
 	ctlr->transfer_one = rockchip_spi_transfer_one;
 	ctlr->max_transfer_size = rockchip_spi_max_transfer_size;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/6] spi: rockchip: Fix error in getting num-cs property
  2022-02-11  3:43 [PATCH 1/6] spi: rockchip: Stop spi slave dma receiver when cs inactive Jon Lin
  2022-02-11  3:43 ` [PATCH 2/6] spi: rockchip: Preset cs-high and clk polarity in setup progress Jon Lin
@ 2022-02-11  3:43 ` Jon Lin
  2022-02-11 11:25   ` Mark Brown
  2022-02-11  3:43 ` [PATCH 4/6] spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops Jon Lin
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Jon Lin @ 2022-02-11  3:43 UTC (permalink / raw)
  To: broonie
  Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel,
	Jon Lin

Get num-cs u32 from dts of_node property rather than u16.

Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

 drivers/spi/spi-rockchip.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 1738a2611a2b..5f6a70664871 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -738,7 +738,7 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 	struct spi_controller *ctlr;
 	struct resource *mem;
 	struct device_node *np = pdev->dev.of_node;
-	u32 rsd_nsecs;
+	u32 rsd_nsecs, num_cs;
 	bool slave_mode;
 
 	slave_mode = of_property_read_bool(np, "spi-slave");
@@ -848,8 +848,9 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 		 * rk spi0 has two native cs, spi1..5 one cs only
 		 * if num-cs is missing in the dts, default to 1
 		 */
-		if (of_property_read_u16(np, "num-cs", &ctlr->num_chipselect))
-			ctlr->num_chipselect = 1;
+		if (of_property_read_u32(np, "num-cs", &num_cs))
+			num_cs = 1;
+		ctlr->num_chipselect = num_cs;
 		ctlr->use_gpio_descriptors = true;
 	}
 	ctlr->dev.of_node = pdev->dev.of_node;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/6] spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops
  2022-02-11  3:43 [PATCH 1/6] spi: rockchip: Stop spi slave dma receiver when cs inactive Jon Lin
  2022-02-11  3:43 ` [PATCH 2/6] spi: rockchip: Preset cs-high and clk polarity in setup progress Jon Lin
  2022-02-11  3:43 ` [PATCH 3/6] spi: rockchip: Fix error in getting num-cs property Jon Lin
@ 2022-02-11  3:43 ` Jon Lin
  2022-02-11  3:43 ` [PATCH v10 5/6] spi: rockchip: Support cs-gpio Jon Lin
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Jon Lin @ 2022-02-11  3:43 UTC (permalink / raw)
  To: broonie
  Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel,
	shengfei Xu, Jon Lin

From: shengfei Xu <xsf@rock-chips.com>

the wakeup interrupt handler which is guaranteed not to run while
@resume noirq() is being executed. the patch can help to avoid the
wakeup source try to access spi when the spi is in suspend mode.

Signed-off-by: shengfei Xu <xsf@rock-chips.com>
Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

 drivers/spi/spi-rockchip.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 5f6a70664871..5457af616a44 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -959,14 +959,14 @@ static int rockchip_spi_suspend(struct device *dev)
 {
 	int ret;
 	struct spi_controller *ctlr = dev_get_drvdata(dev);
+	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
 
 	ret = spi_controller_suspend(ctlr);
 	if (ret < 0)
 		return ret;
 
-	ret = pm_runtime_force_suspend(dev);
-	if (ret < 0)
-		return ret;
+	clk_disable_unprepare(rs->spiclk);
+	clk_disable_unprepare(rs->apb_pclk);
 
 	pinctrl_pm_select_sleep_state(dev);
 
@@ -981,10 +981,14 @@ static int rockchip_spi_resume(struct device *dev)
 
 	pinctrl_pm_select_default_state(dev);
 
-	ret = pm_runtime_force_resume(dev);
+	ret = clk_prepare_enable(rs->apb_pclk);
 	if (ret < 0)
 		return ret;
 
+	ret = clk_prepare_enable(rs->spiclk);
+	if (ret < 0)
+		clk_disable_unprepare(rs->apb_pclk);
+
 	ret = spi_controller_resume(ctlr);
 	if (ret < 0) {
 		clk_disable_unprepare(rs->spiclk);
@@ -1026,7 +1030,7 @@ static int rockchip_spi_runtime_resume(struct device *dev)
 #endif /* CONFIG_PM */
 
 static const struct dev_pm_ops rockchip_spi_pm = {
-	SET_SYSTEM_SLEEP_PM_OPS(rockchip_spi_suspend, rockchip_spi_resume)
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rockchip_spi_suspend, rockchip_spi_resume)
 	SET_RUNTIME_PM_OPS(rockchip_spi_runtime_suspend,
 			   rockchip_spi_runtime_resume, NULL)
 };
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v10 5/6] spi: rockchip: Support cs-gpio
  2022-02-11  3:43 [PATCH 1/6] spi: rockchip: Stop spi slave dma receiver when cs inactive Jon Lin
                   ` (2 preceding siblings ...)
  2022-02-11  3:43 ` [PATCH 4/6] spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops Jon Lin
@ 2022-02-11  3:43 ` Jon Lin
  2022-02-11 11:48   ` Mark Brown
  2022-02-11  3:43 ` [PATCH 5/6] spi: rockchip: terminate dma transmission when slave abort Jon Lin
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Jon Lin @ 2022-02-11  3:43 UTC (permalink / raw)
  To: broonie
  Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel,
	Jon Lin

1.Add standard cs-gpio support
2.Refer to spi-controller.yaml for details

Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None

 drivers/spi/spi-rockchip.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 950d3bce443b..fbd750b1d28e 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -157,7 +157,8 @@
  */
 #define ROCKCHIP_SPI_MAX_TRANLEN		0xffff
 
-#define ROCKCHIP_SPI_MAX_CS_NUM			2
+/* 2 for native cs, 2 for cs-gpio */
+#define ROCKCHIP_SPI_MAX_CS_NUM			4
 #define ROCKCHIP_SPI_VER2_TYPE1			0x05EC0002
 #define ROCKCHIP_SPI_VER2_TYPE2			0x00110002
 
@@ -245,11 +246,15 @@ static void rockchip_spi_set_cs(struct spi_device *spi, bool enable)
 		/* Keep things powered as long as CS is asserted */
 		pm_runtime_get_sync(rs->dev);
 
-		ROCKCHIP_SPI_SET_BITS(rs->regs + ROCKCHIP_SPI_SER,
-				      BIT(spi->chip_select));
+		if (spi->cs_gpiod)
+			ROCKCHIP_SPI_SET_BITS(rs->regs + ROCKCHIP_SPI_SER, 1);
+		else
+			ROCKCHIP_SPI_SET_BITS(rs->regs + ROCKCHIP_SPI_SER, BIT(spi->chip_select));
 	} else {
-		ROCKCHIP_SPI_CLR_BITS(rs->regs + ROCKCHIP_SPI_SER,
-				      BIT(spi->chip_select));
+		if (spi->cs_gpiod)
+			ROCKCHIP_SPI_CLR_BITS(rs->regs + ROCKCHIP_SPI_SER, 1);
+		else
+			ROCKCHIP_SPI_CLR_BITS(rs->regs + ROCKCHIP_SPI_SER, BIT(spi->chip_select));
 
 		/* Drop reference from when we first asserted CS */
 		pm_runtime_put(rs->dev);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 5/6] spi: rockchip: terminate dma transmission when slave abort
  2022-02-11  3:43 [PATCH 1/6] spi: rockchip: Stop spi slave dma receiver when cs inactive Jon Lin
                   ` (3 preceding siblings ...)
  2022-02-11  3:43 ` [PATCH v10 5/6] spi: rockchip: Support cs-gpio Jon Lin
@ 2022-02-11  3:43 ` Jon Lin
  2022-02-11 11:49   ` Mark Brown
  2022-02-11  3:43 ` [PATCH 6/6] spi: rockchip: clear interrupt status in error handler Jon Lin
  2022-02-11  3:43 ` [PATCH v10 6/6] spi: rockchip: Support SPI_CS_HIGH Jon Lin
  6 siblings, 1 reply; 16+ messages in thread
From: Jon Lin @ 2022-02-11  3:43 UTC (permalink / raw)
  To: broonie
  Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel,
	Jon Lin

After slave abort, all DMA should be stopped, or it will affect the
next transmission:

[   31.693877] Unable to handle kernel paging request at virtual address ffffff8105a2a7c0
[   31.694643] Mem abort info:
[   31.694898]   ESR = 0x96000045
[   31.695179]   EC = 0x25: DABT (current EL), IL = 32 bits
[   31.695653]   SET = 0, FnV = 0
[   31.695931]   EA = 0, S1PTW = 0
[   31.696218] Data abort info:
[   31.696485]   ISV = 0, ISS = 0x00000045
[   31.696832]   CM = 0, WnR = 1
[   31.697112] swapper pgtable: 4k pages, 39-bit VAs, pgdp=000000000142f000
[   31.697713] [ffffff8105a2a7c0] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
[   31.698502] Internal error: Oops: 96000045 [#1] SMP
[   31.698943] Modules linked in:
[   31.699235] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G        W         5.10.43 #8
[   31.699895] Hardware name: Rockchip RK3588 EVB1 LP4 V10 Board (DT)
[   31.700455] pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
[   31.701000] pc : rockchip_spi_slave_abort+0x150/0x18c
[   31.701456] lr : rockchip_spi_slave_abort+0x78/0x18c

Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

 drivers/spi/spi-rockchip.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 5457af616a44..3d4e95eda4a6 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -617,8 +617,6 @@ static int rockchip_spi_slave_abort(struct spi_controller *ctlr)
 	if (atomic_read(&rs->state) & RXDMA) {
 		dmaengine_pause(ctlr->dma_rx);
 		status = dmaengine_tx_status(ctlr->dma_rx, ctlr->dma_rx->cookie, &state);
-		dmaengine_terminate_sync(ctlr->dma_rx);
-		atomic_set(&rs->state, 0);
 		if (status == DMA_ERROR) {
 			rs->rx = rs->xfer->rx_buf;
 			rs->xfer->len = 0;
@@ -648,6 +646,11 @@ static int rockchip_spi_slave_abort(struct spi_controller *ctlr)
 	}
 
 out:
+	if (atomic_read(&rs->state) & RXDMA)
+		dmaengine_terminate_sync(ctlr->dma_rx);
+	if (atomic_read(&rs->state) & TXDMA)
+		dmaengine_terminate_sync(ctlr->dma_tx);
+	atomic_set(&rs->state, 0);
 	spi_enable_chip(rs, false);
 	rs->slave_abort = true;
 	spi_finalize_current_transfer(ctlr);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 6/6] spi: rockchip: clear interrupt status in error handler
  2022-02-11  3:43 [PATCH 1/6] spi: rockchip: Stop spi slave dma receiver when cs inactive Jon Lin
                   ` (4 preceding siblings ...)
  2022-02-11  3:43 ` [PATCH 5/6] spi: rockchip: terminate dma transmission when slave abort Jon Lin
@ 2022-02-11  3:43 ` Jon Lin
  2022-02-11  3:43 ` [PATCH v10 6/6] spi: rockchip: Support SPI_CS_HIGH Jon Lin
  6 siblings, 0 replies; 16+ messages in thread
From: Jon Lin @ 2022-02-11  3:43 UTC (permalink / raw)
  To: broonie
  Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel,
	Jon Lin

The interrupt status bit of the previous error data transmition will
affect the next operation and cause continuous SPI transmission failure.

Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

 drivers/spi/spi-rockchip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 3d4e95eda4a6..bc547b79a2a1 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -278,8 +278,9 @@ static void rockchip_spi_handle_err(struct spi_controller *ctlr,
 	 */
 	spi_enable_chip(rs, false);
 
-	/* make sure all interrupts are masked */
+	/* make sure all interrupts are masked and status cleared */
 	writel_relaxed(0, rs->regs + ROCKCHIP_SPI_IMR);
+	writel_relaxed(0xffffffff, rs->regs + ROCKCHIP_SPI_ICR);
 
 	if (atomic_read(&rs->state) & TXDMA)
 		dmaengine_terminate_async(ctlr->dma_tx);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v10 6/6] spi: rockchip: Support SPI_CS_HIGH
  2022-02-11  3:43 [PATCH 1/6] spi: rockchip: Stop spi slave dma receiver when cs inactive Jon Lin
                   ` (5 preceding siblings ...)
  2022-02-11  3:43 ` [PATCH 6/6] spi: rockchip: clear interrupt status in error handler Jon Lin
@ 2022-02-11  3:43 ` Jon Lin
  6 siblings, 0 replies; 16+ messages in thread
From: Jon Lin @ 2022-02-11  3:43 UTC (permalink / raw)
  To: broonie
  Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel,
	Jon Lin

1.Add standard spi-cs-high support
2.Refer to spi-controller.yaml for details

Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None

 drivers/spi/spi-rockchip.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index fbd750b1d28e..540861ca2ba3 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -107,6 +107,8 @@
 #define CR0_OPM_MASTER				0x0
 #define CR0_OPM_SLAVE				0x1
 
+#define CR0_SOI_OFFSET				23
+
 #define CR0_MTM_OFFSET				0x21
 
 /* Bit fields in SER, 2bit */
@@ -236,7 +238,7 @@ static void rockchip_spi_set_cs(struct spi_device *spi, bool enable)
 {
 	struct spi_controller *ctlr = spi->controller;
 	struct rockchip_spi *rs = spi_controller_get_devdata(ctlr);
-	bool cs_asserted = !enable;
+	bool cs_asserted = spi->mode & SPI_CS_HIGH ? enable : !enable;
 
 	/* Return immediately for no-op */
 	if (cs_asserted == rs->cs_asserted[spi->chip_select])
@@ -507,6 +509,8 @@ static int rockchip_spi_config(struct rockchip_spi *rs,
 	cr0 |= (spi->mode & 0x3U) << CR0_SCPH_OFFSET;
 	if (spi->mode & SPI_LSB_FIRST)
 		cr0 |= CR0_FBM_LSB << CR0_FBM_OFFSET;
+	if (spi->mode & SPI_CS_HIGH)
+		cr0 |= BIT(spi->chip_select) << CR0_SOI_OFFSET;
 
 	if (xfer->rx_buf && xfer->tx_buf)
 		cr0 |= CR0_XFM_TR << CR0_XFM_OFFSET;
@@ -795,6 +799,14 @@ static int rockchip_spi_probe(struct platform_device *pdev)
 		ctlr->can_dma = rockchip_spi_can_dma;
 	}
 
+	switch (readl_relaxed(rs->regs + ROCKCHIP_SPI_VERSION)) {
+	case ROCKCHIP_SPI_VER2_TYPE2:
+		ctlr->mode_bits |= SPI_CS_HIGH;
+		break;
+	default:
+		break;
+	}
+
 	ret = devm_spi_register_controller(&pdev->dev, ctlr);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to register controller\n");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6] spi: rockchip: Preset cs-high and clk polarity in setup progress
  2022-02-11  3:43 ` [PATCH 2/6] spi: rockchip: Preset cs-high and clk polarity in setup progress Jon Lin
@ 2022-02-11 11:24   ` Mark Brown
       [not found]     ` <4222ce7d-a1e3-1728-fec2-976946b06ba9@rock-chips.com>
  0 siblings, 1 reply; 16+ messages in thread
From: Mark Brown @ 2022-02-11 11:24 UTC (permalink / raw)
  To: Jon Lin; +Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On Fri, Feb 11, 2022 at 11:43:38AM +0800, Jon Lin wrote:

> +static int rockchip_spi_setup(struct spi_device *spi)
> +{
> +	struct rockchip_spi *rs = spi_controller_get_devdata(spi->controller);
> +	u32 cr0;
> +
> +	pm_runtime_get_sync(rs->dev);
> +
> +	cr0 = readl_relaxed(rs->regs + ROCKCHIP_SPI_CTRLR0);
> +
> +	cr0 |= ((spi->mode & 0x3) << CR0_SCPH_OFFSET);
> +	if (spi->mode & SPI_CS_HIGH)
> +		cr0 |= BIT(spi->chip_select) << CR0_SOI_OFFSET;

What ensures that this read/modify/write doesn't race with a transfer
running on another client device in the case where the controller has
more than one device connected?  Similarly with the mode, though it's
not great to have devices with different modes connected to a single
controller.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/6] spi: rockchip: Fix error in getting num-cs property
  2022-02-11  3:43 ` [PATCH 3/6] spi: rockchip: Fix error in getting num-cs property Jon Lin
@ 2022-02-11 11:25   ` Mark Brown
  0 siblings, 0 replies; 16+ messages in thread
From: Mark Brown @ 2022-02-11 11:25 UTC (permalink / raw)
  To: Jon Lin; +Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 231 bytes --]

On Fri, Feb 11, 2022 at 11:43:39AM +0800, Jon Lin wrote:
> Get num-cs u32 from dts of_node property rather than u16.

Bug fixes should go at the start of a series so they can be sent as
fixes without dependencies on anything else.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v10 5/6] spi: rockchip: Support cs-gpio
  2022-02-11  3:43 ` [PATCH v10 5/6] spi: rockchip: Support cs-gpio Jon Lin
@ 2022-02-11 11:48   ` Mark Brown
  0 siblings, 0 replies; 16+ messages in thread
From: Mark Brown @ 2022-02-11 11:48 UTC (permalink / raw)
  To: Jon Lin; +Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

On Fri, Feb 11, 2022 at 11:43:41AM +0800, Jon Lin wrote:
> 1.Add standard cs-gpio support
> 2.Refer to spi-controller.yaml for details
> 
> Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
> ---
> 
> Changes in v10: None
> Changes in v9: None
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None

Why is this the one patch in the series with any versioning information?

> -		ROCKCHIP_SPI_SET_BITS(rs->regs + ROCKCHIP_SPI_SER,
> -				      BIT(spi->chip_select));
> +		if (spi->cs_gpiod)
> +			ROCKCHIP_SPI_SET_BITS(rs->regs + ROCKCHIP_SPI_SER, 1);
> +		else
> +			ROCKCHIP_SPI_SET_BITS(rs->regs + ROCKCHIP_SPI_SER, BIT(spi->chip_select));

This appears to be making the device control chip select 0 if a GPIO
chip select is used - that's going to work poorly if there's a device
using that chip select.  It should be fine to prohibit that
configuration if the hardware requires that a GPIO be controlled,
especially if the native chip select can be pinmuxed to a GPIO, but it
ought to be at least documented that this won't work and ideally
detected.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 5/6] spi: rockchip: terminate dma transmission when slave abort
  2022-02-11  3:43 ` [PATCH 5/6] spi: rockchip: terminate dma transmission when slave abort Jon Lin
@ 2022-02-11 11:49   ` Mark Brown
  0 siblings, 0 replies; 16+ messages in thread
From: Mark Brown @ 2022-02-11 11:49 UTC (permalink / raw)
  To: Jon Lin; +Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

On Fri, Feb 11, 2022 at 11:43:42AM +0800, Jon Lin wrote:

> After slave abort, all DMA should be stopped, or it will affect the
> next transmission:

Again, this is a fix and should be at the start of the series.

> 
> [   31.693877] Unable to handle kernel paging request at virtual address ffffff8105a2a7c0
> [   31.694643] Mem abort info:
> [   31.694898]   ESR = 0x96000045
> [   31.695179]   EC = 0x25: DABT (current EL), IL = 32 bits
> [   31.695653]   SET = 0, FnV = 0
> [   31.695931]   EA = 0, S1PTW = 0

Please think hard before including complete backtraces in upstream
reports, they are very large and contain almost no useful information
relative to their size so often obscure the relevant content in your
message. If part of the backtrace is usefully illustrative (it often is
for search engines if nothing else) then it's usually better to pull out
the relevant sections.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6] spi: rockchip: Preset cs-high and clk polarity in setup progress
       [not found]     ` <4222ce7d-a1e3-1728-fec2-976946b06ba9@rock-chips.com>
@ 2022-02-14 12:49       ` Mark Brown
       [not found]         ` <e0f0ca0d-40df-cf86-9471-9272bcc171f9@rock-chips.com>
  0 siblings, 1 reply; 16+ messages in thread
From: Mark Brown @ 2022-02-14 12:49 UTC (permalink / raw)
  To: Jon Lin; +Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On Mon, Feb 14, 2022 at 04:40:19PM +0800, Jon Lin wrote:
> 在 2022/2/11 19:24, Mark Brown 写道:

> > > +   cr0 |= ((spi->mode & 0x3) << CR0_SCPH_OFFSET);
> > > +   if (spi->mode & SPI_CS_HIGH)
> > > +           cr0 |= BIT(spi->chip_select) << CR0_SOI_OFFSET;

> > What ensures that this read/modify/write doesn't race with a transfer
> > running on another client device in the case where the controller has
> > more than one device connected?  Similarly with the mode, though it's
> > not great to have devices with different modes connected to a single
> > controller.

> I have no idea how to deal with the conflict configuration between
> different cs, and also I find nothing strategy in others spi drivers.
> As we all know, some configurations should be consistent for different
> CS devices, such as SPI_CPOL, so I suggest the framework to make
> corresponding early warning prompts.

As covered in the documentation setup() for one device may run while
another is active, therefore if multiple devices are configured in the
same register you should use a lock to ensure there can't be multiple
writes.  Note that the above appears to not just be setting the mode but
also the chip select so if you've got two SPI_CS_HIGH devices then
they'll both be going in and separately setting cr0.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6] spi: rockchip: Preset cs-high and clk polarity in setup progress
       [not found]         ` <e0f0ca0d-40df-cf86-9471-9272bcc171f9@rock-chips.com>
@ 2022-02-15 12:36           ` Mark Brown
  2022-02-16  1:23             ` Jon Lin
  0 siblings, 1 reply; 16+ messages in thread
From: Mark Brown @ 2022-02-15 12:36 UTC (permalink / raw)
  To: Jon Lin; +Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 838 bytes --]

On Tue, Feb 15, 2022 at 11:00:54AM +0800, Jon Lin wrote:
> 在 2022/2/14 20:49, Mark Brown 写道:

> > As covered in the documentation setup() for one device may run while
> > another is active, therefore if multiple devices are configured in the
> > same register you should use a lock to ensure there can't be multiple
> > writes.  Note that the above appears to not just be setting the mode but
> > also the chip select so if you've got two SPI_CS_HIGH devices then
> > they'll both be going in and separately setting cr0.

> Is the io_mutex in function spi_setup is good enough?

It's not supposed to be for that but looking at the code quickly I
*think* setup() is never called with io_mutex held so it might well be
fine - you should double check though.  If not you'd need to add another
lock in your driver data.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/6] spi: rockchip: Preset cs-high and clk polarity in setup progress
  2022-02-15 12:36           ` Mark Brown
@ 2022-02-16  1:23             ` Jon Lin
  0 siblings, 0 replies; 16+ messages in thread
From: Jon Lin @ 2022-02-16  1:23 UTC (permalink / raw)
  To: Mark Brown
  Cc: heiko, linux-spi, linux-arm-kernel, linux-rockchip, linux-kernel



在 2022/2/15 20:36, Mark Brown 写道:
> On Tue, Feb 15, 2022 at 11:00:54AM +0800, Jon Lin wrote:
>> 在 2022/2/14 20:49, Mark Brown 写道:
> 
>>> As covered in the documentation setup() for one device may run while
>>> another is active, therefore if multiple devices are configured in the
>>> same register you should use a lock to ensure there can't be multiple
>>> writes.  Note that the above appears to not just be setting the mode but
>>> also the chip select so if you've got two SPI_CS_HIGH devices then
>>> they'll both be going in and separately setting cr0.
> 
>> Is the io_mutex in function spi_setup is good enough?
> 
> It's not supposed to be for that but looking at the code quickly I
> *think* setup() is never called with io_mutex held so it might well be
> fine - you should double check though.  If not you'd need to add another
> lock in your driver data.

》setup() is never called with io_mutex held
I think so. and I think the io_mutex is enough for me.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v10 5/6] spi: rockchip: Support cs-gpio
  2021-06-21 10:47 [PATCH v10 0/6] Support ROCKCHIP SPI new feature Jon Lin
@ 2021-06-21 10:48 ` Jon Lin
  0 siblings, 0 replies; 16+ messages in thread
From: Jon Lin @ 2021-06-21 10:48 UTC (permalink / raw)
  To: broonie
  Cc: jon.lin, heiko, robh+dt, linux-spi, linux-arm-kernel,
	linux-rockchip, linux-kernel, devicetree

1.Add standard cs-gpio support
2.Refer to spi-controller.yaml for details

Signed-off-by: Jon Lin <jon.lin@rock-chips.com>
---

Changes in v10: None
Changes in v9: None
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None

 drivers/spi/spi-rockchip.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c
index 950d3bce443b..fbd750b1d28e 100644
--- a/drivers/spi/spi-rockchip.c
+++ b/drivers/spi/spi-rockchip.c
@@ -157,7 +157,8 @@
  */
 #define ROCKCHIP_SPI_MAX_TRANLEN		0xffff
 
-#define ROCKCHIP_SPI_MAX_CS_NUM			2
+/* 2 for native cs, 2 for cs-gpio */
+#define ROCKCHIP_SPI_MAX_CS_NUM			4
 #define ROCKCHIP_SPI_VER2_TYPE1			0x05EC0002
 #define ROCKCHIP_SPI_VER2_TYPE2			0x00110002
 
@@ -245,11 +246,15 @@ static void rockchip_spi_set_cs(struct spi_device *spi, bool enable)
 		/* Keep things powered as long as CS is asserted */
 		pm_runtime_get_sync(rs->dev);
 
-		ROCKCHIP_SPI_SET_BITS(rs->regs + ROCKCHIP_SPI_SER,
-				      BIT(spi->chip_select));
+		if (spi->cs_gpiod)
+			ROCKCHIP_SPI_SET_BITS(rs->regs + ROCKCHIP_SPI_SER, 1);
+		else
+			ROCKCHIP_SPI_SET_BITS(rs->regs + ROCKCHIP_SPI_SER, BIT(spi->chip_select));
 	} else {
-		ROCKCHIP_SPI_CLR_BITS(rs->regs + ROCKCHIP_SPI_SER,
-				      BIT(spi->chip_select));
+		if (spi->cs_gpiod)
+			ROCKCHIP_SPI_CLR_BITS(rs->regs + ROCKCHIP_SPI_SER, 1);
+		else
+			ROCKCHIP_SPI_CLR_BITS(rs->regs + ROCKCHIP_SPI_SER, BIT(spi->chip_select));
 
 		/* Drop reference from when we first asserted CS */
 		pm_runtime_put(rs->dev);
-- 
2.17.1




^ permalink raw reply related	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2022-02-16  1:24 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-11  3:43 [PATCH 1/6] spi: rockchip: Stop spi slave dma receiver when cs inactive Jon Lin
2022-02-11  3:43 ` [PATCH 2/6] spi: rockchip: Preset cs-high and clk polarity in setup progress Jon Lin
2022-02-11 11:24   ` Mark Brown
     [not found]     ` <4222ce7d-a1e3-1728-fec2-976946b06ba9@rock-chips.com>
2022-02-14 12:49       ` Mark Brown
     [not found]         ` <e0f0ca0d-40df-cf86-9471-9272bcc171f9@rock-chips.com>
2022-02-15 12:36           ` Mark Brown
2022-02-16  1:23             ` Jon Lin
2022-02-11  3:43 ` [PATCH 3/6] spi: rockchip: Fix error in getting num-cs property Jon Lin
2022-02-11 11:25   ` Mark Brown
2022-02-11  3:43 ` [PATCH 4/6] spi: rockchip: Suspend and resume the bus during NOIRQ_SYSTEM_SLEEP_PM ops Jon Lin
2022-02-11  3:43 ` [PATCH v10 5/6] spi: rockchip: Support cs-gpio Jon Lin
2022-02-11 11:48   ` Mark Brown
2022-02-11  3:43 ` [PATCH 5/6] spi: rockchip: terminate dma transmission when slave abort Jon Lin
2022-02-11 11:49   ` Mark Brown
2022-02-11  3:43 ` [PATCH 6/6] spi: rockchip: clear interrupt status in error handler Jon Lin
2022-02-11  3:43 ` [PATCH v10 6/6] spi: rockchip: Support SPI_CS_HIGH Jon Lin
  -- strict thread matches above, loose matches on Subject: below --
2021-06-21 10:47 [PATCH v10 0/6] Support ROCKCHIP SPI new feature Jon Lin
2021-06-21 10:48 ` [PATCH v10 5/6] spi: rockchip: Support cs-gpio Jon Lin

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).