Some improvements for mvebu/a38x

From: Dirk Eibach <[email protected]>

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 <[email protected]>

From: Dirk Eibach <[email protected]>

(quoted diff) +0 -0

@@ -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;
                }

Hi Dirk,

2015-10-28 16:44 GMT+01:00 <[email protected]>:

Quoted 5 lines

From: Dirk Eibach <[email protected]>

(quoted diff) +0 -0

@@ -344,7 +345,6 @@ void pci_init_board(void)
 

Quoted 9 lines

(quoted diff) +0 -0

                /* 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;
}

Am 18.11.2015 um 13:48 schrieb Dirk Eibach:

Quoted 6 lines

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.

Quoted 2 lines

So this should perform better:

Quoted 6 lines

/* 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.

From: Dirk Eibach <[email protected]>

mvebu gpio is based on kirkwood.

Signed-off-by Dirk Eibach <[email protected]>

From: Dirk Eibach <[email protected]>

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

Signed-off-by Dirk Eibach <[email protected]>

---

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

drivers/ddr/marvell/a38x/ddr3_hws_hw_training_def.h +2 -2

index 7500a72..06d0ab1 100644
@@ -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

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

From: Dirk Eibach <[email protected]>

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

Signed-off-by Dirk Eibach <[email protected]>

---

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

From: Dirk Eibach <[email protected]>

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

Signed-off-by Dirk Eibach <[email protected]>

---

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

drivers/ddr/marvell/a38x/ddr3_init.c +0 -0

index d6ed8e0..cbfc58c 100644

Hi Dirk,

Quoted 21 lines · dirk.eibach at gdsys.cc

From: Dirk Eibach <[email protected]>

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

Signed-off-by Dirk Eibach <[email protected]>

---

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

drivers/ddr/marvell/a38x/ddr3_init.c +0 -0

index d6ed8e0..cbfc58c 100644
@@ -306,8 +306,6 @@ int ddr3_init(void)
                SAR1_CPU_CORE_OFFSET;
        switch (soc_num) {
        case 0x3:

drivers/ddr/marvell/a38x/ddr3_init.c int ddr3_init(void) · line 311

Quoted 5 lines

drivers/ddr/marvell/a38x/ddr3_init.c +0 -2

-               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?

2015-10-28 17:35 GMT+01:00 Stefan Roese <[email protected]>:

Quoted 24 lines

Hi Dirk,

Quoted 21 lines · dirk.eibach at gdsys.cc

From: Dirk Eibach <[email protected]>

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

Signed-off-by Dirk Eibach <[email protected]>

---

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

drivers/ddr/marvell/a38x/ddr3_init.c +0 -0

index d6ed8e0..cbfc58c 100644
@@ -306,8 +306,6 @@ int ddr3_init(void)
                 SAR1_CPU_CORE_OFFSET;
         switch (soc_num) {
         case 0x3:

Quoted 8 lines

drivers/ddr/marvell/a38x/ddr3_init.c int ddr3_init(void) · line 311

Quoted 5 lines

drivers/ddr/marvell/a38x/ddr3_init.c +0 -2

-               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.

From: Dirk Eibach <[email protected]>

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

Signed-off-by Dirk Eibach <[email protected]>

From: Dirk Eibach <[email protected]>

Armada 38x has two spi controllers.

Signed-off-by Dirk Eibach <[email protected]>

---

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

drivers/spi/kirkwood_spi.c +12 -6

index e7b0982..200c391 100644
@@ -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)

drivers/spi/kirkwood_spi.c +25 -2

+       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

Quoted 54 lines · On 28 October 2015 at 21:14, <[email protected]> wrote:

From: Dirk Eibach <[email protected]>

Armada 38x has two spi controllers.

Signed-off-by Dirk Eibach <[email protected]>

---

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

drivers/spi/kirkwood_spi.c +14 -6

index e7b0982..200c391 100644
@@ -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;
 

drivers/spi/kirkwood_spi.c struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs, · line 60

Quoted 6 lines

drivers/spi/kirkwood_spi.c +6 -0

+       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.

... 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.

2015-10-28 17:39 GMT+01:00 Stefan Roese <[email protected]>:

Quoted 6 lines

... 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?

Hi Dirk,

Quoted 6 lines · Dirk Eibach

2015-10-28 17:39 GMT+01:00 Stefan Roese <[email protected]>:

Quoted 4 lines

... And please note that you can use the
runtime SoC detection for this:

Quoted 7 lines

Quoted 3 lines

         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.