Date: Tue, 18 Aug 2015 22:47:52 +0000 From: Brian Fundakowski Feldman <brianfundakowskifeldman@gmail.com> To: Tom Jones <jones@sdf.org> Cc: embedded@freebsd.org Subject: Re: spigen(4) SPI Generic IO interface -- need comments Message-ID: <CAEv1%2BOUhSAJxxWAfW2GUFVw=H-_KOs2dGg2d7uhZnFbqsHE5Qw@mail.gmail.com> In-Reply-To: <20150817160423.GB3078@gmail.com> References: <CAEv1%2BOU4cFpMpeQGfnCP7L4Q_k18rOSOA9JBnKUa99DS5dFnWA@mail.gmail.com> <20150817160423.GB3078@gmail.com>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] On Mon, Aug 17, 2015 at 12:04 PM Tom Jones <jones@sdf.org> wrote: > On Mon, Aug 17, 2015 at 10:00:26AM -0400, Brian Fundakowski Feldman wrote: > > I'm woefully out-of-practice with my kernel hackery (but still pretty > > proficient in jiggery-pokery) so I would like to get comments on a little > > driver I just made for interfacing arbitrarily in userland with SPI > > components. The only thing I'm exposing is a /dev/spigenN node with a > > single transfer ioctl and I put together a test circuit and program with > an > > MCP3008 10-bit ADC IC to validate that it basically works, other than the > > limitation that the transfers must be octet-multiply-sized, but I haven't > > looked at the SoC's (I'm using a Raspberry Pi 2) data sheet to tell > whether > > that's just a limit on the spibus(4) interface or the Broadcom SPI driver > > or the Broadcom SoC itself. > > > > I hit one snag in development where I simply called the ioctl wrong and > > found copyin(9) to page fault HARD if given a bogus user address to copy > > from, and panic the kernel. I can post up the test program if anyone > wants > > but it's very trivial: I just align the start bit and the command data > into > > the least significant bits of the first octet, shift it up two positions > so > > the NULs get clocked out as part of the command field, and provide two > > octets for the data field to retrieve back the 10-bit digital value. > > Oh, cool. > > I did the same earlier this year, have you seen[1]?. > > The FreeBSD i2c api is the same/very similar the linux one[2][3]. Have you > considered adding some of the ioctls[3] or the data structures to make it > easier to port code? > > [1]: > https://lists.freebsd.org/pipermail/freebsd-embedded/2015-April/002466.html > [2]: https://www.kernel.org/doc/Documentation/i2c/dev-interface > [3]: > https://www.freebsd.org/cgi/man.cgi?query=iic&apropos=0&sektion=0&manpath=FreeBSD+10.2-RELEASE&arch=default&format=html > [4]: https://www.kernel.org/doc/Documentation/spi/spidev I've iterated a bit on this to try to make some more sensible API, behaving reasonably about being able to set the SPI clock speed. I'm going to implement an mmap handler so I can have my low-latency operation mode, as well. I don't like the Linux APIs one bit because it's just not safe to allow all those configuration changes on a per-transfer basis... Moving this to -embedded because it's more apt than -hackers. [-- Attachment #2 --] Index: arm/broadcom/bcm2835/bcm2835_spi.c =================================================================== --- arm/broadcom/bcm2835/bcm2835_spi.c (revision 286890) +++ arm/broadcom/bcm2835/bcm2835_spi.c (working copy) @@ -107,6 +107,25 @@ BCM_SPI_WRITE(sc, off, reg); } +/* + * Set the clock speed register and return the clock speed actually used, + * after corrections to fit within SPI_CORE_CLK. + */ +static uint32_t +bcm_spi_set_clock_speed(struct bcm_spi_softc *sc, const uint32_t clock_speed) +{ + uint32_t clk = SPI_CORE_CLK / clock_speed; + + if (clk <= 1) + clk = 2; + else if (clk % 2) + clk--; + if (clk > 0xffff) + clk = 0; + BCM_SPI_WRITE(sc, SPI_CLK, clk); + return (clk == 0 ? 0 : SPI_CORE_CLK / clk); +} + static int bcm_spi_clock_proc(SYSCTL_HANDLER_ARGS) { @@ -117,26 +136,19 @@ sc = (struct bcm_spi_softc *)arg1; BCM_SPI_LOCK(sc); - clk = BCM_SPI_READ(sc, SPI_CLK); + clk = sc->sc_clock_speed; BCM_SPI_UNLOCK(sc); - clk &= 0xffff; - if (clk == 0) - clk = 65536; - clk = SPI_CORE_CLK / clk; error = sysctl_handle_int(oidp, &clk, sizeof(clk), req); if (error != 0 || req->newptr == NULL) return (error); - clk = SPI_CORE_CLK / clk; - if (clk <= 1) - clk = 2; - else if (clk % 2) - clk--; - if (clk > 0xffff) - clk = 0; BCM_SPI_LOCK(sc); - BCM_SPI_WRITE(sc, SPI_CLK, clk); + if (sc->sc_flags & BCM_SPI_BUSY) { + BCM_SPI_UNLOCK(sc); + return (EBUSY); + } + sc->sc_clock_speed = bcm_spi_set_clock_speed(sc, clk); BCM_SPI_UNLOCK(sc); return (0); @@ -310,7 +322,8 @@ BCM_SPI_WRITE(sc, SPI_CS, SPI_CS_CLEAR_RXFIFO | SPI_CS_CLEAR_TXFIFO); /* Set the SPI clock to 500Khz. */ - BCM_SPI_WRITE(sc, SPI_CLK, SPI_CORE_CLK / 500000); + sc->sc_clock_speed = 500000; + BCM_SPI_WRITE(sc, SPI_CLK, SPI_CORE_CLK / sc->sc_clock_speed); #ifdef BCM_SPI_DEBUG bcm_spi_printr(dev); @@ -418,6 +431,7 @@ bcm_spi_transfer(device_t dev, device_t child, struct spi_command *cmd) { struct bcm_spi_softc *sc; + const uint32_t clock_speed_hz = cmd->clock_speed_hz; int cs, err; sc = device_get_softc(dev); @@ -450,6 +464,10 @@ SPI_CS_CLEAR_RXFIFO | SPI_CS_CLEAR_TXFIFO, SPI_CS_CLEAR_RXFIFO | SPI_CS_CLEAR_TXFIFO); + /* Switch clock speed if necessary. */ + if (clock_speed_hz != 0 && clock_speed_hz != sc->sc_clock_speed) + bcm_spi_set_clock_speed(sc, clock_speed_hz); + /* Save a pointer to the SPI command. */ sc->sc_cmd = cmd; sc->sc_read = 0; @@ -470,6 +488,10 @@ /* Make sure the SPI engine and interrupts are disabled. */ bcm_spi_modifyreg(sc, SPI_CS, SPI_CS_TA | SPI_CS_INTR | SPI_CS_INTD, 0); + /* Switch the clock speed back if necessary. */ + if (clock_speed_hz != 0 && clock_speed_hz != sc->sc_clock_speed) + bcm_spi_set_clock_speed(sc, sc->sc_clock_speed); + /* Release the controller and wakeup the next thread waiting for it. */ sc->sc_flags = 0; wakeup_one(dev); @@ -487,6 +509,7 @@ return (err); } + static phandle_t bcm_spi_get_node(device_t bus, device_t dev) { Index: arm/broadcom/bcm2835/bcm2835_spivar.h =================================================================== --- arm/broadcom/bcm2835/bcm2835_spivar.h (revision 286890) +++ arm/broadcom/bcm2835/bcm2835_spivar.h (working copy) @@ -54,6 +54,7 @@ uint32_t sc_read; uint32_t sc_flags; uint32_t sc_written; + uint32_t sc_clock_speed; void * sc_intrhand; }; Index: arm/conf/RPI2 =================================================================== --- arm/conf/RPI2 (revision 286890) +++ arm/conf/RPI2 (working copy) @@ -24,7 +24,7 @@ include "../broadcom/bcm2835/std.rpi" include "../broadcom/bcm2835/std.bcm2836" -options HZ=100 +options HZ=1000 options SCHED_ULE # ULE scheduler options SMP # Enable multiple cores options PLATFORM @@ -35,7 +35,7 @@ #options VERBOSE_SYSINIT # Enable verbose sysinit messages options KDB # Enable kernel debugger support # For minimum debugger support (stable branch) use: -#options KDB_TRACE # Print a stack trace for a panic +options KDB_TRACE # Print a stack trace for a panic # For full debugger support use this instead: options DDB # Enable the kernel debugger options INVARIANTS # Enable calls of extra sanity checking Index: arm/lpc/ssd1289.c =================================================================== --- arm/lpc/ssd1289.c (revision 286890) +++ arm/lpc/ssd1289.c (working copy) @@ -157,7 +157,8 @@ static __inline void ssd1289_spi_send(struct ssd1289_softc *sc, uint8_t *data, int len) { - struct spi_command cmd; + struct spi_command cmd = SPI_COMMAND_INITIALIZER; + uint8_t buffer[8]; cmd.tx_cmd = data; cmd.tx_cmd_sz = len; Index: conf/files =================================================================== --- conf/files (revision 286890) +++ conf/files (working copy) @@ -2452,6 +2452,7 @@ dev/spibus/ofw_spibus.c optional fdt spibus dev/spibus/spibus.c optional spibus \ dependency "spibus_if.h" +dev/spibus/spigen.c optional spibus dev/spibus/spibus_if.m optional spibus dev/ste/if_ste.c optional ste pci dev/stg/tmc18c30.c optional stg Index: dev/spibus/spi.h =================================================================== --- dev/spibus/spi.h (revision 286890) +++ dev/spibus/spi.h (working copy) @@ -27,6 +27,7 @@ */ struct spi_command { + uint32_t clock_speed_hz; void *tx_cmd; uint32_t tx_cmd_sz; void *rx_cmd; @@ -37,4 +38,6 @@ uint32_t rx_data_sz; }; +#define SPI_COMMAND_INITIALIZER { 0 } + #define SPI_CHIP_SELECT_HIGH 0x1 /* Chip select high (else low) */ Index: dev/spibus/spigen.c =================================================================== --- dev/spibus/spigen.c (revision 0) +++ dev/spibus/spigen.c (working copy) @@ -0,0 +1,279 @@ +/*- + * Copyright (c) 2015 Brian Fundakowski Feldman. All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +#include <sys/cdefs.h> +__FBSDID("$FreeBSD$"); + +#include <sys/param.h> +#include <sys/systm.h> +#include <sys/bus.h> +#include <sys/conf.h> +#include <sys/kernel.h> +#include <sys/lock.h> +#include <sys/malloc.h> +#include <sys/mutex.h> +#include <sys/module.h> +#include <sys/spigenio.h> +#include <sys/sysctl.h> + +#include <dev/spibus/spi.h> + +#include "spibus_if.h" + +struct spigen_softc { + device_t sc_dev; + struct cdev *sc_cdev; + struct mtx sc_mtx; + uint32_t sc_clock_speed; + uint32_t sc_command_length_max; + uint32_t sc_data_length_max; + void *sc_command_data_area; +}; + +static int +spigen_probe(device_t dev) +{ + device_set_desc(dev, "SPI Generic IO"); + return (0); +} + +static int spigen_open(struct cdev *, int, int, struct thread *); +static int spigen_ioctl(struct cdev *, u_long, caddr_t, int, struct thread *); +static int spigen_close(struct cdev *, int, int, struct thread *); + +static struct cdevsw spigen_cdevsw = { + .d_version = D_VERSION, + .d_name = "spigen", + .d_open = spigen_open, + .d_ioctl = spigen_ioctl, + .d_close = spigen_close +}; + +static int +spigen_command_length_max_proc(SYSCTL_HANDLER_ARGS) +{ + struct spigen_softc *sc = (struct spigen_softc *)arg1; + uint32_t command_length_max; + int error; + + mtx_lock(&sc->sc_mtx); + command_length_max = sc->sc_command_length_max; + mtx_unlock(&sc->sc_mtx); + error = sysctl_handle_int(oidp, &command_length_max, + sizeof(command_length_max), req); + if (error == 0 && req->newptr != NULL) { + mtx_lock(&sc->sc_mtx); + /* XXX EBUSY when mmapped test */ + sc->sc_command_length_max = command_length_max; + mtx_unlock(&sc->sc_mtx); + } + return (error); +} + +static int +spigen_data_length_max_proc(SYSCTL_HANDLER_ARGS) +{ + struct spigen_softc *sc = (struct spigen_softc *)arg1; + uint32_t data_length_max; + int error; + + mtx_lock(&sc->sc_mtx); + data_length_max = sc->sc_data_length_max; + mtx_unlock(&sc->sc_mtx); + error = sysctl_handle_int(oidp, &data_length_max, + sizeof(data_length_max), req); + if (error == 0 && req->newptr != NULL) { + mtx_lock(&sc->sc_mtx); + /* XXX EBUSY when mmapped test */ + sc->sc_data_length_max = data_length_max; + mtx_unlock(&sc->sc_mtx); + } + return (error); +} + +static void +spigen_sysctl_init(struct spigen_softc *sc) +{ + struct sysctl_ctx_list *ctx; + struct sysctl_oid *tree_node; + struct sysctl_oid_list *tree; + + /* + * Add system sysctl tree/handlers. + */ + ctx = device_get_sysctl_ctx(sc->sc_dev); + tree_node = device_get_sysctl_tree(sc->sc_dev); + tree = SYSCTL_CHILDREN(tree_node); + SYSCTL_ADD_PROC(ctx, tree, OID_AUTO, "command_length_max", + CTLFLAG_MPSAFE | CTLFLAG_RW | CTLTYPE_UINT, sc, sizeof(*sc), + spigen_command_length_max_proc, "IU", "SPI command header portion (octets)"); + SYSCTL_ADD_PROC(ctx, tree, OID_AUTO, "data_length_max", + CTLFLAG_MPSAFE | CTLFLAG_RW | CTLTYPE_UINT, sc, sizeof(*sc), + spigen_data_length_max_proc, "IU", "SPI data trailer portion (octets)"); +} + +static int +spigen_attach(device_t dev) +{ + struct spigen_softc *sc; + const int unit = device_get_unit(dev); + + sc = device_get_softc(dev); + sc->sc_dev = dev; + sc->sc_cdev = make_dev(&spigen_cdevsw, unit, + UID_ROOT, GID_OPERATOR, 0660, "spigen%d", unit); + sc->sc_cdev->si_drv1 = dev; + sc->sc_command_length_max = PAGE_SIZE; + sc->sc_data_length_max = PAGE_SIZE; + mtx_init(&sc->sc_mtx, device_get_nameunit(dev), NULL, MTX_DEF); + spigen_sysctl_init(sc); + + return (0); +} + +static int +spigen_open(struct cdev *dev, int oflags, int devtype, struct thread *td) +{ + + return (0); +} + +static int +spigen_transfer(struct cdev *cdev, struct spigen_transfer *st) +{ + struct spi_command transfer = SPI_COMMAND_INITIALIZER; + device_t dev = cdev->si_drv1; + struct spigen_softc *sc = device_get_softc(dev); + int error = 0; + + mtx_lock(&sc->sc_mtx); + if (st->st_command.iov_len == 0 || st->st_data.iov_len == 0) + error = EINVAL; + else if (st->st_command.iov_len > sc->sc_command_length_max || + st->st_data.iov_len > sc->sc_data_length_max) + error = ENOMEM; + else + transfer.clock_speed_hz = sc->sc_clock_speed; + mtx_unlock(&sc->sc_mtx); + if (error) + return (error); + +#if 0 + device_printf(dev, "cmd %p %u data %p %u\n", st->st_command.iov_base, + st->st_command.iov_len, st->st_data.iov_base, st->st_data.iov_len); +#endif + transfer.tx_cmd = transfer.rx_cmd = malloc(st->st_command.iov_len, + M_DEVBUF, M_WAITOK); + if (transfer.tx_cmd == NULL) + return (ENOMEM); + transfer.tx_data = transfer.rx_data = malloc(st->st_data.iov_len, + M_DEVBUF, M_WAITOK); + if (transfer.tx_data == NULL) { + free(transfer.tx_cmd, M_DEVBUF); + return (ENOMEM); + } + + error = copyin(st->st_command.iov_base, transfer.tx_cmd, + transfer.tx_cmd_sz = transfer.rx_cmd_sz = st->st_command.iov_len); + if (error == 0) + error = copyin(st->st_data.iov_base, transfer.tx_data, + transfer.tx_data_sz = transfer.rx_data_sz = + st->st_data.iov_len); + if (error == 0) + error = SPIBUS_TRANSFER(device_get_parent(dev), dev, &transfer); + if (error == 0) { + error = copyout(transfer.rx_cmd, st->st_command.iov_base, + transfer.rx_cmd_sz); + if (error == 0) + error = copyout(transfer.rx_data, st->st_data.iov_base, + transfer.rx_data_sz); + } + + free(transfer.tx_cmd, M_DEVBUF); + free(transfer.tx_data, M_DEVBUF); + return (error); +} + +static int +spigen_ioctl(struct cdev *cdev, u_long cmd, caddr_t data, int fflag, + struct thread *td) +{ + device_t dev = cdev->si_drv1; + struct spigen_softc *sc = device_get_softc(dev); + int error; + + switch (cmd) { + case SPIGENIOC_TRANSFER: + error = spigen_transfer(cdev, (struct spigen_transfer *)data); + break; + case SPIGENIOC_GET_CLOCK_SPEED: + mtx_lock(&sc->sc_mtx); + *(uint32_t *)data = sc->sc_clock_speed; + mtx_unlock(&sc->sc_mtx); + error = 0; + break; + case SPIGENIOC_SET_CLOCK_SPEED: + mtx_lock(&sc->sc_mtx); + sc->sc_clock_speed = *(uint32_t *)data; + mtx_unlock(&sc->sc_mtx); + error = 0; + break; + default: + error = EOPNOTSUPP; + } + return (error); +} + +static int +spigen_close(struct cdev *dev, int fflag, int devtype, struct thread *td) +{ + + return (0); +} + +static int +spigen_detach(device_t dev) +{ + + return (EIO); +} + +static devclass_t spigen_devclass; + +static device_method_t spigen_methods[] = { + /* Device interface */ + DEVMETHOD(device_probe, spigen_probe), + DEVMETHOD(device_attach, spigen_attach), + DEVMETHOD(device_detach, spigen_detach), + + { 0, 0 } +}; + +static driver_t spigen_driver = { + "spigen", + spigen_methods, + sizeof(struct spigen_softc), +}; + +DRIVER_MODULE(spigen, spibus, spigen_driver, spigen_devclass, 0, 0); Property changes on: dev/spibus/spigen.c ___________________________________________________________________ Added: svn:eol-style ## -0,0 +1 ## +native \ No newline at end of property Added: svn:keywords ## -0,0 +1 ## +FreeBSD=%H \ No newline at end of property Added: svn:mime-type ## -0,0 +1 ## +text/plain \ No newline at end of property Index: sys/spigenio.h =================================================================== --- sys/spigenio.h (revision 0) +++ sys/spigenio.h (working copy) @@ -0,0 +1,52 @@ +/*- + * Copyright (c) 2000 Doug Rabson + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + * + * $FreeBSD$ + */ + +#ifndef _SYS_SPIGENIO_H_ +#define _SYS_SPIGENIO_H_ + +#include <sys/_iovec.h> + +struct spigen_transfer { + struct iovec st_command; /* master to slave */ + struct iovec st_data; /* slave to master and/or master to slave */ +}; + +struct spigen_transfer_mmapped { + size_t stm_command_length; /* at offset 0 in mmap(2) area */ + size_t stm_data_length; /* at offset stm_command_length */ +}; + +#define SPIGENIOC_BASE 'S' +#define SPIGENIOC_TRANSFER _IOW(SPIGENIOC_BASE, 0, \ + struct spigen_transfer) +#define SPIGENIOC_TRANSFER_MMAPPED _IOW(SPIGENIOC_BASE, 1, \ + struct spigen_transfer_mmapped) +#define SPIGENIOC_GET_CLOCK_SPEED _IOR(SPIGENIOC_BASE, 2, uint32_t) +#define SPIGENIOC_SET_CLOCK_SPEED _IOW(SPIGENIOC_BASE, 3, uint32_t) + +#endif /* !_SYS_SPIGENIO_H_ */ Property changes on: sys/spigenio.h ___________________________________________________________________ Added: svn:eol-style ## -0,0 +1 ## +native \ No newline at end of property Added: svn:keywords ## -0,0 +1 ## +FreeBSD=%H \ No newline at end of property Added: svn:mime-type ## -0,0 +1 ## +text/plain \ No newline at end of propertyhelp
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAEv1%2BOUhSAJxxWAfW2GUFVw=H-_KOs2dGg2d7uhZnFbqsHE5Qw>