Re: [PATCH v2 5/9] ram: octeon: Add MIPS Octeon3 DDR4 support (part 1/3)
Hi Daniel,
On 19.08.20 16:30, Daniel Schwierzeck wrote:
Am Montag, den 17.08.2020, 14:12 +0200 schrieb Stefan Roese:
From: Aaron Williams
This Octeon 3 DDR driver is ported from the 2013 Cavium / Marvell U-Boot
repository. It currently supports DDR4 on Octeon 3. It can be later
extended to support also DDR3 and Octeon 2 platforms.
Part 1 adds the base U-Boot RAM driver, which will be instantiated by
the DT based probing.
Signed-off-by: Aaron Williams
Signed-off-by: Stefan Roese
---
Changes in v2:
- Don't re-init after relocation
- Some unsupported Octeon families removed (only Octeon 2 & 3 supported
in general)
drivers/ram/octeon/octeon_ddr.c | 2730 +++
1 file changed, 2730 insertions(+)
create mode 100644 drivers/ram/octeon/octeon_ddr.c
some general notes:
- there are still some C++ style comments
Yes. Please see my comment from "[PATCH v2 4/9] mips: octeon: Add
octeon_ddr.h header" on this.
- there are a lot of non-static functions, are them really used in
other source files and why?
This separation into multiple files is historical. One reason for this
split is, that the file "octeon3_lmc.c" will not be not used by all
Octeon versions. It will be replaced (Makefile switch) by a different
driver file for other Octeon SoC's - once we get to supporting these.
Of course you can split the code into
multiple files but this file is the driver entrypoint and shouldn't
export anything
I'm not sure, if I understand this logic fully. You want me to split
this file again, so that it only includes the (DM) driver entrypoint?
What's the reasoning for this?
diff --git a/drivers/ram/octeon/octeon_ddr.c b/drivers/ram/octeon/octeon_ddr.c
new file mode 100644
index 00..0b347b61fd
--- /dev/null
+++ b/drivers/ram/octeon/octeon_ddr.c
@@ -0,0 +1,2730 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Marvell International Ltd.
+ *
+ * https://spdx.org/licenses
this line is superfluous
Removed.
+ */
+
+#include
try to avoid common.h in new code
Yes, sorry. I should have remembered this. But these changes started
before "common.h" got deprecated. I'll check all DDR files for this.
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#include
do Octeon ARM and MIPS share the same DDR3/4 memory controller? If yes,
would this driver support both archs and would ARM provide an own
version of octeon_ddr.h?
No. The DDR3/4 code is *only* used by the "older" MIPS Octeon II/III.
The ARM Octeon TX variants have the DDR init code integrated in the
other binaries, like TF-A. So no code for newer SoC will even be
integrated into this code base.
+static int octeon_ddr_probe(struct udevice *dev)
+{
+ struct ddr_priv *priv = dev_get_priv(dev);
+ struct ofnode_phandle_args l2c_node;
+ struct ddr_conf *ddr_conf_ptr;
+ u32 ddr_conf_valid_mask = 0;
+ u32 measured_ddr_hertz = 0;
+ int conf_table_count;
+ int def_ddr_freq;
+ u32 mem_mbytes = 0;
+ u32 ddr_hertz;
+ u32 ddr_ref_hertz;
+ int alt_refclk;
+ const char *eptr;
+ fdt_addr_t addr;
+ u64 *ptr;
+ u64 val;
+ int ret;
+ int i;
+
+ /* Don't try to re-init the DDR controller after relocation */
+ if (gd->flags & GD_FLG_RELOC)
+ return 0;
+
+ /*
+* Dummy read all local variables into cache, so that they are
+* locked in cache when the DDR code runs with flushes etc enabled
+*/
+ ptr = (u64 *)_end;
+ for (i = 0; i < (0x10 / sizeof(u64)); i++)
+ val = readq(ptr++);
+
+ /*
+* The base addresses of LMC and L2C are read from the DT. This
+* makes it possible to use the DDR init code without the need
+* of the "node" variable, describing on which node to access. The
+* node number is already included implicitly in the base addresses
+* read from the DT this way.
+*/
+
+ /* Get LMC base address */
+ priv->lmc_base = dev_remap_addr(dev);
+ debug("%s: lmc_base=%p\n", __func__, priv->lmc_base);
+
+ /* Get L2C base address */
+ ret = dev_read_phandle_with_args(dev, "l2c-handle", NULL, 0, 0,
+_node);
+ if (ret) {
+ printf("Can't access L2C node!\n");
+ return -ENODEV;
+ }
+
+ addr = ofnode_get_addr(l2c_node.node);
+ if (addr == FDT_ADDR_T_NONE) {
+ printf("Can't access L2C node!\n");
+ return -ENODEV;
+ }
+
+ priv->l2c_base = map_physmem(addr, 0, MAP_NOCACHE);
+ debug("%s: l2c_base=%p\n", __func__, priv->l2c_base);
why not dev_remap_addr()?
I'm not sure, how I can access the "udev" necessary for this for the
L2C DT node:
l2c: l2c@118008000 {
#address-cells = <1>;
#size-cells = <0>;
Re: [PATCH v2 5/9] ram: octeon: Add MIPS Octeon3 DDR4 support (part 1/3)
Am Montag, den 17.08.2020, 14:12 +0200 schrieb Stefan Roese:
> From: Aaron Williams
>
> This Octeon 3 DDR driver is ported from the 2013 Cavium / Marvell U-Boot
> repository. It currently supports DDR4 on Octeon 3. It can be later
> extended to support also DDR3 and Octeon 2 platforms.
>
> Part 1 adds the base U-Boot RAM driver, which will be instantiated by
> the DT based probing.
>
> Signed-off-by: Aaron Williams
> Signed-off-by: Stefan Roese
>
> ---
>
> Changes in v2:
> - Don't re-init after relocation
> - Some unsupported Octeon families removed (only Octeon 2 & 3 supported
> in general)
>
> drivers/ram/octeon/octeon_ddr.c | 2730 +++
> 1 file changed, 2730 insertions(+)
> create mode 100644 drivers/ram/octeon/octeon_ddr.c
some general notes:
- there are still some C++ style comments
- there are a lot of non-static functions, are them really used in
other source files and why? Of course you can split the code into
multiple files but this file is the driver entrypoint and shouldn't
export anything
>
> diff --git a/drivers/ram/octeon/octeon_ddr.c b/drivers/ram/octeon/octeon_ddr.c
> new file mode 100644
> index 00..0b347b61fd
> --- /dev/null
> +++ b/drivers/ram/octeon/octeon_ddr.c
> @@ -0,0 +1,2730 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Marvell International Ltd.
> + *
> + * https://spdx.org/licenses
this line is superfluous
> + */
> +
> +#include
try to avoid common.h in new code
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include
do Octeon ARM and MIPS share the same DDR3/4 memory controller? If yes,
would this driver support both archs and would ARM provide an own
version of octeon_ddr.h?
> +
> +#define CONFIG_REF_HERTZ 5000
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/* Sign of an integer */
> +static s64 _sign(s64 v)
> +{
> + return (v < 0);
> +}
> +
> +#ifndef DDR_NO_DEBUG
> +char *lookup_env(struct ddr_priv *priv, const char *format, ...)
> +{
> + char *s;
> + unsigned long value;
> + va_list args;
> + char buffer[64];
> +
> + va_start(args, format);
> + vsnprintf(buffer, sizeof(buffer), format, args);
> + va_end(args);
> +
> + s = ddr_getenv_debug(priv, buffer);
> + if (s) {
> + value = simple_strtoul(s, NULL, 0);
> + printf("Parameter found in environment %s=\"%s\" 0x%lx (%ld)\n",
> +buffer, s, value, value);
> + }
> +
> + return s;
> +}
> +
> +char *lookup_env_ull(struct ddr_priv *priv, const char *format, ...)
> +{
> + char *s;
> + u64 value;
> + va_list args;
> + char buffer[64];
> +
> + va_start(args, format);
> + vsnprintf(buffer, sizeof(buffer), format, args);
> + va_end(args);
> +
> + s = ddr_getenv_debug(priv, buffer);
> + if (s) {
> + value = simple_strtoull(s, NULL, 0);
> + printf("Parameter found in environment. %s = 0x%016llx\n",
> +buffer, value);
> + }
> +
> + return s;
> +}
> +#else
> +char *lookup_env(struct ddr_priv *priv, const char *format, ...)
> +{
> + return NULL;
> +}
> +
> +char *lookup_env_ull(struct ddr_priv *priv, const char *format, ...)
> +{
> + return NULL;
> +}
> +#endif
> +
> +/* Number of L2C Tag-and-data sections (TADs) that are connected to LMC. */
> +#define CVMX_L2C_TADS ((OCTEON_IS_MODEL(OCTEON_CN68XX) || \
> + OCTEON_IS_MODEL(OCTEON_CN73XX) || \
> + OCTEON_IS_MODEL(OCTEON_CNF75XX)) ? 4 : \
> + (OCTEON_IS_MODEL(OCTEON_CN78XX)) ? 8 : 1)
> +
> +/* Number of L2C IOBs connected to LMC. */
> +#define CVMX_L2C_IOBS ((OCTEON_IS_MODEL(OCTEON_CN68XX) || \
> + OCTEON_IS_MODEL(OCTEON_CN78XX) || \
> + OCTEON_IS_MODEL(OCTEON_CN73XX) || \
> + OCTEON_IS_MODEL(OCTEON_CNF75XX)) ? 2 : 1)
> +
> +#define CVMX_L2C_MAX_MEMSZ_ALLOWED (OCTEON_IS_OCTEON2() ?\
> + (32 * CVMX_L2C_TADS) : \
> + (OCTEON_IS_MODEL(OCTEON_CN70XX) ? \
> + 512 : (OCTEON_IS_OCTEON3() ? 1024 : 0)))
> +
> +/**
> + * Initialize the BIG address in L2C+DRAM to generate proper error
> + * on reading/writing to an non-existent memory location.
> + *
> + * @param node OCX CPU node number
> + * @param mem_size Amount of DRAM configured in MB.
> + * @param mode Allow/Disallow reporting errors L2C_INT_SUM[BIGRD,BIGWR].
> + */
> +static void cvmx_l2c_set_big_size(struct ddr_priv *priv, u64 mem_size, int
> mode)
> +{
> + if ((OCTEON_IS_OCTEON2() || OCTEON_IS_OCTEON3()) &&
> + !OCTEON_IS_MODEL(OCTEON_CN63XX_PASS1_X)) {
> + union cvmx_l2c_big_ctl big_ctl;
> + int bits = 0, zero_bits = 0;
> + u64 mem;
>
[PATCH v2 5/9] ram: octeon: Add MIPS Octeon3 DDR4 support (part 1/3)
From: Aaron Williams
This Octeon 3 DDR driver is ported from the 2013 Cavium / Marvell U-Boot
repository. It currently supports DDR4 on Octeon 3. It can be later
extended to support also DDR3 and Octeon 2 platforms.
Part 1 adds the base U-Boot RAM driver, which will be instantiated by
the DT based probing.
Signed-off-by: Aaron Williams
Signed-off-by: Stefan Roese
---
Changes in v2:
- Don't re-init after relocation
- Some unsupported Octeon families removed (only Octeon 2 & 3 supported
in general)
drivers/ram/octeon/octeon_ddr.c | 2730 +++
1 file changed, 2730 insertions(+)
create mode 100644 drivers/ram/octeon/octeon_ddr.c
diff --git a/drivers/ram/octeon/octeon_ddr.c b/drivers/ram/octeon/octeon_ddr.c
new file mode 100644
index 00..0b347b61fd
--- /dev/null
+++ b/drivers/ram/octeon/octeon_ddr.c
@@ -0,0 +1,2730 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Marvell International Ltd.
+ *
+ * https://spdx.org/licenses
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#include
+
+#define CONFIG_REF_HERTZ 5000
+
+DECLARE_GLOBAL_DATA_PTR;
+
+/* Sign of an integer */
+static s64 _sign(s64 v)
+{
+ return (v < 0);
+}
+
+#ifndef DDR_NO_DEBUG
+char *lookup_env(struct ddr_priv *priv, const char *format, ...)
+{
+ char *s;
+ unsigned long value;
+ va_list args;
+ char buffer[64];
+
+ va_start(args, format);
+ vsnprintf(buffer, sizeof(buffer), format, args);
+ va_end(args);
+
+ s = ddr_getenv_debug(priv, buffer);
+ if (s) {
+ value = simple_strtoul(s, NULL, 0);
+ printf("Parameter found in environment %s=\"%s\" 0x%lx (%ld)\n",
+ buffer, s, value, value);
+ }
+
+ return s;
+}
+
+char *lookup_env_ull(struct ddr_priv *priv, const char *format, ...)
+{
+ char *s;
+ u64 value;
+ va_list args;
+ char buffer[64];
+
+ va_start(args, format);
+ vsnprintf(buffer, sizeof(buffer), format, args);
+ va_end(args);
+
+ s = ddr_getenv_debug(priv, buffer);
+ if (s) {
+ value = simple_strtoull(s, NULL, 0);
+ printf("Parameter found in environment. %s = 0x%016llx\n",
+ buffer, value);
+ }
+
+ return s;
+}
+#else
+char *lookup_env(struct ddr_priv *priv, const char *format, ...)
+{
+ return NULL;
+}
+
+char *lookup_env_ull(struct ddr_priv *priv, const char *format, ...)
+{
+ return NULL;
+}
+#endif
+
+/* Number of L2C Tag-and-data sections (TADs) that are connected to LMC. */
+#define CVMX_L2C_TADS ((OCTEON_IS_MODEL(OCTEON_CN68XX) || \
+OCTEON_IS_MODEL(OCTEON_CN73XX) || \
+OCTEON_IS_MODEL(OCTEON_CNF75XX)) ? 4 : \
+ (OCTEON_IS_MODEL(OCTEON_CN78XX)) ? 8 : 1)
+
+/* Number of L2C IOBs connected to LMC. */
+#define CVMX_L2C_IOBS ((OCTEON_IS_MODEL(OCTEON_CN68XX) || \
+OCTEON_IS_MODEL(OCTEON_CN78XX) || \
+OCTEON_IS_MODEL(OCTEON_CN73XX) || \
+OCTEON_IS_MODEL(OCTEON_CNF75XX)) ? 2 : 1)
+
+#define CVMX_L2C_MAX_MEMSZ_ALLOWED (OCTEON_IS_OCTEON2() ? \
+ (32 * CVMX_L2C_TADS) : \
+ (OCTEON_IS_MODEL(OCTEON_CN70XX) ? \
+512 : (OCTEON_IS_OCTEON3() ? 1024 : 0)))
+
+/**
+ * Initialize the BIG address in L2C+DRAM to generate proper error
+ * on reading/writing to an non-existent memory location.
+ *
+ * @param node OCX CPU node number
+ * @param mem_size Amount of DRAM configured in MB.
+ * @param mode Allow/Disallow reporting errors L2C_INT_SUM[BIGRD,BIGWR].
+ */
+static void cvmx_l2c_set_big_size(struct ddr_priv *priv, u64 mem_size, int
mode)
+{
+ if ((OCTEON_IS_OCTEON2() || OCTEON_IS_OCTEON3()) &&
+ !OCTEON_IS_MODEL(OCTEON_CN63XX_PASS1_X)) {
+ union cvmx_l2c_big_ctl big_ctl;
+ int bits = 0, zero_bits = 0;
+ u64 mem;
+
+ if (mem_size > (CVMX_L2C_MAX_MEMSZ_ALLOWED * 1024ull)) {
+ printf("WARNING: Invalid memory size(%lld) requested,
should be <= %lld\n",
+ mem_size,
+ (u64)CVMX_L2C_MAX_MEMSZ_ALLOWED * 1024);
+ mem_size = CVMX_L2C_MAX_MEMSZ_ALLOWED * 1024;
+ }
+
+ mem = mem_size;
+ while (mem) {
+ if ((mem & 1) == 0)
+ zero_bits++;
+ bits++;
+ mem >>= 1;
+ }
+
+ if ((bits - zero_bits) != 1 || (bits - 9) <= 0) {
+ printf("ERROR: Invalid DRAM size (%lld) requested,
refer
