spi: phytium: update phytium spi controller driver support#1538
spi: phytium: update phytium spi controller driver support#1538newearth-ss wants to merge 7 commits intodeepin-community:linux-6.6.yfrom
Conversation
If the mtd_debug tool is used for erasing data, the user layer does not split the erasing. The core layer handles the split operation. Mainline: NA Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
Functions that configure capacity registers should shift left instead of right, which will lead to capacity register configured error. Mainline: Open-Source Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
The dummy bit of the cmd_port register is incorrectly configured, which will results in a 1-1-4 indirect read error. Mainline: Open-Source Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
If the device tree or ACPI table does not describe the flash node, the capacity register should not be configured for the QSPI controller driver. Mainline: Open-Source Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
The error information is stored to the specified ddr address, and when an exception occurs, the error information can be retrieved from this address. Mainline: Open-Source Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
The spi-v2 driver is adapted to the mcp251x device. Furthermore, spi-v2 currently only supports half-duplex. Mainline: Open-Source Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
The spi-v2 driver is adapted to the tpm device. Furthermore, spi-v2 currently only supports half-duplex. Mainline: Open-Source Signed-off-by: Cui Fulong <cuifulong2112@phytium.com.cn> Signed-off-by: Peng Min <pengmin1540@phytium.com.cn> Signed-off-by: Wang Yinfeng <wangyinfeng@phytium.com.cn>
|
[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 |
Reviewer's GuideUpdates Phytium SPI and QSPI controller drivers to better integrate with spi-mem/spi-nor, fix capacity/dummy-cycle handling, add debug-log support, and adjust controller flags and probe defaults. Sequence diagram for spi_mem spi_nor command handling in spi_phyt_transfer_onesequenceDiagram
participant spi_core
participant spi_master
participant phytium_spi as fts
participant spi_device as spi
participant spi_mem
participant spi_nor as nor
spi_core->>spi_master: queue transfer_one(spi, transfer)
spi_master->>fts: spi_phyt_transfer_one(master, spi, transfer)
activate fts
fts->>spi: spi_get_ctldata(spi)
fts->>spi: spi_get_drvdata(spi)
spi-->>fts: spi_mem pointer
alt has_spi_mem
fts->>spi_mem: spi_mem_get_drvdata(mem)
spi_mem-->>fts: spi_nor pointer nor
alt single_byte_tx_for_nor
fts->>fts: check mem == nor.spimem
fts->>fts: set tx, rx, len
alt tx_is_SPINOR_OP_WREN and spi_write_flag == 0
fts->>fts: spi_phytium_write_pre(..., 0)
fts-->>spi_master: return 0
deactivate fts
else tx_is_SPINOR_OP_BE_4K and spi_write_flag == 1 and flash_read == 0 and flash_erase != 1
fts->>fts: spi_write_flag++
fts->>fts: flash_erase = 1
fts-->>spi_master: return 0
activate fts
deactivate fts
else other_command
fts->>fts: flags = 0
fts->>fts: spi_phytium_write(..., flags, spi_write_flag)
fts-->>spi_master: return status
activate fts
deactivate fts
end
else not_single_byte_or_not_nor
fts-->>spi_master: handle as generic transfer
activate fts
deactivate fts
end
else no_spi_mem
fts-->>spi_master: handle as generic transfer
activate fts
deactivate fts
end
Class diagram for updated phytium_spi structure and callbacksclassDiagram
class phytium_spi {
+void __iomem *base
+struct spi_master *master
+u32 tx_len
+u32 rx_len
+void *tx
+void *tx_end
+void *rx
+u32 len
+u32 cur_rx_head
+u32 cur_rx_tail
+u32 cur_tx_head
+u32 cur_tx_tail
+struct completion cmd_completion
+u8 flags
+u8 spi_write_flag
+u8 flash_erase
+u8 flash_read
+bool debug_enabled
+bool alive_enabled
+struct timer_list timer
+u32 runtimes
+u64 ddr_paddr
+char *log
+u32 log_size
+void (*watchdog)(struct phytium_spi *fts)
+void (*handle_debug_err)(struct phytium_spi *fts)
}
class spi_master {
+struct device dev
+u32 flags
}
class device {
+struct device_node *of_node
+struct fwnode_handle *fwnode
}
class spi_phyt_hw_init {
+void spi_phyt_hw_init(struct device *dev, struct phytium_spi *fts)
}
class spi_handle_debug_err {
+void spi_handle_debug_err(struct phytium_spi *fts)
}
class spi_phytium_print_status {
+int spi_phytium_print_status(struct phytium_spi *fts, u8 status0, u8 status1)
}
class spi_phyt_add_host {
+int spi_phyt_add_host(struct device *dev, struct phytium_spi *fts)
}
phytium_spi --> spi_master : master
spi_master --> device : dev
spi_phyt_hw_init --> phytium_spi : initializes
spi_phyt_hw_init --> device : uses
spi_handle_debug_err --> phytium_spi : uses
spi_phytium_print_status --> phytium_spi : uses
spi_phyt_add_host --> phytium_spi : configures
spi_phyt_add_host --> spi_master : allocates
phytium_spi ..> spi_handle_debug_err : handle_debug_err callback
phytium_spi ..> spi_phyt_hw_init : uses log setup
phytium_spi ..> spi_phytium_print_status : status based debug
spi_phyt_add_host ..> spi_handle_debug_err : assigns callback
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Hi @newearth-ss. 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_phyt_transfer_one(),struct spi_nor *norcan be used uninitialized or NULL (e.g. whenmem == NULLorspi_mem_get_drvdata(mem)returns NULL) in theif (mem == nor->spimem && ...)condition; add proper NULL checks and initialization before dereferencingnor. - In
spi_phyt_hw_init(),fts->log_sizederived fromSPI_REGFILE_SIZE_MASKcan be zero, which leads todevm_ioremap()with size 0 and later loops overfts->log_size; add a guard to skip mapping and log handling when the encoded size is zero or otherwise invalid.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `spi_phyt_transfer_one()`, `struct spi_nor *nor` can be used uninitialized or NULL (e.g. when `mem == NULL` or `spi_mem_get_drvdata(mem)` returns NULL) in the `if (mem == nor->spimem && ...)` condition; add proper NULL checks and initialization before dereferencing `nor`.
- In `spi_phyt_hw_init()`, `fts->log_size` derived from `SPI_REGFILE_SIZE_MASK` can be zero, which leads to `devm_ioremap()` with size 0 and later loops over `fts->log_size`; add a guard to skip mapping and log handling when the encoded size is zero or otherwise invalid.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 and QSPI controller drivers with bug fixes, a new debug log buffer feature, and configuration adjustments. The changes include fixing critical shift-direction bugs in QSPI flash capacity encoding, adding debug log buffer discovery/dumping for the SPI v2 controller, adjusting SPI-NOR write/erase handling, and preventing direct mapping when no flashes are attached.
Changes:
- Fix QSPI flash capacity encoding (right shift → left shift), dummy-cycle configuration, and direct mapping guard for zero flashes.
- Add debug log buffer support (MMIO-mapped log region, error handler hook) and mark the SPI v2 master as half-duplex capable.
- Adjust SPI-NOR transfer_one logic to gate write/erase commands based on the spi_mem instance, remove premature CS return guards, and change default global_cs to 1.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| drivers/spi/spi-phytium.h | Add debug log defines, flags, ddr_paddr, log, log_size, and handle_debug_err fields to the phytium_spi struct |
| drivers/spi/spi-phytium-v2.c | Remove CS early-return guards, add spi_mem/spi_nor-based gating in transfer_one, add debug log handler and hw_init log mapping, set half-duplex flag, wire up debug_err hook, fix includes |
| drivers/spi/spi-phytium-qspi.c | Fix >> to << in capacity encoding, fix dummy cycle shift constant, add fnum != 0 guard for direct mapping, bump version |
| drivers/spi/spi-phytium-plat-v2.c | Change default global_cs from 0 to 1, bump driver version |
| drivers/spi/spi-phytium-common.c | Call handle_debug_err when debug is enabled and an error status is detected |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -158,7 +158,7 @@ static int spi_phyt_transfer_one(struct spi_master *master, | |||
| chip->tmode = TMOD_TO; | |||
| } | |||
|
|
|||
| if (fts->tx) { | |||
| if (mem == nor->spimem && fts->tx && fts->len == 1) { | |||
There was a problem hiding this comment.
Critical null-pointer dereference: when mem is NULL (i.e., the SPI device is not backed by a spi-mem driver), nor is uninitialized. On line 161, the code unconditionally dereferences nor->spimem regardless of whether mem was NULL. This will crash with a null/garbage pointer dereference for any non-SPI-NOR SPI device.
The condition should check mem before accessing nor, e.g., if (mem && mem == nor->spimem && fts->tx && fts->len == 1).
| fts->log = devm_ioremap(dev, fts->ddr_paddr, fts->log_size); | ||
|
|
||
| if (IS_ERR(fts->log)) { | ||
| dev_err(dev, "log_addr is err\n"); | ||
| return; | ||
| } | ||
|
|
||
| for (i = 0; i < fts->log_size; i++) | ||
| fts->log[i] = 0; |
There was a problem hiding this comment.
devm_ioremap() returns NULL on failure, not an ERR_PTR. The IS_ERR() check will never catch a NULL return, so a NULL fts->log will be passed through to the subsequent loop where fts->log[i] = 0 will cause a null-pointer dereference.
The check should use if (!fts->log) instead of if (IS_ERR(fts->log)).
Additionally, when log_size is 0 (the size bits in the register are zero), devm_ioremap will be called with a size of 0, which is also problematic. A guard for log_size == 0 should be added before the devm_ioremap call.
| #include <linux/spi/spi-mem.h> | ||
| #include <linux/mtd/spi-nor.h> |
There was a problem hiding this comment.
Duplicate include: <linux/mtd/spi-nor.h> is included on both line 19 and line 27. The second include is redundant and should be removed.
| dev_info(dev, "log len :%d,addr: 0x%llx,size:%d\n", | ||
| len, (u64)fts->log, fts->log_size); |
There was a problem hiding this comment.
Logging a kernel pointer via (u64)fts->log leaks kernel address information to the kernel log, which is a security concern (KASLR bypass). Use %p or %px format specifier instead of casting to u64 and printing with %llx. In production code, %p is preferred as it prints a hashed pointer.
| char *log; | ||
| u32 log_size; |
There was a problem hiding this comment.
The log field is declared as char * in the struct, but devm_ioremap() returns void __iomem *. Accessing MMIO memory through a plain char * pointer using direct array indexing (e.g., fts->log[i] = 0, strnlen(fts->log, ...)) bypasses the __iomem annotation and can trigger sparse warnings. MMIO memory should be accessed through readb()/writeb() or memset_io()/memcpy_fromio() accessors.
| void spi_handle_debug_err(struct phytium_spi *fts) | ||
| { | ||
| struct device *dev = &fts->master->dev; | ||
| u32 reg, len, i; | ||
|
|
||
| reg = phytium_read_regfile(fts, SPI_REGFILE_DEBUG); | ||
|
|
||
| if (reg & SPI_REGFILE_HAVE_LOG) { | ||
| len = strnlen(fts->log, fts->log_size); |
There was a problem hiding this comment.
spi_handle_debug_err does not check if fts->log is NULL before calling strnlen(fts->log, ...) and accessing fts->log[i]. If spi_phyt_hw_init failed to map the log buffer (or log_size was 0), this will crash. A null check for fts->log and a zero check for fts->log_size should be added at the top of this function.
| fts->flags = 0; | ||
|
|
||
| ret = spi_phytium_write(fts, spi->chip_select, transfer->bits_per_word, | ||
| spi->mode, chip->tmode, 1, fts->spi_write_flag); | ||
| spi->mode, chip->tmode, fts->flags, fts->spi_write_flag); |
There was a problem hiding this comment.
Setting fts->flags = 0 and then immediately passing fts->flags as the argument on the next line is functionally equivalent to just passing 0 directly (which was the original value 1). Using an instance field (fts->flags) for what is effectively a local constant is confusing — it appears to be a field meant for some other purpose elsewhere, but here it's unconditionally set to 0 and then read. If the intent is to always pass 0 here, just pass the literal 0 and document why this changed from 1. If fts->flags is meant to carry state across calls, setting it to 0 unconditionally right before use defeats that purpose.
| if (len > SPI_LOG_LINE_MAX_LEN) { | ||
| for (i = 0; i + SPI_LOG_LINE_MAX_LEN < len; i += SPI_LOG_LINE_MAX_LEN) | ||
| dev_info(dev, "(log)%.*s\n", SPI_LOG_LINE_MAX_LEN, &fts->log[i]); | ||
| } else { | ||
| dev_info(dev, "(log)%.*s\n", SPI_LOG_LINE_MAX_LEN, &fts->log[0]); | ||
| } |
There was a problem hiding this comment.
When len > SPI_LOG_LINE_MAX_LEN, the loop prints full chunks of SPI_LOG_LINE_MAX_LEN characters but never prints the remaining tail portion. For example, if len is 500, it prints characters 0-399 but characters 400-499 are silently dropped. After the for-loop, the remaining len - i bytes should also be printed.
This patch updates the support for phytium spi controller driver.
Summary by Sourcery
Update Phytium SPI and QSPI controller drivers with improved SPI-NOR handling, debugging/logging support, and configuration fixes.
New Features:
Bug Fixes:
Enhancements: