[PATCH] edac: mpc85xx: add support for mpc83xx memory controller

Add support for the Freescale MPC83xx memory controller to the existing
driver for the Freescale MPC85xx memory controller. The only difference
between the two processors are in the CS_BNDS register parsing code.

The L2 cache controller does not exist on the MPC83xx, but the OF subsystem
will not use the driver if the device is not present in the OF device tree.

Signed-off-by: Ira W. Snyder <redacted>
---

This was tested on an MPC8349EMDS-based board.

I don't really like how the MCE disable works on mpc85xx (clearing the
HID1 register), but this can be revisited when mpc86xx support gets
added. It sucks to have this happen before the probe() routine is called
and we know what kind of hardware we're actually running on.

 drivers/edac/Kconfig        |    6 +++---
 drivers/edac/mpc85xx_edac.c |   38 ++++++++++++++++++++++++++++----------
 drivers/edac/mpc85xx_edac.h |    3 +++
 3 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 4339b1a..78303f9 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig

@@ -176,11 +176,11 @@ config EDAC_I5100
 	  San Clemente MCH.
 
 config EDAC_MPC85XX
-	tristate "Freescale MPC85xx"
-	depends on EDAC_MM_EDAC && FSL_SOC && MPC85xx
+	tristate "Freescale MPC83xx / MPC85xx"
+	depends on EDAC_MM_EDAC && FSL_SOC && (PPC_83xx || MPC85xx)
 	help
 	  Support for error detection and correction on the Freescale
-	  MPC8560, MPC8540, MPC8548
+	  MPC8349, MPC8560, MPC8540, MPC8548
 
 config EDAC_MV64X60
 	tristate "Marvell MV64x60"

diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
index b4f5c63..3b05130 100644
--- a/drivers/edac/mpc85xx_edac.c
+++ b/drivers/edac/mpc85xx_edac.c

@@ -43,7 +43,9 @@ static u32 orig_pci_err_en;
 #endif
 
 static u32 orig_l2_err_disable;
+#ifdef CONFIG_MPC85xx
 static u32 orig_hid1[2];
+#endif
 
 /************************ MC SYSFS parts ***********************************/
 

@@ -738,7 +740,8 @@ static irqreturn_t mpc85xx_mc_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static void __devinit mpc85xx_init_csrows(struct mem_ctl_info *mci)
+static void __devinit mpc85xx_init_csrows(struct mem_ctl_info *mci,
+					  const struct of_device_id *match)
 {
 	struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
 	struct csrow_info *csrow;

@@ -784,18 +787,26 @@ static void __devinit mpc85xx_init_csrows(struct mem_ctl_info *mci)
 	}
 
 	for (index = 0; index < mci->nr_csrows; index++) {
-		u32 start;
-		u32 end;
+		u32 start, end, extra;
 
 		csrow = &mci->csrows[index];
 		cs_bnds = in_be32(pdata->mc_vbase + MPC85XX_MC_CS_BNDS_0 +
 				  (index * MPC85XX_MC_CS_BNDS_OFS));
-		start = (cs_bnds & 0xfff0000) << 4;
-		end = ((cs_bnds & 0xfff) << 20);
+
+		if (match->data && match->data == MPC83xx) {
+			start = (cs_bnds & 0x00ff0000) << 8;
+			end   = (cs_bnds & 0x000000ff) << 24;
+			extra = 0x00ffffff;
+		} else {
+			start = (cs_bnds & 0x0fff0000) << 4;
+			end   = (cs_bnds & 0x00000fff) << 20;
+			extra = 0x000fffff;
+		}
+
 		if (start)
-			start |= 0xfffff;
+			start |= extra;
 		if (end)
-			end |= 0xfffff;
+			end |= extra;
 
 		if (start == end)
 			continue;	/* not populated */

@@ -886,7 +897,7 @@ static int __devinit mpc85xx_mc_err_probe(struct of_device *op,
 
 	mpc85xx_set_mc_sysfs_attributes(mci);
 
-	mpc85xx_init_csrows(mci);
+	mpc85xx_init_csrows(mci, match);
 
 #ifdef CONFIG_EDAC_DEBUG
 	edac_mc_register_mcidev_debug((struct attribute **)debug_attr);

@@ -986,6 +997,7 @@ static struct of_device_id mpc85xx_mc_err_of_match[] = {
 	{ .compatible = "fsl,mpc8560-memory-controller", },
 	{ .compatible = "fsl,mpc8568-memory-controller", },
 	{ .compatible = "fsl,mpc8572-memory-controller", },
+	{ .compatible = "fsl,mpc8349-memory-controller", .data = MPC83xx, },
 	{},
 };
 

@@ -1001,13 +1013,13 @@ static struct of_platform_driver mpc85xx_mc_err_driver = {
 		   },
 };
 
-
+#ifdef CONFIG_MPC85xx
 static void __init mpc85xx_mc_clear_rfxe(void *data)
 {
 	orig_hid1[smp_processor_id()] = mfspr(SPRN_HID1);
 	mtspr(SPRN_HID1, (orig_hid1[smp_processor_id()] & ~0x20000));
 }
-
+#endif
 
 static int __init mpc85xx_mc_init(void)
 {

@@ -1040,26 +1052,32 @@ static int __init mpc85xx_mc_init(void)
 		printk(KERN_WARNING EDAC_MOD_STR "PCI fails to register\n");
 #endif
 
+#ifdef CONFIG_MPC85xx
 	/*
 	 * need to clear HID1[RFXE] to disable machine check int
 	 * so we can catch it
 	 */
 	if (edac_op_state == EDAC_OPSTATE_INT)
 		on_each_cpu(mpc85xx_mc_clear_rfxe, NULL, 0);
+#endif
 
 	return 0;
 }
 
 module_init(mpc85xx_mc_init);
 
+#ifdef CONFIG_MPC85xx
 static void __exit mpc85xx_mc_restore_hid1(void *data)
 {
 	mtspr(SPRN_HID1, orig_hid1[smp_processor_id()]);
 }
+#endif
 
 static void __exit mpc85xx_mc_exit(void)
 {
+#ifdef CONFIG_MPC85xx
 	on_each_cpu(mpc85xx_mc_restore_hid1, NULL, 0);
+#endif
 #ifdef CONFIG_PCI
 	of_unregister_platform_driver(&mpc85xx_pci_err_driver);
 #endif

diff --git a/drivers/edac/mpc85xx_edac.h b/drivers/edac/mpc85xx_edac.h
index 52432ee..fb166d0 100644
--- a/drivers/edac/mpc85xx_edac.h
+++ b/drivers/edac/mpc85xx_edac.h

@@ -139,6 +139,9 @@
 #define MPC85XX_PCI_GAS_TIMR		0x0020
 #define MPC85XX_PCI_PCIX_TIMR		0x0024
 
+/* Device types */
+#define MPC83xx ((void *)1)
+
 struct mpc85xx_mc_pdata {
 	char *name;
 	int edac_idx;

-- 
1.5.4.3

On Jul 8, 2009, at 11:19 AM, Ira W. Snyder wrote:

Add support for the Freescale MPC83xx memory controller to the  
existing
driver for the Freescale MPC85xx memory controller. The only  
difference
between the two processors are in the CS_BNDS register parsing code.

The L2 cache controller does not exist on the MPC83xx, but the OF  
subsystem
will not use the driver if the device is not present in the OF  
device tree.

Signed-off-by: Ira W. Snyder <redacted>
---

This was tested on an MPC8349EMDS-based board.

I don't really like how the MCE disable works on mpc85xx (clearing the
HID1 register), but this can be revisited when mpc86xx support gets
added. It sucks to have this happen before the probe() routine is  
called
and we know what kind of hardware we're actually running on.

drivers/edac/Kconfig        |    6 +++---
drivers/edac/mpc85xx_edac.c |   38 +++++++++++++++++++++++++++ 
+----------
drivers/edac/mpc85xx_edac.h |    3 +++
3 files changed, 34 insertions(+), 13 deletions(-)

[snip]

I don't understand this at all.. The only difference between 83xx &  
85xx is that we should have an extra 4-bits for 36-bit physical  
addresses.

We should be able to write this code in such a way that it works for  
both 83xx & 85xx.

                 start = (cs_bnds & 0xffff0000) >> 16;
                 end = (cs_bnds & 0xffff);

		if (start == end)
			continue;
		start <<= (20 - PAGE_SHIFT);
		end <<= (20 - PAGE_SHIFT);
		end |= (1 << (20 - PAGE_SHIFT)) - 1;

- k

On Wed, Jul 08, 2009 at 01:09:39PM -0500, Kumar Gala wrote:

On Jul 8, 2009, at 11:19 AM, Ira W. Snyder wrote:

quoted

Add support for the Freescale MPC83xx memory controller to the  
existing
driver for the Freescale MPC85xx memory controller. The only  
difference
between the two processors are in the CS_BNDS register parsing code.

The L2 cache controller does not exist on the MPC83xx, but the OF  
subsystem
will not use the driver if the device is not present in the OF device 
tree.

Signed-off-by: Ira W. Snyder <redacted>
---

This was tested on an MPC8349EMDS-based board.

I don't really like how the MCE disable works on mpc85xx (clearing the
HID1 register), but this can be revisited when mpc86xx support gets
added. It sucks to have this happen before the probe() routine is  
called
and we know what kind of hardware we're actually running on.

drivers/edac/Kconfig        |    6 +++---
drivers/edac/mpc85xx_edac.c |   38 +++++++++++++++++++++++++++ 
+----------
drivers/edac/mpc85xx_edac.h |    3 +++
3 files changed, 34 insertions(+), 13 deletions(-)

[snip]

quoted

/************************ MC SYSFS parts  
***********************************/

@@ -738,7 +740,8 @@ static irqreturn_t mpc85xx_mc_isr(int irq, void  

*dev_id)
	return IRQ_HANDLED;
}

-static void __devinit mpc85xx_init_csrows(struct mem_ctl_info *mci)
+static void __devinit mpc85xx_init_csrows(struct mem_ctl_info *mci,
+					  const struct of_device_id *match)
{
	struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
	struct csrow_info *csrow;

@@ -784,18 +787,26 @@ static void __devinit mpc85xx_init_csrows(struct 

mem_ctl_info *mci)
	}

	for (index = 0; index < mci->nr_csrows; index++) {
-		u32 start;
-		u32 end;
+		u32 start, end, extra;

		csrow = &mci->csrows[index];
		cs_bnds = in_be32(pdata->mc_vbase + MPC85XX_MC_CS_BNDS_0 +
				  (index * MPC85XX_MC_CS_BNDS_OFS));
-		start = (cs_bnds & 0xfff0000) << 4;
-		end = ((cs_bnds & 0xfff) << 20);
+
+		if (match->data && match->data == MPC83xx) {
+			start = (cs_bnds & 0x00ff0000) << 8;
+			end   = (cs_bnds & 0x000000ff) << 24;
+			extra = 0x00ffffff;
+		} else {
+			start = (cs_bnds & 0x0fff0000) << 4;
+			end   = (cs_bnds & 0x00000fff) << 20;
+			extra = 0x000fffff;
+		}

I don't understand this at all.. The only difference between 83xx & 85xx 
is that we should have an extra 4-bits for 36-bit physical addresses.

We should be able to write this code in such a way that it works for  
both 83xx & 85xx.

                start = (cs_bnds & 0xffff0000) >> 16;
                end = (cs_bnds & 0xffff);

		if (start == end)
			continue;
		start <<= (20 - PAGE_SHIFT);
		end <<= (20 - PAGE_SHIFT);
		end |= (1 << (20 - PAGE_SHIFT)) - 1;

Sorry, I don't know a thing about PAGE_SHIFT. Looking in
arch/powerpc/platforms/Kconfig.cputype, PPC_85xx doesn't seem to imply a
change in PAGE_SIZE, which changes the PAGE_SHIFT in
arch/powerpc/include/asm/page.h.

Also, are you sure you cannot use 4K pages on an 85xx? If you can use 4K
pages on 85xx, then PAGE_SHIFT == 12, and your solution breaks.

I admit I don't know much about all of the different powerpc platforms.

Ira

On Jul 8, 2009, at 2:33 PM, Ira W. Snyder wrote:

On Wed, Jul 08, 2009 at 01:09:39PM -0500, Kumar Gala wrote:

quoted

On Jul 8, 2009, at 11:19 AM, Ira W. Snyder wrote:

quoted

Add support for the Freescale MPC83xx memory controller to the
existing
driver for the Freescale MPC85xx memory controller. The only
difference
between the two processors are in the CS_BNDS register parsing code.

The L2 cache controller does not exist on the MPC83xx, but the OF
subsystem
will not use the driver if the device is not present in the OF  
device
tree.

Signed-off-by: Ira W. Snyder <redacted>
---

This was tested on an MPC8349EMDS-based board.

I don't really like how the MCE disable works on mpc85xx (clearing  
the
HID1 register), but this can be revisited when mpc86xx support gets
added. It sucks to have this happen before the probe() routine is
called
and we know what kind of hardware we're actually running on.

drivers/edac/Kconfig        |    6 +++---
drivers/edac/mpc85xx_edac.c |   38 +++++++++++++++++++++++++++
+----------
drivers/edac/mpc85xx_edac.h |    3 +++
3 files changed, 34 insertions(+), 13 deletions(-)

[snip]

quoted

/************************ MC SYSFS parts
***********************************/

@@ -738,7 +740,8 @@ static irqreturn_t mpc85xx_mc_isr(int irq, void

*dev_id)
	return IRQ_HANDLED;
}

-static void __devinit mpc85xx_init_csrows(struct mem_ctl_info *mci)
+static void __devinit mpc85xx_init_csrows(struct mem_ctl_info *mci,
+					  const struct of_device_id *match)
{
	struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
	struct csrow_info *csrow;

@@ -784,18 +787,26 @@ static void __devinit  

mpc85xx_init_csrows(struct
mem_ctl_info *mci)
	}

	for (index = 0; index < mci->nr_csrows; index++) {
-		u32 start;
-		u32 end;
+		u32 start, end, extra;

		csrow = &mci->csrows[index];
		cs_bnds = in_be32(pdata->mc_vbase + MPC85XX_MC_CS_BNDS_0 +
				  (index * MPC85XX_MC_CS_BNDS_OFS));
-		start = (cs_bnds & 0xfff0000) << 4;
-		end = ((cs_bnds & 0xfff) << 20);
+
+		if (match->data && match->data == MPC83xx) {
+			start = (cs_bnds & 0x00ff0000) << 8;
+			end   = (cs_bnds & 0x000000ff) << 24;
+			extra = 0x00ffffff;
+		} else {
+			start = (cs_bnds & 0x0fff0000) << 4;
+			end   = (cs_bnds & 0x00000fff) << 20;
+			extra = 0x000fffff;
+		}

I don't understand this at all.. The only difference between 83xx &  
85xx
is that we should have an extra 4-bits for 36-bit physical addresses.

We should be able to write this code in such a way that it works for
both 83xx & 85xx.

               start = (cs_bnds & 0xffff0000) >> 16;
               end = (cs_bnds & 0xffff);

		if (start == end)
			continue;
		start <<= (20 - PAGE_SHIFT);
		end <<= (20 - PAGE_SHIFT);
		end |= (1 << (20 - PAGE_SHIFT)) - 1;

Sorry, I don't know a thing about PAGE_SHIFT. Looking in
arch/powerpc/platforms/Kconfig.cputype, PPC_85xx doesn't seem to  
imply a
change in PAGE_SIZE, which changes the PAGE_SHIFT in
arch/powerpc/include/asm/page.h.

Also, are you sure you cannot use 4K pages on an 85xx? If you can  
use 4K
pages on 85xx, then PAGE_SHIFT == 12, and your solution breaks.

I admit I don't know much about all of the different powerpc  
platforms.

PAGE_SHIFT is the same.. I was folding in the code from below the diff:

                 csrow->first_page = start >> PAGE_SHIFT;
                 csrow->last_page = end >> PAGE_SHIFT;

into the calculation of start/end.  I know PAGE_SHIFT is a minimum 12  
on any PPC based system (and at this point its always 12 on 85xx/ 
83xx).  The idea is to ensure that when shift start/end up that it  
still fits in 32-bits.  This is to handle the case of >4G of memory.

- k

On Wed, Jul 08, 2009 at 03:58:02PM -0500, Kumar Gala wrote:

On Jul 8, 2009, at 2:33 PM, Ira W. Snyder wrote:

quoted

On Wed, Jul 08, 2009 at 01:09:39PM -0500, Kumar Gala wrote:

quoted

On Jul 8, 2009, at 11:19 AM, Ira W. Snyder wrote:

quoted

Add support for the Freescale MPC83xx memory controller to the
existing
driver for the Freescale MPC85xx memory controller. The only
difference
between the two processors are in the CS_BNDS register parsing code.

The L2 cache controller does not exist on the MPC83xx, but the OF
subsystem
will not use the driver if the device is not present in the OF  
device
tree.

Signed-off-by: Ira W. Snyder <redacted>
---

This was tested on an MPC8349EMDS-based board.

I don't really like how the MCE disable works on mpc85xx (clearing  
the
HID1 register), but this can be revisited when mpc86xx support gets
added. It sucks to have this happen before the probe() routine is
called
and we know what kind of hardware we're actually running on.

drivers/edac/Kconfig        |    6 +++---
drivers/edac/mpc85xx_edac.c |   38 +++++++++++++++++++++++++++
+----------
drivers/edac/mpc85xx_edac.h |    3 +++
3 files changed, 34 insertions(+), 13 deletions(-)

[snip]

quoted

/************************ MC SYSFS parts
***********************************/

@@ -738,7 +740,8 @@ static irqreturn_t mpc85xx_mc_isr(int irq, void

*dev_id)
	return IRQ_HANDLED;
}

-static void __devinit mpc85xx_init_csrows(struct mem_ctl_info *mci)
+static void __devinit mpc85xx_init_csrows(struct mem_ctl_info *mci,
+					  const struct of_device_id *match)
{
	struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
	struct csrow_info *csrow;

@@ -784,18 +787,26 @@ static void __devinit  

mpc85xx_init_csrows(struct
mem_ctl_info *mci)
	}

	for (index = 0; index < mci->nr_csrows; index++) {
-		u32 start;
-		u32 end;
+		u32 start, end, extra;

		csrow = &mci->csrows[index];
		cs_bnds = in_be32(pdata->mc_vbase + MPC85XX_MC_CS_BNDS_0 +
				  (index * MPC85XX_MC_CS_BNDS_OFS));
-		start = (cs_bnds & 0xfff0000) << 4;
-		end = ((cs_bnds & 0xfff) << 20);
+
+		if (match->data && match->data == MPC83xx) {
+			start = (cs_bnds & 0x00ff0000) << 8;
+			end   = (cs_bnds & 0x000000ff) << 24;
+			extra = 0x00ffffff;
+		} else {
+			start = (cs_bnds & 0x0fff0000) << 4;
+			end   = (cs_bnds & 0x00000fff) << 20;
+			extra = 0x000fffff;
+		}

I don't understand this at all.. The only difference between 83xx &  
85xx
is that we should have an extra 4-bits for 36-bit physical addresses.

We should be able to write this code in such a way that it works for
both 83xx & 85xx.

               start = (cs_bnds & 0xffff0000) >> 16;
               end = (cs_bnds & 0xffff);

		if (start == end)
			continue;
		start <<= (20 - PAGE_SHIFT);
		end <<= (20 - PAGE_SHIFT);
		end |= (1 << (20 - PAGE_SHIFT)) - 1;

Sorry, I don't know a thing about PAGE_SHIFT. Looking in
arch/powerpc/platforms/Kconfig.cputype, PPC_85xx doesn't seem to imply 
a
change in PAGE_SIZE, which changes the PAGE_SHIFT in
arch/powerpc/include/asm/page.h.

Also, are you sure you cannot use 4K pages on an 85xx? If you can use 
4K
pages on 85xx, then PAGE_SHIFT == 12, and your solution breaks.

I admit I don't know much about all of the different powerpc  
platforms.

PAGE_SHIFT is the same.. I was folding in the code from below the diff:

                csrow->first_page = start >> PAGE_SHIFT;
                csrow->last_page = end >> PAGE_SHIFT;

into the calculation of start/end.  I know PAGE_SHIFT is a minimum 12 on 
any PPC based system (and at this point its always 12 on 85xx/83xx).  The 
idea is to ensure that when shift start/end up that it still fits in 
32-bits.  This is to handle the case of >4G of memory.

Hello Kumar,

I must not understand something going on here. Your proposed code
doesn't work at all on my board. The
/sys/devices/system/edac/mc/mc0/size_mb doesn't come out correctly.

The attached patch DOES work on my board, but I'm confident that it does
NOT work on a system with PAGE_SIZE != 4096. Any idea what I did wrong?

If I'm reading things correctly:
csrow->first_page	full address of the first page (NOT pfn)
csrow->last_page	full address of the last  page (NOT pfn)
csrow->nr_pages		number of pages

The EDAC subsystem does csrow->nr_pages * PAGE_SIZE to get the size_mb
sysfs value.

If csrow->first_page and csrow->last_page ARE supposed to be the pfn,
then I think the original code got it wrong, and the calculation for
csrow->nr_pages needs to be changed.

Thanks,
Ira

From a442c2e5a1cfbe29ea9d2ea7d45237677ff7899d Mon Sep 17 00:00:00 2001

From: Ira W. Snyder <redacted>
Date: Wed, 8 Jul 2009 08:59:16 -0700
Subject: [PATCH] edac: mpc85xx: add support for mpc83xx memory controller

Add support for the Freescale MPC83xx memory controller to the existing
driver for the Freescale MPC85xx memory controller. The only difference
between the two processors are in the CS_BNDS register parsing code, which
has been changed so it will work on both processors.

The L2 cache controller does not exist on the MPC83xx, but the OF subsystem
will not use the driver if the device is not present in the OF device tree.

Signed-off-by: Ira W. Snyder <redacted>
---
 drivers/edac/Kconfig        |    6 +++---
 drivers/edac/mpc85xx_edac.c |   30 ++++++++++++++++++++----------
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 4339b1a..78303f9 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig

@@ -176,11 +176,11 @@ config EDAC_I5100
 	  San Clemente MCH.
 
 config EDAC_MPC85XX
-	tristate "Freescale MPC85xx"
-	depends on EDAC_MM_EDAC && FSL_SOC && MPC85xx
+	tristate "Freescale MPC83xx / MPC85xx"
+	depends on EDAC_MM_EDAC && FSL_SOC && (PPC_83xx || MPC85xx)
 	help
 	  Support for error detection and correction on the Freescale
-	  MPC8560, MPC8540, MPC8548
+	  MPC8349, MPC8560, MPC8540, MPC8548
 
 config EDAC_MV64X60
 	tristate "Marvell MV64x60"

diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
index b4f5c63..e4832b4 100644
--- a/drivers/edac/mpc85xx_edac.c
+++ b/drivers/edac/mpc85xx_edac.c

@@ -43,7 +43,9 @@ static u32 orig_pci_err_en;
 #endif
 
 static u32 orig_l2_err_disable;
+#ifdef CONFIG_MPC85xx
 static u32 orig_hid1[2];
+#endif
 
 /************************ MC SYSFS parts ***********************************/
 

@@ -790,18 +792,19 @@ static void __devinit mpc85xx_init_csrows(struct mem_ctl_info *mci)
 		csrow = &mci->csrows[index];
 		cs_bnds = in_be32(pdata->mc_vbase + MPC85XX_MC_CS_BNDS_0 +
 				  (index * MPC85XX_MC_CS_BNDS_OFS));
-		start = (cs_bnds & 0xfff0000) << 4;
-		end = ((cs_bnds & 0xfff) << 20);
-		if (start)
-			start |= 0xfffff;
-		if (end)
-			end |= 0xfffff;
+
+		start = (cs_bnds & 0xffff0000) >> 16;
+		end   = (cs_bnds & 0x0000ffff);
 
 		if (start == end)
 			continue;	/* not populated */
 
-		csrow->first_page = start >> PAGE_SHIFT;
-		csrow->last_page = end >> PAGE_SHIFT;
+		start <<= PAGE_SHIFT;
+		end   <<= PAGE_SHIFT;
+		end    |= (1 << PAGE_SHIFT) - 1;
+
+		csrow->first_page = start;
+		csrow->last_page = end;
 		csrow->nr_pages = csrow->last_page + 1 - csrow->first_page;
 		csrow->grain = 8;
 		csrow->mtype = mtype;

@@ -986,6 +989,7 @@ static struct of_device_id mpc85xx_mc_err_of_match[] = {
 	{ .compatible = "fsl,mpc8560-memory-controller", },
 	{ .compatible = "fsl,mpc8568-memory-controller", },
 	{ .compatible = "fsl,mpc8572-memory-controller", },
+	{ .compatible = "fsl,mpc8349-memory-controller", },
 	{},
 };
 

@@ -1001,13 +1005,13 @@ static struct of_platform_driver mpc85xx_mc_err_driver = {
 		   },
 };
 
-
+#ifdef CONFIG_MPC85xx
 static void __init mpc85xx_mc_clear_rfxe(void *data)
 {
 	orig_hid1[smp_processor_id()] = mfspr(SPRN_HID1);
 	mtspr(SPRN_HID1, (orig_hid1[smp_processor_id()] & ~0x20000));
 }
-
+#endif
 
 static int __init mpc85xx_mc_init(void)
 {

@@ -1040,26 +1044,32 @@ static int __init mpc85xx_mc_init(void)
 		printk(KERN_WARNING EDAC_MOD_STR "PCI fails to register\n");
 #endif
 
+#ifdef CONFIG_MPC85xx
 	/*
 	 * need to clear HID1[RFXE] to disable machine check int
 	 * so we can catch it
 	 */
 	if (edac_op_state == EDAC_OPSTATE_INT)
 		on_each_cpu(mpc85xx_mc_clear_rfxe, NULL, 0);
+#endif
 
 	return 0;
 }
 
 module_init(mpc85xx_mc_init);
 
+#ifdef CONFIG_MPC85xx
 static void __exit mpc85xx_mc_restore_hid1(void *data)
 {
 	mtspr(SPRN_HID1, orig_hid1[smp_processor_id()]);
 }
+#endif
 
 static void __exit mpc85xx_mc_exit(void)
 {
+#ifdef CONFIG_MPC85xx
 	on_each_cpu(mpc85xx_mc_restore_hid1, NULL, 0);
+#endif
 #ifdef CONFIG_PCI
 	of_unregister_platform_driver(&mpc85xx_pci_err_driver);
 #endif

-- 
1.5.4.3

Hello Kumar,

I must not understand something going on here. Your proposed code
doesn't work at all on my board. The
/sys/devices/system/edac/mc/mc0/size_mb doesn't come out correctly.

What does it come out as?  How much memory do you have in the system?

The attached patch DOES work on my board, but I'm confident that it  
does
NOT work on a system with PAGE_SIZE != 4096. Any idea what I did  
wrong?

If I'm reading things correctly:
csrow->first_page	full address of the first page (NOT pfn)
csrow->last_page	full address of the last  page (NOT pfn)
csrow->nr_pages		number of pages

The EDAC subsystem does csrow->nr_pages * PAGE_SIZE to get the size_mb
sysfs value.

If csrow->first_page and csrow->last_page ARE supposed to be the pfn,
then I think the original code got it wrong, and the calculation for
csrow->nr_pages needs to be changed.

Thanks,
Ira

[snip]

can you printk what cs_bnds values are in your setup.

+
+		start = (cs_bnds & 0xffff0000) >> 16;
+		end   = (cs_bnds & 0x0000ffff);

		if (start == end)
			continue;	/* not populated */

-		csrow->first_page = start >> PAGE_SHIFT;
-		csrow->last_page = end >> PAGE_SHIFT;
+		start <<= PAGE_SHIFT;
+		end   <<= PAGE_SHIFT;
+		end    |= (1 << PAGE_SHIFT) - 1;
+
+		csrow->first_page = start;
+		csrow->last_page = end;

This seems odd to me... I can't believe this is working out properly.

		csrow->nr_pages = csrow->last_page + 1 - csrow->first_page;
		csrow->grain = 8;
		csrow->mtype = mtype;

Lets get some real values on the table for your system so I can get a  
sense of what's really going on.

- k

On Thu, Jul 09, 2009 at 12:58:53PM -0500, Kumar Gala wrote:

quoted

Hello Kumar,

I must not understand something going on here. Your proposed code
doesn't work at all on my board. The
/sys/devices/system/edac/mc/mc0/size_mb doesn't come out correctly.

What does it come out as?  How much memory do you have in the system?

The size_mb shows as 0 with your code. See the explanation below. With
my code it shows as 256MB, as expected.

I have 256MB memory in the system.

quoted

The attached patch DOES work on my board, but I'm confident that it  
does
NOT work on a system with PAGE_SIZE != 4096. Any idea what I did  
wrong?

If I'm reading things correctly:
csrow->first_page	full address of the first page (NOT pfn)
csrow->last_page	full address of the last  page (NOT pfn)
csrow->nr_pages		number of pages

The EDAC subsystem does csrow->nr_pages * PAGE_SIZE to get the size_mb
sysfs value.

If csrow->first_page and csrow->last_page ARE supposed to be the pfn,
then I think the original code got it wrong, and the calculation for
csrow->nr_pages needs to be changed.

Thanks,
Ira

[snip]

quoted

/************************ MC SYSFS parts  
***********************************/

@@ -790,18 +792,19 @@ static void __devinit mpc85xx_init_csrows(struct 

mem_ctl_info *mci)
		csrow = &mci->csrows[index];
		cs_bnds = in_be32(pdata->mc_vbase + MPC85XX_MC_CS_BNDS_0 +
				  (index * MPC85XX_MC_CS_BNDS_OFS));
-		start = (cs_bnds & 0xfff0000) << 4;
-		end = ((cs_bnds & 0xfff) << 20);
-		if (start)
-			start |= 0xfffff;
-		if (end)
-			end |= 0xfffff;

can you printk what cs_bnds values are in your setup.

I am only using a single chip select. CS0_BNDS (register 0xe0002000) is
0x0000000F.

quoted

+
+		start = (cs_bnds & 0xffff0000) >> 16;
+		end   = (cs_bnds & 0x0000ffff);

This is the same in both our versions.

start == 0x0
end   == 0xF

quoted

		if (start == end)
			continue;	/* not populated */

-		csrow->first_page = start >> PAGE_SHIFT;
-		csrow->last_page = end >> PAGE_SHIFT;
+		start <<= PAGE_SHIFT;
+		end   <<= PAGE_SHIFT;
+		end    |= (1 << PAGE_SHIFT) - 1;
+

MY VERSION

start == 0x0
end   == 0xffff

first_page == 0x0
last_page  == 0xffff

YOUR VERSION (<<= (20 - PAGE_SHIFT), etc.)

start == 0x0
end   == 0xfff

first_page == 0x0
last_page  == 0x0

quoted

+		csrow->first_page = start;
+		csrow->last_page = end;

This seems odd to me... I can't believe this is working out properly.

quoted

		csrow->nr_pages = csrow->last_page + 1 - csrow->first_page;

The calculation is unchanged here from the original code. Due to the
">> PAGE_SHIFT", nr_pages ends up as 1 in your version.

MY VERSION

nr_pages == 0xffff + 1 - 0 == 0x10000

0x10000 * 4096 / 1024 / 1024 == 256 MB


YOUR VERSION

nr_pages == 0x0 + 1 - 0x0 == 1

0x1 * 4096 / 1024 / 1024 == 0MB

quoted

		csrow->grain = 8;
		csrow->mtype = mtype;

Lets get some real values on the table for your system so I can get a  
sense of what's really going on.

Thanks for the help.
Ira

On Jul 9, 2009, at 1:17 PM, Ira W. Snyder wrote:

On Thu, Jul 09, 2009 at 12:58:53PM -0500, Kumar Gala wrote:

quoted

quoted

Hello Kumar,

I must not understand something going on here. Your proposed code
doesn't work at all on my board. The
/sys/devices/system/edac/mc/mc0/size_mb doesn't come out correctly.

What does it come out as?  How much memory do you have in the system?

The size_mb shows as 0 with your code. See the explanation below. With
my code it shows as 256MB, as expected.

I have 256MB memory in the system.

quoted

quoted

The attached patch DOES work on my board, but I'm confident that it
does
NOT work on a system with PAGE_SIZE != 4096. Any idea what I did
wrong?

If I'm reading things correctly:
csrow->first_page	full address of the first page (NOT pfn)
csrow->last_page	full address of the last  page (NOT pfn)
csrow->nr_pages		number of pages

The EDAC subsystem does csrow->nr_pages * PAGE_SIZE to get the  
size_mb
sysfs value.

If csrow->first_page and csrow->last_page ARE supposed to be the  
pfn,
then I think the original code got it wrong, and the calculation for
csrow->nr_pages needs to be changed.

Thanks,
Ira

[snip]

quoted

/************************ MC SYSFS parts
***********************************/

@@ -790,18 +792,19 @@ static void __devinit  

mpc85xx_init_csrows(struct
mem_ctl_info *mci)
		csrow = &mci->csrows[index];
		cs_bnds = in_be32(pdata->mc_vbase + MPC85XX_MC_CS_BNDS_0 +
				  (index * MPC85XX_MC_CS_BNDS_OFS));
-		start = (cs_bnds & 0xfff0000) << 4;
-		end = ((cs_bnds & 0xfff) << 20);
-		if (start)
-			start |= 0xfffff;
-		if (end)
-			end |= 0xfffff;

can you printk what cs_bnds values are in your setup.

I am only using a single chip select. CS0_BNDS (register 0xe0002000)  
is
0x0000000F.

quoted

quoted

+
+		start = (cs_bnds & 0xffff0000) >> 16;
+		end   = (cs_bnds & 0x0000ffff);

This is the same in both our versions.

start == 0x0
end   == 0xF

quoted

quoted

		if (start == end)
			continue;	/* not populated */

-		csrow->first_page = start >> PAGE_SHIFT;
-		csrow->last_page = end >> PAGE_SHIFT;
+		start <<= PAGE_SHIFT;
+		end   <<= PAGE_SHIFT;
+		end    |= (1 << PAGE_SHIFT) - 1;
+

MY VERSION

start == 0x0
end   == 0xffff

first_page == 0x0
last_page  == 0xffff

YOUR VERSION (<<= (20 - PAGE_SHIFT), etc.)

My math was wrong it should be ( <<= (24 - PAGE_SHIFT) )

With that I think things work out.

- k

start == 0x0
end   == 0xfff

first_page == 0x0
last_page  == 0x0

quoted

quoted

+		csrow->first_page = start;
+		csrow->last_page = end;

This seems odd to me... I can't believe this is working out properly.

quoted

		csrow->nr_pages = csrow->last_page + 1 - csrow->first_page;

The calculation is unchanged here from the original code. Due to the
">> PAGE_SHIFT", nr_pages ends up as 1 in your version.

MY VERSION

nr_pages == 0xffff + 1 - 0 == 0x10000

0x10000 * 4096 / 1024 / 1024 == 256 MB


YOUR VERSION

nr_pages == 0x0 + 1 - 0x0 == 1

0x1 * 4096 / 1024 / 1024 == 0MB

quoted

quoted

		csrow->grain = 8;
		csrow->mtype = mtype;

Lets get some real values on the table for your system so I can get a
sense of what's really going on.

Thanks for the help.
Ira

On Thu, Jul 09, 2009 at 01:25:43PM -0500, Kumar Gala wrote:

On Jul 9, 2009, at 1:17 PM, Ira W. Snyder wrote:

quoted

On Thu, Jul 09, 2009 at 12:58:53PM -0500, Kumar Gala wrote:

quoted

quoted

Hello Kumar,

I must not understand something going on here. Your proposed code
doesn't work at all on my board. The
/sys/devices/system/edac/mc/mc0/size_mb doesn't come out correctly.

What does it come out as?  How much memory do you have in the system?

The size_mb shows as 0 with your code. See the explanation below. With
my code it shows as 256MB, as expected.

I have 256MB memory in the system.

quoted

quoted

The attached patch DOES work on my board, but I'm confident that it
does
NOT work on a system with PAGE_SIZE != 4096. Any idea what I did
wrong?

If I'm reading things correctly:
csrow->first_page	full address of the first page (NOT pfn)
csrow->last_page	full address of the last  page (NOT pfn)
csrow->nr_pages		number of pages

The EDAC subsystem does csrow->nr_pages * PAGE_SIZE to get the  
size_mb
sysfs value.

If csrow->first_page and csrow->last_page ARE supposed to be the  
pfn,
then I think the original code got it wrong, and the calculation for
csrow->nr_pages needs to be changed.

Thanks,
Ira

[snip]

quoted

/************************ MC SYSFS parts
***********************************/

@@ -790,18 +792,19 @@ static void __devinit  

mpc85xx_init_csrows(struct
mem_ctl_info *mci)
		csrow = &mci->csrows[index];
		cs_bnds = in_be32(pdata->mc_vbase + MPC85XX_MC_CS_BNDS_0 +
				  (index * MPC85XX_MC_CS_BNDS_OFS));
-		start = (cs_bnds & 0xfff0000) << 4;
-		end = ((cs_bnds & 0xfff) << 20);
-		if (start)
-			start |= 0xfffff;
-		if (end)
-			end |= 0xfffff;

can you printk what cs_bnds values are in your setup.

I am only using a single chip select. CS0_BNDS (register 0xe0002000)  
is
0x0000000F.

quoted

quoted

+
+		start = (cs_bnds & 0xffff0000) >> 16;
+		end   = (cs_bnds & 0x0000ffff);

This is the same in both our versions.

start == 0x0
end   == 0xF

quoted

quoted

		if (start == end)
			continue;	/* not populated */

-		csrow->first_page = start >> PAGE_SHIFT;
-		csrow->last_page = end >> PAGE_SHIFT;
+		start <<= PAGE_SHIFT;
+		end   <<= PAGE_SHIFT;
+		end    |= (1 << PAGE_SHIFT) - 1;
+

MY VERSION

start == 0x0
end   == 0xffff

first_page == 0x0
last_page  == 0xffff

YOUR VERSION (<<= (20 - PAGE_SHIFT), etc.)

My math was wrong it should be ( <<= (24 - PAGE_SHIFT) )

With that I think things work out.

Yep, that works out great. This solution is much better than my original
code. The 83xx doesn't need to be special-cased anymore.

I checked the math for a 85xx with 64GB of memory. Assuming it uses 64K
pages (PAGE_SHIFT == 16), then everything works out.

I'll submit a new patch now.

Thanks for the help,
Ira

On Jul 9, 2009, at 2:35 PM, Ira W. Snyder wrote:

On Thu, Jul 09, 2009 at 01:25:43PM -0500, Kumar Gala wrote:

quoted

On Jul 9, 2009, at 1:17 PM, Ira W. Snyder wrote:

quoted

On Thu, Jul 09, 2009 at 12:58:53PM -0500, Kumar Gala wrote:

quoted

quoted

Hello Kumar,

I must not understand something going on here. Your proposed code
doesn't work at all on my board. The
/sys/devices/system/edac/mc/mc0/size_mb doesn't come out  
correctly.

What does it come out as?  How much memory do you have in the  
system?

The size_mb shows as 0 with your code. See the explanation below.  
With
my code it shows as 256MB, as expected.

I have 256MB memory in the system.

quoted

quoted

The attached patch DOES work on my board, but I'm confident that  
it
does
NOT work on a system with PAGE_SIZE != 4096. Any idea what I did
wrong?

If I'm reading things correctly:
csrow->first_page	full address of the first page (NOT pfn)
csrow->last_page	full address of the last  page (NOT pfn)
csrow->nr_pages		number of pages

The EDAC subsystem does csrow->nr_pages * PAGE_SIZE to get the
size_mb
sysfs value.

If csrow->first_page and csrow->last_page ARE supposed to be the
pfn,
then I think the original code got it wrong, and the calculation  
for
csrow->nr_pages needs to be changed.

Thanks,
Ira

[snip]

quoted

/************************ MC SYSFS parts
***********************************/

@@ -790,18 +792,19 @@ static void __devinit

mpc85xx_init_csrows(struct
mem_ctl_info *mci)
		csrow = &mci->csrows[index];
		cs_bnds = in_be32(pdata->mc_vbase + MPC85XX_MC_CS_BNDS_0 +
				  (index * MPC85XX_MC_CS_BNDS_OFS));
-		start = (cs_bnds & 0xfff0000) << 4;
-		end = ((cs_bnds & 0xfff) << 20);
-		if (start)
-			start |= 0xfffff;
-		if (end)
-			end |= 0xfffff;

can you printk what cs_bnds values are in your setup.

I am only using a single chip select. CS0_BNDS (register 0xe0002000)
is
0x0000000F.

quoted

quoted

+
+		start = (cs_bnds & 0xffff0000) >> 16;
+		end   = (cs_bnds & 0x0000ffff);

This is the same in both our versions.

start == 0x0
end   == 0xF

quoted

quoted

		if (start == end)
			continue;	/* not populated */

-		csrow->first_page = start >> PAGE_SHIFT;
-		csrow->last_page = end >> PAGE_SHIFT;
+		start <<= PAGE_SHIFT;
+		end   <<= PAGE_SHIFT;
+		end    |= (1 << PAGE_SHIFT) - 1;
+

MY VERSION

start == 0x0
end   == 0xffff

first_page == 0x0
last_page  == 0xffff

YOUR VERSION (<<= (20 - PAGE_SHIFT), etc.)

My math was wrong it should be ( <<= (24 - PAGE_SHIFT) )

With that I think things work out.

Yep, that works out great. This solution is much better than my  
original
code. The 83xx doesn't need to be special-cased anymore.

I checked the math for a 85xx with 64GB of memory. Assuming it uses  
64K
pages (PAGE_SHIFT == 16), then everything works out.

Does the math work for a 64GB system w/4K pages?

- k

On Thu, Jul 09, 2009 at 03:15:29PM -0500, Kumar Gala wrote:

On Jul 9, 2009, at 2:35 PM, Ira W. Snyder wrote:

quoted

On Thu, Jul 09, 2009 at 01:25:43PM -0500, Kumar Gala wrote:

quoted

On Jul 9, 2009, at 1:17 PM, Ira W. Snyder wrote:

quoted

On Thu, Jul 09, 2009 at 12:58:53PM -0500, Kumar Gala wrote:

quoted

quoted

Hello Kumar,

I must not understand something going on here. Your proposed code
doesn't work at all on my board. The
/sys/devices/system/edac/mc/mc0/size_mb doesn't come out  
correctly.

What does it come out as?  How much memory do you have in the  
system?

The size_mb shows as 0 with your code. See the explanation below.  
With
my code it shows as 256MB, as expected.

I have 256MB memory in the system.

quoted

quoted

The attached patch DOES work on my board, but I'm confident 
that it
does
NOT work on a system with PAGE_SIZE != 4096. Any idea what I did
wrong?

If I'm reading things correctly:
csrow->first_page	full address of the first page (NOT pfn)
csrow->last_page	full address of the last  page (NOT pfn)
csrow->nr_pages		number of pages

The EDAC subsystem does csrow->nr_pages * PAGE_SIZE to get the
size_mb
sysfs value.

If csrow->first_page and csrow->last_page ARE supposed to be the
pfn,
then I think the original code got it wrong, and the 
calculation for
csrow->nr_pages needs to be changed.

Thanks,
Ira

[snip]

quoted

/************************ MC SYSFS parts
***********************************/

@@ -790,18 +792,19 @@ static void __devinit

mpc85xx_init_csrows(struct
mem_ctl_info *mci)
		csrow = &mci->csrows[index];
		cs_bnds = in_be32(pdata->mc_vbase + MPC85XX_MC_CS_BNDS_0 +
				  (index * MPC85XX_MC_CS_BNDS_OFS));
-		start = (cs_bnds & 0xfff0000) << 4;
-		end = ((cs_bnds & 0xfff) << 20);
-		if (start)
-			start |= 0xfffff;
-		if (end)
-			end |= 0xfffff;

can you printk what cs_bnds values are in your setup.

I am only using a single chip select. CS0_BNDS (register 0xe0002000)
is
0x0000000F.

quoted

quoted

+
+		start = (cs_bnds & 0xffff0000) >> 16;
+		end   = (cs_bnds & 0x0000ffff);

This is the same in both our versions.

start == 0x0
end   == 0xF

quoted

quoted

		if (start == end)
			continue;	/* not populated */

-		csrow->first_page = start >> PAGE_SHIFT;
-		csrow->last_page = end >> PAGE_SHIFT;
+		start <<= PAGE_SHIFT;
+		end   <<= PAGE_SHIFT;
+		end    |= (1 << PAGE_SHIFT) - 1;
+

MY VERSION

start == 0x0
end   == 0xffff

first_page == 0x0
last_page  == 0xffff

YOUR VERSION (<<= (20 - PAGE_SHIFT), etc.)

My math was wrong it should be ( <<= (24 - PAGE_SHIFT) )

With that I think things work out.

Yep, that works out great. This solution is much better than my  
original
code. The 83xx doesn't need to be special-cased anymore.

I checked the math for a 85xx with 64GB of memory. Assuming it uses  
64K
pages (PAGE_SHIFT == 16), then everything works out.

Does the math work for a 64GB system w/4K pages?

Yes, the math works there too. I just double-checked by hand. The v2
patch should work fine on both the 83xx and 85xx (both 4K and 64K
pages).

Thanks again for the help on this one.
Ira