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

[Jagan Teki]

Hi Stefan,

On 5 April 2016 at 11:32, Stefan Roese <sr at 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 at 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 at denx.de>
>>>> Cc: Luka Perkov <luka.perkov at sartura.hr>
>>>> Cc: Jagan Teki <jteki at 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? may be we can put this on to probe or setup_slave area?

>>
>>> I think we can do the desired cs activate or deactivate through
>>> existing spi_cs_activate/deactivate functions itself.
>>
>>
>> This is exactly what this patch does. The CS number is
>> configured with the clrsetbits_le32() call above. And activated /
>> deactivated in the existing functions.
>>
>> Or do you mean something else?
>
>
> Any updates on this? I would like to have this multiple CS support
> added in this release. I can push it via the marvell git repository
> if you have no objections.

thanks!
-- 
Jagan.