Message-ID: <20161220204451.knlh4zqz53gx4neq@pengutronix.de>
Date:   Tue, 20 Dec 2016 21:44:51 +0100
From:   Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
To:     Joshua Clayton <stillcompiling@...il.com>
Cc:     Mark Rutland <mark.rutland@....com>,
        Moritz Fischer <moritz.fischer@...us.com>,
        devicetree@...r.kernel.org,
        Alan Tull <atull@...nsource.altera.com>,
        Catalin Marinas <catalin.marinas@....com>,
        linux-fpga@...r.kernel.org, Will Deacon <will.deacon@....com>,
        Russell King <linux@...linux.org.uk>,
        linux-kernel@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Sascha Hauer <kernel@...gutronix.de>,
        Fabio Estevam <fabio.estevam@....com>,
        Anatolij Gustschin <agust@...x.de>,
        Shawn Guo <shawnguo@...nel.org>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v6 4/5] fpga manager: Add cyclone-ps-spi driver for
 Altera FPGAs

Hello Joshua,

On Tue, Dec 20, 2016 at 11:47:37AM -0800, Joshua Clayton wrote:
> >> + *  Copyright (c) 2017 United Western Technologies, Corporation
> > In which timezone it's already 2017? s/  / /
> >
> LOL. It will be 2017 long before 4.11 was my thinking.
> I guess I've never spent much time on time stamp etiquette for copyright.
> It said "2015" in v5, despite still being revised.

Well, you have to put the date when you worked on the driver.

> >> + *
> >> + *  Joshua Clayton <stillcompiling@...il.com>
> >> + *
> >> + * Manage Altera FPGA firmware that is loaded over spi using the passive
> >> + * serial configuration method.
> >> + * Firmware must be in binary "rbf" format.
> >> + * Works on Cyclone V. Should work on cyclone series.
> >> + * May work on other Altera FPGAs.
> > I can test this later on an Arria 10. I'm not sure what the connection
> > between "Cyclone" and "Arria" is, but the protocol looks similar.
> My guess was it would be.
> Would be wonderful to have someone to test.

I didn't come around yet.

> >> + *
> >> + */
> >> +
> >> +#include <linux/bitrev.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/fpga/fpga-mgr.h>
> >> +#include <linux/gpio/consumer.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of_gpio.h>
> >> +#include <linux/spi/spi.h>
> >> +#include <linux/sizes.h>
> >> +
> >> +#define FPGA_RESET_TIME		50   /* time in usecs to trigger FPGA config */
> >> +#define FPGA_MIN_DELAY		50   /* min usecs to wait for config status */
> >> +#define FPGA_MAX_DELAY		1000 /* max usecs to wait for config status */
> >> +
> >> +struct cyclonespi_conf {
> >> +	struct gpio_desc *config;
> >> +	struct gpio_desc *status;
> >> +	struct spi_device *spi;
> >> +};
> >> +
> >> +static const struct of_device_id of_ef_match[] = {
> >> +	{ .compatible = "altr,cyclone-ps-spi-fpga-mgr", },
> >> +	{}
> >> +};
> >> +MODULE_DEVICE_TABLE(of, of_ef_match);
> > barebox already has such a driver, the binding is available at
> >
> > 	https://git.pengutronix.de/cgit/barebox/tree/Documentation/devicetree/bindings/firmware/altr,passive-serial.txt
> >
> > (This isn't completely accurate because nstat is optional in the driver.)
> Interesting.
> The binding looks ... like we should synchronize those bindings.

ack.

> >> +	gpiod_set_value(conf->config, 1);
> >> +	usleep_range(FPGA_RESET_TIME, FPGA_RESET_TIME + 20);
> >> +	if (!gpiod_get_value(conf->status)) {
> >> +		dev_err(&mgr->dev, "Status pin should be low.\n");
> > You write this when get_value returns 0. There is something fishy.
> I'll take a look. These gpios are "active low", so a logical 1 is a
> physical low, IIRC. Maybe I should change the wording:
> Something like dev_err(&mgr->dev, "Status pin should be in reset.\n");

maybe "inactive"? Then then you should better use nconfig as dts name
(as done in the barebox binding) and put the active low information into
the flag.

> >> +		return -EIO;
> >> +	}
> >> +
> >> +	gpiod_set_value(conf->config, 0);
> >> +	for (i = 0; i < (FPGA_MAX_DELAY / FPGA_MIN_DELAY); i++) {
> >> +		usleep_range(FPGA_MIN_DELAY, FPGA_MIN_DELAY + 20);
> >> +		if (!gpiod_get_value(conf->status))
> >> +			return 0;
> >> +	}
> >> +
> >> +	dev_err(&mgr->dev, "Status pin not ready.\n");
> >> +	return -EIO;
> > For Arria 10 the documentation has:
> >
> > 	To ensure a successful configuration, send the entire
> > 	configuration data to the device. CONF_DONE is released high
> > 	when the device receives all the configuration data
> > 	successfully. After CONF_DONE goes high, send two additional
> > 	falling edges on DCLK to begin initialization and enter user
> > 	mode.
> >
> > ISTR this is necessary for Arria V, too.
> DCLK is the spi clock, yes?
> Would sending an extra byte after CONF_DONE is released suffice?

The barebox driver sends two bytes.

> >> +}
> >> +
> >> +static int cyclonespi_write(struct fpga_manager *mgr, const char *buf,
> >> +			    size_t count)
> >> +{
> >> +	struct cyclonespi_conf *conf = (struct cyclonespi_conf *)mgr->priv;
> >> +	const char *fw_data = buf;
> >> +	const char *fw_data_end = fw_data + count;
> >> +
> >> +	while (fw_data < fw_data_end) {
> >> +		int ret;
> >> +		size_t stride = min(fw_data_end - fw_data, SZ_4K);
> >> +
> >> +		rev_buf((void *)fw_data, stride);
> > This isn't necessary if the spi controller supports SPI_LSB_FIRST. At
> > least the mvebu spi core can do this for you. (The driver doesn't
> > support it yet, though.)
> This is true, but many of them do not.

And I think it's hard to detect if the adapter driver supports this or
simply ignores it.
 
> Moritz Fischer had  proposal to add things like this with a flag.
> It could then be part of the library, rather than part of the driver
> 
> Speaking of which,
> I made an unsuccessful attempt to hack generic lsb first SPI
> with an extra bounce buffer.
> Sending was fine, but I ran into trouble with LSB first rx
> (I think) because of dma.

Link?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Powered by blists - more mailing lists