>-----Original Message----- >From: Jagan Teki [mailto:jagannadh.t...@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-kariche...@ti.com> >wrote: >>>-----Original Message----- >>>From: Jagan Teki [mailto:jagannadh.t...@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-kariche...@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 <rch...@ti.com> >>>> Signed-off-by: Murali Karicheri <m-kariche...@ti.com> >>>> Acked-by: Tom Rini <tr...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
