| Message ID | b033d16ec31d26a895726ee72ec89b43943dfbae.1406547656.git.jiri.prchal@aksignal.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Thank you for that patch, a few questions/comments below. On 28/07/2014 at 13:43:40 +0200, Jiri Prchal 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@aksignal.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" > + This is not really related to the issue solved by that patch, maybe it could go in another patch ? > /* 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[]; > }; > > /* Controller-specific per-slave state */ > struct atmel_spi_device { > - unsigned int npcs_pin; > 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); > } 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); > 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); > } > > 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); > } > > 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; > - > 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; > 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; > This driver can still be probed without DT, this will break here, please don't assume that DT is present. > /* 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)); > 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; My guess is that you don't need to change that part. > 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); Again, DT maybe not be compiled in or that driver may be probed from platform data. > + 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); > + if (ret < 0) { > + dev_err(&pdev->dev, > + "failed to configure csgpio#%u (%d)\n", > + i, ret); > + goto out_free; > + } > + } > + > /* > * Scratch buffer is used for throwaway rx and tx data. > * It's coherent to minimize dcache pollution. > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi, Dne 28.7.2014 v 14:21 Alexandre Belloni napsal(a): > Hi, > > Thank you for that patch, a few questions/comments below. > > On 28/07/2014 at 13:43:40 +0200, Jiri Prchal 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@aksignal.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" >> + > > This is not really related to the issue solved by that patch, maybe it > could go in another patch ? Probably yes, but is used for this patch, I based it upon spi-efm32. > >> /* 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[]; >> }; >> >> /* Controller-specific per-slave state */ >> struct atmel_spi_device { >> - unsigned int npcs_pin; >> 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); >> } 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); >> 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); >> } >> >> 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); >> } >> >> 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; >> - >> 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; >> 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; >> > > This driver can still be probed without DT, this will break here, please > don't assume that DT is present. Yes, but I copied it from spi-efm32 and looked at others, is almost the same. > > >> /* 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)); >> 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; > > My guess is that you don't need to change that part. I think I need it. What about 3 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); > > Again, DT maybe not be compiled in or that driver may be probed from > platform data. Is another way how to do it? I see cs-gpios in master struct, but where it gets filled. > >> + 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); >> + if (ret < 0) { >> + dev_err(&pdev->dev, >> + "failed to configure csgpio#%u (%d)\n", >> + i, ret); >> + goto out_free; >> + } >> + } >> + >> /* >> * Scratch buffer is used for throwaway rx and tx data. >> * It's coherent to minimize dcache pollution. >> -- >> 1.9.1 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
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@aksignal.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@aksignal.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
(Adding Mark in Cc:) On 28/07/2014 at 15:06:34 +0200, Ji?í Prchal wrote : > >>+#define DRIVER_NAME "atmel-spi" > >>+ > > > >This is not really related to the issue solved by that patch, maybe it > >could go in another patch ? > Probably yes, but is used for this patch, I based it upon spi-efm32. > > Yeah, I wouldn't use it anyway, see my comment below. > >> 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; > >> > > > >This driver can still be probed without DT, this will break here, please > >don't assume that DT is present. > Yes, but I copied it from spi-efm32 and looked at others, is almost the same. Maybe it is similar in other drivers but at91 platforms can still be booted without DT, This will fail with: atmel_spi: probe of atmel_spi.0 failed with error -22 The efm32 only supports DT, it doesn't have board files/platform data. > > > > > >> /* 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)); > >> 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; > > > >My guess is that you don't need to change that part. > I think I need it. What about 3 cs? This will be taken care of by the spi core, in of_spi_register_master(): nb = of_gpio_named_count(np, "cs-gpios"); master->num_chipselect = max_t(int, nb, master->num_chipselect); > > > >> 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); > > > >Again, DT maybe not be compiled in or that driver may be probed from > >platform data. > Is another way how to do it? I see cs-gpios in master struct, but where it gets filled. > > It is filled when registering the master, here when calling devm_spi_register_master(). It may be too late at that point. > >>+ 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); Maybe you could use the CS number ifor the label here instead of DRIVER_NAME, at least, use pdev->dev->name and completely drop DRIVER_NAME. > >>+ if (ret < 0) { > >>+ dev_err(&pdev->dev, > >>+ "failed to configure csgpio#%u (%d)\n", > >>+ i, ret); > >>+ goto out_free; > >>+ } > >>+ } > >>+ Mark: maybe it would make sense to do devm_gpio_request_one() in of_spi_register_master(), after of_get_named_gpio. While this solves the particular issue Ji?í is seeing, this will not solve the case where PA14 (CS0) is not used by the spi driver at all. It will remained muxed as CS0 and toggle when the spi master needs to access CS0 until another driver muxes it to something else. I still believe we should explicitly ask pinctrl to mux them as gpios.
Hi Alexandre, On Tue, 29 Jul 2014 00:38:59 +0200 Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > > >>+ if (ret < 0) { > > >>+ dev_err(&pdev->dev, > > >>+ "failed to configure csgpio#%u (%d)\n", > > >>+ i, ret); > > >>+ goto out_free; > > >>+ } > > >>+ } > > >>+ > > Mark: maybe it would make sense to do devm_gpio_request_one() in > of_spi_register_master(), after of_get_named_gpio. Might be the cleanest way to solve this issue, but you'll have to modify all the drivers that request cs_gpio in their setup method/callback. > > While this solves the particular issue Ji?í is seeing, this will not > solve the case where PA14 (CS0) is not used by the spi driver at all. It > will remained muxed as CS0 and toggle when the spi master needs to > access CS0 until another driver muxes it to something else. I still > believe we should explicitly ask pinctrl to mux them as gpios. > Do we really care about this case ? After all, if a given pin needs a specific muxing during kernel boot (i.e. a pin connected to a gpio-led that needs to stay in its previous state or a pin connected to the reset line of a device that needs to stay up and running during kernel boot) the bootloader/bootstrap should have muxed this pin appropriately before booting the kernel. What do you mean by "we should explicitly ask pinctrl to mux them as gpios" ? Do you mean configuring all the pins as GPIOs when the pin controller is probed, or just adding a new pinctrl state configuring the pin as an output GPIO and reference it in the pinctrl-0 property of the spi controller. If the former, you'll break devices that needs their pins to stay in the state they were during the bootloader/boostrap phase. The latter won't work if the pin you request as GPIO is later requested by another device (which, if I'm correct, is exactly the case you're trying to solve). Best Regards, Boris
On Tue, Jul 29, 2014 at 12:38:59AM +0200, Alexandre Belloni wrote: > Mark: maybe it would make sense to do devm_gpio_request_one() in > of_spi_register_master(), after of_get_named_gpio. You need to transition all the drivers doing things manually but yes. As I keep saying all the GPIO handling needs to be completely refactored. > While this solves the particular issue Ji?í is seeing, this will not > solve the case where PA14 (CS0) is not used by the spi driver at all. It > will remained muxed as CS0 and toggle when the spi master needs to > access CS0 until another driver muxes it to something else. I still > believe we should explicitly ask pinctrl to mux them as gpios. I'm just guessing as to what the issue that he's seeing is but isn't this just saying that the board pinmuxing is wrong? If the board doesn't want the pin to be used by the SPI controller it shouldn't be configuring it that way.
On 29/07/2014 at 10:00:17 +0200, Boris Brezillon wrote : > Hi Alexandre, > > > While this solves the particular issue Ji?í is seeing, this will not > > solve the case where PA14 (CS0) is not used by the spi driver at all. It > > will remained muxed as CS0 and toggle when the spi master needs to > > access CS0 until another driver muxes it to something else. I still > > believe we should explicitly ask pinctrl to mux them as gpios. > > > > Do we really care about this case ? > After all, if a given pin needs a specific muxing during kernel boot > (i.e. a pin connected to a gpio-led that needs to stay in its previous > state or a pin connected to the reset line of a device that needs to > stay up and running during kernel boot) the bootloader/bootstrap should > have muxed this pin appropriately before booting the kernel. > Yeah, you are right. > What do you mean by "we should explicitly ask pinctrl to mux them as > gpios" ? > Do you mean configuring all the pins as GPIOs when the pin controller is > probed, or just adding a new pinctrl state configuring the pin as an > output GPIO and reference it in the pinctrl-0 property of the spi > controller. > > If the former, you'll break devices that needs their pins to stay in > the state they were during the bootloader/boostrap phase. > The latter won't work if the pin you request as GPIO is later requested > by another device (which, if I'm correct, is exactly the case you're > trying to solve). > Again you are right, let's not care about that use case. I still feel that the pinctrl-0 property has to be filled correctly.
On Jul 31, 2014, at 11:59 PM, Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > On 29/07/2014 at 10:00:17 +0200, Boris Brezillon wrote : >> Hi Alexandre, >> >>> While this solves the particular issue Ji?í is seeing, this will not >>> solve the case where PA14 (CS0) is not used by the spi driver at all. It >>> will remained muxed as CS0 and toggle when the spi master needs to >>> access CS0 until another driver muxes it to something else. I still >>> believe we should explicitly ask pinctrl to mux them as gpios. This is not the job of the kernel but to the bootloader no the pinctrl binding is not here and will never be here the configure a pin as a GPIO or to a specific state. This the job of the driver or the bootloader >>> >> >> Do we really care about this case ? >> After all, if a given pin needs a specific muxing during kernel boot >> (i.e. a pin connected to a gpio-led that needs to stay in its previous >> state or a pin connected to the reset line of a device that needs to >> stay up and running during kernel boot) the bootloader/bootstrap should >> have muxed this pin appropriately before booting the kernel. >> > > Yeah, you are right. > > >> What do you mean by "we should explicitly ask pinctrl to mux them as >> gpios" ? >> Do you mean configuring all the pins as GPIOs when the pin controller is >> probed, or just adding a new pinctrl state configuring the pin as an >> output GPIO and reference it in the pinctrl-0 property of the spi >> controller. >> >> If the former, you'll break devices that needs their pins to stay in >> the state they were during the bootloader/boostrap phase. >> The latter won't work if the pin you request as GPIO is later requested >> by another device (which, if I'm correct, is exactly the case you're >> trying to solve). >> > > Again you are right, let's not care about that use case. I still feel > that the pinctrl-0 property has to be filled correctly. > > -- > Alexandre Belloni, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 01/08/2014 at 00:10:05 +0800, Jean-Christophe PLAGNIOL-VILLARD wrote : > >>> While this solves the particular issue Ji?í is seeing, this will not > >>> solve the case where PA14 (CS0) is not used by the spi driver at all. It > >>> will remained muxed as CS0 and toggle when the spi master needs to > >>> access CS0 until another driver muxes it to something else. I still > >>> believe we should explicitly ask pinctrl to mux them as gpios. > > This is not the job of the kernel but to the bootloader > no the pinctrl binding is not here and will never be here the configure a pin as a GPIO or This is exactly what AT91_PERIPH_GPIO does though. Anyway, not request the pinmuxing in the driver is not a good idea, then nothing prevents another driver to request them to do something completely different. > to a specific state. This the job of the driver or the bootloader > I agree that it must not allow to set the state, we are talking about muxing.
On Aug 1, 2014, at 12:48 AM, Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > > On 01/08/2014 at 00:10:05 +0800, Jean-Christophe PLAGNIOL-VILLARD wrote : >>>>> While this solves the particular issue Ji?í is seeing, this will not >>>>> solve the case where PA14 (CS0) is not used by the spi driver at all. It >>>>> will remained muxed as CS0 and toggle when the spi master needs to >>>>> access CS0 until another driver muxes it to something else. I still >>>>> believe we should explicitly ask pinctrl to mux them as gpios. >> >> This is not the job of the kernel but to the bootloader >> no the pinctrl binding is not here and will never be here the configure a pin as a GPIO or > > This is exactly what AT91_PERIPH_GPIO does though. Anyway, not request > the pinmuxing in the driver is not a good idea, then nothing prevents > another driver to request them to do something completely different. here it’s the same issue if you need to pin as GPIO use the GPIO API not the pinctrl AT91_PERIPH_GPIO is use the set specific pinmux (bouncing & etc…) > >> to a specific state. This the job of the driver or the bootloader >> > > I agree that it must not allow to set the state, we are talking about > muxing. > > > -- > Alexandre Belloni, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com
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[]; }; /* Controller-specific per-slave state */ struct atmel_spi_device { - unsigned int npcs_pin; 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); } 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); 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); } 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); } 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; - 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; 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; /* 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)); 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); + if (ret < 0) { + dev_err(&pdev->dev, + "failed to configure csgpio#%u (%d)\n", + i, ret); + goto out_free; + } + } + /* * Scratch buffer is used for throwaway rx and tx data. * It's coherent to minimize dcache pollution.
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@aksignal.cz> --- drivers/spi/spi-atmel.c | 68 +++++++++++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 27 deletions(-)