| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20141112144409.GC26488@ulmo>
Date: Wed, 12 Nov 2014 15:44:10 +0100
From: Thierry Reding <thierry.reding@...il.com>
To: Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc: linux-tegra@...r.kernel.org,
Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
mikko.perttunen@...si.fi, acourbot@...dia.com,
Mikko Perttunen <mperttunen@...dia.com>,
Stephen Warren <swarren@...dotorg.org>,
Alexandre Courbot <gnurou@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 09/13] memory: tegra: Add API needed by the EMC driver
On Wed, Nov 12, 2014 at 08:56:32AM +0100, Tomeu Vizoso wrote:
> From: Mikko Perttunen <mperttunen@...dia.com>
>
> 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
Content of type "application/pgp-signature" skipped
Powered by blists - more mailing lists