| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140728170653.087dfbfd@bbrezillon>
Date: Mon, 28 Jul 2014 17:06:53 +0200
From: Boris BREZILLON <boris.brezillon@...e-electrons.com>
To: Jiri Prchal <jiri.prchal@...ignal.cz>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, nicolas.ferre@...el.com,
voice.shen@...el.com
Subject: Re: [PATCH] ARM: at91: spi: request all csgpio in spi probe
Hello Jiri,
The least you can do when someone helps you debugging something is to
keep him in Cc of the following versions of your patch...
On Mon, 28 Jul 2014 13:43:40 +0200
Jiri Prchal <jiri.prchal@...ignal.cz> wrote:
> This fix problem with PA14 set as periph A left from ROMBOOT and need it
> to be set to gpio before spi bus communicate with chip on CS0 on other
> gpio. As I reported a week ago.
> Request all csgpios in spi_probe since cs depends on each other and must
> be all of them in right state before any communication on bus. They are
> part of bus, like miso, mosi, clk, not part of chips, they are defined
> in parent spi node, not in child chip node.
> Csgpio moved to atmel_spi struct as part of master bus.
>
> Signed-off-by: Jiri Prchal <jiri.prchal@...ignal.cz>
> ---
> drivers/spi/spi-atmel.c | 68 +++++++++++++++++++++++++++++--------------------
> 1 file changed, 41 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
> index 92a6f0d..1fb7bb9 100644
> --- a/drivers/spi/spi-atmel.c
> +++ b/drivers/spi/spi-atmel.c
> @@ -22,11 +22,14 @@
> #include <linux/platform_data/atmel.h>
> #include <linux/platform_data/dma-atmel.h>
> #include <linux/of.h>
> +#include <linux/of_gpio.h>
>
> #include <linux/io.h>
> #include <linux/gpio.h>
> #include <linux/pinctrl/consumer.h>
>
> +#define DRIVER_NAME "atmel-spi"
> +
> /* SPI register offsets */
> #define SPI_CR 0x0000
> #define SPI_MR 0x0004
> @@ -242,11 +245,13 @@ struct atmel_spi {
>
> bool keep_cs;
> bool cs_active;
> +
> + /* chip selects */
> + unsigned int csgpio[];
Do you really need to store these gpio ids in atmel_spi struct (they
are already stored in the spi_master).
> };
>
> /* Controller-specific per-slave state */
> struct atmel_spi_device {
> - unsigned int npcs_pin;
Keep this field to handle both DT and non-DT cases.
> u32 csr;
> };
>
> @@ -312,7 +317,7 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi)
> }
>
> mr = spi_readl(as, MR);
> - gpio_set_value(asd->npcs_pin, active);
> + gpio_set_value(as->csgpio[spi->chip_select], active);
Drop this change.
> } else {
> u32 cpol = (spi->mode & SPI_CPOL) ? SPI_BIT(CPOL) : 0;
> int i;
> @@ -329,18 +334,16 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi)
> mr = spi_readl(as, MR);
> mr = SPI_BFINS(PCS, ~(1 << spi->chip_select), mr);
> if (spi->chip_select != 0)
> - gpio_set_value(asd->npcs_pin, active);
> + gpio_set_value(as->csgpio[spi->chip_select], active);
Ditto.
> spi_writel(as, MR, mr);
> }
>
> dev_dbg(&spi->dev, "activate %u%s, mr %08x\n",
> - asd->npcs_pin, active ? " (high)" : "",
> - mr);
> + as->csgpio[spi->chip_select], active ? " (high)" : "", mr);
Ditto.
> }
>
> static void cs_deactivate(struct atmel_spi *as, struct spi_device *spi)
> {
> - struct atmel_spi_device *asd = spi->controller_state;
> unsigned active = spi->mode & SPI_CS_HIGH;
> u32 mr;
>
> @@ -354,11 +357,10 @@ static void cs_deactivate(struct atmel_spi *as, struct spi_device *spi)
> }
>
> dev_dbg(&spi->dev, "DEactivate %u%s, mr %08x\n",
> - asd->npcs_pin, active ? " (low)" : "",
> - mr);
> + as->csgpio[spi->chip_select], active ? " (low)" : "", mr);
>
> if (atmel_spi_is_v2(as) || spi->chip_select != 0)
> - gpio_set_value(asd->npcs_pin, !active);
> + gpio_set_value(as->csgpio[spi->chip_select], !active);
Ditto.
> }
>
> static void atmel_spi_lock(struct atmel_spi *as) __acquires(&as->lock)
> @@ -989,8 +991,6 @@ static int atmel_spi_setup(struct spi_device *spi)
> struct atmel_spi_device *asd;
> u32 csr;
> unsigned int bits = spi->bits_per_word;
> - unsigned int npcs_pin;
> - int ret;
>
> as = spi_master_get_devdata(spi->master);
>
> @@ -1017,27 +1017,13 @@ static int atmel_spi_setup(struct spi_device *spi)
> csr |= SPI_BF(DLYBS, 0);
> csr |= SPI_BF(DLYBCT, 0);
>
> - /* chipselect must have been muxed as GPIO (e.g. in board setup) */
> - npcs_pin = (unsigned int)spi->controller_data;
> -
> - if (gpio_is_valid(spi->cs_gpio))
> - npcs_pin = spi->cs_gpio;
> -
Keep this block and just add:
else {
ret = gpio_request(npcs_pin, dev_name(&spi->dev));
if (ret) {
kfree(asd);
return ret;
}
gpio_direction_output(npcs_pin,
!(spi->mode & SPI_CS_HIGH));
}
> asd = spi->controller_state;
> if (!asd) {
> asd = kzalloc(sizeof(struct atmel_spi_device), GFP_KERNEL);
> if (!asd)
> return -ENOMEM;
>
> - ret = gpio_request(npcs_pin, dev_name(&spi->dev));
> - if (ret) {
> - kfree(asd);
> - return ret;
> - }
> -
> - asd->npcs_pin = npcs_pin;
Keep this line.
> spi->controller_state = asd;
> - gpio_direction_output(npcs_pin, !(spi->mode & SPI_CS_HIGH));
> }
>
> asd->csr = csr;
> @@ -1290,6 +1276,15 @@ static int atmel_spi_probe(struct platform_device *pdev)
> int ret;
> struct spi_master *master;
> struct atmel_spi *as;
> + struct device_node *np = pdev->dev.of_node;
> + int num_cs, i;
> +
> + if (!np)
> + return -EINVAL;
> +
> + num_cs = of_gpio_named_count(np, "cs-gpios");
> + if (num_cs < 0)
> + return num_cs;
As stated by Alexandre, you cannot do this, because it will break
non-DT board support.
This should do the trick:
int num_cs = 0;
if (np) {
num_cs = of_gpio_named_count(np, "cs-gpios");
if (num_cs < 0)
return num_cs;
}
>
> /* Select default pin state */
> pinctrl_pm_select_default_state(&pdev->dev);
> @@ -1308,7 +1303,7 @@ static int atmel_spi_probe(struct platform_device *pdev)
>
> /* setup spi core then atmel-specific driver state */
> ret = -ENOMEM;
> - master = spi_alloc_master(&pdev->dev, sizeof(*as));
> + master = spi_alloc_master(&pdev->dev, sizeof(*as) + num_cs * sizeof(unsigned));
Drop this change.
> if (!master)
> goto out_free;
>
> @@ -1317,7 +1312,7 @@ static int atmel_spi_probe(struct platform_device *pdev)
> master->bits_per_word_mask = SPI_BPW_RANGE_MASK(8, 16);
> master->dev.of_node = pdev->dev.of_node;
> master->bus_num = pdev->id;
> - master->num_chipselect = master->dev.of_node ? 0 : 4;
> + master->num_chipselect = num_cs;
> master->setup = atmel_spi_setup;
> master->transfer_one_message = atmel_spi_transfer_one_message;
> master->cleanup = atmel_spi_cleanup;
> @@ -1325,6 +1320,25 @@ static int atmel_spi_probe(struct platform_device *pdev)
>
> as = spi_master_get_devdata(master);
>
> + for (i = 0; i < master->num_chipselect; ++i) {
> + ret = of_get_named_gpio(np, "cs-gpios", i);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to get csgpio#%u (%d)\n",
> + i, ret);
> + goto out_free;
> + }
> + as->csgpio[i] = ret;
> + dev_dbg(&pdev->dev, "csgpio#%u = %u\n", i, as->csgpio[i]);
> + ret = devm_gpio_request_one(&pdev->dev, as->csgpio[i],
> + GPIOF_OUT_INIT_HIGH, DRIVER_NAME);
Are you sure we always want to initialize this gpio to high ?
What if SPI_CS_HIGH is set ?
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "failed to configure csgpio#%u (%d)\n",
> + i, ret);
> + goto out_free;
> + }
> + }
> +
You could move this block in a dt specific function (how about
naming it of_atmel_spi_init ?) and test np availability before calling
this function.
My suggestions are far from perfect, but at least it fixes the DT case
and keep the non-DT in a working state.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists