[U-Boot] [PATCH] spi: kirkwood_spi: Add support for multiple chip-selects on MVEBU

[Stefan Roese <sr@denx.de>]

Hi Jagan,

On 06.04.2016 13:21, Jagan Teki wrote:
> On 5 April 2016 at 11:32, Stefan Roese <sr@denx.de> wrote:
>> Hi Jagan,
>>
>>
>> On 23.03.2016 15:36, Stefan Roese wrote:
>>>
>>> Hi Jagan,
>>>
>>> please excuse the long delay here. Some comments below.
>>>
>>> On 12.02.2016 21:27, Jagan Teki wrote:
>>>>
>>>> On 12 February 2016 at 18:15, Stefan Roese <sr@denx.de> wrote:
>>>>>
>>>>> Currently only chip-select 0 is supported by the kirkwood SPI driver.
>>>>> The Armada XP / 38x SoCs also use this driver and support multiple chip
>>>>> selects. This patch adds support for multiple CS on MVEBU.
>>>>>
>>>>> The register definitions are restructured a bit with this patch.
>>>>> Grouping
>>>>> them to the corresponding registers.
>>>>>
>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>> Cc: Luka Perkov <luka.perkov@sartura.hr>
>>>>> Cc: Jagan Teki <jteki@openedev.com>
>>>>> ---
>>>>>    arch/arm/include/asm/arch-mvebu/spi.h | 17 ++++++++++++-----
>>>>>    drivers/spi/kirkwood_spi.c            |  6 ++++++
>>>>>    2 files changed, 18 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/include/asm/arch-mvebu/spi.h
>>>>> b/arch/arm/include/asm/arch-mvebu/spi.h
>>>>> index e512dce..4b66499 100644
>>>>> --- a/arch/arm/include/asm/arch-mvebu/spi.h
>>>>> +++ b/arch/arm/include/asm/arch-mvebu/spi.h
>>>>> @@ -32,13 +32,15 @@ struct kwspi_registers {
>>>>>    #define SCK_MPP10      (1 << 1)
>>>>>    #define MISO_MPP11     (1 << 2)
>>>>>
>>>>> +/* Control Register */
>>>>> +#define KWSPI_CSN_ACT          (1 << 0) /* Activates serial memory
>>>>> interface */
>>>>> +#define KWSPI_SMEMRDY          (1 << 1) /* SerMem Data xfer ready */
>>>>> +#define KWSPI_CS_SHIFT         2       /* chip select shift */
>>>>> +#define KWSPI_CS_MASK          0x7     /* chip select mask */
>>>>> +
>>>>> +/* Configuration Register */
>>>>>    #define KWSPI_CLKPRESCL_MASK   0x1f
>>>>>    #define KWSPI_CLKPRESCL_MIN    0x12
>>>>> -#define KWSPI_CSN_ACT          1 /* Activates serial memory
>>>>> interface */
>>>>> -#define KWSPI_SMEMRDY          (1 << 1) /* SerMem Data xfer ready */
>>>>> -#define KWSPI_IRQUNMASK                1 /* unmask SPI interrupt */
>>>>> -#define KWSPI_IRQMASK          0 /* mask SPI interrupt */
>>>>> -#define KWSPI_SMEMRDIRQ                1 /* SerMem data xfer ready
>>>>> irq */
>>>>>    #define KWSPI_XFERLEN_1BYTE    0
>>>>>    #define KWSPI_XFERLEN_2BYTE    (1 << 5)
>>>>>    #define KWSPI_XFERLEN_MASK     (1 << 5)
>>>>> @@ -47,6 +49,11 @@ struct kwspi_registers {
>>>>>    #define KWSPI_ADRLEN_3BYTE     (2 << 8)
>>>>>    #define KWSPI_ADRLEN_4BYTE     (3 << 8)
>>>>>    #define KWSPI_ADRLEN_MASK      (3 << 8)
>>>>> +
>>>>> +#define KWSPI_IRQUNMASK                1 /* unmask SPI interrupt */
>>>>> +#define KWSPI_IRQMASK          0 /* mask SPI interrupt */
>>>>> +#define KWSPI_SMEMRDIRQ                1 /* SerMem data xfer ready
>>>>> irq */
>>>>> +
>>>>>    #define KWSPI_TIMEOUT          10000
>>>>>
>>>>>    #endif /* __KW_SPI_H__ */
>>>>> diff --git a/drivers/spi/kirkwood_spi.c b/drivers/spi/kirkwood_spi.c
>>>>> index 7890796..fa30be4 100644
>>>>> --- a/drivers/spi/kirkwood_spi.c
>>>>> +++ b/drivers/spi/kirkwood_spi.c
>>>>> @@ -280,6 +280,12 @@ static int mvebu_spi_xfer(struct udevice *dev,
>>>>> unsigned int bitlen,
>>>>>           struct udevice *bus = dev->parent;
>>>>>           struct mvebu_spi_platdata *plat = dev_get_platdata(bus);
>>>>>
>>>>> +#ifdef CONFIG_ARCH_MVEBU
>>>>> +       clrsetbits_le32(&plat->spireg->ctrl,
>>>>> +                       KWSPI_CS_MASK << KWSPI_CS_SHIFT,
>>>>> +                       spi_chip_select(dev) << KWSPI_CS_SHIFT);
>>>>> +#endif
>>>>
>>>>
>>>> This looks clearing a desired cs from 'sf probe' why is this need
>>>> here?
>>>
>>>
>>> Its not clearing a CS but configuring the CS number in the CTRL
>>> register. This configured CS number will get enabled or disabled
>>> exactly the same way as when its zero (without this patch) in
>>> the existing spi_cs_activate/deactivate functions
>
> OK, if ie the case configure doesn't require for each spi_xfer, isn't
> it?

Correct.

> may be we can put this on to probe or setup_slave area?

AFAIK, probe() is only called once per SPI controller. Without
any device (chip-select) to use. We need a function that's
called per SPI device for this specific chip-select
configuration. Checking a bit further, I could use claim_bus()
for this. If you are okay, I'll send a v2, using clain_bus()
for this CS configuration.

Okay?

Thanks,
Stefan