[U-Boot] [PATCH v3 8/9] spi: davinci: add support for multiple bus and chip select
Karicheri, Muralidharan
m-karicheri2 at ti.com
Mon Mar 31 22:09:40 CEST 2014
>-----Original Message-----
>From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
>Sent: Thursday, March 27, 2014 1:47 PM
>To: Karicheri, Muralidharan
>Cc: Rini, Tom; u-boot at 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 at ti.com> wrote:
>>>-----Original Message-----
>>>From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
>>>Sent: Sunday, March 23, 2014 7:07 AM
>>>To: Karicheri, Muralidharan
>>>Cc: Rini, Tom; u-boot at 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 at 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 at ti.com>
>>>> Signed-off-by: Murali Karicheri <m-karicheri2 at ti.com>
>>>> Acked-by: Tom Rini <trini at 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.
More information about the U-Boot
mailing list