This is a qspi driver for the Xilinx Zynq-7000 chip. It could be useful for anyone wanting to boot a system from flash memory instead of SD cards.
Details
Details
- Reviewers
manu - Group Reviewers
ARM - Commits
- rS356895: zilinx/zy7_qspi: Add a qspi driver for Zynq platforms.
Modest testing has been done on both a Zedboard and a Zybo. I also compiled this driver as a module and successfully loaded and unloaded it.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/arm/xilinx/zy7_qspi.c | ||
|---|---|---|
| 565 ↗ | (On Diff #40302) | I really want to rewrite this attach routine. For one thing, right here, if zy7_qspi_init_hw() fails (because it can't set the clock divider), the resources aren't released. |
Comment Actions
The GENERIC kernel boots (I just tried it).
I have worked on using gnu DTSs for Zynq boards (that's why there are two compatible strings for this driver). I got stuck with issues using the clock infrastructure. I'll revisit it soon but I'm treating that as a separate project.
Comment Actions
Add driver to GENERIC kernel. Add untested support for dual parallel and stacked memories.
| sys/arm/xilinx/zy7_qspi.c | ||
|---|---|---|
| 514 ↗ | (On Diff #40901) | This should be at the top |
| 727 ↗ | (On Diff #40901) | Shouldn't you depend on ofw_spibus ? And also add a DRIVER_MODULE it (the ofw_spibus) |
| sys/dts/arm/zedboard.dts | ||
| 72 ↗ | (On Diff #40901) | I see nothing in the linux dts for the presence of this spi flash, is it optional ? (same comment on the zybo) |
| sys/arm/xilinx/zy7_qspi.c | ||
|---|---|---|
| 2 ↗ | (On Diff #40901) | I'm going to add the proper license identifier here. |
| 514 ↗ | (On Diff #40901) | Ok. I propose putting it at line 202. |
| 727 ↗ | (On Diff #40901) | Something like this? +DRIVER_MODULE(ofw_spibus, zy7_qspi, ofw_spibus_driver, ofw_spibus_devclass, 0, 0); I'm looking at sys/arm/freescale/imx/imx_spi.c as an example. |
| sys/dts/arm/zedboard.dts | ||
| 72 ↗ | (On Diff #40901) | It's not optional. Just about every dev board has a qspi flash. The linux dts is incomplete. Xilinx has their own linux repository on github and I guess they aren't good at upstreaming their stuff. See: https://github.com/Xilinx/linux-xlnx/blob/master/arch/arm/boot/dts/zynq-zed.dts |
Comment Actions
Add license identifier. Add comment saying what this is. Move forward declaration of zy7_detach. Add MODULE_DEPEND and DRIVER_MODULE.
Comment Actions
Fix support for dual parallel and stacked memories (changes provided by John Sears). Change config option from zy7_qspi to just qspi.
Comment Actions
Any chance of getting this committed before code freeze? I think it's ready. I'm not comfortable making the changes to mx25l.c to support dual stacked memories (I have no hardware to test that) but I've left in the hooks needed in this driver.
Comment Actions
The main style issue I can see is the indentation of lines that have been split. They should be tabbed to the same level, then add 4 spaces.
| sys/arm/conf/GENERIC | ||
|---|---|---|
| 174 ↗ | (On Diff #46438) | Can you keep the spi controller list sorted. It would also be useful to have a comment saying which device this is. qspi might also be a bit too generic. Is it Xilinx specific? |
| sys/arm/xilinx/zy7_qspi.c | ||
| 109 ↗ | (On Diff #46438) | You should start the comment on a new line. i.e. /* * QSPI ... |
| 228 ↗ | (On Diff #46438) | This should be indented to the same level as the if + 4 spaces. |
| 428 ↗ | (On Diff #46438) | What are these magic numbers? |
| sys/arm/conf/GENERIC | ||
|---|---|---|
| 174 ↗ | (On Diff #46438) | I'm not sure if quad-spi is Xilinx-specific. The driver is pretty Xilinx-specific although it's probably a Synopsys Designware core. I originally had it as zy7_qspi. Maybe I can switch it to zynq_qspi? |
| sys/arm/conf/GENERIC | ||
|---|---|---|
| 174 ↗ | (On Diff #46438) | I probably shouldn't put this into the GENERIC kernel anyway. I haven't been testing GENERIC on the Zedboard and the other Zynq drivers aren't in there either. |
| sys/arm/xilinx/zy7_qspi.c | ||
| 428 ↗ | (On Diff #46438) | The support for dual memories was given to me and I don't understand it entirely. These are flash memory commands. It looks like mx25lreg.h (sys/dev/flash) is the defacto header file for flash command defines (the other drivers include it). I could look up these commands, add them to mx25lreg.h, and then include it here. |
Comment Actions
I've made suggested changes but the driver has regressed: the flash device doesn't get recognized. I'm looking into it.
Comment Actions
Change driver declaration to fix a bug that caused the wrong kind of spibus to attach. Thanks, Ian Lepore.
Comment Actions
generally I like this. Couple of minor spacing nits, and a question about a possible improper use of KASSERT
| sys/arm/xilinx/zy7_qspi.c | ||
|---|---|---|
| 322 ↗ | (On Diff #47320) | minor style nit, this should be indented tab + 4 spaces, not 2 tabs. |
| 444 ↗ | (On Diff #47320) | minor nit: tab + 4 indent. |
| 480 ↗ | (On Diff #47320) | Is this bug workaround documented somewhere? If so a reference to it wouldn't be bad here... |
| 676 ↗ | (On Diff #47320) | Wouldn't the user be able to trigger a panic with this? Is it possible to have these two conditions in here, or are there sufficient checks elsewhere that I've missed? |
Comment Actions
I had the incorrect clock value for Zybo. It uses a 200 Mhz reference clock just like the Zedboard. So put default reference clock value and spi clock value into zynq-7000.dtsi
| sys/arm/conf/ZEDBOARD | ||
|---|---|---|
| 61–62 ↗ | (On Diff #64920) | Can you also add these to GENERIC |
| sys/arm/xilinx/zy7_qspi.c | ||
| 237–238 ↗ | (On Diff #64920) | When splitting a line you should tab the new line to the same level as the previous line then add 4 spaces, e.g. in this case: <tab><tab><space><space><space><space>. There are a few more places in this diff where you'll need to apply this. The int n should also be at the top of the function. |
| sys/arm/conf/ZEDBOARD | ||
|---|---|---|
| 61–62 ↗ | (On Diff #64920) | Getting a GENERIC kernel to work on the Zynq might take a little work. I'd rather treat that as a separate effort. |
| sys/arm/xilinx/zy7_qspi.c | ||
| 237–238 ↗ | (On Diff #64920) | It took all morning but I think I have wrangled emacs to stop indenting like this. Let me know what you think. |
| sys/arm/conf/ZEDBOARD | ||
|---|---|---|
| 61–62 ↗ | (On Diff #64920) | Hmmm. I just booted a GENERIC kernel on a Zynq and it worked. Okay, will add to GENERIC. |
| sys/arm/conf/GENERIC | ||
|---|---|---|
| 175 ↗ | (On Diff #64976) | mx25l will be automatically loaded via devmatch if a node is compatible in the dtb, no need to add it to GENERIC as it's not needed to boot (that goes for all other spi driver that could just be module and not be in the GENERIC kernel but that's another story ...). |
| sys/arm/conf/GENERIC | ||
|---|---|---|
| 175 ↗ | (On Diff #64976) | I tried this (removing mx25l) and it doesn't work for me. It might be something wrong with the way I build my SD image. I'll try to figure it out. I also built this driver as a module and it works provided I manually kldload it. |
Comment Actions
Remove mx25l from GENERIC conf. Tweak compatible strings for flash memory in zedboard/zynq .dts files so that devd can find the mx25l module.
| sys/arm/conf/GENERIC | ||
|---|---|---|
| 175 ↗ | (On Diff #64976) | The problem was devd couldn't find the module because "s25fl128" isn't in mx25l's compat_data. I tweaked the dts files and now it works. |
| sys/arm/conf/GENERIC | ||
|---|---|---|
| 175 ↗ | (On Diff #64976) | If the flash support ID command you can just put "jedec,spi-nor" as the compatible string. |