Patchwork [U-Boot,v4,05/10] SPI: Add Dove SPI driver

login
register
mail settings
Submitter Sascha Silbe
Date May 26, 2013, 6:36 p.m.
Message ID <1369593423-19763-6-git-send-email-t-uboot@infra-silbe.de>
Download mbox | patch
Permalink /patch/246468/
State Superseded
Delegated to: Jagannadha Sutradharudu Teki
Headers show

Comments

Sascha Silbe - May 26, 2013, 6:36 p.m.
From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

This adds an SPI driver for Marvell Dove SoCs. This driver is taken
from kirkwood_spi but removes mpp configuration as dove has dedicated
spi pins.

As a future clean-up step, the code for orion5x, kirkwood and dove
could be merged, with MPP configuration being be handled as part of
cpu/board-specific setup.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Signed-off-by: Sascha Silbe <t-uboot@infra-silbe.de>
---
 v3->v4: renamed to dove, adjusted description, removed unused
         variable, made checkpatch clean

 drivers/spi/Makefile   |   1 +
 drivers/spi/dove_spi.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 213 insertions(+)
 create mode 100644 drivers/spi/dove_spi.c
Jagannadha Sutradharudu Teki - June 3, 2013, 6:20 p.m.
Hi,

Looks ok to me as per coding style after a quick look.

On Mon, May 27, 2013 at 12:06 AM, Sascha Silbe <t-uboot@infra-silbe.de> wrote:
> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>
> This adds an SPI driver for Marvell Dove SoCs. This driver is taken
> from kirkwood_spi but removes mpp configuration as dove has dedicated
> spi pins.
>
> As a future clean-up step, the code for orion5x, kirkwood and dove
> could be merged, with MPP configuration being be handled as part of
> cpu/board-specific setup.
>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Signed-off-by: Sascha Silbe <t-uboot@infra-silbe.de>
> ---
>  v3->v4: renamed to dove, adjusted description, removed unused
>          variable, made checkpatch clean
>
>  drivers/spi/Makefile   |   1 +
>  drivers/spi/dove_spi.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 213 insertions(+)
>  create mode 100644 drivers/spi/dove_spi.c
>
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index d08609e..62ad970 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -38,6 +38,7 @@ COBJS-$(CONFIG_BFIN_SPI6XX) += bfin_spi6xx.o
>  COBJS-$(CONFIG_CF_SPI) += cf_spi.o
>  COBJS-$(CONFIG_CF_QSPI) += cf_qspi.o
>  COBJS-$(CONFIG_DAVINCI_SPI) += davinci_spi.o
> +COBJS-$(CONFIG_DOVE_SPI) += dove_spi.o
>  COBJS-$(CONFIG_EXYNOS_SPI) += exynos_spi.o
>  COBJS-$(CONFIG_ICH_SPI) +=  ich.o
>  COBJS-$(CONFIG_KIRKWOOD_SPI) += kirkwood_spi.o
> diff --git a/drivers/spi/dove_spi.c b/drivers/spi/dove_spi.c
> new file mode 100644
> index 0000000..c61ba89
> --- /dev/null
> +++ b/drivers/spi/dove_spi.c
> @@ -0,0 +1,212 @@
> +/*
> + * Marvell Dove SoCs common spi driver
> + *
> + * Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> + * based on kirkwood_spi.c written by
> + *  Prafulla Wadaskar <prafulla@marvell.com>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> + * MA 02110-1301 USA
> + */
> +
> +#include <common.h>
> +#include <malloc.h>
> +#include <spi.h>
> +#include <asm/io.h>
> +#include <asm/arch/config.h>
> +
> +/* SPI Registers on dove SOC */
> +struct dovespi_registers {
> +       u32 ctrl;       /* 0x00 */
> +       u32 cfg;        /* 0x04 */
> +       u32 dout;       /* 0x08 */
> +       u32 din;        /* 0x0c */
> +       u32 irq_cause;  /* 0x10 */
> +       u32 irq_mask;   /* 0x14 */
> +};
> +
> +#define DOVESPI_CLKPRESCL_MASK 0x1f
> +#define DOVESPI_CLKPRESCL_MIN  0x12
> +#define DOVESPI_CSN_ACT        1 /* Activates serial memory interface */
> +#define DOVESPI_SMEMRDY        (1 << 1) /* SerMem Data xfer ready */
> +#define DOVESPI_IRQUNMASK      1 /* unmask SPI interrupt */
> +#define DOVESPI_IRQMASK        0 /* mask SPI interrupt */
> +#define DOVESPI_SMEMRDIRQ      1 /* SerMem data xfer ready irq */
> +#define DOVESPI_XFERLEN_1BYTE  0
> +#define DOVESPI_XFERLEN_2BYTE  (1 << 5)
> +#define DOVESPI_XFERLEN_MASK   (1 << 5)
> +#define DOVESPI_ADRLEN_1BYTE   0
> +#define DOVESPI_ADRLEN_2BYTE   (1 << 8)
> +#define DOVESPI_ADRLEN_3BYTE   (2 << 8)
> +#define DOVESPI_ADRLEN_4BYTE   (3 << 8)
> +#define DOVESPI_ADRLEN_MASK    (3 << 8)
> +#define DOVESPI_TIMEOUT        10000
> +
> +static struct dovespi_registers *spireg =
> +       (struct dovespi_registers *)DOVE_SPI_BASE;
> +
> +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> +                               unsigned int max_hz, unsigned int mode)
> +{
> +       struct spi_slave *slave;
> +       u32 data;
> +
> +       if (!spi_cs_is_valid(bus, cs))
> +               return NULL;
> +

Done use the below tag code instead go for spi_alloc_slave()
see the sample code on "drivers/spi/exynos_spi.c"

----------------------- TAG+
> +       slave = malloc(sizeof(struct spi_slave));
> +       if (!slave)
> +               return NULL;
> +
> +       slave->bus = bus;
> +       slave->cs = cs;
> +
--------------------- TAG-

> +       writel(~DOVESPI_CSN_ACT | DOVESPI_SMEMRDY, &spireg->ctrl);
> +
> +       /* calculate spi clock prescaller using max_hz */
> +       data = ((CONFIG_SYS_TCLK / 2) / max_hz) + 0x10;
> +       data = data < DOVESPI_CLKPRESCL_MIN ? DOVESPI_CLKPRESCL_MIN : data;
> +       data = data > DOVESPI_CLKPRESCL_MASK ? DOVESPI_CLKPRESCL_MASK : data;
> +
> +       /* program spi clock prescaller using max_hz */
> +       writel(DOVESPI_ADRLEN_3BYTE | data, &spireg->cfg);
> +       debug("data = 0x%08x\n", data);
> +
> +       writel(DOVESPI_SMEMRDIRQ, &spireg->irq_cause);
> +       writel(DOVESPI_IRQMASK, &spireg->irq_mask);
> +
> +       return slave;
> +}
> +
> +void spi_free_slave(struct spi_slave *slave)
> +{
> +       free(slave);
> +}
> +
> +__attribute__((weak)) int board_spi_claim_bus(struct spi_slave *slave)

Why your using __attribute__((weak)) here, may be use to pre-load the
symbol library
but what is the use case here?

--
Thanks,
Jagan.
Jagannadha Sutradharudu Teki - June 12, 2013, 6:58 p.m.
Hi,

On 03-06-2013 23:50, Jagan Teki wrote:
> Hi,
>
> Looks ok to me as per coding style after a quick look.
>
> On Mon, May 27, 2013 at 12:06 AM, Sascha Silbe <t-uboot@infra-silbe.de> wrote:
>> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>
>> This adds an SPI driver for Marvell Dove SoCs. This driver is taken
>> from kirkwood_spi but removes mpp configuration as dove has dedicated
>> spi pins.
>>
>> As a future clean-up step, the code for orion5x, kirkwood and dove
>> could be merged, with MPP configuration being be handled as part of
>> cpu/board-specific setup.
>>
>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> Signed-off-by: Sascha Silbe <t-uboot@infra-silbe.de>
>> ---
>>   v3->v4: renamed to dove, adjusted description, removed unused
>>           variable, made checkpatch clean
>>
>>   drivers/spi/Makefile   |   1 +
>>   drivers/spi/dove_spi.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 213 insertions(+)
>>   create mode 100644 drivers/spi/dove_spi.c
>>
>> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
>> index d08609e..62ad970 100644
>> --- a/drivers/spi/Makefile
>> +++ b/drivers/spi/Makefile
>> @@ -38,6 +38,7 @@ COBJS-$(CONFIG_BFIN_SPI6XX) += bfin_spi6xx.o
>>   COBJS-$(CONFIG_CF_SPI) += cf_spi.o
>>   COBJS-$(CONFIG_CF_QSPI) += cf_qspi.o
>>   COBJS-$(CONFIG_DAVINCI_SPI) += davinci_spi.o
>> +COBJS-$(CONFIG_DOVE_SPI) += dove_spi.o
>>   COBJS-$(CONFIG_EXYNOS_SPI) += exynos_spi.o
>>   COBJS-$(CONFIG_ICH_SPI) +=  ich.o
>>   COBJS-$(CONFIG_KIRKWOOD_SPI) += kirkwood_spi.o
>> diff --git a/drivers/spi/dove_spi.c b/drivers/spi/dove_spi.c
>> new file mode 100644
>> index 0000000..c61ba89
>> --- /dev/null
>> +++ b/drivers/spi/dove_spi.c
>> @@ -0,0 +1,212 @@
>> +/*
>> + * Marvell Dove SoCs common spi driver
>> + *
>> + * Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> + * based on kirkwood_spi.c written by
>> + *  Prafulla Wadaskar <prafulla@marvell.com>
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
>> + * MA 02110-1301 USA
>> + */
>> +
>> +#include <common.h>
>> +#include <malloc.h>
>> +#include <spi.h>
>> +#include <asm/io.h>
>> +#include <asm/arch/config.h>
>> +
>> +/* SPI Registers on dove SOC */
>> +struct dovespi_registers {
>> +       u32 ctrl;       /* 0x00 */
>> +       u32 cfg;        /* 0x04 */
>> +       u32 dout;       /* 0x08 */
>> +       u32 din;        /* 0x0c */
>> +       u32 irq_cause;  /* 0x10 */
>> +       u32 irq_mask;   /* 0x14 */
>> +};
>> +
>> +#define DOVESPI_CLKPRESCL_MASK 0x1f
>> +#define DOVESPI_CLKPRESCL_MIN  0x12
>> +#define DOVESPI_CSN_ACT        1 /* Activates serial memory interface */
>> +#define DOVESPI_SMEMRDY        (1 << 1) /* SerMem Data xfer ready */
>> +#define DOVESPI_IRQUNMASK      1 /* unmask SPI interrupt */
>> +#define DOVESPI_IRQMASK        0 /* mask SPI interrupt */
>> +#define DOVESPI_SMEMRDIRQ      1 /* SerMem data xfer ready irq */
>> +#define DOVESPI_XFERLEN_1BYTE  0
>> +#define DOVESPI_XFERLEN_2BYTE  (1 << 5)
>> +#define DOVESPI_XFERLEN_MASK   (1 << 5)
>> +#define DOVESPI_ADRLEN_1BYTE   0
>> +#define DOVESPI_ADRLEN_2BYTE   (1 << 8)
>> +#define DOVESPI_ADRLEN_3BYTE   (2 << 8)
>> +#define DOVESPI_ADRLEN_4BYTE   (3 << 8)
>> +#define DOVESPI_ADRLEN_MASK    (3 << 8)
>> +#define DOVESPI_TIMEOUT        10000
>> +
>> +static struct dovespi_registers *spireg =
>> +       (struct dovespi_registers *)DOVE_SPI_BASE;
>> +
>> +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
>> +                               unsigned int max_hz, unsigned int mode)
>> +{
>> +       struct spi_slave *slave;
>> +       u32 data;
>> +
>> +       if (!spi_cs_is_valid(bus, cs))
>> +               return NULL;
>> +
>
> Done use the below tag code instead go for spi_alloc_slave()
> see the sample code on "drivers/spi/exynos_spi.c"
>
> ----------------------- TAG+
>> +       slave = malloc(sizeof(struct spi_slave));
>> +       if (!slave)
>> +               return NULL;
>> +
>> +       slave->bus = bus;
>> +       slave->cs = cs;
>> +
> --------------------- TAG-
>
>> +       writel(~DOVESPI_CSN_ACT | DOVESPI_SMEMRDY, &spireg->ctrl);
>> +
>> +       /* calculate spi clock prescaller using max_hz */
>> +       data = ((CONFIG_SYS_TCLK / 2) / max_hz) + 0x10;
>> +       data = data < DOVESPI_CLKPRESCL_MIN ? DOVESPI_CLKPRESCL_MIN : data;
>> +       data = data > DOVESPI_CLKPRESCL_MASK ? DOVESPI_CLKPRESCL_MASK : data;
>> +
>> +       /* program spi clock prescaller using max_hz */
>> +       writel(DOVESPI_ADRLEN_3BYTE | data, &spireg->cfg);
>> +       debug("data = 0x%08x\n", data);
>> +
>> +       writel(DOVESPI_SMEMRDIRQ, &spireg->irq_cause);
>> +       writel(DOVESPI_IRQMASK, &spireg->irq_mask);
>> +
>> +       return slave;
>> +}
>> +
>> +void spi_free_slave(struct spi_slave *slave)
>> +{
>> +       free(slave);
>> +}
>> +
>> +__attribute__((weak)) int board_spi_claim_bus(struct spi_slave *slave)
>
> Why your using __attribute__((weak)) here, may be use to pre-load the
> symbol library
> but what is the use case here?
>
> --
> Thanks,
> Jagan.
>
Any update on this.

Thanks,
Jagan.
Sebastian Hesselbarth - June 12, 2013, 7:26 p.m.
On 06/12/2013 08:58 PM, Jagan Teki wrote:
> On 03-06-2013 23:50, Jagan Teki wrote:
>> Looks ok to me as per coding style after a quick look.
>> On Mon, May 27, 2013 at 12:06 AM, Sascha Silbe
>> <t-uboot@infra-silbe.de> wrote:
>>> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>>
>>> This adds an SPI driver for Marvell Dove SoCs. This driver is taken
>>> from kirkwood_spi but removes mpp configuration as dove has dedicated
>>> spi pins.
>>>
>>> As a future clean-up step, the code for orion5x, kirkwood and dove
>>> could be merged, with MPP configuration being be handled as part of
>>> cpu/board-specific setup.
>>>
>>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>> Signed-off-by: Sascha Silbe <t-uboot@infra-silbe.de>
>>> ---
>>> v3->v4: renamed to dove, adjusted description, removed unused
>>> variable, made checkpatch clean
[...]
> Any update on this.

Is any of you even listening? Please do _not_ name it after Dove! It is
compatible with _at least_ Kirkwood, Orion5x and MV78x00. Now is the
chance to have a common name or you will end up with either non-sense
naming or four copies of that very driver.

Originally, it was named after the Linux group of SoCs compatible with
Dove (orion-spi), IIRC Prafulla suggested mv-spi.

Sebastian
Jagannadha Sutradharudu Teki - June 12, 2013, 7:30 p.m.
On Thu, Jun 13, 2013 at 12:56 AM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:
> On 06/12/2013 08:58 PM, Jagan Teki wrote:
>>
>> On 03-06-2013 23:50, Jagan Teki wrote:
>>>
>>> Looks ok to me as per coding style after a quick look.
>>> On Mon, May 27, 2013 at 12:06 AM, Sascha Silbe
>>> <t-uboot@infra-silbe.de> wrote:
>>>>
>>>> From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>>>
>>>> This adds an SPI driver for Marvell Dove SoCs. This driver is taken
>>>> from kirkwood_spi but removes mpp configuration as dove has dedicated
>>>> spi pins.
>>>>
>>>> As a future clean-up step, the code for orion5x, kirkwood and dove
>>>> could be merged, with MPP configuration being be handled as part of
>>>> cpu/board-specific setup.
>>>>
>>>> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>>>> Signed-off-by: Sascha Silbe <t-uboot@infra-silbe.de>
>>>> ---
>>>> v3->v4: renamed to dove, adjusted description, removed unused
>>>> variable, made checkpatch clean
>
> [...]
>>
>> Any update on this.
>
>
> Is any of you even listening? Please do _not_ name it after Dove! It is
> compatible with _at least_ Kirkwood, Orion5x and MV78x00. Now is the
> chance to have a common name or you will end up with either non-sense
> naming or four copies of that very driver.
>
> Originally, it was named after the Linux group of SoCs compatible with
> Dove (orion-spi), IIRC Prafulla suggested mv-spi.
>
> Sebastian

Common to use means orion_spi instead of Dove?

--
Thanks,
Jagan.
Sebastian Hesselbarth - June 12, 2013, 7:33 p.m.
On 06/12/2013 09:30 PM, Jagan Teki wrote:
> On Thu, Jun 13, 2013 at 12:56 AM, Sebastian Hesselbarth
> <sebastian.hesselbarth@gmail.com>  wrote:
>> Is any of you even listening? Please do _not_ name it after Dove! It is
>> compatible with _at least_ Kirkwood, Orion5x and MV78x00. Now is the
>> chance to have a common name or you will end up with either non-sense
>> naming or four copies of that very driver.
>>
>> Originally, it was named after the Linux group of SoCs compatible with
>> Dove (orion-spi), IIRC Prafulla suggested mv-spi.
>
> Common to use means orion_spi instead of Dove?

Either orion_spi as it was named originally, or mv_spi as Prafulla
suggested. Then move mpp (pinctrl) from kirkwood_spi to corresponding
boards, switch to orion_/mv_spi, and remove kirkwood_spi.

I suggest orion_spi, but Prafulla had his word so it should be mv_spi
I guess.

Sebastian
Sascha Silbe - June 25, 2013, 7:33 p.m.
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> writes:

> Either orion_spi as it was named originally, or mv_spi as Prafulla
> suggested. Then move mpp (pinctrl) from kirkwood_spi to corresponding
> boards, switch to orion_/mv_spi, and remove kirkwood_spi.
>
> I suggest orion_spi, but Prafulla had his word so it should be mv_spi
> I guess.

I've given it a try and modified the Kirkwood GPIO and SPI drivers
rather than duplicating their code. As I neither know the Dove SoCs nor
have any hardware using them, I don't feel comfortable merging this with
any existing orion code. For similar reasons and also for a lack of time
I've left the MPP support code in place, just protected with
CONFIG_KIRKWOOD.

As for the naming: there's already a GPIO driver called mvgpio that
looks quite different from the Kirkwood / Dove one. Naming the latter
one mv_gpio would work for the build tools, but needlessly cause
confusion to developers. Therefore I chose to keep the current names of
the Kirkwood drivers (kw_gpio, kirkwood_spi). If anyone has a better
suggestion, I'm happy to change the names.

Sebastian, does the resulting code still match your intentions or would
you like me to remove your Signed-off-by from those two patches when
posting the updated patch series?

Sascha
Sebastian Hesselbarth - June 25, 2013, 7:58 p.m.
On 06/25/2013 09:33 PM, Sascha Silbe wrote:
> Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>  writes:
>> Either orion_spi as it was named originally, or mv_spi as Prafulla
>> suggested. Then move mpp (pinctrl) from kirkwood_spi to corresponding
>> boards, switch to orion_/mv_spi, and remove kirkwood_spi.
>>
>> I suggest orion_spi, but Prafulla had his word so it should be mv_spi
>> I guess.
>
> I've given it a try and modified the Kirkwood GPIO and SPI drivers
> rather than duplicating their code. As I neither know the Dove SoCs nor
> have any hardware using them, I don't feel comfortable merging this with
> any existing orion code. For similar reasons and also for a lack of time
> I've left the MPP support code in place, just protected with
> CONFIG_KIRKWOOD.

Sascha,

I was under the impression that you resent the patches because you have
a CuBox you want to get supported. Anyway, I will not get back to
u-boot Dove support anytime soon. It just takes to long to get any
valuable review/ack from Marvell maintainer.

> As for the naming: there's already a GPIO driver called mvgpio that
> looks quite different from the Kirkwood / Dove one. Naming the latter
> one mv_gpio would work for the build tools, but needlessly cause
> confusion to developers. Therefore I chose to keep the current names of
> the Kirkwood drivers (kw_gpio, kirkwood_spi). If anyone has a better
> suggestion, I'm happy to change the names.

Naming was the main discussion of this patch set back then, there have
been some "???" on the mpp stuff. I suggested  several times not to use
"mv" as I knew that Marvell PXA also uses "mv" as prefix all over.

The PXA SPI controller is based on different IP. Unfortunately PXA
naming is not consitent over SoCs, Dove is also nick-named PXA510
sometimes.

Actually, we are merging Orion SoCs and Armada 370/XP to mach-mvebu -
so the best name would be mvebu_spi as the spi controller is also in
Armada 370/XP.

But Prafulla is u-boot Marvell maintainer, he said to use mv prefix -
not my call anymore.

> Sebastian, does the resulting code still match your intentions or would
> you like me to remove your Signed-off-by from those two patches when
> posting the updated patch series?

Uhm, did you forget to send code?

I will not likely get back to Dove support on u-boot. I cannot really
re-test on Kirkwood and it just takes to long to get feedback.
I suggest to remove my Signed-off-by and especially the MAINTAINERS
entry.

Sebastian
Sascha Silbe - June 25, 2013, 8:09 p.m.
Hello Jagan,

Jagan Teki <jagannadh.teki@gmail.com> writes:

> Looks ok to me as per coding style after a quick look.

Thanks for the review.


[...]
> Done use the below tag code instead go for spi_alloc_slave()
> see the sample code on "drivers/spi/exynos_spi.c"
>
> ----------------------- TAG+
>> +       slave = malloc(sizeof(struct spi_slave));
>> +       if (!slave)
>> +               return NULL;
>> +
>> +       slave->bus = bus;
>> +       slave->cs = cs;
>> +
> --------------------- TAG-

That's going to be fixed in the next version which builds directly on
kirkwood_spi (rather than duplicating it), which already uses
spi_alloc_slave().


>> +__attribute__((weak)) int board_spi_claim_bus(struct spi_slave *slave)
>
> Why your using __attribute__((weak)) here, may be use to pre-load the
> symbol library
> but what is the use case here?

That's coming from kirkwood_spi. The Keymile boards apparently use it to
select between NAND and SPI flash (see board/keymile/km_arm/km_arm.c).

Sascha
Sascha Silbe - June 25, 2013, 8:38 p.m.
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> writes:

[...]
> I was under the impression that you resent the patches because you have
> a CuBox you want to get supported.

Exactly. I'd like to see support for CuBox enter mainline
U-Boot. However, there's also a limit to the amount of time I can afford
to spend on it (both per week and in total). TBH, I would have hoped the
_vendor_ (SolidRun) would take a more active role in this. For that
reason I'm now going to evaluate a Wandboard Quad, where more effort
seems to get spent on upstreaming (not to mention that the schematics
are available, which makes working on board support a lot easier). When
I ordered the CuBox Pro, the then-existing Wandboard variants (Solo and
DualLite) lacked SATA support, but the Quad meets my requirements.


> Naming was the main discussion of this patch set back then, there have
> been some "???" on the mpp stuff. I suggested  several times not to use
> "mv" as I knew that Marvell PXA also uses "mv" as prefix all over.
>
> The PXA SPI controller is based on different IP. Unfortunately PXA
> naming is not consitent over SoCs, Dove is also nick-named PXA510
> sometimes.
>
> Actually, we are merging Orion SoCs and Armada 370/XP to mach-mvebu -
> so the best name would be mvebu_spi as the spi controller is also in
> Armada 370/XP.

You're confusing me even more than I already was. :)

It's probably best to leave this reorganisation to someone else who
better knows the relationships between the several SoC families from
Marvell.


[...]
> Uhm, did you forget to send code?

No, I was holding back until I knew whether you still feel comfortable
being associated with the patches. I'll send them out now.


> I will not likely get back to Dove support on u-boot. I cannot really
> re-test on Kirkwood and it just takes to long to get feedback.
> I suggest to remove my Signed-off-by and especially the MAINTAINERS
> entry.

OK, will do. Sorry you feel that way.

Sascha
Sebastian Hesselbarth - June 25, 2013, 8:50 p.m.
On 06/25/2013 10:38 PM, Sascha Silbe wrote:
> Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>  writes:
> [...]
>> I was under the impression that you resent the patches because you have
>> a CuBox you want to get supported.
>
> Exactly. I'd like to see support for CuBox enter mainline
> U-Boot. However, there's also a limit to the amount of time I can afford
> to spend on it (both per week and in total). TBH, I would have hoped the
> _vendor_ (SolidRun) would take a more active role in this. For that
> reason I'm now going to evaluate a Wandboard Quad, where more effort
> seems to get spent on upstreaming (not to mention that the schematics
> are available, which makes working on board support a lot easier). When
> I ordered the CuBox Pro, the then-existing Wandboard variants (Solo and
> DualLite) lacked SATA support, but the Quad meets my requirements.

 From what I know, Rabeeh would love to add support for CuBox to u-boot.
But I guess his spare time is very limited, too.

>> Naming was the main discussion of this patch set back then, there have
>> been some "???" on the mpp stuff. I suggested  several times not to use
>> "mv" as I knew that Marvell PXA also uses "mv" as prefix all over.
>>
>> The PXA SPI controller is based on different IP. Unfortunately PXA
>> naming is not consitent over SoCs, Dove is also nick-named PXA510
>> sometimes.
>>
>> Actually, we are merging Orion SoCs and Armada 370/XP to mach-mvebu -
>> so the best name would be mvebu_spi as the spi controller is also in
>> Armada 370/XP.
>
> You're confusing me even more than I already was. :)
>
> It's probably best to leave this reorganisation to someone else who
> better knows the relationships between the several SoC families from
> Marvell.

Linux has some good overview of the non-PXA Marvell SoCs. Naming is
a mess and IP is reused often. But from what I can say, Orion matches
Dove, Kirkwood, Orion5x, and Discovery Innovation (mv78x00). Armada
370/XP share some of the peripheral IP but especially irq is different.

With respect to SPI, mvebu (Orion + Armada 370/XP) maybe matches best
as it is used in all of them with minor differences. PXA type SoCs use
a different IP for SPI.

> [...]
>> Uhm, did you forget to send code?
>
> No, I was holding back until I knew whether you still feel comfortable
> being associated with the patches. I'll send them out now.

Ok, I will try to give it a review at least.

Sebastian
Sascha Silbe - June 25, 2013, 9:27 p.m.
Changes from v4:
- some patches got included in master, so they've been dropped from
  the series
- removed Sebastian Hesselbarth's Signed-off-by and MAINTAINERS entry
  on his request
- modified Kirkwood GPIO and SPI drivers rather than duplicating them


This version has only been tested (lightly) on CuBox Pro, not on
OpenRD (Kirkwood). The latter box is in production use, so I'd like to
know whether the current approach is acceptable before I spend
considerable time on testing.


Sascha Silbe (8):
  ARM: dove: add support for Marvell Dove SoC
  usb: ehci-marvell: add support for second USB controller
  GPIO: add Dove support to Kirkwood GPIO driver
  MMC: sdhci: Add support for dove sdhci
  SPI: add Dove support to Kirkwood SPI driver
  block: mvsata: add dove include
  NET: mvgbe: avoid unused variable warning when used without phylib
    support
  Boards: Add support for SolidRun CuBox

 arch/arm/cpu/armv7/dove/Makefile                   |  49 ++++
 arch/arm/cpu/armv7/dove/cpu.c                      | 274 ++++++++++++++++++
 arch/arm/cpu/armv7/dove/dram.c                     | 117 ++++++++
 arch/arm/cpu/armv7/dove/lowlevel_init.S            |  83 ++++++
 arch/arm/cpu/armv7/dove/mpp.c                      | 318 +++++++++++++++++++++
 arch/arm/cpu/armv7/dove/timer.c                    | 176 ++++++++++++
 arch/arm/cpu/armv7/dove/usb.c                      | 101 +++++++
 arch/arm/include/asm/arch-dove/config.h            | 153 ++++++++++
 arch/arm/include/asm/arch-dove/cpu.h               | 204 +++++++++++++
 arch/arm/include/asm/arch-dove/dove.h              |  98 +++++++
 arch/arm/include/asm/arch-dove/gpio.h              |  35 +++
 arch/arm/include/asm/arch-dove/mmc.h               |  27 ++
 arch/arm/include/asm/arch-dove/mpp.h               | 283 ++++++++++++++++++
 arch/arm/include/asm/arch-dove/usb.h               |  27 ++
 arch/arm/include/asm/arch-kirkwood/gpio.h          |  42 +--
 arch/arm/include/asm/arch-kirkwood/spi.h           |  27 --
 board/solidrun/cubox/Makefile                      |  45 +++
 board/solidrun/cubox/cubox.c                       | 142 +++++++++
 board/solidrun/cubox/kwbimage-spi-1gb.cfg          |  76 +++++
 board/solidrun/cubox/kwbimage-spi-2gb.cfg          |  76 +++++
 board/solidrun/cubox/kwbimage-uart.cfg             |  76 +++++
 boards.cfg                                         |   3 +
 drivers/block/mvsata_ide.c                         |   2 +
 drivers/gpio/kw_gpio.c                             |  43 +--
 drivers/mmc/Makefile                               |   1 +
 drivers/mmc/dove_sdhci.c                           | 101 +++++++
 drivers/net/mvgbe.c                                |   5 +-
 drivers/spi/kirkwood_spi.c                         |  47 ++-
 drivers/usb/host/ehci-marvell.c                    |  35 ++-
 include/configs/cubox.h                            | 185 ++++++++++++
 .../asm/arch-kirkwood/gpio.h => include/kw_gpio.h  |  43 ++-
 31 files changed, 2769 insertions(+), 125 deletions(-)
 create mode 100644 arch/arm/cpu/armv7/dove/Makefile
 create mode 100644 arch/arm/cpu/armv7/dove/cpu.c
 create mode 100644 arch/arm/cpu/armv7/dove/dram.c
 create mode 100644 arch/arm/cpu/armv7/dove/lowlevel_init.S
 create mode 100644 arch/arm/cpu/armv7/dove/mpp.c
 create mode 100644 arch/arm/cpu/armv7/dove/timer.c
 create mode 100644 arch/arm/cpu/armv7/dove/usb.c
 create mode 100644 arch/arm/include/asm/arch-dove/config.h
 create mode 100644 arch/arm/include/asm/arch-dove/cpu.h
 create mode 100644 arch/arm/include/asm/arch-dove/dove.h
 create mode 100644 arch/arm/include/asm/arch-dove/gpio.h
 create mode 100644 arch/arm/include/asm/arch-dove/mmc.h
 create mode 100644 arch/arm/include/asm/arch-dove/mpp.h
 create mode 100644 arch/arm/include/asm/arch-dove/usb.h
 create mode 100644 board/solidrun/cubox/Makefile
 create mode 100644 board/solidrun/cubox/cubox.c
 create mode 100644 board/solidrun/cubox/kwbimage-spi-1gb.cfg
 create mode 100644 board/solidrun/cubox/kwbimage-spi-2gb.cfg
 create mode 100644 board/solidrun/cubox/kwbimage-uart.cfg
 create mode 100644 drivers/mmc/dove_sdhci.c
 create mode 100644 include/configs/cubox.h
 copy arch/arm/include/asm/arch-kirkwood/gpio.h => include/kw_gpio.h (60%)
Sascha Silbe - March 3, 2014, 10:43 p.m.
Hello Jagan,

Jagan Teki <jagannadh.teki@gmail.com> writes:

> Any update on this.

The Wandboard Quad is working well for my purposes and much easier to
work with (schematics and very extensive data sheets available, mainline
support in both U-Boot and Linux). As a result, I'm focusing my limited
resources on the Wandboard rather than the Cubox.

Sascha
Otavio Salvador - March 3, 2014, 11:48 p.m.
On Mon, Mar 3, 2014 at 7:43 PM, Sascha Silbe <t-uboot@infra-silbe.de> wrote:
> Jagan Teki <jagannadh.teki@gmail.com> writes:
>
>> Any update on this.
>
> The Wandboard Quad is working well for my purposes and much easier to
> work with (schematics and very extensive data sheets available, mainline
> support in both U-Boot and Linux). As a result, I'm focusing my limited
> resources on the Wandboard rather than the Cubox.

Cubox-i has been merged in current mainline; so it may be good a recheck ;-)

Patch

diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index d08609e..62ad970 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -38,6 +38,7 @@  COBJS-$(CONFIG_BFIN_SPI6XX) += bfin_spi6xx.o
 COBJS-$(CONFIG_CF_SPI) += cf_spi.o
 COBJS-$(CONFIG_CF_QSPI) += cf_qspi.o
 COBJS-$(CONFIG_DAVINCI_SPI) += davinci_spi.o
+COBJS-$(CONFIG_DOVE_SPI) += dove_spi.o
 COBJS-$(CONFIG_EXYNOS_SPI) += exynos_spi.o
 COBJS-$(CONFIG_ICH_SPI) +=  ich.o
 COBJS-$(CONFIG_KIRKWOOD_SPI) += kirkwood_spi.o
diff --git a/drivers/spi/dove_spi.c b/drivers/spi/dove_spi.c
new file mode 100644
index 0000000..c61ba89
--- /dev/null
+++ b/drivers/spi/dove_spi.c
@@ -0,0 +1,212 @@ 
+/*
+ * Marvell Dove SoCs common spi driver
+ *
+ * Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
+ * based on kirkwood_spi.c written by
+ *  Prafulla Wadaskar <prafulla@marvell.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301 USA
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <spi.h>
+#include <asm/io.h>
+#include <asm/arch/config.h>
+
+/* SPI Registers on dove SOC */
+struct dovespi_registers {
+	u32 ctrl;	/* 0x00 */
+	u32 cfg;	/* 0x04 */
+	u32 dout;	/* 0x08 */
+	u32 din;	/* 0x0c */
+	u32 irq_cause;	/* 0x10 */
+	u32 irq_mask;	/* 0x14 */
+};
+
+#define DOVESPI_CLKPRESCL_MASK	0x1f
+#define DOVESPI_CLKPRESCL_MIN	0x12
+#define DOVESPI_CSN_ACT	1 /* Activates serial memory interface */
+#define DOVESPI_SMEMRDY	(1 << 1) /* SerMem Data xfer ready */
+#define DOVESPI_IRQUNMASK	1 /* unmask SPI interrupt */
+#define DOVESPI_IRQMASK	0 /* mask SPI interrupt */
+#define DOVESPI_SMEMRDIRQ	1 /* SerMem data xfer ready irq */
+#define DOVESPI_XFERLEN_1BYTE	0
+#define DOVESPI_XFERLEN_2BYTE	(1 << 5)
+#define DOVESPI_XFERLEN_MASK	(1 << 5)
+#define DOVESPI_ADRLEN_1BYTE	0
+#define DOVESPI_ADRLEN_2BYTE	(1 << 8)
+#define DOVESPI_ADRLEN_3BYTE	(2 << 8)
+#define DOVESPI_ADRLEN_4BYTE	(3 << 8)
+#define DOVESPI_ADRLEN_MASK	(3 << 8)
+#define DOVESPI_TIMEOUT	10000
+
+static struct dovespi_registers *spireg =
+	(struct dovespi_registers *)DOVE_SPI_BASE;
+
+struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
+				unsigned int max_hz, unsigned int mode)
+{
+	struct spi_slave *slave;
+	u32 data;
+
+	if (!spi_cs_is_valid(bus, cs))
+		return NULL;
+
+	slave = malloc(sizeof(struct spi_slave));
+	if (!slave)
+		return NULL;
+
+	slave->bus = bus;
+	slave->cs = cs;
+
+	writel(~DOVESPI_CSN_ACT | DOVESPI_SMEMRDY, &spireg->ctrl);
+
+	/* calculate spi clock prescaller using max_hz */
+	data = ((CONFIG_SYS_TCLK / 2) / max_hz) + 0x10;
+	data = data < DOVESPI_CLKPRESCL_MIN ? DOVESPI_CLKPRESCL_MIN : data;
+	data = data > DOVESPI_CLKPRESCL_MASK ? DOVESPI_CLKPRESCL_MASK : data;
+
+	/* program spi clock prescaller using max_hz */
+	writel(DOVESPI_ADRLEN_3BYTE | data, &spireg->cfg);
+	debug("data = 0x%08x\n", data);
+
+	writel(DOVESPI_SMEMRDIRQ, &spireg->irq_cause);
+	writel(DOVESPI_IRQMASK, &spireg->irq_mask);
+
+	return slave;
+}
+
+void spi_free_slave(struct spi_slave *slave)
+{
+	free(slave);
+}
+
+__attribute__((weak)) int board_spi_claim_bus(struct spi_slave *slave)
+{
+	return 0;
+}
+
+int spi_claim_bus(struct spi_slave *slave)
+{
+	return board_spi_claim_bus(slave);
+}
+
+__attribute__((weak)) void board_spi_release_bus(struct spi_slave *slave)
+{
+}
+
+void spi_release_bus(struct spi_slave *slave)
+{
+	board_spi_release_bus(slave);
+}
+
+#ifndef CONFIG_SPI_CS_IS_VALID
+/*
+ * you can define this function board specific
+ * define above CONFIG in board specific config file and
+ * provide the function in board specific src file
+ */
+int spi_cs_is_valid(unsigned int bus, unsigned int cs)
+{
+	return (bus == 0 && (cs == 0 || cs == 1));
+}
+#endif
+
+void spi_init(void)
+{
+}
+
+void spi_cs_activate(struct spi_slave *slave)
+{
+	writel(readl(&spireg->ctrl) | DOVESPI_IRQUNMASK, &spireg->ctrl);
+}
+
+void spi_cs_deactivate(struct spi_slave *slave)
+{
+	writel(readl(&spireg->ctrl) & DOVESPI_IRQMASK, &spireg->ctrl);
+}
+
+int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
+	     void *din, unsigned long flags)
+{
+	unsigned int tmpdout, tmpdin;
+	int tm, isread = 0;
+
+	debug("spi_xfer: slave %u:%u dout %p din %p bitlen %u\n",
+	      slave->bus, slave->cs, dout, din, bitlen);
+
+	if (flags & SPI_XFER_BEGIN)
+		spi_cs_activate(slave);
+
+	/*
+	 * handle data in 8-bit chunks
+	 * TBD: 2byte xfer mode to be enabled
+	 */
+	writel(((readl(&spireg->cfg) & ~DOVESPI_XFERLEN_MASK) |
+		DOVESPI_XFERLEN_1BYTE), &spireg->cfg);
+
+	while (bitlen > 4) {
+		debug("loopstart bitlen %d\n", bitlen);
+		tmpdout = 0;
+
+		/* Shift data so it's msb-justified */
+		if (dout)
+			tmpdout = *(u32 *)dout & 0x0ff;
+
+		writel(~DOVESPI_SMEMRDIRQ, &spireg->irq_cause);
+		writel(tmpdout, &spireg->dout);	/* Write the data out */
+		debug("*** spi_xfer: ... %08x written, bitlen %d\n",
+		      tmpdout, bitlen);
+
+		/*
+		 * Wait for SPI transmit to get out
+		 * or time out (1 second = 1000 ms)
+		 * The NE event must be read and cleared first
+		 */
+		for (tm = 0, isread = 0; tm < DOVESPI_TIMEOUT; ++tm) {
+			if (readl(&spireg->irq_cause) & DOVESPI_SMEMRDIRQ) {
+				isread = 1;
+				tmpdin = readl(&spireg->din);
+				debug
+					("spi_xfer: din %p..%08x read\n",
+					din, tmpdin);
+
+				if (din) {
+					*((u8 *)din) = (u8)tmpdin;
+					din += 1;
+				}
+				if (dout)
+					dout += 1;
+				bitlen -= 8;
+			}
+			if (isread)
+				break;
+		}
+		if (tm >= DOVESPI_TIMEOUT)
+			printf("*** spi_xfer: Time out during SPI transfer\n");
+
+		debug("loopend bitlen %d\n", bitlen);
+	}
+
+	if (flags & SPI_XFER_END)
+		spi_cs_deactivate(slave);
+
+	return 0;
+}