Page MenuHomeFreeBSD

[new driver] zilinx/zy7_qspi: Add a qspi driver for Zynq platforms.
ClosedPublic

Authored by skibo on Mar 14 2018, 10:05 PM.

Details

Summary

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.

Test Plan

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

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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.

Properly free resources if zy7_qspi_init_hw() fails.

Why do we still use a non-GENERIC kernel config and custom DTS for this board ?

In D14698#311089, @manu wrote:

Why do we still use a non-GENERIC kernel config and custom DTS for this board ?

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.

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);
+MODULE_DEPEND(zy7_qspi, ofw_spibus, 1, 1, 1);

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
https://github.com/Xilinx/linux-xlnx/blob/master/arch/arm/boot/dts/zynq-7000.dtsi

Add license identifier. Add comment saying what this is. Move forward declaration of zy7_detach. Add MODULE_DEPEND and DRIVER_MODULE.

Fix support for dual parallel and stacked memories (changes provided by John Sears). Change config option from zy7_qspi to just qspi.

linimon retitled this revision from Add a qspi driver for Zynq platforms. to [new driver] zilinx/zy7_qspi: Add a qspi driver for Zynq platforms..Aug 2 2018, 10:18 PM

Add compatible string for Zynq Ultrascale which uses same qspi device.

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.

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.

Take driver out of GENERIC kerne. Make other minor fix-ups.

skibo marked an inline comment as done.

Add sys/dev/flash/mx25lreg.h which has two new flash commands added.

I've made suggested changes but the driver has regressed: the flash device doesn't get recognized. I'm looking into it.

Change driver declaration to fix a bug that caused the wrong kind of spibus to attach. Thanks, Ian Lepore.

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?

it looks to my eye that all prior comments had been addressed...

Indentation, cite updated reference manual w.r.t. transmit fifo quirk.

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.

Fix tiny regression in documentation reference.

skibo added inline comments.
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.

skibo added inline comments.
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.

Add device to GENERIC kernel.

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.

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.

This revision is now accepted and ready to land.Jan 19 2020, 7:58 PM