[U-Boot] [PATCH v0 0/5] Some improvements for mvebu/a38x

[U-Boot] [PATCH v0 0/5] Some improvements for mvebu/a38x

[dirk.eibach at gdsys.cc]

From: Dirk Eibach <dirk.eibach@gdsys.cc>




Dirk Eibach (5):
  pci: mvebu: Fix Armada 38x support
  arm: mvebu: Add gpio support
  arm: mvebu: Fix SAR1_CPU_CORE_MASK
  arm: mvebu: Fix ddr3_init() cpu config
  spi: Add support for Armada 38x second controller

 arch/arm/mach-mvebu/include/mach/gpio.h            | 41 ++++++++++++++++-
 arch/arm/mach-mvebu/include/mach/soc.h             |  1 +
 .../ddr/marvell/a38x/ddr3_hws_hw_training_def.h    |  7 +--
 drivers/ddr/marvell/a38x/ddr3_init.c               |  2 -
 drivers/pci/pci_mvebu.c                            | 22 ++++-----
 drivers/spi/kirkwood_spi.c                         | 52 +++++++++++++++++++---
 6 files changed, 98 insertions(+), 27 deletions(-)

-- 
2.1.3

[U-Boot] [PATCH v0 1/5] pci: mvebu: Fix Armada 38x support

[dirk.eibach at gdsys.cc]

From: Dirk Eibach <dirk.eibach@gdsys.cc>

Armada 38x has 4 pci ports, not 3.
The optimization in pci_init_board() seems to assume,
that every port has 3 lanes. This is obviously wrong
and breaks support for Armada 38x.

Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
---

 arch/arm/mach-mvebu/include/mach/soc.h |  1 +
 drivers/pci/pci_mvebu.c                | 22 +++++++++++-----------
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-mvebu/include/mach/soc.h b/arch/arm/mach-mvebu/include/mach/soc.h
index 02c21bc..a1014b3 100644
--- a/arch/arm/mach-mvebu/include/mach/soc.h
+++ b/arch/arm/mach-mvebu/include/mach/soc.h
@@ -67,6 +67,7 @@
 #define MVEBU_USB20_BASE	(MVEBU_REGISTER(0x58000))
 #define MVEBU_EGIGA0_BASE	(MVEBU_REGISTER(0x70000))
 #define MVEBU_EGIGA1_BASE	(MVEBU_REGISTER(0x74000))
+#define MVEBU_REG_PCIE0_BASE	(MVEBU_REGISTER(0x80000))
 #define MVEBU_AXP_SATA_BASE	(MVEBU_REGISTER(0xa0000))
 #define MVEBU_SATA0_BASE	(MVEBU_REGISTER(0xa8000))
 #define MVEBU_NAND_BASE		(MVEBU_REGISTER(0xd0000))
diff --git a/drivers/pci/pci_mvebu.c b/drivers/pci/pci_mvebu.c
index fd2744d..50e6419 100644
--- a/drivers/pci/pci_mvebu.c
+++ b/drivers/pci/pci_mvebu.c
@@ -90,26 +90,27 @@ static void __iomem *mvebu_pcie_membase = (void __iomem *)MBUS_PCI_MEM_BASE;
 
 #if defined(CONFIG_ARMADA_38X)
 #define PCIE_BASE(if)					\
-	((if) == 0 ?					\
-	 MVEBU_REG_PCIE_BASE + 0x40000 :		\
-	 MVEBU_REG_PCIE_BASE + 0x4000 * (if))
+	((if) == 0 ?							\
+	MVEBU_REG_PCIE0_BASE : \
+	(MVEBU_REG_PCIE_BASE + 0x4000 * (if - 1)))
 
 /*
  * On A38x MV6820 these PEX ports are supported:
  *  0 - Port 0.0
- *  1 - Port 0.1
- *  2 - Port 0.2
+ *  1 - Port 1.0
+ *  2 - Port 2.0
+ *  3 - Port 3.0
  */
-#define MAX_PEX 3
+#define MAX_PEX 4
 static struct mvebu_pcie pcie_bus[MAX_PEX];
 
 static void mvebu_get_port_lane(struct mvebu_pcie *pcie, int pex_idx,
 				int *mem_target, int *mem_attr)
 {
-	u8 port[] = { 0, 1, 2 };
-	u8 lane[] = { 0, 0, 0 };
-	u8 target[] = { 8, 4, 4 };
-	u8 attr[] = { 0xe8, 0xe8, 0xd8 };
+	u8 port[] = { 0, 1, 2, 3 };
+	u8 lane[] = { 0, 0, 0, 0 };
+	u8 target[] = { 8, 4, 4, 4 };
+	u8 attr[] = { 0xe8, 0xe8, 0xd8, 0xb8 };
 
 	pcie->port = port[pex_idx];
 	pcie->lane = lane[pex_idx];
@@ -344,7 +345,6 @@ void pci_init_board(void)
 
 		/* Don't read at all from pci registers if port power is down */
 		if (pcie->lane == 0 && SELECT(soc_ctrl, pcie->port) == 0) {
-			i += 3;
 			debug("%s: skipping port %d\n", __func__, pcie->port);
 			continue;
 		}
-- 
2.1.3

[U-Boot] [PATCH v0 1/5] pci: mvebu: Fix Armada 38x support

[Stefan Roese]

Hi Dirk,

On 28.10.2015 16:44, dirk.eibach at gdsys.cc wrote:
> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>
> Armada 38x has 4 pci ports, not 3.
> The optimization in pci_init_board() seems to assume,
> that every port has 3 lanes. This is obviously wrong
> and breaks support for Armada 38x.
>
> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>

Looks good, so:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v0 1/5] pci: mvebu: Fix Armada 38x support

[Anton Schubert]

Hi Dirk,

2015-10-28 16:44 GMT+01:00 <dirk.eibach@gdsys.cc>:

> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>
> @@ -344,7 +345,6 @@ void pci_init_board(void)
>
>                 /* Don't read at all from pci registers if port power is
> down */
>                 if (pcie->lane == 0 && SELECT(soc_ctrl, pcie->port) == 0) {
> -                       i += 3;
>                         debug("%s: skipping port %d\n", __func__,
> pcie->port);
>                         continue;
>                 }
>

Is there a specific reason why you removed this line or was it just by
mistake? Because I think doing so would break Armada XP in certain Serdes
Configurations, as it doesn't like it's PCI registers being read if the
port is off.

Kind Regards,
Anton

[U-Boot] [PATCH v0 1/5] pci: mvebu: Fix Armada 38x support

[Dirk Eibach]

Hi Anton,

2015-11-17 13:55 GMT+01:00 Anton Schubert <anton.schubert@gmx.de>:
> Hi Dirk,
>
> 2015-10-28 16:44 GMT+01:00 <dirk.eibach@gdsys.cc>:
>>
>> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>>
>> @@ -344,7 +345,6 @@ void pci_init_board(void)
>>
>>                 /* Don't read at all from pci registers if port power is
>> down */
>>                 if (pcie->lane == 0 && SELECT(soc_ctrl, pcie->port) == 0)
>> {
>> -                       i += 3;
>>                         debug("%s: skipping port %d\n", __func__,
>> pcie->port);
>>                         continue;
>>                 }
>
>
> Is there a specific reason why you removed this line or was it just by
> mistake? Because I think doing so would break Armada XP in certain Serdes
> Configurations, as it doesn't like it's PCI registers being read if the port
> is off.

I assume the idea is to go to the next port if the current port is
disabled. But adding 3 to the index does not seem to be the right
thing to do, since Armada XP has ports with 4 lanes, but also with
ports with one lane.
I assume that iterating over all lanes would not be a problem, but by
mistake the pcie->lane == 0  was left in the condition. So this should
perform better:

/* Don't read at all from pci registers if port power is down */
if (SELECT(soc_ctrl, pcie->port) == 0) {
    if (pcie->lane == 0)
        debug("%s: skipping port %d\n", __func__, pcie->port);
    continue;
}

What do you think?

Cheers
Dirk

[U-Boot] [PATCH v0 1/5] pci: mvebu: Fix Armada 38x support

[Anton Schubert]

Am 18.11.2015 um 13:48 schrieb Dirk Eibach:
> I assume the idea is to go to the next port if the current port is
> disabled. But adding 3 to the index does not seem to be the right
> thing to do, since Armada XP has ports with 4 lanes, but also with
> ports with one lane.
> I assume that iterating over all lanes would not be a problem, but by
> mistake the pcie->lane == 0  was left in the condition. 
Yeah you are right. The additional condition was superfluous in the
original version.

> So this should perform better:
>
> /* Don't read at all from pci registers if port power is down */
> if (SELECT(soc_ctrl, pcie->port) == 0) {
>     if (pcie->lane == 0)
>         debug("%s: skipping port %d\n", __func__, pcie->port);
>     continue;
> }
I agree.

Regards,
Anton

[U-Boot] [PATCH v0 1/5] pci: mvebu: Fix Armada 38x support

[Dirk Eibach]

2015-11-18 14:23 GMT+01:00 Anton Schubert <anton.schubert@gmx.de>:
> Am 18.11.2015 um 13:48 schrieb Dirk Eibach:
>> I assume the idea is to go to the next port if the current port is
>> disabled. But adding 3 to the index does not seem to be the right
>> thing to do, since Armada XP has ports with 4 lanes, but also with
>> ports with one lane.
>> I assume that iterating over all lanes would not be a problem, but by
>> mistake the pcie->lane == 0  was left in the condition.
> Yeah you are right. The additional condition was superfluous in the
> original version.
>
>> So this should perform better:
>>
>> /* Don't read at all from pci registers if port power is down */
>> if (SELECT(soc_ctrl, pcie->port) == 0) {
>>     if (pcie->lane == 0)
>>         debug("%s: skipping port %d\n", __func__, pcie->port);
>>     continue;
>> }
> I agree.

Fine. I will add this to the v1 series.

Cheers
Dirk

[U-Boot] [PATCH v0 2/5] arm: mvebu: Add gpio support

[dirk.eibach at gdsys.cc]

From: Dirk Eibach <dirk.eibach@gdsys.cc>

mvebu gpio is based on kirkwood.

Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
---

 arch/arm/mach-mvebu/include/mach/gpio.h | 41 +++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-mvebu/include/mach/gpio.h b/arch/arm/mach-mvebu/include/mach/gpio.h
index 09e3c50..cd869ee 100644
--- a/arch/arm/mach-mvebu/include/mach/gpio.h
+++ b/arch/arm/mach-mvebu/include/mach/gpio.h
@@ -1,10 +1,47 @@
 /*
- * SPDX-License-Identifier:      GPL-2.0+
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/*
+ * Based on (mostly copied from) plat-orion based Linux 2.6 kernel driver.
+ * Removed kernel level irq handling. Took some macros from kernel to
+ * allow build.
+ *
+ * Dieter Kiermaier dk-arm-linux at gmx.de
  */
 
 #ifndef __MACH_MVEBU_GPIO_H
 #define __MACH_MVEBU_GPIO_H
 
-/* Empty file - sdhci requires this. */
+/* got from kernel include/linux/bitops.h */
+#define BITS_PER_BYTE 8
+#define BITS_TO_LONGS(nr)	DIV_ROUND_UP(nr, BITS_PER_BYTE * sizeof(long))
+
+#define GPIO_MAX		59
+#define GPIO_OFF(pin)		(((pin) >> 5) ? 0x0040 : 0x0000)
+#define GPIO_OUT(pin)		(MVEBU_GPIO0_BASE + GPIO_OFF(pin) + 0x00)
+#define GPIO_IO_CONF(pin)	(MVEBU_GPIO0_BASE + GPIO_OFF(pin) + 0x04)
+#define GPIO_BLINK_EN(pin)	(MVEBU_GPIO0_BASE + GPIO_OFF(pin) + 0x08)
+#define GPIO_IN_POL(pin)	(MVEBU_GPIO0_BASE + GPIO_OFF(pin) + 0x0c)
+#define GPIO_DATA_IN(pin)	(MVEBU_GPIO0_BASE + GPIO_OFF(pin) + 0x10)
+#define GPIO_EDGE_CAUSE(pin)	(MVEBU_GPIO0_BASE + GPIO_OFF(pin) + 0x14)
+#define GPIO_EDGE_MASK(pin)	(MVEBU_GPIO0_BASE + GPIO_OFF(pin) + 0x18)
+#define GPIO_LEVEL_MASK(pin)	(MVEBU_GPIO0_BASE + GPIO_OFF(pin) + 0x1c)
+
+/*
+ * Kirkwood-specific GPIO API
+ */
+
+void kw_gpio_set_valid(unsigned pin, int mode);
+int kw_gpio_is_valid(unsigned pin, int mode);
+int kw_gpio_direction_input(unsigned pin);
+int kw_gpio_direction_output(unsigned pin, int value);
+int kw_gpio_get_value(unsigned pin);
+void kw_gpio_set_value(unsigned pin, int value);
+void kw_gpio_set_blink(unsigned pin, int blink);
+void kw_gpio_set_unused(unsigned pin);
+
+#define GPIO_INPUT_OK		(1 << 0)
+#define GPIO_OUTPUT_OK		(1 << 1)
 
 #endif
-- 
2.1.3

[U-Boot] [PATCH v0 2/5] arm: mvebu: Add gpio support

[Stefan Roese]

Hi Dirk,

On 28.10.2015 16:44, dirk.eibach at gdsys.cc wrote:
> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>
> mvebu gpio is based on kirkwood.
>
> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>

Again, I would really like to see a DM based driver for this.
It shouldn't be too much work. GPIO is perhaps the simplest
driver here. Perhaps you can spare some cycles for such a
task...

Thanks,
Stefan

[U-Boot] [PATCH v0 3/5] arm: mvebu: Fix SAR1_CPU_CORE_MASK

[dirk.eibach at gdsys.cc]

From: Dirk Eibach <dirk.eibach@gdsys.cc>

SAR1_CPU_CORE_MASK was wrong, probably copy/paste
from another architecture.

Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
---

 drivers/ddr/marvell/a38x/ddr3_hws_hw_training_def.h | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/ddr/marvell/a38x/ddr3_hws_hw_training_def.h b/drivers/ddr/marvell/a38x/ddr3_hws_hw_training_def.h
index 7500a72..06d0ab1 100644
--- a/drivers/ddr/marvell/a38x/ddr3_hws_hw_training_def.h
+++ b/drivers/ddr/marvell/a38x/ddr3_hws_hw_training_def.h
@@ -23,8 +23,8 @@
 
 #define CPU_CONFIGURATION_REG(id)	(0x21800 + (id * 0x100))
 #define CPU_MRVL_ID_OFFSET		0x10
-#define SAR1_CPU_CORE_MASK		0x00000018
-#define SAR1_CPU_CORE_OFFSET		3
+#define SAR1_CPU_CORE_MASK		0x38000000
+#define SAR1_CPU_CORE_OFFSET		27
 
 #define NEW_FABRIC_TWSI_ADDR		0x4e
 #ifdef DB_784MP_GP
@@ -461,7 +461,4 @@
 #define CLK_CPU_2200			13
 #define CLK_CPU_2400			14
 
-#define SAR1_CPU_CORE_MASK		0x00000018
-#define SAR1_CPU_CORE_OFFSET		3
-
 #endif /* _DDR3_HWS_HW_TRAINING_DEF_H */
-- 
2.1.3

[U-Boot] [PATCH v0 3/5] arm: mvebu: Fix SAR1_CPU_CORE_MASK

[Stefan Roese]

Hi Dirk,

On 28.10.2015 16:44, dirk.eibach at gdsys.cc wrote:
> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>
> SAR1_CPU_CORE_MASK was wrong, probably copy/paste
> from another architecture.
>
> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
> ---
>
>   drivers/ddr/marvell/a38x/ddr3_hws_hw_training_def.h | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/ddr/marvell/a38x/ddr3_hws_hw_training_def.h b/drivers/ddr/marvell/a38x/ddr3_hws_hw_training_def.h
> index 7500a72..06d0ab1 100644
> --- a/drivers/ddr/marvell/a38x/ddr3_hws_hw_training_def.h
> +++ b/drivers/ddr/marvell/a38x/ddr3_hws_hw_training_def.h
> @@ -23,8 +23,8 @@
>
>   #define CPU_CONFIGURATION_REG(id)	(0x21800 + (id * 0x100))
>   #define CPU_MRVL_ID_OFFSET		0x10
> -#define SAR1_CPU_CORE_MASK		0x00000018
> -#define SAR1_CPU_CORE_OFFSET		3
> +#define SAR1_CPU_CORE_MASK		0x38000000
> +#define SAR1_CPU_CORE_OFFSET		27
>
>   #define NEW_FABRIC_TWSI_ADDR		0x4e
>   #ifdef DB_784MP_GP
> @@ -461,7 +461,4 @@
>   #define CLK_CPU_2200			13
>   #define CLK_CPU_2400			14
>
> -#define SAR1_CPU_CORE_MASK		0x00000018
> -#define SAR1_CPU_CORE_OFFSET		3
> -
>   #endif /* _DDR3_HWS_HW_TRAINING_DEF_H */

Thanks for spotting. Seems to be correct from the datasheet. How did
you find this problem? What exactly did happen on your board?

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v0 3/5] arm: mvebu: Fix SAR1_CPU_CORE_MASK

[Dirk Eibach]

Hi Stefan,

2015-10-28 17:34 GMT+01:00 Stefan Roese <sr@denx.de>:
> Thanks for spotting. Seems to be correct from the datasheet. How did
> you find this problem? What exactly did happen on your board?

I did a code review when we had some DDR3 problems on initial board
bringup. This issue had no visible consequences.

Cheers
Dirk

[U-Boot] [PATCH v0 3/5] arm: mvebu: Fix SAR1_CPU_CORE_MASK

[Luka Perkov]

On Wed, Oct 28, 2015 at 04:44:14PM +0100, dirk.eibach at gdsys.cc wrote:
> From: Dirk Eibach <dirk.eibach@gdsys.cc>
> 
> SAR1_CPU_CORE_MASK was wrong, probably copy/paste
> from another architecture.
> 
> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
> ---
> 
>  drivers/ddr/marvell/a38x/ddr3_hws_hw_training_def.h | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)

Applied in marvell/master. Thank you!

Luka

[U-Boot] [PATCH v0 4/5] arm: mvebu: Fix ddr3_init() cpu config

[dirk.eibach at gdsys.cc]

From: Dirk Eibach <dirk.eibach@gdsys.cc>

Armada 38x has a maximum of two cores. Probably copy/paste
bug from Armada XP.

Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
---

 drivers/ddr/marvell/a38x/ddr3_init.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/ddr/marvell/a38x/ddr3_init.c b/drivers/ddr/marvell/a38x/ddr3_init.c
index d6ed8e0..cbfc58c 100644
--- a/drivers/ddr/marvell/a38x/ddr3_init.c
+++ b/drivers/ddr/marvell/a38x/ddr3_init.c
@@ -306,8 +306,6 @@ int ddr3_init(void)
 		SAR1_CPU_CORE_OFFSET;
 	switch (soc_num) {
 	case 0x3:
-		reg_bit_set(CPU_CONFIGURATION_REG(3), CPU_MRVL_ID_OFFSET);
-		reg_bit_set(CPU_CONFIGURATION_REG(2), CPU_MRVL_ID_OFFSET);
 	case 0x1:
 		reg_bit_set(CPU_CONFIGURATION_REG(1), CPU_MRVL_ID_OFFSET);
 	case 0x0:
-- 
2.1.3

[U-Boot] [PATCH v0 4/5] arm: mvebu: Fix ddr3_init() cpu config

[Stefan Roese]

Hi Dirk,

On 28.10.2015 16:44, dirk.eibach at gdsys.cc wrote:
> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>
> Armada 38x has a maximum of two cores. Probably copy/paste
> bug from Armada XP.
>
> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
> ---
>
>   drivers/ddr/marvell/a38x/ddr3_init.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/drivers/ddr/marvell/a38x/ddr3_init.c b/drivers/ddr/marvell/a38x/ddr3_init.c
> index d6ed8e0..cbfc58c 100644
> --- a/drivers/ddr/marvell/a38x/ddr3_init.c
> +++ b/drivers/ddr/marvell/a38x/ddr3_init.c
> @@ -306,8 +306,6 @@ int ddr3_init(void)
>   		SAR1_CPU_CORE_OFFSET;
>   	switch (soc_num) {
>   	case 0x3:
> -		reg_bit_set(CPU_CONFIGURATION_REG(3), CPU_MRVL_ID_OFFSET);
> -		reg_bit_set(CPU_CONFIGURATION_REG(2), CPU_MRVL_ID_OFFSET);
>   	case 0x1:
>   		reg_bit_set(CPU_CONFIGURATION_REG(1), CPU_MRVL_ID_OFFSET);
>   	case 0x0:

Shouldn't you remove the "case 0x3:" line as well?

Thanks,
Stefan

[U-Boot] [PATCH v0 4/5] arm: mvebu: Fix ddr3_init() cpu config

[Dirk Eibach]

Hi Stefan,

2015-10-28 17:35 GMT+01:00 Stefan Roese <sr@denx.de>:
> Hi Dirk,
>
> On 28.10.2015 16:44, dirk.eibach at gdsys.cc wrote:
>>
>> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>>
>> Armada 38x has a maximum of two cores. Probably copy/paste
>> bug from Armada XP.
>>
>> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
>> ---
>>
>>   drivers/ddr/marvell/a38x/ddr3_init.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/ddr/marvell/a38x/ddr3_init.c
>> b/drivers/ddr/marvell/a38x/ddr3_init.c
>> index d6ed8e0..cbfc58c 100644
>> --- a/drivers/ddr/marvell/a38x/ddr3_init.c
>> +++ b/drivers/ddr/marvell/a38x/ddr3_init.c
>> @@ -306,8 +306,6 @@ int ddr3_init(void)
>>                 SAR1_CPU_CORE_OFFSET;
>>         switch (soc_num) {
>>         case 0x3:
>> -               reg_bit_set(CPU_CONFIGURATION_REG(3), CPU_MRVL_ID_OFFSET);
>> -               reg_bit_set(CPU_CONFIGURATION_REG(2), CPU_MRVL_ID_OFFSET);
>>         case 0x1:
>>                 reg_bit_set(CPU_CONFIGURATION_REG(1), CPU_MRVL_ID_OFFSET);
>>         case 0x0:
>
>
> Shouldn't you remove the "case 0x3:" line as well?

Nope, according to Reset Configuration Pins table in the hardware spec
0 means Armada 380 (singlecore), 1 means Armada 385 (dualcore) and 3
means Armada 388 (dualcore). So handling soc_num 1 and 3 the same way
is perfectly allright.

Cheers
Dirk

[U-Boot] [PATCH v0 4/5] arm: mvebu: Fix ddr3_init() cpu config

[Stefan Roese]

Hi Dirk,

On 29.10.2015 10:51, Dirk Eibach wrote:
> 2015-10-28 17:35 GMT+01:00 Stefan Roese <sr@denx.de>:
>> Hi Dirk,
>>
>> On 28.10.2015 16:44, dirk.eibach at gdsys.cc wrote:
>>>
>>> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>>>
>>> Armada 38x has a maximum of two cores. Probably copy/paste
>>> bug from Armada XP.
>>>
>>> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
>>> ---
>>>
>>>    drivers/ddr/marvell/a38x/ddr3_init.c | 2 --
>>>    1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/ddr/marvell/a38x/ddr3_init.c
>>> b/drivers/ddr/marvell/a38x/ddr3_init.c
>>> index d6ed8e0..cbfc58c 100644
>>> --- a/drivers/ddr/marvell/a38x/ddr3_init.c
>>> +++ b/drivers/ddr/marvell/a38x/ddr3_init.c
>>> @@ -306,8 +306,6 @@ int ddr3_init(void)
>>>                  SAR1_CPU_CORE_OFFSET;
>>>          switch (soc_num) {
>>>          case 0x3:
>>> -               reg_bit_set(CPU_CONFIGURATION_REG(3), CPU_MRVL_ID_OFFSET);
>>> -               reg_bit_set(CPU_CONFIGURATION_REG(2), CPU_MRVL_ID_OFFSET);
>>>          case 0x1:
>>>                  reg_bit_set(CPU_CONFIGURATION_REG(1), CPU_MRVL_ID_OFFSET);
>>>          case 0x0:
>>
>>
>> Shouldn't you remove the "case 0x3:" line as well?
>
> Nope, according to Reset Configuration Pins table in the hardware spec
> 0 means Armada 380 (singlecore), 1 means Armada 385 (dualcore) and 3
> means Armada 388 (dualcore). So handling soc_num 1 and 3 the same way
> is perfectly allright.

Thanks for the explanation:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH v0 4/5] arm: mvebu: Fix ddr3_init() cpu config

[Stefan Roese]

On 28.10.2015 16:44, dirk.eibach at gdsys.cc wrote:
> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>
> Armada 38x has a maximum of two cores. Probably copy/paste
> bug from Armada XP.
>
> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>

Applied to u-boot-marvell/master.

Thanks,
Stefan

[U-Boot] [PATCH v0 5/5] spi: Add support for Armada 38x second controller

[dirk.eibach at gdsys.cc]

From: Dirk Eibach <dirk.eibach@gdsys.cc>

Armada 38x has two spi controllers.

Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
---

 drivers/spi/kirkwood_spi.c | 52 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/kirkwood_spi.c b/drivers/spi/kirkwood_spi.c
index e7b0982..200c391 100644
--- a/drivers/spi/kirkwood_spi.c
+++ b/drivers/spi/kirkwood_spi.c
@@ -18,17 +18,25 @@
 #endif
 #include <asm/arch-mvebu/spi.h>
 
-static struct kwspi_registers *spireg =
-	(struct kwspi_registers *)MVEBU_SPI_BASE;
-
 #ifdef CONFIG_KIRKWOOD
 static u32 cs_spi_mpp_back[2];
 #endif
 
+struct kwspi_slave {
+	struct spi_slave slave;
+	struct kwspi_registers *spireg;
+};
+
+static inline struct kwspi_slave *to_kwspi(struct spi_slave *slave)
+{
+	return container_of(slave, struct kwspi_slave, slave);
+}
+
 struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
 				unsigned int max_hz, unsigned int mode)
 {
-	struct spi_slave *slave;
+	struct kwspi_slave *kwspi_slave;
+	struct kwspi_registers *spireg;
 	u32 data;
 #ifdef CONFIG_KIRKWOOD
 	static const u32 kwspi_mpp_config[2][2] = {
@@ -40,10 +48,27 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
 	if (!spi_cs_is_valid(bus, cs))
 		return NULL;
 
-	slave = spi_alloc_slave_base(bus, cs);
-	if (!slave)
+	kwspi_slave = spi_alloc_slave(struct kwspi_slave, bus, cs);
+	if (!kwspi_slave)
 		return NULL;
 
+	switch (bus) {
+	case 0:
+		kwspi_slave->spireg = (struct kwspi_registers *)MVEBU_SPI_BASE;
+		break;
+#ifdef CONFIG_ARMADA_38X
+	/* Armada 38x has two SPI controllers */
+	case 1:
+		kwspi_slave->spireg =
+			(struct kwspi_registers *)(MVEBU_SPI_BASE + 0x80);
+		break;
+#endif
+	default:
+		return NULL;
+	}
+
+	spireg = kwspi_slave->spireg;
+
 	writel(KWSPI_SMEMRDY, &spireg->ctrl);
 
 	/* calculate spi clock prescaller using max_hz */
@@ -63,7 +88,7 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
 	kirkwood_mpp_conf(kwspi_mpp_config[cs ? 1 : 0], cs_spi_mpp_back);
 #endif
 
-	return slave;
+	return &kwspi_slave->slave;
 }
 
 void spi_free_slave(struct spi_slave *slave)
@@ -137,7 +162,12 @@ void spi_release_bus(struct spi_slave *slave)
  */
 int spi_cs_is_valid(unsigned int bus, unsigned int cs)
 {
+#ifdef CONFIG_ARMADA_38X
+	/* Armada 38x has two SPI controllers */
+	return (bus < 2) && (cs < 3);
+#else
 	return bus == 0 && (cs == 0 || cs == 1);
+#endif
 }
 #endif
 
@@ -147,11 +177,17 @@ void spi_init(void)
 
 void spi_cs_activate(struct spi_slave *slave)
 {
+	struct kwspi_slave *kwspi_slave = to_kwspi(slave);
+	struct kwspi_registers *spireg = kwspi_slave->spireg;
+
 	setbits_le32(&spireg->ctrl, KWSPI_CSN_ACT);
 }
 
 void spi_cs_deactivate(struct spi_slave *slave)
 {
+	struct kwspi_slave *kwspi_slave = to_kwspi(slave);
+	struct kwspi_registers *spireg = kwspi_slave->spireg;
+
 	clrbits_le32(&spireg->ctrl, KWSPI_CSN_ACT);
 }
 
@@ -160,6 +196,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
 {
 	unsigned int tmpdout, tmpdin;
 	int tm, isread = 0;
+	struct kwspi_slave *kwspi_slave = to_kwspi(slave);
+	struct kwspi_registers *spireg = kwspi_slave->spireg;
 
 	debug("spi_xfer: slave %u:%u dout %p din %p bitlen %u\n",
 	      slave->bus, slave->cs, dout, din, bitlen);
-- 
2.1.3

[U-Boot] [PATCH v0 5/5] spi: Add support for Armada 38x second controller

[Jagan Teki]

On 28 October 2015 at 21:14,  <dirk.eibach@gdsys.cc> wrote:
> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>
> Armada 38x has two spi controllers.
>
> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
> ---
>
>  drivers/spi/kirkwood_spi.c | 52 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/spi/kirkwood_spi.c b/drivers/spi/kirkwood_spi.c
> index e7b0982..200c391 100644
> --- a/drivers/spi/kirkwood_spi.c
> +++ b/drivers/spi/kirkwood_spi.c
> @@ -18,17 +18,25 @@
>  #endif
>  #include <asm/arch-mvebu/spi.h>
>
> -static struct kwspi_registers *spireg =
> -       (struct kwspi_registers *)MVEBU_SPI_BASE;
> -
>  #ifdef CONFIG_KIRKWOOD
>  static u32 cs_spi_mpp_back[2];
>  #endif
>
> +struct kwspi_slave {
> +       struct spi_slave slave;
> +       struct kwspi_registers *spireg;
> +};
> +
> +static inline struct kwspi_slave *to_kwspi(struct spi_slave *slave)
> +{
> +       return container_of(slave, struct kwspi_slave, slave);
> +}
> +
>  struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
>                                 unsigned int max_hz, unsigned int mode)
>  {
> -       struct spi_slave *slave;
> +       struct kwspi_slave *kwspi_slave;
> +       struct kwspi_registers *spireg;
>         u32 data;
>  #ifdef CONFIG_KIRKWOOD
>         static const u32 kwspi_mpp_config[2][2] = {
> @@ -40,10 +48,27 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
>         if (!spi_cs_is_valid(bus, cs))
>                 return NULL;
>
> -       slave = spi_alloc_slave_base(bus, cs);
> -       if (!slave)
> +       kwspi_slave = spi_alloc_slave(struct kwspi_slave, bus, cs);
> +       if (!kwspi_slave)
>                 return NULL;
>
> +       switch (bus) {
> +       case 0:
> +               kwspi_slave->spireg = (struct kwspi_registers *)MVEBU_SPI_BASE;
> +               break;
> +#ifdef CONFIG_ARMADA_38X
> +       /* Armada 38x has two SPI controllers */

Can you please add this through driver-model, I understand it's bit
big task but I can help you at any moment.

> +       case 1:
> +               kwspi_slave->spireg =
> +                       (struct kwspi_registers *)(MVEBU_SPI_BASE + 0x80);
> +               break;
> +#endif
> +       default:
> +               return NULL;
> +       }
> +
> +       spireg = kwspi_slave->spireg;
> +
>         writel(KWSPI_SMEMRDY, &spireg->ctrl);
>
>         /* calculate spi clock prescaller using max_hz */
> @@ -63,7 +88,7 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
>         kirkwood_mpp_conf(kwspi_mpp_config[cs ? 1 : 0], cs_spi_mpp_back);
>  #endif
>
> -       return slave;
> +       return &kwspi_slave->slave;
>  }
>
>  void spi_free_slave(struct spi_slave *slave)
> @@ -137,7 +162,12 @@ void spi_release_bus(struct spi_slave *slave)
>   */
>  int spi_cs_is_valid(unsigned int bus, unsigned int cs)
>  {
> +#ifdef CONFIG_ARMADA_38X
> +       /* Armada 38x has two SPI controllers */
> +       return (bus < 2) && (cs < 3);
> +#else
>         return bus == 0 && (cs == 0 || cs == 1);
> +#endif
>  }
>  #endif
>
> @@ -147,11 +177,17 @@ void spi_init(void)
>
>  void spi_cs_activate(struct spi_slave *slave)
>  {
> +       struct kwspi_slave *kwspi_slave = to_kwspi(slave);
> +       struct kwspi_registers *spireg = kwspi_slave->spireg;
> +
>         setbits_le32(&spireg->ctrl, KWSPI_CSN_ACT);
>  }
>
>  void spi_cs_deactivate(struct spi_slave *slave)
>  {
> +       struct kwspi_slave *kwspi_slave = to_kwspi(slave);
> +       struct kwspi_registers *spireg = kwspi_slave->spireg;
> +
>         clrbits_le32(&spireg->ctrl, KWSPI_CSN_ACT);
>  }
>
> @@ -160,6 +196,8 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,
>  {
>         unsigned int tmpdout, tmpdin;
>         int tm, isread = 0;
> +       struct kwspi_slave *kwspi_slave = to_kwspi(slave);
> +       struct kwspi_registers *spireg = kwspi_slave->spireg;
>
>         debug("spi_xfer: slave %u:%u dout %p din %p bitlen %u\n",
>               slave->bus, slave->cs, dout, din, bitlen);
> --
> 2.1.3
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

-- 
Jagan | openedev.

[U-Boot] [PATCH v0 5/5] spi: Add support for Armada 38x second controller

[Stefan Roese]

Hi Dirk,

On 28.10.2015 17:29, Jagan Teki wrote:
> On 28 October 2015 at 21:14,  <dirk.eibach@gdsys.cc> wrote:
>> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>>
>> Armada 38x has two spi controllers.
>>
>> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
>> ---
>>
>>   drivers/spi/kirkwood_spi.c | 52 +++++++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 45 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/spi/kirkwood_spi.c b/drivers/spi/kirkwood_spi.c
>> index e7b0982..200c391 100644
>> --- a/drivers/spi/kirkwood_spi.c
>> +++ b/drivers/spi/kirkwood_spi.c
>> @@ -18,17 +18,25 @@
>>   #endif
>>   #include <asm/arch-mvebu/spi.h>
>>
>> -static struct kwspi_registers *spireg =
>> -       (struct kwspi_registers *)MVEBU_SPI_BASE;
>> -
>>   #ifdef CONFIG_KIRKWOOD
>>   static u32 cs_spi_mpp_back[2];
>>   #endif
>>
>> +struct kwspi_slave {
>> +       struct spi_slave slave;
>> +       struct kwspi_registers *spireg;
>> +};
>> +
>> +static inline struct kwspi_slave *to_kwspi(struct spi_slave *slave)
>> +{
>> +       return container_of(slave, struct kwspi_slave, slave);
>> +}
>> +
>>   struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
>>                                  unsigned int max_hz, unsigned int mode)
>>   {
>> -       struct spi_slave *slave;
>> +       struct kwspi_slave *kwspi_slave;
>> +       struct kwspi_registers *spireg;
>>          u32 data;
>>   #ifdef CONFIG_KIRKWOOD
>>          static const u32 kwspi_mpp_config[2][2] = {
>> @@ -40,10 +48,27 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
>>          if (!spi_cs_is_valid(bus, cs))
>>                  return NULL;
>>
>> -       slave = spi_alloc_slave_base(bus, cs);
>> -       if (!slave)
>> +       kwspi_slave = spi_alloc_slave(struct kwspi_slave, bus, cs);
>> +       if (!kwspi_slave)
>>                  return NULL;
>>
>> +       switch (bus) {
>> +       case 0:
>> +               kwspi_slave->spireg = (struct kwspi_registers *)MVEBU_SPI_BASE;
>> +               break;
>> +#ifdef CONFIG_ARMADA_38X
>> +       /* Armada 38x has two SPI controllers */
>
> Can you please add this through driver-model, I understand it's bit
> big task but I can help you at any moment.

Yes, please do. I know this is additional work. But we really need to
get there at some time. And please note that you can use the
runtime SoC detection for this:

	if (mvebu_soc_family() == MVEBU_SOC_A38X)

So no new #idefs are needed in such places.

Thanks,
Stefan

[U-Boot] [PATCH v0 5/5] spi: Add support for Armada 38x second controller

[Dirk Eibach]

Hi Stefan,

2015-10-28 17:39 GMT+01:00 Stefan Roese <sr@denx.de>:
> ...  And please note that you can use the
> runtime SoC detection for this:
>
>         if (mvebu_soc_family() == MVEBU_SOC_A38X)
>
> So no new #idefs are needed in such places.

Just give me a quick update please.  Why is runtime detection better?
Is it about code coverage? What about binary footprint?

Cheers
Dirk

[U-Boot] [PATCH v0 5/5] spi: Add support for Armada 38x second controller

[Stefan Roese]

Hi Dirk,

On 29.10.2015 10:54, Dirk Eibach wrote:
> 2015-10-28 17:39 GMT+01:00 Stefan Roese <sr@denx.de>:
>> ...  And please note that you can use the
>> runtime SoC detection for this:
>>
>>          if (mvebu_soc_family() == MVEBU_SOC_A38X)
>>
>> So no new #idefs are needed in such places.
>
> Just give me a quick update please.  Why is runtime detection better?
> Is it about code coverage? What about binary footprint?

We try hard not to add more #idef's to the U-Boot source code
if possible. This is definitely one of the reasons. Another
is, that this runtime detection will enable support for
multiple SoC's in one binary image. This is currently not the
case, but we should try to work this way if its not too
hard. And these places for the SoC detection are pretty easy
to achieve.

The image size will of course be affected. But this drawback is
outweighed by the pros noted above. At least from my point of
view.

Thanks,
Stefan

[U-Boot] [PATCH v0 5/5] spi: Add support for Armada 38x second controller

[Dirk Eibach]

Hi Stefan,

2015-10-29 11:02 GMT+01:00 Stefan Roese <sr@denx.de>:
> Hi Dirk,
>
> On 29.10.2015 10:54, Dirk Eibach wrote:
>>
>> 2015-10-28 17:39 GMT+01:00 Stefan Roese <sr@denx.de>:
>>>
>>> ...  And please note that you can use the
>>> runtime SoC detection for this:
>>>
>>>          if (mvebu_soc_family() == MVEBU_SOC_A38X)
>>>
>>> So no new #idefs are needed in such places.
>>
>>
>> Just give me a quick update please.  Why is runtime detection better?
>> Is it about code coverage? What about binary footprint?
>
>
> We try hard not to add more #idef's to the U-Boot source code
> if possible. This is definitely one of the reasons. Another
> is, that this runtime detection will enable support for
> multiple SoC's in one binary image. This is currently not the
> case, but we should try to work this way if its not too
> hard. And these places for the SoC detection are pretty easy
> to achieve.
>
> The image size will of course be affected. But this drawback is
> outweighed by the pros noted above. At least from my point of
> view.

Ok, that's the information I needed. I will adapt this to runtime detection.

Cheers
Dirk