[PATCH v2 5/9] ram: octeon: Add MIPS Octeon3 DDR4 support (part 1/3)

[Stefan Roese]

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 at 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 at marvell.com>
>> Signed-off-by: Stefan Roese <sr at 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.
>> + *
>> + * https://spdx.org/licenses
> 
> 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 at 1180080000000 {
			#address-cells = <1>;
			#size-cells = <0>;
			compatible = "cavium,octeon-7xxx-l2c";
			reg = <0x11800 0x80000000 0x0 0x01000000>;
			u-boot,dm-pre-reloc;
		};

		lmc: lmc at 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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de