| Message ID | 1395435098-30003-9-git-send-email-m-karicheri2@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
Hi, On Sat, Mar 22, 2014 at 2:21 AM, Murali Karicheri <m-karicheri2@ti.com> wrote: > Currently davinci spi driver supports only bus 0 cs 0. > This patch allows driver to support bus 1 and bus 2 with > configurable number of chip selects. Also defaults are > selected in a way to avoid regression on other platforms > that uses davinci spi driver and has only one spi bus. > > Signed-off-by: Rex Chang <rchang@ti.com> > Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> > Acked-by: Tom Rini <trini@ti.com> > --- > drivers/spi/davinci_spi.c | 60 ++++++++++++++++++++++++++++++++++++++++++--- > drivers/spi/davinci_spi.h | 33 +++++++++++++++++++++++++ > 2 files changed, 90 insertions(+), 3 deletions(-) > > diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c > index e3fb321..b682bc4 100644 > --- a/drivers/spi/davinci_spi.c > +++ b/drivers/spi/davinci_spi.c > @@ -32,7 +32,27 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs, > if (!ds) > return NULL; > > - ds->regs = (struct davinci_spi_regs *)CONFIG_SYS_SPI_BASE; > + ds->slave.bus = bus; > + ds->slave.cs = cs; > + > + switch (bus) { > + case SPI0_BUS: > + ds->regs = (struct davinci_spi_regs *)SPI0_BASE; > + break; > +#ifdef CONFIG_SYS_SPI1 > + case SPI1_BUS: > + ds->regs = (struct davinci_spi_regs *)SPI0_BASE; > + break; > +#endif > +#ifdef CONFIG_SYS_SPI2 > + case SPI2_BUS: > + ds->regs = (struct davinci_spi_regs *)SPI2_BASE; > + break; > +#endif > + default: /* Invalid bus number */ > + return NULL; > + } > + > ds->freq = max_hz; > > return &ds->slave; > @@ -59,7 +79,7 @@ int spi_claim_bus(struct spi_slave *slave) > writel(SPIGCR1_MASTER_MASK | SPIGCR1_CLKMOD_MASK, &ds->regs->gcr1); > > /* CS, CLK, SIMO and SOMI are functional pins */ > - writel((SPIPC0_EN0FUN_MASK | SPIPC0_CLKFUN_MASK | > + writel(((1 << slave->cs) | SPIPC0_CLKFUN_MASK | > SPIPC0_DOFUN_MASK | SPIPC0_DIFUN_MASK), &ds->regs->pc0); > > /* setup format */ > @@ -262,9 +282,43 @@ out: > return 0; > } > > +#ifdef CONFIG_SYS_SPI1 > +static int bus1_cs_valid(unsigned int bus, unsigned int cs) > +{ > + if ((bus == SPI1_BUS) && (cs < SPI1_NUM_CS)) > + return 1; > + return 0; > +} > +#else > +static int bus1_cs_valid(unsigned int bus, unsigned int cs) > +{ > + return 0; > +} > +#endif > + > +#ifdef CONFIG_SYS_SPI2 > +static int bus2_cs_valid(unsigned int bus, unsigned int cs) > +{ > + if ((bus == SPI2_BUS) && (cs < SPI2_NUM_CS)) > + return 1; > + return 0; > +} > +#else > +static int bus2_cs_valid(unsigned int bus, unsigned int cs) > +{ > + return 0; > +} > +#endif > + > int spi_cs_is_valid(unsigned int bus, unsigned int cs) > { > - return bus == 0 && cs == 0; > + if ((bus == SPI0_BUS) && (cs < SPI0_NUM_CS)) > + return 1; > + else if (bus1_cs_valid(bus, cs)) > + return 1; > + else if (bus2_cs_valid(bus, cs)) > + return 1; > + return 0; > } > > void spi_cs_activate(struct spi_slave *slave) > diff --git a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h > index 33f69b5..d4612d3 100644 > --- a/drivers/spi/davinci_spi.h > +++ b/drivers/spi/davinci_spi.h > @@ -74,6 +74,39 @@ struct davinci_spi_regs { > /* SPIDEF */ > #define SPIDEF_CSDEF0_MASK BIT(0) > > +#define SPI0_BUS 0 > +#define SPI0_BASE CONFIG_SYS_SPI_BASE > +/* > + * Define default SPI0_NUM_CS as 1 for existing platforms that uses this > + * driver. Platform can configure number of CS using CONFIG_SYS_SPI0_NUM_CS > + * if more than one CS is supported and by defining CONFIG_SYS_SPI0. > + */ > +#ifndef CONFIG_SYS_SPI0 > +#define SPI0_NUM_CS 1 > +#else > +#define SPI0_NUM_CS CONFIG_SYS_SPI0_NUM_CS > +#endif > + > +/* > + * define CONFIG_SYS_SPI1 when platform has spi-1 device (bus #1) and > + * CONFIG_SYS_SPI1_NUM_CS defines number of CS on this bus > + */ > +#ifdef CONFIG_SYS_SPI1 > +#define SPI1_BUS 1 > +#define SPI1_NUM_CS CONFIG_SYS_SPI1_NUM_CS > +#define SPI1_BASE CONFIG_SYS_SPI1_BASE > +#endif > + > +/* > + * define CONFIG_SYS_SPI2 when platform has spi-2 device (bus #2) and > + * CONFIG_SYS_SPI2_NUM_CS defines number of CS on this bus > + */ > +#ifdef CONFIG_SYS_SPI2 > +#define SPI2_BUS 2 > +#define SPI2_NUM_CS CONFIG_SYS_SPI2_NUM_CS > +#define SPI2_BASE CONFIG_SYS_SPI2_BASE > +#endif > + > struct davinci_spi_slave { > struct spi_slave slave; > struct davinci_spi_regs *regs; > -- > 1.7.9.5 This code looks more static, can you try get the bus and cs from sf probe and according assign the reg_base. fyi: look at drivers/spi/zynq_spi.c where based on the bus number we assign the reg_base and prior to that we can do the same for bus and cs. Let me know for any more info. thanks!
>-----Original Message----- >From: Jagan Teki [mailto:jagannadh.teki@gmail.com] >Sent: Sunday, March 23, 2014 7:07 AM >To: Karicheri, Muralidharan >Cc: Rini, Tom; u-boot@lists.denx.de; Chang, Rex >Subject: Re: [U-Boot] [PATCH v3 8/9] spi: davinci: add support for multiple bus and chip >select > >Hi, > >On Sat, Mar 22, 2014 at 2:21 AM, Murali Karicheri <m-karicheri2@ti.com> wrote: >> Currently davinci spi driver supports only bus 0 cs 0. >> This patch allows driver to support bus 1 and bus 2 with configurable >> number of chip selects. Also defaults are selected in a way to avoid >> regression on other platforms that uses davinci spi driver and has >> only one spi bus. >> >> Signed-off-by: Rex Chang <rchang@ti.com> >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >> Acked-by: Tom Rini <trini@ti.com> >> --- >> drivers/spi/davinci_spi.c | 60 >++++++++++++++++++++++++++++++++++++++++++--- >> drivers/spi/davinci_spi.h | 33 +++++++++++++++++++++++++ >> 2 files changed, 90 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c >> index e3fb321..b682bc4 100644 >> --- a/drivers/spi/davinci_spi.c >> +++ b/drivers/spi/davinci_spi.c >> @@ -32,7 +32,27 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int >cs, >> if (!ds) >> return NULL; >> >> - ds->regs = (struct davinci_spi_regs *)CONFIG_SYS_SPI_BASE; >> + ds->slave.bus = bus; >> + ds->slave.cs = cs; >> + >> + switch (bus) { >> + case SPI0_BUS: >> + ds->regs = (struct davinci_spi_regs *)SPI0_BASE; >> + break; >> +#ifdef CONFIG_SYS_SPI1 >> + case SPI1_BUS: >> + ds->regs = (struct davinci_spi_regs *)SPI0_BASE; >> + break; >> +#endif >> +#ifdef CONFIG_SYS_SPI2 >> + case SPI2_BUS: >> + ds->regs = (struct davinci_spi_regs *)SPI2_BASE; >> + break; >> +#endif >> + default: /* Invalid bus number */ >> + return NULL; >> + } >> + >> ds->freq = max_hz; >> >> return &ds->slave; >> @@ -59,7 +79,7 @@ int spi_claim_bus(struct spi_slave *slave) >> writel(SPIGCR1_MASTER_MASK | SPIGCR1_CLKMOD_MASK, >> &ds->regs->gcr1); >> >> /* CS, CLK, SIMO and SOMI are functional pins */ >> - writel((SPIPC0_EN0FUN_MASK | SPIPC0_CLKFUN_MASK | >> + writel(((1 << slave->cs) | SPIPC0_CLKFUN_MASK | >> SPIPC0_DOFUN_MASK | SPIPC0_DIFUN_MASK), >> &ds->regs->pc0); >> >> /* setup format */ >> @@ -262,9 +282,43 @@ out: >> return 0; >> } >> >> +#ifdef CONFIG_SYS_SPI1 >> +static int bus1_cs_valid(unsigned int bus, unsigned int cs) { >> + if ((bus == SPI1_BUS) && (cs < SPI1_NUM_CS)) >> + return 1; >> + return 0; >> +} >> +#else >> +static int bus1_cs_valid(unsigned int bus, unsigned int cs) { >> + return 0; >> +} >> +#endif >> + >> +#ifdef CONFIG_SYS_SPI2 >> +static int bus2_cs_valid(unsigned int bus, unsigned int cs) { >> + if ((bus == SPI2_BUS) && (cs < SPI2_NUM_CS)) >> + return 1; >> + return 0; >> +} >> +#else >> +static int bus2_cs_valid(unsigned int bus, unsigned int cs) { >> + return 0; >> +} >> +#endif >> + >> int spi_cs_is_valid(unsigned int bus, unsigned int cs) { >> - return bus == 0 && cs == 0; >> + if ((bus == SPI0_BUS) && (cs < SPI0_NUM_CS)) >> + return 1; >> + else if (bus1_cs_valid(bus, cs)) >> + return 1; >> + else if (bus2_cs_valid(bus, cs)) >> + return 1; >> + return 0; >> } >> >> void spi_cs_activate(struct spi_slave *slave) diff --git >> a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h index >> 33f69b5..d4612d3 100644 >> --- a/drivers/spi/davinci_spi.h >> +++ b/drivers/spi/davinci_spi.h >> @@ -74,6 +74,39 @@ struct davinci_spi_regs { >> /* SPIDEF */ >> #define SPIDEF_CSDEF0_MASK BIT(0) >> >> +#define SPI0_BUS 0 >> +#define SPI0_BASE CONFIG_SYS_SPI_BASE >> +/* >> + * Define default SPI0_NUM_CS as 1 for existing platforms that uses >> +this >> + * driver. Platform can configure number of CS using >> +CONFIG_SYS_SPI0_NUM_CS >> + * if more than one CS is supported and by defining CONFIG_SYS_SPI0. >> + */ >> +#ifndef CONFIG_SYS_SPI0 >> +#define SPI0_NUM_CS 1 >> +#else >> +#define SPI0_NUM_CS CONFIG_SYS_SPI0_NUM_CS >> +#endif >> + >> +/* >> + * define CONFIG_SYS_SPI1 when platform has spi-1 device (bus #1) and >> + * CONFIG_SYS_SPI1_NUM_CS defines number of CS on this bus */ #ifdef >> +CONFIG_SYS_SPI1 >> +#define SPI1_BUS 1 >> +#define SPI1_NUM_CS CONFIG_SYS_SPI1_NUM_CS >> +#define SPI1_BASE CONFIG_SYS_SPI1_BASE >> +#endif >> + >> +/* >> + * define CONFIG_SYS_SPI2 when platform has spi-2 device (bus #2) and >> + * CONFIG_SYS_SPI2_NUM_CS defines number of CS on this bus */ #ifdef >> +CONFIG_SYS_SPI2 >> +#define SPI2_BUS 2 >> +#define SPI2_NUM_CS CONFIG_SYS_SPI2_NUM_CS >> +#define SPI2_BASE CONFIG_SYS_SPI2_BASE >> +#endif >> + >> struct davinci_spi_slave { >> struct spi_slave slave; >> struct davinci_spi_regs *regs; >> -- >> 1.7.9.5 > >This code looks more static, can you try get the bus and cs from sf probe and according >assign the reg_base. > >fyi: look at drivers/spi/zynq_spi.c where based on the bus number we assign the reg_base >and prior to that we can do the same for bus and cs. > Jagan, Thanks for your comments. I looked at the driver code. In the example driver you provided the reg_base is chosen based on device number and the hardware.h define the base0 and base1 address and assign it based on device number. davinci_spi is re-used on many devices. All of the DaVinci devices such as dm644x, dm355, dm365 etc that mostly has one SPI device vs keystone devices such k2h/k that has 3 devices. If I follow this approach, I need to define BASE1 and BASE2 in the hardware.h of all devices that this software must run. This is not right since DaVinci devices has only one SPI device I think this is what best we can do given that the driver needs to be re-used across multiple devices and static is the way to go unless you have better suggestion to handle this scenario. Thanks. Murali >Let me know for any more info. > >thanks! >-- >Jagan.
On Thu, Mar 27, 2014 at 1:19 AM, Karicheri, Muralidharan <m-karicheri2@ti.com> wrote: >>-----Original Message----- >>From: Jagan Teki [mailto:jagannadh.teki@gmail.com] >>Sent: Sunday, March 23, 2014 7:07 AM >>To: Karicheri, Muralidharan >>Cc: Rini, Tom; u-boot@lists.denx.de; Chang, Rex >>Subject: Re: [U-Boot] [PATCH v3 8/9] spi: davinci: add support for multiple bus and chip >>select >> >>Hi, >> >>On Sat, Mar 22, 2014 at 2:21 AM, Murali Karicheri <m-karicheri2@ti.com> wrote: >>> Currently davinci spi driver supports only bus 0 cs 0. >>> This patch allows driver to support bus 1 and bus 2 with configurable >>> number of chip selects. Also defaults are selected in a way to avoid >>> regression on other platforms that uses davinci spi driver and has >>> only one spi bus. >>> >>> Signed-off-by: Rex Chang <rchang@ti.com> >>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >>> Acked-by: Tom Rini <trini@ti.com> >>> --- >>> drivers/spi/davinci_spi.c | 60 >>++++++++++++++++++++++++++++++++++++++++++--- >>> drivers/spi/davinci_spi.h | 33 +++++++++++++++++++++++++ >>> 2 files changed, 90 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c >>> index e3fb321..b682bc4 100644 >>> --- a/drivers/spi/davinci_spi.c >>> +++ b/drivers/spi/davinci_spi.c >>> @@ -32,7 +32,27 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int >>cs, >>> if (!ds) >>> return NULL; >>> >>> - ds->regs = (struct davinci_spi_regs *)CONFIG_SYS_SPI_BASE; >>> + ds->slave.bus = bus; >>> + ds->slave.cs = cs; >>> + >>> + switch (bus) { >>> + case SPI0_BUS: >>> + ds->regs = (struct davinci_spi_regs *)SPI0_BASE; >>> + break; >>> +#ifdef CONFIG_SYS_SPI1 >>> + case SPI1_BUS: >>> + ds->regs = (struct davinci_spi_regs *)SPI0_BASE; >>> + break; >>> +#endif >>> +#ifdef CONFIG_SYS_SPI2 >>> + case SPI2_BUS: >>> + ds->regs = (struct davinci_spi_regs *)SPI2_BASE; >>> + break; >>> +#endif >>> + default: /* Invalid bus number */ >>> + return NULL; >>> + } >>> + >>> ds->freq = max_hz; >>> >>> return &ds->slave; >>> @@ -59,7 +79,7 @@ int spi_claim_bus(struct spi_slave *slave) >>> writel(SPIGCR1_MASTER_MASK | SPIGCR1_CLKMOD_MASK, >>> &ds->regs->gcr1); >>> >>> /* CS, CLK, SIMO and SOMI are functional pins */ >>> - writel((SPIPC0_EN0FUN_MASK | SPIPC0_CLKFUN_MASK | >>> + writel(((1 << slave->cs) | SPIPC0_CLKFUN_MASK | >>> SPIPC0_DOFUN_MASK | SPIPC0_DIFUN_MASK), >>> &ds->regs->pc0); >>> >>> /* setup format */ >>> @@ -262,9 +282,43 @@ out: >>> return 0; >>> } >>> >>> +#ifdef CONFIG_SYS_SPI1 >>> +static int bus1_cs_valid(unsigned int bus, unsigned int cs) { >>> + if ((bus == SPI1_BUS) && (cs < SPI1_NUM_CS)) >>> + return 1; >>> + return 0; >>> +} >>> +#else >>> +static int bus1_cs_valid(unsigned int bus, unsigned int cs) { >>> + return 0; >>> +} >>> +#endif >>> + >>> +#ifdef CONFIG_SYS_SPI2 >>> +static int bus2_cs_valid(unsigned int bus, unsigned int cs) { >>> + if ((bus == SPI2_BUS) && (cs < SPI2_NUM_CS)) >>> + return 1; >>> + return 0; >>> +} >>> +#else >>> +static int bus2_cs_valid(unsigned int bus, unsigned int cs) { >>> + return 0; >>> +} >>> +#endif >>> + >>> int spi_cs_is_valid(unsigned int bus, unsigned int cs) { >>> - return bus == 0 && cs == 0; >>> + if ((bus == SPI0_BUS) && (cs < SPI0_NUM_CS)) >>> + return 1; >>> + else if (bus1_cs_valid(bus, cs)) >>> + return 1; >>> + else if (bus2_cs_valid(bus, cs)) >>> + return 1; >>> + return 0; >>> } >>> >>> void spi_cs_activate(struct spi_slave *slave) diff --git >>> a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h index >>> 33f69b5..d4612d3 100644 >>> --- a/drivers/spi/davinci_spi.h >>> +++ b/drivers/spi/davinci_spi.h >>> @@ -74,6 +74,39 @@ struct davinci_spi_regs { >>> /* SPIDEF */ >>> #define SPIDEF_CSDEF0_MASK BIT(0) >>> >>> +#define SPI0_BUS 0 >>> +#define SPI0_BASE CONFIG_SYS_SPI_BASE >>> +/* >>> + * Define default SPI0_NUM_CS as 1 for existing platforms that uses >>> +this >>> + * driver. Platform can configure number of CS using >>> +CONFIG_SYS_SPI0_NUM_CS >>> + * if more than one CS is supported and by defining CONFIG_SYS_SPI0. >>> + */ >>> +#ifndef CONFIG_SYS_SPI0 >>> +#define SPI0_NUM_CS 1 >>> +#else >>> +#define SPI0_NUM_CS CONFIG_SYS_SPI0_NUM_CS >>> +#endif >>> + >>> +/* >>> + * define CONFIG_SYS_SPI1 when platform has spi-1 device (bus #1) and >>> + * CONFIG_SYS_SPI1_NUM_CS defines number of CS on this bus */ #ifdef >>> +CONFIG_SYS_SPI1 >>> +#define SPI1_BUS 1 >>> +#define SPI1_NUM_CS CONFIG_SYS_SPI1_NUM_CS >>> +#define SPI1_BASE CONFIG_SYS_SPI1_BASE >>> +#endif >>> + >>> +/* >>> + * define CONFIG_SYS_SPI2 when platform has spi-2 device (bus #2) and >>> + * CONFIG_SYS_SPI2_NUM_CS defines number of CS on this bus */ #ifdef >>> +CONFIG_SYS_SPI2 >>> +#define SPI2_BUS 2 >>> +#define SPI2_NUM_CS CONFIG_SYS_SPI2_NUM_CS >>> +#define SPI2_BASE CONFIG_SYS_SPI2_BASE >>> +#endif >>> + >>> struct davinci_spi_slave { >>> struct spi_slave slave; >>> struct davinci_spi_regs *regs; >>> -- >>> 1.7.9.5 >> >>This code looks more static, can you try get the bus and cs from sf probe and according >>assign the reg_base. >> >>fyi: look at drivers/spi/zynq_spi.c where based on the bus number we assign the reg_base >>and prior to that we can do the same for bus and cs. >> > > Jagan, > > Thanks for your comments. > > I looked at the driver code. In the example driver you provided > the reg_base is chosen based on device number and the hardware.h > define the base0 and base1 address and assign it based on device > number. davinci_spi is re-used on many devices. All of the > DaVinci devices such as dm644x, dm355, dm365 etc that mostly has > one SPI device vs keystone devices such k2h/k that has 3 devices. > If I follow this approach, I need to define BASE1 and BASE2 > in the hardware.h of all devices that this software must run. > This is not right since DaVinci devices has only one SPI device > > I think this is what best we can do given that the driver needs to be > re-used across multiple devices and static is the way to go unless > you have better suggestion to handle this scenario. Agree with your comments as reg_base is not possible to define at one place due to this you may not try as I suggested. OK, atleast can you aviod using busx_cs_valid() calls, try to manage spi_cs_is_valid() itself. thanks!
>-----Original Message----- >From: Jagan Teki [mailto:jagannadh.teki@gmail.com] >Sent: Thursday, March 27, 2014 1:47 PM >To: Karicheri, Muralidharan >Cc: Rini, Tom; u-boot@lists.denx.de; Chang, Rex >Subject: Re: [U-Boot] [PATCH v3 8/9] spi: davinci: add support for multiple bus and chip >select > >On Thu, Mar 27, 2014 at 1:19 AM, Karicheri, Muralidharan <m-karicheri2@ti.com> wrote: >>>-----Original Message----- >>>From: Jagan Teki [mailto:jagannadh.teki@gmail.com] >>>Sent: Sunday, March 23, 2014 7:07 AM >>>To: Karicheri, Muralidharan >>>Cc: Rini, Tom; u-boot@lists.denx.de; Chang, Rex >>>Subject: Re: [U-Boot] [PATCH v3 8/9] spi: davinci: add support for >>>multiple bus and chip select >>> >>>Hi, >>> >>>On Sat, Mar 22, 2014 at 2:21 AM, Murali Karicheri <m-karicheri2@ti.com> wrote: >>>> Currently davinci spi driver supports only bus 0 cs 0. >>>> This patch allows driver to support bus 1 and bus 2 with >>>> configurable number of chip selects. Also defaults are selected in a >>>> way to avoid regression on other platforms that uses davinci spi >>>> driver and has only one spi bus. >>>> >>>> Signed-off-by: Rex Chang <rchang@ti.com> >>>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >>>> Acked-by: Tom Rini <trini@ti.com> >>>> --- >>>> drivers/spi/davinci_spi.c | 60 >>>++++++++++++++++++++++++++++++++++++++++++--- >>>> drivers/spi/davinci_spi.h | 33 +++++++++++++++++++++++++ >>>> 2 files changed, 90 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c >>>> index e3fb321..b682bc4 100644 >>>> --- a/drivers/spi/davinci_spi.c >>>> +++ b/drivers/spi/davinci_spi.c >>>> @@ -32,7 +32,27 @@ struct spi_slave *spi_setup_slave(unsigned int >>>> bus, unsigned int >>>cs, >>>> if (!ds) >>>> return NULL; >>>> >>>> - ds->regs = (struct davinci_spi_regs *)CONFIG_SYS_SPI_BASE; >>>> + ds->slave.bus = bus; >>>> + ds->slave.cs = cs; >>>> + >>>> + switch (bus) { >>>> + case SPI0_BUS: >>>> + ds->regs = (struct davinci_spi_regs *)SPI0_BASE; >>>> + break; >>>> +#ifdef CONFIG_SYS_SPI1 >>>> + case SPI1_BUS: >>>> + ds->regs = (struct davinci_spi_regs *)SPI0_BASE; >>>> + break; >>>> +#endif >>>> +#ifdef CONFIG_SYS_SPI2 >>>> + case SPI2_BUS: >>>> + ds->regs = (struct davinci_spi_regs *)SPI2_BASE; >>>> + break; >>>> +#endif >>>> + default: /* Invalid bus number */ >>>> + return NULL; >>>> + } >>>> + >>>> ds->freq = max_hz; >>>> >>>> return &ds->slave; >>>> @@ -59,7 +79,7 @@ int spi_claim_bus(struct spi_slave *slave) >>>> writel(SPIGCR1_MASTER_MASK | SPIGCR1_CLKMOD_MASK, >>>> &ds->regs->gcr1); >>>> >>>> /* CS, CLK, SIMO and SOMI are functional pins */ >>>> - writel((SPIPC0_EN0FUN_MASK | SPIPC0_CLKFUN_MASK | >>>> + writel(((1 << slave->cs) | SPIPC0_CLKFUN_MASK | >>>> SPIPC0_DOFUN_MASK | SPIPC0_DIFUN_MASK), >>>> &ds->regs->pc0); >>>> >>>> /* setup format */ >>>> @@ -262,9 +282,43 @@ out: >>>> return 0; >>>> } >>>> >>>> +#ifdef CONFIG_SYS_SPI1 >>>> +static int bus1_cs_valid(unsigned int bus, unsigned int cs) { >>>> + if ((bus == SPI1_BUS) && (cs < SPI1_NUM_CS)) >>>> + return 1; >>>> + return 0; >>>> +} >>>> +#else >>>> +static int bus1_cs_valid(unsigned int bus, unsigned int cs) { >>>> + return 0; >>>> +} >>>> +#endif >>>> + >>>> +#ifdef CONFIG_SYS_SPI2 >>>> +static int bus2_cs_valid(unsigned int bus, unsigned int cs) { >>>> + if ((bus == SPI2_BUS) && (cs < SPI2_NUM_CS)) >>>> + return 1; >>>> + return 0; >>>> +} >>>> +#else >>>> +static int bus2_cs_valid(unsigned int bus, unsigned int cs) { >>>> + return 0; >>>> +} >>>> +#endif >>>> + >>>> int spi_cs_is_valid(unsigned int bus, unsigned int cs) { >>>> - return bus == 0 && cs == 0; >>>> + if ((bus == SPI0_BUS) && (cs < SPI0_NUM_CS)) >>>> + return 1; >>>> + else if (bus1_cs_valid(bus, cs)) >>>> + return 1; >>>> + else if (bus2_cs_valid(bus, cs)) >>>> + return 1; >>>> + return 0; >>>> } >>>> >>>> void spi_cs_activate(struct spi_slave *slave) diff --git >>>> a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h index >>>> 33f69b5..d4612d3 100644 >>>> --- a/drivers/spi/davinci_spi.h >>>> +++ b/drivers/spi/davinci_spi.h >>>> @@ -74,6 +74,39 @@ struct davinci_spi_regs { >>>> /* SPIDEF */ >>>> #define SPIDEF_CSDEF0_MASK BIT(0) >>>> >>>> +#define SPI0_BUS 0 >>>> +#define SPI0_BASE CONFIG_SYS_SPI_BASE >>>> +/* >>>> + * Define default SPI0_NUM_CS as 1 for existing platforms that uses >>>> +this >>>> + * driver. Platform can configure number of CS using >>>> +CONFIG_SYS_SPI0_NUM_CS >>>> + * if more than one CS is supported and by defining CONFIG_SYS_SPI0. >>>> + */ >>>> +#ifndef CONFIG_SYS_SPI0 >>>> +#define SPI0_NUM_CS 1 >>>> +#else >>>> +#define SPI0_NUM_CS CONFIG_SYS_SPI0_NUM_CS >>>> +#endif >>>> + >>>> +/* >>>> + * define CONFIG_SYS_SPI1 when platform has spi-1 device (bus #1) >>>> +and >>>> + * CONFIG_SYS_SPI1_NUM_CS defines number of CS on this bus */ >>>> +#ifdef >>>> +CONFIG_SYS_SPI1 >>>> +#define SPI1_BUS 1 >>>> +#define SPI1_NUM_CS CONFIG_SYS_SPI1_NUM_CS >>>> +#define SPI1_BASE CONFIG_SYS_SPI1_BASE >>>> +#endif >>>> + >>>> +/* >>>> + * define CONFIG_SYS_SPI2 when platform has spi-2 device (bus #2) >>>> +and >>>> + * CONFIG_SYS_SPI2_NUM_CS defines number of CS on this bus */ >>>> +#ifdef >>>> +CONFIG_SYS_SPI2 >>>> +#define SPI2_BUS 2 >>>> +#define SPI2_NUM_CS CONFIG_SYS_SPI2_NUM_CS >>>> +#define SPI2_BASE CONFIG_SYS_SPI2_BASE >>>> +#endif >>>> + >>>> struct davinci_spi_slave { >>>> struct spi_slave slave; >>>> struct davinci_spi_regs *regs; >>>> -- >>>> 1.7.9.5 >>> >>>This code looks more static, can you try get the bus and cs from sf >>>probe and according assign the reg_base. >>> >>>fyi: look at drivers/spi/zynq_spi.c where based on the bus number we >>>assign the reg_base and prior to that we can do the same for bus and cs. >>> >> >> Jagan, >> >> Thanks for your comments. >> >> I looked at the driver code. In the example driver you provided the >> reg_base is chosen based on device number and the hardware.h define >> the base0 and base1 address and assign it based on device number. >> davinci_spi is re-used on many devices. All of the DaVinci devices >> such as dm644x, dm355, dm365 etc that mostly has one SPI device vs >> keystone devices such k2h/k that has 3 devices. >> If I follow this approach, I need to define BASE1 and BASE2 in the >> hardware.h of all devices that this software must run. >> This is not right since DaVinci devices has only one SPI device >> >> I think this is what best we can do given that the driver needs to be >> re-used across multiple devices and static is the way to go unless you >> have better suggestion to handle this scenario. > >Agree with your comments as reg_base is not possible to define at one place due to this >you may not try as I suggested. > >OK, atleast can you aviod using busx_cs_valid() calls, try to manage >spi_cs_is_valid() itself. > Jagan, Thanks again. But to me it is just a personal opinion. I would let it go ASIS unless you can give me a strong reason to change the code and provide a sample as well. The if else statement with #ifdef interleaved will make it really messy. I will make it static inline as the function call is unnecessary. Murali >thanks! >-- >Jagan.
>-----Original Message----- >From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] On Behalf Of >Karicheri, Muralidharan >Sent: Monday, March 31, 2014 4:10 PM >To: Jagan Teki >Cc: Rini, Tom; u-boot@lists.denx.de; Chang, Rex >Subject: Re: [U-Boot] [PATCH v3 8/9] spi: davinci: add support for multiple bus and chip >select > >>-----Original Message----- >>From: Jagan Teki [mailto:jagannadh.teki@gmail.com] >>Sent: Thursday, March 27, 2014 1:47 PM >>To: Karicheri, Muralidharan >>Cc: Rini, Tom; u-boot@lists.denx.de; Chang, Rex >>Subject: Re: [U-Boot] [PATCH v3 8/9] spi: davinci: add support for >>multiple bus and chip select >> >>On Thu, Mar 27, 2014 at 1:19 AM, Karicheri, Muralidharan <m-karicheri2@ti.com> >wrote: >>>>-----Original Message----- >>>>From: Jagan Teki [mailto:jagannadh.teki@gmail.com] >>>>Sent: Sunday, March 23, 2014 7:07 AM >>>>To: Karicheri, Muralidharan >>>>Cc: Rini, Tom; u-boot@lists.denx.de; Chang, Rex >>>>Subject: Re: [U-Boot] [PATCH v3 8/9] spi: davinci: add support for >>>>multiple bus and chip select >>>> >>>>Hi, >>>> >>>>On Sat, Mar 22, 2014 at 2:21 AM, Murali Karicheri <m-karicheri2@ti.com> wrote: >>>>> Currently davinci spi driver supports only bus 0 cs 0. >>>>> This patch allows driver to support bus 1 and bus 2 with >>>>> configurable number of chip selects. Also defaults are selected in >>>>> a way to avoid regression on other platforms that uses davinci spi >>>>> driver and has only one spi bus. >>>>> >>>>> Signed-off-by: Rex Chang <rchang@ti.com> >>>>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >>>>> Acked-by: Tom Rini <trini@ti.com> >>>>> --- >>>>> drivers/spi/davinci_spi.c | 60 >>>>++++++++++++++++++++++++++++++++++++++++++--- >>>>> drivers/spi/davinci_spi.h | 33 +++++++++++++++++++++++++ >>>>> 2 files changed, 90 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c >>>>> index e3fb321..b682bc4 100644 >>>>> --- a/drivers/spi/davinci_spi.c >>>>> +++ b/drivers/spi/davinci_spi.c >>>>> @@ -32,7 +32,27 @@ struct spi_slave *spi_setup_slave(unsigned int >>>>> bus, unsigned int >>>>cs, >>>>> if (!ds) >>>>> return NULL; >>>>> >>>>> - ds->regs = (struct davinci_spi_regs *)CONFIG_SYS_SPI_BASE; >>>>> + ds->slave.bus = bus; >>>>> + ds->slave.cs = cs; >>>>> + >>>>> + switch (bus) { >>>>> + case SPI0_BUS: >>>>> + ds->regs = (struct davinci_spi_regs *)SPI0_BASE; >>>>> + break; >>>>> +#ifdef CONFIG_SYS_SPI1 >>>>> + case SPI1_BUS: >>>>> + ds->regs = (struct davinci_spi_regs *)SPI0_BASE; >>>>> + break; >>>>> +#endif >>>>> +#ifdef CONFIG_SYS_SPI2 >>>>> + case SPI2_BUS: >>>>> + ds->regs = (struct davinci_spi_regs *)SPI2_BASE; >>>>> + break; >>>>> +#endif >>>>> + default: /* Invalid bus number */ >>>>> + return NULL; >>>>> + } >>>>> + >>>>> ds->freq = max_hz; >>>>> >>>>> return &ds->slave; >>>>> @@ -59,7 +79,7 @@ int spi_claim_bus(struct spi_slave *slave) >>>>> writel(SPIGCR1_MASTER_MASK | SPIGCR1_CLKMOD_MASK, >>>>> &ds->regs->gcr1); >>>>> >>>>> /* CS, CLK, SIMO and SOMI are functional pins */ >>>>> - writel((SPIPC0_EN0FUN_MASK | SPIPC0_CLKFUN_MASK | >>>>> + writel(((1 << slave->cs) | SPIPC0_CLKFUN_MASK | >>>>> SPIPC0_DOFUN_MASK | SPIPC0_DIFUN_MASK), >>>>> &ds->regs->pc0); >>>>> >>>>> /* setup format */ >>>>> @@ -262,9 +282,43 @@ out: >>>>> return 0; >>>>> } >>>>> >>>>> +#ifdef CONFIG_SYS_SPI1 >>>>> +static int bus1_cs_valid(unsigned int bus, unsigned int cs) { >>>>> + if ((bus == SPI1_BUS) && (cs < SPI1_NUM_CS)) >>>>> + return 1; >>>>> + return 0; >>>>> +} >>>>> +#else >>>>> +static int bus1_cs_valid(unsigned int bus, unsigned int cs) { >>>>> + return 0; >>>>> +} >>>>> +#endif >>>>> + >>>>> +#ifdef CONFIG_SYS_SPI2 >>>>> +static int bus2_cs_valid(unsigned int bus, unsigned int cs) { >>>>> + if ((bus == SPI2_BUS) && (cs < SPI2_NUM_CS)) >>>>> + return 1; >>>>> + return 0; >>>>> +} >>>>> +#else >>>>> +static int bus2_cs_valid(unsigned int bus, unsigned int cs) { >>>>> + return 0; >>>>> +} >>>>> +#endif >>>>> + >>>>> int spi_cs_is_valid(unsigned int bus, unsigned int cs) { >>>>> - return bus == 0 && cs == 0; >>>>> + if ((bus == SPI0_BUS) && (cs < SPI0_NUM_CS)) >>>>> + return 1; >>>>> + else if (bus1_cs_valid(bus, cs)) >>>>> + return 1; >>>>> + else if (bus2_cs_valid(bus, cs)) >>>>> + return 1; >>>>> + return 0; >>>>> } >>>>> >>>>> void spi_cs_activate(struct spi_slave *slave) diff --git >>>>> a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h index >>>>> 33f69b5..d4612d3 100644 >>>>> --- a/drivers/spi/davinci_spi.h >>>>> +++ b/drivers/spi/davinci_spi.h >>>>> @@ -74,6 +74,39 @@ struct davinci_spi_regs { >>>>> /* SPIDEF */ >>>>> #define SPIDEF_CSDEF0_MASK BIT(0) >>>>> >>>>> +#define SPI0_BUS 0 >>>>> +#define SPI0_BASE CONFIG_SYS_SPI_BASE >>>>> +/* >>>>> + * Define default SPI0_NUM_CS as 1 for existing platforms that >>>>> +uses this >>>>> + * driver. Platform can configure number of CS using >>>>> +CONFIG_SYS_SPI0_NUM_CS >>>>> + * if more than one CS is supported and by defining CONFIG_SYS_SPI0. >>>>> + */ >>>>> +#ifndef CONFIG_SYS_SPI0 >>>>> +#define SPI0_NUM_CS 1 >>>>> +#else >>>>> +#define SPI0_NUM_CS CONFIG_SYS_SPI0_NUM_CS >>>>> +#endif >>>>> + >>>>> +/* >>>>> + * define CONFIG_SYS_SPI1 when platform has spi-1 device (bus #1) >>>>> +and >>>>> + * CONFIG_SYS_SPI1_NUM_CS defines number of CS on this bus */ >>>>> +#ifdef >>>>> +CONFIG_SYS_SPI1 >>>>> +#define SPI1_BUS 1 >>>>> +#define SPI1_NUM_CS CONFIG_SYS_SPI1_NUM_CS >>>>> +#define SPI1_BASE CONFIG_SYS_SPI1_BASE >>>>> +#endif >>>>> + >>>>> +/* >>>>> + * define CONFIG_SYS_SPI2 when platform has spi-2 device (bus #2) >>>>> +and >>>>> + * CONFIG_SYS_SPI2_NUM_CS defines number of CS on this bus */ >>>>> +#ifdef >>>>> +CONFIG_SYS_SPI2 >>>>> +#define SPI2_BUS 2 >>>>> +#define SPI2_NUM_CS CONFIG_SYS_SPI2_NUM_CS >>>>> +#define SPI2_BASE CONFIG_SYS_SPI2_BASE >>>>> +#endif >>>>> + >>>>> struct davinci_spi_slave { >>>>> struct spi_slave slave; >>>>> struct davinci_spi_regs *regs; >>>>> -- >>>>> 1.7.9.5 >>>> >>>>This code looks more static, can you try get the bus and cs from sf >>>>probe and according assign the reg_base. >>>> >>>>fyi: look at drivers/spi/zynq_spi.c where based on the bus number we >>>>assign the reg_base and prior to that we can do the same for bus and cs. >>>> >>> >>> Jagan, >>> >>> Thanks for your comments. >>> >>> I looked at the driver code. In the example driver you provided the >>> reg_base is chosen based on device number and the hardware.h define >>> the base0 and base1 address and assign it based on device number. >>> davinci_spi is re-used on many devices. All of the DaVinci devices >>> such as dm644x, dm355, dm365 etc that mostly has one SPI device vs >>> keystone devices such k2h/k that has 3 devices. >>> If I follow this approach, I need to define BASE1 and BASE2 in the >>> hardware.h of all devices that this software must run. >>> This is not right since DaVinci devices has only one SPI device >>> >>> I think this is what best we can do given that the driver needs to be >>> re-used across multiple devices and static is the way to go unless >>> you have better suggestion to handle this scenario. >> >>Agree with your comments as reg_base is not possible to define at one >>place due to this you may not try as I suggested. >> >>OK, atleast can you aviod using busx_cs_valid() calls, try to manage >>spi_cs_is_valid() itself. >> >Jagan, > >Thanks again. But to me it is just a personal opinion. I would let it go ASIS unless you can >give me a strong reason to change the code and provide a sample as well. The if else >statement with #ifdef interleaved will make it really messy. I will make it static inline as >the function call is unnecessary. > Jagan, Ok. I found my original code will add additional return statements on platforms that doesn't have more than one SPI device. So reworked the patch as you have suggested. Please see the v5 for the change and I will be sending the same today. Murali >Murali >>thanks! >>-- >>Jagan. >_______________________________________________ >U-Boot mailing list >U-Boot@lists.denx.de >http://lists.denx.de/mailman/listinfo/u-boot
diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c index e3fb321..b682bc4 100644 --- a/drivers/spi/davinci_spi.c +++ b/drivers/spi/davinci_spi.c @@ -32,7 +32,27 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs, if (!ds) return NULL; - ds->regs = (struct davinci_spi_regs *)CONFIG_SYS_SPI_BASE; + ds->slave.bus = bus; + ds->slave.cs = cs; + + switch (bus) { + case SPI0_BUS: + ds->regs = (struct davinci_spi_regs *)SPI0_BASE; + break; +#ifdef CONFIG_SYS_SPI1 + case SPI1_BUS: + ds->regs = (struct davinci_spi_regs *)SPI0_BASE; + break; +#endif +#ifdef CONFIG_SYS_SPI2 + case SPI2_BUS: + ds->regs = (struct davinci_spi_regs *)SPI2_BASE; + break; +#endif + default: /* Invalid bus number */ + return NULL; + } + ds->freq = max_hz; return &ds->slave; @@ -59,7 +79,7 @@ int spi_claim_bus(struct spi_slave *slave) writel(SPIGCR1_MASTER_MASK | SPIGCR1_CLKMOD_MASK, &ds->regs->gcr1); /* CS, CLK, SIMO and SOMI are functional pins */ - writel((SPIPC0_EN0FUN_MASK | SPIPC0_CLKFUN_MASK | + writel(((1 << slave->cs) | SPIPC0_CLKFUN_MASK | SPIPC0_DOFUN_MASK | SPIPC0_DIFUN_MASK), &ds->regs->pc0); /* setup format */ @@ -262,9 +282,43 @@ out: return 0; } +#ifdef CONFIG_SYS_SPI1 +static int bus1_cs_valid(unsigned int bus, unsigned int cs) +{ + if ((bus == SPI1_BUS) && (cs < SPI1_NUM_CS)) + return 1; + return 0; +} +#else +static int bus1_cs_valid(unsigned int bus, unsigned int cs) +{ + return 0; +} +#endif + +#ifdef CONFIG_SYS_SPI2 +static int bus2_cs_valid(unsigned int bus, unsigned int cs) +{ + if ((bus == SPI2_BUS) && (cs < SPI2_NUM_CS)) + return 1; + return 0; +} +#else +static int bus2_cs_valid(unsigned int bus, unsigned int cs) +{ + return 0; +} +#endif + int spi_cs_is_valid(unsigned int bus, unsigned int cs) { - return bus == 0 && cs == 0; + if ((bus == SPI0_BUS) && (cs < SPI0_NUM_CS)) + return 1; + else if (bus1_cs_valid(bus, cs)) + return 1; + else if (bus2_cs_valid(bus, cs)) + return 1; + return 0; } void spi_cs_activate(struct spi_slave *slave) diff --git a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h index 33f69b5..d4612d3 100644 --- a/drivers/spi/davinci_spi.h +++ b/drivers/spi/davinci_spi.h @@ -74,6 +74,39 @@ struct davinci_spi_regs { /* SPIDEF */ #define SPIDEF_CSDEF0_MASK BIT(0) +#define SPI0_BUS 0 +#define SPI0_BASE CONFIG_SYS_SPI_BASE +/* + * Define default SPI0_NUM_CS as 1 for existing platforms that uses this + * driver. Platform can configure number of CS using CONFIG_SYS_SPI0_NUM_CS + * if more than one CS is supported and by defining CONFIG_SYS_SPI0. + */ +#ifndef CONFIG_SYS_SPI0 +#define SPI0_NUM_CS 1 +#else +#define SPI0_NUM_CS CONFIG_SYS_SPI0_NUM_CS +#endif + +/* + * define CONFIG_SYS_SPI1 when platform has spi-1 device (bus #1) and + * CONFIG_SYS_SPI1_NUM_CS defines number of CS on this bus + */ +#ifdef CONFIG_SYS_SPI1 +#define SPI1_BUS 1 +#define SPI1_NUM_CS CONFIG_SYS_SPI1_NUM_CS +#define SPI1_BASE CONFIG_SYS_SPI1_BASE +#endif + +/* + * define CONFIG_SYS_SPI2 when platform has spi-2 device (bus #2) and + * CONFIG_SYS_SPI2_NUM_CS defines number of CS on this bus + */ +#ifdef CONFIG_SYS_SPI2 +#define SPI2_BUS 2 +#define SPI2_NUM_CS CONFIG_SYS_SPI2_NUM_CS +#define SPI2_BASE CONFIG_SYS_SPI2_BASE +#endif + struct davinci_spi_slave { struct spi_slave slave; struct davinci_spi_regs *regs;