Re: [PATCH V4 4/8] memory: tegra: Add Tegra210 EMC clock driver

[Joseph Lo <josephl@nvidia.com>]

On 5/29/19 9:26 PM, Dmitry Osipenko wrote:
> 29.05.2019 11:21, Joseph Lo пишет:
>> This is the initial patch for Tegra210 EMC clock driver, which doesn't
>> include the support code and detail sequence for clock scaling yet.
>>
>> The driver is designed to support LPDDR4 SDRAM. Because of the LPDDR4
>> devices need to do initial time training before it can be used, the
>> firmware will help to do that at early boot stage. Then, the trained
>> table of the rates we support will pass to the kernel via DT. So the
>> driver can get the trained table for clock scaling support.
>>
>> For the higher rate support (above 800MHz), the periodic training is
>> needed for the timing compensation. So basically, two methodologies for
>> clock scaling are supported, one is following the clock changing
>> sequence to update the EMC table to EMC registers and another is if the
>> rate needs periodic training, then we will start a timer to do that
>> periodically until it scales to the lower rate.
>>
>> Based on the work of Peter De Schrijver <pdeschrijver@nvidia.com>.
>>
>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
>> ---
>> v4:
>> - remove the statistic data in debugfs
>> - add tegra210_clk_register_emc API to make it compatible with the case
>>    if the kernel still uses the older DTB which doesn't have EMC node.
>>    And the MC and EMC clock can still be registered successfully.
>> v3:
>> - address almost all the comments from the previous version
>> - remove the DT parser of EMC table
>> - The EMC table is passing as a binary blob now.
>> ---
>>   drivers/memory/tegra/Kconfig        |  10 +
>>   drivers/memory/tegra/Makefile       |   1 +
>>   drivers/memory/tegra/tegra210-emc.c | 671 ++++++++++++++++++++++++++++
>>   drivers/memory/tegra/tegra210-emc.h | 156 +++++++
>>   include/soc/tegra/emc.h             |   2 +
>>   5 files changed, 840 insertions(+)
>>   create mode 100644 drivers/memory/tegra/tegra210-emc.c
>>   create mode 100644 drivers/memory/tegra/tegra210-emc.h
>>
>> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
>> index 4680124ddcab..9d051bcdbee3 100644
>> --- a/drivers/memory/tegra/Kconfig
>> +++ b/drivers/memory/tegra/Kconfig
>> @@ -26,3 +26,13 @@ config TEGRA124_EMC
>>   	  Tegra124 chips. The EMC controls the external DRAM on the board.
>>   	  This driver is required to change memory timings / clock rate for
>>   	  external memory.
>> +
>> +config TEGRA210_EMC
>> +	bool "NVIDIA Tegra210 External Memory Controller driver"
>> +	default y
> 
> This is not enough since you're leaving possibility to disable
> compilation of the driver, but the compilation will fail because of the
> unresolved symbol (tegra210_clk_register_emc).
> 
>> +	depends on TEGRA_MC && ARCH_TEGRA_210_SOC
>> +	help
>> +	  This driver is for the External Memory Controller (EMC) found on
>> +	  Tegra210 chips. The EMC controls the external DRAM on the board.
>> +	  This driver is required to change memory timings / clock rate for
>> +	  external memory.
> 
> Either TEGRA210_EMC Kconfig option shall be always force-selected for
> T210 or you should move all the clk-related code into drivers/clk/tegra/.
> 
> Could you please give a rationale for having EMC clock code within the
> EMC driver?

I didn't have a specific reason for that initially, just wanted the 
clock code and EMC driver together for easier maintenance.

But considering the fix in v4, that makes it backward compatible with 
the case if the kernel uses the older DT without EMC node, I think it's 
better to move the clock code into the clk folder now.

> 
> [snip]
> 
>> +static int tegra210_emc_probe(struct platform_device *pdev)
>> +{
>> +	int i;
>> +	unsigned long table_rate;
>> +	unsigned long current_rate;
>> +	struct clk *emc_clk;
>> +	struct device_node *np;
>> +	struct platform_device *mc;
>> +	struct resource res;
>> +	struct tegra_emc *emc;
>> +	void *table_addr;
>> +
>> +	emc_clk = devm_clk_get(&pdev->dev, "emc");
>> +	if (IS_ERR(emc_clk))
>> +		return PTR_ERR(emc_clk);
> 
> Please add newline in a such places in the code to make it more readable.
> 
>> +	emc = clk_hw_to_emc(__clk_get_hw(emc_clk));
>> +
>> +	np = of_parse_phandle(pdev->dev.of_node, "nvidia,memory-controller", 0);
>> +	if (!np) {
>> +		dev_err(&pdev->dev, "could not get memory controller\n");
>> +		return -ENOENT;
>> +	}
>> +
>> +	mc = of_find_device_by_node(np);
>> +	of_node_put(np);
>> +	if (!mc)
>> +		return -ENOENT;
>> +
>> +	emc->mc = platform_get_drvdata(mc);
>> +	if (!emc->mc)
>> +		return -EPROBE_DEFER;
>> +
>> +	emc->emc_base[REG_EMC] = devm_platform_ioremap_resource(pdev, 0);
>> +	emc->emc_base[REG_EMC0] = devm_platform_ioremap_resource(pdev, 1);
>> +	emc->emc_base[REG_EMC1] = devm_platform_ioremap_resource(pdev, 2);
>> +
>> +	for (i = 0; i < TEGRA_EMC_SRC_COUNT; i++) {
>> +		if (!IS_ERR(emc_src[i]))
>> +			clk_put(emc_src[i]);
>> +
>> +		emc_src[i] = devm_clk_get(&pdev->dev, emc_src_names[i]);
>> +		if (IS_ERR(emc_src[i])) {
>> +			dev_err(&pdev->dev, "Can not find EMC source clock\n");
>> +			return -ENODATA;
>> +		}
>> +	}
>> +
>> +	np = of_parse_phandle(pdev->dev.of_node, "memory-region", 0);
>> +	if (!np) {
>> +		dev_err(&pdev->dev, "could not find EMC table\n");
>> +		return -ENODATA;
>> +	}
>> +
>> +	if (!of_device_is_compatible(np, "nvidia,tegra210-emc-table") ||
>> +	    !of_device_is_available(np)) {
>> +		dev_err(&pdev->dev, "EMC table is invalid\n");
>> +		return -ENODATA;
>> +	}
>> +
>> +	of_address_to_resource(np, 0, &res);
>> +	table_addr = memremap(res.start, resource_size(&res), MEMREMAP_WB);
>> +	of_node_put(np);
>> +	if (!table_addr) {
>> +		dev_err(&pdev->dev, "could not map EMC table\n");
>> +		return -ENOMEM;
>> +	}
>> +	emc->emc_table = (struct emc_table *)table_addr;
> 
> There is no need to cast a void. Please make sure that you don't have
> any non-upstream patches applied that are changing compiler flags, also
> make sure that you're using proper compiler (GCC) if you're getting a
> warning here.
> 
>> +
>> +	for (i = 0; i < TEGRA_EMC_MAX_FREQS; i++)
>> +		if (emc->emc_table[i].rev != 0)
>> +			emc->emc_table_size++;
>> +		else
>> +			break;
>> +
>> +	/* check the supported sequence */
>> +	while (seq->table_rev) {
>> +		if (seq->table_rev == emc->emc_table[0].rev)
>> +			break;
>> +		seq++;
>> +	}
>> +	if (!seq->set_clock) {
>> +		seq = NULL;
>> +		dev_err(&pdev->dev, "Invalid EMC sequence for table Rev. %d\n",
>> +			emc->emc_table[0].rev);
>> +		return -ENODATA;
>> +	}
>> +
>> +	emc_clk_sel = devm_kcalloc(&pdev->dev,
>> +				   emc->emc_table_size,
>> +				   sizeof(struct emc_sel),
>> +				   GFP_KERNEL);
>> +
>> +	/* calculate the rate from source clock */
>> +	current_rate = emc_get_src_clk_rate() / 1000;
>> +
>> +	/* validate the table */
>> +	for (i = 0; i < emc->emc_table_size; i++) {
>> +		table_rate = emc->emc_table[i].rate;
>> +		if (!table_rate)
>> +			continue;
>> +
>> +		if (i && ((table_rate <= emc->emc_table[i-1].rate) ||
>> +		   (emc->emc_table[i].min_volt <
>> +		    emc->emc_table[i-1].min_volt)))
>> +			continue;
>> +
>> +		if (emc->emc_table[i].rev != emc->emc_table[0].rev)
>> +			continue;
>> +
>> +		if (find_matching_input(&emc->emc_table[i], &emc_clk_sel[i]))
>> +			continue;
>> +
>> +		if (table_rate == current_rate)
>> +			emc->current_timing = &emc->emc_table[i];
>> +	}
> 
> I'm suggesting to factor out all the validations into a separate
> function for clarity.
> 

Okay, will fix.

Thanks,
Joseph

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel