LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/1] edac x38: new MC driver module
@ 2008-10-17 21:39 dougthompson
  2008-10-20 23:32 ` Andrew Morton
  0 siblings, 1 reply; 51+ messages in thread
From: dougthompson @ 2008-10-17 21:39 UTC (permalink / raw)
  To: mitake, dougthompson, linux-kernel, akpm

From: Hitoshi Mitake <mitake@clustcom.com>

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 <mitake@clustcom.com>
Signed-off-by: Doug Thompson <dougthompson@xmission.com>
---

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");

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-10-17 21:39 [PATCH 1/1] edac x38: new MC driver module dougthompson
@ 2008-10-20 23:32 ` Andrew Morton
  2008-11-05 22:29   ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: Andrew Morton @ 2008-10-20 23:32 UTC (permalink / raw)
  To: dougthompson; +Cc: mitake, dougthompson, linux-kernel

On Fri, 17 Oct 2008 15:39:45 -0600
dougthompson@xmission.com wrote:

> From: Hitoshi Mitake <mitake@clustcom.com>
> 
> 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 <mitake@clustcom.com>
> Signed-off-by: Doug Thompson <dougthompson@xmission.com>
> ---
> 
> 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.



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-05 22:29   ` Hitoshi Mitake
@ 2008-11-05 16:26     ` Doug Thompson
  2008-11-07  0:46       ` Andrew Morton
  0 siblings, 1 reply; 51+ messages in thread
From: Doug Thompson @ 2008-11-05 16:26 UTC (permalink / raw)
  To: Hitoshi Mitake, Andrew Morton; +Cc: dougthompson, linux-kernel, ktaka


--- Hitoshi Mitake <mitake@clustcom.com> wrote:

> On Mon, 20 Oct 2008 16:32:28 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Fri, 17 Oct 2008 15:39:45 -0600
> > dougthompson@xmission.com wrote:
> > 
> > > From: Hitoshi Mitake <mitake@clustcom.com>
> > > 
> > > 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 <mitake@clustcom.com>
> > > Signed-off-by: Doug Thompson <dougthompson@xmission.com>
> > > ---
> > > 
> > > 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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-10-20 23:32 ` Andrew Morton
@ 2008-11-05 22:29   ` Hitoshi Mitake
  2008-11-05 16:26     ` Doug Thompson
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-05 22:29 UTC (permalink / raw)
  To: Andrew Morton; +Cc: dougthompson, linux-kernel, ktaka

On Mon, 20 Oct 2008 16:32:28 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 17 Oct 2008 15:39:45 -0600
> dougthompson@xmission.com wrote:
> 
> > From: Hitoshi Mitake <mitake@clustcom.com>
> > 
> > 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 <mitake@clustcom.com>
> > Signed-off-by: Doug Thompson <dougthompson@xmission.com>
> > ---
> > 
> > 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 <mitake@clustcom.com>
Signed-off-by: Doug Thompson <dougthompson@xmission.com>
---

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");

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-05 16:26     ` Doug Thompson
@ 2008-11-07  0:46       ` Andrew Morton
  2008-11-07 15:28         ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: Andrew Morton @ 2008-11-07  0:46 UTC (permalink / raw)
  To: Doug Thompson; +Cc: mitake, dougthompson, linux-kernel, ktaka

On Wed, 5 Nov 2008 08:26:12 -0800 (PST)
Doug Thompson <norsk5@yahoo.com> 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.


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-07 15:28         ` Hitoshi Mitake
@ 2008-11-07  6:31           ` Andrew Morton
  2008-11-07 15:38             ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: Andrew Morton @ 2008-11-07  6:31 UTC (permalink / raw)
  To: Hitoshi Mitake; +Cc: Doug Thompson, dougthompson, linux-kernel, ktaka

On Fri, 7 Nov 2008 15:28:30 +0000 Hitoshi Mitake <mitake@clustcom.com> wrote:

> On Thu, 6 Nov 2008 16:46:41 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Wed, 5 Nov 2008 08:26:12 -0800 (PST)
> > Doug Thompson <norsk5@yahoo.com> 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 <mitake@clustcom.com>
> Signed-off-by: Doug Thompson <dougthompson@xmission.com>

We will need a description of this patch for the changelog, please.  We
always do.


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-07 15:38             ` Hitoshi Mitake
@ 2008-11-07  7:11               ` Andrew Morton
  2008-11-09 15:10                 ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: Andrew Morton @ 2008-11-07  7:11 UTC (permalink / raw)
  To: Hitoshi Mitake; +Cc: Doug Thompson, dougthompson, linux-kernel, ktaka

On Fri, 7 Nov 2008 15:38:24 +0000 Hitoshi Mitake <mitake@clustcom.com> 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 <mitake@clustcom.com>
> Signed-off-by: Doug Thompson <dougthompson@xmission.com>
> ---
> 
> 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.


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-07  0:46       ` Andrew Morton
@ 2008-11-07 15:28         ` Hitoshi Mitake
  2008-11-07  6:31           ` Andrew Morton
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-07 15:28 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Doug Thompson, dougthompson, linux-kernel, ktaka

On Thu, 6 Nov 2008 16:46:41 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 5 Nov 2008 08:26:12 -0800 (PST)
> Doug Thompson <norsk5@yahoo.com> 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 <mitake@clustcom.com>
Signed-off-by: Doug Thompson <dougthompson@xmission.com>
---

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);

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-07  6:31           ` Andrew Morton
@ 2008-11-07 15:38             ` Hitoshi Mitake
  2008-11-07  7:11               ` Andrew Morton
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-07 15:38 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Doug Thompson, dougthompson, linux-kernel, ktaka

On Thu, 6 Nov 2008 22:31:22 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Fri, 7 Nov 2008 15:28:30 +0000 Hitoshi Mitake <mitake@clustcom.com> wrote:
> 
> > On Thu, 6 Nov 2008 16:46:41 -0800
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > On Wed, 5 Nov 2008 08:26:12 -0800 (PST)
> > > Doug Thompson <norsk5@yahoo.com> 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 <mitake@clustcom.com>
> > Signed-off-by: Doug Thompson <dougthompson@xmission.com>
> 
> 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 <mitake@clustcom.com>
Signed-off-by: Doug Thompson <dougthompson@xmission.com>
---

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);

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-07  7:11               ` Andrew Morton
@ 2008-11-09 15:10                 ` Hitoshi Mitake
  2008-11-09 19:26                   ` Andrew Morton
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-09 15:10 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Hitoshi Mitake, Doug Thompson, dougthompson, linux-kernel, ktaka

I'm a same person as mitake@clustcom.com. h.mitake@gmail.com is my
private mail address.

On Fri, Nov 7, 2008 at 4:11 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 7 Nov 2008 15:38:24 +0000 Hitoshi Mitake <mitake@clustcom.com> 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 <mitake@clustcom.com>
>> Signed-off-by: Doug Thompson <dougthompson@xmission.com>
>> ---
>>
>> 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.)

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-09 15:10                 ` Hitoshi Mitake
@ 2008-11-09 19:26                   ` Andrew Morton
  2008-11-11  6:11                     ` Paul Mundt
  2008-11-18 12:16                     ` Ralf Baechle
  0 siblings, 2 replies; 51+ messages in thread
From: Andrew Morton @ 2008-11-09 19:26 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Hitoshi Mitake, Doug Thompson, dougthompson, linux-kernel, ktaka,
	linux-arch

(cc linux-arch)

On Mon, 10 Nov 2008 00:10:34 +0900 "Hitoshi Mitake" <h.mitake@gmail.com> 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.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-09 19:26                   ` Andrew Morton
@ 2008-11-11  6:11                     ` Paul Mundt
  2008-11-13 15:15                       ` Hitoshi Mitake
  2008-11-18 12:16                     ` Ralf Baechle
  1 sibling, 1 reply; 51+ messages in thread
From: Paul Mundt @ 2008-11-11  6:11 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Hitoshi Mitake, Hitoshi Mitake, Doug Thompson, dougthompson,
	linux-kernel, ktaka, linux-arch

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.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-11  6:11                     ` Paul Mundt
@ 2008-11-13 15:15                       ` Hitoshi Mitake
  0 siblings, 0 replies; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-13 15:15 UTC (permalink / raw)
  To: Paul Mundt
  Cc: Andrew Morton, Hitoshi Mitake, Doug Thompson, dougthompson,
	linux-kernel, ktaka, linux-arch

On Tue, 11 Nov 2008 15:11:40 +0900
Paul Mundt <lethal@linux-sh.org> 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.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-09 19:26                   ` Andrew Morton
  2008-11-11  6:11                     ` Paul Mundt
@ 2008-11-18 12:16                     ` Ralf Baechle
  2008-11-18 12:32                       ` Russell King
  1 sibling, 1 reply; 51+ messages in thread
From: Ralf Baechle @ 2008-11-18 12:16 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Hitoshi Mitake, Hitoshi Mitake, Doug Thompson, dougthompson,
	linux-kernel, ktaka, linux-arch

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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-18 12:16                     ` Ralf Baechle
@ 2008-11-18 12:32                       ` Russell King
  2008-11-20 16:19                         ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: Russell King @ 2008-11-18 12:32 UTC (permalink / raw)
  To: Ralf Baechle
  Cc: Andrew Morton, Hitoshi Mitake, Hitoshi Mitake, Doug Thompson,
	dougthompson, linux-kernel, ktaka, linux-arch

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:

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-18 12:32                       ` Russell King
@ 2008-11-20 16:19                         ` Hitoshi Mitake
  2008-11-23 23:52                           ` H. Peter Anvin
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-20 16:19 UTC (permalink / raw)
  To: Russell King
  Cc: Ralf Baechle, Andrew Morton, Doug Thompson, dougthompson,
	linux-kernel, linux-arch

On Tue, 18 Nov 2008 12:32:15 +0000
Russell King <rmk+lkml@arm.linux.org.uk> 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 <h.mitake@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-20 16:19                         ` Hitoshi Mitake
@ 2008-11-23 23:52                           ` H. Peter Anvin
  2008-11-24 17:18                             ` Luck, Tony
  0 siblings, 1 reply; 51+ messages in thread
From: H. Peter Anvin @ 2008-11-23 23:52 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* RE: [PATCH 1/1] edac x38: new MC driver module
  2008-11-23 23:52                           ` H. Peter Anvin
@ 2008-11-24 17:18                             ` Luck, Tony
  2008-11-24 18:02                               ` H. Peter Anvin
  0 siblings, 1 reply; 51+ messages in thread
From: Luck, Tony @ 2008-11-24 17:18 UTC (permalink / raw)
  To: H. Peter Anvin, Hitoshi Mitake
  Cc: Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 333 bytes --]

>       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Ç+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-24 17:18                             ` Luck, Tony
@ 2008-11-24 18:02                               ` H. Peter Anvin
  2008-11-25  2:55                                 ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: H. Peter Anvin @ 2008-11-24 18:02 UTC (permalink / raw)
  To: Luck, Tony
  Cc: Hitoshi Mitake, Russell King, Ralf Baechle, Andrew Morton,
	Doug Thompson, dougthompson, linux-kernel, linux-arch

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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-24 18:02                               ` H. Peter Anvin
@ 2008-11-25  2:55                                 ` Hitoshi Mitake
  2008-11-25  5:13                                   ` H. Peter Anvin
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-25  2:55 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Luck, Tony, Russell King, Ralf Baechle, Andrew Morton,
	Doug Thompson, dougthompson, linux-kernel, linux-arch

On Mon, 24 Nov 2008 10:02:40 -0800
"H. Peter Anvin" <hpa@zytor.com> 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 <h.mitake@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-25  2:55                                 ` Hitoshi Mitake
@ 2008-11-25  5:13                                   ` H. Peter Anvin
  2008-11-25 15:30                                     ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: H. Peter Anvin @ 2008-11-25  5:13 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Luck, Tony, Russell King, Ralf Baechle, Andrew Morton,
	Doug Thompson, dougthompson, linux-kernel, linux-arch

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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-25  5:13                                   ` H. Peter Anvin
@ 2008-11-25 15:30                                     ` Hitoshi Mitake
  2008-11-25 15:46                                       ` Geert Uytterhoeven
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-25 15:30 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Luck, Tony, Russell King, Ralf Baechle, Andrew Morton,
	Doug Thompson, dougthompson, linux-kernel, linux-arch

On Mon, 24 Nov 2008 21:13:58 -0800
"H. Peter Anvin" <hpa@zytor.com> 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);
}


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-25 15:30                                     ` Hitoshi Mitake
@ 2008-11-25 15:46                                       ` Geert Uytterhoeven
  2008-11-25 16:10                                         ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: Geert Uytterhoeven @ 2008-11-25 15:46 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: H. Peter Anvin, Luck, Tony, Russell King, Ralf Baechle,
	Andrew Morton, Doug Thompson, dougthompson, linux-kernel,
	linux-arch

On Wed, 26 Nov 2008, Hitoshi Mitake wrote:
> On Mon, 24 Nov 2008 21:13:58 -0800
> "H. Peter Anvin" <hpa@zytor.com> 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 -- geert@linux-m68k.org

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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-25 15:46                                       ` Geert Uytterhoeven
@ 2008-11-25 16:10                                         ` Hitoshi Mitake
  2008-11-29  0:11                                           ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-25 16:10 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: H. Peter Anvin, Luck, Tony, Russell King, Ralf Baechle,
	Andrew Morton, Doug Thompson, dougthompson, linux-kernel,
	linux-arch

On Tue, 25 Nov 2008 16:46:18 +0100 (CET)
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> On Wed, 26 Nov 2008, Hitoshi Mitake wrote:
> > On Mon, 24 Nov 2008 21:13:58 -0800
> > "H. Peter Anvin" <hpa@zytor.com> 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...


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-25 16:10                                         ` Hitoshi Mitake
@ 2008-11-29  0:11                                           ` Hitoshi Mitake
  0 siblings, 0 replies; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-29  0:11 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Geert Uytterhoeven, H. Peter Anvin, Luck, Tony, Russell King,
	Ralf Baechle, Andrew Morton, Doug Thompson, dougthompson,
	linux-kernel, linux-arch

On Wed, 26 Nov 2008 01:10:59 +0900
Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Tue, 25 Nov 2008 16:46:18 +0100 (CET)
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> > On Wed, 26 Nov 2008, Hitoshi Mitake wrote:
> > > On Mon, 24 Nov 2008 21:13:58 -0800
> > > "H. Peter Anvin" <hpa@zytor.com> 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 <h.mitake@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2009-02-21 13:09                               ` Sam Ravnborg
                                                   ` (4 preceding siblings ...)
  2009-02-22 14:20                                 ` Hitoshi Mitake
@ 2009-02-22 14:21                                 ` Hitoshi Mitake
  5 siblings, 0 replies; 51+ messages in thread
From: Hitoshi Mitake @ 2009-02-22 14:21 UTC (permalink / raw)
  To: Sam Ravnborg, davem
  Cc: Ingo Molnar, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 
> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/sparc/Kconfig |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index c3ea215..eb5efa3 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -527,3 +527,23 @@ source "security/Kconfig"
 source "crypto/Kconfig"
 
 source "lib/Kconfig"
+
+config HAVE_READQ
+	def_bool y
+	depends on SPARC64
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	depends on SPARC64
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2009-02-21 13:09                               ` Sam Ravnborg
                                                   ` (3 preceding siblings ...)
  2009-02-22 14:19                                 ` Hitoshi Mitake
@ 2009-02-22 14:20                                 ` Hitoshi Mitake
  2009-02-22 14:21                                 ` Hitoshi Mitake
  5 siblings, 0 replies; 51+ messages in thread
From: Hitoshi Mitake @ 2009-02-22 14:20 UTC (permalink / raw)
  To: Sam Ravnborg, lethal
  Cc: Ingo Molnar, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 
> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/sh/Kconfig |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/sh/Kconfig b/arch/sh/Kconfig
index ebabe51..7b1f664 100644
--- a/arch/sh/Kconfig
+++ b/arch/sh/Kconfig
@@ -699,3 +699,21 @@ source "security/Kconfig"
 source "crypto/Kconfig"
 
 source "lib/Kconfig"
+
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2009-02-21 13:09                               ` Sam Ravnborg
                                                   ` (2 preceding siblings ...)
  2009-02-22 14:18                                 ` Hitoshi Mitake
@ 2009-02-22 14:19                                 ` Hitoshi Mitake
  2009-02-22 14:20                                 ` Hitoshi Mitake
  2009-02-22 14:21                                 ` Hitoshi Mitake
  5 siblings, 0 replies; 51+ messages in thread
From: Hitoshi Mitake @ 2009-02-22 14:19 UTC (permalink / raw)
  To: Sam Ravnborg, benh
  Cc: Ingo Molnar, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 
> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/powerpc/Kconfig |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 74cc312..a517a8f 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -900,3 +900,21 @@ config PPC_LIB_RHEAP
 	bool
 
 source "arch/powerpc/kvm/Kconfig"
+
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2009-02-21 13:09                               ` Sam Ravnborg
  2009-02-22 14:15                                 ` Hitoshi Mitake
  2009-02-22 14:16                                 ` Hitoshi Mitake
@ 2009-02-22 14:18                                 ` Hitoshi Mitake
  2009-02-22 14:19                                 ` Hitoshi Mitake
                                                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 51+ messages in thread
From: Hitoshi Mitake @ 2009-02-22 14:18 UTC (permalink / raw)
  To: Sam Ravnborg, ralf
  Cc: Ingo Molnar, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 
> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

---
 arch/mips/Kconfig |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 600eef3..44aeeff 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -2132,3 +2132,21 @@ source "security/Kconfig"
 source "crypto/Kconfig"
 
 source "lib/Kconfig"
+
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2009-02-21 13:09                               ` Sam Ravnborg
  2009-02-22 14:15                                 ` Hitoshi Mitake
@ 2009-02-22 14:16                                 ` Hitoshi Mitake
  2009-02-22 14:18                                 ` Hitoshi Mitake
                                                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 51+ messages in thread
From: Hitoshi Mitake @ 2009-02-22 14:16 UTC (permalink / raw)
  To: Sam Ravnborg, tony.luck
  Cc: Ingo Molnar, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 
> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/ia64/Kconfig |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index 6183aec..2e035b0 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -691,3 +691,21 @@ config IOMMU_HELPER
 
 config IOMMU_API
 	def_bool (DMAR)
+
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2009-02-21 13:09                               ` Sam Ravnborg
@ 2009-02-22 14:15                                 ` Hitoshi Mitake
  2009-02-22 14:16                                 ` Hitoshi Mitake
                                                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 51+ messages in thread
From: Hitoshi Mitake @ 2009-02-22 14:15 UTC (permalink / raw)
  To: Sam Ravnborg, rth
  Cc: Ingo Molnar, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

On Sat, 21 Feb 2009 14:09:37 +0100
Sam Ravnborg <sam@ravnborg.org> wrote:

> > Hi,
> > 
> > Very sorry for long distance between my previous post and this...
> > 
> > I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> > which have readq() and writeq().
> > 
> > But there is problem.
> > I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> > accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> > but these helps are invisible when I doing menuconfig.
> > (when type '/' and search readq string, HAVE_READQ found, but
> > help string is not printed...)
> > 
> > Do you have some nice technique that make these helps visible easily?
> 

Thank you for your replying, Russell and Sam.

> The options are not visible in menuconfig and therefore the is not
> much point in displaying help for them thre when you search for the symbol.
> But the help contained in the KConfig file is fully justified as it is
> now documented why/when to select these options.
> 
> 	Sam

Hmm, so I try to send my patches to each maintainers of architectures.

description of the patch: Adding HAVE_READQ, HAVE_WRITEQ and their help texts
to each architecture's Kconfig file which have readq() and writeq().

Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/alpha/Kconfig |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig
index 9fb8aae..ecca919 100644
--- a/arch/alpha/Kconfig
+++ b/arch/alpha/Kconfig
@@ -672,3 +672,20 @@ source "crypto/Kconfig"
 
 source "lib/Kconfig"
 
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
-- 
1.6.1.2


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2009-02-21 10:11                             ` Hitoshi Mitake
  2009-02-21 10:39                               ` Russell King
@ 2009-02-21 13:09                               ` Sam Ravnborg
  2009-02-22 14:15                                 ` Hitoshi Mitake
                                                   ` (5 more replies)
  1 sibling, 6 replies; 51+ messages in thread
From: Sam Ravnborg @ 2009-02-21 13:09 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Ingo Molnar, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

> Hi,
> 
> Very sorry for long distance between my previous post and this...
> 
> I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
> which have readq() and writeq().
> 
> But there is problem.
> I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> but these helps are invisible when I doing menuconfig.
> (when type '/' and search readq string, HAVE_READQ found, but
> help string is not printed...)
> 
> Do you have some nice technique that make these helps visible easily?

The options are not visible in menuconfig and therefore the is not
much point in displaying help for them thre when you search for the symbol.
But the help contained in the KConfig file is fully justified as it is
now documented why/when to select these options.

	Sam

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2009-02-21 10:11                             ` Hitoshi Mitake
@ 2009-02-21 10:39                               ` Russell King
  2009-02-21 13:09                               ` Sam Ravnborg
  1 sibling, 0 replies; 51+ messages in thread
From: Russell King @ 2009-02-21 10:39 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Ingo Molnar, Sam Ravnborg, H. Peter Anvin, Geert Uytterhoeven,
	Luck, Tony, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

On Sat, Feb 21, 2009 at 07:11:01PM +0900, Hitoshi Mitake wrote:
> But there is problem.
> I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
> accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
> but these helps are invisible when I doing menuconfig.
> (when type '/' and search readq string, HAVE_READQ found, but
> help string is not printed...)

Yes, they will be, because users can't change the options.  The point
of providing a help text for them is so that when someone comes across
the options in a few months or years time, they know what the options
are (supposed to be) doing.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-12-01 23:58                           ` Hitoshi Mitake
  2008-12-04 15:58                             ` Hitoshi Mitake
@ 2009-02-21 10:11                             ` Hitoshi Mitake
  2009-02-21 10:39                               ` Russell King
  2009-02-21 13:09                               ` Sam Ravnborg
  1 sibling, 2 replies; 51+ messages in thread
From: Hitoshi Mitake @ 2009-02-21 10:11 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Ingo Molnar, Sam Ravnborg, H. Peter Anvin, Geert Uytterhoeven,
	Luck, Tony, Russell King, Ralf Baechle, Andrew Morton,
	Doug Thompson, dougthompson, linux-kernel, linux-arch

On Tue, 2 Dec 2008 08:58:19 +0900
"Hitoshi Mitake" <h.mitake@gmail.com> wrote:

> On Mon, Dec 1, 2008 at 22:59, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> >
> >> On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <mingo@elte.hu> wrote:
> >> >
> >> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> >> >
> >> >> On Sun, 30 Nov 2008 10:24:07 +0100
> >> >> Ingo Molnar <mingo@elte.hu> 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!


Hi,

Very sorry for long distance between my previous post and this...

I wrote a patch to add HAVE_READQ and HAVE_WRITEQ to each architecture's Kconfig file
which have readq() and writeq().

But there is problem.
I wrote helps for HAVE_READQ and HAVE_WRITEQ in Kconfig file
accodring to the advice by Russell King ( http://marc.info/?l=linux-kernel&m=122701161824218&w=2 ),
but these helps are invisible when I doing menuconfig.
(when type '/' and search readq string, HAVE_READQ found, but
help string is not printed...)

Do you have some nice technique that make these helps visible easily?
I paste a patch for x86 as sample. If I could find a good way,
I'll rewrite patches and send them to maintainers of each architectures.


Signed-off-by: Hitoshi Mitake <h.mitake@gmail.com>

---
 arch/x86/Kconfig |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 9c39095..d22f9a6 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
@@ -1980,6 +1978,25 @@ config HAVE_ATOMIC_IOMAP
 	def_bool y
 	depends on X86_32
 
+config HAVE_READQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  readq() function. readq() is a function to read 8 bytes from
+	  I/O space. Each drivers use readq() must depend on this symbol.
+	  Because lots of little private readq() implementations
+	  all over the tree is sucks.
+
+config HAVE_WRITEQ
+	def_bool y
+	help
+	  This is a sign to represent that this architecture provides
+	  writeq() function. writeq() is a function to read 8 bytes from
+	  I/O space. Each drivers use writeq() must depend on this symbol.
+	  Because lots of little private writeq() implementations
+	  all over the tree is sucks.
+
+
 source "net/Kconfig"
 
 source "drivers/Kconfig"
-- 
1.6.1.2



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-12-04 15:58                             ` Hitoshi Mitake
@ 2009-01-16  1:24                               ` Hitoshi Mitake
  0 siblings, 0 replies; 51+ messages in thread
From: Hitoshi Mitake @ 2009-01-16  1:24 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: linux-kernel


I wrote wrong mail address of LKML:-(
Resending, sorry.

On Fri, 5 Dec 2008 00:58:41 +0900
Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Tue, 2 Dec 2008 08:58:19 +0900
> "Hitoshi Mitake" <h.mitake@gmail.com> wrote:
> 
> > On Mon, Dec 1, 2008 at 22:59, Ingo Molnar <mingo@elte.hu> wrote:
> > >
> > > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> > >
> > >> On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <mingo@elte.hu> wrote:
> > >> >
> > >> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> > >> >
> > >> >> On Sun, 30 Nov 2008 10:24:07 +0100
> > >> >> Ingo Molnar <mingo@elte.hu> 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 <h.mitake@gmail.com>

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"

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-12-01 23:58                           ` Hitoshi Mitake
@ 2008-12-04 15:58                             ` Hitoshi Mitake
  2009-01-16  1:24                               ` Hitoshi Mitake
  2009-02-21 10:11                             ` Hitoshi Mitake
  1 sibling, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-12-04 15:58 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Ingo Molnar, Sam Ravnborg, H. Peter Anvin, Geert Uytterhoeven,
	Luck, Tony, Russell King, Ralf Baechle, Andrew Morton,
	Doug Thompson, dougthompson, linux-kernel, linux-arch

On Tue, 2 Dec 2008 08:58:19 +0900
"Hitoshi Mitake" <h.mitake@gmail.com> wrote:

> On Mon, Dec 1, 2008 at 22:59, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> >
> >> On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <mingo@elte.hu> wrote:
> >> >
> >> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> >> >
> >> >> On Sun, 30 Nov 2008 10:24:07 +0100
> >> >> Ingo Molnar <mingo@elte.hu> 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 <h.mitake@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-12-01 13:59                         ` Ingo Molnar
@ 2008-12-01 23:58                           ` Hitoshi Mitake
  2008-12-04 15:58                             ` Hitoshi Mitake
  2009-02-21 10:11                             ` Hitoshi Mitake
  0 siblings, 2 replies; 51+ messages in thread
From: Hitoshi Mitake @ 2008-12-01 23:58 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Sam Ravnborg, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

On Mon, Dec 1, 2008 at 22:59, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Hitoshi Mitake <h.mitake@gmail.com> wrote:
>
>> On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <mingo@elte.hu> wrote:
>> >
>> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
>> >
>> >> On Sun, 30 Nov 2008 10:24:07 +0100
>> >> Ingo Molnar <mingo@elte.hu> 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!

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-12-01 13:51                       ` Hitoshi Mitake
@ 2008-12-01 13:59                         ` Ingo Molnar
  2008-12-01 23:58                           ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: Ingo Molnar @ 2008-12-01 13:59 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Sam Ravnborg, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch


* Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Hitoshi Mitake <h.mitake@gmail.com> wrote:
> >
> >> On Sun, 30 Nov 2008 10:24:07 +0100
> >> Ingo Molnar <mingo@elte.hu> 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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-30 16:15                     ` Ingo Molnar
@ 2008-12-01 13:51                       ` Hitoshi Mitake
  2008-12-01 13:59                         ` Ingo Molnar
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-12-01 13:51 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Sam Ravnborg, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

On Mon, Dec 1, 2008 at 01:15, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Hitoshi Mitake <h.mitake@gmail.com> wrote:
>
>> On Sun, 30 Nov 2008 10:24:07 +0100
>> Ingo Molnar <mingo@elte.hu> 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.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-30 15:20                   ` Hitoshi Mitake
@ 2008-11-30 16:15                     ` Ingo Molnar
  2008-12-01 13:51                       ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: Ingo Molnar @ 2008-11-30 16:15 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Sam Ravnborg, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch


* Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Sun, 30 Nov 2008 10:24:07 +0100
> Ingo Molnar <mingo@elte.hu> 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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-30  9:24                 ` Ingo Molnar
@ 2008-11-30 15:20                   ` Hitoshi Mitake
  2008-11-30 16:15                     ` Ingo Molnar
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-30 15:20 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Sam Ravnborg, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

On Sun, 30 Nov 2008 10:24:07 +0100
Ingo Molnar <mingo@elte.hu> 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.

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-30  8:37               ` Ingo Molnar
@ 2008-11-30  9:24                 ` Ingo Molnar
  2008-11-30 15:20                   ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: Ingo Molnar @ 2008-11-30  9:24 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Sam Ravnborg, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch


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 <mingo@elte.hu>
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 <mingo@elte.hu>
---
 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;
 

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-30  8:16             ` Hitoshi Mitake
@ 2008-11-30  8:37               ` Ingo Molnar
  2008-11-30  9:24                 ` Ingo Molnar
  0 siblings, 1 reply; 51+ messages in thread
From: Ingo Molnar @ 2008-11-30  8:37 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Sam Ravnborg, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch


* Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Sat, 29 Nov 2008 19:01:44 +0100
> Sam Ravnborg <sam@ravnborg.org> 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 <mingo@elte.hu>
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 <mingo@elte.hu>
---
 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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-29 18:01           ` Sam Ravnborg
@ 2008-11-30  8:16             ` Hitoshi Mitake
  2008-11-30  8:37               ` Ingo Molnar
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-30  8:16 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Ingo Molnar, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

On Sat, 29 Nov 2008 19:01:44 +0100
Sam Ravnborg <sam@ravnborg.org> 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 <h.mitake@gmail.com>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
---
 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


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-29 13:24         ` Hitoshi Mitake
@ 2008-11-29 18:01           ` Sam Ravnborg
  2008-11-30  8:16             ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: Sam Ravnborg @ 2008-11-29 18:01 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Ingo Molnar, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

On Sat, Nov 29, 2008 at 10:24:49PM +0900, Hitoshi Mitake wrote:
> On Sat, 29 Nov 2008 11:52:29 +0100
> Sam Ravnborg <sam@ravnborg.org> 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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-29 10:52       ` Sam Ravnborg
@ 2008-11-29 13:24         ` Hitoshi Mitake
  2008-11-29 18:01           ` Sam Ravnborg
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-29 13:24 UTC (permalink / raw)
  To: Sam Ravnborg
  Cc: Ingo Molnar, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

On Sat, 29 Nov 2008 11:52:29 +0100
Sam Ravnborg <sam@ravnborg.org> 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 <h.mitake@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-29 10:26     ` Hitoshi Mitake
@ 2008-11-29 10:52       ` Sam Ravnborg
  2008-11-29 13:24         ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: Sam Ravnborg @ 2008-11-29 10:52 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Ingo Molnar, H. Peter Anvin, Geert Uytterhoeven, Luck, Tony,
	Russell King, Ralf Baechle, Andrew Morton, Doug Thompson,
	dougthompson, linux-kernel, linux-arch

> 
> 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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-29  9:38   ` Ingo Molnar
@ 2008-11-29 10:26     ` Hitoshi Mitake
  2008-11-29 10:52       ` Sam Ravnborg
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-29 10:26 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: H. Peter Anvin, Geert Uytterhoeven, Luck, Tony, Russell King,
	Ralf Baechle, Andrew Morton, Doug Thompson, dougthompson,
	linux-kernel, linux-arch

On Sat, 29 Nov 2008 10:38:58 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Hitoshi Mitake <h.mitake@gmail.com> 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?

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-29  7:47 ` Hitoshi Mitake
@ 2008-11-29  9:38   ` Ingo Molnar
  2008-11-29 10:26     ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: Ingo Molnar @ 2008-11-29  9:38 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: H. Peter Anvin, Geert Uytterhoeven, Luck, Tony, Russell King,
	Ralf Baechle, Andrew Morton, Doug Thompson, dougthompson,
	linux-kernel, linux-arch


* Hitoshi Mitake <h.mitake@gmail.com> 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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 1/1] edac x38: new MC driver module
  2008-11-29  0:56 H. Peter Anvin
@ 2008-11-29  7:47 ` Hitoshi Mitake
  2008-11-29  9:38   ` Ingo Molnar
  0 siblings, 1 reply; 51+ messages in thread
From: Hitoshi Mitake @ 2008-11-29  7:47 UTC (permalink / raw)
  To: H. Peter Anvin
  Cc: Geert Uytterhoeven, Luck, Tony, Russell King, Ralf Baechle,
	Andrew Morton, Doug Thompson, dougthompson, linux-kernel,
	linux-arch

On Fri, 28 Nov 2008 16:56:39 -0800
"H. Peter Anvin" <hpa@zytor.com> 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 <h.mitake@gmail.com>
---
 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


^ permalink raw reply	[flat|nested] 51+ messages in thread

* RE: [PATCH 1/1] edac x38: new MC driver module
@ 2008-11-29  0:56 H. Peter Anvin
  2008-11-29  7:47 ` Hitoshi Mitake
  0 siblings, 1 reply; 51+ messages in thread
From: H. Peter Anvin @ 2008-11-29  0:56 UTC (permalink / raw)
  To: Hitoshi Mitake
  Cc: Geert Uytterhoeven, Luck, Tony, Russell King, Ralf Baechle,
	Andrew Morton, Doug Thompson, dougthompson, linux-kernel,
	linux-arch

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 <h.mitake@gmail.com>
Sent: Friday, November 28, 2008 16:11
To: Hitoshi Mitake <h.mitake@gmail.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>; H. Peter Anvin <hpa@zytor.com>; Luck, Tony <tony.luck@intel.com>; Russell King <rmk+lkml@arm.linux.org.uk>; Ralf Baechle <ralf@linux-mips.org>; Andrew Morton <akpm@linux-foundation.org>; Doug Thompson <norsk5@yahoo.com>; dougthompson@xmission.com <dougthompson@xmission.com>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; linux-arch@vger.kernel.org <linux-arch@vger.kernel.org>
Subject: Re: [PATCH 1/1] edac x38: new MC driver module

On Wed, 26 Nov 2008 01:10:59 +0900
Hitoshi Mitake <h.mitake@gmail.com> wrote:

> On Tue, 25 Nov 2008 16:46:18 +0100 (CET)
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> > On Wed, 26 Nov 2008, Hitoshi Mitake wrote:
> > > On Mon, 24 Nov 2008 21:13:58 -0800
> > > "H. Peter Anvin" <hpa@zytor.com> 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 <h.mitake@gmail.com>
---
 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



^ permalink raw reply	[flat|nested] 51+ messages in thread

end of thread, other threads:[~2009-02-22 14:21 UTC | newest]

Thread overview: 51+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-17 21:39 [PATCH 1/1] edac x38: new MC driver module dougthompson
2008-10-20 23:32 ` Andrew Morton
2008-11-05 22:29   ` Hitoshi Mitake
2008-11-05 16:26     ` Doug Thompson
2008-11-07  0:46       ` Andrew Morton
2008-11-07 15:28         ` Hitoshi Mitake
2008-11-07  6:31           ` Andrew Morton
2008-11-07 15:38             ` Hitoshi Mitake
2008-11-07  7:11               ` Andrew Morton
2008-11-09 15:10                 ` Hitoshi Mitake
2008-11-09 19:26                   ` Andrew Morton
2008-11-11  6:11                     ` Paul Mundt
2008-11-13 15:15                       ` Hitoshi Mitake
2008-11-18 12:16                     ` Ralf Baechle
2008-11-18 12:32                       ` Russell King
2008-11-20 16:19                         ` Hitoshi Mitake
2008-11-23 23:52                           ` H. Peter Anvin
2008-11-24 17:18                             ` Luck, Tony
2008-11-24 18:02                               ` H. Peter Anvin
2008-11-25  2:55                                 ` Hitoshi Mitake
2008-11-25  5:13                                   ` H. Peter Anvin
2008-11-25 15:30                                     ` Hitoshi Mitake
2008-11-25 15:46                                       ` Geert Uytterhoeven
2008-11-25 16:10                                         ` Hitoshi Mitake
2008-11-29  0:11                                           ` Hitoshi Mitake
2008-11-29  0:56 H. Peter Anvin
2008-11-29  7:47 ` Hitoshi Mitake
2008-11-29  9:38   ` Ingo Molnar
2008-11-29 10:26     ` Hitoshi Mitake
2008-11-29 10:52       ` Sam Ravnborg
2008-11-29 13:24         ` Hitoshi Mitake
2008-11-29 18:01           ` Sam Ravnborg
2008-11-30  8:16             ` Hitoshi Mitake
2008-11-30  8:37               ` Ingo Molnar
2008-11-30  9:24                 ` Ingo Molnar
2008-11-30 15:20                   ` Hitoshi Mitake
2008-11-30 16:15                     ` Ingo Molnar
2008-12-01 13:51                       ` Hitoshi Mitake
2008-12-01 13:59                         ` Ingo Molnar
2008-12-01 23:58                           ` Hitoshi Mitake
2008-12-04 15:58                             ` Hitoshi Mitake
2009-01-16  1:24                               ` Hitoshi Mitake
2009-02-21 10:11                             ` Hitoshi Mitake
2009-02-21 10:39                               ` Russell King
2009-02-21 13:09                               ` Sam Ravnborg
2009-02-22 14:15                                 ` Hitoshi Mitake
2009-02-22 14:16                                 ` Hitoshi Mitake
2009-02-22 14:18                                 ` Hitoshi Mitake
2009-02-22 14:19                                 ` Hitoshi Mitake
2009-02-22 14:20                                 ` Hitoshi Mitake
2009-02-22 14:21                                 ` Hitoshi Mitake

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).