spi: zynq: Add Zynq 7000 Quad SPI driver - #113254
Conversation
d529d1f to
6917da5
Compare
|
Tested erase, write, and read operations with the Micron N25Q256A11 flash. Quad SPI has not been tested yet, as the spi-nor driver doesn't support it. |
zephyrbot
added
area: Devicetree Binding
| interrupts: | ||
| required: true | ||
|
|
||
| clock-frequency: |
There was a problem hiding this comment.
The binding makes clock-frequency required, but the driver never reads
clock-frequency. It obtains the reference rate at runtime via
clock_control_get_rate(config->clock_dev, ...), where clock_dev comes from
DEVICE_DT_GET(DT_INST_CLOCKS_CTLR(n)) (driver line ~596) — i.e. it depends on a clocks
property. That clocks property is not declared/required by the binding, and the in-tree
node added to dts/arm/xilinx/zynq7000.dtsi has neither clocks nor clock-frequency.
Consequently: (a) any board that enables this node following the binding (adding only
clock-frequency) will fail to build because DT_INST_CLOCKS_CTLR(n) has no phandle to resolve;
(b) clock-frequency is dead/unused. Note clock-frequency is already inherited from
spi-controller.yaml, so re-declaring it as required here is also redundant. remove it from the bindings if driver is not using it
There was a problem hiding this comment.
Fixed. I have updated the driver to actually use the clock-frequency property now.
I would have preferred to use clock_control_get_rate(), but the Zynq 7000 clock control driver PR (#64996) unfortunately went stale. Because we currently lack a runtime clock controller for this platform, falling back to a static clock-frequency is the most viable path forward.
Once a proper clock control driver is upstreamed, we can refactor this driver to query it at runtime.
| bool "Xilinx Zynq Quad SPI driver" | ||
| default y | ||
| depends on DT_HAS_XLNX_ZYNQ_QSPI_ENABLED | ||
| select EVENTS |
There was a problem hiding this comment.
select EVENTS appears unnecessary — the driver uses spi_context (semaphore-based
sync via SPI_CONTEXT_INIT_SYNC / spi_context_wait_for_completion) and contains no k_event
usage; spi_context.h does not use the events API either. (The sibling Kconfig.xlnx and
Kconfig.xlnx_zynqmp_gqspi also select EVENTS, so this looks copy-pasted.) Also the prompt
"Xilinx Zynq Quad SPI driver" includes the word "driver"; upstream prefers a short noun phrase
There was a problem hiding this comment.
I've removed select EVENTS from the Kconfig.
Regarding the prompt, I kept 'Xilinx Zynq Quad SPI driver' for now to remain consistent with the existing SPI drivers in the tree (which largely include the word 'driver' in their prompts). However, if you'd prefer me to change it to match the newer style guideline, let me know and I'll update it!
As a side note: I also considered moving the FIFO processing out of the ISR, but left it there to avoid increasing the risk of FIFO overflow/underflow.
| return -ENOTSUP; | ||
| } | ||
|
|
||
| LOG_DBG("freqency requested=%u input frequency=%u CR_BAUD_RATE_DIV=0x%x actual " |
There was a problem hiding this comment.
freqency is misspelled (should be frequency). Also treshholds at line ~566
(/* set FIFO treshholds */, should be thresholds).
There was a problem hiding this comment.
Fixed both typos. Thanks for catching those!
| } | ||
| #endif /* CONFIG_SPI_ASYNC */ | ||
|
|
||
| static int zynq_qspi_release(const struct device *dev, const struct spi_config *spi_cfg) |
There was a problem hiding this comment.
spi_cfg is unused; missing ARG_UNUSED(spi_cfg);.
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Two consecutive blank lines between functions. remove one
| @@ -0,0 +1,614 @@ | |||
| /* | |||
| * Copyright (c) 2024 Simon Maurer <mail@maurer.systems> | |||
kedareswararao
left a comment
kedareswararao
left a comment
There was a problem hiding this comment.
Changes looks fine to me
|
fixed a problem in the ISR (remainder was left in the FIFO) |
|
|
This pull request has been marked as stale because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 7 days. Note that if it gets closed, you can ask someone to reopen it for you if you do not have the permissions to do so. |
|
Hi everyone, just wanted to provide a gentle ping on this PR. Let me know if there's anything I can do to help move it forward or if you are waiting on any changes! |
|
@mausys I have removed the stale lable please take a look into the CI failures if it's not releated your changes please rebase changes on top of latest main so that CI checks will re triggered |
Add a driver for the Xilinx Zynq 7000 Quad SPI controller. Note that while the hardware supports legacy SPI, this driver only supports flash mode. Signed-off-by: Simon Maurer <mail@maurer.systems>
|
You have been identified as a likely reviewer for the code this pull request changes, but could not be added to its review request automatically. Please review it if you are able to. |
4 participants
Add a driver for the Xilinx Zynq 7000 Quad SPI controller. Note that while the hardware supports legacy SPI, this driver only supports flash mode.