From: Mac Lin <[email protected]>
v2:
- Changing CNS3XXX to cns3xxx in subjec for part of the patches are filtered due to "capital Triple-X in subject".
v1:
The first patch prepare the mach-cns3xxx power management code for drivers use.
The other two patches add the bus glue for EHCI/OHCI controller in mach-cns3xxx
and usb.
This patchset is based on linux-2.6.37-rc2
Mac Lin (3):
ARM: cns3xxx: Add new and export the old power management functions
ARM: cns3xxx: Add architecture definition for EHCI/OHCI controller
USB: cns3xxx: Add EHCI and OHCI bus glue for cns3xxx SOCs
arch/arm/mach-cns3xxx/cns3420vb.c | 50 ++++++++
arch/arm/mach-cns3xxx/core.h | 4 +-
arch/arm/mach-cns3xxx/include/mach/cns3xxx.h | 2 -
arch/arm/mach-cns3xxx/include/mach/pm.h | 21 +++
arch/arm/mach-cns3xxx/pm.c | 21 +++
drivers/usb/Kconfig | 2 +
drivers/usb/host/Kconfig | 15 +++
drivers/usb/host/ehci-cns3xxx.c | 176 ++++++++++++++++++++++++++
drivers/usb/host/ehci-hcd.c | 5 +
drivers/usb/host/ohci-cns3xxx.c | 171 +++++++++++++++++++++++++
drivers/usb/host/ohci-hcd.c | 5 +
11 files changed, 468 insertions(+), 4 deletions(-)
create mode 100644 arch/arm/mach-cns3xxx/include/mach/pm.h
create mode 100644 drivers/usb/host/ehci-cns3xxx.c
create mode 100644 drivers/usb/host/ohci-cns3xxx.c
From: Mac Lin <[email protected]>
This patch add cns3xxx_pwr_clk_dis, and export thoes power management functions
that may be used by many other device drivers on CNS3XXX.
Signed-off-by: Mac Lin <[email protected]>
---
arch/arm/mach-cns3xxx/core.h | 4 ++--
arch/arm/mach-cns3xxx/include/mach/pm.h | 19 +++++++++++++++++++
arch/arm/mach-cns3xxx/pm.c | 17 +++++++++++++++++
3 files changed, 38 insertions(+), 2 deletions(-)
create mode 100644 arch/arm/mach-cns3xxx/include/mach/pm.h
diff --git a/arch/arm/mach-cns3xxx/core.h b/arch/arm/mach-cns3xxx/core.h
index 6b33ec1..c6636bd 100644
--- a/arch/arm/mach-cns3xxx/core.h
+++ b/arch/arm/mach-cns3xxx/core.h
@@ -11,13 +11,13 @@
#ifndef __CNS3XXX_CORE_H
#define __CNS3XXX_CORE_H
+#include <mach/pm.h>
+
extern void __iomem *gic_cpu_base_addr;
extern struct sys_timer cns3xxx_timer;
void __init cns3xxx_map_io(void);
void __init cns3xxx_init_irq(void);
void cns3xxx_power_off(void);
-void cns3xxx_pwr_power_up(unsigned int block);
-void cns3xxx_pwr_power_down(unsigned int block);
#endif /* __CNS3XXX_CORE_H */
diff --git a/arch/arm/mach-cns3xxx/include/mach/pm.h b/arch/arm/mach-cns3xxx/include/mach/pm.h
new file mode 100644
index 0000000..102617b
--- /dev/null
+++ b/arch/arm/mach-cns3xxx/include/mach/pm.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright 2000 Deep Blue Solutions Ltd
+ * Copyright 2004 ARM Limited
+ * Copyright 2008 Cavium Networks
+ *
+ * This file is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, Version 2, as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __CNS3XXX_PM_H
+#define __CNS3XXX_PM_H
+
+void cns3xxx_pwr_clk_en(unsigned int block);
+void cns3xxx_pwr_clk_dis(unsigned int block);
+void cns3xxx_pwr_power_up(unsigned int block);
+void cns3xxx_pwr_power_down(unsigned int block);
+
+#endif /* __CNS3XXX_PM_H */
diff --git a/arch/arm/mach-cns3xxx/pm.c b/arch/arm/mach-cns3xxx/pm.c
index 38e4470..a4495e6 100644
--- a/arch/arm/mach-cns3xxx/pm.c
+++ b/arch/arm/mach-cns3xxx/pm.c
@@ -6,6 +6,8 @@
* published by the Free Software Foundation.
*/
+#include <linux/init.h>
+#include <linux/module.h>
#include <linux/io.h>
#include <linux/delay.h>
#include <mach/system.h>
@@ -18,6 +20,16 @@ void cns3xxx_pwr_clk_en(unsigned int block)
reg |= (block & PM_CLK_GATE_REG_MASK);
__raw_writel(reg, PM_CLK_GATE_REG);
}
+EXPORT_SYMBOL(cns3xxx_pwr_clk_en);
+
+void cns3xxx_pwr_clk_dis(unsigned int block)
+{
+ u32 reg = __raw_readl(PM_CLK_GATE_REG);
+
+ reg &= ~(block & PM_CLK_GATE_REG_MASK);
+ __raw_writel(reg, PM_CLK_GATE_REG);
+}
+EXPORT_SYMBOL(cns3xxx_pwr_clk_dis);
void cns3xxx_pwr_power_up(unsigned int block)
{
@@ -29,6 +41,7 @@ void cns3xxx_pwr_power_up(unsigned int block)
/* Wait for 300us for the PLL output clock locked. */
udelay(300);
};
+EXPORT_SYMBOL(cns3xxx_pwr_power_up);
void cns3xxx_pwr_power_down(unsigned int block)
{
@@ -38,6 +51,7 @@ void cns3xxx_pwr_power_down(unsigned int block)
reg |= (block & CNS3XXX_PWR_PLL_ALL);
__raw_writel(reg, PM_PLL_HM_PD_CTRL_REG);
};
+EXPORT_SYMBOL(cns3xxx_pwr_power_down);
static void cns3xxx_pwr_soft_rst_force(unsigned int block)
{
@@ -56,6 +70,7 @@ static void cns3xxx_pwr_soft_rst_force(unsigned int block)
__raw_writel(reg, PM_SOFT_RST_REG);
}
+EXPORT_SYMBOL(cns3xxx_pwr_soft_rst_force);
void cns3xxx_pwr_soft_rst(unsigned int block)
{
@@ -69,6 +84,7 @@ void cns3xxx_pwr_soft_rst(unsigned int block)
}
cns3xxx_pwr_soft_rst_force(block);
}
+EXPORT_SYMBOL(cns3xxx_pwr_soft_rst);
void arch_reset(char mode, const char *cmd)
{
@@ -99,3 +115,4 @@ int cns3xxx_cpu_clock(void)
return cpu;
}
+EXPORT_SYMBOL(cns3xxx_cpu_clock);
--
1.7.3
From: Mac Lin <[email protected]>
This patch add plateform_device for EHCI and OHCI controller on CNS3XXX. Power
reference count (usb_pwr_ref) is used to control enabling and disabling the
single clock control for both EHCI and OHCI controller.
It also remove EHCI/OHCI unused virtual address definitions.
Signed-off-by: Mac Lin <[email protected]>
---
arch/arm/mach-cns3xxx/cns3420vb.c | 50 ++++++++++++++++++++++++++
arch/arm/mach-cns3xxx/include/mach/cns3xxx.h | 2 -
arch/arm/mach-cns3xxx/include/mach/pm.h | 2 +
arch/arm/mach-cns3xxx/pm.c | 4 ++
4 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-cns3xxx/cns3420vb.c b/arch/arm/mach-cns3xxx/cns3420vb.c
index 90fe9ab..4824223 100644
--- a/arch/arm/mach-cns3xxx/cns3420vb.c
+++ b/arch/arm/mach-cns3xxx/cns3420vb.c
@@ -107,11 +107,61 @@ static void __init cns3420_early_serial_setup(void)
#endif
}
+static struct resource cns3xxx_usb_ehci_resource[] = {
+ [0] = {
+ .start = CNS3XXX_USB_BASE,
+ .end = CNS3XXX_USB_BASE + SZ_16M - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ [1] = {
+ .start = IRQ_CNS3XXX_USB_EHCI,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static u64 cns3xxx_usb_dma_mask = 0xffffffffULL;
+
+static struct platform_device cns3xxx_usb_ehci_device = {
+ .name = "cns3xxx-ehci",
+ .num_resources = ARRAY_SIZE(cns3xxx_usb_ehci_resource),
+ .resource = cns3xxx_usb_ehci_resource,
+ .dev = {
+ .dma_mask = &cns3xxx_usb_dma_mask,
+ .coherent_dma_mask = 0xffffffffULL,
+ },
+};
+
+static struct resource cns3xxx_usb_ohci_resource[] = {
+ [0] = {
+ .start = CNS3XXX_USB_OHCI_BASE,
+ .end = CNS3XXX_USB_OHCI_BASE + SZ_16M - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ [1] = {
+ .start = IRQ_CNS3XXX_USB_OHCI,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static u64 cns3xxx_usb_ohci_dma_mask = 0xffffffffULL;
+static struct platform_device cns3xxx_usb_ohci_device = {
+ .name = "cns3xxx-ohci",
+ .dev = {
+ .dma_mask = &cns3xxx_usb_ohci_dma_mask,
+ .coherent_dma_mask = 0xffffffffULL,
+ },
+ .num_resources = 2,
+ .resource = cns3xxx_usb_ohci_resource,
+};
+
+
/*
* Initialization
*/
static struct platform_device *cns3420_pdevs[] __initdata = {
&cns3420_nor_pdev,
+ &cns3xxx_usb_ehci_device,
+ &cns3xxx_usb_ohci_device,
};
static void __init cns3420_init(void)
diff --git a/arch/arm/mach-cns3xxx/include/mach/cns3xxx.h b/arch/arm/mach-cns3xxx/include/mach/cns3xxx.h
index 6dbce13..191c8e5 100644
--- a/arch/arm/mach-cns3xxx/include/mach/cns3xxx.h
+++ b/arch/arm/mach-cns3xxx/include/mach/cns3xxx.h
@@ -165,7 +165,6 @@
#define CNS3XXX_USBOTG_BASE_VIRT 0xFFF15000
#define CNS3XXX_USB_BASE 0x82000000 /* USB Host Control */
-#define CNS3XXX_USB_BASE_VIRT 0xFFF16000
#define CNS3XXX_SATA2_BASE 0x83000000 /* SATA */
#define CNS3XXX_SATA2_SIZE SZ_16M
@@ -184,7 +183,6 @@
#define CNS3XXX_2DG_BASE_VIRT 0xFFF1B000
#define CNS3XXX_USB_OHCI_BASE 0x88000000 /* USB OHCI */
-#define CNS3XXX_USB_OHCI_BASE_VIRT 0xFFF1C000
#define CNS3XXX_L2C_BASE 0x92000000 /* L2 Cache Control */
#define CNS3XXX_L2C_BASE_VIRT 0xFFF27000
diff --git a/arch/arm/mach-cns3xxx/include/mach/pm.h b/arch/arm/mach-cns3xxx/include/mach/pm.h
index 102617b..ddbe7d7 100644
--- a/arch/arm/mach-cns3xxx/include/mach/pm.h
+++ b/arch/arm/mach-cns3xxx/include/mach/pm.h
@@ -16,4 +16,6 @@ void cns3xxx_pwr_clk_dis(unsigned int block);
void cns3xxx_pwr_power_up(unsigned int block);
void cns3xxx_pwr_power_down(unsigned int block);
+extern atomic_t usb_pwr_ref;
+
#endif /* __CNS3XXX_PM_H */
diff --git a/arch/arm/mach-cns3xxx/pm.c b/arch/arm/mach-cns3xxx/pm.c
index a4495e6..784af93 100644
--- a/arch/arm/mach-cns3xxx/pm.c
+++ b/arch/arm/mach-cns3xxx/pm.c
@@ -10,6 +10,7 @@
#include <linux/module.h>
#include <linux/io.h>
#include <linux/delay.h>
+#include <asm/atomic.h>
#include <mach/system.h>
#include <mach/cns3xxx.h>
@@ -116,3 +117,6 @@ int cns3xxx_cpu_clock(void)
return cpu;
}
EXPORT_SYMBOL(cns3xxx_cpu_clock);
+
+atomic_t usb_pwr_ref = ATOMIC_INIT(0);
+EXPORT_SYMBOL(usb_pwr_ref);
--
1.7.3
From: Mac Lin <[email protected]>
The CNS3XXX SOC has include USB EHCI and OHCI compatible controllers. This
patch adds the necessary glue logic to allow ehci-hcd and ohci-hcd drivers to
work on CNS3XXX
The EHCI and OHCI controllers share a common clock control and reset bit,
therefore additional check for the timming of enabling and disabling is
required. The USB bit of PLL Power Down Control is also shared by OTG, 24MHz
UART clock, Crypto clock, PCIe reference clock, and Clock Scale Generator.
Therefore we only ensure it is enabled, while not disabling it.
Signed-off-by: Mac Lin <[email protected]>
---
drivers/usb/Kconfig | 2 +
drivers/usb/host/Kconfig | 15 ++++
drivers/usb/host/ehci-cns3xxx.c | 176 +++++++++++++++++++++++++++++++++++++++
drivers/usb/host/ehci-hcd.c | 5 +
drivers/usb/host/ohci-cns3xxx.c | 171 +++++++++++++++++++++++++++++++++++++
drivers/usb/host/ohci-hcd.c | 5 +
6 files changed, 374 insertions(+), 0 deletions(-)
create mode 100644 drivers/usb/host/ehci-cns3xxx.c
create mode 100644 drivers/usb/host/ohci-cns3xxx.c
diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 67eb377..5a7c8f1 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -41,6 +41,7 @@ config USB_ARCH_HAS_OHCI
default y if MFD_TC6393XB
default y if ARCH_W90X900
default y if ARCH_DAVINCI_DA8XX
+ default y if ARCH_CNS3XXX
# PPC:
default y if STB03xxx
default y if PPC_MPC52xx
@@ -66,6 +67,7 @@ config USB_ARCH_HAS_EHCI
default y if ARCH_AT91SAM9G45
default y if ARCH_MXC
default y if ARCH_OMAP3
+ default y if ARCH_CNS3XXX
default PCI
# ARM SA1111 chips have a non-PCI based "OHCI-compatible" USB host interface.
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 6f4f8e6..f8970d1 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -147,6 +147,14 @@ config USB_W90X900_EHCI
---help---
Enables support for the W90X900 USB controller
+config USB_CNS3XXX_EHCI
+ bool "Cavium CNS3XXX EHCI Module"
+ depends on USB_EHCI_HCD && ARCH_CNS3XXX
+ ---help---
+ Enable support for the CNS3XXX SOC's on-chip EHCI controller.
+ It is needed for high-speed (480Mbit/sec) USB 2.0 device
+ support.
+
config USB_OXU210HP_HCD
tristate "OXU210HP HCD support"
depends on USB
@@ -286,6 +294,13 @@ config USB_OHCI_HCD_SSB
If unsure, say N.
+config USB_CNS3XXX_OHCI
+ bool "Cavium CNS3XXX OHCI Module"
+ depends on USB_OHCI_HCD && ARCH_CNS3XXX
+ ---help---
+ Enable support for the CNS3XXX SOC's on-chip OHCI controller.
+ It is needed for low-speed USB 1.0 device support.
+
config USB_OHCI_BIG_ENDIAN_DESC
bool
depends on USB_OHCI_HCD
diff --git a/drivers/usb/host/ehci-cns3xxx.c b/drivers/usb/host/ehci-cns3xxx.c
new file mode 100644
index 0000000..87c0ee8
--- /dev/null
+++ b/drivers/usb/host/ehci-cns3xxx.c
@@ -0,0 +1,176 @@
+/*
+ * Copyright 2008 Cavium Networks
+ *
+ * This file is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, Version 2, as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/platform_device.h>
+#include <asm/atomic.h>
+#include <mach/cns3xxx.h>
+#include <mach/pm.h>
+
+static int cns3xxx_ehci_init(struct usb_hcd *hcd)
+{
+ struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+ int retval = 0;
+
+ /* EHCI and OHCI share the same clock and power,
+ * resetting twice would cause the 1st controller been reset.
+ * Therefore only do power up at the first up device, and
+ * power down at the last down device.
+ */
+ if (1 == atomic_inc_return(&usb_pwr_ref)) {
+ cns3xxx_pwr_power_up(1<<PM_PLL_HM_PD_CTRL_REG_OFFSET_PLL_USB);
+ cns3xxx_pwr_clk_en(1<<PM_CLK_GATE_REG_OFFSET_USB_HOST);
+ cns3xxx_pwr_soft_rst(1<<PM_SOFT_RST_REG_OFFST_USB_HOST);
+ }
+
+ ehci->caps = hcd->regs;
+ ehci->regs = hcd->regs
+ + HC_LENGTH(ehci_readl(ehci, &ehci->caps->hc_capbase));
+ ehci->hcs_params = ehci_readl(ehci, &ehci->caps->hcs_params);
+
+ hcd->has_tt = 0;
+ ehci_reset(ehci);
+
+ retval = ehci_init(hcd);
+ if (retval)
+ return retval;
+
+ ehci_port_power(ehci, 0);
+
+ return retval;
+}
+
+static const struct hc_driver cns3xxx_ehci_hc_driver = {
+ .description = hcd_name,
+ .product_desc = "CNS3XXX EHCI Host Controller",
+ .hcd_priv_size = sizeof(struct ehci_hcd),
+ .irq = ehci_irq,
+ .flags = HCD_MEMORY | HCD_USB2,
+ .reset = cns3xxx_ehci_init,
+ .start = ehci_run,
+ .stop = ehci_stop,
+ .shutdown = ehci_shutdown,
+ .urb_enqueue = ehci_urb_enqueue,
+ .urb_dequeue = ehci_urb_dequeue,
+ .endpoint_disable = ehci_endpoint_disable,
+ .endpoint_reset = ehci_endpoint_reset,
+ .get_frame_number = ehci_get_frame,
+ .hub_status_data = ehci_hub_status_data,
+ .hub_control = ehci_hub_control,
+#ifdef CONFIG_PM
+ .bus_suspend = ehci_bus_suspend,
+ .bus_resume = ehci_bus_resume,
+#endif
+ .relinquish_port = ehci_relinquish_port,
+ .port_handed_over = ehci_port_handed_over,
+
+ .clear_tt_buffer_complete = ehci_clear_tt_buffer_complete,
+};
+
+static int cns3xxx_ehci_probe(struct platform_device *pdev)
+{
+ struct usb_hcd *hcd;
+ const struct hc_driver *driver = &cns3xxx_ehci_hc_driver;
+ struct resource *res;
+ int irq;
+ int retval;
+
+ if (usb_disabled())
+ return -ENODEV;
+
+ res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ if (!res) {
+ dev_err(&pdev->dev,
+ "Found HC with no IRQ. Check %s setup!\n",
+ dev_name(&pdev->dev));
+ return -ENODEV;
+ }
+ irq = res->start;
+
+ hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
+ if (!hcd)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev,
+ "Found HC with no register addr. Check %s setup!\n",
+ dev_name(&pdev->dev));
+ retval = -ENODEV;
+ goto err1;
+ }
+ hcd->rsrc_start = res->start;
+ hcd->rsrc_len = res->end - res->start + 1;
+
+#ifdef CNS3XXX_USB_BASE_VIRT
+ hcd->regs = (void __iomem *) CNS3XXX_USB_BASE_VIRT;
+#else
+ if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len,
+ driver->description)) {
+ dev_dbg(&pdev->dev, "controller already in use\n");
+ retval = -EBUSY;
+ goto err1;
+ }
+
+ hcd->regs = ioremap(hcd->rsrc_start, hcd->rsrc_len);
+
+ if (hcd->regs == NULL) {
+ dev_dbg(&pdev->dev, "error mapping memory\n");
+ retval = -EFAULT;
+ goto err2;
+ }
+#endif
+
+ retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
+ if (retval == 0)
+ return retval;
+
+#ifndef CNS3XXX_USB_BASE_VIRT
+ iounmap(hcd->regs);
+err2:
+ release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
+#endif
+err1:
+ usb_put_hcd(hcd);
+
+ return retval;
+}
+
+static int cns3xxx_ehci_remove(struct platform_device *pdev)
+{
+ struct usb_hcd *hcd = platform_get_drvdata(pdev);
+
+ usb_remove_hcd(hcd);
+#ifndef CNS3XXX_USB_BASE_VIRT
+ iounmap(hcd->regs);
+ release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
+#endif
+
+ /* EHCI and OHCI share the same clock and power,
+ * resetting twice would cause the 1st controller been reset.
+ * Therefore only do power up at the first up device, and
+ * power down at the last down device.
+ */
+ if (0 == atomic_dec_return(&usb_pwr_ref))
+ cns3xxx_pwr_clk_dis(1<<PM_CLK_GATE_REG_OFFSET_USB_HOST);
+
+ usb_put_hcd(hcd);
+
+ platform_set_drvdata(pdev, NULL);
+
+ return 0;
+}
+
+MODULE_ALIAS("platform:cns3xxx-ehci");
+
+static struct platform_driver cns3xxx_ehci_driver = {
+ .probe = cns3xxx_ehci_probe,
+ .remove = cns3xxx_ehci_remove,
+ .driver = {
+ .name = "cns3xxx-ehci",
+ },
+};
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 502a7e6..c5dd1c3 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1216,6 +1216,11 @@ MODULE_LICENSE ("GPL");
#define PLATFORM_DRIVER ehci_octeon_driver
#endif
+#ifdef CONFIG_USB_CNS3XXX_EHCI
+#include "ehci-cns3xxx.c"
+#define PLATFORM_DRIVER cns3xxx_ehci_driver
+#endif
+
#if !defined(PCI_DRIVER) && !defined(PLATFORM_DRIVER) && \
!defined(PS3_SYSTEM_BUS_DRIVER) && !defined(OF_PLATFORM_DRIVER) && \
!defined(XILINX_OF_PLATFORM_DRIVER)
diff --git a/drivers/usb/host/ohci-cns3xxx.c b/drivers/usb/host/ohci-cns3xxx.c
new file mode 100644
index 0000000..7617b8c
--- /dev/null
+++ b/drivers/usb/host/ohci-cns3xxx.c
@@ -0,0 +1,171 @@
+/*
+ * Copyright 2008 Cavium Networks
+ *
+ * This file is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, Version 2, as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/platform_device.h>
+#include <asm/atomic.h>
+#include <mach/cns3xxx.h>
+#include <mach/pm.h>
+
+static int __devinit
+cns3xxx_ohci_start(struct usb_hcd *hcd)
+{
+ struct ohci_hcd *ohci = hcd_to_ohci(hcd);
+ int ret;
+
+ /* EHCI and OHCI share the same clock and power,
+ * resetting twice would cause the 1st controller been reset.
+ * Therefore only do power up at the first up device, and
+ * power down at the last down device.
+ */
+ if (1 == atomic_inc_return(&usb_pwr_ref)) {
+ cns3xxx_pwr_power_up(1<<PM_PLL_HM_PD_CTRL_REG_OFFSET_PLL_USB);
+ cns3xxx_pwr_clk_en(1<<PM_CLK_GATE_REG_OFFSET_USB_HOST);
+ cns3xxx_pwr_soft_rst(1<<PM_SOFT_RST_REG_OFFST_USB_HOST);
+ }
+
+ ret = ohci_init(ohci);
+ if (ret < 0)
+ return ret;
+
+ ohci->num_ports = 1;
+
+ ret = ohci_run(ohci);
+ if (ret < 0) {
+ err("can't start %s", hcd->self.bus_name);
+ ohci_stop(hcd);
+ return ret;
+ }
+ return 0;
+}
+
+static const struct hc_driver cns3xxx_ohci_hc_driver = {
+ .description = hcd_name,
+ .product_desc = "CNS3XXX OHCI Host controller",
+ .hcd_priv_size = sizeof(struct ohci_hcd),
+ .irq = ohci_irq,
+ .flags = HCD_USB11 | HCD_MEMORY,
+ .start = cns3xxx_ohci_start,
+ .stop = ohci_stop,
+ .shutdown = ohci_shutdown,
+ .urb_enqueue = ohci_urb_enqueue,
+ .urb_dequeue = ohci_urb_dequeue,
+ .endpoint_disable = ohci_endpoint_disable,
+ .get_frame_number = ohci_get_frame,
+ .hub_status_data = ohci_hub_status_data,
+ .hub_control = ohci_hub_control,
+#ifdef CONFIG_PM
+ .bus_suspend = ohci_bus_suspend,
+ .bus_resume = ohci_bus_resume,
+#endif
+ .start_port_reset = ohci_start_port_reset,
+};
+
+static int cns3xxx_ohci_probe(struct platform_device *pdev)
+{
+ struct usb_hcd *hcd = NULL;
+ const struct hc_driver *driver = &cns3xxx_ohci_hc_driver;
+ struct resource *res;
+ int irq;
+ int retval;
+
+ if (usb_disabled())
+ return -ENODEV;
+
+ res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ if (!res) {
+ dev_err(&pdev->dev,
+ "Found HC with no IRQ. Check %s setup!\n",
+ dev_name(&pdev->dev));
+ return -ENODEV;
+ }
+ irq = res->start;
+
+ hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
+ if (!hcd)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev,
+ "Found HC with no register addr. Check %s setup!\n",
+ dev_name(&pdev->dev));
+ retval = -ENODEV;
+ goto err1;
+ }
+ hcd->rsrc_start = res->start;
+ hcd->rsrc_len = res->end - res->start + 1;
+
+#ifdef CNS3XXX_USB_OHCI_BASE_VIRT
+ hcd->regs = (void __iomem *) CNS3XXX_USB_OHCI_BASE_VIRT;
+#else
+ if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len,
+ driver->description)) {
+ dev_dbg(&pdev->dev, "controller already in use\n");
+ retval = -EBUSY;
+ goto err1;
+ }
+
+ hcd->regs = ioremap(hcd->rsrc_start, hcd->rsrc_len);
+
+ if (hcd->regs == NULL) {
+ dev_dbg(&pdev->dev, "error mapping memory\n");
+ retval = -EFAULT;
+ goto err2;
+ }
+#endif
+
+ ohci_hcd_init(hcd_to_ohci(hcd));
+
+ retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
+ if (retval == 0)
+ return retval;
+
+#ifndef CNS3XXX_USB_OHCI_BASE_VIRT
+ iounmap(hcd->regs);
+err2:
+ release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
+#endif
+err1:
+ usb_put_hcd(hcd);
+ return retval;
+}
+
+static int cns3xxx_ohci_remove(struct platform_device *pdev)
+{
+ struct usb_hcd *hcd = platform_get_drvdata(pdev);
+
+ usb_remove_hcd(hcd);
+#ifndef CNS3XXX_USB_OHCI_BASE_VIRT
+ iounmap(hcd->regs);
+ release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
+#endif
+
+ /* EHCI and OHCI share the same clock and power,
+ * resetting twice would cause the 1st controller been reset.
+ * Therefore only do power up at the first up device, and
+ * power down at the last down device.
+ */
+ if (0 == atomic_dec_return(&usb_pwr_ref))
+ cns3xxx_pwr_clk_dis(1<<PM_CLK_GATE_REG_OFFSET_USB_HOST);
+
+ usb_put_hcd(hcd);
+
+ platform_set_drvdata(pdev, NULL);
+
+ return 0;
+}
+
+MODULE_ALIAS("platform:cns3xxx-ohci");
+
+static struct platform_driver ohci_hcd_cns3xxx_driver = {
+ .probe = cns3xxx_ohci_probe,
+ .remove = cns3xxx_ohci_remove,
+ .driver = {
+ .name = "cns3xxx-ohci",
+ },
+};
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 5179acb..5cb6731 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -1111,6 +1111,11 @@ MODULE_LICENSE ("GPL");
#define PLATFORM_DRIVER ohci_octeon_driver
#endif
+#ifdef CONFIG_USB_CNS3XXX_OHCI
+#include "ohci-cns3xxx.c"
+#define PLATFORM_DRIVER ohci_hcd_cns3xxx_driver
+#endif
+
#if !defined(PCI_DRIVER) && \
!defined(PLATFORM_DRIVER) && \
!defined(OMAP1_PLATFORM_DRIVER) && \
--
1.7.3
On Tue, Nov 23, 2010 at 12:32:44AM +0800, [email protected] wrote:
> From: Mac Lin <[email protected]>
>
> This patch add plateform_device for EHCI and OHCI controller on CNS3XXX. Power
> reference count (usb_pwr_ref) is used to control enabling and disabling the
> single clock control for both EHCI and OHCI controller.
>
> It also remove EHCI/OHCI unused virtual address definitions.
>
> Signed-off-by: Mac Lin <[email protected]>
> ---
[...]
I fixed a few small stylistic issues in this patch. Plus,
> + },
> + [1] = {
> + .start = IRQ_CNS3XXX_USB_OHCI,
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static u64 cns3xxx_usb_ohci_dma_mask = 0xffffffffULL;
Changed this to DMA_BIT_MASK(32).
> +static struct platform_device cns3xxx_usb_ohci_device = {
> + .name = "cns3xxx-ohci",
> + .dev = {
> + .dma_mask = &cns3xxx_usb_ohci_dma_mask,
> + .coherent_dma_mask = 0xffffffffULL,
Ditto.
[...]
> --- a/arch/arm/mach-cns3xxx/include/mach/pm.h
> +++ b/arch/arm/mach-cns3xxx/include/mach/pm.h
> @@ -16,4 +16,6 @@ void cns3xxx_pwr_clk_dis(unsigned int block);
> void cns3xxx_pwr_power_up(unsigned int block);
> void cns3xxx_pwr_power_down(unsigned int block);
>
> +extern atomic_t usb_pwr_ref;
You needed #include <asm/atomic.h> in this file, I added this for you.
...and in the end, applied the following patch to CNS3xxx tree:
commit bcea8081b6b6a298db016f33de8d52845deda9d6
Author: Mac Lin <[email protected]>
Date: Tue Nov 23 00:32:44 2010 +0800
ARM: cns3xxx: Add architecture definition for EHCI/OHCI controller
This patch add plateform_device for EHCI and OHCI controller on CNS3XXX.
Power reference count (usb_pwr_ref) is used to control enabling and
disabling the single clock control for both EHCI and OHCI controller.
It also removes EHCI/OHCI unused virtual address definitions.
Signed-off-by: Mac Lin <[email protected]>
Signed-off-by: Anton Vorontsov <[email protected]>
diff --git a/arch/arm/mach-cns3xxx/cns3420vb.c b/arch/arm/mach-cns3xxx/cns3420vb.c
index 9df8391..795ad77 100644
--- a/arch/arm/mach-cns3xxx/cns3420vb.c
+++ b/arch/arm/mach-cns3xxx/cns3420vb.c
@@ -17,6 +17,7 @@
#include <linux/kernel.h>
#include <linux/compiler.h>
#include <linux/io.h>
+#include <linux/dma-mapping.h>
#include <linux/serial_core.h>
#include <linux/serial_8250.h>
#include <linux/platform_device.h>
@@ -108,10 +109,63 @@ static void __init cns3420_early_serial_setup(void)
}
/*
+ * USB
+ */
+static struct resource cns3xxx_usb_ehci_resources[] = {
+ [0] = {
+ .start = CNS3XXX_USB_BASE,
+ .end = CNS3XXX_USB_BASE + SZ_16M - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ [1] = {
+ .start = IRQ_CNS3XXX_USB_EHCI,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static u64 cns3xxx_usb_ehci_dma_mask = DMA_BIT_MASK(32);
+
+static struct platform_device cns3xxx_usb_ehci_device = {
+ .name = "cns3xxx-ehci",
+ .num_resources = ARRAY_SIZE(cns3xxx_usb_ehci_resources),
+ .resource = cns3xxx_usb_ehci_resources,
+ .dev = {
+ .dma_mask = &cns3xxx_usb_ehci_dma_mask,
+ .coherent_dma_mask = DMA_BIT_MASK(32),
+ },
+};
+
+static struct resource cns3xxx_usb_ohci_resources[] = {
+ [0] = {
+ .start = CNS3XXX_USB_OHCI_BASE,
+ .end = CNS3XXX_USB_OHCI_BASE + SZ_16M - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ [1] = {
+ .start = IRQ_CNS3XXX_USB_OHCI,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static u64 cns3xxx_usb_ohci_dma_mask = DMA_BIT_MASK(32);
+
+static struct platform_device cns3xxx_usb_ohci_device = {
+ .name = "cns3xxx-ohci",
+ .num_resources = ARRAY_SIZE(cns3xxx_usb_ohci_resources),
+ .resource = cns3xxx_usb_ohci_resources,
+ .dev = {
+ .dma_mask = &cns3xxx_usb_ohci_dma_mask,
+ .coherent_dma_mask = DMA_BIT_MASK(32),
+ },
+};
+
+/*
* Initialization
*/
static struct platform_device *cns3420_pdevs[] __initdata = {
&cns3420_nor_pdev,
+ &cns3xxx_usb_ehci_device,
+ &cns3xxx_usb_ohci_device,
};
static void __init cns3420_init(void)
diff --git a/arch/arm/mach-cns3xxx/include/mach/cns3xxx.h b/arch/arm/mach-cns3xxx/include/mach/cns3xxx.h
index 6dbce13..191c8e5 100644
--- a/arch/arm/mach-cns3xxx/include/mach/cns3xxx.h
+++ b/arch/arm/mach-cns3xxx/include/mach/cns3xxx.h
@@ -165,7 +165,6 @@
#define CNS3XXX_USBOTG_BASE_VIRT 0xFFF15000
#define CNS3XXX_USB_BASE 0x82000000 /* USB Host Control */
-#define CNS3XXX_USB_BASE_VIRT 0xFFF16000
#define CNS3XXX_SATA2_BASE 0x83000000 /* SATA */
#define CNS3XXX_SATA2_SIZE SZ_16M
@@ -184,7 +183,6 @@
#define CNS3XXX_2DG_BASE_VIRT 0xFFF1B000
#define CNS3XXX_USB_OHCI_BASE 0x88000000 /* USB OHCI */
-#define CNS3XXX_USB_OHCI_BASE_VIRT 0xFFF1C000
#define CNS3XXX_L2C_BASE 0x92000000 /* L2 Cache Control */
#define CNS3XXX_L2C_BASE_VIRT 0xFFF27000
diff --git a/arch/arm/mach-cns3xxx/include/mach/pm.h b/arch/arm/mach-cns3xxx/include/mach/pm.h
index 102617b..6eae7f7 100644
--- a/arch/arm/mach-cns3xxx/include/mach/pm.h
+++ b/arch/arm/mach-cns3xxx/include/mach/pm.h
@@ -11,9 +11,13 @@
#ifndef __CNS3XXX_PM_H
#define __CNS3XXX_PM_H
+#include <asm/atomic.h>
+
void cns3xxx_pwr_clk_en(unsigned int block);
void cns3xxx_pwr_clk_dis(unsigned int block);
void cns3xxx_pwr_power_up(unsigned int block);
void cns3xxx_pwr_power_down(unsigned int block);
+extern atomic_t usb_pwr_ref;
+
#endif /* __CNS3XXX_PM_H */
diff --git a/arch/arm/mach-cns3xxx/pm.c b/arch/arm/mach-cns3xxx/pm.c
index 7b672e5..78fbaba 100644
--- a/arch/arm/mach-cns3xxx/pm.c
+++ b/arch/arm/mach-cns3xxx/pm.c
@@ -10,6 +10,7 @@
#include <linux/module.h>
#include <linux/io.h>
#include <linux/delay.h>
+#include <asm/atomic.h>
#include <mach/system.h>
#include <mach/cns3xxx.h>
#include <mach/pm.h>
@@ -117,3 +118,6 @@ int cns3xxx_cpu_clock(void)
return cpu;
}
EXPORT_SYMBOL(cns3xxx_cpu_clock);
+
+atomic_t usb_pwr_ref = ATOMIC_INIT(0);
+EXPORT_SYMBOL(usb_pwr_ref);
On Tue, Nov 23, 2010 at 12:32:43AM +0800, [email protected] wrote:
> From: Mac Lin <[email protected]>
>
> This patch add cns3xxx_pwr_clk_dis, and export thoes power management functions
> that may be used by many other device drivers on CNS3XXX.
>
> Signed-off-by: Mac Lin <[email protected]>
> ---
> arch/arm/mach-cns3xxx/core.h | 4 ++--
> arch/arm/mach-cns3xxx/include/mach/pm.h | 19 +++++++++++++++++++
> arch/arm/mach-cns3xxx/pm.c | 17 +++++++++++++++++
> 3 files changed, 38 insertions(+), 2 deletions(-)
> create mode 100644 arch/arm/mach-cns3xxx/include/mach/pm.h
>
> diff --git a/arch/arm/mach-cns3xxx/core.h b/arch/arm/mach-cns3xxx/core.h
> index 6b33ec1..c6636bd 100644
> --- a/arch/arm/mach-cns3xxx/core.h
> +++ b/arch/arm/mach-cns3xxx/core.h
> @@ -11,13 +11,13 @@
> #ifndef __CNS3XXX_CORE_H
> #define __CNS3XXX_CORE_H
>
> +#include <mach/pm.h>
This isn't needed in this file. Instead, devices.c should include
it. Plus, pm.c needs to include mach/pm.h, otherwise sparse gives
the following warnings:
CHECK arch/arm/mach-cns3xxx/pm.c
pm.c:26:6: warning: symbol 'cns3xxx_pwr_clk_dis' was not declared. Should it be static?
pm.c:35:6: warning: symbol 'cns3xxx_pwr_power_up' was not declared. Should it be static?
pm.c:47:6: warning: symbol 'cns3xxx_pwr_power_down' was not declared. Should it be static?
pm.c:121:10: warning: symbol 'usb_pwr_ref' was not declared. Should it be static?
I've fixed it up, and applied the patch to cns3xxx tree.
Thanks!
--
Anton Vorontsov
Email: [email protected]
On Tue, Nov 23, 2010 at 12:32:45AM +0800, [email protected] wrote:
> From: Mac Lin <[email protected]>
>
> The CNS3XXX SOC has include USB EHCI and OHCI compatible controllers. This
> patch adds the necessary glue logic to allow ehci-hcd and ohci-hcd drivers to
> work on CNS3XXX
>
> The EHCI and OHCI controllers share a common clock control and reset bit,
> therefore additional check for the timming of enabling and disabling is
> required. The USB bit of PLL Power Down Control is also shared by OTG, 24MHz
> UART clock, Crypto clock, PCIe reference clock, and Clock Scale Generator.
> Therefore we only ensure it is enabled, while not disabling it.
>
> Signed-off-by: Mac Lin <[email protected]>
Thanks for the patch!
A few (mostly cosmetic) comments down below.
> ---
> drivers/usb/Kconfig | 2 +
> drivers/usb/host/Kconfig | 15 ++++
> drivers/usb/host/ehci-cns3xxx.c | 176 +++++++++++++++++++++++++++++++++++++++
> drivers/usb/host/ehci-hcd.c | 5 +
> drivers/usb/host/ohci-cns3xxx.c | 171 +++++++++++++++++++++++++++++++++++++
> drivers/usb/host/ohci-hcd.c | 5 +
> 6 files changed, 374 insertions(+), 0 deletions(-)
> create mode 100644 drivers/usb/host/ehci-cns3xxx.c
> create mode 100644 drivers/usb/host/ohci-cns3xxx.c
>
> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> index 67eb377..5a7c8f1 100644
> --- a/drivers/usb/Kconfig
> +++ b/drivers/usb/Kconfig
> @@ -41,6 +41,7 @@ config USB_ARCH_HAS_OHCI
> default y if MFD_TC6393XB
> default y if ARCH_W90X900
> default y if ARCH_DAVINCI_DA8XX
> + default y if ARCH_CNS3XXX
> # PPC:
> default y if STB03xxx
> default y if PPC_MPC52xx
> @@ -66,6 +67,7 @@ config USB_ARCH_HAS_EHCI
> default y if ARCH_AT91SAM9G45
> default y if ARCH_MXC
> default y if ARCH_OMAP3
> + default y if ARCH_CNS3XXX
> default PCI
>
> # ARM SA1111 chips have a non-PCI based "OHCI-compatible" USB host interface.
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 6f4f8e6..f8970d1 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -147,6 +147,14 @@ config USB_W90X900_EHCI
> ---help---
> Enables support for the W90X900 USB controller
>
> +config USB_CNS3XXX_EHCI
> + bool "Cavium CNS3XXX EHCI Module"
> + depends on USB_EHCI_HCD && ARCH_CNS3XXX
> + ---help---
> + Enable support for the CNS3XXX SOC's on-chip EHCI controller.
> + It is needed for high-speed (480Mbit/sec) USB 2.0 device
> + support.
> +
> config USB_OXU210HP_HCD
> tristate "OXU210HP HCD support"
> depends on USB
> @@ -286,6 +294,13 @@ config USB_OHCI_HCD_SSB
>
> If unsure, say N.
>
> +config USB_CNS3XXX_OHCI
> + bool "Cavium CNS3XXX OHCI Module"
> + depends on USB_OHCI_HCD && ARCH_CNS3XXX
> + ---help---
> + Enable support for the CNS3XXX SOC's on-chip OHCI controller.
> + It is needed for low-speed USB 1.0 device support.
> +
> config USB_OHCI_BIG_ENDIAN_DESC
> bool
> depends on USB_OHCI_HCD
> diff --git a/drivers/usb/host/ehci-cns3xxx.c b/drivers/usb/host/ehci-cns3xxx.c
> new file mode 100644
> index 0000000..87c0ee8
> --- /dev/null
> +++ b/drivers/usb/host/ehci-cns3xxx.c
> @@ -0,0 +1,176 @@
> +/*
> + * Copyright 2008 Cavium Networks
> + *
> + * This file is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, Version 2, as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <asm/atomic.h>
> +#include <mach/cns3xxx.h>
> +#include <mach/pm.h>
> +
> +static int cns3xxx_ehci_init(struct usb_hcd *hcd)
> +{
> + struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> + int retval = 0;
No need for = 0 initializer;
> +
> + /* EHCI and OHCI share the same clock and power,
> + * resetting twice would cause the 1st controller been reset.
> + * Therefore only do power up at the first up device, and
> + * power down at the last down device.
> + */
> + if (1 == atomic_inc_return(&usb_pwr_ref)) {
1 == thing() isn't readable. I'd suggest to change the order.
If you fear for 'thing() = 1' type of errors, don't worry, gcc
will warn. ;-)
> + cns3xxx_pwr_power_up(1<<PM_PLL_HM_PD_CTRL_REG_OFFSET_PLL_USB);
Spaces around <<.
> + cns3xxx_pwr_clk_en(1<<PM_CLK_GATE_REG_OFFSET_USB_HOST);
> + cns3xxx_pwr_soft_rst(1<<PM_SOFT_RST_REG_OFFST_USB_HOST);
> + }
> +
> + ehci->caps = hcd->regs;
> + ehci->regs = hcd->regs
> + + HC_LENGTH(ehci_readl(ehci, &ehci->caps->hc_capbase));
> + ehci->hcs_params = ehci_readl(ehci, &ehci->caps->hcs_params);
> +
> + hcd->has_tt = 0;
> + ehci_reset(ehci);
> +
> + retval = ehci_init(hcd);
> + if (retval)
> + return retval;
> +
> + ehci_port_power(ehci, 0);
> +
> + return retval;
> +}
> +
> +static const struct hc_driver cns3xxx_ehci_hc_driver = {
> + .description = hcd_name,
> + .product_desc = "CNS3XXX EHCI Host Controller",
> + .hcd_priv_size = sizeof(struct ehci_hcd),
> + .irq = ehci_irq,
> + .flags = HCD_MEMORY | HCD_USB2,
> + .reset = cns3xxx_ehci_init,
> + .start = ehci_run,
> + .stop = ehci_stop,
> + .shutdown = ehci_shutdown,
> + .urb_enqueue = ehci_urb_enqueue,
> + .urb_dequeue = ehci_urb_dequeue,
> + .endpoint_disable = ehci_endpoint_disable,
> + .endpoint_reset = ehci_endpoint_reset,
> + .get_frame_number = ehci_get_frame,
> + .hub_status_data = ehci_hub_status_data,
> + .hub_control = ehci_hub_control,
> +#ifdef CONFIG_PM
> + .bus_suspend = ehci_bus_suspend,
> + .bus_resume = ehci_bus_resume,
> +#endif
> + .relinquish_port = ehci_relinquish_port,
> + .port_handed_over = ehci_port_handed_over,
> +
> + .clear_tt_buffer_complete = ehci_clear_tt_buffer_complete,
> +};
> +
> +static int cns3xxx_ehci_probe(struct platform_device *pdev)
> +{
> + struct usb_hcd *hcd;
> + const struct hc_driver *driver = &cns3xxx_ehci_hc_driver;
> + struct resource *res;
> + int irq;
> + int retval;
> +
> + if (usb_disabled())
> + return -ENODEV;
> +
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (!res) {
> + dev_err(&pdev->dev,
the pdev->dev is very repetitive. Please, introduce
'struct device *dev';
> + "Found HC with no IRQ. Check %s setup!\n",
> + dev_name(&pdev->dev));
No need for dev_name() thing here. dev_err and friends will print
device name anyway.
> + return -ENODEV;
> + }
Empty line here.
> + irq = res->start;
> +
> + hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
> + if (!hcd)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev,
> + "Found HC with no register addr. Check %s setup!\n",
> + dev_name(&pdev->dev));
> + retval = -ENODEV;
> + goto err1;
> + }
> + hcd->rsrc_start = res->start;
> + hcd->rsrc_len = res->end - res->start + 1;
> +
> +#ifdef CNS3XXX_USB_BASE_VIRT
> + hcd->regs = (void __iomem *) CNS3XXX_USB_BASE_VIRT;
> +#else
> + if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len,
> + driver->description)) {
> + dev_dbg(&pdev->dev, "controller already in use\n");
> + retval = -EBUSY;
> + goto err1;
> + }
> +
> + hcd->regs = ioremap(hcd->rsrc_start, hcd->rsrc_len);
> +
No need for this empty line.
> + if (hcd->regs == NULL) {
> + dev_dbg(&pdev->dev, "error mapping memory\n");
> + retval = -EFAULT;
> + goto err2;
> + }
> +#endif
> +
> + retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
> + if (retval == 0)
> + return retval;
> +
> +#ifndef CNS3XXX_USB_BASE_VIRT
> + iounmap(hcd->regs);
> +err2:
> + release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
> +#endif
> +err1:
> + usb_put_hcd(hcd);
> +
> + return retval;
> +}
> +
> +static int cns3xxx_ehci_remove(struct platform_device *pdev)
> +{
> + struct usb_hcd *hcd = platform_get_drvdata(pdev);
> +
> + usb_remove_hcd(hcd);
> +#ifndef CNS3XXX_USB_BASE_VIRT
> + iounmap(hcd->regs);
> + release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
> +#endif
> +
> + /* EHCI and OHCI share the same clock and power,
> + * resetting twice would cause the 1st controller been reset.
> + * Therefore only do power up at the first up device, and
> + * power down at the last down device.
> + */
Please use canonical multiline comments, i.e.
/*
* Text here.
*/
> + if (0 == atomic_dec_return(&usb_pwr_ref))
if (atomic_dec_return(&usb_pwr_ref) == 0)
> + cns3xxx_pwr_clk_dis(1<<PM_CLK_GATE_REG_OFFSET_USB_HOST);
spaces around <<.
> +
> + usb_put_hcd(hcd);
> +
> + platform_set_drvdata(pdev, NULL);
> +
> + return 0;
> +}
> +
> +MODULE_ALIAS("platform:cns3xxx-ehci");
> +
> +static struct platform_driver cns3xxx_ehci_driver = {
> + .probe = cns3xxx_ehci_probe,
> + .remove = cns3xxx_ehci_remove,
> + .driver = {
> + .name = "cns3xxx-ehci",
> + },
> +};
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 502a7e6..c5dd1c3 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -1216,6 +1216,11 @@ MODULE_LICENSE ("GPL");
> #define PLATFORM_DRIVER ehci_octeon_driver
> #endif
>
> +#ifdef CONFIG_USB_CNS3XXX_EHCI
> +#include "ehci-cns3xxx.c"
> +#define PLATFORM_DRIVER cns3xxx_ehci_driver
> +#endif
> +
> #if !defined(PCI_DRIVER) && !defined(PLATFORM_DRIVER) && \
> !defined(PS3_SYSTEM_BUS_DRIVER) && !defined(OF_PLATFORM_DRIVER) && \
> !defined(XILINX_OF_PLATFORM_DRIVER)
> diff --git a/drivers/usb/host/ohci-cns3xxx.c b/drivers/usb/host/ohci-cns3xxx.c
> new file mode 100644
> index 0000000..7617b8c
> --- /dev/null
> +++ b/drivers/usb/host/ohci-cns3xxx.c
> @@ -0,0 +1,171 @@
> +/*
> + * Copyright 2008 Cavium Networks
> + *
> + * This file is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, Version 2, as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <asm/atomic.h>
> +#include <mach/cns3xxx.h>
> +#include <mach/pm.h>
> +
> +static int __devinit
> +cns3xxx_ohci_start(struct usb_hcd *hcd)
> +{
> + struct ohci_hcd *ohci = hcd_to_ohci(hcd);
> + int ret;
Either you indent declarations or not in the single file
(I'd prefer not to), otherwise it's very inconsistent.
> +
> + /* EHCI and OHCI share the same clock and power,
> + * resetting twice would cause the 1st controller been reset.
> + * Therefore only do power up at the first up device, and
> + * power down at the last down device.
> + */
> + if (1 == atomic_inc_return(&usb_pwr_ref)) {
unreadable order
> + cns3xxx_pwr_power_up(1<<PM_PLL_HM_PD_CTRL_REG_OFFSET_PLL_USB);
spaces
> + cns3xxx_pwr_clk_en(1<<PM_CLK_GATE_REG_OFFSET_USB_HOST);
> + cns3xxx_pwr_soft_rst(1<<PM_SOFT_RST_REG_OFFST_USB_HOST);
> + }
> +
> + ret = ohci_init(ohci);
> + if (ret < 0)
> + return ret;
> +
> + ohci->num_ports = 1;
> +
> + ret = ohci_run(ohci);
> + if (ret < 0) {
> + err("can't start %s", hcd->self.bus_name);
> + ohci_stop(hcd);
> + return ret;
> + }
> + return 0;
> +}
> +
> +static const struct hc_driver cns3xxx_ohci_hc_driver = {
> + .description = hcd_name,
> + .product_desc = "CNS3XXX OHCI Host controller",
> + .hcd_priv_size = sizeof(struct ohci_hcd),
> + .irq = ohci_irq,
> + .flags = HCD_USB11 | HCD_MEMORY,
> + .start = cns3xxx_ohci_start,
> + .stop = ohci_stop,
> + .shutdown = ohci_shutdown,
> + .urb_enqueue = ohci_urb_enqueue,
> + .urb_dequeue = ohci_urb_dequeue,
> + .endpoint_disable = ohci_endpoint_disable,
> + .get_frame_number = ohci_get_frame,
> + .hub_status_data = ohci_hub_status_data,
> + .hub_control = ohci_hub_control,
> +#ifdef CONFIG_PM
> + .bus_suspend = ohci_bus_suspend,
> + .bus_resume = ohci_bus_resume,
> +#endif
> + .start_port_reset = ohci_start_port_reset,
> +};
> +
> +static int cns3xxx_ohci_probe(struct platform_device *pdev)
> +{
> + struct usb_hcd *hcd = NULL;
No need for = NULL initializer.
> + const struct hc_driver *driver = &cns3xxx_ohci_hc_driver;
> + struct resource *res;
> + int irq;
> + int retval;
> +
> + if (usb_disabled())
> + return -ENODEV;
> +
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (!res) {
> + dev_err(&pdev->dev,
> + "Found HC with no IRQ. Check %s setup!\n",
> + dev_name(&pdev->dev));
> + return -ENODEV;
> + }
> + irq = res->start;
> +
> + hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
> + if (!hcd)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev,
pdev->dev is repetitive
> + "Found HC with no register addr. Check %s setup!\n",
> + dev_name(&pdev->dev));
no need for dev_name
> + retval = -ENODEV;
> + goto err1;
> + }
> + hcd->rsrc_start = res->start;
> + hcd->rsrc_len = res->end - res->start + 1;
> +
> +#ifdef CNS3XXX_USB_OHCI_BASE_VIRT
> + hcd->regs = (void __iomem *) CNS3XXX_USB_OHCI_BASE_VIRT;
> +#else
> + if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len,
> + driver->description)) {
> + dev_dbg(&pdev->dev, "controller already in use\n");
> + retval = -EBUSY;
> + goto err1;
> + }
> +
> + hcd->regs = ioremap(hcd->rsrc_start, hcd->rsrc_len);
> +
I'd not place this empty line here.
> + if (hcd->regs == NULL) {
I'd write it as if (!hcd->regs), which is more natural.
> + dev_dbg(&pdev->dev, "error mapping memory\n");
> + retval = -EFAULT;
> + goto err2;
> + }
> +#endif
> +
> + ohci_hcd_init(hcd_to_ohci(hcd));
> +
> + retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
> + if (retval == 0)
> + return retval;
> +
> +#ifndef CNS3XXX_USB_OHCI_BASE_VIRT
> + iounmap(hcd->regs);
> +err2:
> + release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
> +#endif
> +err1:
> + usb_put_hcd(hcd);
> + return retval;
> +}
> +
> +static int cns3xxx_ohci_remove(struct platform_device *pdev)
> +{
> + struct usb_hcd *hcd = platform_get_drvdata(pdev);
> +
> + usb_remove_hcd(hcd);
> +#ifndef CNS3XXX_USB_OHCI_BASE_VIRT
> + iounmap(hcd->regs);
> + release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
> +#endif
> +
> + /* EHCI and OHCI share the same clock and power,
> + * resetting twice would cause the 1st controller been reset.
> + * Therefore only do power up at the first up device, and
> + * power down at the last down device.
> + */
> + if (0 == atomic_dec_return(&usb_pwr_ref))
order
> + cns3xxx_pwr_clk_dis(1<<PM_CLK_GATE_REG_OFFSET_USB_HOST);
> +
> + usb_put_hcd(hcd);
Thanks,
--
Anton Vorontsov
Email: [email protected]
2010/11/25 Anton Vorontsov <[email protected]>:
> On Tue, Nov 23, 2010 at 12:32:44AM +0800, [email protected] wrote:
>> From: Mac Lin <[email protected]>
>>
>> This patch add plateform_device for EHCI and OHCI controller on CNS3XXX. Power
>> reference count (usb_pwr_ref) is used to control enabling and disabling the
>> single clock control for both EHCI and OHCI controller.
>>
>> It also remove EHCI/OHCI unused virtual address definitions.
>>
>> Signed-off-by: Mac Lin <[email protected]>
>> ---
> [...]
>
> I fixed a few small stylistic issues in this patch. Plus,
>
>> + ? ? },
>> + ? ? [1] = {
>> + ? ? ? ? ? ? .start ? ? ? ? ?= IRQ_CNS3XXX_USB_OHCI,
>> + ? ? ? ? ? ? .flags ? ? ? ? ?= IORESOURCE_IRQ,
>> + ? ? },
>> +};
>> +
>> +static u64 cns3xxx_usb_ohci_dma_mask = 0xffffffffULL;
>
> Changed this to DMA_BIT_MASK(32).
>
>> +static struct platform_device cns3xxx_usb_ohci_device = {
>> + ? ? .name = "cns3xxx-ohci",
>> + ? ? .dev ? ? ? ? ? ? ? ?= {
>> + ? ? ? ? ? ? .dma_mask ? ? ? = &cns3xxx_usb_ohci_dma_mask,
>> + ? ? ? ? ? ? .coherent_dma_mask = 0xffffffffULL,
>
> Ditto.
>
> [...]
>> --- a/arch/arm/mach-cns3xxx/include/mach/pm.h
>> +++ b/arch/arm/mach-cns3xxx/include/mach/pm.h
>> @@ -16,4 +16,6 @@ void cns3xxx_pwr_clk_dis(unsigned int block);
>> ?void cns3xxx_pwr_power_up(unsigned int block);
>> ?void cns3xxx_pwr_power_down(unsigned int block);
>>
>> +extern atomic_t usb_pwr_ref;
>
> You needed #include <asm/atomic.h> in this file, I added this for you.
>
> ...and in the end, applied the following patch to CNS3xxx tree:
>
> commit bcea8081b6b6a298db016f33de8d52845deda9d6
> Author: Mac Lin <[email protected]>
> Date: ? Tue Nov 23 00:32:44 2010 +0800
>
> ? ?ARM: cns3xxx: Add architecture definition for EHCI/OHCI controller
>
> ? ?This patch add plateform_device for EHCI and OHCI controller on CNS3XXX.
> ? ?Power reference count (usb_pwr_ref) is used to control enabling and
> ? ?disabling the single clock control for both EHCI and OHCI controller.
>
> ? ?It also removes EHCI/OHCI unused virtual address definitions.
>
> ? ?Signed-off-by: Mac Lin <[email protected]>
> ? ?Signed-off-by: Anton Vorontsov <[email protected]>
>
> diff --git a/arch/arm/mach-cns3xxx/cns3420vb.c b/arch/arm/mach-cns3xxx/cns3420vb.c
> index 9df8391..795ad77 100644
> --- a/arch/arm/mach-cns3xxx/cns3420vb.c
> +++ b/arch/arm/mach-cns3xxx/cns3420vb.c
> @@ -17,6 +17,7 @@
> ?#include <linux/kernel.h>
> ?#include <linux/compiler.h>
> ?#include <linux/io.h>
> +#include <linux/dma-mapping.h>
> ?#include <linux/serial_core.h>
> ?#include <linux/serial_8250.h>
> ?#include <linux/platform_device.h>
> @@ -108,10 +109,63 @@ static void __init cns3420_early_serial_setup(void)
> ?}
>
> ?/*
> + * USB
> + */
> +static struct resource cns3xxx_usb_ehci_resources[] = {
> + ? ? ? [0] = {
> + ? ? ? ? ? ? ? .start = CNS3XXX_USB_BASE,
> + ? ? ? ? ? ? ? .end ? = CNS3XXX_USB_BASE + SZ_16M - 1,
> + ? ? ? ? ? ? ? .flags = IORESOURCE_MEM,
> + ? ? ? },
> + ? ? ? [1] = {
> + ? ? ? ? ? ? ? .start = IRQ_CNS3XXX_USB_EHCI,
> + ? ? ? ? ? ? ? .flags = IORESOURCE_IRQ,
> + ? ? ? },
> +};
> +
> +static u64 cns3xxx_usb_ehci_dma_mask = DMA_BIT_MASK(32);
> +
> +static struct platform_device cns3xxx_usb_ehci_device = {
> + ? ? ? .name ? ? ? ? ?= "cns3xxx-ehci",
> + ? ? ? .num_resources = ARRAY_SIZE(cns3xxx_usb_ehci_resources),
> + ? ? ? .resource ? ? ?= cns3xxx_usb_ehci_resources,
> + ? ? ? .dev ? ? ? ? ? = {
> + ? ? ? ? ? ? ? .dma_mask ? ? ? ? ?= &cns3xxx_usb_ehci_dma_mask,
> + ? ? ? ? ? ? ? .coherent_dma_mask = DMA_BIT_MASK(32),
> + ? ? ? },
> +};
> +
> +static struct resource cns3xxx_usb_ohci_resources[] = {
> + ? ? ? [0] = {
> + ? ? ? ? ? ? ? .start = CNS3XXX_USB_OHCI_BASE,
> + ? ? ? ? ? ? ? .end ? = CNS3XXX_USB_OHCI_BASE + SZ_16M - 1,
> + ? ? ? ? ? ? ? .flags = IORESOURCE_MEM,
> + ? ? ? },
> + ? ? ? [1] = {
> + ? ? ? ? ? ? ? .start = IRQ_CNS3XXX_USB_OHCI,
> + ? ? ? ? ? ? ? .flags = IORESOURCE_IRQ,
> + ? ? ? },
> +};
> +
> +static u64 cns3xxx_usb_ohci_dma_mask = DMA_BIT_MASK(32);
> +
> +static struct platform_device cns3xxx_usb_ohci_device = {
> + ? ? ? .name ? ? ? ? ?= "cns3xxx-ohci",
> + ? ? ? .num_resources = ARRAY_SIZE(cns3xxx_usb_ohci_resources),
> + ? ? ? .resource ? ? ?= cns3xxx_usb_ohci_resources,
> + ? ? ? .dev ? ? ? ? ? = {
> + ? ? ? ? ? ? ? .dma_mask ? ? ? ? ?= &cns3xxx_usb_ohci_dma_mask,
> + ? ? ? ? ? ? ? .coherent_dma_mask = DMA_BIT_MASK(32),
> + ? ? ? },
> +};
> +
> +/*
> ?* Initialization
> ?*/
> ?static struct platform_device *cns3420_pdevs[] __initdata = {
> ? ? ? ?&cns3420_nor_pdev,
> + ? ? ? &cns3xxx_usb_ehci_device,
> + ? ? ? &cns3xxx_usb_ohci_device,
> ?};
>
> ?static void __init cns3420_init(void)
> diff --git a/arch/arm/mach-cns3xxx/include/mach/cns3xxx.h b/arch/arm/mach-cns3xxx/include/mach/cns3xxx.h
> index 6dbce13..191c8e5 100644
> --- a/arch/arm/mach-cns3xxx/include/mach/cns3xxx.h
> +++ b/arch/arm/mach-cns3xxx/include/mach/cns3xxx.h
> @@ -165,7 +165,6 @@
> ?#define CNS3XXX_USBOTG_BASE_VIRT ? ? ? ? ? ? ? 0xFFF15000
>
> ?#define CNS3XXX_USB_BASE ? ? ? ? ? ? ? ? ? ? ? 0x82000000 ? ? ?/* USB Host Control */
> -#define CNS3XXX_USB_BASE_VIRT ? ? ? ? ? ? ? ? ?0xFFF16000
>
> ?#define CNS3XXX_SATA2_BASE ? ? ? ? ? ? ? ? ? ? 0x83000000 ? ? ?/* SATA */
> ?#define CNS3XXX_SATA2_SIZE ? ? ? ? ? ? ? ? ? ? SZ_16M
> @@ -184,7 +183,6 @@
> ?#define CNS3XXX_2DG_BASE_VIRT ? ? ? ? ? ? ? ? ?0xFFF1B000
>
> ?#define CNS3XXX_USB_OHCI_BASE ? ? ? ? ? ? ? ? ?0x88000000 ? ? ?/* USB OHCI */
> -#define CNS3XXX_USB_OHCI_BASE_VIRT ? ? ? ? ? ? 0xFFF1C000
>
> ?#define CNS3XXX_L2C_BASE ? ? ? ? ? ? ? ? ? ? ? 0x92000000 ? ? ?/* L2 Cache Control */
> ?#define CNS3XXX_L2C_BASE_VIRT ? ? ? ? ? ? ? ? ?0xFFF27000
> diff --git a/arch/arm/mach-cns3xxx/include/mach/pm.h b/arch/arm/mach-cns3xxx/include/mach/pm.h
> index 102617b..6eae7f7 100644
> --- a/arch/arm/mach-cns3xxx/include/mach/pm.h
> +++ b/arch/arm/mach-cns3xxx/include/mach/pm.h
> @@ -11,9 +11,13 @@
> ?#ifndef __CNS3XXX_PM_H
> ?#define __CNS3XXX_PM_H
>
> +#include <asm/atomic.h>
> +
> ?void cns3xxx_pwr_clk_en(unsigned int block);
> ?void cns3xxx_pwr_clk_dis(unsigned int block);
> ?void cns3xxx_pwr_power_up(unsigned int block);
> ?void cns3xxx_pwr_power_down(unsigned int block);
>
> +extern atomic_t usb_pwr_ref;
> +
> ?#endif /* __CNS3XXX_PM_H */
> diff --git a/arch/arm/mach-cns3xxx/pm.c b/arch/arm/mach-cns3xxx/pm.c
> index 7b672e5..78fbaba 100644
> --- a/arch/arm/mach-cns3xxx/pm.c
> +++ b/arch/arm/mach-cns3xxx/pm.c
> @@ -10,6 +10,7 @@
> ?#include <linux/module.h>
> ?#include <linux/io.h>
> ?#include <linux/delay.h>
> +#include <asm/atomic.h>
It's not necessary for mach/pm.h already included it, right?
> ?#include <mach/system.h>
> ?#include <mach/cns3xxx.h>
> ?#include <mach/pm.h>
> @@ -117,3 +118,6 @@ int cns3xxx_cpu_clock(void)
> ? ? ? ?return cpu;
> ?}
> ?EXPORT_SYMBOL(cns3xxx_cpu_clock);
> +
> +atomic_t usb_pwr_ref = ATOMIC_INIT(0);
> +EXPORT_SYMBOL(usb_pwr_ref);
>
On Thu, Nov 25, 2010 at 03:25:38PM +0800, Lin Mac wrote:
[...]
> > #include <linux/module.h>
> > #include <linux/io.h>
> > #include <linux/delay.h>
> > +#include <asm/atomic.h>
> It's not necessary for mach/pm.h already included it, right?
Technically, it's not necessary. But in Linux we don't rely on
headers including other headers. Think that someday mach/pm.h
may not include asm/atomic.h, so the build will break.
Thanks,
>
> > #include <mach/system.h>
> > #include <mach/cns3xxx.h>
> > #include <mach/pm.h>
> > @@ -117,3 +118,6 @@ int cns3xxx_cpu_clock(void)
> > return cpu;
> > }
> > EXPORT_SYMBOL(cns3xxx_cpu_clock);
> > +
> > +atomic_t usb_pwr_ref = ATOMIC_INIT(0);
> > +EXPORT_SYMBOL(usb_pwr_ref);
--
Anton Vorontsov
Email: [email protected]
On Wed, Nov 24, 2010 at 07:25:12PM +0300, Anton Vorontsov wrote:
> On Tue, Nov 23, 2010 at 12:32:45AM +0800, [email protected] wrote:
> > From: Mac Lin <[email protected]>
> >
> > The CNS3XXX SOC has include USB EHCI and OHCI compatible controllers. This
> > patch adds the necessary glue logic to allow ehci-hcd and ohci-hcd drivers to
> > work on CNS3XXX
> >
> > The EHCI and OHCI controllers share a common clock control and reset bit,
> > therefore additional check for the timming of enabling and disabling is
> > required. The USB bit of PLL Power Down Control is also shared by OTG, 24MHz
> > UART clock, Crypto clock, PCIe reference clock, and Clock Scale Generator.
> > Therefore we only ensure it is enabled, while not disabling it.
> >
> > Signed-off-by: Mac Lin <[email protected]>
>
> Thanks for the patch!
>
> A few (mostly cosmetic) comments down below.
[...]
> > +#ifdef CNS3XXX_USB_BASE_VIRT
> > + hcd->regs = (void __iomem *) CNS3XXX_USB_BASE_VIRT;
> > +#else
Oh, and also. Please get rid of these #ifdefs. There is no
CNS3XXX_USB_BASE_VIRT anymore. So, just remove them.
Thanks!
--
Anton Vorontsov
Email: [email protected]
2010/11/25 Anton Vorontsov <[email protected]>:
> On Thu, Nov 25, 2010 at 03:25:38PM +0800, Lin Mac wrote:
> [...]
>> > ?#include <linux/module.h>
>> > ?#include <linux/io.h>
>> > ?#include <linux/delay.h>
>> > +#include <asm/atomic.h>
>> It's not necessary for mach/pm.h already included it, right?
>
> Technically, it's not necessary. But in Linux we don't rely on
> headers including other headers. Think that someday mach/pm.h
> may not include asm/atomic.h, so the build will break.
>
> Thanks,
Get it. So should I re-submit all 3 patches, or just the last one?
>>
>> > ?#include <mach/system.h>
>> > ?#include <mach/cns3xxx.h>
>> > ?#include <mach/pm.h>
>> > @@ -117,3 +118,6 @@ int cns3xxx_cpu_clock(void)
>> > ? ? ? ?return cpu;
>> > ?}
>> > ?EXPORT_SYMBOL(cns3xxx_cpu_clock);
>> > +
>> > +atomic_t usb_pwr_ref = ATOMIC_INIT(0);
>> > +EXPORT_SYMBOL(usb_pwr_ref);
On Thu, Nov 25, 2010 at 09:51:23PM +0800, Lin Mac wrote:
> 2010/11/25 Anton Vorontsov <[email protected]>:
> > On Thu, Nov 25, 2010 at 03:25:38PM +0800, Lin Mac wrote:
> > [...]
> >> > #include <linux/module.h>
> >> > #include <linux/io.h>
> >> > #include <linux/delay.h>
> >> > +#include <asm/atomic.h>
> >> It's not necessary for mach/pm.h already included it, right?
> >
> > Technically, it's not necessary. But in Linux we don't rely on
> > headers including other headers. Think that someday mach/pm.h
> > may not include asm/atomic.h, so the build will break.
> >
> > Thanks,
>
> Get it. So should I re-submit all 3 patches, or just the last one?
Just the last one, thanks!
--
Anton Vorontsov
Email: [email protected]
On Thu, 25 Nov 2010, Anton Vorontsov wrote:
> On Thu, Nov 25, 2010 at 09:51:23PM +0800, Lin Mac wrote:
> > 2010/11/25 Anton Vorontsov <[email protected]>:
> > > On Thu, Nov 25, 2010 at 03:25:38PM +0800, Lin Mac wrote:
> > > [...]
> > >> > #include <linux/module.h>
> > >> > #include <linux/io.h>
> > >> > #include <linux/delay.h>
> > >> > +#include <asm/atomic.h>
> > >> It's not necessary for mach/pm.h already included it, right?
> > >
> > > Technically, it's not necessary. But in Linux we don't rely on
> > > headers including other headers.
Just saw this. Don't be ridiculous -- this happens all over the place.
People generally aren't aware of it, because it's next to impossible to
verify that all the headers needed by a source file are included
directly.
In fact, there are many cases where it would be considered a _mistake_
not to rely on a nested include. For example, include/linux/spinlock.h
itself includes many other files which should not be included directly.
> > > Think that someday mach/pm.h
> > > may not include asm/atomic.h, so the build will break.
Then whoever breaks the build by changing the header file will be
obliged to fix it.
Alan Stern
On Thu, Nov 25, 2010 at 10:37:16AM -0500, Alan Stern wrote:
> > > 2010/11/25 Anton Vorontsov <[email protected]>:
> > > > On Thu, Nov 25, 2010 at 03:25:38PM +0800, Lin Mac wrote:
> > > > [...]
> > > >> > Â #include <linux/module.h>
> > > >> > Â #include <linux/io.h>
> > > >> > Â #include <linux/delay.h>
> > > >> > +#include <asm/atomic.h>
> > > >> It's not necessary for mach/pm.h already included it, right?
> > > >
> > > > Technically, it's not necessary. But in Linux we don't rely on
> > > > headers including other headers.
>
> Just saw this. Don't be ridiculous -- this happens all over the place.
> People generally aren't aware of it, because it's next to impossible to
> verify that all the headers needed by a source file are included
> directly.
Sure. But that doesn't mean we shouldn't try to keep these things
sane enough.
I just noticed that the patch introduced atomic_t usage, but didn't
include asm/atomic.h, so I corrected this before applying that patch.
So I don't see what's the fuss is about. :-)
Thanks,
--
Anton Vorontsov
Email: [email protected]