| Message ID | 2e287ca758002621ef8eed3db9df37678e26af5e.1463708766.git.dalias@libc.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote: > --- /dev/null > +++ b/drivers/spi/spi-jcore.c > +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t) > +{ > + struct jcore_spi *hw = spi_master_get_devdata(master); > + > + void *ctrl_reg = hw->base + CTRL_REG; > + void *data_reg = hw->base + DATA_REG; > + int timeout; unsigned int > + int xmit; u32 > + int status; u32 > + > + /* data buffers */ > + const unsigned char *tx; > + unsigned char *rx; > + int len; unsigned int > + int count; unsigned int > + > + jcore_spi_baudrate(hw, t->speed_hz); > + > + xmit = hw->csReg | hw->speedReg | JCORE_SPI_CTRL_XMIT; > + tx = t->tx_buf; > + rx = t->rx_buf; > + len = t->len; > + > + for (count = 0; count < len; count++) { > + timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP; > + do status = readl(ctrl_reg); > + while ((status & JCORE_SPI_STAT_BUSY) && --timeout); do { ... } while (...) > + if (!timeout) break; if (...) ... > + > + writel(tx ? *tx++ : 0, data_reg); You can remove the check for tx if you set the SPI_MASTER_MUST_TX flag in spi_master.flags. > + writel(xmit, ctrl_reg); > + > + timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP; > + do status = readl(ctrl_reg); > + while ((status & JCORE_SPI_STAT_BUSY) && --timeout); do { ... } while (...) > + if (!timeout) break; if (...) ... > + > + if (rx) *rx++ = readl(data_reg); if (...) ... > + } Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote: > Signed-off-by: Rich Felker <dalias@libc.org> > --- > My previous post of the patch series accidentally omitted omitted > Cc'ing of subsystem maintainers for the necessary clocksource, > irqchip, and spi drivers. Please ack if this looks ok because I want > to get it merged as part of the arch/sh pull request for 4.7. This is *extremely* late for a first posting of a driver for v4.7 (you missed the list as well as the maintainers). > +static void jcore_spi_chipsel(struct spi_device *spi, bool value) > +{ > + struct jcore_spi *hw = spi_master_get_devdata(spi->master); > + > + pr_debug("%s: CS=%d\n", __func__, value); dev_dbg() > + > + hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS ) > + ^ (!value << 2*spi->chip_select); Why the xor here and not an or? The coding style is also weird, a mix of extra spaces around the () and missing ones around *. I'm finding the intent of the code confusing here. > +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t) Coding style, please keep lines under 80 columns unless there's a good reason. > +#if !USE_MESSAGE_MODE > + spi_finalize_current_transfer(master); > +#endif I'm not sure what the if is about but it doesn't belong upstream, you shouldn't be open coding bits of the framework. > + /* register our spi controller */ > + err = spi_register_master(master); devm_ > +static int jcore_spi_remove(struct platform_device *dev) > +{ > + struct jcore_spi *hw = platform_get_drvdata(dev); > + struct spi_master *master = hw->master; > + > + platform_set_drvdata(dev, NULL); > + spi_master_put(master); > + return 0; > +} This can be removed entirely. > +static const struct of_device_id jcore_spi_of_match[] = { > + { .compatible = "jcore,spi2" }, > + {}, > +}; This is adding a DT binding with no binding document. All new DT bindings need to be documented. > + .owner = THIS_MODULE, > + .pm = NULL, No need to set either of these.
On Fri, May 20, 2016 at 10:15:08AM +0200, Geert Uytterhoeven wrote: > On Fri, May 20, 2016 at 4:53 AM, Rich Felker <dalias@libc.org> wrote: > > --- /dev/null > > +++ b/drivers/spi/spi-jcore.c > > > +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t) > > +{ > > + struct jcore_spi *hw = spi_master_get_devdata(master); > > + > > + void *ctrl_reg = hw->base + CTRL_REG; > > + void *data_reg = hw->base + DATA_REG; > > + int timeout; > > unsigned int All of these have value ranges where the type is irrelevant, but I'm happy to change it to whatever types you prefer. > > + jcore_spi_baudrate(hw, t->speed_hz); > > + > > + xmit = hw->csReg | hw->speedReg | JCORE_SPI_CTRL_XMIT; > > + tx = t->tx_buf; > > + rx = t->rx_buf; > > + len = t->len; > > + > > + for (count = 0; count < len; count++) { > > + timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP; > > + do status = readl(ctrl_reg); > > + while ((status & JCORE_SPI_STAT_BUSY) && --timeout); > > do { > ... > } while (...) > > > + if (!timeout) break; > > if (...) > ... OK. (for this and other instances) > > + > > + writel(tx ? *tx++ : 0, data_reg); > > You can remove the check for tx if you set the SPI_MASTER_MUST_TX > flag in spi_master.flags. I don't want to do that, because the new version of the hardware that's going to support DMA can only do unidirectional DMA, and thus we need to be able to see if either tx or rx is null. Rich -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 20, 2016 at 11:23:17AM +0100, Mark Brown wrote: > On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote: > > Signed-off-by: Rich Felker <dalias@libc.org> > > --- > > My previous post of the patch series accidentally omitted omitted > > Cc'ing of subsystem maintainers for the necessary clocksource, > > irqchip, and spi drivers. Please ack if this looks ok because I want > > to get it merged as part of the arch/sh pull request for 4.7. > > This is *extremely* late for a first posting of a driver for v4.7 (you > missed the list as well as the maintainers). I'm sorry for the mix-up. The kernel workflow is still fairly new to me and I've been fighting with git format-patch/send-email and get_maintainer.pl to get big patch series prepared the way I want and sent to the right people/lists. I think I've got it right now though. > > +static void jcore_spi_chipsel(struct spi_device *spi, bool value) > > +{ > > + struct jcore_spi *hw = spi_master_get_devdata(spi->master); > > + > > + pr_debug("%s: CS=%d\n", __func__, value); > > dev_dbg() OK. > > + hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS ) > > + ^ (!value << 2*spi->chip_select); > > Why the xor here and not an or? The coding style is also weird, a mix > of extra spaces around the () and missing ones around *. I'm finding > the intent of the code confusing here. The intent is to set all chipselect bits (the 3 macro values) high except possibly spi->chip_select. The xor is to turn off a bit, not turn it on. &~ would also have worked; would that be more idiomatic? Since the individual CS bits and their names in the hardware aren't actually relevant to the driver, I'm replacing them with a single: #define JCORE_SPI_CTRL_CS_BITS 0x15 so I can just write: hw->csReg = JCORE_SPI_CTRL_CS_BITS ^ (!value << 2*spi->chip_select); Does that look better, or should I opt for &~? > > +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t) > > Coding style, please keep lines under 80 columns unless there's a good > reason. OK. > > +#if !USE_MESSAGE_MODE > > + spi_finalize_current_transfer(master); > > +#endif > > I'm not sure what the if is about but it doesn't belong upstream, you > shouldn't be open coding bits of the framework. I can explain the motivation and see what you think is best to do. I'd like to be able to provide just the transfer_one operation, and use the generic transfer_one_message. However, at 50 MHz cpu clock, the stats collection and other overhead in spi.c's generic spi_transfer_one_message takes up so much time between the end of one SD card transfer and the beginning of the next that the overall transfer rate is something like 15-20% higher with my version. Another consideration is that DMA support is being added to the hardware right now, and I think we're going to want to be able to queue up whole messages for DMA rather than just individual transfers; in that case, spi_transfer_one_message is probably not suitable anyway. So we'll probably have to end up having our own transfer_one_message function anyway. If that's not actually needed, a possible alternative for fixing the performance problem might be adding a Kconfig option to turn off all the unnecessary overhead (stats, etc.) in spi_transfer_one_message. I could work on that instead (or in addition), and I wouldn't consider it critical to get in for this merge window. > > + /* register our spi controller */ > > + err = spi_register_master(master); > > devm_ > > > +static int jcore_spi_remove(struct platform_device *dev) > > +{ > > + struct jcore_spi *hw = platform_get_drvdata(dev); > > + struct spi_master *master = hw->master; > > + > > + platform_set_drvdata(dev, NULL); > > + spi_master_put(master); > > + return 0; > > +} > > This can be removed entirely. OK. Does using the devm register function cause removal to be handled generically, or is there another reason it's not needed? > > +static const struct of_device_id jcore_spi_of_match[] = { > > + { .compatible = "jcore,spi2" }, > > + {}, > > +}; > > This is adding a DT binding with no binding document. All new DT > bindings need to be documented. The DT binding was in patch 05/12. Should linux-spi have been added to the Cc list for it? get_maintainer.pl didn't include it. > > + .owner = THIS_MODULE, > > + .pm = NULL, > > No need to set either of these. OK. Rich -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, May 20, 2016 at 07:24:14PM -0400, Rich Felker wrote: > On Fri, May 20, 2016 at 11:23:17AM +0100, Mark Brown wrote: > > This is *extremely* late for a first posting of a driver for v4.7 (you > > missed the list as well as the maintainers). > I'm sorry for the mix-up. The kernel workflow is still fairly new to > me and I've been fighting with git format-patch/send-email and > get_maintainer.pl to get big patch series prepared the way I want and > sent to the right people/lists. I think I've got it right now though. One question here is why this is even part of a series - it's adding a new controller driver which wouldn't normally have any sort of direct build or other dependency on anything else or have other things depending on it. Unless there are dependencies it is generally best to send separate changes separately as far as possible so that there is no need to worry about issues with one part of the series slowing down other parts of the series. If it does make sense to send a series you need to communicate what's going on with dependencies with all the maintainers, normally at least the cover letter if not the entire series should go to everyone. You should never rely on get_maintainers, you need to think about what it's telling you. It both misses people and generates false positives (especially when it looks at git history). It's useful to look at since it normally gets you most of the way there but > > > + hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS ) > > > + ^ (!value << 2*spi->chip_select); > > Why the xor here and not an or? The coding style is also weird, a mix > > of extra spaces around the () and missing ones around *. I'm finding > > the intent of the code confusing here. > The intent is to set all chipselect bits (the 3 macro values) high > except possibly spi->chip_select. The xor is to turn off a bit, not > turn it on. &~ would also have worked; would that be more idiomatic? No, the reader has to be able to tell what the code is doing. If this made sense the thing to do would be to write out the logic operations explicitly to make it clear that every step is deliberate. However in this case it sounds like the code is just plain buggy, though - the driver is being asked to set a specific chip select to a specific value but instead of just doing that it is also trying to also change some other settings. Don't do that, just have the function do what it was asked. If the calling code has problems it'll need fixing anyway and if some feature you hadn't anticipated ends up meaning the combination of operations makes sense then things will just work. > > > +#if !USE_MESSAGE_MODE > > > + spi_finalize_current_transfer(master); > > > +#endif > > I'm not sure what the if is about but it doesn't belong upstream, you > > shouldn't be open coding bits of the framework. > I can explain the motivation and see what you think is best to do. I'd > like to be able to provide just the transfer_one operation, and use > the generic transfer_one_message. However, at 50 MHz cpu clock, the > stats collection and other overhead in spi.c's generic > spi_transfer_one_message takes up so much time between the end of one > SD card transfer and the beginning of the next that the overall > transfer rate is something like 15-20% higher with my version. No, this is just not a good approach. If the generic code isn't working for you improve the generic code, don't just copy it and open code the bits you like. The reason Linux has all these great framework is that people collaborate to make the generic code as good and fully featured as they can rather than open coding everything in drivers. Doing things in drivers results in lots of code duplication which increases the cost of maintaining things and requires that everyone waste time copying code in order to keep the feature set consistent between drivers. Drivers should do things specific to a given piece of hardware, anything in the generic code should be dealt with in the generic code. > Another consideration is that DMA support is being added to the > hardware right now, and I think we're going to want to be able to > queue up whole messages for DMA rather than just individual transfers; > in that case, spi_transfer_one_message is probably not suitable > anyway. So we'll probably have to end up having our own > transfer_one_message function anyway. This is again a simple generic optimisation which would be best implemented in generic code - it's not just this device which would benefit from the ability to coalesce compatible transfers. There is no reason for individual drivers to even think about such an optimisation, the core should just be handling them a transfer with a coalesced DMA transfer in it. We're not doing it at present because someone needs to take the time to write the code that figures out if adjacent transfers are compatible and merges them if they are. > If that's not actually needed, a possible alternative for fixing the > performance problem might be adding a Kconfig option to turn off all > the unnecessary overhead (stats, etc.) in spi_transfer_one_message. I > could work on that instead (or in addition), and I wouldn't consider > it critical to get in for this merge window. This driver is *not* going in during this merge window, sorry. You need to get code into maintainer trees before the merge window opens but this was only sent to maintainers after the merge window was already open. This isn't the end of the world, there will be another kernel release in a few months. > > > + platform_set_drvdata(dev, NULL); > > > + spi_master_put(master); > > > + return 0; > > > +} > > This can be removed entirely. > OK. Does using the devm register function cause removal to be handled > generically, or is there another reason it's not needed? Yes. > > > +static const struct of_device_id jcore_spi_of_match[] = { > > > + { .compatible = "jcore,spi2" }, > > > + {}, > > > +}; > > This is adding a DT binding with no binding document. All new DT > > bindings need to be documented. > The DT binding was in patch 05/12. Should linux-spi have been added to > the Cc list for it? get_maintainer.pl didn't include it. Yes, the documentation and the code need to be reviewed together. It's hard to tell if the code implements the bindings without seeing both.
On Mon, May 23, 2016 at 04:30:37PM +0100, Mark Brown wrote: > On Fri, May 20, 2016 at 07:24:14PM -0400, Rich Felker wrote: > > On Fri, May 20, 2016 at 11:23:17AM +0100, Mark Brown wrote: > > > > This is *extremely* late for a first posting of a driver for v4.7 (you > > > missed the list as well as the maintainers). > > > I'm sorry for the mix-up. The kernel workflow is still fairly new to > > me and I've been fighting with git format-patch/send-email and > > get_maintainer.pl to get big patch series prepared the way I want and > > sent to the right people/lists. I think I've got it right now though. > > One question here is why this is even part of a series - it's adding a > new controller driver which wouldn't normally have any sort of direct > build or other dependency on anything else or have other things > depending on it. Unless there are dependencies it is generally best to > send separate changes separately as far as possible so that there is no > need to worry about issues with one part of the series slowing down > other parts of the series. I grouped the patches as a series because they make up support for a complete SoC. While some of the peripheral drivers may well be useful for non-J2 systems in the future (the cores are all open source, BSD licensed), there's no short-term benefit to having these drivers without the main J2 support. In terms of hard dependencies, the mimas_v2 dts file depends on the jcore,spi binding, but nothing depends on the driver. Still it's important to have for the sake of actually making use of it all. > > > > + hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS ) > > > > + ^ (!value << 2*spi->chip_select); > > > > Why the xor here and not an or? The coding style is also weird, a mix > > > of extra spaces around the () and missing ones around *. I'm finding > > > the intent of the code confusing here. > > > The intent is to set all chipselect bits (the 3 macro values) high > > except possibly spi->chip_select. The xor is to turn off a bit, not > > turn it on. &~ would also have worked; would that be more idiomatic? > > No, the reader has to be able to tell what the code is doing. If this > made sense the thing to do would be to write out the logic operations > explicitly to make it clear that every step is deliberate. However in > this case it sounds like the code is just plain buggy, though - the > driver is being asked to set a specific chip select to a specific value > but instead of just doing that it is also trying to also change some > other settings. It may be redundant, if the general SPI framework handles mutual exclusion of chipselects, but it's not buggy. Only a single chipselect can be active (low) at once; with multiple chips selected very bad things will happen. I don't see any documentation of the high-level SPI framework in Linux and what (if anything) it does to ensure that all other chipselects are disabled before enabling one, which I why I wrote the code so that the other chipselects are explicitly disabled. > Don't do that, just have the function do what it was > asked. If the calling code has problems it'll need fixing anyway and if > some feature you hadn't anticipated ends up meaning the combination of > operations makes sense then things will just work. Setting just a single bit seems more complicated and error-prone to me; explicit effort has to be made to ensure that the hardware state remains in sync with the driver's view of it. As written now, the whole register (with all the cs/speed/etc. bits) is just written every time. > > > > +#if !USE_MESSAGE_MODE > > > > + spi_finalize_current_transfer(master); > > > > +#endif > > > > I'm not sure what the if is about but it doesn't belong upstream, you > > > shouldn't be open coding bits of the framework. > > > I can explain the motivation and see what you think is best to do. I'd > > like to be able to provide just the transfer_one operation, and use > > the generic transfer_one_message. However, at 50 MHz cpu clock, the > > stats collection and other overhead in spi.c's generic > > spi_transfer_one_message takes up so much time between the end of one > > SD card transfer and the beginning of the next that the overall > > transfer rate is something like 15-20% higher with my version. > > No, this is just not a good approach. If the generic code isn't working > for you improve the generic code, don't just copy it and open code the > bits you like. The reason Linux has all these great framework is that > people collaborate to make the generic code as good and fully featured > as they can rather than open coding everything in drivers. Doing things > in drivers results in lots of code duplication which increases the cost > of maintaining things and requires that everyone waste time copying code > in order to keep the feature set consistent between drivers. I totally agree with this principle, and I'm fine with dropping the offending code. I may maintain it out-of-tree as a performance patch for a while, but DMA and other improvements will hopefully make it unnecessary. > > Another consideration is that DMA support is being added to the > > hardware right now, and I think we're going to want to be able to > > queue up whole messages for DMA rather than just individual transfers; > > in that case, spi_transfer_one_message is probably not suitable > > anyway. So we'll probably have to end up having our own > > transfer_one_message function anyway. > > This is again a simple generic optimisation which would be best > implemented in generic code - it's not just this device which would > benefit from the ability to coalesce compatible transfers. There is no > reason for individual drivers to even think about such an optimisation, > the core should just be handling them a transfer with a coalesced DMA > transfer in it. We're not doing it at present because someone needs to > take the time to write the code that figures out if adjacent transfers > are compatible and merges them if they are. OK, then let's go with that approach. As long as there's a viable way forward for doing it in a way that benefits all devices/drivers, I prefer it anyway. I just didn't want to hit a situation where I later propose that and a mainainer (you or someone else) tells me it's not possible or not necessary. Rich -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 23, 2016 at 04:29:38PM -0400, Rich Felker wrote: > On Mon, May 23, 2016 at 04:30:37PM +0100, Mark Brown wrote: > > One question here is why this is even part of a series - it's adding a > > new controller driver which wouldn't normally have any sort of direct > > build or other dependency on anything else or have other things > > depending on it. Unless there are dependencies it is generally best to > > send separate changes separately as far as possible so that there is no > > need to worry about issues with one part of the series slowing down > > other parts of the series. > I grouped the patches as a series because they make up support for a > complete SoC. While some of the peripheral drivers may well be useful > for non-J2 systems in the future (the cores are all open source, BSD > licensed), there's no short-term benefit to having these drivers > without the main J2 support. That's what -next is for, merging everyone's trees together to give something to test. Practically speaking most maintainence is done at the build level, it's how we do all the other SoCs so that people can see what's going on at the subsystem level. > > No, the reader has to be able to tell what the code is doing. If this > > made sense the thing to do would be to write out the logic operations > > explicitly to make it clear that every step is deliberate. However in > > this case it sounds like the code is just plain buggy, though - the > > driver is being asked to set a specific chip select to a specific value > > but instead of just doing that it is also trying to also change some > > other settings. > It may be redundant, if the general SPI framework handles mutual > exclusion of chipselects, but it's not buggy. Only a single chipselect > can be active (low) at once; with multiple chips selected very bad > things will happen. I don't see any documentation of the high-level > SPI framework in Linux and what (if anything) it does to ensure that > all other chipselects are disabled before enabling one, which I why I > wrote the code so that the other chipselects are explicitly disabled. There is no such guarantee because there are applications where it makes sense to write to multiple chips simultaneously - this is one common way of doing simultaneous updates over multiple audio audio CODECs to ensure synchronization for example. It's also just not something that it makes any sense to worry about at the indivudal driver level. The generic code is responsible for ensuring that things work well, writing bodges that silently try to work around the generic code is always a recipie for fragility, especially if that code is hard to understand. Either the driver was making unwarranted assumptions that break use cases it didn't think of or we end up having to find and fix issues multiple times due to duplication.
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 9d8c84b..5bd2ccf 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -285,6 +285,10 @@ config SPI_IMX This enables using the Freescale i.MX SPI controllers in master mode. +config SPI_JCORE + tristate "J-Core SPI Master" + depends on OF + config SPI_LM70_LLP tristate "Parallel port adapter for LM70 eval board (DEVELOPMENT)" depends on PARPORT diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index fbb255c..6a34124 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -46,6 +46,7 @@ obj-$(CONFIG_SPI_FSL_SPI) += spi-fsl-spi.o obj-$(CONFIG_SPI_GPIO) += spi-gpio.o obj-$(CONFIG_SPI_IMG_SPFI) += spi-img-spfi.o obj-$(CONFIG_SPI_IMX) += spi-imx.o +obj-$(CONFIG_SPI_JCORE) += spi-jcore.o obj-$(CONFIG_SPI_LM70_LLP) += spi-lm70llp.o obj-$(CONFIG_SPI_LP8841_RTC) += spi-lp8841-rtc.o obj-$(CONFIG_SPI_MESON_SPIFC) += spi-meson-spifc.o diff --git a/drivers/spi/spi-jcore.c b/drivers/spi/spi-jcore.c new file mode 100644 index 0000000..552304c --- /dev/null +++ b/drivers/spi/spi-jcore.c @@ -0,0 +1,266 @@ +/* + * J-Core SPI controller driver + * + * Copyright (C) 2012-2016 SEI Inc. + * + * Current version by Rich Felker + * Based loosely on initial version by Oleksandr G Zhadan + * + */ +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/errno.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/spi/spi.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/delay.h> + +#define USE_MESSAGE_MODE 1 + +#define DRV_NAME "jcore_spi" + +#define MAX_SPI_SPEED 12500000 /* 12.5 MHz */ + +#define CTRL_REG 0x0 +#define DATA_REG 0x4 + +#define SPI_NOCHIP_CS 0 +#define SPI_FLASH_CS 1 +#define SPI_CONF_CS 2 +#define SPI_SD_CS 2 +#define SPI_CODEC_CS 3 + +#define JCORE_SPI_CTRL_ACS 0x01 +#define JCORE_SPI_CTRL_XMIT 0x02 +#define JCORE_SPI_STAT_BUSY 0x02 +#define JCORE_SPI_CTRL_CCS 0x04 +#define JCORE_SPI_CTRL_LOOP 0x08 +#define JCORE_SPI_CTRL_DCS 0x10 + +#define JCORE_SPI_WAIT_RDY_MAX_LOOP 2000000 /* in usec */ + +struct jcore_spi { + struct spi_master *master; + void __iomem *base; + volatile unsigned int ctrlReg; + unsigned int csReg; + unsigned int speedReg; + unsigned int speed_hz; +}; + +static void jcore_spi_wait_till_ready(struct jcore_spi *hw, int timeout) +{ + while (timeout--) { + hw->ctrlReg = readl(hw->base + CTRL_REG); + if (!(hw->ctrlReg & JCORE_SPI_STAT_BUSY)) + return; + cpu_relax(); + } + pr_err("%s: Timeout..\n", __func__); +} + +static void jcore_spi_program(struct jcore_spi *hw) +{ + jcore_spi_wait_till_ready(hw, JCORE_SPI_WAIT_RDY_MAX_LOOP); + writel(hw->csReg | hw->speedReg, hw->base + CTRL_REG); +} + +static void jcore_spi_chipsel(struct spi_device *spi, bool value) +{ + struct jcore_spi *hw = spi_master_get_devdata(spi->master); + + pr_debug("%s: CS=%d\n", __func__, value); + + hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS ) + ^ (!value << 2*spi->chip_select); + + jcore_spi_program(hw); +} + +static void jcore_spi_baudrate(struct jcore_spi *hw, int speed) +{ + if (speed == hw->speed_hz) return; + hw->speed_hz = speed; + hw->speedReg = ((MAX_SPI_SPEED / speed) - 1) << 27; + jcore_spi_program(hw); + pr_debug("%s: speed=%d pre=0x%x\n", __func__, speed, hw->speedReg); +} + +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t) +{ + struct jcore_spi *hw = spi_master_get_devdata(master); + + void *ctrl_reg = hw->base + CTRL_REG; + void *data_reg = hw->base + DATA_REG; + int timeout; + int xmit; + int status; + + /* data buffers */ + const unsigned char *tx; + unsigned char *rx; + int len; + int count; + + jcore_spi_baudrate(hw, t->speed_hz); + + xmit = hw->csReg | hw->speedReg | JCORE_SPI_CTRL_XMIT; + tx = t->tx_buf; + rx = t->rx_buf; + len = t->len; + + for (count = 0; count < len; count++) { + timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP; + do status = readl(ctrl_reg); + while ((status & JCORE_SPI_STAT_BUSY) && --timeout); + if (!timeout) break; + + writel(tx ? *tx++ : 0, data_reg); + writel(xmit, ctrl_reg); + + timeout = JCORE_SPI_WAIT_RDY_MAX_LOOP; + do status = readl(ctrl_reg); + while ((status & JCORE_SPI_STAT_BUSY) && --timeout); + if (!timeout) break; + + if (rx) *rx++ = readl(data_reg); + } + +#if !USE_MESSAGE_MODE + spi_finalize_current_transfer(master); +#endif + + return count<len ? -EREMOTEIO : 0; +} + +#if USE_MESSAGE_MODE +static int jcore_spi_transfer_one_message(struct spi_master *master, + struct spi_message *msg) +{ + struct spi_device *spi = msg->spi; + struct spi_transfer *xfer; + bool keep_cs = false; + int ret = 0; + + jcore_spi_chipsel(spi, false); + + list_for_each_entry(xfer, &msg->transfers, transfer_list) { + ret = jcore_spi_txrx(master, spi, xfer); + if (ret) break; + if (xfer->delay_usecs) + udelay(xfer->delay_usecs); + if (xfer->cs_change) { + if (list_is_last(&xfer->transfer_list, + &msg->transfers)) { + keep_cs = true; + } else { + jcore_spi_chipsel(spi, true); + udelay(10); + jcore_spi_chipsel(spi, false); + } + } + msg->actual_length += xfer->len; + } + + if (!keep_cs) + jcore_spi_chipsel(spi, true); + + msg->status = ret; + + spi_finalize_current_message(master); + + return ret; +} +#endif + +static int jcore_spi_probe(struct platform_device *pdev) +{ + struct device_node *node = pdev->dev.of_node; + struct jcore_spi *hw; + struct spi_master *master; + struct resource *res; + int err = -ENODEV; + + master = spi_alloc_master(&pdev->dev, sizeof(struct jcore_spi)); + if (!master) + return err; + + /* setup the master state. */ + master->num_chipselect = 3; + master->mode_bits = SPI_MODE_3; +#if USE_MESSAGE_MODE + master->transfer_one_message = jcore_spi_transfer_one_message; +#else + master->transfer_one = jcore_spi_txrx; +#endif + master->set_cs = jcore_spi_chipsel; + master->dev.of_node = node; + + hw = spi_master_get_devdata(master); + hw->master = master; + platform_set_drvdata(pdev, hw); + + /* find and map our resources */ + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + goto exit_busy; + if (!devm_request_mem_region + (&pdev->dev, res->start, resource_size(res), pdev->name)) + goto exit_busy; + hw->base = + devm_ioremap_nocache(&pdev->dev, res->start, resource_size(res)); + if (!hw->base) + goto exit_busy; + + jcore_spi_baudrate(hw, 400000); + + pdev->dev.dma_mask = 0; + /* register our spi controller */ + err = spi_register_master(master); + if (err) + goto exit; + dev_info(&pdev->dev, "base %p, noirq\n", hw->base); + + return 0; + +exit_busy: + err = -EBUSY; +exit: + platform_set_drvdata(pdev, NULL); + spi_master_put(master); + return err; +} + +static int jcore_spi_remove(struct platform_device *dev) +{ + struct jcore_spi *hw = platform_get_drvdata(dev); + struct spi_master *master = hw->master; + + platform_set_drvdata(dev, NULL); + spi_master_put(master); + return 0; +} + +static const struct of_device_id jcore_spi_of_match[] = { + { .compatible = "jcore,spi2" }, + {}, +}; + +static struct platform_driver jcore_spi_driver = { + .probe = jcore_spi_probe, + .remove = jcore_spi_remove, + .driver = { + .name = DRV_NAME, + .owner = THIS_MODULE, + .pm = NULL, + .of_match_table = jcore_spi_of_match, + }, +}; + +module_platform_driver(jcore_spi_driver); + +MODULE_DESCRIPTION("J-Core SPI driver"); +MODULE_AUTHOR("Rich Felker <dalias@libc.org>"); +MODULE_ALIAS("platform:" DRV_NAME);
Signed-off-by: Rich Felker <dalias@libc.org> --- My previous post of the patch series accidentally omitted omitted Cc'ing of subsystem maintainers for the necessary clocksource, irqchip, and spi drivers. Please ack if this looks ok because I want to get it merged as part of the arch/sh pull request for 4.7. drivers/spi/Kconfig | 4 + drivers/spi/Makefile | 1 + drivers/spi/spi-jcore.c | 266 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 271 insertions(+) create mode 100644 drivers/spi/spi-jcore.c