driver: add oob support for spi-rockchip and pl330 by StevenFryto · Pull Request #2 · EOS-team/radxa-kernel

StevenFryto · 2025-04-10T08:35:41Z

This patch introduces out-of-band (OOB) pulse transmission support for the spi-rockchip driver and implements OOB mode for the pl330 DMA controller.
The implementation has been verified using the oob-spi example provided in libevl.

However, there are still several areas that require further improvement:
1.Code cleanup and addition of corresponding Kconfig entries.
2.The flag value should be cleared when recycling DMA descriptors, to prevent in-band SPI from accidentally using descriptors marked with out-of-band flags.
3.The granularity of dma_pulse_oob can be further refined.
4.The logic for enabling the chip select signal may need additional improvements.
5.The accuracy of rockchip_spi_pulse_oob_transfer still needs further evaluation.

…nterrupt from oob to inband stage

…fer continuously

ruiqurm · 2025-04-10T11:47:49Z

drivers/dma/pl330.c


 	/* To protect channel manipulation */
-	spinlock_t lock;
+	hard_spinlock_t oob_lock;


Since the lock is not only used in oob-mode, should we change its name with prefix "oob_"?

ruiqurm

Hi. Thanks your contribution!
Overall, the basic logic of the code is sound. It's clear that you've put a great deal of thought into it, and the implementation of the core functions is quite clear, which is already good.
However, there are perhaps some minor details that could use a bit more polishing. I have left some comments, where you may get the idea.

ruiqurm · 2025-04-10T13:04:58Z

drivers/dma/pl330.c

+		ret = 2;
+	}
+
+	if (!running_oob()) {


Should we handle the callback of descriptor that marked as DMA_OOB_INTERRUPT directly here?

All descriptors that marked as DMA_OOB_INTERRUPT have been handled during the oob stage. I think we only need to maintain the origin logic of the driver in the inband stage.

ruiqurm · 2025-04-10T13:10:23Z

drivers/dma/pl330.c


+	unsigned long mask = val;
+
+	// printk("pl330->pcfg.num_events = %d\n", pl330->pcfg.num_events);


Mayve we can handle the events only when running oob, so that you do not have to store the pending state?

IMO the pending state is necessary because it is used to store and forward the event which should be handled in the inband stage. When running oob, we only need to handle the desc which is marked as DMA_OOB_INTERRUPT and DMA_OOB_PULSED , the other should be forward to inband.

drivers/dma/pl330.c

ruiqurm · 2025-04-10T14:28:31Z

drivers/dma/pl330.c

-	if (pl330_update(data))
+	pr_info("------------------%s:%d------------------\n", __func__, __LINE__);
+	int state = pl330_update(data);
+	if (state == 1)


Maybe we could use other more readbale macro instead of some literals?

ok, i will delete these unnecessary dbg info later.

I mean the state == 2 is ambiguous if someone not read through your code.

ruiqurm · 2025-04-10T14:34:20Z

drivers/dma/pl330.c

+	raw_spin_lock(&pch->thread->dmac->oob_lock);
+	// TODO:
+	dev_info(pch->dmac->ddma.dev, "%s:%d\n", __func__, __LINE__);
+	pl330_start_thread(pch->thread);


Could we use _trigger here? Since we can not afford to reset the dma on runtime, can we just panic or if there is some other unwind methods?

I think using _trigger here is better and i will try later. For the second question, i notice that __trigger always returns True, maybe we can only judge the state of the dmac and dma channel by reading DS and ES registers.

ruiqurm · 2025-04-12T07:26:33Z

drivers/spi/spi-rockchip.c

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


IMO it is wired here. if we want to start the transfer, we should guarantee the cs is active. If not, maybe we should return some error code and some debug information.

cs_inactive is only used in SPI slave mode. Since in the current EVL implementation, the out-of-band feature of the SPI controller is only effective in master mode, the use of cs_inactive here is indeed redundant. I will remove it.

ruiqurm · 2025-04-12T07:27:28Z

drivers/spi/spi-rockchip.c

+	 spi_enable_chip(rs, false);
+
+	struct spi_device *spi = xfer->spi;
+	if (spi_get_csgpiod(spi, 0))


Same as above.

ruiqurm · 2025-04-12T07:28:15Z

drivers/spi/spi-rockchip.c

+	rockchip_spi_oob_config(rs, spi, xfer, ctlr->slave_abort);
+
+	// TODO: CS operation is more complex and need more design
+	if (spi_get_csgpiod(spi, 0))


Do we need to support cs-gpio here?

ruiqurm · 2025-04-12T07:34:10Z

drivers/spi/spi-rockchip.c

+	 */
+	if (rs->high_speed_state) {
+		if (rs->freq > IO_DRIVER_4MA_MAX_SCLK_OUT)
+			pinctrl_select_state(rs->dev->pins->p,


Note that pinctrl_select_state might utilize an inband lock in this context, as can be seen in the pinctrl_commit_state function. However, typically this function is called only once. As a result, the likelihood of a priority inversion incident occurring is quite low.

ruiqurm · 2025-04-12T07:39:54Z

drivers/spi/spi-rockchip.c

+	 * If speed is larger than IO_DRIVER_4MA_MAX_SCLK_OUT,
+	 * set higher driver strength.
+	 */
+	if (rs->high_speed_state) {


A majority of the current configuration is for general setup purposes. Is it possible to make certain assumptions and simplify the function?
For instance, does our use case involve the high_speed_state? If it does, we need to retain this part. If not, can we put it in our prepare function?

Since the current EVL out-of-band SPI framework supports full-duplex out-of-band communication only when the SPI controller is in master mode, I think we should remove configurations related to slave mode. Also, since DMA is mandatory, we should enable the relevant DMA settings by default. As for other configurations that do not conflict with these two points, we might as well keep them for now to make the most of the SPI controller’s capabilities.
There’s no need to place high_speed_state in the prepare function. The start function is also executed in-band, so it won’t affect real-time performance.

1. change pl330_dmac.pool_lock from hard_spinlock to spinlock, because it will never be used OOB; 2. change pl330_dmac.oob_lock and dma_pl330_chan.oob_lock from hard_spinlock to hybrid_spinlock; 3. fix the bug that desc's oob flag will influence inband spi transfer; 4. replace `pl330_start_thread` with `_trigger` in pl330_pulse_oob; 5. add marco to clarify the result of `pl330_update`.

StevenFryto added 7 commits April 10, 2025 08:15

add oob logic for spi-rockchip

db0663b

change &ctlr->dev in dev_info to cs->dev

e24ce95

add initual oob operation for pl330

b99465f

add oob irq handler for pl330

adf081f

fix bitmap clear error, now we can successfully migrate non-oob dma i…

4ef2c26

…nterrupt from oob to inband stage

add CS logic for spi-rockchip and oob spi work

f75b04c

fix return logic in __trigger so that we can manually pulse oob-trans…

9b8ec30

…fer continuously

StevenFryto changed the base branch from linux-6.1-stan-rkr4.1 to linux-evl-6.1-stan-rkr4.1 April 10, 2025 08:37

StevenFryto assigned StevenFryto and ruiqurm and unassigned StevenFryto Apr 10, 2025

ruiqurm reviewed Apr 10, 2025

View reviewed changes

ruiqurm reviewed Apr 12, 2025

View reviewed changes

StevenFryto added 3 commits April 22, 2025 17:30

add kconfig for oob spi-rockchip and oob pl330

f2da8bd

spi: add pm_runtime_* when chip select

08a821b


		unsigned long mask = val;

		// printk("pl330->pcfg.num_events = %d\n", pl330->pcfg.num_events);

Comments

Conversation

StevenFryto commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruiqurm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StevenFryto May 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

StevenFryto commented Apr 10, 2025 •

edited

Loading

StevenFryto May 4, 2025 •

edited

Loading