Re: [PATCH v4 09/13] memory: tegra: Add API needed by the EMC driver

[Thierry Reding]

On Wed, Nov 12, 2014 at 08:56:32AM +0100, Tomeu Vizoso wrote:
> From: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> 
> The EMC driver needs to know the number of external memory devices and also
> needs to update the EMEM configuration based on the new rate of the memory bus.
> 
> To know how to update the EMEM config, looks up the values of the burst regs in
> the DT, for a given timing.

This looks somewhat wide. Typically commit messages should wrap after
column 72.

> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
[...]
> index 286b9c5..9d8585a 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -13,6 +13,9 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> +#include <linux/sort.h>
> +
> +#include <soc/tegra/fuse.h>
>  
>  #include "mc.h"
>  
> @@ -48,6 +51,9 @@
>  #define  MC_EMEM_ARB_CFG_CYCLES_PER_UPDATE_MASK	0x1ff
>  #define MC_EMEM_ARB_MISC0 0xd8
>  
> +#define MC_EMEM_ADR_CFG 0x54
> +#define MC_EMEM_ADR_CFG_EMEM_NUMDEV BIT(0)
> +
>  static const struct of_device_id tegra_mc_of_match[] = {
>  #ifdef CONFIG_ARCH_TEGRA_3x_SOC
>  	{ .compatible = "nvidia,tegra30-mc", .data = &tegra30_mc_soc },
> @@ -93,6 +99,124 @@ static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc)
>  	return 0;
>  }
>  
> +void tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate)
> +{
> +	int i;

num_timings is unsigned, so this variable should be as well.

> +	struct tegra_mc_timing timing;
> +
> +	for (i = 0; i < mc->num_timings; ++i) {

There's no reason for the prefix increment here, postfix would be more
idiomatic I think.

> +		timing = mc->timings[i];
> +
> +		if (timing.rate == rate)
> +			break;
> +	}
> +
> +	if (i == mc->num_timings) {

Perhaps turn this into something like this for better readability:

	struct tegra_mc_timing *timing = NULL;

	for (i = 0; i < mc->num_timings; i++) {
		if (mc->timings[i].rate == rate) {
			timing = &mc->timings[i];
			break;
		}
	}

	if (!timing) {

> +		dev_err(mc->dev, "no memory timing registered for rate %lu\n", rate);
> +		return;
> +	}
> +
> +	for (i = 0; i < mc->soc->num_emem_regs; ++i)
> +		mc_writel(mc, timing.emem_data[i], mc->soc->emem_regs[i]);
> +}
> +
> +int tegra_mc_get_emem_device_count(struct tegra_mc *mc)
> +{
> +	u8 dram_count;
> +
> +	dram_count = mc_readl(mc, MC_EMEM_ADR_CFG);
> +	dram_count &= MC_EMEM_ADR_CFG_EMEM_NUMDEV;
> +	dram_count++;
> +
> +	return dram_count;
> +}

Please either make this return unsigned int. signed int would indicate
that the return value needs to be checked.

> +
> +

There's an extra blank line here.

> +static int load_one_timing_from_dt(struct tegra_mc *mc,
> +				   struct tegra_mc_timing *timing,
> +				   struct device_node *node)
> +{
> +	int err;
> +	u32 tmp;
> +
> +	err = of_property_read_u32(node, "clock-frequency", &tmp);
> +	if (err) {
> +		dev_err(mc->dev,
> +			"timing %s: failed to read rate\n", node->name);
> +		return err;
> +	}
> +
> +	timing->rate = tmp;
> +	timing->emem_data = devm_kzalloc(mc->dev, sizeof(u32) * mc->soc->num_emem_regs, GFP_KERNEL);

devm_kcalloc() perhaps? And wrap it to fit within 78 columns.

> +	if (!timing->emem_data)
> +		return -ENOMEM;
> +
> +	err = of_property_read_u32_array(node, "nvidia,emem-configuration",
> +					 timing->emem_data,
> +					 mc->soc->num_emem_regs);
> +	if (err) {
> +		dev_err(mc->dev,
> +			"timing %s: failed to read emc burst data\n",

s/emc/EMC/

> +			node->name);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int load_timings_from_dt(struct tegra_mc *mc,
> +				struct device_node *node)

No need to wrap this, it fits on one line.

> +{
> +	struct device_node *child;
> +	int child_count = of_get_child_count(node);
> +	int i = 0, err;
> +
> +	mc->timings = devm_kzalloc(mc->dev,
> +				      sizeof(struct tegra_mc_timing) * child_count,
> +				      GFP_KERNEL);

devm_kcalloc(), and alignment looks wrong here.

> +	if (!mc->timings)
> +		return -ENOMEM;
> +
> +	mc->num_timings = child_count;
> +
> +	for_each_child_of_node(node, child) {
> +		struct tegra_mc_timing *timing = mc->timings + i++;

Perhaps move the declaration of timing one level up, then you can use
sizeof(*timing) in the call to devm_k{z,c}alloc().

And maybe write this as: timing = &mc->timings[i++];, that's more
consistent with the other parts of the driver.

> +
> +		err = load_one_timing_from_dt(mc, timing, child);

Perhaps leave away the "_from_dt" suffix, there's no alternative to load
this data from, right?

> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tegra_mc_setup_timings(struct tegra_mc *mc)
> +{
> +	struct device_node *node;
> +	u32 ram_code, node_ram_code;
> +	int err;
> +
> +	ram_code = tegra_read_ram_code();
> +
> +	mc->num_timings = 0;
> +
> +	for_each_child_of_node(mc->dev->of_node, node) {
> +		err = of_property_read_u32(node, "nvidia,ram-code", &node_ram_code);
> +		if (err || (node_ram_code != ram_code))
> +			continue;
> +
> +		err = load_timings_from_dt(mc, node);
> +		if (err)
> +			return err;
> +		break;
> +	}
> +
> +	if (mc->num_timings == 0)
> +		dev_warn(mc->dev, "no memory timings for ram code %u registered\n", ram_code);

s/ram/RAM/

>  static const char *const status_names[32] = {
>  	[ 1] = "External interrupt",
>  	[ 6] = "EMEM address decode error",
> @@ -250,6 +374,12 @@ static int tegra_mc_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> +	err = tegra_mc_setup_timings(mc);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "failed to setup timings: %d\n", err);
> +		return err;
> +	}
> +
>  	if (IS_ENABLED(CONFIG_TEGRA_IOMMU_SMMU)) {
>  		mc->smmu = tegra_smmu_probe(&pdev->dev, mc->soc->smmu, mc);
>  		if (IS_ERR(mc->smmu)) {
> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
> index 8fabe99..4596411 100644
> --- a/drivers/memory/tegra/mc.h
> +++ b/drivers/memory/tegra/mc.h
> @@ -14,6 +14,12 @@
>  
>  struct page;
>  
> +struct tegra_mc_timing {
> +	unsigned long rate;
> +
> +	u32 *emem_data;

The emem_ prefix isn't very useful in my opinion.

> +};
> +
>  struct tegra_smmu_enable {
>  	unsigned int reg;
>  	unsigned int bit;
> @@ -67,6 +73,9 @@ struct tegra_mc_soc {
>  	const struct tegra_mc_client *clients;
>  	unsigned int num_clients;
>  
> +	const unsigned int *emem_regs;

These are offsets to registers, right? I would typically use unsigned
long for that, but that's really just a nitpick.

> +	unsigned int num_emem_regs;
> +
>  	unsigned int num_address_bits;
>  	unsigned int atom_size;
>  
> @@ -84,6 +93,9 @@ struct tegra_mc {
>  
>  	const struct tegra_mc_soc *soc;
>  	unsigned long tick;
> +
> +	struct tegra_mc_timing *timings;
> +	unsigned int num_timings;
>  };
>  
>  static inline u32 mc_readl(struct tegra_mc *mc, unsigned long offset)
> diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
> index ccd19d8..bba8e1c 100644
> --- a/drivers/memory/tegra/tegra124.c
> +++ b/drivers/memory/tegra/tegra124.c
> @@ -15,6 +15,48 @@
>  
>  #include "mc.h"
>  
> +#define MC_EMEM_ARB_CFG				0x90
> +#define MC_EMEM_ARB_OUTSTANDING_REQ		0x94
> +#define MC_EMEM_ARB_TIMING_RCD			0x98
> +#define MC_EMEM_ARB_TIMING_RP			0x9c
> +#define MC_EMEM_ARB_TIMING_RC			0xa0
> +#define MC_EMEM_ARB_TIMING_RAS			0xa4
> +#define MC_EMEM_ARB_TIMING_FAW			0xa8
> +#define MC_EMEM_ARB_TIMING_RRD			0xac
> +#define MC_EMEM_ARB_TIMING_RAP2PRE		0xb0
> +#define MC_EMEM_ARB_TIMING_WAP2PRE		0xb4
> +#define MC_EMEM_ARB_TIMING_R2R			0xb8
> +#define MC_EMEM_ARB_TIMING_W2W			0xbc
> +#define MC_EMEM_ARB_TIMING_R2W			0xc0
> +#define MC_EMEM_ARB_TIMING_W2R			0xc4
> +#define MC_EMEM_ARB_DA_TURNS			0xd0
> +#define MC_EMEM_ARB_DA_COVERS			0xd4
> +#define MC_EMEM_ARB_MISC0			0xd8
> +#define MC_EMEM_ARB_MISC1			0xdc
> +#define MC_EMEM_ARB_RING1_THROTTLE		0xe0
> +
> +static int tegra124_mc_emem_regs[] = {

static const, please. And the type should match that of
struct tegra_mc_soc's .emem_regs field.

Thierry

Attachment: pgpr2YfyUlhTg.pgp
Description: PGP signature