SPI:phytium:update phytium SPI controller driver support to 6.6.0.4#1685
Open
xiaqian1486 wants to merge 12 commits into
Open
SPI:phytium:update phytium SPI controller driver support to 6.6.0.4#1685xiaqian1486 wants to merge 12 commits into
xiaqian1486 wants to merge 12 commits into
Conversation
Reviewer's GuideUpdates the Phytium SPI controller drivers to a newer hardware/firmware spec: tightens timeouts and interrupt handling to fix hung_task issues, adds a new generic full‑duplex transfer path and DDR-high addressing based on a regfile version register, switches DMA and DDR features to be probed from hardware instead of CPU ID, adds ACPI FixedDMA-aware DMA setup, and cleans up debug-log and GPIO/CS handling. Sequence diagram for updated Phytium SPI V2 full‑duplex transfer and completionsequenceDiagram
participant SpiCore as spi_master
participant PhyDrv as spi_phyt_transfer_one
participant Xfer as spi_phytium_xfer
participant Ctlr as spi_phytium_set
participant Wait as spi_phytium_check_result
participant IRQ as spi_phyt_irq
SpiCore->>PhyDrv: spi_phyt_transfer_one(master, spi, transfer, mem)
PhyDrv->>PhyDrv: set fts->tx, fts->rx, fts->len
PhyDrv->>PhyDrv: check fts->half_duplex
alt tx and rx and !half_duplex
PhyDrv->>Xfer: spi_phytium_xfer(fts, chip_select, bits_per_word, mode, tmode, flags)
loop chunks
Xfer->>Ctlr: spi_phytium_set(fts)
Ctlr->>Wait: spi_phytium_check_result(fts)
Wait->>Wait: phytium_write_regfile(fts, SPI_REGFILE_AP2RV_INTR_STATE, 0x10)
Wait->>Wait: wait_for_completion_interruptible_timeout(&fts->cmd_completion, timeout)
end
else other paths
PhyDrv->>PhyDrv: legacy read/write handling
end
IRQ-->>Wait: spi_phyt_irq(irq, dev_id)
IRQ->>IRQ: writel_relaxed(0, RV2AP_INTR_STATE)
IRQ->>IRQ: writel_relaxed(0x10, RV2AP_INT_CLEAN)
IRQ->>Wait: complete(&fts->cmd_completion)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @xiaqian1486. Thanks for your PR. 😃 |
|
Hi @xiaqian1486. Thanks for your PR. I'm waiting for a deepin-community member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In spi_phytium_xfer(), the length calculation and buffer progression look wrong for full‑duplex: you compute len using (rx_end - rx) twice, never reference tx_end, never advance fts->tx, and never use the local
datapointer, so the TX side will not progress correctly across chunks. - The use of wait_for_completion_interruptible_timeout() in spi_phytium_check_result() changes semantics compared to wait_for_completion_timeout(), but the return value is still treated as an unsigned timeout count; a signal interruption (negative return) will be misinterpreted as a large nonzero timeout and skip the error path—consider handling negative returns explicitly.
- SPI_REGFILE_DDR_HIGH_REG is defined twice in spi-phytium.h (before and after the DEBUG register block), which is redundant and may confuse future maintainers; it would be better to keep a single definition in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In spi_phytium_xfer(), the length calculation and buffer progression look wrong for full‑duplex: you compute len using (rx_end - rx) twice, never reference tx_end, never advance fts->tx, and never use the local `data` pointer, so the TX side will not progress correctly across chunks.
- The use of wait_for_completion_interruptible_timeout() in spi_phytium_check_result() changes semantics compared to wait_for_completion_timeout(), but the return value is still treated as an unsigned timeout count; a signal interruption (negative return) will be misinterpreted as a large nonzero timeout and skip the error path—consider handling negative returns explicitly.
- SPI_REGFILE_DDR_HIGH_REG is defined twice in spi-phytium.h (before and after the DEBUG register block), which is redundant and may confuse future maintainers; it would be better to keep a single definition in one place.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR updates the Phytium SPI controller drivers to a newer upstream version, adding new hardware capability detection (version/DDRx/full-duplex), improving restart/timeout handling, and extending DMA enablement (including ACPI FixedDMA).
Changes:
- Add regfile feature/version bits, DDR-high address support, and full-/half-duplex capability handling for SPI-V2.
- Rework command completion signaling/timeouts and optimize debug-log buffer clearing/mapping.
- Enable DMA on ACPI FixedDMA platforms and adjust Kconfig dependencies to restrict builds to ARCH_PHYTIUM.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| drivers/spi/spi-phytium.h | Adds regfile offsets/version bits and new transfer sub-commands. |
| drivers/spi/spi-phytium-common.c | Adjusts completion/timeout signaling and adds a new full-duplex xfer helper. |
| drivers/spi/spi-phytium-v2.c | Updates interrupt completion ordering, full-duplex gating, DDR-high log mapping, and timer suspend/resume. |
| drivers/spi/spi-phytium-plat.c | Adds ACPI FixedDMA detection and changes DMA enablement logic; defaults global CS on. |
| drivers/spi/spi-phytium-plat-v2.c | Switches DMA capability detection to regfile version bits. |
| drivers/spi/spi-phytium-dma.c | Requests DMA channels via ACPI FixedDMA when present. |
| drivers/spi/Kconfig | Restricts Phytium SPI configs to ARCH_PHYTIUM and tightens PCI dependency. |
| drivers/mtd/parsers/acpipart_core.c | Initializes child_handle to avoid use of an uninitialized pointer. |
| drivers/spi/spi-phytium-v2.c.orig | Patch artifact file added (should be removed). |
| drivers/spi/spi-phytium-v2.c.rej | Patch artifact file added (should be removed). |
| drivers/spi/Kconfig.orig | Patch artifact file added (should be removed). |
| drivers/spi/Kconfig.rej | Patch artifact file added (should be removed). |
Comments suppressed due to low confidence (2)
drivers/spi/spi-phytium-common.c:125
wait_for_completion_interruptible_timeout()returns a signedlong(0 on timeout, <0 on signal). Storing it in anunsigned long longand only checking== 0will mis-handle negative returns (they become large positive values), potentially treating an interrupted wait as success. Use a signed type (e.g.,long timeleft) and handletimeleft < 0explicitly.
unsigned long long ms = 20000;
struct msg *msg = (struct msg *)fts->tx_shmem_addr;
if (fts->flash_erase == 2)
ms = 200000;
reinit_completion(&fts->cmd_completion);
phytium_write_regfile(fts, SPI_REGFILE_AP2RV_INTR_STATE, 0x10);
ms = wait_for_completion_interruptible_timeout(&fts->cmd_completion, msecs_to_jiffies(ms));
if (ms == 0) {
dev_err(&fts->master->dev, "SPI controller timed out\n");
return -1;
drivers/mtd/parsers/acpipart_core.c:37
child_handleis now initialized (good), but the subsequentdevice_get_next_child_node(dev, child_handle);call ignores the returned fwnode, leavingchild_handleas NULL. If this is meant to probe for a 'partitions' child, assign the return value tochild_handle(and drop the reference when done), otherwise the dedicated-subnode detection will never work.
struct fwnode_handle *child_handle = NULL;
bool dedicated = true;
struct device *dev;
dev = &master->dev;
adev = ACPI_COMPANION(&master->dev);
if (!master->parent) {/*master*/
device_get_next_child_node(dev, child_handle);
if (!child_handle) {
pr_debug("%s: 'partitions' subnode not found on %pOF. Trying to parse direct subnodes as partitions.\n",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+61
to
+76
| #define SPI_REGFILE_SIZE (0x48) | ||
| #define SPI_REGFILE_DDR_HIGH_REG (0x4c) | ||
|
|
||
| #define SPI_REGFILE_SOFTWARE2 (0x54) | ||
| #define SPI_REGFILE_FULL_DUPLEX BIT(9) | ||
|
|
||
| #define SPI_REGFILE_DEBUG (0x58) | ||
| #define SPI_REGFILE_DEBUG_VAL BIT(0) | ||
| #define SPI_REGFILE_ALIVE_VAL BIT(1) | ||
| #define SPI_REGFILE_HEARTBIT_VAL BIT(2) | ||
| #define SPI_REGFILE_HAVE_LOG BIT(3) | ||
| #define SPI_REGFILE_SIZE_MASK GENMASK(7, 4) | ||
| #define SPI_REGFILE_ADDR_MASK GENMASK(27, 8) | ||
|
|
||
| #define SPI_REGFILE_DDR_HIGH_REG (0x4c) | ||
| #define SPI_REGFILE_VERSION_REG (0x700) |
Comment on lines
136
to
142
| /* check is use dma transfer */ | ||
| if ((device_property_read_string_array(&pdev->dev, "dma-names", | ||
| NULL, 0) > 0) && | ||
| device_property_present(&pdev->dev, "dmas")) { | ||
| NULL, 0 > 0) && | ||
| device_property_present(&pdev->dev, "dmas")) || | ||
| (has_acpi_companion(&pdev->dev) && | ||
| phytium_acpi_has_FixedDMA(dev))) { | ||
| fts->dma_en = true; |
Comment on lines
+464
to
+470
| do { | ||
| if (fts->dma_get_ddrdata) | ||
| len = min_t(u32, (u32)(fts->rx_end - fts->rx), | ||
| (u32)(fts->rx_end - fts->rx)); | ||
| else | ||
| len = min_t(u32, (u32)(fts->rx_end - fts->rx), 128); | ||
|
|
Comment on lines
+492
to
+518
| fts->msg->cmd_subid = PHYTSPI_MSG_CMD_DATA_XFER; | ||
| memcpy_byte((void *)smem_tx, fts->tx, len); | ||
| *(u64 *)&fts->msg->data[0] = sizeof(struct msg); | ||
| *(u64 *)&fts->msg->data[8] = sizeof(struct msg) + 128; | ||
| } | ||
|
|
||
| *(u32 *)&fts->msg->data[16] = len; | ||
| fts->msg->data[20] = cs; | ||
| fts->msg->data[21] = dfs; | ||
| fts->msg->data[22] = mode; | ||
| fts->msg->data[23] = tmode; | ||
| if (first == 1) | ||
| fts->msg->data[24] = 1; | ||
| else | ||
| fts->msg->data[24] = flags; | ||
| fts->msg->data[24] = first; | ||
| ret = spi_phytium_set(fts); | ||
| if (ret) { | ||
| dev_err(&fts->master->dev, "AP <-> RV interaction failed\n"); | ||
| return ret; | ||
| } | ||
| if (len <= 16 || !fts->dma_get_ddrdata) | ||
| memcpy_byte(fts->rx, (void *)smem_rx, len); | ||
|
|
||
| fts->rx += len; | ||
| first = 0; | ||
| } while (fts->rx_end > fts->rx); |
| fts->msg->data[24] = 1; | ||
| else | ||
| fts->msg->data[24] = flags; | ||
| fts->msg->data[24] = first; |
| static void spi_phyt_hw_init(struct device *dev, struct phytium_spi *fts) | ||
| { | ||
| u32 reg, i; | ||
| u32 reg, i, reg_ddr_high; |
Comment on lines
+1
to
+5
| // SPDX-License-Identifier: GPL-2.0 | ||
| /* | ||
| * Phytium SPI core controller driver. | ||
| * | ||
| * Copyright (c) 2023-2024, Phytium Technology Co., Ltd.. |
Comment on lines
+1
to
+15
| --- drivers/spi/spi-phytium-v2.c | ||
| +++ drivers/spi/spi-phytium-v2.c | ||
| @@ -137,10 +137,10 @@ static int spi_phyt_transfer_one(struct spi_master *master, | ||
| struct phytium_spi *fts = spi_master_get_devdata(master); | ||
| struct chip_data *chip = spi_get_ctldata(spi); | ||
| struct spi_mem *mem = spi_get_drvdata(spi); | ||
| - struct spi_nor *nor; | ||
| + struct spi_nor *nor = NULL; | ||
| int ret; | ||
|
|
||
| - if (mem) | ||
| + if (mem && (mem->spi == spi)) | ||
| nor = spi_mem_get_drvdata(mem); | ||
|
|
||
| fts->tx = (void *)transfer->tx_buf; |
Comment on lines
+792
to
+800
| config SPI_PHYTIUM | ||
| tristate | ||
| depends on ARCH_PHYTIUM | ||
|
|
||
| config SPI_PHYTIUM_PLAT | ||
| tristate "Phytium SPI controller platform support" | ||
| depends on ARCH_PHYTIUM || COMPILE_TEST | ||
| select SPI_PHYTIUM | ||
| help |
Comment on lines
+1
to
+19
| --- drivers/spi/Kconfig | ||
| +++ drivers/spi/Kconfig | ||
| @@ -771,6 +771,7 @@ config SPI_PHYTIUM | ||
|
|
||
| config SPI_PHYTIUM_PLAT | ||
| tristate "Phytium SPI controller platform support" | ||
| + depends on ARCH_PHYTIUM | ||
| select SPI_PHYTIUM | ||
| help | ||
| This selects a platform driver for Phytium SPI controller. | ||
| @@ -780,7 +781,7 @@ config SPI_PHYTIUM_PLAT | ||
|
|
||
| config SPI_PHYTIUM_PCI | ||
| tristate "Phytium SPI controller PCI support" | ||
| - depends on PCI | ||
| + depends on PCI && ARCH_PHYTIUM | ||
| select SPI_PHYTIUM | ||
| help | ||
| This selects a PCI driver for Phytium SPI controller. |
This driver is exclusively for the PHYTIUM platform and is not compatible with other SoCs. This restriction prevents errors on unsupported platform. Mainline: Open-Source Signed-off-by:Xia Qian <xiaqian1486@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
The driver uses global-cs register(0x100) for chip selection by default. Slove the problem of not being able to read the device ID when using the internal chip selection register(0x10). Mainline: Open-Source Signed-off-by:Xia Qian <xiaqian1486@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
…tem restart Change the wait_for_completion_timeout function to the wait_for_completion_interruptable_timeout function. To Slove the issue of the "hung_task" during system restart, which will lead to system crash occasionally. Mainline: Open-Source Signed-off-by:Xia Qian <xiaqian1486@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
When the SPI device wakes up from sleep mode, it may hang up or timeout at extremely low probability. So We delete the timer during hibernation and restore it upon waking up. Mainline: Open-Source Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
Replace the original loop assignment with the more standard and efficient memset function to achieve the clearing operation of the debug-log buffer. Mainline: Open-Source Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
If the SPI interrupt is enabled before the waiting period is over, it will cause the SPI controller to timeout at low probability. Mainline: Open-Source Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
The time required for most requests is less than 1 second. Only the erase-chip takes a longer time, which will take several minutes. Therefore, it is not appropriate to use a uniform maxmum duration as the timeout period. Mainline: Open-Source Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
Enable DMA when the SPI controller is described by ACPI using FixedDMA. The driver now detects the ACPI firmware path, acquires RX/TX channels by index, and arms the DMA path accordingly, while preserving the existing Device Tree behavior. This prevents unintended fallback to PIO on ACPI platforms. Mainline: Open-Source Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn> Signed-off-by: zhuling <zhuling2709@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
In order to accommodate devices such as spidev and tpm that support full-duplex transmission, full-duplex support has been added to the spi-v2 driver. Mainline: NA Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
Determine whether to enable DMA andwhether it is compatible with 32-bit and 45-bit physical memory addresses by reading the regfile version register added to the spi-v2 driver. Mainline: NA Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn> Signed-off-by: Peng Yao <pengyao2712@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
The warning message 'child_handle is uninitialied' will occur when compiled, owing to uninitialized value for the child_handle parameter. Mainline: NA Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn> Signed-off-by: Peng Yao <pengyao2712@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
This patch addresses two issues with Phytium SPI-V2 CAN device handling: 1. Add hardware connection validation before accessing SPI device driver data. When CAN device is described in DTS but not physically connected, prevent crashes by checking spi_device validity before obtaining spi_mem drv data. 2. Enable GPIO-based chip select simulation in SPI subsystem. Implement GPIO CS control by extracting GPIO chip select description from DTS/ACPI configuration and managing GPIO CS state within the SPI subsystem stack. 3.The cs-gpios control is placed in the SPI subsystem,Due to code redundancy,the phytium controller driver code cs-gpios has been removed. This ensures proper handling of disconnected CAN devices and provides flexible chip select control for SPI-based CAN implementations. Mainline: NA Signed-off-by: Xia Qian <xiaqian1486@phytium.com.cn> Signed-off-by: Peng Yao <pengyao2712@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patches updates the support for phytium SPI controller driver.
Summary by Sourcery
Update Phytium SPI controller drivers for newer hardware capabilities, improved transfer modes, and more robust interrupt, DMA, and logging handling.
New Features:
Bug Fixes:
Enhancements: