From: Hitoshi Mitake <[email protected]>
I wrote a new module for Intel X38 chipset.
This chipset is very similar to Intel 3200 chipset,
but there are some different points,
so I copyed i3200_edac.c and modified.
This is a Intel's web page describing this chipset.
http://www.intel.com/Products/Desktop/Chipsets/X38/X38-overview.htm
I've tested this new module with broken memory,
and it seems working well.
This is a patch, please use.
Hitoshi
Signed-off-by: Hitoshi Mitake <[email protected]>
Signed-off-by: Doug Thompson <[email protected]>
---
Index: linux-2.6.27/drivers/edac/Kconfig
===================================================================
--- linux-2.6.27.orig/drivers/edac/Kconfig
+++ linux-2.6.27/drivers/edac/Kconfig
@@ -102,6 +102,13 @@ config EDAC_I3000
Support for error detection and correction on the Intel
3000 and 3010 server chipsets.
+config EDAC_X38
+ tristate "Intel X38"
+ depends on EDAC_MM_EDAC && PCI && X86
+ help
+ Support for error detection and correction on the Intel
+ X38 server chipsets.
+
config EDAC_I82860
tristate "Intel 82860"
depends on EDAC_MM_EDAC && PCI && X86_32
Index: linux-2.6.27/drivers/edac/Makefile
===================================================================
--- linux-2.6.27.orig/drivers/edac/Makefile
+++ linux-2.6.27/drivers/edac/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_EDAC_I82443BXGX) += i82443
obj-$(CONFIG_EDAC_I82875P) += i82875p_edac.o
obj-$(CONFIG_EDAC_I82975X) += i82975x_edac.o
obj-$(CONFIG_EDAC_I3000) += i3000_edac.o
+obj-$(CONFIG_EDAC_X38) += x38_edac.o
obj-$(CONFIG_EDAC_I82860) += i82860_edac.o
obj-$(CONFIG_EDAC_R82600) += r82600_edac.o
obj-$(CONFIG_EDAC_PASEMI) += pasemi_edac.o
Index: linux-2.6.27/drivers/edac/x38_edac.c
===================================================================
--- /dev/null
+++ linux-2.6.27/drivers/edac/x38_edac.c
@@ -0,0 +1,524 @@
+/*
+ * Intel X38 Memory Controller kernel module
+ * Copyright (C) 2008 Cluster Computing, Inc.
+ *
+ * This file may be distributed under the terms of the
+ * GNU General Public License.
+ *
+ * This file is based on i3200_edac.c
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/pci.h>
+#include <linux/pci_ids.h>
+#include <linux/slab.h>
+#include <linux/edac.h>
+#include "edac_core.h"
+
+#define X38_REVISION "1.1"
+
+#define EDAC_MOD_STR "x38_edac"
+
+#define PCI_DEVICE_ID_INTEL_X38_HB 0x29e0
+
+#define X38_RANKS 8
+#define X38_RANKS_PER_CHANNEL 4
+#define X38_CHANNELS 2
+
+/* Intel X38 register addresses - device 0 function 0 - DRAM Controller */
+
+#define X38_MCHBAR_LOW 0x48 /* MCH Memory Mapped Register BAR */
+#define X38_MCHBAR_HIGH 0x4b
+#define X38_MCHBAR_MASK 0xfffffc000ULL /* bits 35:14 */
+#define X38_MMR_WINDOW_SIZE 16384
+
+#define X38_TOM 0xa0 /* Top of Memory (16b)
+ *
+ * 15:10 reserved
+ * 9:0 total populated physical memory
+ */
+#define X38_TOM_MASK 0x3ff /* bits 9:0 */
+#define X38_TOM_SHIFT 26 /* 64MiB grain */
+
+#define X38_ERRSTS 0xc8 /* Error Status Register (16b)
+ *
+ * 15 reserved
+ * 14 Isochronous TBWRR Run Behind FIFO Full
+ * (ITCV)
+ * 13 Isochronous TBWRR Run Behind FIFO Put
+ * (ITSTV)
+ * 12 reserved
+ * 11 MCH Thermal Sensor Event
+ * for SMI/SCI/SERR (GTSE)
+ * 10 reserved
+ * 9 LOCK to non-DRAM Memory Flag (LCKF)
+ * 8 reserved
+ * 7 DRAM Throttle Flag (DTF)
+ * 6:2 reserved
+ * 1 Multi-bit DRAM ECC Error Flag (DMERR)
+ * 0 Single-bit DRAM ECC Error Flag (DSERR)
+ */
+#define X38_ERRSTS_UE 0x0002
+#define X38_ERRSTS_CE 0x0001
+#define X38_ERRSTS_BITS (X38_ERRSTS_UE | X38_ERRSTS_CE)
+
+
+/* Intel MMIO register space - device 0 function 0 - MMR space */
+
+#define X38_C0DRB 0x200 /* Channel 0 DRAM Rank Boundary (16b x 4)
+ *
+ * 15:10 reserved
+ * 9:0 Channel 0 DRAM Rank Boundary Address
+ */
+#define X38_C1DRB 0x600 /* Channel 1 DRAM Rank Boundary (16b x 4) */
+#define X38_DRB_MASK 0x3ff /* bits 9:0 */
+#define X38_DRB_SHIFT 26 /* 64MiB grain */
+
+#define X38_C0ECCERRLOG 0x280 /* Channel 0 ECC Error Log (64b)
+ *
+ * 63:48 Error Column Address (ERRCOL)
+ * 47:32 Error Row Address (ERRROW)
+ * 31:29 Error Bank Address (ERRBANK)
+ * 28:27 Error Rank Address (ERRRANK)
+ * 26:24 reserved
+ * 23:16 Error Syndrome (ERRSYND)
+ * 15: 2 reserved
+ * 1 Multiple Bit Error Status (MERRSTS)
+ * 0 Correctable Error Status (CERRSTS)
+ */
+#define X38_C1ECCERRLOG 0x680 /* Channel 1 ECC Error Log (64b) */
+#define X38_ECCERRLOG_CE 0x1
+#define X38_ECCERRLOG_UE 0x2
+#define X38_ECCERRLOG_RANK_BITS 0x18000000
+#define X38_ECCERRLOG_SYNDROME_BITS 0xff0000
+
+#define X38_CAPID0 0xe0 /* see P.94 of spec for details */
+
+static int x38_channel_num;
+
+static int how_many_channel(struct pci_dev *pdev)
+{
+ unsigned char capid0_8b; /* 8th byte of CAPID0 */
+
+ pci_read_config_byte(pdev, X38_CAPID0 + 8, &capid0_8b);
+ if (capid0_8b & 0x20) { /* check DCD: Dual Channel Disable */
+ debugf0("In single channel mode.\n");
+ x38_channel_num = 1;
+ } else {
+ debugf0("In dual channel mode.\n");
+ x38_channel_num = 2;
+ }
+
+ return x38_channel_num;
+}
+
+static unsigned long eccerrlog_syndrome(u64 log)
+{
+ return (log & X38_ECCERRLOG_SYNDROME_BITS) >> 16;
+}
+
+static int eccerrlog_row(int channel, u64 log)
+{
+ return ((log & X38_ECCERRLOG_RANK_BITS) >> 27) |
+ (channel * X38_RANKS_PER_CHANNEL);
+}
+
+enum x38_chips {
+ X38 = 0,
+};
+
+struct x38_dev_info {
+ const char *ctl_name;
+};
+
+struct x38_error_info {
+ u16 errsts;
+ u16 errsts2;
+ u64 eccerrlog[X38_CHANNELS];
+};
+
+static const struct x38_dev_info x38_devs[] = {
+ [X38] = {
+ .ctl_name = "x38"},
+};
+
+static struct pci_dev *mci_pdev;
+static int x38_registered = 1;
+
+
+static void x38_clear_error_info(struct mem_ctl_info *mci)
+{
+ struct pci_dev *pdev;
+
+ pdev = to_pci_dev(mci->dev);
+
+ /*
+ * Clear any error bits.
+ * (Yes, we really clear bits by writing 1 to them.)
+ */
+ pci_write_bits16(pdev, X38_ERRSTS, X38_ERRSTS_BITS,
+ X38_ERRSTS_BITS);
+}
+
+static u64 x38_readq(const void __iomem *addr)
+{
+ return readl(addr) | (((u64)readl(addr + 4)) << 32);
+}
+
+static void x38_get_and_clear_error_info(struct mem_ctl_info *mci,
+ struct x38_error_info *info)
+{
+ struct pci_dev *pdev;
+ void __iomem *window = mci->pvt_info;
+
+ pdev = to_pci_dev(mci->dev);
+
+ /*
+ * This is a mess because there is no atomic way to read all the
+ * registers at once and the registers can transition from CE being
+ * overwritten by UE.
+ */
+ pci_read_config_word(pdev, X38_ERRSTS, &info->errsts);
+ if (!(info->errsts & X38_ERRSTS_BITS))
+ return;
+
+ info->eccerrlog[0] = x38_readq(window + X38_C0ECCERRLOG);
+ if (x38_channel_num == 2)
+ info->eccerrlog[1] = x38_readq(window + X38_C1ECCERRLOG);
+
+ pci_read_config_word(pdev, X38_ERRSTS, &info->errsts2);
+
+ /*
+ * If the error is the same for both reads then the first set
+ * of reads is valid. If there is a change then there is a CE
+ * with no info and the second set of reads is valid and
+ * should be UE info.
+ */
+ if ((info->errsts ^ info->errsts2) & X38_ERRSTS_BITS) {
+ info->eccerrlog[0] = x38_readq(window + X38_C0ECCERRLOG);
+ if (x38_channel_num == 2)
+ info->eccerrlog[1] =
+ x38_readq(window + X38_C1ECCERRLOG);
+ }
+
+ x38_clear_error_info(mci);
+}
+
+static void x38_process_error_info(struct mem_ctl_info *mci,
+ struct x38_error_info *info)
+{
+ int channel;
+ u64 log;
+
+ if (!(info->errsts & X38_ERRSTS_BITS))
+ return;
+
+ if ((info->errsts ^ info->errsts2) & X38_ERRSTS_BITS) {
+ edac_mc_handle_ce_no_info(mci, "UE overwrote CE");
+ info->errsts = info->errsts2;
+ }
+
+ for (channel = 0; channel < x38_channel_num; channel++) {
+ log = info->eccerrlog[channel];
+ if (log & X38_ECCERRLOG_UE) {
+ edac_mc_handle_ue(mci, 0, 0,
+ eccerrlog_row(channel, log), "x38 UE");
+ } else if (log & X38_ECCERRLOG_CE) {
+ edac_mc_handle_ce(mci, 0, 0,
+ eccerrlog_syndrome(log),
+ eccerrlog_row(channel, log), 0, "x38 CE");
+ }
+ }
+}
+
+static void x38_check(struct mem_ctl_info *mci)
+{
+ struct x38_error_info info;
+
+ debugf1("MC%d: %s()\n", mci->mc_idx, __func__);
+ x38_get_and_clear_error_info(mci, &info);
+ x38_process_error_info(mci, &info);
+}
+
+
+void __iomem *x38_map_mchbar(struct pci_dev *pdev)
+{
+ union {
+ u64 mchbar;
+ struct {
+ u32 mchbar_low;
+ u32 mchbar_high;
+ };
+ } u;
+ void __iomem *window;
+
+ pci_read_config_dword(pdev, X38_MCHBAR_LOW, &u.mchbar_low);
+ pci_write_config_dword(pdev, X38_MCHBAR_LOW, u.mchbar_low | 0x1);
+ pci_read_config_dword(pdev, X38_MCHBAR_HIGH, &u.mchbar_high);
+ u.mchbar &= X38_MCHBAR_MASK;
+
+ if (u.mchbar != (resource_size_t)u.mchbar) {
+ printk(KERN_ERR
+ "x38: mmio space beyond accessible range (0x%llx)\n",
+ (unsigned long long)u.mchbar);
+ return NULL;
+ }
+
+ window = ioremap_nocache(u.mchbar, X38_MMR_WINDOW_SIZE);
+ if (!window)
+ printk(KERN_ERR "x38: cannot map mmio space at 0x%llx\n",
+ (unsigned long long)u.mchbar);
+
+ return window;
+}
+
+
+static void x38_get_drbs(void __iomem *window,
+ u16 drbs[X38_CHANNELS][X38_RANKS_PER_CHANNEL])
+{
+ int i;
+
+ for (i = 0; i < X38_RANKS_PER_CHANNEL; i++) {
+ drbs[0][i] = readw(window + X38_C0DRB + 2*i) & X38_DRB_MASK;
+ drbs[1][i] = readw(window + X38_C1DRB + 2*i) & X38_DRB_MASK;
+ }
+}
+
+static bool x38_is_stacked(struct pci_dev *pdev,
+ u16 drbs[X38_CHANNELS][X38_RANKS_PER_CHANNEL])
+{
+ u16 tom;
+
+ pci_read_config_word(pdev, X38_TOM, &tom);
+ tom &= X38_TOM_MASK;
+
+ return drbs[X38_CHANNELS - 1][X38_RANKS_PER_CHANNEL - 1] == tom;
+}
+
+static unsigned long drb_to_nr_pages(
+ u16 drbs[X38_CHANNELS][X38_RANKS_PER_CHANNEL],
+ bool stacked, int channel, int rank)
+{
+ int n;
+
+ n = drbs[channel][rank];
+ if (rank > 0)
+ n -= drbs[channel][rank - 1];
+ if (stacked && (channel == 1) && drbs[channel][rank] ==
+ drbs[channel][X38_RANKS_PER_CHANNEL - 1]) {
+ n -= drbs[0][X38_RANKS_PER_CHANNEL - 1];
+ }
+
+ n <<= (X38_DRB_SHIFT - PAGE_SHIFT);
+ return n;
+}
+
+static int x38_probe1(struct pci_dev *pdev, int dev_idx)
+{
+ int rc;
+ int i;
+ struct mem_ctl_info *mci = NULL;
+ unsigned long last_page;
+ u16 drbs[X38_CHANNELS][X38_RANKS_PER_CHANNEL];
+ bool stacked;
+ void __iomem *window;
+
+ debugf0("MC: %s()\n", __func__);
+
+ window = x38_map_mchbar(pdev);
+ if (!window)
+ return -ENODEV;
+
+ x38_get_drbs(window, drbs);
+
+ how_many_channel(pdev);
+
+ /* FIXME: unconventional pvt_info usage */
+ mci = edac_mc_alloc(0, X38_RANKS, x38_channel_num, 0);
+ if (!mci)
+ return -ENOMEM;
+
+ debugf3("MC: %s(): init mci\n", __func__);
+
+ mci->dev = &pdev->dev;
+ mci->mtype_cap = MEM_FLAG_DDR2;
+
+ mci->edac_ctl_cap = EDAC_FLAG_SECDED;
+ mci->edac_cap = EDAC_FLAG_SECDED;
+
+ mci->mod_name = EDAC_MOD_STR;
+ mci->mod_ver = X38_REVISION;
+ mci->ctl_name = x38_devs[dev_idx].ctl_name;
+ mci->dev_name = pci_name(pdev);
+ mci->edac_check = x38_check;
+ mci->ctl_page_to_phys = NULL;
+ mci->pvt_info = window;
+
+ stacked = x38_is_stacked(pdev, drbs);
+
+ /*
+ * The dram rank boundary (DRB) reg values are boundary addresses
+ * for each DRAM rank with a granularity of 64MB. DRB regs are
+ * cumulative; the last one will contain the total memory
+ * contained in all ranks.
+ */
+ last_page = -1UL;
+ for (i = 0; i < mci->nr_csrows; i++) {
+ unsigned long nr_pages;
+ struct csrow_info *csrow = &mci->csrows[i];
+
+ nr_pages = drb_to_nr_pages(drbs, stacked,
+ i / X38_RANKS_PER_CHANNEL,
+ i % X38_RANKS_PER_CHANNEL);
+
+ if (nr_pages == 0) {
+ csrow->mtype = MEM_EMPTY;
+ continue;
+ }
+
+ csrow->first_page = last_page + 1;
+ last_page += nr_pages;
+ csrow->last_page = last_page;
+ csrow->nr_pages = nr_pages;
+
+ csrow->grain = nr_pages << PAGE_SHIFT;
+ csrow->mtype = MEM_DDR2;
+ csrow->dtype = DEV_UNKNOWN;
+ csrow->edac_mode = EDAC_UNKNOWN;
+ }
+
+ x38_clear_error_info(mci);
+
+ rc = -ENODEV;
+ if (edac_mc_add_mc(mci)) {
+ debugf3("MC: %s(): failed edac_mc_add_mc()\n", __func__);
+ goto fail;
+ }
+
+ /* get this far and it's successful */
+ debugf3("MC: %s(): success\n", __func__);
+ return 0;
+
+fail:
+ iounmap(window);
+ if (mci)
+ edac_mc_free(mci);
+
+ return rc;
+}
+
+static int __devinit x38_init_one(struct pci_dev *pdev,
+ const struct pci_device_id *ent)
+{
+ int rc;
+
+ debugf0("MC: %s()\n", __func__);
+
+ if (pci_enable_device(pdev) < 0)
+ return -EIO;
+
+ rc = x38_probe1(pdev, ent->driver_data);
+ if (!mci_pdev)
+ mci_pdev = pci_dev_get(pdev);
+
+ return rc;
+}
+
+static void __devexit x38_remove_one(struct pci_dev *pdev)
+{
+ struct mem_ctl_info *mci;
+
+ debugf0("%s()\n", __func__);
+
+ mci = edac_mc_del_mc(&pdev->dev);
+ if (!mci)
+ return;
+
+ iounmap(mci->pvt_info);
+
+ edac_mc_free(mci);
+}
+
+static const struct pci_device_id x38_pci_tbl[] __devinitdata = {
+ {
+ PCI_VEND_DEV(INTEL, X38_HB), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+ X38},
+ {
+ 0,
+ } /* 0 terminated list. */
+};
+
+MODULE_DEVICE_TABLE(pci, x38_pci_tbl);
+
+static struct pci_driver x38_driver = {
+ .name = EDAC_MOD_STR,
+ .probe = x38_init_one,
+ .remove = __devexit_p(x38_remove_one),
+ .id_table = x38_pci_tbl,
+};
+
+static int __init x38_init(void)
+{
+ int pci_rc;
+
+ debugf3("MC: %s()\n", __func__);
+
+ /* Ensure that the OPSTATE is set correctly for POLL or NMI */
+ opstate_init();
+
+ pci_rc = pci_register_driver(&x38_driver);
+ if (pci_rc < 0)
+ goto fail0;
+
+ if (!mci_pdev) {
+ x38_registered = 0;
+ mci_pdev = pci_get_device(PCI_VENDOR_ID_INTEL,
+ PCI_DEVICE_ID_INTEL_X38_HB, NULL);
+ if (!mci_pdev) {
+ debugf0("x38 pci_get_device fail\n");
+ pci_rc = -ENODEV;
+ goto fail1;
+ }
+
+ pci_rc = x38_init_one(mci_pdev, x38_pci_tbl);
+ if (pci_rc < 0) {
+ debugf0("x38 init fail\n");
+ pci_rc = -ENODEV;
+ goto fail1;
+ }
+ }
+
+ return 0;
+
+fail1:
+ pci_unregister_driver(&x38_driver);
+
+fail0:
+ if (mci_pdev)
+ pci_dev_put(mci_pdev);
+
+ return pci_rc;
+}
+
+static void __exit x38_exit(void)
+{
+ debugf3("MC: %s()\n", __func__);
+
+ pci_unregister_driver(&x38_driver);
+ if (!x38_registered) {
+ x38_remove_one(mci_pdev);
+ pci_dev_put(mci_pdev);
+ }
+}
+
+module_init(x38_init);
+module_exit(x38_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cluster Computing, Inc. Hitoshi Mitake");
+MODULE_DESCRIPTION("MC support for Intel X38 memory hub controllers");
+
+module_param(edac_op_state, int, 0444);
+MODULE_PARM_DESC(edac_op_state, "EDAC Error Reporting state: 0=Poll,1=NMI");
On Fri, 17 Oct 2008 15:39:45 -0600
[email protected] wrote:
> From: Hitoshi Mitake <[email protected]>
>
> I wrote a new module for Intel X38 chipset.
> This chipset is very similar to Intel 3200 chipset,
> but there are some different points,
> so I copyed i3200_edac.c and modified.
>
> This is a Intel's web page describing this chipset.
> http://www.intel.com/Products/Desktop/Chipsets/X38/X38-overview.htm
>
> I've tested this new module with broken memory,
> and it seems working well.
>
> This is a patch, please use.
> Hitoshi
>
> Signed-off-by: Hitoshi Mitake <[email protected]>
> Signed-off-by: Doug Thompson <[email protected]>
> ---
>
> Index: linux-2.6.27/drivers/edac/Kconfig
> ===================================================================
> --- linux-2.6.27.orig/drivers/edac/Kconfig
> +++ linux-2.6.27/drivers/edac/Kconfig
> @@ -102,6 +102,13 @@ config EDAC_I3000
> Support for error detection and correction on the Intel
> 3000 and 3010 server chipsets.
>
> +config EDAC_X38
> + tristate "Intel X38"
> + depends on EDAC_MM_EDAC && PCI && X86
> + help
> + Support for error detection and correction on the Intel
> + X38 server chipsets.
Is this truly X86, or will this driver only ever be used on x86_64 kernels?
>
> ...
>
> +static u64 x38_readq(const void __iomem *addr)
> +{
> + return readl(addr) | (((u64)readl(addr + 4)) << 32);
> +}
Because the x86_64 architecture already implements readq(), and it
would be nice to avoid creating a private version.
On Mon, 20 Oct 2008 16:32:28 -0700
Andrew Morton <[email protected]> wrote:
> On Fri, 17 Oct 2008 15:39:45 -0600
> [email protected] wrote:
>
> > From: Hitoshi Mitake <[email protected]>
> >
> > I wrote a new module for Intel X38 chipset.
> > This chipset is very similar to Intel 3200 chipset,
> > but there are some different points,
> > so I copyed i3200_edac.c and modified.
> >
> > This is a Intel's web page describing this chipset.
> > http://www.intel.com/Products/Desktop/Chipsets/X38/X38-overview.htm
> >
> > I've tested this new module with broken memory,
> > and it seems working well.
> >
> > This is a patch, please use.
> > Hitoshi
> >
> > Signed-off-by: Hitoshi Mitake <[email protected]>
> > Signed-off-by: Doug Thompson <[email protected]>
> > ---
> >
> > Index: linux-2.6.27/drivers/edac/Kconfig
> > ===================================================================
> > --- linux-2.6.27.orig/drivers/edac/Kconfig
> > +++ linux-2.6.27/drivers/edac/Kconfig
> > @@ -102,6 +102,13 @@ config EDAC_I3000
> > Support for error detection and correction on the Intel
> > 3000 and 3010 server chipsets.
> >
> > +config EDAC_X38
> > + tristate "Intel X38"
> > + depends on EDAC_MM_EDAC && PCI && X86
> > + help
> > + Support for error detection and correction on the Intel
> > + X38 server chipsets.
>
> Is this truly X86, or will this driver only ever be used on x86_64 kernels?
I didn't know readq() of x86_64, and missed the case
to use this driver on x86_64. Thank you.
I wrote new version of this driver and tested. It works well.
This is replacement for old version.
Or should I send diff of two versions?
Signed-off-by: Hitoshi Mitake <[email protected]>
Signed-off-by: Doug Thompson <[email protected]>
---
Index: linux-2.6.27/drivers/edac/Kconfig
===================================================================
--- linux-2.6.27.orig/drivers/edac/Kconfig 2008-10-31 01:37:28.000000000 +0900
+++ linux-2.6.27/drivers/edac/Kconfig 2008-10-31 01:37:46.000000000 +0900
@@ -102,6 +102,13 @@
Support for error detection and correction on the Intel
3000 and 3010 server chipsets.
+config EDAC_X38
+ tristate "Intel X38"
+ depends on EDAC_MM_EDAC && PCI && (X86 || X86_64)
+ help
+ Support for error detection and correction on the Intel
+ X38 server chipsets.
+
config EDAC_I82860
tristate "Intel 82860"
depends on EDAC_MM_EDAC && PCI && X86_32
Index: linux-2.6.27/drivers/edac/Makefile
===================================================================
--- linux-2.6.27.orig/drivers/edac/Makefile 2008-10-31 01:37:31.000000000 +0900
+++ linux-2.6.27/drivers/edac/Makefile 2008-10-31 01:37:46.000000000 +0900
@@ -26,6 +26,7 @@
obj-$(CONFIG_EDAC_I82875P) += i82875p_edac.o
obj-$(CONFIG_EDAC_I82975X) += i82975x_edac.o
obj-$(CONFIG_EDAC_I3000) += i3000_edac.o
+obj-$(CONFIG_EDAC_X38) += x38_edac.o
obj-$(CONFIG_EDAC_I82860) += i82860_edac.o
obj-$(CONFIG_EDAC_R82600) += r82600_edac.o
obj-$(CONFIG_EDAC_PASEMI) += pasemi_edac.o
Index: linux-2.6.27/drivers/edac/x38_edac.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.27/drivers/edac/x38_edac.c 2008-10-31 01:37:46.000000000 +0900
@@ -0,0 +1,526 @@
+/*
+ * Intel X38 Memory Controller kernel module
+ * Copyright (C) 2008 Cluster Computing, Inc.
+ *
+ * This file may be distributed under the terms of the
+ * GNU General Public License.
+ *
+ * This file is based on i3200_edac.c
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/pci.h>
+#include <linux/pci_ids.h>
+#include <linux/slab.h>
+#include <linux/edac.h>
+#include "edac_core.h"
+
+#define X38_REVISION "1.1"
+
+#define EDAC_MOD_STR "x38_edac"
+
+#define PCI_DEVICE_ID_INTEL_X38_HB 0x29e0
+
+#define X38_RANKS 8
+#define X38_RANKS_PER_CHANNEL 4
+#define X38_CHANNELS 2
+
+/* Intel X38 register addresses - device 0 function 0 - DRAM Controller */
+
+#define X38_MCHBAR_LOW 0x48 /* MCH Memory Mapped Register BAR */
+#define X38_MCHBAR_HIGH 0x4b
+#define X38_MCHBAR_MASK 0xfffffc000ULL /* bits 35:14 */
+#define X38_MMR_WINDOW_SIZE 16384
+
+#define X38_TOM 0xa0 /* Top of Memory (16b)
+ *
+ * 15:10 reserved
+ * 9:0 total populated physical memory
+ */
+#define X38_TOM_MASK 0x3ff /* bits 9:0 */
+#define X38_TOM_SHIFT 26 /* 64MiB grain */
+
+#define X38_ERRSTS 0xc8 /* Error Status Register (16b)
+ *
+ * 15 reserved
+ * 14 Isochronous TBWRR Run Behind FIFO Full
+ * (ITCV)
+ * 13 Isochronous TBWRR Run Behind FIFO Put
+ * (ITSTV)
+ * 12 reserved
+ * 11 MCH Thermal Sensor Event
+ * for SMI/SCI/SERR (GTSE)
+ * 10 reserved
+ * 9 LOCK to non-DRAM Memory Flag (LCKF)
+ * 8 reserved
+ * 7 DRAM Throttle Flag (DTF)
+ * 6:2 reserved
+ * 1 Multi-bit DRAM ECC Error Flag (DMERR)
+ * 0 Single-bit DRAM ECC Error Flag (DSERR)
+ */
+#define X38_ERRSTS_UE 0x0002
+#define X38_ERRSTS_CE 0x0001
+#define X38_ERRSTS_BITS (X38_ERRSTS_UE | X38_ERRSTS_CE)
+
+
+/* Intel MMIO register space - device 0 function 0 - MMR space */
+
+#define X38_C0DRB 0x200 /* Channel 0 DRAM Rank Boundary (16b x 4)
+ *
+ * 15:10 reserved
+ * 9:0 Channel 0 DRAM Rank Boundary Address
+ */
+#define X38_C1DRB 0x600 /* Channel 1 DRAM Rank Boundary (16b x 4) */
+#define X38_DRB_MASK 0x3ff /* bits 9:0 */
+#define X38_DRB_SHIFT 26 /* 64MiB grain */
+
+#define X38_C0ECCERRLOG 0x280 /* Channel 0 ECC Error Log (64b)
+ *
+ * 63:48 Error Column Address (ERRCOL)
+ * 47:32 Error Row Address (ERRROW)
+ * 31:29 Error Bank Address (ERRBANK)
+ * 28:27 Error Rank Address (ERRRANK)
+ * 26:24 reserved
+ * 23:16 Error Syndrome (ERRSYND)
+ * 15: 2 reserved
+ * 1 Multiple Bit Error Status (MERRSTS)
+ * 0 Correctable Error Status (CERRSTS)
+ */
+#define X38_C1ECCERRLOG 0x680 /* Channel 1 ECC Error Log (64b) */
+#define X38_ECCERRLOG_CE 0x1
+#define X38_ECCERRLOG_UE 0x2
+#define X38_ECCERRLOG_RANK_BITS 0x18000000
+#define X38_ECCERRLOG_SYNDROME_BITS 0xff0000
+
+#define X38_CAPID0 0xe0 /* see P.94 of spec for details */
+
+static int x38_channel_num;
+
+static int how_many_channel(struct pci_dev *pdev)
+{
+ unsigned char capid0_8b; /* 8th byte of CAPID0 */
+
+ pci_read_config_byte(pdev, X38_CAPID0 + 8, &capid0_8b);
+ if (capid0_8b & 0x20) { /* check DCD: Dual Channel Disable */
+ debugf0("In single channel mode.\n");
+ x38_channel_num = 1;
+ } else {
+ debugf0("In dual channel mode.\n");
+ x38_channel_num = 2;
+ }
+
+ return x38_channel_num;
+}
+
+static unsigned long eccerrlog_syndrome(u64 log)
+{
+ return (log & X38_ECCERRLOG_SYNDROME_BITS) >> 16;
+}
+
+static int eccerrlog_row(int channel, u64 log)
+{
+ return ((log & X38_ECCERRLOG_RANK_BITS) >> 27) |
+ (channel * X38_RANKS_PER_CHANNEL);
+}
+
+enum x38_chips {
+ X38 = 0,
+};
+
+struct x38_dev_info {
+ const char *ctl_name;
+};
+
+struct x38_error_info {
+ u16 errsts;
+ u16 errsts2;
+ u64 eccerrlog[X38_CHANNELS];
+};
+
+static const struct x38_dev_info x38_devs[] = {
+ [X38] = {
+ .ctl_name = "x38"},
+};
+
+static struct pci_dev *mci_pdev;
+static int x38_registered = 1;
+
+
+static void x38_clear_error_info(struct mem_ctl_info *mci)
+{
+ struct pci_dev *pdev;
+
+ pdev = to_pci_dev(mci->dev);
+
+ /*
+ * Clear any error bits.
+ * (Yes, we really clear bits by writing 1 to them.)
+ */
+ pci_write_bits16(pdev, X38_ERRSTS, X38_ERRSTS_BITS,
+ X38_ERRSTS_BITS);
+}
+
+#ifndef CONFIG_X86_64
+static u64 readq(const void __iomem *addr)
+{
+ return readl(addr) | (((u64)readl(addr + 4)) << 32);
+}
+#endif /* CONFIG_X86_64 */
+
+static void x38_get_and_clear_error_info(struct mem_ctl_info *mci,
+ struct x38_error_info *info)
+{
+ struct pci_dev *pdev;
+ void __iomem *window = mci->pvt_info;
+
+ pdev = to_pci_dev(mci->dev);
+
+ /*
+ * This is a mess because there is no atomic way to read all the
+ * registers at once and the registers can transition from CE being
+ * overwritten by UE.
+ */
+ pci_read_config_word(pdev, X38_ERRSTS, &info->errsts);
+ if (!(info->errsts & X38_ERRSTS_BITS))
+ return;
+
+ info->eccerrlog[0] = readq(window + X38_C0ECCERRLOG);
+ if (x38_channel_num == 2)
+ info->eccerrlog[1] = readq(window + X38_C1ECCERRLOG);
+
+ pci_read_config_word(pdev, X38_ERRSTS, &info->errsts2);
+
+ /*
+ * If the error is the same for both reads then the first set
+ * of reads is valid. If there is a change then there is a CE
+ * with no info and the second set of reads is valid and
+ * should be UE info.
+ */
+ if ((info->errsts ^ info->errsts2) & X38_ERRSTS_BITS) {
+ info->eccerrlog[0] = readq(window + X38_C0ECCERRLOG);
+ if (x38_channel_num == 2)
+ info->eccerrlog[1] =
+ readq(window + X38_C1ECCERRLOG);
+ }
+
+ x38_clear_error_info(mci);
+}
+
+static void x38_process_error_info(struct mem_ctl_info *mci,
+ struct x38_error_info *info)
+{
+ int channel;
+ u64 log;
+
+ if (!(info->errsts & X38_ERRSTS_BITS))
+ return;
+
+ if ((info->errsts ^ info->errsts2) & X38_ERRSTS_BITS) {
+ edac_mc_handle_ce_no_info(mci, "UE overwrote CE");
+ info->errsts = info->errsts2;
+ }
+
+ for (channel = 0; channel < x38_channel_num; channel++) {
+ log = info->eccerrlog[channel];
+ if (log & X38_ECCERRLOG_UE) {
+ edac_mc_handle_ue(mci, 0, 0,
+ eccerrlog_row(channel, log), "x38 UE");
+ } else if (log & X38_ECCERRLOG_CE) {
+ edac_mc_handle_ce(mci, 0, 0,
+ eccerrlog_syndrome(log),
+ eccerrlog_row(channel, log), 0, "x38 CE");
+ }
+ }
+}
+
+static void x38_check(struct mem_ctl_info *mci)
+{
+ struct x38_error_info info;
+
+ debugf1("MC%d: %s()\n", mci->mc_idx, __func__);
+ x38_get_and_clear_error_info(mci, &info);
+ x38_process_error_info(mci, &info);
+}
+
+
+void __iomem *x38_map_mchbar(struct pci_dev *pdev)
+{
+ union {
+ u64 mchbar;
+ struct {
+ u32 mchbar_low;
+ u32 mchbar_high;
+ };
+ } u;
+ void __iomem *window;
+
+ pci_read_config_dword(pdev, X38_MCHBAR_LOW, &u.mchbar_low);
+ pci_write_config_dword(pdev, X38_MCHBAR_LOW, u.mchbar_low | 0x1);
+ pci_read_config_dword(pdev, X38_MCHBAR_HIGH, &u.mchbar_high);
+ u.mchbar &= X38_MCHBAR_MASK;
+
+ if (u.mchbar != (resource_size_t)u.mchbar) {
+ printk(KERN_ERR
+ "x38: mmio space beyond accessible range (0x%llx)\n",
+ (unsigned long long)u.mchbar);
+ return NULL;
+ }
+
+ window = ioremap_nocache(u.mchbar, X38_MMR_WINDOW_SIZE);
+ if (!window)
+ printk(KERN_ERR "x38: cannot map mmio space at 0x%llx\n",
+ (unsigned long long)u.mchbar);
+
+ return window;
+}
+
+
+static void x38_get_drbs(void __iomem *window,
+ u16 drbs[X38_CHANNELS][X38_RANKS_PER_CHANNEL])
+{
+ int i;
+
+ for (i = 0; i < X38_RANKS_PER_CHANNEL; i++) {
+ drbs[0][i] = readw(window + X38_C0DRB + 2*i) & X38_DRB_MASK;
+ drbs[1][i] = readw(window + X38_C1DRB + 2*i) & X38_DRB_MASK;
+ }
+}
+
+static bool x38_is_stacked(struct pci_dev *pdev,
+ u16 drbs[X38_CHANNELS][X38_RANKS_PER_CHANNEL])
+{
+ u16 tom;
+
+ pci_read_config_word(pdev, X38_TOM, &tom);
+ tom &= X38_TOM_MASK;
+
+ return drbs[X38_CHANNELS - 1][X38_RANKS_PER_CHANNEL - 1] == tom;
+}
+
+static unsigned long drb_to_nr_pages(
+ u16 drbs[X38_CHANNELS][X38_RANKS_PER_CHANNEL],
+ bool stacked, int channel, int rank)
+{
+ int n;
+
+ n = drbs[channel][rank];
+ if (rank > 0)
+ n -= drbs[channel][rank - 1];
+ if (stacked && (channel == 1) && drbs[channel][rank] ==
+ drbs[channel][X38_RANKS_PER_CHANNEL - 1]) {
+ n -= drbs[0][X38_RANKS_PER_CHANNEL - 1];
+ }
+
+ n <<= (X38_DRB_SHIFT - PAGE_SHIFT);
+ return n;
+}
+
+static int x38_probe1(struct pci_dev *pdev, int dev_idx)
+{
+ int rc;
+ int i;
+ struct mem_ctl_info *mci = NULL;
+ unsigned long last_page;
+ u16 drbs[X38_CHANNELS][X38_RANKS_PER_CHANNEL];
+ bool stacked;
+ void __iomem *window;
+
+ debugf0("MC: %s()\n", __func__);
+
+ window = x38_map_mchbar(pdev);
+ if (!window)
+ return -ENODEV;
+
+ x38_get_drbs(window, drbs);
+
+ how_many_channel(pdev);
+
+ /* FIXME: unconventional pvt_info usage */
+ mci = edac_mc_alloc(0, X38_RANKS, x38_channel_num, 0);
+ if (!mci)
+ return -ENOMEM;
+
+ debugf3("MC: %s(): init mci\n", __func__);
+
+ mci->dev = &pdev->dev;
+ mci->mtype_cap = MEM_FLAG_DDR2;
+
+ mci->edac_ctl_cap = EDAC_FLAG_SECDED;
+ mci->edac_cap = EDAC_FLAG_SECDED;
+
+ mci->mod_name = EDAC_MOD_STR;
+ mci->mod_ver = X38_REVISION;
+ mci->ctl_name = x38_devs[dev_idx].ctl_name;
+ mci->dev_name = pci_name(pdev);
+ mci->edac_check = x38_check;
+ mci->ctl_page_to_phys = NULL;
+ mci->pvt_info = window;
+
+ stacked = x38_is_stacked(pdev, drbs);
+
+ /*
+ * The dram rank boundary (DRB) reg values are boundary addresses
+ * for each DRAM rank with a granularity of 64MB. DRB regs are
+ * cumulative; the last one will contain the total memory
+ * contained in all ranks.
+ */
+ last_page = -1UL;
+ for (i = 0; i < mci->nr_csrows; i++) {
+ unsigned long nr_pages;
+ struct csrow_info *csrow = &mci->csrows[i];
+
+ nr_pages = drb_to_nr_pages(drbs, stacked,
+ i / X38_RANKS_PER_CHANNEL,
+ i % X38_RANKS_PER_CHANNEL);
+
+ if (nr_pages == 0) {
+ csrow->mtype = MEM_EMPTY;
+ continue;
+ }
+
+ csrow->first_page = last_page + 1;
+ last_page += nr_pages;
+ csrow->last_page = last_page;
+ csrow->nr_pages = nr_pages;
+
+ csrow->grain = nr_pages << PAGE_SHIFT;
+ csrow->mtype = MEM_DDR2;
+ csrow->dtype = DEV_UNKNOWN;
+ csrow->edac_mode = EDAC_UNKNOWN;
+ }
+
+ x38_clear_error_info(mci);
+
+ rc = -ENODEV;
+ if (edac_mc_add_mc(mci)) {
+ debugf3("MC: %s(): failed edac_mc_add_mc()\n", __func__);
+ goto fail;
+ }
+
+ /* get this far and it's successful */
+ debugf3("MC: %s(): success\n", __func__);
+ return 0;
+
+fail:
+ iounmap(window);
+ if (mci)
+ edac_mc_free(mci);
+
+ return rc;
+}
+
+static int __devinit x38_init_one(struct pci_dev *pdev,
+ const struct pci_device_id *ent)
+{
+ int rc;
+
+ debugf0("MC: %s()\n", __func__);
+
+ if (pci_enable_device(pdev) < 0)
+ return -EIO;
+
+ rc = x38_probe1(pdev, ent->driver_data);
+ if (!mci_pdev)
+ mci_pdev = pci_dev_get(pdev);
+
+ return rc;
+}
+
+static void __devexit x38_remove_one(struct pci_dev *pdev)
+{
+ struct mem_ctl_info *mci;
+
+ debugf0("%s()\n", __func__);
+
+ mci = edac_mc_del_mc(&pdev->dev);
+ if (!mci)
+ return;
+
+ iounmap(mci->pvt_info);
+
+ edac_mc_free(mci);
+}
+
+static const struct pci_device_id x38_pci_tbl[] __devinitdata = {
+ {
+ PCI_VEND_DEV(INTEL, X38_HB), PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+ X38},
+ {
+ 0,
+ } /* 0 terminated list. */
+};
+
+MODULE_DEVICE_TABLE(pci, x38_pci_tbl);
+
+static struct pci_driver x38_driver = {
+ .name = EDAC_MOD_STR,
+ .probe = x38_init_one,
+ .remove = __devexit_p(x38_remove_one),
+ .id_table = x38_pci_tbl,
+};
+
+static int __init x38_init(void)
+{
+ int pci_rc;
+
+ debugf3("MC: %s()\n", __func__);
+
+ /* Ensure that the OPSTATE is set correctly for POLL or NMI */
+ opstate_init();
+
+ pci_rc = pci_register_driver(&x38_driver);
+ if (pci_rc < 0)
+ goto fail0;
+
+ if (!mci_pdev) {
+ x38_registered = 0;
+ mci_pdev = pci_get_device(PCI_VENDOR_ID_INTEL,
+ PCI_DEVICE_ID_INTEL_X38_HB, NULL);
+ if (!mci_pdev) {
+ debugf0("x38 pci_get_device fail\n");
+ pci_rc = -ENODEV;
+ goto fail1;
+ }
+
+ pci_rc = x38_init_one(mci_pdev, x38_pci_tbl);
+ if (pci_rc < 0) {
+ debugf0("x38 init fail\n");
+ pci_rc = -ENODEV;
+ goto fail1;
+ }
+ }
+
+ return 0;
+
+fail1:
+ pci_unregister_driver(&x38_driver);
+
+fail0:
+ if (mci_pdev)
+ pci_dev_put(mci_pdev);
+
+ return pci_rc;
+}
+
+static void __exit x38_exit(void)
+{
+ debugf3("MC: %s()\n", __func__);
+
+ pci_unregister_driver(&x38_driver);
+ if (!x38_registered) {
+ x38_remove_one(mci_pdev);
+ pci_dev_put(mci_pdev);
+ }
+}
+
+module_init(x38_init);
+module_exit(x38_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Cluster Computing, Inc. Hitoshi Mitake");
+MODULE_DESCRIPTION("MC support for Intel X38 memory hub controllers");
+
+module_param(edac_op_state, int, 0444);
+MODULE_PARM_DESC(edac_op_state, "EDAC Error Reporting state: 0=Poll,1=NMI");
--- Hitoshi Mitake <[email protected]> wrote:
> On Mon, 20 Oct 2008 16:32:28 -0700
> Andrew Morton <[email protected]> wrote:
>
> > On Fri, 17 Oct 2008 15:39:45 -0600
> > [email protected] wrote:
> >
> > > From: Hitoshi Mitake <[email protected]>
> > >
> > > I wrote a new module for Intel X38 chipset.
> > > This chipset is very similar to Intel 3200 chipset,
> > > but there are some different points,
> > > so I copyed i3200_edac.c and modified.
> > >
> > > This is a Intel's web page describing this chipset.
> > > http://www.intel.com/Products/Desktop/Chipsets/X38/X38-overview.htm
> > >
> > > I've tested this new module with broken memory,
> > > and it seems working well.
> > >
> > > This is a patch, please use.
> > > Hitoshi
> > >
> > > Signed-off-by: Hitoshi Mitake <[email protected]>
> > > Signed-off-by: Doug Thompson <[email protected]>
> > > ---
> > >
> > > Index: linux-2.6.27/drivers/edac/Kconfig
> > > ===================================================================
> > > --- linux-2.6.27.orig/drivers/edac/Kconfig
> > > +++ linux-2.6.27/drivers/edac/Kconfig
> > > @@ -102,6 +102,13 @@ config EDAC_I3000
> > > Support for error detection and correction on the Intel
> > > 3000 and 3010 server chipsets.
> > >
> > > +config EDAC_X38
> > > + tristate "Intel X38"
> > > + depends on EDAC_MM_EDAC && PCI && X86
> > > + help
> > > + Support for error detection and correction on the Intel
> > > + X38 server chipsets.
> >
> > Is this truly X86, or will this driver only ever be used on x86_64 kernels?
>
> I didn't know readq() of x86_64, and missed the case
> to use this driver on x86_64. Thank you.
>
> I wrote new version of this driver and tested. It works well.
> This is replacement for old version.
> Or should I send diff of two versions?
I would suggest sending a diff of what is now in the -mm tree to what it should be.
doug t
W1DUG
On Wed, 5 Nov 2008 08:26:12 -0800 (PST)
Doug Thompson <[email protected]> wrote:
> > I wrote new version of this driver and tested. It works well.
> > This is replacement for old version.
> > Or should I send diff of two versions?
>
>
> I would suggest sending a diff of what is now in the -mm tree to what it should be.
This driver is now in the mainline kernel so yes, a patch against that
kernel is the only appropriate way of updating the driver.
On Thu, 6 Nov 2008 16:46:41 -0800
Andrew Morton <[email protected]> wrote:
> On Wed, 5 Nov 2008 08:26:12 -0800 (PST)
> Doug Thompson <[email protected]> wrote:
>
> > > I wrote new version of this driver and tested. It works well.
> > > This is replacement for old version.
> > > Or should I send diff of two versions?
> >
> >
> > I would suggest sending a diff of what is now in the -mm tree to what it should be.
>
> This driver is now in the mainline kernel so yes, a patch against that
> kernel is the only appropriate way of updating the driver.
>
Sorry, and thanks for notification, Doug and Andrew.
This is patch against mainline kernel.
Signed-off-by: Hitoshi Mitake <[email protected]>
Signed-off-by: Doug Thompson <[email protected]>
---
Index: linux-2.6.28-rc3-git2/drivers/edac/Kconfig
===================================================================
--- linux-2.6.28-rc3-git2.orig/drivers/edac/Kconfig 2008-11-07 11:27:05.000000000 +0000
+++ linux-2.6.28-rc3-git2/drivers/edac/Kconfig 2008-11-07 11:27:14.000000000 +0000
@@ -104,7 +104,7 @@
config EDAC_X38
tristate "Intel X38"
- depends on EDAC_MM_EDAC && PCI && X86
+ depends on EDAC_MM_EDAC && PCI && (X86 || X86_64)
help
Support for error detection and correction on the Intel
X38 server chipsets.
Index: linux-2.6.28-rc3-git2/drivers/edac/x38_edac.c
===================================================================
--- linux-2.6.28-rc3-git2.orig/drivers/edac/x38_edac.c 2008-11-07 11:27:06.000000000 +0000
+++ linux-2.6.28-rc3-git2/drivers/edac/x38_edac.c 2008-11-07 11:27:29.000000000 +0000
@@ -162,10 +162,12 @@
X38_ERRSTS_BITS);
}
-static u64 x38_readq(const void __iomem *addr)
+#ifndef CONFIG_X86_64
+static u64 readq(const void __iomem *addr)
{
return readl(addr) | (((u64)readl(addr + 4)) << 32);
}
+#endif /* CONFIG_X86_64 */
static void x38_get_and_clear_error_info(struct mem_ctl_info *mci,
struct x38_error_info *info)
@@ -184,9 +186,9 @@
if (!(info->errsts & X38_ERRSTS_BITS))
return;
- info->eccerrlog[0] = x38_readq(window + X38_C0ECCERRLOG);
+ info->eccerrlog[0] = readq(window + X38_C0ECCERRLOG);
if (x38_channel_num == 2)
- info->eccerrlog[1] = x38_readq(window + X38_C1ECCERRLOG);
+ info->eccerrlog[1] = readq(window + X38_C1ECCERRLOG);
pci_read_config_word(pdev, X38_ERRSTS, &info->errsts2);
@@ -197,10 +199,10 @@
* should be UE info.
*/
if ((info->errsts ^ info->errsts2) & X38_ERRSTS_BITS) {
- info->eccerrlog[0] = x38_readq(window + X38_C0ECCERRLOG);
+ info->eccerrlog[0] = readq(window + X38_C0ECCERRLOG);
if (x38_channel_num == 2)
info->eccerrlog[1] =
- x38_readq(window + X38_C1ECCERRLOG);
+ readq(window + X38_C1ECCERRLOG);
}
x38_clear_error_info(mci);
On Fri, 7 Nov 2008 15:28:30 +0000 Hitoshi Mitake <[email protected]> wrote:
> On Thu, 6 Nov 2008 16:46:41 -0800
> Andrew Morton <[email protected]> wrote:
>
> > On Wed, 5 Nov 2008 08:26:12 -0800 (PST)
> > Doug Thompson <[email protected]> wrote:
> >
> > > > I wrote new version of this driver and tested. It works well.
> > > > This is replacement for old version.
> > > > Or should I send diff of two versions?
> > >
> > >
> > > I would suggest sending a diff of what is now in the -mm tree to what it should be.
> >
> > This driver is now in the mainline kernel so yes, a patch against that
> > kernel is the only appropriate way of updating the driver.
> >
>
> Sorry, and thanks for notification, Doug and Andrew.
> This is patch against mainline kernel.
>
> Signed-off-by: Hitoshi Mitake <[email protected]>
> Signed-off-by: Doug Thompson <[email protected]>
We will need a description of this patch for the changelog, please. We
always do.
On Thu, 6 Nov 2008 22:31:22 -0800
Andrew Morton <[email protected]> wrote:
> On Fri, 7 Nov 2008 15:28:30 +0000 Hitoshi Mitake <[email protected]> wrote:
>
> > On Thu, 6 Nov 2008 16:46:41 -0800
> > Andrew Morton <[email protected]> wrote:
> >
> > > On Wed, 5 Nov 2008 08:26:12 -0800 (PST)
> > > Doug Thompson <[email protected]> wrote:
> > >
> > > > > I wrote new version of this driver and tested. It works well.
> > > > > This is replacement for old version.
> > > > > Or should I send diff of two versions?
> > > >
> > > >
> > > > I would suggest sending a diff of what is now in the -mm tree to what it should be.
> > >
> > > This driver is now in the mainline kernel so yes, a patch against that
> > > kernel is the only appropriate way of updating the driver.
> > >
> >
> > Sorry, and thanks for notification, Doug and Andrew.
> > This is patch against mainline kernel.
> >
> > Signed-off-by: Hitoshi Mitake <[email protected]>
> > Signed-off-by: Doug Thompson <[email protected]>
>
> We will need a description of this patch for the changelog, please. We
> always do.
>
Sorry, I missed...
This patch makes x38_edac.c to use kernel's
readq() function when it is compiled for x86_64.
Signed-off-by: Hitoshi Mitake <[email protected]>
Signed-off-by: Doug Thompson <[email protected]>
---
Index: linux-2.6.28-rc3-git2/drivers/edac/Kconfig
===================================================================
--- linux-2.6.28-rc3-git2.orig/drivers/edac/Kconfig 2008-11-07 11:27:05.000000000 +0000
+++ linux-2.6.28-rc3-git2/drivers/edac/Kconfig 2008-11-07 11:27:14.000000000 +0000
@@ -104,7 +104,7 @@
config EDAC_X38
tristate "Intel X38"
- depends on EDAC_MM_EDAC && PCI && X86
+ depends on EDAC_MM_EDAC && PCI && (X86 || X86_64)
help
Support for error detection and correction on the Intel
X38 server chipsets.
Index: linux-2.6.28-rc3-git2/drivers/edac/x38_edac.c
===================================================================
--- linux-2.6.28-rc3-git2.orig/drivers/edac/x38_edac.c 2008-11-07 11:27:06.000000000 +0000
+++ linux-2.6.28-rc3-git2/drivers/edac/x38_edac.c 2008-11-07 11:27:29.000000000 +0000
@@ -162,10 +162,12 @@
X38_ERRSTS_BITS);
}
-static u64 x38_readq(const void __iomem *addr)
+#ifndef CONFIG_X86_64
+static u64 readq(const void __iomem *addr)
{
return readl(addr) | (((u64)readl(addr + 4)) << 32);
}
+#endif /* CONFIG_X86_64 */
static void x38_get_and_clear_error_info(struct mem_ctl_info *mci,
struct x38_error_info *info)
@@ -184,9 +186,9 @@
if (!(info->errsts & X38_ERRSTS_BITS))
return;
- info->eccerrlog[0] = x38_readq(window + X38_C0ECCERRLOG);
+ info->eccerrlog[0] = readq(window + X38_C0ECCERRLOG);
if (x38_channel_num == 2)
- info->eccerrlog[1] = x38_readq(window + X38_C1ECCERRLOG);
+ info->eccerrlog[1] = readq(window + X38_C1ECCERRLOG);
pci_read_config_word(pdev, X38_ERRSTS, &info->errsts2);
@@ -197,10 +199,10 @@
* should be UE info.
*/
if ((info->errsts ^ info->errsts2) & X38_ERRSTS_BITS) {
- info->eccerrlog[0] = x38_readq(window + X38_C0ECCERRLOG);
+ info->eccerrlog[0] = readq(window + X38_C0ECCERRLOG);
if (x38_channel_num == 2)
info->eccerrlog[1] =
- x38_readq(window + X38_C1ECCERRLOG);
+ readq(window + X38_C1ECCERRLOG);
}
x38_clear_error_info(mci);
On Fri, 7 Nov 2008 15:38:24 +0000 Hitoshi Mitake <[email protected]> wrote:
>
> This patch makes x38_edac.c to use kernel's
> readq() function when it is compiled for x86_64.
>
> Signed-off-by: Hitoshi Mitake <[email protected]>
> Signed-off-by: Doug Thompson <[email protected]>
> ---
>
> Index: linux-2.6.28-rc3-git2/drivers/edac/Kconfig
> ===================================================================
> --- linux-2.6.28-rc3-git2.orig/drivers/edac/Kconfig 2008-11-07 11:27:05.000000000 +0000
> +++ linux-2.6.28-rc3-git2/drivers/edac/Kconfig 2008-11-07 11:27:14.000000000 +0000
> @@ -104,7 +104,7 @@
>
> config EDAC_X38
> tristate "Intel X38"
> - depends on EDAC_MM_EDAC && PCI && X86
> + depends on EDAC_MM_EDAC && PCI && (X86 || X86_64)
CONFIG_X86 is true for both CONFIG_X86_32=y amek CONFIG_X86_64=y, so
this change isn't needed. I'll fix that up.
> --- linux-2.6.28-rc3-git2.orig/drivers/edac/x38_edac.c 2008-11-07 11:27:06.000000000 +0000
> +++ linux-2.6.28-rc3-git2/drivers/edac/x38_edac.c 2008-11-07 11:27:29.000000000 +0000
> @@ -162,10 +162,12 @@
> X38_ERRSTS_BITS);
> }
>
> -static u64 x38_readq(const void __iomem *addr)
> +#ifndef CONFIG_X86_64
> +static u64 readq(const void __iomem *addr)
hm, it'd be nice if there was some more general way of determining
whether the architecture provides readq/writeq.
I'm a same person as [email protected]. [email protected] is my
private mail address.
On Fri, Nov 7, 2008 at 4:11 PM, Andrew Morton <[email protected]> wrote:
> On Fri, 7 Nov 2008 15:38:24 +0000 Hitoshi Mitake <[email protected]> wrote:
>
>>
>> This patch makes x38_edac.c to use kernel's
>> readq() function when it is compiled for x86_64.
>>
>> Signed-off-by: Hitoshi Mitake <[email protected]>
>> Signed-off-by: Doug Thompson <[email protected]>
>> ---
>>
>> Index: linux-2.6.28-rc3-git2/drivers/edac/Kconfig
>> ===================================================================
>> --- linux-2.6.28-rc3-git2.orig/drivers/edac/Kconfig 2008-11-07 11:27:05.000000000 +0000
>> +++ linux-2.6.28-rc3-git2/drivers/edac/Kconfig 2008-11-07 11:27:14.000000000 +0000
>> @@ -104,7 +104,7 @@
>>
>> config EDAC_X38
>> tristate "Intel X38"
>> - depends on EDAC_MM_EDAC && PCI && X86
>> + depends on EDAC_MM_EDAC && PCI && (X86 || X86_64)
>
> CONFIG_X86 is true for both CONFIG_X86_32=y amek CONFIG_X86_64=y, so
> this change isn't needed. I'll fix that up.
Thanks for your fix up. This was redundancy, I should not change that point...
>
>> --- linux-2.6.28-rc3-git2.orig/drivers/edac/x38_edac.c 2008-11-07 11:27:06.000000000 +0000
>> +++ linux-2.6.28-rc3-git2/drivers/edac/x38_edac.c 2008-11-07 11:27:29.000000000 +0000
>> @@ -162,10 +162,12 @@
>> X38_ERRSTS_BITS);
>> }
>>
>> -static u64 x38_readq(const void __iomem *addr)
>> +#ifndef CONFIG_X86_64
>> +static u64 readq(const void __iomem *addr)
>
> hm, it'd be nice if there was some more general way of determining
> whether the architecture provides readq/writeq.
>
I found this code in include/asm-x86/io.h
#ifdef CONFIG_X86_64
...
/* Let people know we have them */
#define readq readq
#define writeq writeq
#endif
x86 programmers are able to know existence of readq/writeq by using
this definition.
And I grepped,
% grep readq `find include/asm-* -name "*.h"`
include/asm-mips/io.h: ".set mips3" "\t\t# __readq" "\n\t" \
include/asm-mips/io.h:#define readq_relaxed readq
include/asm-mips/io.h:#define readq readq
include/asm-mips/txx9/tx4927.h: ((__u32)__raw_readq(&tx4927_ccfgptr->crir)
>> 16)
include/asm-mips/txx9/tx4927.h:#define
TX4927_SDRAMC_CR(ch) __raw_readq(&tx4927_sdramcptr->cr[(ch)])
include/asm-mips/txx9/tx4927.h:#define
TX4927_EBUSC_CR(ch) __raw_readq(&tx4927_ebuscptr->cr[(ch)])
include/asm-mips/txx9/tx4927.h: ____raw_writeq(____raw_readq(adr) & ~bits, adr);
include/asm-mips/txx9/tx4927.h: ____raw_writeq(____raw_readq(adr) | bits, adr);
include/asm-mips/txx9/tx4927.h: ____raw_writeq(____raw_readq(&tx4927_ccfgptr->ccfg)
include/asm-mips/txx9/tx4927.h: ____raw_writeq((____raw_readq(&tx4927_ccfgptr->ccfg)
include/asm-mips/txx9/tx4927.h: ____raw_writeq((____raw_readq(&tx4927_ccfgptr->ccfg)
include/asm-mips/txx9/tx4938.h: ((__u32)__raw_readq(&tx4938_ccfgptr->crir)
>> 16)
include/asm-parisc/io.h:static inline unsigned long long
gsc_readq(unsigned long addr)
include/asm-parisc/io.h:static inline unsigned long long
__raw_readq(const volatile void __iomem *addr)
include/asm-parisc/io.h:#define readq(addr) __fswab64(__raw_readq(addr))
include/asm-parisc/io.h:#define readq_relaxed(addr) readq(addr)
include/asm-x86/io.h:build_mmio_read(readq, "q", unsigned long, "=r", :"memory")
include/asm-x86/io.h:build_mmio_read(__readq, "q", unsigned long, "=r", )
include/asm-x86/io.h:#define readq_relaxed(a) __readq(a)
include/asm-x86/io.h:#define __raw_readq __readq
include/asm-x86/io.h:#define readq readq
It seems that architectures that provide readq/writeq are
mips, parisc and x86 (and x86_64).
mips and x86 provides this line
#define readq readq
to let user know existence of readq (and writeq),
and parisc doesn't provide.
But there is,
#define readq(addr) __fswab64(__raw_readq(addr))
in parisc.
There is a difference between mips and x86's readq/writeq and
parisc's readq/writeq. mips and x86's definition is only token,
but parisc's definition is macro function.
But these defines can be used to determine existence of readq/writeq
by common preprocessor like this,
#ifndef readq
/* programmer can define private version of readq (or writeq) */
#endif
Is this way enough general for our requirement?
(If we use this as general way to determine existence of readq/writeq,
I want other architecture's developer (whose architecture provides
readq/writeq in the future)
to support same way.)
(cc linux-arch)
On Mon, 10 Nov 2008 00:10:34 +0900 "Hitoshi Mitake" <[email protected]> wrote:
> ...
>
> >> -static u64 x38_readq(const void __iomem *addr)
> >> +#ifndef CONFIG_X86_64
> >> +static u64 readq(const void __iomem *addr)
> >
> > hm, it'd be nice if there was some more general way of determining
> > whether the architecture provides readq/writeq.
> >
>
>
> I found this code in include/asm-x86/io.h
>
> #ifdef CONFIG_X86_64
>
> ...
>
> /* Let people know we have them */
> #define readq readq
> #define writeq writeq
> #endif
>
> x86 programmers are able to know existence of readq/writeq by using
> this definition.
>
> And I grepped,
>
> % grep readq `find include/asm-* -name "*.h"`
> include/asm-mips/io.h: ".set mips3" "\t\t# __readq" "\n\t" \
> include/asm-mips/io.h:#define readq_relaxed readq
> include/asm-mips/io.h:#define readq readq
> include/asm-mips/txx9/tx4927.h: ((__u32)__raw_readq(&tx4927_ccfgptr->crir)
> >> 16)
> include/asm-mips/txx9/tx4927.h:#define
> TX4927_SDRAMC_CR(ch) __raw_readq(&tx4927_sdramcptr->cr[(ch)])
> include/asm-mips/txx9/tx4927.h:#define
> TX4927_EBUSC_CR(ch) __raw_readq(&tx4927_ebuscptr->cr[(ch)])
> include/asm-mips/txx9/tx4927.h: ____raw_writeq(____raw_readq(adr) & ~bits, adr);
> include/asm-mips/txx9/tx4927.h: ____raw_writeq(____raw_readq(adr) | bits, adr);
> include/asm-mips/txx9/tx4927.h: ____raw_writeq(____raw_readq(&tx4927_ccfgptr->ccfg)
> include/asm-mips/txx9/tx4927.h: ____raw_writeq((____raw_readq(&tx4927_ccfgptr->ccfg)
> include/asm-mips/txx9/tx4927.h: ____raw_writeq((____raw_readq(&tx4927_ccfgptr->ccfg)
> include/asm-mips/txx9/tx4938.h: ((__u32)__raw_readq(&tx4938_ccfgptr->crir)
> >> 16)
> include/asm-parisc/io.h:static inline unsigned long long
> gsc_readq(unsigned long addr)
> include/asm-parisc/io.h:static inline unsigned long long
> __raw_readq(const volatile void __iomem *addr)
> include/asm-parisc/io.h:#define readq(addr) __fswab64(__raw_readq(addr))
> include/asm-parisc/io.h:#define readq_relaxed(addr) readq(addr)
> include/asm-x86/io.h:build_mmio_read(readq, "q", unsigned long, "=r", :"memory")
> include/asm-x86/io.h:build_mmio_read(__readq, "q", unsigned long, "=r", )
> include/asm-x86/io.h:#define readq_relaxed(a) __readq(a)
> include/asm-x86/io.h:#define __raw_readq __readq
> include/asm-x86/io.h:#define readq readq
>
> It seems that architectures that provide readq/writeq are
> mips, parisc and x86 (and x86_64).
>
> mips and x86 provides this line
> #define readq readq
> to let user know existence of readq (and writeq),
> and parisc doesn't provide.
> But there is,
> #define readq(addr) __fswab64(__raw_readq(addr))
> in parisc.
>
> There is a difference between mips and x86's readq/writeq and
> parisc's readq/writeq. mips and x86's definition is only token,
> but parisc's definition is macro function.
>
> But these defines can be used to determine existence of readq/writeq
> by common preprocessor like this,
> #ifndef readq
> /* programmer can define private version of readq (or writeq) */
> #endif
>
> Is this way enough general for our requirement?
> (If we use this as general way to determine existence of readq/writeq,
> I want other architecture's developer (whose architecture provides
> readq/writeq in the future)
> to support same way.)
Yes, I'd say that
#ifdef readq
Is a suitable way of determining whether the architecture implements
readq and writeq. It isn't pretty, but it will suffice.
A problem with it is that drivers will then do
#ifndef readq
<provide a local implementation here>
#endif
which rather sucks - we don't want lots of little private readq/writeq
implementations all over the tree.
Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
disable these drivers on the architectures which don't provide
readq/writeq support.
Also, I'm not sure that we have sufficiently defined the semantics of
these operations, and whether all the architectures which do purport to
implement them actually implement them with the same semantics.
On Sun, Nov 09, 2008 at 11:26:46AM -0800, Andrew Morton wrote:
> (cc linux-arch)
>
> > It seems that architectures that provide readq/writeq are
> > mips, parisc and x86 (and x86_64).
> >
There are more than that, grep arch/*/include also.
In addition to mips, parisc, and x86, there is ia64, alpha, sh, and
sparc.
> #ifdef readq
>
> Is a suitable way of determining whether the architecture implements
> readq and writeq. It isn't pretty, but it will suffice.
>
> A problem with it is that drivers will then do
>
> #ifndef readq
> <provide a local implementation here>
> #endif
>
> which rather sucks - we don't want lots of little private readq/writeq
> implementations all over the tree.
>
> Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> disable these drivers on the architectures which don't provide
> readq/writeq support.
>
However this is handled, we don't want a rehash of the read/writes{b,w,l} fiasco.
Allowing drivers to do their own local implementations of these things
has always been a complete disaster. A Kconfig option will at least take
care of having these craptastic ifdef lists for architectures in every
driver that rolls its own implementation.
Even a sub-optimal asm-generic version would be preferable, if the
semantics are well enough defined and consistently adhered to.
On Tue, 11 Nov 2008 15:11:40 +0900
Paul Mundt <[email protected]> wrote:
> On Sun, Nov 09, 2008 at 11:26:46AM -0800, Andrew Morton wrote:
> > (cc linux-arch)
> >
> > > It seems that architectures that provide readq/writeq are
> > > mips, parisc and x86 (and x86_64).
> > >
> There are more than that, grep arch/*/include also.
>
> In addition to mips, parisc, and x86, there is ia64, alpha, sh, and
> sparc.
I didn't noticed these functions, thanks.
>
> > #ifdef readq
> >
> > Is a suitable way of determining whether the architecture implements
> > readq and writeq. It isn't pretty, but it will suffice.
> >
> > A problem with it is that drivers will then do
> >
> > #ifndef readq
> > <provide a local implementation here>
> > #endif
> >
> > which rather sucks - we don't want lots of little private readq/writeq
> > implementations all over the tree.
> >
> > Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> > disable these drivers on the architectures which don't provide
> > readq/writeq support.
> >
> However this is handled, we don't want a rehash of the read/writes{b,w,l} fiasco.
>
> Allowing drivers to do their own local implementations of these things
> has always been a complete disaster. A Kconfig option will at least take
> care of having these craptastic ifdef lists for architectures in every
> driver that rolls its own implementation.
>
> Even a sub-optimal asm-generic version would be preferable, if the
> semantics are well enough defined and consistently adhered to.
I found nice source file, lib/iomap.c.
There are architecture independent read/write{b,w,l} (named like ioread8).
I want to implement architecture independent readq/writeq in lib/iomap.c .
Andrew told,
> Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> disable these drivers on the architectures which don't provide
> readq/writeq support.
But I want to use x38_edac.c on x86_32 environment,
and I think similar desire may occur in the future.
Is this way good enough? Request for comment.
On Sun, Nov 09, 2008 at 11:26:46AM -0800, Andrew Morton wrote:
> Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> disable these drivers on the architectures which don't provide
> readq/writeq support.
And we also need to define the exact semantics. Questions coming to mind:
o are implementations performing 2 32-bit accesses acceptable?
o if so, what ordering for the two accesses is acceptable?
Ralf
On Tue, Nov 18, 2008 at 12:16:20PM +0000, Ralf Baechle wrote:
> On Sun, Nov 09, 2008 at 11:26:46AM -0800, Andrew Morton wrote:
>
> > Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> > disable these drivers on the architectures which don't provide
> > readq/writeq support.
>
> And we also need to define the exact semantics. Questions coming to mind:
>
> o are implementations performing 2 32-bit accesses acceptable?
> o if so, what ordering for the two accesses is acceptable?
and don't forget to document the semantics. If we're going to end up
with CONFIG_ARCH_HAS_READQ which architectures can select, I suggest
putting it in the help for that symbol. Why not another random file
in Documentation/ ? Because it's a random file in Documentation/
that'll be overlooked when someone decided to select ARCH_HAS_READQ.
If it's along side the relevent config option, there is a higher
chance it will be noticed.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tue, 18 Nov 2008 12:32:15 +0000
Russell King <[email protected]> wrote:
> On Tue, Nov 18, 2008 at 12:16:20PM +0000, Ralf Baechle wrote:
> > On Sun, Nov 09, 2008 at 11:26:46AM -0800, Andrew Morton wrote:
> >
> > > Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> > > disable these drivers on the architectures which don't provide
> > > readq/writeq support.
> >
> > And we also need to define the exact semantics. Questions coming to mind:
> >
> > o are implementations performing 2 32-bit accesses acceptable?
> > o if so, what ordering for the two accesses is acceptable?
>
> and don't forget to document the semantics. If we're going to end up
> with CONFIG_ARCH_HAS_READQ which architectures can select, I suggest
> putting it in the help for that symbol. Why not another random file
> in Documentation/ ? Because it's a random file in Documentation/
> that'll be overlooked when someone decided to select ARCH_HAS_READQ.
> If it's along side the relevent config option, there is a higher
> chance it will be noticed.
>
Sorry for my late response...
I knew that implementing architecture-independed readq/writeq is too hard.
To check that implementation is good for every architecture and test that readq/writeq are
difficult works.
So I wrote patch in Andrew's way.
This patch adds ARCH_HAS_READQ to X86_32 and X86_64, adds ARCH_HAS_WRITEQ to X86_64
and adds readq() to X86_32 (writeq is yet).
I want someone to review it. If this patch is good enough, I'll write help document and more patch
adding ARCH_HAS_READQ and ARCH_HAS_WRITEQ to other architectre which has readq/writeq.
description of this patch: Adding config value to x86 architecture to determine existence of readq/writeq
Signed-off-by: Hitoshi Mitake <[email protected]>
---
arch/x86/Kconfig | 3 +++
arch/x86/include/asm/io.h | 8 ++++++++
2 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..8f3c949 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -11,9 +11,12 @@ config 64BIT
config X86_32
def_bool !64BIT
+ select ARCH_HAS_READQ
config X86_64
def_bool 64BIT
+ select ARCH_HAS_READQ
+ select ARCH_HAS_WRITEQ
### Arch settings
config X86
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..2a8fc26 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -57,6 +57,14 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
/* Let people know we have them */
#define readq readq
#define writeq writeq
+
+#else /* CONFIG_X86_32 */
+
+static inline unsigned long readq(const volatile void __iomem *addr)
+{
+ return readl(addr) | (((u64)readl(addr + 4)) << 32);
+}
+
#endif
extern int iommu_bio_merge;
--
1.5.6.5
Hitoshi Mitake wrote:
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -57,6 +57,14 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
> /* Let people know we have them */
> #define readq readq
> #define writeq writeq
> +
> +#else /* CONFIG_X86_32 */
> +
> +static inline unsigned long readq(const volatile void __iomem *addr)
> +{
> + return readl(addr) | (((u64)readl(addr + 4)) << 32);
> +}
> +
> #endif
>
Let's see:
* undefined ordering of operations (at least on x86, they really should
be performed in LSB first order.)
* Using "unsigned long" for a 64-bit number on a 32-bit architecture.
* Arithmetic on a void pointer.
Try something like:
static inline u64 readq(const volative void __iomem *addr)
{
volatile u32 __iomem *__p = addr;
u32 __l, __h;
__l = readl(p);
__h = readl(p+1);
return __l + ((u64)__h << 32);
}
-hpa
> volatile u32 __iomem *__p = addr;
> u32 __l, __h;
Do we really need all the "__" inside an inline function? Why
not just call these "p", "l" and "h"?
-Tony
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Luck, Tony wrote:
>> volatile u32 __iomem *__p = addr;
>> u32 __l, __h;
>
> Do we really need all the "__" inside an inline function? Why
> not just call these "p", "l" and "h"?
Sorry, user space habit (in userspace, and in anything that can be
included from userspace, one needs the __ to keep the namespace clean.)
-hpa
On Mon, 24 Nov 2008 10:02:40 -0800
"H. Peter Anvin" <[email protected]> wrote:
Thanks for your reviews and advices, Peter and Tony.
> Luck, Tony wrote:
> >> volatile u32 __iomem *__p = addr;
> >> u32 __l, __h;
> >
> > Do we really need all the "__" inside an inline function? Why
> > not just call these "p", "l" and "h"?
>
> Sorry, user space habit (in userspace, and in anything that can be
> included from userspace, one needs the __ to keep the namespace clean.)
I think which need __ is variable type name, not each variable name.
Name of each local variables can't effect namespace.
And I found this comment in asm-generic/int-ll64.h,
/*
* __xx is ok: it doesn't pollute the POSIX namespace. Use these in the
* header files exported to user space
*/
According to your advice, I rewrote the patch, how is this?
description of this patch: Adding config value to x86 architecture to determine existence of readq/writeq
Signed-off-by: Hitoshi Mitake <[email protected]>
---
arch/x86/Kconfig | 3 +++
arch/x86/include/asm/io.h | 17 +++++++++++++++++
2 files changed, 20 insertions(+), 0 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..8f3c949 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -11,9 +11,12 @@ config 64BIT
config X86_32
def_bool !64BIT
+ select ARCH_HAS_READQ
config X86_64
def_bool 64BIT
+ select ARCH_HAS_READQ
+ select ARCH_HAS_WRITEQ
### Arch settings
config X86
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..5322d18 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -4,6 +4,7 @@
#define ARCH_HAS_IOREMAP_WC
#include <linux/compiler.h>
+#include <asm-generic/int-ll64.h>
#define build_mmio_read(name, size, type, reg, barrier) \
static inline type name(const volatile void __iomem *addr) \
@@ -57,6 +58,22 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
/* Let people know we have them */
#define readq readq
#define writeq writeq
+
+#else /* CONFIG_X86_32 from here */
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+ const volatile u32 __iomem *p = addr;
+ u32 l, h;
+
+ l = readl(p);
+ h = readl(p+1);
+
+ return l + ((u64)h << 32);
+}
+
+#define readq readq
+
#endif
extern int iommu_bio_merge;
--
1.5.6.5
Hitoshi Mitake wrote:
> I think which need __ is variable type name, not each variable name.
> Name of each local variables can't effect namespace.
Wrong. It affects the namespace in the sense that it can interfere with
user-created macros. Again, this is only an issue for user space.
> And I found this comment in asm-generic/int-ll64.h,
> /*
> * __xx is ok: it doesn't pollute the POSIX namespace. Use these in the
> * header files exported to user space
> */
>
> According to your advice, I rewrote the patch, how is this?
Are you planning to add writeq() as well?
-hpa
On Mon, 24 Nov 2008 21:13:58 -0800
"H. Peter Anvin" <[email protected]> wrote:
> Hitoshi Mitake wrote:
> > I think which need __ is variable type name, not each variable name.
> > Name of each local variables can't effect namespace.
>
> Wrong. It affects the namespace in the sense that it can interfere with
> user-created macros. Again, this is only an issue for user space.
Sorry, I've misunderstood. I didn't think about macros.
>
> > And I found this comment in asm-generic/int-ll64.h,
> > /*
> > * __xx is ok: it doesn't pollute the POSIX namespace. Use these in the
> > * header files exported to user space
> > */
> >
> > According to your advice, I rewrote the patch, how is this?
>
> Are you planning to add writeq() as well?
>
Yes, I want to add writeq().
But there's a problem that
I don't have a plan to use writeq() now, so I can't test writeq() soon.
How is this? I think it isn't bad. I want to hear your opinion.
static inline void writeq(__u64 val, volatile void __iomem *addr)
{
writel((unsigned int)val, addr);
writel((unsigned int)(val >> 32), addr+1);
}
On Wed, 26 Nov 2008, Hitoshi Mitake wrote:
> On Mon, 24 Nov 2008 21:13:58 -0800
> "H. Peter Anvin" <[email protected]> wrote:
> > Are you planning to add writeq() as well?
>
> Yes, I want to add writeq().
> But there's a problem that
> I don't have a plan to use writeq() now, so I can't test writeq() soon.
>
> How is this? I think it isn't bad. I want to hear your opinion.
>
> static inline void writeq(__u64 val, volatile void __iomem *addr)
> {
> writel((unsigned int)val, addr);
> writel((unsigned int)(val >> 32), addr+1);
^
4
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, 25 Nov 2008 16:46:18 +0100 (CET)
Geert Uytterhoeven <[email protected]> wrote:
> On Wed, 26 Nov 2008, Hitoshi Mitake wrote:
> > On Mon, 24 Nov 2008 21:13:58 -0800
> > "H. Peter Anvin" <[email protected]> wrote:
> > > Are you planning to add writeq() as well?
> >
> > Yes, I want to add writeq().
> > But there's a problem that
> > I don't have a plan to use writeq() now, so I can't test writeq() soon.
> >
> > How is this? I think it isn't bad. I want to hear your opinion.
> >
> > static inline void writeq(__u64 val, volatile void __iomem *addr)
> > {
> > writel((unsigned int)val, addr);
> > writel((unsigned int)(val >> 32), addr+1);
> ^
> 4
>
> > }
>
Thanks, I missed about void pointer...
On Wed, 26 Nov 2008 01:10:59 +0900
Hitoshi Mitake <[email protected]> wrote:
> On Tue, 25 Nov 2008 16:46:18 +0100 (CET)
> Geert Uytterhoeven <[email protected]> wrote:
>
> > On Wed, 26 Nov 2008, Hitoshi Mitake wrote:
> > > On Mon, 24 Nov 2008 21:13:58 -0800
> > > "H. Peter Anvin" <[email protected]> wrote:
> > > > Are you planning to add writeq() as well?
> > >
> > > Yes, I want to add writeq().
> > > But there's a problem that
> > > I don't have a plan to use writeq() now, so I can't test writeq() soon.
> > >
> > > How is this? I think it isn't bad. I want to hear your opinion.
> > >
> > > static inline void writeq(__u64 val, volatile void __iomem *addr)
> > > {
> > > writel((unsigned int)val, addr);
> > > writel((unsigned int)(val >> 32), addr+1);
> > ^
> > 4
> >
> > > }
> >
>
> Thanks, I missed about void pointer...
>
I rewrote the patch. I think newest version is good enough.
Could you please review this?
description of this patch
Adding implementation of readq/writeq to x86_32,
and adding config value to x86 architecture to determine existence of readq/writeq
Signed-off-by: Hitoshi Mitake <[email protected]>
---
arch/x86/Kconfig | 4 ++++
arch/x86/include/asm/io.h | 24 ++++++++++++++++++++++++
2 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..10bd84c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -11,9 +11,13 @@ config 64BIT
config X86_32
def_bool !64BIT
+ select ARCH_HAS_READQ
+ select ARCH_HAS_WRITEQ
config X86_64
def_bool 64BIT
+ select ARCH_HAS_READQ
+ select ARCH_HAS_WRITEQ
### Arch settings
config X86
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..ac15d0e 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -4,6 +4,7 @@
#define ARCH_HAS_IOREMAP_WC
#include <linux/compiler.h>
+#include <asm-generic/int-ll64.h>
#define build_mmio_read(name, size, type, reg, barrier) \
static inline type name(const volatile void __iomem *addr) \
@@ -57,6 +58,29 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
/* Let people know we have them */
#define readq readq
#define writeq writeq
+
+#else /* CONFIG_X86_32 from here */
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+ const volatile u32 __iomem *p = addr;
+ u32 l, h;
+
+ l = readl(p);
+ h = readl(p+1);
+
+ return l + ((u64)h << 32);
+}
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+ writel((unsigned int)val, addr);
+ writel((unsigned int)(val >> 32), addr+4);
+}
+
+#define readq readq
+#define writeq writeq
+
#endif
extern int iommu_bio_merge;
--
1.5.6.5
The cast is unnecessary; one can argue for a cast to (u32) for clarity, but personally I think writel() is a clear enough especialy given the context.
--
Sent from my mobile phone (pardon any lack of formatting)
-----Original Message-----
From: Hitoshi Mitake <[email protected]>
Sent: Friday, November 28, 2008 16:11
To: Hitoshi Mitake <[email protected]>
Cc: Geert Uytterhoeven <[email protected]>; H. Peter Anvin <[email protected]>; Luck, Tony <[email protected]>; Russell King <[email protected]>; Ralf Baechle <[email protected]>; Andrew Morton <[email protected]>; Doug Thompson <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>
Subject: Re: [PATCH 1/1] edac x38: new MC driver module
On Wed, 26 Nov 2008 01:10:59 +0900
Hitoshi Mitake <[email protected]> wrote:
> On Tue, 25 Nov 2008 16:46:18 +0100 (CET)
> Geert Uytterhoeven <[email protected]> wrote:
>
> > On Wed, 26 Nov 2008, Hitoshi Mitake wrote:
> > > On Mon, 24 Nov 2008 21:13:58 -0800
> > > "H. Peter Anvin" <[email protected]> wrote:
> > > > Are you planning to add writeq() as well?
> > >
> > > Yes, I want to add writeq().
> > > But there's a problem that
> > > I don't have a plan to use writeq() now, so I can't test writeq() soon.
> > >
> > > How is this? I think it isn't bad. I want to hear your opinion.
> > >
> > > static inline void writeq(__u64 val, volatile void __iomem *addr)
> > > {
> > > writel((unsigned int)val, addr);
> > > writel((unsigned int)(val >> 32), addr+1);
> > ^
> > 4
> >
> > > }
> >
>
> Thanks, I missed about void pointer...
>
I rewrote the patch. I think newest version is good enough.
Could you please review this?
description of this patch
Adding implementation of readq/writeq to x86_32,
and adding config value to x86 architecture to determine existence of readq/writeq
Signed-off-by: Hitoshi Mitake <[email protected]>
---
arch/x86/Kconfig | 4 ++++
arch/x86/include/asm/io.h | 24 ++++++++++++++++++++++++
2 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..10bd84c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -11,9 +11,13 @@ config 64BIT
config X86_32
def_bool !64BIT
+ select ARCH_HAS_READQ
+ select ARCH_HAS_WRITEQ
config X86_64
def_bool 64BIT
+ select ARCH_HAS_READQ
+ select ARCH_HAS_WRITEQ
### Arch settings
config X86
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..ac15d0e 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -4,6 +4,7 @@
#define ARCH_HAS_IOREMAP_WC
#include <linux/compiler.h>
+#include <asm-generic/int-ll64.h>
#define build_mmio_read(name, size, type, reg, barrier) \
static inline type name(const volatile void __iomem *addr) \
@@ -57,6 +58,29 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
/* Let people know we have them */
#define readq readq
#define writeq writeq
+
+#else /* CONFIG_X86_32 from here */
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+ const volatile u32 __iomem *p = addr;
+ u32 l, h;
+
+ l = readl(p);
+ h = readl(p+1);
+
+ return l + ((u64)h << 32);
+}
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+ writel((unsigned int)val, addr);
+ writel((unsigned int)(val >> 32), addr+4);
+}
+
+#define readq readq
+#define writeq writeq
+
#endif
extern int iommu_bio_merge;
--
1.5.6.5
On Fri, 28 Nov 2008 16:56:39 -0800
"H. Peter Anvin" <[email protected]> wrote:
> The cast is unnecessary; one can argue for a cast to (u32) for clarity, but personally I think writel() is a clear enough especialy given the context.
>
Thanks for your advice. I removed redundant cat. How is this?
description of this patch
Adding implementation of readq/writeq to x86_32,
and adding config value to x86 architecture to determine existence of readq/writeq
Signed-off-by: Hitoshi Mitake <[email protected]>
---
arch/x86/Kconfig | 4 ++++
arch/x86/include/asm/io.h | 24 ++++++++++++++++++++++++
2 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..10bd84c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -11,9 +11,13 @@ config 64BIT
config X86_32
def_bool !64BIT
+ select ARCH_HAS_READQ
+ select ARCH_HAS_WRITEQ
config X86_64
def_bool 64BIT
+ select ARCH_HAS_READQ
+ select ARCH_HAS_WRITEQ
### Arch settings
config X86
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..ddc67aa 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -4,6 +4,7 @@
#define ARCH_HAS_IOREMAP_WC
#include <linux/compiler.h>
+#include <asm-generic/int-ll64.h>
#define build_mmio_read(name, size, type, reg, barrier) \
static inline type name(const volatile void __iomem *addr) \
@@ -57,6 +58,29 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
/* Let people know we have them */
#define readq readq
#define writeq writeq
+
+#else /* CONFIG_X86_32 from here */
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+ const volatile u32 __iomem *p = addr;
+ u32 l, h;
+
+ l = readl(p);
+ h = readl(p+1);
+
+ return l + ((u64)h << 32);
+}
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+ writel(val, addr);
+ writel(val >> 32, addr+4);
+}
+
+#define readq readq
+#define writeq writeq
+
#endif
extern int iommu_bio_merge;
--
1.5.6.5
* Hitoshi Mitake <[email protected]> wrote:
> +#define readq readq
> +#define writeq writeq
hm, that's done to override the generic definition? Looks weird and i
think that's rather fragile - it's easy to somehow get the generic header
without this override.
Ingo
On Sat, 29 Nov 2008 10:38:58 +0100
Ingo Molnar <[email protected]> wrote:
>
> * Hitoshi Mitake <[email protected]> wrote:
>
> > +#define readq readq
> > +#define writeq writeq
>
> hm, that's done to override the generic definition? Looks weird and i
> think that's rather fragile - it's easy to somehow get the generic header
> without this override.
No, the purpose of this #define is to let user of this function to know there's readq/writeq.
Like this,
#ifdef readq
/* do something */
#endif
But this is old way. ARCH_HAS_READQ and ARCH_HAS_WRITEQ are new ways
to determine existence of readq/writeq. Drivers which use readq/writeq should
depend on these values in their Kconfig file.
This definitions may be redundant. But there are some architectures
which already have this definition for same purpose. So I added.
Should I remove these?
>
> But this is old way. ARCH_HAS_READQ and ARCH_HAS_WRITEQ are new ways
> to determine existence of readq/writeq. Drivers which use readq/writeq should
> depend on these values in their Kconfig file.
If we look at arch/x86/Kconfig we see:
### Arch settings
config X86
def_bool y
select HAVE_AOUT if X86_32
select HAVE_UNSTABLE_SCHED_CLOCK
select HAVE_IDE
select HAVE_OPROFILE
select HAVE_IOREMAP_PROT
select HAVE_KPROBES
select ARCH_WANT_OPTIONAL_GPIOLIB
...
So the normal syntax here is "HAVE_XXX_XXX" - not ARCH_HAS_XXX_XXX
If you update your patch please use this syntax,
and locate the select under X86 - not under the 32/64 entries.
But I do not see why adding these in the first place.
See following advice from Linus:
http://marc.info/?l=linux-arch&m=121710129310710&w=2
===> Quote:
I really think that whoever started that 'HAVE_ARCH_x'/'ARCH_HAS_x' mess
with totally random symbols that have NOTHING WHAT-SO-EVER to do with the
actual symbols in question (so they do _not_ show up in grep'ing for some
use) should be shot.
We should never _ever_ use that model. And we use it way too much.
We should generally strive for the simpler and much more obvious
/* Generic definition */
#ifndef symbol
int symbol(..)
...
#endif
and then architecture code can do
#define symbol(x) ...
or if they want to do a function, and you _really_ don't like the '__weak'
part (or you want to make it an inline function and don't want the clash
with the real declaration), then you can just do
static inline int symbol(x)
{
...
}
#define symbol symbol
and again it all works fine WITHOUT having to introduce some idiotic new
and unrelated element called ARCH_HAS_SYMBOL.
<====
Sam
On Sat, 29 Nov 2008 11:52:29 +0100
Sam Ravnborg <[email protected]> wrote:
> >
> > But this is old way. ARCH_HAS_READQ and ARCH_HAS_WRITEQ are new ways
> > to determine existence of readq/writeq. Drivers which use readq/writeq should
> > depend on these values in their Kconfig file.
>
> If we look at arch/x86/Kconfig we see:
> ### Arch settings
> config X86
> def_bool y
> select HAVE_AOUT if X86_32
> select HAVE_UNSTABLE_SCHED_CLOCK
> select HAVE_IDE
> select HAVE_OPROFILE
> select HAVE_IOREMAP_PROT
> select HAVE_KPROBES
> select ARCH_WANT_OPTIONAL_GPIOLIB
> ...
>
> So the normal syntax here is "HAVE_XXX_XXX" - not ARCH_HAS_XXX_XXX
>
> If you update your patch please use this syntax,
> and locate the select under X86 - not under the 32/64 entries.
Thanks for your notification. I didn't notice that syntax rule.
>
> But I do not see why adding these in the first place.
>
Andrew Morton told that drivers which need readq/writeq should use ones of kernel,
and if architecture part of kernel does not provide readq/writeq, drivers should be disabled.
This is Andrew's mail:
http://marc.info/?l=linux-kernel&m=122625885124798&w=2
===> Quote:
#ifdef readq
Is a suitable way of determining whether the architecture implements
readq and writeq. It isn't pretty, but it will suffice.
A problem with it is that drivers will then do
#ifndef readq
<provide a local implementation here>
#endif
which rather sucks - we don't want lots of little private readq/writeq
implementations all over the tree.
Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
disable these drivers on the architectures which don't provide
readq/writeq support.
<====
This is new patch.
description of this patch
Adding implementation of readq/writeq to x86_32,
and adding config value to x86 architecture to determine existence of readq/writeq
Signed-off-by: Hitoshi Mitake <[email protected]>
---
arch/x86/Kconfig | 2 ++
arch/x86/include/asm/io.h | 24 ++++++++++++++++++++++++
2 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..75408fe 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -34,6 +34,8 @@ config X86
select HAVE_ARCH_TRACEHOOK
select HAVE_GENERIC_DMA_COHERENT if X86_32
select HAVE_EFFICIENT_UNALIGNED_ACCESS
+ select HAVE_READQ
+ select HAVE_WRITEQ
config ARCH_DEFCONFIG
string
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..ddc67aa 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -4,6 +4,7 @@
#define ARCH_HAS_IOREMAP_WC
#include <linux/compiler.h>
+#include <asm-generic/int-ll64.h>
#define build_mmio_read(name, size, type, reg, barrier) \
static inline type name(const volatile void __iomem *addr) \
@@ -57,6 +58,29 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
/* Let people know we have them */
#define readq readq
#define writeq writeq
+
+#else /* CONFIG_X86_32 from here */
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+ const volatile u32 __iomem *p = addr;
+ u32 l, h;
+
+ l = readl(p);
+ h = readl(p+1);
+
+ return l + ((u64)h << 32);
+}
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+ writel(val, addr);
+ writel(val >> 32, addr+4);
+}
+
+#define readq readq
+#define writeq writeq
+
#endif
extern int iommu_bio_merge;
--
1.5.6.5
On Sat, Nov 29, 2008 at 10:24:49PM +0900, Hitoshi Mitake wrote:
> On Sat, 29 Nov 2008 11:52:29 +0100
> Sam Ravnborg <[email protected]> wrote:
>
> > >
> > > But this is old way. ARCH_HAS_READQ and ARCH_HAS_WRITEQ are new ways
> > > to determine existence of readq/writeq. Drivers which use readq/writeq should
> > > depend on these values in their Kconfig file.
> >
> > If we look at arch/x86/Kconfig we see:
> > ### Arch settings
> > config X86
> > def_bool y
> > select HAVE_AOUT if X86_32
> > select HAVE_UNSTABLE_SCHED_CLOCK
> > select HAVE_IDE
> > select HAVE_OPROFILE
> > select HAVE_IOREMAP_PROT
> > select HAVE_KPROBES
> > select ARCH_WANT_OPTIONAL_GPIOLIB
> > ...
> >
> > So the normal syntax here is "HAVE_XXX_XXX" - not ARCH_HAS_XXX_XXX
> >
> > If you update your patch please use this syntax,
> > and locate the select under X86 - not under the 32/64 entries.
>
> Thanks for your notification. I didn't notice that syntax rule.
>
> >
> > But I do not see why adding these in the first place.
> >
>
> Andrew Morton told that drivers which need readq/writeq should use ones of kernel,
> and if architecture part of kernel does not provide readq/writeq, drivers should be disabled.
>
> This is Andrew's mail:
> http://marc.info/?l=linux-kernel&m=122625885124798&w=2
> ===> Quote:
>
> #ifdef readq
>
> Is a suitable way of determining whether the architecture implements
> readq and writeq. It isn't pretty, but it will suffice.
>
> A problem with it is that drivers will then do
>
> #ifndef readq
> <provide a local implementation here>
> #endif
>
> which rather sucks - we don't want lots of little private readq/writeq
> implementations all over the tree.
>
> Perhaps it would be better to have a CONFIG_ARCH_HAS_READQ and to then
> disable these drivers on the architectures which don't provide
> readq/writeq support.
>
> <====
I see both rationales and you combine them in your patch - OK.
And the reason why you cannot just add this to
include/linux/io.h is that not all architectures
provide a readl()/writel() I assume.
Feel free to add my Acked-by: to the patch.
Sam
On Sat, 29 Nov 2008 19:01:44 +0100
Sam Ravnborg <[email protected]> wrote:
> I see both rationales and you combine them in your patch - OK.
>
> And the reason why you cannot just add this to
> include/linux/io.h is that not all architectures
> provide a readl()/writel() I assume.
>
Yes, I can't say all architectures provide readl/writel.
And there may be some architecture depended problems,
so I can't decide to add my readq/writeq as architecture independent ones.
> Feel free to add my Acked-by: to the patch.
Thanks, I added your Acked-by to new patch.
description of this patch
Adding implementation of readq/writeq to x86_32,
and adding config value to x86 architecture to determine existence of readq/writeq
Signed-off-by: Hitoshi Mitake <[email protected]>
Acked-by: Sam Ravnborg <[email protected]>
---
arch/x86/Kconfig | 2 ++
arch/x86/include/asm/io.h | 24 ++++++++++++++++++++++++
2 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ac22bb7..75408fe 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -34,6 +34,8 @@ config X86
select HAVE_ARCH_TRACEHOOK
select HAVE_GENERIC_DMA_COHERENT if X86_32
select HAVE_EFFICIENT_UNALIGNED_ACCESS
+ select HAVE_READQ
+ select HAVE_WRITEQ
config ARCH_DEFCONFIG
string
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index ac2abc8..ddc67aa 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -4,6 +4,7 @@
#define ARCH_HAS_IOREMAP_WC
#include <linux/compiler.h>
+#include <asm-generic/int-ll64.h>
#define build_mmio_read(name, size, type, reg, barrier) \
static inline type name(const volatile void __iomem *addr) \
@@ -57,6 +58,29 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
/* Let people know we have them */
#define readq readq
#define writeq writeq
+
+#else /* CONFIG_X86_32 from here */
+
+static inline __u64 readq(const volatile void __iomem *addr)
+{
+ const volatile u32 __iomem *p = addr;
+ u32 l, h;
+
+ l = readl(p);
+ h = readl(p+1);
+
+ return l + ((u64)h << 32);
+}
+
+static inline void writeq(__u64 val, volatile void __iomem *addr)
+{
+ writel(val, addr);
+ writel(val >> 32, addr+4);
+}
+
+#define readq readq
+#define writeq writeq
+
#endif
extern int iommu_bio_merge;
--
1.5.6.5
* Hitoshi Mitake <[email protected]> wrote:
> On Sat, 29 Nov 2008 19:01:44 +0100
> Sam Ravnborg <[email protected]> wrote:
>
> > I see both rationales and you combine them in your patch - OK.
> >
> > And the reason why you cannot just add this to
> > include/linux/io.h is that not all architectures
> > provide a readl()/writel() I assume.
> >
> Yes, I can't say all architectures provide readl/writel.
> And there may be some architecture depended problems,
> so I can't decide to add my readq/writeq as architecture independent ones.
>
> > Feel free to add my Acked-by: to the patch.
>
> Thanks, I added your Acked-by to new patch.
applied to tip/x86/io, thanks! I also did the small cleanup below.
Ingo
------------>
>From 458102461ccedc3ab7847132b5db6a53782dc9a8 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Sun, 30 Nov 2008 09:33:55 +0100
Subject: [PATCH] x86: provide readq()/writeq() on 32-bit too, cleanup
Impact: cleanup
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/include/asm/io.h | 17 +++++++----------
1 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 2594644..3ccfaf6 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -55,21 +55,17 @@ build_mmio_write(__writeq, "q", unsigned long, "r", )
#define __raw_readq __readq
#define __raw_writeq writeq
-/* Let people know we have them */
-#define readq readq
-#define writeq writeq
-
#else /* CONFIG_X86_32 from here */
static inline __u64 readq(const volatile void __iomem *addr)
{
const volatile u32 __iomem *p = addr;
- u32 l, h;
+ u32 low, high;
- l = readl(p);
- h = readl(p + 1);
+ low = readl(p);
+ high = readl(p + 1);
- return l + ((u64)h << 32);
+ return low + ((u64)high << 32);
}
static inline void writeq(__u64 val, volatile void __iomem *addr)
@@ -78,11 +74,12 @@ static inline void writeq(__u64 val, volatile void __iomem *addr)
writel(val >> 32, addr+4);
}
+#endif
+
+/* Let people know that we have them */
#define readq readq
#define writeq writeq
-#endif
-
extern int iommu_bio_merge;
#ifdef CONFIG_X86_32
the 32-bit build broke promptly - readq/writeq is a family of APIs that
has to be either fully provided or not provided at all. The fix is below.
Ingo
--------------->
>From 16ebd68883e3583a66733d8b12ba55c8985af3f6 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Sun, 30 Nov 2008 10:20:20 +0100
Subject: [PATCH] x86: provide readq()/writeq() on 32-bit too, complete
if HAVE_READQ/HAVE_WRITEQ are defined, the full range of readq/writeq
APIs has to be provided to drivers:
drivers/infiniband/hw/amso1100/c2.c: In function 'c2_tx_ring_alloc':
drivers/infiniband/hw/amso1100/c2.c:133: error: implicit declaration of function '__raw_writeq'
So provide them on 32-bit as well. Also, map all the APIs to the
strongest ordering variant. It's way too easy to mess such details
up in drivers and the difference between "memory" and "" constrained
asm() constructs is in the noise range.
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/include/asm/io.h | 18 +++++++++---------
1 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 3ccfaf6..9036156 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -46,16 +46,11 @@ build_mmio_write(__writel, "l", unsigned int, "r", )
#define mmiowb() barrier()
#ifdef CONFIG_X86_64
+
build_mmio_read(readq, "q", unsigned long, "=r", :"memory")
-build_mmio_read(__readq, "q", unsigned long, "=r", )
build_mmio_write(writeq, "q", unsigned long, "r", :"memory")
-build_mmio_write(__writeq, "q", unsigned long, "r", )
-
-#define readq_relaxed(a) __readq(a)
-#define __raw_readq __readq
-#define __raw_writeq writeq
-#else /* CONFIG_X86_32 from here */
+#else
static inline __u64 readq(const volatile void __iomem *addr)
{
@@ -76,9 +71,14 @@ static inline void writeq(__u64 val, volatile void __iomem *addr)
#endif
+#define readq_relaxed(a) readq(a)
+
+#define __raw_readq(a) readq(a)
+#define __raw_writeq(a) writeq(a)
+
/* Let people know that we have them */
-#define readq readq
-#define writeq writeq
+#define readq readq
+#define writeq writeq
extern int iommu_bio_merge;
On Sun, 30 Nov 2008 10:24:07 +0100
Ingo Molnar <[email protected]> wrote:
>
> the 32-bit build broke promptly - readq/writeq is a family of APIs that
> has to be either fully provided or not provided at all. The fix is below.
Thanks for your fix and adding!
When will this patch be added to mainline?
I want to rewrite x38_edac.c to adapt new APIs.
* Hitoshi Mitake <[email protected]> wrote:
> On Sun, 30 Nov 2008 10:24:07 +0100
> Ingo Molnar <[email protected]> wrote:
>
> >
> > the 32-bit build broke promptly - readq/writeq is a family of APIs that
> > has to be either fully provided or not provided at all. The fix is below.
>
> Thanks for your fix and adding!
> When will this patch be added to mainline?
> I want to rewrite x38_edac.c to adapt new APIs.
v2.6.29 at the earliest - if there are no regressions. A number of
drivers use these APIs and usage is a bit messy - so bugs could be
triggered, etc.
Ingo
On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <[email protected]> wrote:
>
> * Hitoshi Mitake <[email protected]> wrote:
>
>> On Sun, 30 Nov 2008 10:24:07 +0100
>> Ingo Molnar <[email protected]> wrote:
>>
>> >
>> > the 32-bit build broke promptly - readq/writeq is a family of APIs that
>> > has to be either fully provided or not provided at all. The fix is below.
>>
>> Thanks for your fix and adding!
>> When will this patch be added to mainline?
>> I want to rewrite x38_edac.c to adapt new APIs.
>
> v2.6.29 at the earliest - if there are no regressions. A number of
> drivers use these APIs and usage is a bit messy - so bugs could be
> triggered, etc.
>
Thanks. What is URL of your repository?
I want to look your tree and test it.
* Hitoshi Mitake <[email protected]> wrote:
> On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <[email protected]> wrote:
> >
> > * Hitoshi Mitake <[email protected]> wrote:
> >
> >> On Sun, 30 Nov 2008 10:24:07 +0100
> >> Ingo Molnar <[email protected]> wrote:
> >>
> >> >
> >> > the 32-bit build broke promptly - readq/writeq is a family of APIs that
> >> > has to be either fully provided or not provided at all. The fix is below.
> >>
> >> Thanks for your fix and adding!
> >> When will this patch be added to mainline?
> >> I want to rewrite x38_edac.c to adapt new APIs.
> >
> > v2.6.29 at the earliest - if there are no regressions. A number of
> > drivers use these APIs and usage is a bit messy - so bugs could be
> > triggered, etc.
> >
> Thanks. What is URL of your repository?
> I want to look your tree and test it.
you can pick up tip/master via:
http://people.redhat.com/mingo/tip.git/README
Ingo
On Mon, Dec 1, 2008 at 22:59, Ingo Molnar <[email protected]> wrote:
>
> * Hitoshi Mitake <[email protected]> wrote:
>
>> On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <[email protected]> wrote:
>> >
>> > * Hitoshi Mitake <[email protected]> wrote:
>> >
>> >> On Sun, 30 Nov 2008 10:24:07 +0100
>> >> Ingo Molnar <[email protected]> wrote:
>> >>
>> >> >
>> >> > the 32-bit build broke promptly - readq/writeq is a family of APIs that
>> >> > has to be either fully provided or not provided at all. The fix is below.
>> >>
>> >> Thanks for your fix and adding!
>> >> When will this patch be added to mainline?
>> >> I want to rewrite x38_edac.c to adapt new APIs.
>> >
>> > v2.6.29 at the earliest - if there are no regressions. A number of
>> > drivers use these APIs and usage is a bit messy - so bugs could be
>> > triggered, etc.
>> >
>> Thanks. What is URL of your repository?
>> I want to look your tree and test it.
>
> you can pick up tip/master via:
>
> http://people.redhat.com/mingo/tip.git/README
>
Thanks!
On Tue, 2 Dec 2008 08:58:19 +0900
"Hitoshi Mitake" <[email protected]> wrote:
> On Mon, Dec 1, 2008 at 22:59, Ingo Molnar <[email protected]> wrote:
> >
> > * Hitoshi Mitake <[email protected]> wrote:
> >
> >> On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <[email protected]> wrote:
> >> >
> >> > * Hitoshi Mitake <[email protected]> wrote:
> >> >
> >> >> On Sun, 30 Nov 2008 10:24:07 +0100
> >> >> Ingo Molnar <[email protected]> wrote:
> >> >>
> >> >> >
> >> >> > the 32-bit build broke promptly - readq/writeq is a family of APIs that
> >> >> > has to be either fully provided or not provided at all. The fix is below.
> >> >>
> >> >> Thanks for your fix and adding!
> >> >> When will this patch be added to mainline?
> >> >> I want to rewrite x38_edac.c to adapt new APIs.
> >> >
> >> > v2.6.29 at the earliest - if there are no regressions. A number of
> >> > drivers use these APIs and usage is a bit messy - so bugs could be
> >> > triggered, etc.
> >> >
> >> Thanks. What is URL of your repository?
> >> I want to look your tree and test it.
> >
> > you can pick up tip/master via:
> >
> > http://people.redhat.com/mingo/tip.git/README
> >
>
I build your tree with allmodconfig, but there's no compile error.
And I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to all architecutres without x86.
Signed-off-by: Hitoshi Mitake <[email protected]>
---
arch/alpha/Kconfig | 2 ++
arch/ia64/Kconfig | 2 ++
arch/mips/Kconfig | 2 ++
arch/parisc/Kconfig | 2 ++
arch/powerpc/Kconfig | 2 ++
arch/sh/Kconfig | 2 ++
arch/sparc64/Kconfig | 2 ++
7 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
index 6110197..4355170 100644
--- a/arch/alpha/Kconfig
+++ b/arch/alpha/Kconfig
@@ -8,6 +8,8 @@ config ALPHA
select HAVE_AOUT
select HAVE_IDE
select HAVE_OPROFILE
+ select HAVE_READQ
+ select HAVE_WRITEQ
help
The Alpha is a 64-bit general-purpose processor designed and
marketed by the Digital Equipment Corporation of blessed memory,
diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index 6bd91ed..833bfdd 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -24,6 +24,8 @@ config IA64
select HAVE_DMA_ATTRS
select HAVE_KVM
select HAVE_ARCH_TRACEHOOK
+ select HAVE_READQ
+ select HAVE_WRITEQ
default y
help
The Itanium Processor Family is Intel's 64-bit successor to
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index f4af967..cf4f046 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -7,6 +7,8 @@ config MIPS
# Horrible source of confusion. Die, die, die ...
select EMBEDDED
select RTC_LIB
+ select HAVE_READQ
+ select HAVE_WRITEQ
mainmenu "Linux/MIPS Kernel Configuration"
diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index 644a70b..4eef1f0 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -11,6 +11,8 @@ config PARISC
select HAVE_OPROFILE
select RTC_CLASS
select RTC_DRV_PARISC
+ select HAVE_READQ
+ select HAVE_WRITEQ
help
The PA-RISC microprocessor is designed by Hewlett-Packard and used
in many of their workstations & servers (HP9000 700 and 800 series,
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 525c13a..9e1701f 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -121,6 +121,8 @@ config PPC
select HAVE_DMA_ATTRS if PPC64
select USE_GENERIC_SMP_HELPERS if SMP
select HAVE_OPROFILE
+ select HAVE_READQ
+ select HAVE_WRITEQ
config EARLY_PRINTK
bool
diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index 80119b3..410e1fe 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -13,6 +13,8 @@ config SUPERH
select HAVE_OPROFILE
select HAVE_GENERIC_DMA_COHERENT
select HAVE_IOREMAP_PROT if MMU
+ select HAVE_READQ
+ select HAVE_WRITEQ
help
The SuperH is a RISC processor targeted for use in embedded systems
and consumer electronics; it was also used in the Sega Dreamcast
diff --git a/arch/sparc64/Kconfig b/arch/sparc64/Kconfig
index 3b96e70..b92409f 100644
--- a/arch/sparc64/Kconfig
+++ b/arch/sparc64/Kconfig
@@ -24,6 +24,8 @@ config SPARC64
select RTC_DRV_BQ4802
select RTC_DRV_SUN4V
select RTC_DRV_STARFIRE
+ select HAVE_READQ
+ select HAVE_WRITEQ
config GENERIC_TIME
bool
--
1.5.6.5
I wrote wrong mail address of LKML:-(
Resending, sorry.
On Fri, 5 Dec 2008 00:58:41 +0900
Hitoshi Mitake <[email protected]> wrote:
> On Tue, 2 Dec 2008 08:58:19 +0900
> "Hitoshi Mitake" <[email protected]> wrote:
>
> > On Mon, Dec 1, 2008 at 22:59, Ingo Molnar <[email protected]> wrote:
> > >
> > > * Hitoshi Mitake <[email protected]> wrote:
> > >
> > >> On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <[email protected]> wrote:
> > >> >
> > >> > * Hitoshi Mitake <[email protected]> wrote:
> > >> >
> > >> >> On Sun, 30 Nov 2008 10:24:07 +0100
> > >> >> Ingo Molnar <[email protected]> wrote:
> > >> >>
> > >> >> >
> > >> >> > the 32-bit build broke promptly - readq/writeq is a family of APIs that
> > >> >> > has to be either fully provided or not provided at all. The fix is below.
> > >> >>
> > >> >> Thanks for your fix and adding!
> > >> >> When will this patch be added to mainline?
> > >> >> I want to rewrite x38_edac.c to adapt new APIs.
> > >> >
> > >> > v2.6.29 at the earliest - if there are no regressions. A number of
> > >> > drivers use these APIs and usage is a bit messy - so bugs could be
> > >> > triggered, etc.
> > >> >
> > >> Thanks. What is URL of your repository?
> > >> I want to look your tree and test it.
> > >
> > > you can pick up tip/master via:
> > >
> > > http://people.redhat.com/mingo/tip.git/README
> > >
> >
>
Ingo
I found that my previous patch contents wrong point, sorry.
Previous patch can't provide HAVE_READQ and HAVE_WRITEQ
to Kconfigs of other part well.
So I wrote a patch to fix this.
Please use.
Signed-off-by: Hitoshi Mitake <[email protected]>
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 73f7fe8..6f1abcd 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -19,8 +19,6 @@ config X86_64
config X86
def_bool y
select HAVE_AOUT if X86_32
- select HAVE_READQ
- select HAVE_WRITEQ
select HAVE_UNSTABLE_SCHED_CLOCK
select HAVE_IDE
select HAVE_OPROFILE
@@ -1969,6 +1967,12 @@ config HAVE_ATOMIC_IOMAP
def_bool y
depends on X86_32
+config HAVE_READQ
+ def_bool y
+
+config HAVE_WRITEQ
+ def_bool y
+
source "net/Kconfig"
source "drivers/Kconfig"