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 awilliams@marvell.com
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 awilliams@marvell.com Signed-off-by: Stefan Roese sr@denx.de
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 0000000000..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.
this line is superfluous
Removed.
- */
+#include <common.h>
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 <command.h> +#include <dm.h> +#include <hang.h> +#include <i2c.h> +#include <ram.h> +#include <time.h>
+#include <asm/sections.h> +#include <linux/io.h>
+#include <mach/octeon_ddr.h>
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.
<snip>
+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 < (0x100000 / 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,
&l2c_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@1180080000000 { #address-cells = <1>; #size-cells = <0>; compatible = "cavium,octeon-7xxx-l2c"; reg = <0x11800 0x80000000 0x0 0x01000000>; u-boot,dm-pre-reloc; };
lmc: lmc@1180088000000 { #address-cells = <1>; #size-cells = <0>; compatible = "cavium,octeon-7xxx-ddr4"; reg = <0x11800 0x88000000 0x0 0x02000000>; // 2 IFs u-boot,dm-pre-reloc; l2c-handle = <&l2c>; };
Can I read the mapped L2C controller address more elegant?
- ddr_conf_ptr = octeon_ddr_conf_table_get(&conf_table_count,
&def_ddr_freq);- if (!ddr_conf_ptr) {
printf("ERROR: unable to determine DDR configuration\n");return -ENODEV;- }
- for (i = 0; i < conf_table_count; i++) {
if (ddr_conf_ptr[i].dimm_config_table[0].spd_addrs[0] ||ddr_conf_ptr[i].dimm_config_table[0].spd_ptrs[0])ddr_conf_valid_mask |= 1 << i;- }
- /*
* Check for special case of mismarked 3005 samples,* and adjust cpuid*/- alt_refclk = 0;
- ddr_hertz = def_ddr_freq * 1000000;
- eptr = env_get("ddr_clock_hertz");
- if (eptr) {
ddr_hertz = simple_strtoul(eptr, NULL, 0);gd->mem_clk = divide_nint(ddr_hertz, 1000000);printf("Parameter found in environment. ddr_clock_hertz = %d\n",ddr_hertz);- }
who will set this in the environment and when? Shouldn't this be configurable via device-tree?
As mentioned before, I've ported all this from the original 2013 Octeon U-Boot version with no functional change (intended). These env variables are a tunining method (for testing purpose most likely), which I didn't want to remove. So that the mainline DDR code does not lack any functionality (features, tuning, test methods) that the original code had.
I hope you understand this reasoning.
Thanks, Stefan
- ddr_ref_hertz = octeon3_refclock(alt_refclk,
ddr_hertz,&ddr_conf_ptr[0].dimm_config_table[0]);- debug("Initializing DDR, clock = %uhz, reference = %uhz\n",
ddr_hertz, ddr_ref_hertz);- mem_mbytes = octeon_ddr_initialize(priv, gd->cpu_clk,
ddr_hertz, ddr_ref_hertz,ddr_conf_valid_mask,ddr_conf_ptr, &measured_ddr_hertz);- debug("Mem size in MBYTES: %u\n", mem_mbytes);
- gd->mem_clk = divide_nint(measured_ddr_hertz, 1000000);
- debug("Measured DDR clock %d Hz\n", measured_ddr_hertz);
- if (measured_ddr_hertz != 0) {
if (!gd->mem_clk) {/** If ddr_clock not set, use measured clock* and don't warn*/gd->mem_clk = divide_nint(measured_ddr_hertz, 1000000);} else if ((measured_ddr_hertz > ddr_hertz + 3000000) ||(measured_ddr_hertz < ddr_hertz - 3000000)) {printf("\nWARNING:\n");printf("WARNING: Measured DDR clock mismatch! expected: %lld MHz, measured: %lldMHz, cpu clock: %lu MHz\n",divide_nint(ddr_hertz, 1000000),divide_nint(measured_ddr_hertz, 1000000),gd->cpu_clk);printf("WARNING:\n\n");gd->mem_clk = divide_nint(measured_ddr_hertz, 1000000);}- }
- if (!mem_mbytes)
return -ENODEV;- priv->info.base = CONFIG_SYS_SDRAM_BASE;
- priv->info.size = MB(mem_mbytes);
- /*
* For 6XXX generate a proper error when reading/writing* non-existent memory locations.*/- cvmx_l2c_set_big_size(priv, mem_mbytes, 0);
- debug("Ram size %uMiB\n", mem_mbytes);
- return 0;
+}
+static int octeon_get_info(struct udevice *dev, struct ram_info *info) +{
- struct ddr_priv *priv = dev_get_priv(dev);
- *info = priv->info;
- return 0;
+}
+static struct ram_ops octeon_ops = {
- .get_info = octeon_get_info,
+};
+static const struct udevice_id octeon_ids[] = {
- {.compatible = "cavium,octeon-7xxx-ddr4" },
- { }
+};
+U_BOOT_DRIVER(octeon_ddr) = {
- .name = "octeon_ddr",
- .id = UCLASS_RAM,
- .of_match = octeon_ids,
- .ops = &octeon_ops,
- .probe = octeon_ddr_probe,
- .platdata_auto_alloc_size = sizeof(struct ddr_priv),
+};
Viele Grüße, Stefan