J-Core SPI controller support

Signed-off-by Rich Felker <[email protected]>

---
 .../devicetree/bindings/spi/jcore,spi.txt          | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/jcore,spi.txt

Quoted 5 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 .../devicetree/bindings/spi/jcore,spi.txt          | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/jcore,spi.txt

Acked-by Rob Herring <[email protected]>

Quoted 8 lines · Rob Herring

Quoted 5 lines · Rich Felker

Signed-off-by Rich Felker <[email protected]>

---
 .../devicetree/bindings/spi/jcore,spi.txt          | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/jcore,spi.txt

Acked-by Rob Herring <[email protected]>

Thanks. Since I switched to to using the clk framework rather than a clock-frequency property (at Mark Brown's suggestion/request), I'll be submitting one more version for your review.

After that, who should be responsible for upstreaming this and the other binding patches? You, me, or the subsystem maintainers that the drivers are going through?

drivers/spi/Kconfig config SPI_IMX · line 291

Quoted 4 lines · Rich Felker

drivers/spi/Kconfig +4 -0

+config SPI_JCORE
+	tristate "J-Core SPI Master"
+	depends on OF
+

An architecture or SoC dependency with || COMPILE_TEST would be useful for avoiding cluttering Kconfig for other users. Though as this is in a FPGA it's perhaps likely people will pick this up for other FPGAs so perhaps a comment to that effect if it seems likely.

drivers/spi/spi-jcore.c line 84

Quoted 5 lines

drivers/spi/spi-jcore.c new file +5 -0

+static void jcore_spi_baudrate(struct jcore_spi *hw, int speed)
+{
+	if (speed == hw->speed_hz) return;
+	hw->speed_hz = speed;
+	if (speed >= hw->clock_freq/2)

Coding style, spaces around /.

drivers/spi/spi-jcore.c line 131

Quoted 1 line

drivers/spi/spi-jcore.c new file +1 -0

+	return count<len ? -EREMOTEIO : 0;

Again here, and please also write this as a normal if statement so it's easier to read.

Quoted 4 lines

drivers/spi/spi-jcore.c new file +4 -0

+	/* The SPI clock rate controlled via a configurable clock divider
+	 * which is applied to the reference clock. A 50 MHz reference is
+	 * most suitable for obtaining standard SPI clock rates, but some
+	 * designs may have a different reference clock, and the DT must

drivers/spi/spi-jcore.c line 179

Quoted 5 lines

drivers/spi/spi-jcore.c new file +5 -0

+	 * make the driver aware so that it can properly program the
+	 * requested rate. If omitted, 50 MHz is assumed. */
+	clock_freq = 50000000;
+	of_property_read_u32(node, "clock-frequency", &clock_freq);
+	hw->clock_freq = clock_freq;

Why are you not using the clock API for this? Just require a clock and use clk_get_rate() to find out what rate it is.

drivers/spi/spi-jcore.c line 185

Quoted 1 line

drivers/spi/spi-jcore.c new file +1 -0

+	pdev->dev.dma_mask = 0;

Why are we doing this? There's no DMA code...

drivers/spi/spi-jcore.c line 190

Quoted 1 line

drivers/spi/spi-jcore.c new file +1 -0

+	dev_info(&pdev->dev, "base %p, noirq\n", hw->base);

This is just adding noise to the boot, just remove it - it's useful to log information we get from the silicon but things like the base address don't really add anything and end up cluttering (and slowing) the boot when everything does it.

Quoted 4 lines · Mark Brown

An architecture or SoC dependency with || COMPILE_TEST would be useful for avoiding cluttering Kconfig for other users. Though as this is in a FPGA it's perhaps likely people will pick this up for other FPGAs so perhaps a comment to that effect if it seems likely.

Unlike some of the other SoC hardware (interrupt controller) that's more closely tied to the SH cpu trap behavior, the SPI master seems like something that would be nice and easy to reuse elsewhere. I don't feel strongly about it either way though; I can add the arch dep if you want.

Quoted 2 lines

Why are you not using the clock API for this? Just require a clock and use clk_get_rate() to find out what rate it is.

I thought about that but I'm not familiar with it. I can try to figure it out quickly and test that approach; don't see any reason it shouldn't work. Would you insist on having full support for enabling/disabling the clk when it's in use, or would you be happy with treating it as a fixed clock that's always-on for now and possibly extending it with more functionality later if there's ever hardware where that's relevant/helpful?

Quoted 6 lines · Rich Felker

Quoted 4 lines · Mark Brown

An architecture or SoC dependency with || COMPILE_TEST would be useful for avoiding cluttering Kconfig for other users. Though as this is in a FPGA it's perhaps likely people will pick this up for other FPGAs so perhaps a comment to that effect if it seems likely.

Quoted 5 lines

Unlike some of the other SoC hardware (interrupt controller) that's more closely tied to the SH cpu trap behavior, the SPI master seems like something that would be nice and easy to reuse elsewhere. I don't feel strongly about it either way though; I can add the arch dep if you want.

I guess it depends if anyone is actually doing that or not, if nobody is the dependency would be better.

Quoted 10 lines

Quoted 2 lines

Why are you not using the clock API for this? Just require a clock and use clk_get_rate() to find out what rate it is.

I thought about that but I'm not familiar with it. I can try to figure it out quickly and test that approach; don't see any reason it shouldn't work. Would you insist on having full support for enabling/disabling the clk when it's in use, or would you be happy with treating it as a fixed clock that's always-on for now and possibly extending it with more functionality later if there's ever hardware where that's relevant/helpful?

It's fine to just enable it at startup and leave it on, though the runtime PM ops are trivial and you can set auto_runtime_pm to have the core do the gets and puts.

I was able to get it working via the clk api and I'll include support for this in the next version of the patch, but to actually use it depends on changing arch/sh to use the common clk framework; otherwise there's no way to provide a suitable clk in the DT and have [devm_]clk_get actually pick it up. Should I keep around the option of using clock-frequency too? That would be most convenient.

Quoted 6 lines · Rich Felker

I was able to get it working via the clk api and I'll include support for this in the next version of the patch, but to actually use it depends on changing arch/sh to use the common clk framework; otherwise there's no way to provide a suitable clk in the DT and have [devm_]clk_get actually pick it up. Should I keep around the option of using clock-frequency too? That would be most convenient.

It sounds like at the minute the devices all have one frequency anyway in which case just having a default frequency in there that you fall back to if you get -ENOENT from a failed clock lookup might be enough?

Yes. 50 MHz is the natural default frequency, but I found out at the last minute from the hardware engineers that clocking the current SoC up to 62.5 MHz (for faster cpu) will require the SPI timing to be programmed based on the faster reference clock. This messes up the ability to get optimal SD card transfer rates, so we'll probably end up having a real 50 MHz clock for the SPI anyway, but I thought it was important to be able to handle this issue in the DT binding anyway.

Quoted 13 lines · Mark Brown

Quoted 11 lines · Rich Felker

Quoted 2 lines · Mark Brown

Why are you not using the clock API for this? Just require a clock and use clk_get_rate() to find out what rate it is.

I thought about that but I'm not familiar with it. I can try to figure it out quickly and test that approach; don't see any reason it shouldn't work. Would you insist on having full support for enabling/disabling the clk when it's in use, or would you be happy with treating it as a fixed clock that's always-on for now and possibly extending it with more functionality later if there's ever hardware where that's relevant/helpful?

Quoted 3 lines

It's fine to just enable it at startup and leave it on, though the runtime PM ops are trivial and you can set auto_runtime_pm to have the core do the gets and puts.

I was able to get it working via the clk api and I'll include support for this in the next version of the patch, but to actually use it depends on changing arch/sh to use the common clk framework; otherwise there's no way to provide a suitable clk in the DT and have [devm_]clk_get actually pick it up. Should I keep around the option of using clock-frequency too? That would be most convenient.

I do have a pending patch from Sato-san to switch arch/sh over to CCF but it's part of a series and I don't think it's ready to merge. I may be able to merge just a minimal, safe subset that won't break legacy non-DT configurations, though.

Hi Rich,

Quoted 25 lines · Rich Felker

Quoted 13 lines · Mark Brown

Quoted 11 lines · Rich Felker

Quoted 2 lines · Mark Brown

Why are you not using the clock API for this? Just require a clock and use clk_get_rate() to find out what rate it is.

I thought about that but I'm not familiar with it. I can try to figure it out quickly and test that approach; don't see any reason it shouldn't work. Would you insist on having full support for enabling/disabling the clk when it's in use, or would you be happy with treating it as a fixed clock that's always-on for now and possibly extending it with more functionality later if there's ever hardware where that's relevant/helpful?

Quoted 3 lines

It's fine to just enable it at startup and leave it on, though the runtime PM ops are trivial and you can set auto_runtime_pm to have the core do the gets and puts.

I was able to get it working via the clk api and I'll include support for this in the next version of the patch, but to actually use it depends on changing arch/sh to use the common clk framework; otherwise there's no way to provide a suitable clk in the DT and have [devm_]clk_get actually pick it up. Should I keep around the option of using clock-frequency too? That would be most convenient.

Quoted 4 lines

I do have a pending patch from Sato-san to switch arch/sh over to CCF but it's part of a series and I don't think it's ready to merge. I may be able to merge just a minimal, safe subset that won't break legacy non-DT configurations, though.

I think you can use non-CCF clocks with DT, if you register them first. Cfr. the clk_names[] array and shmobile_clk_workaround() function in v3.18:arch/arm/mach-shmobile/board-koelsch-reference.c and v3.18:arch/arm/mach-shmobile/clock.c

Or was that the other way around?