Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1658318 > unrolled thread

[PATCH] EDAC: mv64x60 calculate memory size correctly

Back to article view | Back to linux.kernel

Contents

  [PATCH] EDAC: mv64x60 calculate memory size correctly Chris Packham <chris.packham@alliedtelesis.co.nz> - 2017-06-06 04:50 +0200
    Re: [PATCH] EDAC: mv64x60 calculate memory size correctly Chris Packham <Chris.Packham@alliedtelesis.co.nz> - 2017-06-06 05:10 +0200
      Re: [PATCH] EDAC: mv64x60 calculate memory size correctly Borislav Petkov <bp@alien8.de> - 2017-06-06 19:00 +0200
        Re: [PATCH] EDAC: mv64x60 calculate memory size correctly Chris Packham <Chris.Packham@alliedtelesis.co.nz> - 2017-06-06 23:20 +0200
          Re: [PATCH] EDAC: mv64x60 calculate memory size correctly Borislav Petkov <bp@alien8.de> - 2017-06-07 11:10 +0200

#1658318 — [PATCH] EDAC: mv64x60 calculate memory size correctly

The #address-cells and #size-cells properties need to be accounted for
when dealing with the "memory" device tree node.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/edac/mv64x60_edac.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/edac/mv64x60_edac.c b/drivers/edac/mv64x60_edac.c
index 43efb086c0b4..09abb963688f 100644
--- a/drivers/edac/mv64x60_edac.c
+++ b/drivers/edac/mv64x60_edac.c
@@ -644,15 +644,27 @@ static irqreturn_t mv64x60_mc_isr(int irq, void *dev_id)
 static void get_total_mem(struct mv64x60_mc_pdata *pdata)
 {
 	struct device_node *np = NULL;
-	const unsigned int *reg;
-
-	np = of_find_node_by_type(NULL, "memory");
-	if (!np)
-		return;
-
-	reg = of_get_property(np, "reg", NULL);
-
-	pdata->total_mem = reg[1];
+	const unsigned int *reg, *reg_end;
+	int len, sw, aw;
+	unsigned long start, size, total_mem = 0;
+
+	for_each_node_by_type(np, "memory") {
+		aw = of_n_addr_cells(np);
+		sw = of_n_size_cells(np);
+		reg = of_get_property(np, "reg", &len);
+		reg_end = reg + (len / sizeof(u32));
+
+		total_mem = 0;
+		do {
+			start = of_read_number(reg, aw);
+			reg += aw;
+			size = of_read_number(reg, sw);
+			reg += sw;
+			total_mem += size;
+		} while (reg < reg_end);
+	}
+
+	pdata->total_mem = total_mem;
 }
 
 static void mv64x60_init_csrows(struct mem_ctl_info *mci,
-- 
2.13.0

[toc] | [next] | [standalone]

#1658324

On 06/06/17 14:41, Chris Packham wrote:
> The #address-cells and #size-cells properties need to be accounted for
> when dealing with the "memory" device tree node.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>   drivers/edac/mv64x60_edac.c | 30 +++++++++++++++++++++---------
>   1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/edac/mv64x60_edac.c b/drivers/edac/mv64x60_edac.c
> index 43efb086c0b4..09abb963688f 100644
> --- a/drivers/edac/mv64x60_edac.c
> +++ b/drivers/edac/mv64x60_edac.c
> @@ -644,15 +644,27 @@ static irqreturn_t mv64x60_mc_isr(int irq, void *dev_id)
>   static void get_total_mem(struct mv64x60_mc_pdata *pdata)
>   {
>   	struct device_node *np = NULL;
> -	const unsigned int *reg;
> -
> -	np = of_find_node_by_type(NULL, "memory");
> -	if (!np)
> -		return;
> -
> -	reg = of_get_property(np, "reg", NULL);
> -
> -	pdata->total_mem = reg[1];
> +	const unsigned int *reg, *reg_end;
> +	int len, sw, aw;
> +	unsigned long start, size, total_mem = 0;
> +
> +	for_each_node_by_type(np, "memory") {
> +		aw = of_n_addr_cells(np);
> +		sw = of_n_size_cells(np);
> +		reg = of_get_property(np, "reg", &len);
> +		reg_end = reg + (len / sizeof(u32));
> +
> +		total_mem = 0;
> +		do {
> +			start = of_read_number(reg, aw);
> +			reg += aw;
> +			size = of_read_number(reg, sw);
> +			reg += sw;
> +			total_mem += size;
> +		} while (reg < reg_end);
> +	}
> +
> +	pdata->total_mem = total_mem;


Just after I sent this I realized the following is probably a better 
approach

+       struct resource res;
+       int ret;
+       unsigned long total_mem = 0;
+
+       for_each_node_by_type(np, "memory") {
+               ret = of_address_to_resource(np, 0, &res);
+               if (ret)
+                       continue;
+
+               total_mem += resource_size(&res);
+       }
+
+       pdata->total_mem = total_mem;

I'll wait for feedback before sending a v2.

>   }
>   
>   static void mv64x60_init_csrows(struct mem_ctl_info *mci,
> 

[toc] | [prev] | [next] | [standalone]

#1658941

On Tue, Jun 06, 2017 at 03:07:04AM +0000, Chris Packham wrote:
> I'll wait for feedback before sending a v2.

You can't be expecting me to review PPC code reliably. :-)

AFAIR from the recent discussion, Michael said that the aim is to remove
CONFIG_MV64X60 and since this driver depends on it, that would make it
obsolete too.

But I don't think we've ever "hijacked" a driver and renamed it to be
used as a driver on a different architecture - that would be just too
unorthodox.

So if I had to decide, I'd suggest you create your own armada_edac.c or
whatever that is and put your code there. Or, if you're going to support
multiple Marvell chips, then call it mv_edac.c or marvell_edac.c or so
and start building a fine driver there.

How does that sound?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [next] | [standalone]

#1659218

On 07/06/17 04:55, Borislav Petkov wrote:
> On Tue, Jun 06, 2017 at 03:07:04AM +0000, Chris Packham wrote:
>> I'll wait for feedback before sending a v2.
> 
> You can't be expecting me to review PPC code reliably. :-)

Yeah sorry I should have included linuxppc-dev. Will do on v2.

I don't think the architecture matters with this particular change. It's 
just about using the appropriate APIs to look something up from the 
architecture agnostic devicetree.

Note that there is a similar pattern in altera_edac.c and cpc925_edac.c 
that could probably benefit from something like my proposed v2.

> AFAIR from the recent discussion, Michael said that the aim is to remove
> CONFIG_MV64X60 and since this driver depends on it, that would make it
> obsolete too.

Which would be reason enough for me to stop tinkering with 
mv64x60_edac.c as you suggest.

> But I don't think we've ever "hijacked" a driver and renamed it to be
> used as a driver on a different architecture - that would be just too
> unorthodox.
> 
> So if I had to decide, I'd suggest you create your own armada_edac.c or
> whatever that is and put your code there. Or, if you're going to support
> multiple Marvell chips, then call it mv_edac.c or marvell_edac.c or so
> and start building a fine driver there.
> 
> How does that sound?

"mvebu" is the current trend for this family of orion/kirkwood/armada 
SoCs. I can take mv64x60_edac.c and gut it to use as a base. That would 
also offer me the opportunity to do some of the other things I've been 
wanting to.

I'll still send out v2 of this patch and include cleanups for altera and 
cpc925 in a series. You can decide which if any to apply.

[toc] | [prev] | [next] | [standalone]

#1659563

On Tue, Jun 06, 2017 at 09:19:04PM +0000, Chris Packham wrote:
> "mvebu" is the current trend for this family of orion/kirkwood/armada 
> SoCs. I can take mv64x60_edac.c and gut it to use as a base.

s/gut it/copy stuff for your own use in your *separate* driver/

Let's leave mv64x60_edac.c alone.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web