From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933657AbcKGStw (ORCPT ); Mon, 7 Nov 2016 13:49:52 -0500 Received: from b.painless.aa.net.uk ([81.187.30.52]:53937 "EHLO b.painless.aa.net.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933504AbcKGSts (ORCPT ); Mon, 7 Nov 2016 13:49:48 -0500 Subject: Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs To: Marek Vasut , atull@opensource.altera.com, moritz.fischer@ettus.com, geert@linux-m68k.org, robh@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org, clifford@clifford.at References: <1478486962-26794-1-git-send-email-joel@airwebreathe.org.uk> <1478486962-26794-3-git-send-email-joel@airwebreathe.org.uk> <2255968d-b97c-2b9c-4e4d-7f3717a748e3@denx.de> From: Joel Holdsworth Message-ID: <01d1a97b-2f43-49a6-51fb-e223ef4dce9b@airwebreathe.org.uk> Date: Mon, 7 Nov 2016 11:49:29 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <2255968d-b97c-2b9c-4e4d-7f3717a748e3@denx.de> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Painless-Spam-Score: -0.2 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Marek, Thanks again for your comments. On 07/11/16 11:01, Marek Vasut wrote: > On 11/07/2016 03:49 AM, Joel Holdsworth wrote: >> The Lattice iCE40 is a family of FPGAs with a minimalistic architecture >> and very regular structure, designed for low-cost, high-volume consumer >> and system applications. > > [...] > >> +static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, >> + const char *buf, size_t count) >> +{ >> + struct ice40_fpga_priv *priv = mgr->priv; >> + struct spi_device *dev = priv->dev; >> + struct spi_message message; >> + struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1, >> + .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY}; > > Should be this way for the sake of readability, fix globally: > > struct spi_transfer assert_cs_then_reset_delay = { > .cs_change = 1, > .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY > }; Sure ok. Personally, I prefer it to be concise, but I'm happy to accept the norms. > > Also I believe this could be const. It doesn't work const - I tried it. spi_message_add_tail() expects it to be non-const. Looking at 'struct spi_transfer' it appears there is various bits of state used to perform the transfer, as well as the pointer to the next item in the single-linked list. > >> + struct spi_transfer housekeeping_delay_then_release_cs = { >> + .delay_usecs = ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY}; > > Don't we have some less hacky way of toggling the nCS ? Is this even nCS > or is this some control pin of the FPGA ? Maybe it should be treated > like a regular GPIO instead ? I've been round this loop before also. drivers/spi/spi.c has a static function 'static void spi_set_cs(struct spi_device *dev, bool enable)'. It manipulates the CS line of devices where CS is built into the SPI master, and manipulates the GPIO on other devices. I don't know why it's non-public - I tried to get an answer from the SPI folks, but didn't get one. I guess they don't want to encourage drivers to manually manipulate the CS line - because SPI transfers are usually meant to be interruptible by higher priority transfers to other devices at any time. The only reason it's legit for me to manually manipulate CS is because I first lock the bus. Previously I had a copy of spi_set_cs copy-pasted into my driver, but in the end I decided to replace that with the zero-length transfers because there's a danger that if the original spi_set_cs() gets rewritten some time, my copy-paste code would leave around some nasty legacy. On the whole, I don't think the zero-length transfers are too egregiously bad, and all the alternatives seem worse to me. >> + const u8 padding[ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES] = {0,}; > > The comma is not needed. True. I'll make that change. >> + /* Check board setup data. */ >> + if (spi->max_speed_hz > 25000000) { >> + dev_err(dev, "Speed is too high\n"); >> + return -EINVAL; >> + } >> + >> + if (spi->max_speed_hz < 1000000) { >> + dev_err(dev, "Speed is too low\n"); >> + return -EINVAL; >> + } > > Do you have some explanation for this limitation ? > Not really no. The 'DS1040 - iCE40 LP/HX Family Data Sheet' page 3-18 claims f_max for Slave SPI Writing is >=1MHz && <=25MHz. The exact reason I don't know. Are they running a PLL off the clock? What if the clock is really jittery - it seems to work fine when I've tested it with bit-banged SPI on the RPi; just as well as with hardware SPI. Or is it something to do with some pre-commit on-chip firmware storage? e.g. to check the CRC. Does the storage buffer have some time limitation before it gets committed to the FPGA core? I'm not sure, so I decided to just reflect the datasheet instructions back to the user. Thanks Joel From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joel Holdsworth Subject: Re: [PATCH v8 3/3] fpga: Add support for Lattice iCE40 FPGAs Date: Mon, 7 Nov 2016 11:49:29 -0700 Message-ID: <01d1a97b-2f43-49a6-51fb-e223ef4dce9b@airwebreathe.org.uk> References: <1478486962-26794-1-git-send-email-joel@airwebreathe.org.uk> <1478486962-26794-3-git-send-email-joel@airwebreathe.org.uk> <2255968d-b97c-2b9c-4e4d-7f3717a748e3@denx.de> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <2255968d-b97c-2b9c-4e4d-7f3717a748e3-ynQEQJNshbs@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Marek Vasut , atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org, moritz.fischer-+aYTwkv1SeIAvxtiuMwx3w@public.gmane.org, geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org, robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, clifford-cPpHkPqGOEfk7+2FdBfRIA@public.gmane.org List-Id: devicetree@vger.kernel.org Hi Marek, Thanks again for your comments. On 07/11/16 11:01, Marek Vasut wrote: > On 11/07/2016 03:49 AM, Joel Holdsworth wrote: >> The Lattice iCE40 is a family of FPGAs with a minimalistic architecture >> and very regular structure, designed for low-cost, high-volume consumer >> and system applications. > > [...] > >> +static int ice40_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags, >> + const char *buf, size_t count) >> +{ >> + struct ice40_fpga_priv *priv = mgr->priv; >> + struct spi_device *dev = priv->dev; >> + struct spi_message message; >> + struct spi_transfer assert_cs_then_reset_delay = {.cs_change = 1, >> + .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY}; > > Should be this way for the sake of readability, fix globally: > > struct spi_transfer assert_cs_then_reset_delay = { > .cs_change = 1, > .delay_usecs = ICE40_SPI_FPGAMGR_RESET_DELAY > }; Sure ok. Personally, I prefer it to be concise, but I'm happy to accept the norms. > > Also I believe this could be const. It doesn't work const - I tried it. spi_message_add_tail() expects it to be non-const. Looking at 'struct spi_transfer' it appears there is various bits of state used to perform the transfer, as well as the pointer to the next item in the single-linked list. > >> + struct spi_transfer housekeeping_delay_then_release_cs = { >> + .delay_usecs = ICE40_SPI_FPGAMGR_HOUSEKEEPING_DELAY}; > > Don't we have some less hacky way of toggling the nCS ? Is this even nCS > or is this some control pin of the FPGA ? Maybe it should be treated > like a regular GPIO instead ? I've been round this loop before also. drivers/spi/spi.c has a static function 'static void spi_set_cs(struct spi_device *dev, bool enable)'. It manipulates the CS line of devices where CS is built into the SPI master, and manipulates the GPIO on other devices. I don't know why it's non-public - I tried to get an answer from the SPI folks, but didn't get one. I guess they don't want to encourage drivers to manually manipulate the CS line - because SPI transfers are usually meant to be interruptible by higher priority transfers to other devices at any time. The only reason it's legit for me to manually manipulate CS is because I first lock the bus. Previously I had a copy of spi_set_cs copy-pasted into my driver, but in the end I decided to replace that with the zero-length transfers because there's a danger that if the original spi_set_cs() gets rewritten some time, my copy-paste code would leave around some nasty legacy. On the whole, I don't think the zero-length transfers are too egregiously bad, and all the alternatives seem worse to me. >> + const u8 padding[ICE40_SPI_FPGAMGR_NUM_ACTIVATION_BYTES] = {0,}; > > The comma is not needed. True. I'll make that change. >> + /* Check board setup data. */ >> + if (spi->max_speed_hz > 25000000) { >> + dev_err(dev, "Speed is too high\n"); >> + return -EINVAL; >> + } >> + >> + if (spi->max_speed_hz < 1000000) { >> + dev_err(dev, "Speed is too low\n"); >> + return -EINVAL; >> + } > > Do you have some explanation for this limitation ? > Not really no. The 'DS1040 - iCE40 LP/HX Family Data Sheet' page 3-18 claims f_max for Slave SPI Writing is >=1MHz && <=25MHz. The exact reason I don't know. Are they running a PLL off the clock? What if the clock is really jittery - it seems to work fine when I've tested it with bit-banged SPI on the RPi; just as well as with hardware SPI. Or is it something to do with some pre-commit on-chip firmware storage? e.g. to check the CRC. Does the storage buffer have some time limitation before it gets committed to the FPGA core? I'm not sure, so I decided to just reflect the datasheet instructions back to the user. Thanks Joel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html