* FOR COMMENT: void __iomem * and similar casts are Bad News
@ 2008-08-27 22:08 Russell King
2008-08-31 21:47 ` David Brownell
2008-09-03 15:33 ` Eduardo Valentin
0 siblings, 2 replies; 58+ messages in thread
From: Russell King @ 2008-08-27 22:08 UTC (permalink / raw)
To: linux-omap
... are bad when overused. Unexpected bugs can creap in. That one cast
which someone added to shut up a perfectly valid compiler warning now
hides a potential problem.
It is possible to eliminate _lots_ of these casts (and the potential
for bad casts) by making things return the right types. Like, making
IO_ADDRESS return "void __iomem *" rather than an integer.
I sent a similar patch to the one below against mainline to Tony today.
I've marked some changes with certain tags:
- FIXME: where I think the code is just plain wrong -
- such as putting a physical address through a macro which converts
virtual to physical addresses (1510 and 1610 mcbsp).
- or passing virtual addresses to DMA functions (mcbsp).
- or passing physical addresses when what's required is a virtual address
(omap_udc).
- CHECKME: where I've changed something to make it build but I don't
know if this is right.
- WBNI: more a preference to see something changed than a bug.
Comments on the FIXMEs are what I'm really interested in, and preferably
having them actually fixed would be a good idea.
I've compile tested this on a few omap defconfigs and it at least builds.
diff --git a/arch/arm/mach-omap1/mcbsp.c b/arch/arm/mach-omap1/mcbsp.c
index 265cfc2..e5ac038 100644
--- a/arch/arm/mach-omap1/mcbsp.c
+++ b/arch/arm/mach-omap1/mcbsp.c
@@ -134,7 +134,7 @@ static struct omap_mcbsp_ops omap1_mcbsp_ops = {
#ifdef CONFIG_ARCH_OMAP730
static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
{
- .virt_base = io_p2v(OMAP730_MCBSP1_BASE),
+ .virt_base = IO_ADDRESS(OMAP730_MCBSP1_BASE),
.dma_rx_sync = OMAP_DMA_MCBSP1_RX,
.dma_tx_sync = OMAP_DMA_MCBSP1_TX,
.rx_irq = INT_730_McBSP1RX,
@@ -142,7 +142,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
.ops = &omap1_mcbsp_ops,
},
{
- .virt_base = io_p2v(OMAP730_MCBSP2_BASE),
+ .virt_base = IO_ADDRESS(OMAP730_MCBSP2_BASE),
.dma_rx_sync = OMAP_DMA_MCBSP3_RX,
.dma_tx_sync = OMAP_DMA_MCBSP3_TX,
.rx_irq = INT_730_McBSP2RX,
@@ -159,7 +159,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
#ifdef CONFIG_ARCH_OMAP15XX
static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
{
- .virt_base = OMAP1510_MCBSP1_BASE,
+ .virt_base = OMAP1510_MCBSP1_BASE, /* FIXME: virtual or physical */
.dma_rx_sync = OMAP_DMA_MCBSP1_RX,
.dma_tx_sync = OMAP_DMA_MCBSP1_TX,
.rx_irq = INT_McBSP1RX,
@@ -168,7 +168,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
.clk_name = "mcbsp_clk",
},
{
- .virt_base = io_p2v(OMAP1510_MCBSP2_BASE),
+ .virt_base = IO_ADDRESS(OMAP1510_MCBSP2_BASE),
.dma_rx_sync = OMAP_DMA_MCBSP2_RX,
.dma_tx_sync = OMAP_DMA_MCBSP2_TX,
.rx_irq = INT_1510_SPI_RX,
@@ -176,7 +176,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
.ops = &omap1_mcbsp_ops,
},
{
- .virt_base = OMAP1510_MCBSP3_BASE,
+ .virt_base = OMAP1510_MCBSP3_BASE, /* FIXME: virtual or physical */
.dma_rx_sync = OMAP_DMA_MCBSP3_RX,
.dma_tx_sync = OMAP_DMA_MCBSP3_TX,
.rx_irq = INT_McBSP3RX,
@@ -194,7 +194,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
#ifdef CONFIG_ARCH_OMAP16XX
static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
{
- .virt_base = OMAP1610_MCBSP1_BASE,
+ .virt_base = OMAP1610_MCBSP1_BASE, /* FIXME: virtual or physical */
.dma_rx_sync = OMAP_DMA_MCBSP1_RX,
.dma_tx_sync = OMAP_DMA_MCBSP1_TX,
.rx_irq = INT_McBSP1RX,
@@ -203,7 +203,7 @@ static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
.clk_name = "mcbsp_clk",
},
{
- .virt_base = io_p2v(OMAP1610_MCBSP2_BASE),
+ .virt_base = IO_ADDRESS(OMAP1610_MCBSP2_BASE),
.dma_rx_sync = OMAP_DMA_MCBSP2_RX,
.dma_tx_sync = OMAP_DMA_MCBSP2_TX,
.rx_irq = INT_1610_McBSP2_RX,
@@ -211,7 +211,7 @@ static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
.ops = &omap1_mcbsp_ops,
},
{
- .virt_base = OMAP1610_MCBSP3_BASE,
+ .virt_base = OMAP1610_MCBSP3_BASE, /* FIXME: virtual or physical */
.dma_rx_sync = OMAP_DMA_MCBSP3_RX,
.dma_tx_sync = OMAP_DMA_MCBSP3_TX,
.rx_irq = INT_McBSP3RX,
diff --git a/arch/arm/mach-omap1/serial.c b/arch/arm/mach-omap1/serial.c
index 0e25a99..4965986 100644
--- a/arch/arm/mach-omap1/serial.c
+++ b/arch/arm/mach-omap1/serial.c
@@ -67,8 +67,8 @@ static void __init omap_serial_reset(struct plat_serial8250_port *p)
static struct plat_serial8250_port serial_platform_data[] = {
{
- .membase = (char*)IO_ADDRESS(OMAP_UART1_BASE),
- .mapbase = (unsigned long)OMAP_UART1_BASE,
+ .membase = IO_ADDRESS(OMAP_UART1_BASE),
+ .mapbase = OMAP_UART1_BASE,
.irq = INT_UART1,
.flags = UPF_BOOT_AUTOCONF,
.iotype = UPIO_MEM,
@@ -76,8 +76,8 @@ static struct plat_serial8250_port serial_platform_data[] = {
.uartclk = OMAP16XX_BASE_BAUD * 16,
},
{
- .membase = (char*)IO_ADDRESS(OMAP_UART2_BASE),
- .mapbase = (unsigned long)OMAP_UART2_BASE,
+ .membase = IO_ADDRESS(OMAP_UART2_BASE),
+ .mapbase = OMAP_UART2_BASE,
.irq = INT_UART2,
.flags = UPF_BOOT_AUTOCONF,
.iotype = UPIO_MEM,
@@ -85,8 +85,8 @@ static struct plat_serial8250_port serial_platform_data[] = {
.uartclk = OMAP16XX_BASE_BAUD * 16,
},
{
- .membase = (char*)IO_ADDRESS(OMAP_UART3_BASE),
- .mapbase = (unsigned long)OMAP_UART3_BASE,
+ .membase = IO_ADDRESS(OMAP_UART3_BASE),
+ .mapbase = OMAP_UART3_BASE,
.irq = INT_UART3,
.flags = UPF_BOOT_AUTOCONF,
.iotype = UPIO_MEM,
diff --git a/arch/arm/mach-omap2/clock.c b/arch/arm/mach-omap2/clock.c
index be5c616..0c6a1b4 100644
--- a/arch/arm/mach-omap2/clock.c
+++ b/arch/arm/mach-omap2/clock.c
@@ -223,10 +223,11 @@ int omap2_wait_clock_ready(void __iomem *reg, u32 mask, const char *name)
static void omap2_clk_wait_ready(struct clk *clk)
{
u32 other_bit, idlest_bit;
- unsigned long reg, other_reg, idlest_reg, prcm_mod, prcm_regid;
+ unsigned long reg, other_reg, idlest_reg, prcm_regid;
+ void __iomem *prcm_mod;
reg = (unsigned long)clk->enable_reg;
- prcm_mod = reg & ~0xff;
+ prcm_mod = (void __iomem *)(reg & ~0xff);
prcm_regid = reg & 0xff;
if (prcm_regid >= CM_FCLKEN1 && prcm_regid <= OMAP24XX_CM_FCLKEN2)
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 9d6c916..7e3de6e 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -59,34 +59,34 @@ static struct resource gpmc_cs_mem[GPMC_CS_NUM];
static DEFINE_SPINLOCK(gpmc_mem_lock);
static unsigned gpmc_cs_map;
-static u32 gpmc_base;
+static void __iomem *gpmc_base;
static struct clk *gpmc_l3_clk;
static void gpmc_write_reg(int idx, u32 val)
{
- __raw_writel(val, (__force void __iomem *)(gpmc_base + idx));
+ __raw_writel(val, gpmc_base + idx);
}
static u32 gpmc_read_reg(int idx)
{
- return __raw_readl((__force void __iomem *)(gpmc_base + idx));
+ return __raw_readl(gpmc_base + idx);
}
void gpmc_cs_write_reg(int cs, int idx, u32 val)
{
- u32 reg_addr;
+ void __iomem *reg_addr;
reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
- __raw_writel(val, (__force void __iomem *)reg_addr);
+ __raw_writel(val, reg_addr);
}
u32 gpmc_cs_read_reg(int cs, int idx)
{
- u32 reg_addr;
+ void __iomem *reg_addr;
reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
- return __raw_readl((__force void __iomem *)reg_addr);
+ return __raw_readl(reg_addr);
}
/* TODO: Add support for gpmc_fck to clock framework and use it */
@@ -417,12 +417,12 @@ void __init gpmc_init(void)
if (cpu_is_omap24xx()) {
gpmc_l3_clk = clk_get(NULL, "core_l3_ck");
if (cpu_is_omap2420())
- gpmc_base = io_p2v(OMAP2420_GPMC_BASE);
+ gpmc_base = IO_ADDRESS(OMAP2420_GPMC_BASE);
else if (cpu_is_omap2430())
- gpmc_base = io_p2v(OMAP243X_GPMC_BASE);
+ gpmc_base = IO_ADDRESS(OMAP243X_GPMC_BASE);
} else if (cpu_is_omap34xx()) {
gpmc_l3_clk = clk_get(NULL, "gpmc_fck");
- gpmc_base = io_p2v(OMAP34XX_GPMC_BASE);
+ gpmc_base = IO_ADDRESS(OMAP34XX_GPMC_BASE);
}
BUG_ON(IS_ERR(gpmc_l3_clk));
diff --git a/arch/arm/mach-omap2/irq.c b/arch/arm/mach-omap2/irq.c
index 82e954a..7014505 100644
--- a/arch/arm/mach-omap2/irq.c
+++ b/arch/arm/mach-omap2/irq.c
@@ -38,7 +38,7 @@
* for each bank.. when in doubt, consult the TRM.
*/
static struct omap_irq_bank {
- unsigned long base_reg;
+ void __iomem *base_reg;
unsigned int nr_irqs;
} __attribute__ ((aligned(4))) irq_banks[] = {
{
@@ -52,12 +52,12 @@ static struct omap_irq_bank {
static void intc_bank_write_reg(u32 val, struct omap_irq_bank *bank, u16 reg)
{
- __raw_writel(val, (__force void __iomem *)(bank->base_reg + reg));
+ __raw_writel(val, bank->base_reg + reg);
}
static u32 intc_bank_read_reg(struct omap_irq_bank *bank, u16 reg)
{
- return __raw_readl((__force void __iomem *)(bank->base_reg + reg));
+ return __raw_readl(bank->base_reg + reg);
}
/* XXX: FIQ and additional INTC support (only MPU at the moment) */
@@ -102,7 +102,7 @@ static void __init omap_irq_bank_init_one(struct omap_irq_bank *bank)
unsigned long tmp;
tmp = intc_bank_read_reg(bank, INTC_REVISION) & 0xff;
- printk(KERN_INFO "IRQ: Found an INTC at 0x%08lx "
+ printk(KERN_INFO "IRQ: Found an INTC at 0x%p "
"(revision %ld.%ld) with %d interrupts\n",
bank->base_reg, tmp >> 4, tmp & 0xf, bank->nr_irqs);
@@ -147,9 +147,9 @@ void __init omap_init_irq(void)
struct omap_irq_bank *bank = irq_banks + i;
if (cpu_is_omap24xx())
- bank->base_reg = io_p2v(OMAP24XX_IC_BASE);
+ bank->base_reg = IO_ADDRESS(OMAP24XX_IC_BASE);
else if (cpu_is_omap34xx())
- bank->base_reg = io_p2v(OMAP34XX_IC_BASE);
+ bank->base_reg = IO_ADDRESS(OMAP34XX_IC_BASE);
omap_irq_bank_init_one(bank);
diff --git a/arch/arm/mach-omap2/mcbsp.c b/arch/arm/mach-omap2/mcbsp.c
index cedb9a6..bf6f413 100644
--- a/arch/arm/mach-omap2/mcbsp.c
+++ b/arch/arm/mach-omap2/mcbsp.c
@@ -148,7 +148,7 @@ static struct omap_mcbsp_ops omap2_mcbsp_ops = {
#ifdef CONFIG_ARCH_OMAP24XX
static struct omap_mcbsp_platform_data omap24xx_mcbsp_pdata[] = {
{
- .virt_base = OMAP2_IO_ADDRESS(OMAP24XX_MCBSP1_BASE),
+ .virt_base = IO_ADDRESS(OMAP24XX_MCBSP1_BASE),
.dma_rx_sync = OMAP24XX_DMA_MCBSP1_RX,
.dma_tx_sync = OMAP24XX_DMA_MCBSP1_TX,
.rx_irq = INT_24XX_MCBSP1_IRQ_RX,
@@ -157,7 +157,7 @@ static struct omap_mcbsp_platform_data omap24xx_mcbsp_pdata[] = {
.clk_name = "mcbsp_clk",
},
{
- .virt_base = OMAP2_IO_ADDRESS(OMAP24XX_MCBSP2_BASE),
+ .virt_base = IO_ADDRESS(OMAP24XX_MCBSP2_BASE),
.dma_rx_sync = OMAP24XX_DMA_MCBSP2_RX,
.dma_tx_sync = OMAP24XX_DMA_MCBSP2_TX,
.rx_irq = INT_24XX_MCBSP2_IRQ_RX,
@@ -175,7 +175,7 @@ static struct omap_mcbsp_platform_data omap24xx_mcbsp_pdata[] = {
#ifdef CONFIG_ARCH_OMAP34XX
static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
{
- .virt_base = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP1_BASE),
+ .virt_base = IO_ADDRESS(OMAP34XX_MCBSP1_BASE),
.dma_rx_sync = OMAP24XX_DMA_MCBSP1_RX,
.dma_tx_sync = OMAP24XX_DMA_MCBSP1_TX,
.rx_irq = INT_24XX_MCBSP1_IRQ_RX,
@@ -184,7 +184,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
.clk_name = "mcbsp_clk",
},
{
- .virt_base = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP2_BASE),
+ .virt_base = IO_ADDRESS(OMAP34XX_MCBSP2_BASE),
.dma_rx_sync = OMAP24XX_DMA_MCBSP2_RX,
.dma_tx_sync = OMAP24XX_DMA_MCBSP2_TX,
.rx_irq = INT_24XX_MCBSP2_IRQ_RX,
@@ -193,21 +193,21 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
.clk_name = "mcbsp_clk",
},
{
- .virt_base = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP3_BASE),
+ .virt_base = IO_ADDRESS(OMAP34XX_MCBSP3_BASE),
.dma_rx_sync = OMAP24XX_DMA_MCBSP3_RX,
.dma_tx_sync = OMAP24XX_DMA_MCBSP3_TX,
.ops = &omap2_mcbsp_ops,
.clk_name = "mcbsp_clk",
},
{
- .virt_base = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP4_BASE),
+ .virt_base = IO_ADDRESS(OMAP34XX_MCBSP4_BASE),
.dma_rx_sync = OMAP24XX_DMA_MCBSP4_RX,
.dma_tx_sync = OMAP24XX_DMA_MCBSP4_TX,
.ops = &omap2_mcbsp_ops,
.clk_name = "mcbsp_clk",
},
{
- .virt_base = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP5_BASE),
+ .virt_base = IO_ADDRESS(OMAP34XX_MCBSP5_BASE),
.dma_rx_sync = OMAP24XX_DMA_MCBSP5_RX,
.dma_tx_sync = OMAP24XX_DMA_MCBSP5_TX,
.ops = &omap2_mcbsp_ops,
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index bd6f8c9..dbbef90 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -28,24 +28,24 @@ static struct clk *uart_fck[OMAP_MAX_NR_PORTS];
static struct plat_serial8250_port serial_platform_data[] = {
{
- .membase = (void __iomem *)IO_ADDRESS(OMAP_UART1_BASE),
- .mapbase = (unsigned long)OMAP_UART1_BASE,
+ .membase = IO_ADDRESS(OMAP_UART1_BASE),
+ .mapbase = OMAP_UART1_BASE,
.irq = 72,
.flags = UPF_BOOT_AUTOCONF,
.iotype = UPIO_MEM,
.regshift = 2,
.uartclk = OMAP24XX_BASE_BAUD * 16,
}, {
- .membase = (void __iomem *)IO_ADDRESS(OMAP_UART2_BASE),
- .mapbase = (unsigned long)OMAP_UART2_BASE,
+ .membase = IO_ADDRESS(OMAP_UART2_BASE),
+ .mapbase = OMAP_UART2_BASE,
.irq = 73,
.flags = UPF_BOOT_AUTOCONF,
.iotype = UPIO_MEM,
.regshift = 2,
.uartclk = OMAP24XX_BASE_BAUD * 16,
}, {
- .membase = (void __iomem *)IO_ADDRESS(OMAP_UART3_BASE),
- .mapbase = (unsigned long)OMAP_UART3_BASE,
+ .membase = IO_ADDRESS(OMAP_UART3_BASE),
+ .mapbase = OMAP_UART3_BASE,
.irq = 74,
.flags = UPF_BOOT_AUTOCONF,
.iotype = UPIO_MEM,
diff --git a/arch/arm/plat-omap/common.c b/arch/arm/plat-omap/common.c
index 2914af0..3cf726b 100644
--- a/arch/arm/plat-omap/common.c
+++ b/arch/arm/plat-omap/common.c
@@ -281,12 +281,12 @@ static void __init __omap2_set_globals(void)
static struct omap_globals omap242x_globals = {
.class = OMAP242X_CLASS,
- .tap = (__force void __iomem *)OMAP2_IO_ADDRESS(0x48014000),
- .sdrc = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_SDRC_BASE),
- .sms = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_SMS_BASE),
- .ctrl = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_CTRL_BASE),
- .prm = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_PRM_BASE),
- .cm = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_CM_BASE),
+ .tap = IO_ADDRESS(0x48014000),
+ .sdrc = IO_ADDRESS(OMAP2420_SDRC_BASE),
+ .sms = IO_ADDRESS(OMAP2420_SMS_BASE),
+ .ctrl = IO_ADDRESS(OMAP2420_CTRL_BASE),
+ .prm = IO_ADDRESS(OMAP2420_PRM_BASE),
+ .cm = IO_ADDRESS(OMAP2420_CM_BASE),
};
void __init omap2_set_globals_242x(void)
@@ -300,12 +300,12 @@ void __init omap2_set_globals_242x(void)
static struct omap_globals omap243x_globals = {
.class = OMAP243X_CLASS,
- .tap = (__force void __iomem *)OMAP2_IO_ADDRESS(0x4900a000),
- .sdrc = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP243X_SDRC_BASE),
- .sms = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP243X_SMS_BASE),
- .ctrl = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP243X_CTRL_BASE),
- .prm = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2430_PRM_BASE),
- .cm = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2430_CM_BASE),
+ .tap = IO_ADDRESS(0x4900a000),
+ .sdrc = IO_ADDRESS(OMAP243X_SDRC_BASE),
+ .sms = IO_ADDRESS(OMAP243X_SMS_BASE),
+ .ctrl = IO_ADDRESS(OMAP243X_CTRL_BASE),
+ .prm = IO_ADDRESS(OMAP2430_PRM_BASE),
+ .cm = IO_ADDRESS(OMAP2430_CM_BASE),
};
void __init omap2_set_globals_243x(void)
@@ -319,12 +319,12 @@ void __init omap2_set_globals_243x(void)
static struct omap_globals omap343x_globals = {
.class = OMAP343X_CLASS,
- .tap = (__force void __iomem *)OMAP2_IO_ADDRESS(0x4830A000),
- .sdrc = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP343X_SDRC_BASE),
- .sms = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP343X_SMS_BASE),
- .ctrl = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP343X_CTRL_BASE),
- .prm = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP3430_PRM_BASE),
- .cm = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP3430_CM_BASE),
+ .tap = IO_ADDRESS(0x4830A000),
+ .sdrc = IO_ADDRESS(OMAP343X_SDRC_BASE),
+ .sms = IO_ADDRESS(OMAP343X_SMS_BASE),
+ .ctrl = IO_ADDRESS(OMAP343X_CTRL_BASE),
+ .prm = IO_ADDRESS(OMAP3430_PRM_BASE),
+ .cm = IO_ADDRESS(OMAP3430_CM_BASE),
};
void __init omap2_set_globals_343x(void)
diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
index 1c6b905..bb3f187 100644
--- a/arch/arm/plat-omap/dma.c
+++ b/arch/arm/plat-omap/dma.c
@@ -2298,13 +2298,13 @@ static int __init omap_init_dma(void)
int ch, r;
if (cpu_class_is_omap1()) {
- omap_dma_base = (void __iomem *)IO_ADDRESS(OMAP1_DMA_BASE);
+ omap_dma_base = IO_ADDRESS(OMAP1_DMA_BASE);
dma_lch_count = OMAP1_LOGICAL_DMA_CH_COUNT;
} else if (cpu_is_omap24xx()) {
- omap_dma_base = (void __iomem *)IO_ADDRESS(OMAP24XX_DMA4_BASE);
+ omap_dma_base = IO_ADDRESS(OMAP24XX_DMA4_BASE);
dma_lch_count = OMAP_DMA4_LOGICAL_DMA_CH_COUNT;
} else if (cpu_is_omap34xx()) {
- omap_dma_base = (void __iomem *)IO_ADDRESS(OMAP34XX_DMA4_BASE);
+ omap_dma_base = IO_ADDRESS(OMAP34XX_DMA4_BASE);
dma_lch_count = OMAP_DMA4_LOGICAL_DMA_CH_COUNT;
} else {
pr_err("DMA init failed for unsupported omap\n");
diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 8b4b553..167ec2f 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -693,7 +693,7 @@ int __init omap_dm_timer_init(void)
for (i = 0; i < dm_timer_count; i++) {
timer = &dm_timers[i];
- timer->io_base = (void __iomem *)io_p2v(timer->phys_base);
+ timer->io_base = IO_ADDRESS(timer->phys_base);
#if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
if (cpu_class_is_omap2()) {
char clk_name[16];
diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
index 3e76ee2..dee380b 100644
--- a/arch/arm/plat-omap/gpio.c
+++ b/arch/arm/plat-omap/gpio.c
@@ -1488,7 +1488,7 @@ static int __init _omap_gpio_init(void)
bank->chip.set = gpio_set;
if (bank_is_mpuio(bank)) {
bank->chip.label = "mpuio";
-#ifdef CONFIG_ARCH_OMAP1
+#ifdef CONFIG_ARCH_OMAP16XX /* CHECKME */
bank->chip.dev = &omap_mpuio_device.dev;
#endif
bank->chip.base = OMAP_MPUIO(0);
diff --git a/arch/arm/plat-omap/include/mach/control.h b/arch/arm/plat-omap/include/mach/control.h
index 1cd98fd..91d7480 100644
--- a/arch/arm/plat-omap/include/mach/control.h
+++ b/arch/arm/plat-omap/include/mach/control.h
@@ -18,18 +18,9 @@
#include <mach/io.h>
-#ifndef __ASSEMBLY__
-#define OMAP242X_CTRL_REGADDR(reg) \
- (void __iomem *)IO_ADDRESS(OMAP242X_CTRL_BASE + (reg))
-#define OMAP243X_CTRL_REGADDR(reg) \
- (void __iomem *)IO_ADDRESS(OMAP243X_CTRL_BASE + (reg))
-#define OMAP343X_CTRL_REGADDR(reg) \
- (void __iomem *)IO_ADDRESS(OMAP343X_CTRL_BASE + (reg))
-#else
#define OMAP242X_CTRL_REGADDR(reg) IO_ADDRESS(OMAP242X_CTRL_BASE + (reg))
#define OMAP243X_CTRL_REGADDR(reg) IO_ADDRESS(OMAP243X_CTRL_BASE + (reg))
#define OMAP343X_CTRL_REGADDR(reg) IO_ADDRESS(OMAP343X_CTRL_BASE + (reg))
-#endif /* __ASSEMBLY__ */
/*
* As elsewhere, the "OMAP2_" prefix indicates that the macro is valid for
diff --git a/arch/arm/plat-omap/include/mach/io.h b/arch/arm/plat-omap/include/mach/io.h
index 7d3481b..5126a0a 100644
--- a/arch/arm/plat-omap/include/mach/io.h
+++ b/arch/arm/plat-omap/include/mach/io.h
@@ -55,14 +55,12 @@
#if defined(CONFIG_ARCH_OMAP1)
-#define IO_PHYS 0xFFFB0000
-#define IO_OFFSET 0x01000000 /* Virtual IO = 0xfefb0000 */
-#define IO_SIZE 0x40000
-#define IO_VIRT (IO_PHYS - IO_OFFSET)
-#define IO_ADDRESS(pa) ((pa) - IO_OFFSET)
-#define OMAP1_IO_ADDRESS(pa) ((pa) - IO_OFFSET)
-#define io_p2v(pa) ((pa) - IO_OFFSET)
-#define io_v2p(va) ((va) + IO_OFFSET)
+#define IO_PHYS 0xFFFB0000
+#define IO_OFFSET 0x01000000 /* Virtual IO = 0xfefb0000 */
+#define IO_SIZE 0x40000
+#define IO_VIRT (IO_PHYS - IO_OFFSET)
+#define __IO_ADDRESS(pa) ((pa) - IO_OFFSET)
+#define io_v2p(va) ((va) + IO_OFFSET)
#elif defined(CONFIG_ARCH_OMAP2)
@@ -90,11 +88,9 @@
#endif
-#define IO_OFFSET 0x90000000
-#define IO_ADDRESS(pa) ((pa) + IO_OFFSET) /* Works for L3 and L4 */
-#define OMAP2_IO_ADDRESS(pa) ((pa) + IO_OFFSET) /* Works for L3 and L4 */
-#define io_p2v(pa) ((pa) + IO_OFFSET) /* Works for L3 and L4 */
-#define io_v2p(va) ((va) - IO_OFFSET) /* Works for L3 and L4 */
+#define IO_OFFSET 0x90000000
+#define __IO_ADDRESS(pa) ((pa) + IO_OFFSET) /* Works for L3 and L4 */
+#define io_v2p(va) ((va) - IO_OFFSET) /* Works for L3 and L4 */
/* DSP */
#define DSP_MEM_24XX_PHYS OMAP2420_DSP_MEM_BASE /* 0x58000000 */
@@ -149,9 +145,7 @@
#define IO_OFFSET 0x90000000
-#define IO_ADDRESS(pa) ((pa) + IO_OFFSET)/* Works for L3 and L4 */
-#define OMAP2_IO_ADDRESS(pa) ((pa) + IO_OFFSET)/* Works for L3 and L4 */
-#define io_p2v(pa) ((pa) + IO_OFFSET)/* Works for L3 and L4 */
+#define __IO_ADDRESS(pa) ((pa) + IO_OFFSET)/* Works for L3 and L4 */
#define io_v2p(va) ((va) - IO_OFFSET)/* Works for L3 and L4 */
/* DSP */
@@ -167,7 +161,13 @@
#endif
-#ifndef __ASSEMBLER__
+#ifdef __ASSEMBLER__
+
+#define IO_ADDRESS(pa) __IO_ADDRESS(pa)
+
+#else
+
+#define IO_ADDRESS(pa) ((void __iomem *)__IO_ADDRESS(pa))
/*
* Functions to access the OMAP IO region
diff --git a/arch/arm/plat-omap/include/mach/mcbsp.h b/arch/arm/plat-omap/include/mach/mcbsp.h
index c04bcd5..3c02bf6 100644
--- a/arch/arm/plat-omap/include/mach/mcbsp.h
+++ b/arch/arm/plat-omap/include/mach/mcbsp.h
@@ -326,7 +326,7 @@ struct omap_mcbsp_ops {
};
struct omap_mcbsp_platform_data {
- u32 virt_base;
+ void __iomem *virt_base;
u8 dma_rx_sync, dma_tx_sync;
u16 rx_irq, tx_irq;
struct omap_mcbsp_ops *ops;
@@ -335,7 +335,7 @@ struct omap_mcbsp_platform_data {
struct omap_mcbsp {
struct device *dev;
- u32 io_base;
+ void __iomem *io_base;
u8 id;
u8 free;
omap_mcbsp_word_length rx_word_length;
diff --git a/arch/arm/plat-omap/include/mach/pm.h b/arch/arm/plat-omap/include/mach/pm.h
index 5a1f3db..e4cf1c5 100644
--- a/arch/arm/plat-omap/include/mach/pm.h
+++ b/arch/arm/plat-omap/include/mach/pm.h
@@ -39,11 +39,11 @@
* Register and offset definitions to be used in PM assembler code
* ----------------------------------------------------------------------------
*/
-#define CLKGEN_REG_ASM_BASE io_p2v(0xfffece00)
+#define CLKGEN_REG_ASM_BASE IO_ADDRESS(0xfffece00)
#define ARM_IDLECT1_ASM_OFFSET 0x04
#define ARM_IDLECT2_ASM_OFFSET 0x08
-#define TCMIF_ASM_BASE io_p2v(0xfffecc00)
+#define TCMIF_ASM_BASE IO_ADDRESS(0xfffecc00)
#define EMIFS_CONFIG_ASM_OFFSET 0x0c
#define EMIFF_SDRAM_CONFIG_ASM_OFFSET 0x20
diff --git a/arch/arm/plat-omap/include/mach/sdrc.h b/arch/arm/plat-omap/include/mach/sdrc.h
index b7db862..c66c838 100644
--- a/arch/arm/plat-omap/include/mach/sdrc.h
+++ b/arch/arm/plat-omap/include/mach/sdrc.h
@@ -75,12 +75,9 @@
* SMS register access
*/
-#define OMAP242X_SMS_REGADDR(reg) \
- (void __iomem *)IO_ADDRESS(OMAP2420_SMS_BASE + reg)
-#define OMAP243X_SMS_REGADDR(reg) \
- (void __iomem *)IO_ADDRESS(OMAP243X_SMS_BASE + reg)
-#define OMAP343X_SMS_REGADDR(reg) \
- (void __iomem *)IO_ADDRESS(OMAP343X_SMS_BASE + reg)
+#define OMAP242X_SMS_REGADDR(reg) IO_ADDRESS(OMAP2420_SMS_BASE + reg)
+#define OMAP243X_SMS_REGADDR(reg) IO_ADDRESS(OMAP243X_SMS_BASE + reg)
+#define OMAP343X_SMS_REGADDR(reg) IO_ADDRESS(OMAP343X_SMS_BASE + reg)
/* SMS register offsets - read/write with sms_{read,write}_reg() */
diff --git a/arch/arm/plat-omap/include/mach/serial.h b/arch/arm/plat-omap/include/mach/serial.h
index abea6ef..c6b7f59 100644
--- a/arch/arm/plat-omap/include/mach/serial.h
+++ b/arch/arm/plat-omap/include/mach/serial.h
@@ -33,9 +33,9 @@
#define OMAP24XX_BASE_BAUD (48000000/16)
#define is_omap_port(p) ({int __ret = 0; \
- if (p == IO_ADDRESS(OMAP_UART1_BASE) || \
- p == IO_ADDRESS(OMAP_UART2_BASE) || \
- p == IO_ADDRESS(OMAP_UART3_BASE)) \
+ if (p == OMAP_UART1_BASE || \
+ p == OMAP_UART2_BASE || \
+ p == OMAP_UART3_BASE) \
__ret = 1; \
__ret; \
})
diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index 0f0e3f3..51d715f 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -30,7 +30,7 @@
struct omap_mcbsp **mcbsp_ptr;
int omap_mcbsp_count;
-void omap_mcbsp_write(u32 io_base, u16 reg, u32 val)
+void omap_mcbsp_write(void __iomem *io_base, u16 reg, u32 val)
{
if (cpu_class_is_omap1() || cpu_is_omap2420())
__raw_writew((u16)val, io_base + reg);
@@ -38,7 +38,7 @@ void omap_mcbsp_write(u32 io_base, u16 reg, u32 val)
__raw_writel(val, io_base + reg);
}
-int omap_mcbsp_read(u32 io_base, u16 reg)
+int omap_mcbsp_read(void __iomem *io_base, u16 reg)
{
if (cpu_class_is_omap1() || cpu_is_omap2420())
return __raw_readw(io_base + reg);
@@ -149,7 +149,7 @@ static void omap_mcbsp_rx_dma_callback(int lch, u16 ch_status, void *data)
void omap_mcbsp_config(unsigned int id, const struct omap_mcbsp_reg_cfg *config)
{
struct omap_mcbsp *mcbsp;
- u32 io_base;
+ void __iomem *io_base;
if (!omap_mcbsp_check_valid_id(id)) {
printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
@@ -158,7 +158,7 @@ void omap_mcbsp_config(unsigned int id, const struct omap_mcbsp_reg_cfg *config)
mcbsp = id_to_mcbsp_ptr(id);
io_base = mcbsp->io_base;
- dev_dbg(mcbsp->dev, "Configuring McBSP%d io_base: 0x%8x\n",
+ dev_dbg(mcbsp->dev, "Configuring McBSP%d io_base: 0x%p\n",
mcbsp->id, io_base);
/* We write the given config */
@@ -306,7 +306,7 @@ EXPORT_SYMBOL(omap_mcbsp_free);
void omap_mcbsp_start(unsigned int id)
{
struct omap_mcbsp *mcbsp;
- u32 io_base;
+ void __iomem *io_base;
u16 w;
if (!omap_mcbsp_check_valid_id(id)) {
@@ -344,7 +344,7 @@ EXPORT_SYMBOL(omap_mcbsp_start);
void omap_mcbsp_stop(unsigned int id)
{
struct omap_mcbsp *mcbsp;
- u32 io_base;
+ void __iomem *io_base;
u16 w;
if (!omap_mcbsp_check_valid_id(id)) {
@@ -373,7 +373,7 @@ EXPORT_SYMBOL(omap_mcbsp_stop);
int omap_mcbsp_pollwrite(unsigned int id, u16 buf)
{
struct omap_mcbsp *mcbsp;
- u32 base;
+ void __iomem *base;
if (!omap_mcbsp_check_valid_id(id)) {
printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
@@ -418,7 +418,7 @@ EXPORT_SYMBOL(omap_mcbsp_pollwrite);
int omap_mcbsp_pollread(unsigned int id, u16 *buf)
{
struct omap_mcbsp *mcbsp;
- u32 base;
+ void __iomem *base;
if (!omap_mcbsp_check_valid_id(id)) {
printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
@@ -465,7 +465,7 @@ EXPORT_SYMBOL(omap_mcbsp_pollread);
void omap_mcbsp_xmit_word(unsigned int id, u32 word)
{
struct omap_mcbsp *mcbsp;
- u32 io_base;
+ void __iomem *io_base;
omap_mcbsp_word_length word_length;
if (!omap_mcbsp_check_valid_id(id)) {
@@ -488,7 +488,7 @@ EXPORT_SYMBOL(omap_mcbsp_xmit_word);
u32 omap_mcbsp_recv_word(unsigned int id)
{
struct omap_mcbsp *mcbsp;
- u32 io_base;
+ void __iomem *io_base;
u16 word_lsb, word_msb = 0;
omap_mcbsp_word_length word_length;
@@ -514,7 +514,7 @@ EXPORT_SYMBOL(omap_mcbsp_recv_word);
int omap_mcbsp_spi_master_xmit_word_poll(unsigned int id, u32 word)
{
struct omap_mcbsp *mcbsp;
- u32 io_base;
+ void __iomem *io_base;
omap_mcbsp_word_length tx_word_length;
omap_mcbsp_word_length rx_word_length;
u16 spcr2, spcr1, attempts = 0, word_lsb, word_msb = 0;
@@ -580,7 +580,8 @@ EXPORT_SYMBOL(omap_mcbsp_spi_master_xmit_word_poll);
int omap_mcbsp_spi_master_recv_word_poll(unsigned int id, u32 *word)
{
struct omap_mcbsp *mcbsp;
- u32 io_base, clock_word = 0;
+ u32 clock_word = 0;
+ void __iomem *io_base;
omap_mcbsp_word_length tx_word_length;
omap_mcbsp_word_length rx_word_length;
u16 spcr2, spcr1, attempts = 0, word_lsb, word_msb = 0;
@@ -701,6 +702,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
omap_set_dma_dest_params(mcbsp->dma_tx_lch,
src_port,
OMAP_DMA_AMODE_CONSTANT,
+ /* FIXME: this is a virtual address */
mcbsp->io_base + OMAP_MCBSP_REG_DXR1,
0, 0);
@@ -764,6 +766,7 @@ int omap_mcbsp_recv_buffer(unsigned int id, dma_addr_t buffer,
omap_set_dma_src_params(mcbsp->dma_rx_lch,
src_port,
OMAP_DMA_AMODE_CONSTANT,
+ /* FIXME: this is a virtual address */
mcbsp->io_base + OMAP_MCBSP_REG_DRR1,
0, 0);
diff --git a/drivers/char/hw_random/omap-rng.c b/drivers/char/hw_random/omap-rng.c
index 154a21f..668bf7d 100644
--- a/drivers/char/hw_random/omap-rng.c
+++ b/drivers/char/hw_random/omap-rng.c
@@ -122,7 +122,7 @@ static int __init omap_rng_probe(struct platform_device *pdev)
return -EBUSY;
dev_set_drvdata(&pdev->dev, mem);
- rng_base = (u32 __force __iomem *)io_p2v(res->start);
+ rng_base = IO_ADDRESS(res->start); /* WBNI: why not ioremap? */
ret = hwrng_register(&omap_rng_ops);
if (ret) {
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 3c4e581..978c072 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -757,7 +757,7 @@ omap_i2c_probe(struct platform_device *pdev)
dev->speed = *speed;
dev->dev = &pdev->dev;
dev->irq = irq->start;
- dev->base = (void __iomem *) IO_ADDRESS(mem->start);
+ dev->base = IO_ADDRESS(mem->start); /* WBNI: why not ioremap? */
platform_set_drvdata(pdev, dev);
if ((r = omap_i2c_get_clocks(dev)) != 0)
diff --git a/drivers/mmc/host/omap.c b/drivers/mmc/host/omap.c
index c160288..802877c 100644
--- a/drivers/mmc/host/omap.c
+++ b/drivers/mmc/host/omap.c
@@ -1455,7 +1455,7 @@ static int __init mmc_omap_probe(struct platform_device *pdev)
host->irq = irq;
host->phys_base = host->mem_res->start;
- host->virt_base = (void __iomem *) IO_ADDRESS(host->phys_base);
+ host->virt_base = IO_ADDRESS(host->phys_base); /* WBNI: why not ioremap? */
if (cpu_is_omap24xx()) {
host->iclk = clk_get(&pdev->dev, "mmc_ick");
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 5a5f95f..2ba5cbb 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -2209,7 +2209,7 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
#ifdef CONFIG_ARCH_OMAP15XX
/* Workaround to enable 115200 baud on OMAP1510 internal ports */
- if (cpu_is_omap1510() && is_omap_port((unsigned int)up->port.membase)) {
+ if (cpu_is_omap1510() && is_omap_port(up->port.mapbase)) {
if (baud == 115200) {
quot = 1;
serial_out(up, UART_OMAP_OSC_12M_SEL, 1);
@@ -2284,7 +2284,7 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
int ret = 0;
#ifdef CONFIG_ARCH_OMAP
- if (is_omap_port((unsigned int)up->port.membase))
+ if (is_omap_port(up->port.mapbase))
size = 0x16 << up->port.regshift;
#endif
diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index 9d2186f..8178665 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -119,12 +119,14 @@ struct omap2_mcspi {
struct clk *fck;
/* Virtual base address of the controller */
void __iomem *base;
+ unsigned long phys;
/* SPI1 has 4 channels, while SPI2 has 2 */
struct omap2_mcspi_dma *dma_channels;
};
struct omap2_mcspi_cs {
void __iomem *base;
+ unsigned long phys;
int word_len;
};
@@ -233,7 +235,7 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
c = count;
word_len = cs->word_len;
- base = (unsigned long) io_v2p(cs->base);
+ base = cs->phys;
tx_reg = base + OMAP2_MCSPI_TX0;
rx_reg = base + OMAP2_MCSPI_RX0;
rx = xfer->rx_buf;
@@ -633,6 +635,7 @@ static int omap2_mcspi_setup(struct spi_device *spi)
if (!cs)
return -ENOMEM;
cs->base = mcspi->base + spi->chip_select * 0x14;
+ cs->phys = mcspi->phys + spi->chip_select * 0x14;
spi->controller_state = cs;
}
@@ -1005,7 +1008,8 @@ static int __init omap2_mcspi_probe(struct platform_device *pdev)
goto err1;
}
- mcspi->base = (void __iomem *) io_p2v(r->start);
+ mcspi->phys = r->start;
+ mcspi->base = IO_ADDRESS(r->start); /* WBNI: why not ioremap? */
INIT_WORK(&mcspi->work, omap2_mcspi_work);
diff --git a/drivers/spi/omap_uwire.c b/drivers/spi/omap_uwire.c
index 5515eb9..3166635 100644
--- a/drivers/spi/omap_uwire.c
+++ b/drivers/spi/omap_uwire.c
@@ -59,7 +59,7 @@
* and irqs should show there too...
*/
#define UWIRE_BASE_PHYS 0xFFFB3000
-#define UWIRE_BASE ((void *__iomem)IO_ADDRESS(UWIRE_BASE_PHYS))
+#define UWIRE_BASE IO_ADDRESS(UWIRE_BASE_PHYS)
/* uWire Registers: */
#define UWIRE_IO_SIZE 0x20
diff --git a/drivers/usb/gadget/omap_udc.c b/drivers/usb/gadget/omap_udc.c
index a2638ee..7032975 100644
--- a/drivers/usb/gadget/omap_udc.c
+++ b/drivers/usb/gadget/omap_udc.c
@@ -786,6 +786,7 @@ static void dma_channel_claim(struct omap_ep *ep, unsigned channel)
omap_set_dma_dest_params(ep->lch,
OMAP_DMA_PORT_TIPB,
OMAP_DMA_AMODE_CONSTANT,
+ /* FIXME: aren't these already physical addresses */
(unsigned long) io_v2p(UDC_DATA_DMA),
0, 0);
}
@@ -803,6 +804,7 @@ static void dma_channel_claim(struct omap_ep *ep, unsigned channel)
omap_set_dma_src_params(ep->lch,
OMAP_DMA_PORT_TIPB,
OMAP_DMA_AMODE_CONSTANT,
+ /* FIXME: aren't these already physical addresses */
(unsigned long) io_v2p(UDC_DATA_DMA),
0, 0);
/* EMIFF or SDRC */
diff --git a/drivers/usb/host/ohci-omap.c b/drivers/usb/host/ohci-omap.c
index 71fb9fd..c2fa621 100644
--- a/drivers/usb/host/ohci-omap.c
+++ b/drivers/usb/host/ohci-omap.c
@@ -208,7 +208,7 @@ static int ohci_omap_init(struct usb_hcd *hcd)
if (cpu_is_omap16xx())
ocpi_enable();
-#ifdef CONFIG_ARCH_OMAP_OTG
+#ifdef CONFIG_USB_OTG /* CHECKME */
if (need_transceiver) {
ohci->transceiver = otg_get_transceiver();
if (ohci->transceiver) {
@@ -343,7 +343,7 @@ static int usb_hcd_omap_probe (const struct hc_driver *driver,
goto err1;
}
- hcd->regs = (void __iomem *) (int) IO_ADDRESS(hcd->rsrc_start);
+ hcd->regs = IO_ADDRESS(hcd->rsrc_start); /* WBNI: why not ioremap? */
ohci = hcd_to_ohci(hcd);
ohci_hcd_init(ohci);
diff --git a/drivers/video/omap/dispc.c b/drivers/video/omap/dispc.c
index 00ad6b2..3e802b7 100644
--- a/drivers/video/omap/dispc.c
+++ b/drivers/video/omap/dispc.c
@@ -156,7 +156,7 @@ struct resmap {
};
static struct {
- u32 base;
+ void __iomem *base;
struct omapfb_mem_desc mem_desc;
struct resmap *res_map[DISPC_MEMTYPE_NUM];
@@ -212,9 +212,9 @@ static void enable_rfbi_mode(int enable)
dispc_write_reg(DISPC_CONTROL, l);
/* Set bypass mode in RFBI module */
- l = __raw_readl(io_p2v(RFBI_CONTROL));
+ l = __raw_readl(IO_ADDRESS(RFBI_CONTROL));
l |= enable ? 0 : (1 << 1);
- __raw_writel(l, io_p2v(RFBI_CONTROL));
+ __raw_writel(l, IO_ADDRESS(RFBI_CONTROL));
}
static void set_lcd_data_lines(int data_lines)
@@ -1353,7 +1353,7 @@ static int omap_dispc_init(struct omapfb_device *fbdev, int ext_mode,
memset(&dispc, 0, sizeof(dispc));
- dispc.base = io_p2v(DISPC_BASE);
+ dispc.base = IO_ADDRESS(DISPC_BASE);
dispc.fbdev = fbdev;
dispc.ext_mode = ext_mode;
@@ -1417,7 +1417,7 @@ static int omap_dispc_init(struct omapfb_device *fbdev, int ext_mode,
}
/* L3 firewall setting: enable access to OCM RAM */
- __raw_writel(0x402000b0, io_p2v(0x680050a0));
+ __raw_writel(0x402000b0, IO_ADDRESS(0x680050a0));
if ((r = alloc_palette_ram()) < 0)
goto fail2;
diff --git a/drivers/video/omap/rfbi.c b/drivers/video/omap/rfbi.c
index 470920d..59f857e 100644
--- a/drivers/video/omap/rfbi.c
+++ b/drivers/video/omap/rfbi.c
@@ -59,7 +59,7 @@
#define DISPC_CONTROL 0x0040
static struct {
- u32 base;
+ void __iomem *base;
void (*lcdc_callback)(void *data);
void *lcdc_callback_data;
unsigned long l4_khz;
@@ -518,7 +518,7 @@ static int rfbi_init(struct omapfb_device *fbdev)
int r;
rfbi.fbdev = fbdev;
- rfbi.base = io_p2v(RFBI_BASE);
+ rfbi.base = IO_ADDRESS(RFBI_BASE);
if ((r = rfbi_get_clocks()) < 0)
return r;
diff --git a/drivers/video/omap/sossi.c b/drivers/video/omap/sossi.c
index b4f67fc..9839d35 100644
--- a/drivers/video/omap/sossi.c
+++ b/drivers/video/omap/sossi.c
@@ -574,7 +574,7 @@ static int sossi_init(struct omapfb_device *fbdev)
struct clk *dpll1out_ck;
int r;
- sossi.base = (void __iomem *)IO_ADDRESS(OMAP_SOSSI_BASE);
+ sossi.base = IO_ADDRESS(OMAP_SOSSI_BASE);
sossi.fbdev = fbdev;
spin_lock_init(&sossi.lock);
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-08-27 22:08 FOR COMMENT: void __iomem * and similar casts are Bad News Russell King
@ 2008-08-31 21:47 ` David Brownell
2008-09-02 22:15 ` Tony Lindgren
2008-09-03 15:33 ` Eduardo Valentin
1 sibling, 1 reply; 58+ messages in thread
From: David Brownell @ 2008-08-31 21:47 UTC (permalink / raw)
To: Russell King; +Cc: linux-omap
On Wednesday 27 August 2008, Russell King wrote:
> I sent a similar patch to the one below against mainline to Tony today.
And I'm glad to see those updates. I used to constantly trip over
code doing strange stuff in those areas, and it would be nice to
see "sparse" approve a lot more driver code...
> I've marked some changes with certain tags:
>
> - FIXME: where I think the code is just plain wrong -
> - such as putting a physical address through a macro which converts
> virtual to physical addresses (1510 and 1610 mcbsp).
> - or passing virtual addresses to DMA functions (mcbsp).
> - or passing physical addresses when what's required is a virtual address
> (omap_udc).
Yeah, the omap_udc one looks like the __REG() conversion patch
just did the wrong thing. Fix looks trivial, and once I verify
it then I'll send it through mainline.
> - CHECKME: where I've changed something to make it build but I don't
> know if this is right.
>
> - WBNI: more a preference to see something changed than a bug.
>
> Comments on the FIXMEs are what I'm really interested in, and preferably
> having them actually fixed would be a good idea.
I verified that the OMAP tree boots on my OMAP5912 OSK, at least
once things like cpufreq, PM, and MTD are disabled. The MTD thing
is a known/solved issue in mainline. (Block queue changes broke
MTD...) The oopses in cpufreq and pm_idle are more cryptic.
So I can try my patch for the omap_udc thing ... even though for
OSK the mainline kernel ** DOES NOT BOOT ** and dies even before
the kernel "hello, I'm RC5!" banner prints. Someone with a JTAG
to throw at this might be able to fix that regression quickly.
- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-08-31 21:47 ` David Brownell
@ 2008-09-02 22:15 ` Tony Lindgren
2008-09-03 7:55 ` Russell King - ARM Linux
` (2 more replies)
0 siblings, 3 replies; 58+ messages in thread
From: Tony Lindgren @ 2008-09-02 22:15 UTC (permalink / raw)
To: David Brownell; +Cc: Russell King, linux-omap, Eduardo Valentin
Hi,
* David Brownell <david-b@pacbell.net> [080831 14:47]:
> On Wednesday 27 August 2008, Russell King wrote:
> > I sent a similar patch to the one below against mainline to Tony today.
>
> And I'm glad to see those updates. I used to constantly trip over
> code doing strange stuff in those areas, and it would be nice to
> see "sparse" approve a lot more driver code...
Back online now. These changes look ggood to me in general, except
I suggest that we use the following standard:
- Keep OMAP1_IO_ADDRESS() and OMAP2_IO_ADDRESS() as I have some
experimental patches to compile in both omap1 and omap2 into the
same binary. Booting the kernel currently still requires some
Makefile.boot patching, but at least compiling everything in makes
things easier to maintain in the long run.
- Use io_p2v() for initializing dynamic stuff as it can be a function
for non-optimized multiboot binaries.
>
> > I've marked some changes with certain tags:
> >
> > - FIXME: where I think the code is just plain wrong -
> > - such as putting a physical address through a macro which converts
> > virtual to physical addresses (1510 and 1610 mcbsp).
> > - or passing virtual addresses to DMA functions (mcbsp).
> > - or passing physical addresses when what's required is a virtual address
> > (omap_udc).
>
> Yeah, the omap_udc one looks like the __REG() conversion patch
> just did the wrong thing. Fix looks trivial, and once I verify
> it then I'll send it through mainline.
>
>
> > - CHECKME: where I've changed something to make it build but I don't
> > know if this is right.
> >
> > - WBNI: more a preference to see something changed than a bug.
> >
> > Comments on the FIXMEs are what I'm really interested in, and preferably
> > having them actually fixed would be a good idea.
>
> I verified that the OMAP tree boots on my OMAP5912 OSK, at least
> once things like cpufreq, PM, and MTD are disabled. The MTD thing
> is a known/solved issue in mainline. (Block queue changes broke
> MTD...) The oopses in cpufreq and pm_idle are more cryptic.
>
> So I can try my patch for the omap_udc thing ... even though for
> OSK the mainline kernel ** DOES NOT BOOT ** and dies even before
> the kernel "hello, I'm RC5!" banner prints. Someone with a JTAG
> to throw at this might be able to fix that regression quickly.
I'll take a look at the OSK boot problem unless somebody else is
already working on it.
Looks like the FIXME mcbsp phys vs virt addresses are wrong for some
board-*.c files. Eduardo, can you please take a look at them?
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-02 22:15 ` Tony Lindgren
@ 2008-09-03 7:55 ` Russell King - ARM Linux
2008-09-03 16:40 ` Tony Lindgren
2008-09-03 15:07 ` Eduardo Valentin
2008-09-03 18:01 ` Tony Lindgren
2 siblings, 1 reply; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-03 7:55 UTC (permalink / raw)
To: Tony Lindgren; +Cc: David Brownell, linux-omap, Eduardo Valentin
On Tue, Sep 02, 2008 at 03:15:10PM -0700, Tony Lindgren wrote:
> Back online now. These changes look ggood to me in general, except
> I suggest that we use the following standard:
>
> - Keep OMAP1_IO_ADDRESS() and OMAP2_IO_ADDRESS() as I have some
> experimental patches to compile in both omap1 and omap2 into the
> same binary. Booting the kernel currently still requires some
> Makefile.boot patching, but at least compiling everything in makes
> things easier to maintain in the long run.
Already decided to do that.
> - Use io_p2v() for initializing dynamic stuff as it can be a function
> for non-optimized multiboot binaries.
It can't become a function - it's used in structure initialization so
must be constant.
There's a few places where it's used where resources are passed into
drivers - in which case if the device is at a different physical address
it's the resources which should be changed, not the translation macro.
Anyway, I've put a modified version in my git tree.
I still want to hear on the other build fix in the patch, and there's
also a missing function for mmc stuff which I've not looked into yet.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-02 22:15 ` Tony Lindgren
2008-09-03 7:55 ` Russell King - ARM Linux
@ 2008-09-03 15:07 ` Eduardo Valentin
2008-09-03 18:01 ` Tony Lindgren
2 siblings, 0 replies; 58+ messages in thread
From: Eduardo Valentin @ 2008-09-03 15:07 UTC (permalink / raw)
To: Tony Lindgren; +Cc: David Brownell, Russell King, linux-omap
Hi Tony,
On Tue, Sep 2, 2008 at 6:15 PM, Tony Lindgren <tony@atomide.com> wrote:
> Hi,
>
> * David Brownell <david-b@pacbell.net> [080831 14:47]:
>> On Wednesday 27 August 2008, Russell King wrote:
>> > I sent a similar patch to the one below against mainline to Tony today.
>>
>> And I'm glad to see those updates. I used to constantly trip over
>> code doing strange stuff in those areas, and it would be nice to
>> see "sparse" approve a lot more driver code...
>
> Back online now. These changes look ggood to me in general, except
> I suggest that we use the following standard:
>
> - Keep OMAP1_IO_ADDRESS() and OMAP2_IO_ADDRESS() as I have some
> experimental patches to compile in both omap1 and omap2 into the
> same binary. Booting the kernel currently still requires some
> Makefile.boot patching, but at least compiling everything in makes
> things easier to maintain in the long run.
>
> - Use io_p2v() for initializing dynamic stuff as it can be a function
> for non-optimized multiboot binaries.
>
>>
>> > I've marked some changes with certain tags:
>> >
>> > - FIXME: where I think the code is just plain wrong -
>> > - such as putting a physical address through a macro which converts
>> > virtual to physical addresses (1510 and 1610 mcbsp).
>> > - or passing virtual addresses to DMA functions (mcbsp).
>> > - or passing physical addresses when what's required is a virtual address
>> > (omap_udc).
>>
>> Yeah, the omap_udc one looks like the __REG() conversion patch
>> just did the wrong thing. Fix looks trivial, and once I verify
>> it then I'll send it through mainline.
>>
>>
>> > - CHECKME: where I've changed something to make it build but I don't
>> > know if this is right.
>> >
>> > - WBNI: more a preference to see something changed than a bug.
>> >
>> > Comments on the FIXMEs are what I'm really interested in, and preferably
>> > having them actually fixed would be a good idea.
>>
>> I verified that the OMAP tree boots on my OMAP5912 OSK, at least
>> once things like cpufreq, PM, and MTD are disabled. The MTD thing
>> is a known/solved issue in mainline. (Block queue changes broke
>> MTD...) The oopses in cpufreq and pm_idle are more cryptic.
>>
>> So I can try my patch for the omap_udc thing ... even though for
>> OSK the mainline kernel ** DOES NOT BOOT ** and dies even before
>> the kernel "hello, I'm RC5!" banner prints. Someone with a JTAG
>> to throw at this might be able to fix that regression quickly.
>
> I'll take a look at the OSK boot problem unless somebody else is
> already working on it.
>
> Looks like the FIXME mcbsp phys vs virt addresses are wrong for some
> board-*.c files. Eduardo, can you please take a look at them?
Sorry, but I didn't get your point here. At least, for board-*.c files there
are only register values definitions. Those are used basically by old alsa
driver. No io address are defined there.
Anyway, I'll review the virtual address use for the mcbsp code.
>
> Tony
>
Cheers,
--
Eduardo Bezerra Valentin
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-08-27 22:08 FOR COMMENT: void __iomem * and similar casts are Bad News Russell King
2008-08-31 21:47 ` David Brownell
@ 2008-09-03 15:33 ` Eduardo Valentin
2008-09-03 18:48 ` Russell King
2008-09-04 9:46 ` Russell King - ARM Linux
1 sibling, 2 replies; 58+ messages in thread
From: Eduardo Valentin @ 2008-09-03 15:33 UTC (permalink / raw)
To: Russell King; +Cc: linux-omap
Hi,
On Wed, Aug 27, 2008 at 6:08 PM, Russell King <rmk@arm.linux.org.uk> wrote:
> ... are bad when overused. Unexpected bugs can creap in. That one cast
> which someone added to shut up a perfectly valid compiler warning now
> hides a potential problem.
>
> It is possible to eliminate _lots_ of these casts (and the potential
> for bad casts) by making things return the right types. Like, making
> IO_ADDRESS return "void __iomem *" rather than an integer.
>
> I sent a similar patch to the one below against mainline to Tony today.
> I've marked some changes with certain tags:
>
> - FIXME: where I think the code is just plain wrong -
> - such as putting a physical address through a macro which converts
> virtual to physical addresses (1510 and 1610 mcbsp).
> - or passing virtual addresses to DMA functions (mcbsp).
> - or passing physical addresses when what's required is a virtual address
> (omap_udc).
>
> - CHECKME: where I've changed something to make it build but I don't
> know if this is right.
>
> - WBNI: more a preference to see something changed than a bug.
>
> Comments on the FIXMEs are what I'm really interested in, and preferably
> having them actually fixed would be a good idea.
>
> I've compile tested this on a few omap defconfigs and it at least builds.
>
> diff --git a/arch/arm/mach-omap1/mcbsp.c b/arch/arm/mach-omap1/mcbsp.c
> index 265cfc2..e5ac038 100644
> --- a/arch/arm/mach-omap1/mcbsp.c
> +++ b/arch/arm/mach-omap1/mcbsp.c
> @@ -134,7 +134,7 @@ static struct omap_mcbsp_ops omap1_mcbsp_ops = {
> #ifdef CONFIG_ARCH_OMAP730
> static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
> {
> - .virt_base = io_p2v(OMAP730_MCBSP1_BASE),
> + .virt_base = IO_ADDRESS(OMAP730_MCBSP1_BASE),
> .dma_rx_sync = OMAP_DMA_MCBSP1_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP1_TX,
> .rx_irq = INT_730_McBSP1RX,
> @@ -142,7 +142,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
> .ops = &omap1_mcbsp_ops,
> },
> {
> - .virt_base = io_p2v(OMAP730_MCBSP2_BASE),
> + .virt_base = IO_ADDRESS(OMAP730_MCBSP2_BASE),
> .dma_rx_sync = OMAP_DMA_MCBSP3_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP3_TX,
> .rx_irq = INT_730_McBSP2RX,
> @@ -159,7 +159,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
> #ifdef CONFIG_ARCH_OMAP15XX
> static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> {
> - .virt_base = OMAP1510_MCBSP1_BASE,
> + .virt_base = OMAP1510_MCBSP1_BASE, /* FIXME: virtual or physical */
AFAIK, OMAP1510_MCBSP1_BASE is physical. So, I'd say:
+ .virt_base = IO_ADDRESS(OMAP1510_MCBSP1_BASE),
Because, plat-omap/mcbsp.c expect .virt_base to be a virtual address.
> .dma_rx_sync = OMAP_DMA_MCBSP1_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP1_TX,
> .rx_irq = INT_McBSP1RX,
> @@ -168,7 +168,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> .clk_name = "mcbsp_clk",
> },
> {
> - .virt_base = io_p2v(OMAP1510_MCBSP2_BASE),
> + .virt_base = IO_ADDRESS(OMAP1510_MCBSP2_BASE),
> .dma_rx_sync = OMAP_DMA_MCBSP2_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP2_TX,
> .rx_irq = INT_1510_SPI_RX,
> @@ -176,7 +176,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> .ops = &omap1_mcbsp_ops,
> },
> {
> - .virt_base = OMAP1510_MCBSP3_BASE,
> + .virt_base = OMAP1510_MCBSP3_BASE, /* FIXME: virtual or physical */
Same here.
> .dma_rx_sync = OMAP_DMA_MCBSP3_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP3_TX,
> .rx_irq = INT_McBSP3RX,
> @@ -194,7 +194,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> #ifdef CONFIG_ARCH_OMAP16XX
> static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
> {
> - .virt_base = OMAP1610_MCBSP1_BASE,
> + .virt_base = OMAP1610_MCBSP1_BASE, /* FIXME: virtual or physical */
And here.
> .dma_rx_sync = OMAP_DMA_MCBSP1_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP1_TX,
> .rx_irq = INT_McBSP1RX,
> @@ -203,7 +203,7 @@ static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
> .clk_name = "mcbsp_clk",
> },
> {
> - .virt_base = io_p2v(OMAP1610_MCBSP2_BASE),
> + .virt_base = IO_ADDRESS(OMAP1610_MCBSP2_BASE),
> .dma_rx_sync = OMAP_DMA_MCBSP2_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP2_TX,
> .rx_irq = INT_1610_McBSP2_RX,
> @@ -211,7 +211,7 @@ static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
> .ops = &omap1_mcbsp_ops,
> },
> {
> - .virt_base = OMAP1610_MCBSP3_BASE,
> + .virt_base = OMAP1610_MCBSP3_BASE, /* FIXME: virtual or physical */
And here.
> .dma_rx_sync = OMAP_DMA_MCBSP3_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP3_TX,
> .rx_irq = INT_McBSP3RX,
> diff --git a/arch/arm/mach-omap1/serial.c b/arch/arm/mach-omap1/serial.c
> index 0e25a99..4965986 100644
> --- a/arch/arm/mach-omap1/serial.c
> +++ b/arch/arm/mach-omap1/serial.c
> @@ -67,8 +67,8 @@ static void __init omap_serial_reset(struct plat_serial8250_port *p)
>
> static struct plat_serial8250_port serial_platform_data[] = {
> {
> - .membase = (char*)IO_ADDRESS(OMAP_UART1_BASE),
> - .mapbase = (unsigned long)OMAP_UART1_BASE,
> + .membase = IO_ADDRESS(OMAP_UART1_BASE),
> + .mapbase = OMAP_UART1_BASE,
> .irq = INT_UART1,
> .flags = UPF_BOOT_AUTOCONF,
> .iotype = UPIO_MEM,
> @@ -76,8 +76,8 @@ static struct plat_serial8250_port serial_platform_data[] = {
> .uartclk = OMAP16XX_BASE_BAUD * 16,
> },
> {
> - .membase = (char*)IO_ADDRESS(OMAP_UART2_BASE),
> - .mapbase = (unsigned long)OMAP_UART2_BASE,
> + .membase = IO_ADDRESS(OMAP_UART2_BASE),
> + .mapbase = OMAP_UART2_BASE,
> .irq = INT_UART2,
> .flags = UPF_BOOT_AUTOCONF,
> .iotype = UPIO_MEM,
> @@ -85,8 +85,8 @@ static struct plat_serial8250_port serial_platform_data[] = {
> .uartclk = OMAP16XX_BASE_BAUD * 16,
> },
> {
> - .membase = (char*)IO_ADDRESS(OMAP_UART3_BASE),
> - .mapbase = (unsigned long)OMAP_UART3_BASE,
> + .membase = IO_ADDRESS(OMAP_UART3_BASE),
> + .mapbase = OMAP_UART3_BASE,
> .irq = INT_UART3,
> .flags = UPF_BOOT_AUTOCONF,
> .iotype = UPIO_MEM,
> diff --git a/arch/arm/mach-omap2/clock.c b/arch/arm/mach-omap2/clock.c
> index be5c616..0c6a1b4 100644
> --- a/arch/arm/mach-omap2/clock.c
> +++ b/arch/arm/mach-omap2/clock.c
> @@ -223,10 +223,11 @@ int omap2_wait_clock_ready(void __iomem *reg, u32 mask, const char *name)
> static void omap2_clk_wait_ready(struct clk *clk)
> {
> u32 other_bit, idlest_bit;
> - unsigned long reg, other_reg, idlest_reg, prcm_mod, prcm_regid;
> + unsigned long reg, other_reg, idlest_reg, prcm_regid;
> + void __iomem *prcm_mod;
>
> reg = (unsigned long)clk->enable_reg;
> - prcm_mod = reg & ~0xff;
> + prcm_mod = (void __iomem *)(reg & ~0xff);
> prcm_regid = reg & 0xff;
>
> if (prcm_regid >= CM_FCLKEN1 && prcm_regid <= OMAP24XX_CM_FCLKEN2)
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 9d6c916..7e3de6e 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -59,34 +59,34 @@ static struct resource gpmc_cs_mem[GPMC_CS_NUM];
> static DEFINE_SPINLOCK(gpmc_mem_lock);
> static unsigned gpmc_cs_map;
>
> -static u32 gpmc_base;
> +static void __iomem *gpmc_base;
>
> static struct clk *gpmc_l3_clk;
>
> static void gpmc_write_reg(int idx, u32 val)
> {
> - __raw_writel(val, (__force void __iomem *)(gpmc_base + idx));
> + __raw_writel(val, gpmc_base + idx);
> }
>
> static u32 gpmc_read_reg(int idx)
> {
> - return __raw_readl((__force void __iomem *)(gpmc_base + idx));
> + return __raw_readl(gpmc_base + idx);
> }
>
> void gpmc_cs_write_reg(int cs, int idx, u32 val)
> {
> - u32 reg_addr;
> + void __iomem *reg_addr;
>
> reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
> - __raw_writel(val, (__force void __iomem *)reg_addr);
> + __raw_writel(val, reg_addr);
> }
>
> u32 gpmc_cs_read_reg(int cs, int idx)
> {
> - u32 reg_addr;
> + void __iomem *reg_addr;
>
> reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
> - return __raw_readl((__force void __iomem *)reg_addr);
> + return __raw_readl(reg_addr);
> }
>
> /* TODO: Add support for gpmc_fck to clock framework and use it */
> @@ -417,12 +417,12 @@ void __init gpmc_init(void)
> if (cpu_is_omap24xx()) {
> gpmc_l3_clk = clk_get(NULL, "core_l3_ck");
> if (cpu_is_omap2420())
> - gpmc_base = io_p2v(OMAP2420_GPMC_BASE);
> + gpmc_base = IO_ADDRESS(OMAP2420_GPMC_BASE);
> else if (cpu_is_omap2430())
> - gpmc_base = io_p2v(OMAP243X_GPMC_BASE);
> + gpmc_base = IO_ADDRESS(OMAP243X_GPMC_BASE);
> } else if (cpu_is_omap34xx()) {
> gpmc_l3_clk = clk_get(NULL, "gpmc_fck");
> - gpmc_base = io_p2v(OMAP34XX_GPMC_BASE);
> + gpmc_base = IO_ADDRESS(OMAP34XX_GPMC_BASE);
> }
>
> BUG_ON(IS_ERR(gpmc_l3_clk));
> diff --git a/arch/arm/mach-omap2/irq.c b/arch/arm/mach-omap2/irq.c
> index 82e954a..7014505 100644
> --- a/arch/arm/mach-omap2/irq.c
> +++ b/arch/arm/mach-omap2/irq.c
> @@ -38,7 +38,7 @@
> * for each bank.. when in doubt, consult the TRM.
> */
> static struct omap_irq_bank {
> - unsigned long base_reg;
> + void __iomem *base_reg;
> unsigned int nr_irqs;
> } __attribute__ ((aligned(4))) irq_banks[] = {
> {
> @@ -52,12 +52,12 @@ static struct omap_irq_bank {
>
> static void intc_bank_write_reg(u32 val, struct omap_irq_bank *bank, u16 reg)
> {
> - __raw_writel(val, (__force void __iomem *)(bank->base_reg + reg));
> + __raw_writel(val, bank->base_reg + reg);
> }
>
> static u32 intc_bank_read_reg(struct omap_irq_bank *bank, u16 reg)
> {
> - return __raw_readl((__force void __iomem *)(bank->base_reg + reg));
> + return __raw_readl(bank->base_reg + reg);
> }
>
> /* XXX: FIQ and additional INTC support (only MPU at the moment) */
> @@ -102,7 +102,7 @@ static void __init omap_irq_bank_init_one(struct omap_irq_bank *bank)
> unsigned long tmp;
>
> tmp = intc_bank_read_reg(bank, INTC_REVISION) & 0xff;
> - printk(KERN_INFO "IRQ: Found an INTC at 0x%08lx "
> + printk(KERN_INFO "IRQ: Found an INTC at 0x%p "
> "(revision %ld.%ld) with %d interrupts\n",
> bank->base_reg, tmp >> 4, tmp & 0xf, bank->nr_irqs);
>
> @@ -147,9 +147,9 @@ void __init omap_init_irq(void)
> struct omap_irq_bank *bank = irq_banks + i;
>
> if (cpu_is_omap24xx())
> - bank->base_reg = io_p2v(OMAP24XX_IC_BASE);
> + bank->base_reg = IO_ADDRESS(OMAP24XX_IC_BASE);
> else if (cpu_is_omap34xx())
> - bank->base_reg = io_p2v(OMAP34XX_IC_BASE);
> + bank->base_reg = IO_ADDRESS(OMAP34XX_IC_BASE);
>
> omap_irq_bank_init_one(bank);
>
> diff --git a/arch/arm/mach-omap2/mcbsp.c b/arch/arm/mach-omap2/mcbsp.c
> index cedb9a6..bf6f413 100644
> --- a/arch/arm/mach-omap2/mcbsp.c
> +++ b/arch/arm/mach-omap2/mcbsp.c
> @@ -148,7 +148,7 @@ static struct omap_mcbsp_ops omap2_mcbsp_ops = {
> #ifdef CONFIG_ARCH_OMAP24XX
> static struct omap_mcbsp_platform_data omap24xx_mcbsp_pdata[] = {
> {
> - .virt_base = OMAP2_IO_ADDRESS(OMAP24XX_MCBSP1_BASE),
> + .virt_base = IO_ADDRESS(OMAP24XX_MCBSP1_BASE),
> .dma_rx_sync = OMAP24XX_DMA_MCBSP1_RX,
> .dma_tx_sync = OMAP24XX_DMA_MCBSP1_TX,
> .rx_irq = INT_24XX_MCBSP1_IRQ_RX,
> @@ -157,7 +157,7 @@ static struct omap_mcbsp_platform_data omap24xx_mcbsp_pdata[] = {
> .clk_name = "mcbsp_clk",
> },
> {
> - .virt_base = OMAP2_IO_ADDRESS(OMAP24XX_MCBSP2_BASE),
> + .virt_base = IO_ADDRESS(OMAP24XX_MCBSP2_BASE),
> .dma_rx_sync = OMAP24XX_DMA_MCBSP2_RX,
> .dma_tx_sync = OMAP24XX_DMA_MCBSP2_TX,
> .rx_irq = INT_24XX_MCBSP2_IRQ_RX,
> @@ -175,7 +175,7 @@ static struct omap_mcbsp_platform_data omap24xx_mcbsp_pdata[] = {
> #ifdef CONFIG_ARCH_OMAP34XX
> static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
> {
> - .virt_base = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP1_BASE),
> + .virt_base = IO_ADDRESS(OMAP34XX_MCBSP1_BASE),
> .dma_rx_sync = OMAP24XX_DMA_MCBSP1_RX,
> .dma_tx_sync = OMAP24XX_DMA_MCBSP1_TX,
> .rx_irq = INT_24XX_MCBSP1_IRQ_RX,
> @@ -184,7 +184,7 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
> .clk_name = "mcbsp_clk",
> },
> {
> - .virt_base = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP2_BASE),
> + .virt_base = IO_ADDRESS(OMAP34XX_MCBSP2_BASE),
> .dma_rx_sync = OMAP24XX_DMA_MCBSP2_RX,
> .dma_tx_sync = OMAP24XX_DMA_MCBSP2_TX,
> .rx_irq = INT_24XX_MCBSP2_IRQ_RX,
> @@ -193,21 +193,21 @@ static struct omap_mcbsp_platform_data omap34xx_mcbsp_pdata[] = {
> .clk_name = "mcbsp_clk",
> },
> {
> - .virt_base = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP3_BASE),
> + .virt_base = IO_ADDRESS(OMAP34XX_MCBSP3_BASE),
> .dma_rx_sync = OMAP24XX_DMA_MCBSP3_RX,
> .dma_tx_sync = OMAP24XX_DMA_MCBSP3_TX,
> .ops = &omap2_mcbsp_ops,
> .clk_name = "mcbsp_clk",
> },
> {
> - .virt_base = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP4_BASE),
> + .virt_base = IO_ADDRESS(OMAP34XX_MCBSP4_BASE),
> .dma_rx_sync = OMAP24XX_DMA_MCBSP4_RX,
> .dma_tx_sync = OMAP24XX_DMA_MCBSP4_TX,
> .ops = &omap2_mcbsp_ops,
> .clk_name = "mcbsp_clk",
> },
> {
> - .virt_base = OMAP2_IO_ADDRESS(OMAP34XX_MCBSP5_BASE),
> + .virt_base = IO_ADDRESS(OMAP34XX_MCBSP5_BASE),
> .dma_rx_sync = OMAP24XX_DMA_MCBSP5_RX,
> .dma_tx_sync = OMAP24XX_DMA_MCBSP5_TX,
> .ops = &omap2_mcbsp_ops,
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index bd6f8c9..dbbef90 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -28,24 +28,24 @@ static struct clk *uart_fck[OMAP_MAX_NR_PORTS];
>
> static struct plat_serial8250_port serial_platform_data[] = {
> {
> - .membase = (void __iomem *)IO_ADDRESS(OMAP_UART1_BASE),
> - .mapbase = (unsigned long)OMAP_UART1_BASE,
> + .membase = IO_ADDRESS(OMAP_UART1_BASE),
> + .mapbase = OMAP_UART1_BASE,
> .irq = 72,
> .flags = UPF_BOOT_AUTOCONF,
> .iotype = UPIO_MEM,
> .regshift = 2,
> .uartclk = OMAP24XX_BASE_BAUD * 16,
> }, {
> - .membase = (void __iomem *)IO_ADDRESS(OMAP_UART2_BASE),
> - .mapbase = (unsigned long)OMAP_UART2_BASE,
> + .membase = IO_ADDRESS(OMAP_UART2_BASE),
> + .mapbase = OMAP_UART2_BASE,
> .irq = 73,
> .flags = UPF_BOOT_AUTOCONF,
> .iotype = UPIO_MEM,
> .regshift = 2,
> .uartclk = OMAP24XX_BASE_BAUD * 16,
> }, {
> - .membase = (void __iomem *)IO_ADDRESS(OMAP_UART3_BASE),
> - .mapbase = (unsigned long)OMAP_UART3_BASE,
> + .membase = IO_ADDRESS(OMAP_UART3_BASE),
> + .mapbase = OMAP_UART3_BASE,
> .irq = 74,
> .flags = UPF_BOOT_AUTOCONF,
> .iotype = UPIO_MEM,
> diff --git a/arch/arm/plat-omap/common.c b/arch/arm/plat-omap/common.c
> index 2914af0..3cf726b 100644
> --- a/arch/arm/plat-omap/common.c
> +++ b/arch/arm/plat-omap/common.c
> @@ -281,12 +281,12 @@ static void __init __omap2_set_globals(void)
>
> static struct omap_globals omap242x_globals = {
> .class = OMAP242X_CLASS,
> - .tap = (__force void __iomem *)OMAP2_IO_ADDRESS(0x48014000),
> - .sdrc = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_SDRC_BASE),
> - .sms = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_SMS_BASE),
> - .ctrl = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_CTRL_BASE),
> - .prm = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_PRM_BASE),
> - .cm = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2420_CM_BASE),
> + .tap = IO_ADDRESS(0x48014000),
> + .sdrc = IO_ADDRESS(OMAP2420_SDRC_BASE),
> + .sms = IO_ADDRESS(OMAP2420_SMS_BASE),
> + .ctrl = IO_ADDRESS(OMAP2420_CTRL_BASE),
> + .prm = IO_ADDRESS(OMAP2420_PRM_BASE),
> + .cm = IO_ADDRESS(OMAP2420_CM_BASE),
> };
>
> void __init omap2_set_globals_242x(void)
> @@ -300,12 +300,12 @@ void __init omap2_set_globals_242x(void)
>
> static struct omap_globals omap243x_globals = {
> .class = OMAP243X_CLASS,
> - .tap = (__force void __iomem *)OMAP2_IO_ADDRESS(0x4900a000),
> - .sdrc = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP243X_SDRC_BASE),
> - .sms = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP243X_SMS_BASE),
> - .ctrl = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP243X_CTRL_BASE),
> - .prm = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2430_PRM_BASE),
> - .cm = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP2430_CM_BASE),
> + .tap = IO_ADDRESS(0x4900a000),
> + .sdrc = IO_ADDRESS(OMAP243X_SDRC_BASE),
> + .sms = IO_ADDRESS(OMAP243X_SMS_BASE),
> + .ctrl = IO_ADDRESS(OMAP243X_CTRL_BASE),
> + .prm = IO_ADDRESS(OMAP2430_PRM_BASE),
> + .cm = IO_ADDRESS(OMAP2430_CM_BASE),
> };
>
> void __init omap2_set_globals_243x(void)
> @@ -319,12 +319,12 @@ void __init omap2_set_globals_243x(void)
>
> static struct omap_globals omap343x_globals = {
> .class = OMAP343X_CLASS,
> - .tap = (__force void __iomem *)OMAP2_IO_ADDRESS(0x4830A000),
> - .sdrc = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP343X_SDRC_BASE),
> - .sms = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP343X_SMS_BASE),
> - .ctrl = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP343X_CTRL_BASE),
> - .prm = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP3430_PRM_BASE),
> - .cm = (__force void __iomem *)OMAP2_IO_ADDRESS(OMAP3430_CM_BASE),
> + .tap = IO_ADDRESS(0x4830A000),
> + .sdrc = IO_ADDRESS(OMAP343X_SDRC_BASE),
> + .sms = IO_ADDRESS(OMAP343X_SMS_BASE),
> + .ctrl = IO_ADDRESS(OMAP343X_CTRL_BASE),
> + .prm = IO_ADDRESS(OMAP3430_PRM_BASE),
> + .cm = IO_ADDRESS(OMAP3430_CM_BASE),
> };
>
> void __init omap2_set_globals_343x(void)
> diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
> index 1c6b905..bb3f187 100644
> --- a/arch/arm/plat-omap/dma.c
> +++ b/arch/arm/plat-omap/dma.c
> @@ -2298,13 +2298,13 @@ static int __init omap_init_dma(void)
> int ch, r;
>
> if (cpu_class_is_omap1()) {
> - omap_dma_base = (void __iomem *)IO_ADDRESS(OMAP1_DMA_BASE);
> + omap_dma_base = IO_ADDRESS(OMAP1_DMA_BASE);
> dma_lch_count = OMAP1_LOGICAL_DMA_CH_COUNT;
> } else if (cpu_is_omap24xx()) {
> - omap_dma_base = (void __iomem *)IO_ADDRESS(OMAP24XX_DMA4_BASE);
> + omap_dma_base = IO_ADDRESS(OMAP24XX_DMA4_BASE);
> dma_lch_count = OMAP_DMA4_LOGICAL_DMA_CH_COUNT;
> } else if (cpu_is_omap34xx()) {
> - omap_dma_base = (void __iomem *)IO_ADDRESS(OMAP34XX_DMA4_BASE);
> + omap_dma_base = IO_ADDRESS(OMAP34XX_DMA4_BASE);
> dma_lch_count = OMAP_DMA4_LOGICAL_DMA_CH_COUNT;
> } else {
> pr_err("DMA init failed for unsupported omap\n");
> diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
> index 8b4b553..167ec2f 100644
> --- a/arch/arm/plat-omap/dmtimer.c
> +++ b/arch/arm/plat-omap/dmtimer.c
> @@ -693,7 +693,7 @@ int __init omap_dm_timer_init(void)
>
> for (i = 0; i < dm_timer_count; i++) {
> timer = &dm_timers[i];
> - timer->io_base = (void __iomem *)io_p2v(timer->phys_base);
> + timer->io_base = IO_ADDRESS(timer->phys_base);
> #if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
> if (cpu_class_is_omap2()) {
> char clk_name[16];
> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
> index 3e76ee2..dee380b 100644
> --- a/arch/arm/plat-omap/gpio.c
> +++ b/arch/arm/plat-omap/gpio.c
> @@ -1488,7 +1488,7 @@ static int __init _omap_gpio_init(void)
> bank->chip.set = gpio_set;
> if (bank_is_mpuio(bank)) {
> bank->chip.label = "mpuio";
> -#ifdef CONFIG_ARCH_OMAP1
> +#ifdef CONFIG_ARCH_OMAP16XX /* CHECKME */
> bank->chip.dev = &omap_mpuio_device.dev;
> #endif
> bank->chip.base = OMAP_MPUIO(0);
> diff --git a/arch/arm/plat-omap/include/mach/control.h b/arch/arm/plat-omap/include/mach/control.h
> index 1cd98fd..91d7480 100644
> --- a/arch/arm/plat-omap/include/mach/control.h
> +++ b/arch/arm/plat-omap/include/mach/control.h
> @@ -18,18 +18,9 @@
>
> #include <mach/io.h>
>
> -#ifndef __ASSEMBLY__
> -#define OMAP242X_CTRL_REGADDR(reg) \
> - (void __iomem *)IO_ADDRESS(OMAP242X_CTRL_BASE + (reg))
> -#define OMAP243X_CTRL_REGADDR(reg) \
> - (void __iomem *)IO_ADDRESS(OMAP243X_CTRL_BASE + (reg))
> -#define OMAP343X_CTRL_REGADDR(reg) \
> - (void __iomem *)IO_ADDRESS(OMAP343X_CTRL_BASE + (reg))
> -#else
> #define OMAP242X_CTRL_REGADDR(reg) IO_ADDRESS(OMAP242X_CTRL_BASE + (reg))
> #define OMAP243X_CTRL_REGADDR(reg) IO_ADDRESS(OMAP243X_CTRL_BASE + (reg))
> #define OMAP343X_CTRL_REGADDR(reg) IO_ADDRESS(OMAP343X_CTRL_BASE + (reg))
> -#endif /* __ASSEMBLY__ */
>
> /*
> * As elsewhere, the "OMAP2_" prefix indicates that the macro is valid for
> diff --git a/arch/arm/plat-omap/include/mach/io.h b/arch/arm/plat-omap/include/mach/io.h
> index 7d3481b..5126a0a 100644
> --- a/arch/arm/plat-omap/include/mach/io.h
> +++ b/arch/arm/plat-omap/include/mach/io.h
> @@ -55,14 +55,12 @@
>
> #if defined(CONFIG_ARCH_OMAP1)
>
> -#define IO_PHYS 0xFFFB0000
> -#define IO_OFFSET 0x01000000 /* Virtual IO = 0xfefb0000 */
> -#define IO_SIZE 0x40000
> -#define IO_VIRT (IO_PHYS - IO_OFFSET)
> -#define IO_ADDRESS(pa) ((pa) - IO_OFFSET)
> -#define OMAP1_IO_ADDRESS(pa) ((pa) - IO_OFFSET)
> -#define io_p2v(pa) ((pa) - IO_OFFSET)
> -#define io_v2p(va) ((va) + IO_OFFSET)
> +#define IO_PHYS 0xFFFB0000
> +#define IO_OFFSET 0x01000000 /* Virtual IO = 0xfefb0000 */
> +#define IO_SIZE 0x40000
> +#define IO_VIRT (IO_PHYS - IO_OFFSET)
> +#define __IO_ADDRESS(pa) ((pa) - IO_OFFSET)
> +#define io_v2p(va) ((va) + IO_OFFSET)
>
> #elif defined(CONFIG_ARCH_OMAP2)
>
> @@ -90,11 +88,9 @@
>
> #endif
>
> -#define IO_OFFSET 0x90000000
> -#define IO_ADDRESS(pa) ((pa) + IO_OFFSET) /* Works for L3 and L4 */
> -#define OMAP2_IO_ADDRESS(pa) ((pa) + IO_OFFSET) /* Works for L3 and L4 */
> -#define io_p2v(pa) ((pa) + IO_OFFSET) /* Works for L3 and L4 */
> -#define io_v2p(va) ((va) - IO_OFFSET) /* Works for L3 and L4 */
> +#define IO_OFFSET 0x90000000
> +#define __IO_ADDRESS(pa) ((pa) + IO_OFFSET) /* Works for L3 and L4 */
> +#define io_v2p(va) ((va) - IO_OFFSET) /* Works for L3 and L4 */
>
> /* DSP */
> #define DSP_MEM_24XX_PHYS OMAP2420_DSP_MEM_BASE /* 0x58000000 */
> @@ -149,9 +145,7 @@
>
>
> #define IO_OFFSET 0x90000000
> -#define IO_ADDRESS(pa) ((pa) + IO_OFFSET)/* Works for L3 and L4 */
> -#define OMAP2_IO_ADDRESS(pa) ((pa) + IO_OFFSET)/* Works for L3 and L4 */
> -#define io_p2v(pa) ((pa) + IO_OFFSET)/* Works for L3 and L4 */
> +#define __IO_ADDRESS(pa) ((pa) + IO_OFFSET)/* Works for L3 and L4 */
> #define io_v2p(va) ((va) - IO_OFFSET)/* Works for L3 and L4 */
>
> /* DSP */
> @@ -167,7 +161,13 @@
>
> #endif
>
> -#ifndef __ASSEMBLER__
> +#ifdef __ASSEMBLER__
> +
> +#define IO_ADDRESS(pa) __IO_ADDRESS(pa)
> +
> +#else
> +
> +#define IO_ADDRESS(pa) ((void __iomem *)__IO_ADDRESS(pa))
>
> /*
> * Functions to access the OMAP IO region
> diff --git a/arch/arm/plat-omap/include/mach/mcbsp.h b/arch/arm/plat-omap/include/mach/mcbsp.h
> index c04bcd5..3c02bf6 100644
> --- a/arch/arm/plat-omap/include/mach/mcbsp.h
> +++ b/arch/arm/plat-omap/include/mach/mcbsp.h
> @@ -326,7 +326,7 @@ struct omap_mcbsp_ops {
> };
>
> struct omap_mcbsp_platform_data {
> - u32 virt_base;
> + void __iomem *virt_base;
> u8 dma_rx_sync, dma_tx_sync;
> u16 rx_irq, tx_irq;
> struct omap_mcbsp_ops *ops;
> @@ -335,7 +335,7 @@ struct omap_mcbsp_platform_data {
>
> struct omap_mcbsp {
> struct device *dev;
> - u32 io_base;
> + void __iomem *io_base;
> u8 id;
> u8 free;
> omap_mcbsp_word_length rx_word_length;
> diff --git a/arch/arm/plat-omap/include/mach/pm.h b/arch/arm/plat-omap/include/mach/pm.h
> index 5a1f3db..e4cf1c5 100644
> --- a/arch/arm/plat-omap/include/mach/pm.h
> +++ b/arch/arm/plat-omap/include/mach/pm.h
> @@ -39,11 +39,11 @@
> * Register and offset definitions to be used in PM assembler code
> * ----------------------------------------------------------------------------
> */
> -#define CLKGEN_REG_ASM_BASE io_p2v(0xfffece00)
> +#define CLKGEN_REG_ASM_BASE IO_ADDRESS(0xfffece00)
> #define ARM_IDLECT1_ASM_OFFSET 0x04
> #define ARM_IDLECT2_ASM_OFFSET 0x08
>
> -#define TCMIF_ASM_BASE io_p2v(0xfffecc00)
> +#define TCMIF_ASM_BASE IO_ADDRESS(0xfffecc00)
> #define EMIFS_CONFIG_ASM_OFFSET 0x0c
> #define EMIFF_SDRAM_CONFIG_ASM_OFFSET 0x20
>
> diff --git a/arch/arm/plat-omap/include/mach/sdrc.h b/arch/arm/plat-omap/include/mach/sdrc.h
> index b7db862..c66c838 100644
> --- a/arch/arm/plat-omap/include/mach/sdrc.h
> +++ b/arch/arm/plat-omap/include/mach/sdrc.h
> @@ -75,12 +75,9 @@
> * SMS register access
> */
>
> -#define OMAP242X_SMS_REGADDR(reg) \
> - (void __iomem *)IO_ADDRESS(OMAP2420_SMS_BASE + reg)
> -#define OMAP243X_SMS_REGADDR(reg) \
> - (void __iomem *)IO_ADDRESS(OMAP243X_SMS_BASE + reg)
> -#define OMAP343X_SMS_REGADDR(reg) \
> - (void __iomem *)IO_ADDRESS(OMAP343X_SMS_BASE + reg)
> +#define OMAP242X_SMS_REGADDR(reg) IO_ADDRESS(OMAP2420_SMS_BASE + reg)
> +#define OMAP243X_SMS_REGADDR(reg) IO_ADDRESS(OMAP243X_SMS_BASE + reg)
> +#define OMAP343X_SMS_REGADDR(reg) IO_ADDRESS(OMAP343X_SMS_BASE + reg)
>
> /* SMS register offsets - read/write with sms_{read,write}_reg() */
>
> diff --git a/arch/arm/plat-omap/include/mach/serial.h b/arch/arm/plat-omap/include/mach/serial.h
> index abea6ef..c6b7f59 100644
> --- a/arch/arm/plat-omap/include/mach/serial.h
> +++ b/arch/arm/plat-omap/include/mach/serial.h
> @@ -33,9 +33,9 @@
> #define OMAP24XX_BASE_BAUD (48000000/16)
>
> #define is_omap_port(p) ({int __ret = 0; \
> - if (p == IO_ADDRESS(OMAP_UART1_BASE) || \
> - p == IO_ADDRESS(OMAP_UART2_BASE) || \
> - p == IO_ADDRESS(OMAP_UART3_BASE)) \
> + if (p == OMAP_UART1_BASE || \
> + p == OMAP_UART2_BASE || \
> + p == OMAP_UART3_BASE) \
> __ret = 1; \
> __ret; \
> })
> diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
> index 0f0e3f3..51d715f 100644
> --- a/arch/arm/plat-omap/mcbsp.c
> +++ b/arch/arm/plat-omap/mcbsp.c
> @@ -30,7 +30,7 @@
> struct omap_mcbsp **mcbsp_ptr;
> int omap_mcbsp_count;
>
> -void omap_mcbsp_write(u32 io_base, u16 reg, u32 val)
> +void omap_mcbsp_write(void __iomem *io_base, u16 reg, u32 val)
> {
> if (cpu_class_is_omap1() || cpu_is_omap2420())
> __raw_writew((u16)val, io_base + reg);
> @@ -38,7 +38,7 @@ void omap_mcbsp_write(u32 io_base, u16 reg, u32 val)
> __raw_writel(val, io_base + reg);
> }
>
> -int omap_mcbsp_read(u32 io_base, u16 reg)
> +int omap_mcbsp_read(void __iomem *io_base, u16 reg)
> {
> if (cpu_class_is_omap1() || cpu_is_omap2420())
> return __raw_readw(io_base + reg);
> @@ -149,7 +149,7 @@ static void omap_mcbsp_rx_dma_callback(int lch, u16 ch_status, void *data)
> void omap_mcbsp_config(unsigned int id, const struct omap_mcbsp_reg_cfg *config)
> {
> struct omap_mcbsp *mcbsp;
> - u32 io_base;
> + void __iomem *io_base;
>
> if (!omap_mcbsp_check_valid_id(id)) {
> printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
> @@ -158,7 +158,7 @@ void omap_mcbsp_config(unsigned int id, const struct omap_mcbsp_reg_cfg *config)
> mcbsp = id_to_mcbsp_ptr(id);
>
> io_base = mcbsp->io_base;
> - dev_dbg(mcbsp->dev, "Configuring McBSP%d io_base: 0x%8x\n",
> + dev_dbg(mcbsp->dev, "Configuring McBSP%d io_base: 0x%p\n",
> mcbsp->id, io_base);
>
> /* We write the given config */
> @@ -306,7 +306,7 @@ EXPORT_SYMBOL(omap_mcbsp_free);
> void omap_mcbsp_start(unsigned int id)
> {
> struct omap_mcbsp *mcbsp;
> - u32 io_base;
> + void __iomem *io_base;
> u16 w;
>
> if (!omap_mcbsp_check_valid_id(id)) {
> @@ -344,7 +344,7 @@ EXPORT_SYMBOL(omap_mcbsp_start);
> void omap_mcbsp_stop(unsigned int id)
> {
> struct omap_mcbsp *mcbsp;
> - u32 io_base;
> + void __iomem *io_base;
> u16 w;
>
> if (!omap_mcbsp_check_valid_id(id)) {
> @@ -373,7 +373,7 @@ EXPORT_SYMBOL(omap_mcbsp_stop);
> int omap_mcbsp_pollwrite(unsigned int id, u16 buf)
> {
> struct omap_mcbsp *mcbsp;
> - u32 base;
> + void __iomem *base;
>
> if (!omap_mcbsp_check_valid_id(id)) {
> printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
> @@ -418,7 +418,7 @@ EXPORT_SYMBOL(omap_mcbsp_pollwrite);
> int omap_mcbsp_pollread(unsigned int id, u16 *buf)
> {
> struct omap_mcbsp *mcbsp;
> - u32 base;
> + void __iomem *base;
>
> if (!omap_mcbsp_check_valid_id(id)) {
> printk(KERN_ERR "%s: Invalid id (%d)\n", __func__, id + 1);
> @@ -465,7 +465,7 @@ EXPORT_SYMBOL(omap_mcbsp_pollread);
> void omap_mcbsp_xmit_word(unsigned int id, u32 word)
> {
> struct omap_mcbsp *mcbsp;
> - u32 io_base;
> + void __iomem *io_base;
> omap_mcbsp_word_length word_length;
>
> if (!omap_mcbsp_check_valid_id(id)) {
> @@ -488,7 +488,7 @@ EXPORT_SYMBOL(omap_mcbsp_xmit_word);
> u32 omap_mcbsp_recv_word(unsigned int id)
> {
> struct omap_mcbsp *mcbsp;
> - u32 io_base;
> + void __iomem *io_base;
> u16 word_lsb, word_msb = 0;
> omap_mcbsp_word_length word_length;
>
> @@ -514,7 +514,7 @@ EXPORT_SYMBOL(omap_mcbsp_recv_word);
> int omap_mcbsp_spi_master_xmit_word_poll(unsigned int id, u32 word)
> {
> struct omap_mcbsp *mcbsp;
> - u32 io_base;
> + void __iomem *io_base;
> omap_mcbsp_word_length tx_word_length;
> omap_mcbsp_word_length rx_word_length;
> u16 spcr2, spcr1, attempts = 0, word_lsb, word_msb = 0;
> @@ -580,7 +580,8 @@ EXPORT_SYMBOL(omap_mcbsp_spi_master_xmit_word_poll);
> int omap_mcbsp_spi_master_recv_word_poll(unsigned int id, u32 *word)
> {
> struct omap_mcbsp *mcbsp;
> - u32 io_base, clock_word = 0;
> + u32 clock_word = 0;
> + void __iomem *io_base;
> omap_mcbsp_word_length tx_word_length;
> omap_mcbsp_word_length rx_word_length;
> u16 spcr2, spcr1, attempts = 0, word_lsb, word_msb = 0;
> @@ -701,6 +702,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
> omap_set_dma_dest_params(mcbsp->dma_tx_lch,
> src_port,
> OMAP_DMA_AMODE_CONSTANT,
> + /* FIXME: this is a virtual address */
> mcbsp->io_base + OMAP_MCBSP_REG_DXR1,
yes, that's true. This is expected to be virtual (mcbsp->io_base).
> 0, 0);
>
> @@ -764,6 +766,7 @@ int omap_mcbsp_recv_buffer(unsigned int id, dma_addr_t buffer,
> omap_set_dma_src_params(mcbsp->dma_rx_lch,
> src_port,
> OMAP_DMA_AMODE_CONSTANT,
> + /* FIXME: this is a virtual address */
> mcbsp->io_base + OMAP_MCBSP_REG_DRR1,
Here as well.
> 0, 0);
>
> diff --git a/drivers/char/hw_random/omap-rng.c b/drivers/char/hw_random/omap-rng.c
> index 154a21f..668bf7d 100644
> --- a/drivers/char/hw_random/omap-rng.c
> +++ b/drivers/char/hw_random/omap-rng.c
> @@ -122,7 +122,7 @@ static int __init omap_rng_probe(struct platform_device *pdev)
> return -EBUSY;
>
> dev_set_drvdata(&pdev->dev, mem);
> - rng_base = (u32 __force __iomem *)io_p2v(res->start);
> + rng_base = IO_ADDRESS(res->start); /* WBNI: why not ioremap? */
>
> ret = hwrng_register(&omap_rng_ops);
> if (ret) {
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 3c4e581..978c072 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -757,7 +757,7 @@ omap_i2c_probe(struct platform_device *pdev)
> dev->speed = *speed;
> dev->dev = &pdev->dev;
> dev->irq = irq->start;
> - dev->base = (void __iomem *) IO_ADDRESS(mem->start);
> + dev->base = IO_ADDRESS(mem->start); /* WBNI: why not ioremap? */
> platform_set_drvdata(pdev, dev);
>
> if ((r = omap_i2c_get_clocks(dev)) != 0)
> diff --git a/drivers/mmc/host/omap.c b/drivers/mmc/host/omap.c
> index c160288..802877c 100644
> --- a/drivers/mmc/host/omap.c
> +++ b/drivers/mmc/host/omap.c
> @@ -1455,7 +1455,7 @@ static int __init mmc_omap_probe(struct platform_device *pdev)
>
> host->irq = irq;
> host->phys_base = host->mem_res->start;
> - host->virt_base = (void __iomem *) IO_ADDRESS(host->phys_base);
> + host->virt_base = IO_ADDRESS(host->phys_base); /* WBNI: why not ioremap? */
>
> if (cpu_is_omap24xx()) {
> host->iclk = clk_get(&pdev->dev, "mmc_ick");
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 5a5f95f..2ba5cbb 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -2209,7 +2209,7 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
>
> #ifdef CONFIG_ARCH_OMAP15XX
> /* Workaround to enable 115200 baud on OMAP1510 internal ports */
> - if (cpu_is_omap1510() && is_omap_port((unsigned int)up->port.membase)) {
> + if (cpu_is_omap1510() && is_omap_port(up->port.mapbase)) {
> if (baud == 115200) {
> quot = 1;
> serial_out(up, UART_OMAP_OSC_12M_SEL, 1);
> @@ -2284,7 +2284,7 @@ static int serial8250_request_std_resource(struct uart_8250_port *up)
> int ret = 0;
>
> #ifdef CONFIG_ARCH_OMAP
> - if (is_omap_port((unsigned int)up->port.membase))
> + if (is_omap_port(up->port.mapbase))
> size = 0x16 << up->port.regshift;
> #endif
>
> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
> index 9d2186f..8178665 100644
> --- a/drivers/spi/omap2_mcspi.c
> +++ b/drivers/spi/omap2_mcspi.c
> @@ -119,12 +119,14 @@ struct omap2_mcspi {
> struct clk *fck;
> /* Virtual base address of the controller */
> void __iomem *base;
> + unsigned long phys;
> /* SPI1 has 4 channels, while SPI2 has 2 */
> struct omap2_mcspi_dma *dma_channels;
> };
>
> struct omap2_mcspi_cs {
> void __iomem *base;
> + unsigned long phys;
> int word_len;
> };
>
> @@ -233,7 +235,7 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
> c = count;
> word_len = cs->word_len;
>
> - base = (unsigned long) io_v2p(cs->base);
> + base = cs->phys;
> tx_reg = base + OMAP2_MCSPI_TX0;
> rx_reg = base + OMAP2_MCSPI_RX0;
> rx = xfer->rx_buf;
> @@ -633,6 +635,7 @@ static int omap2_mcspi_setup(struct spi_device *spi)
> if (!cs)
> return -ENOMEM;
> cs->base = mcspi->base + spi->chip_select * 0x14;
> + cs->phys = mcspi->phys + spi->chip_select * 0x14;
> spi->controller_state = cs;
> }
>
> @@ -1005,7 +1008,8 @@ static int __init omap2_mcspi_probe(struct platform_device *pdev)
> goto err1;
> }
>
> - mcspi->base = (void __iomem *) io_p2v(r->start);
> + mcspi->phys = r->start;
> + mcspi->base = IO_ADDRESS(r->start); /* WBNI: why not ioremap? */
>
> INIT_WORK(&mcspi->work, omap2_mcspi_work);
>
> diff --git a/drivers/spi/omap_uwire.c b/drivers/spi/omap_uwire.c
> index 5515eb9..3166635 100644
> --- a/drivers/spi/omap_uwire.c
> +++ b/drivers/spi/omap_uwire.c
> @@ -59,7 +59,7 @@
> * and irqs should show there too...
> */
> #define UWIRE_BASE_PHYS 0xFFFB3000
> -#define UWIRE_BASE ((void *__iomem)IO_ADDRESS(UWIRE_BASE_PHYS))
> +#define UWIRE_BASE IO_ADDRESS(UWIRE_BASE_PHYS)
>
> /* uWire Registers: */
> #define UWIRE_IO_SIZE 0x20
> diff --git a/drivers/usb/gadget/omap_udc.c b/drivers/usb/gadget/omap_udc.c
> index a2638ee..7032975 100644
> --- a/drivers/usb/gadget/omap_udc.c
> +++ b/drivers/usb/gadget/omap_udc.c
> @@ -786,6 +786,7 @@ static void dma_channel_claim(struct omap_ep *ep, unsigned channel)
> omap_set_dma_dest_params(ep->lch,
> OMAP_DMA_PORT_TIPB,
> OMAP_DMA_AMODE_CONSTANT,
> + /* FIXME: aren't these already physical addresses */
> (unsigned long) io_v2p(UDC_DATA_DMA),
> 0, 0);
> }
> @@ -803,6 +804,7 @@ static void dma_channel_claim(struct omap_ep *ep, unsigned channel)
> omap_set_dma_src_params(ep->lch,
> OMAP_DMA_PORT_TIPB,
> OMAP_DMA_AMODE_CONSTANT,
> + /* FIXME: aren't these already physical addresses */
> (unsigned long) io_v2p(UDC_DATA_DMA),
> 0, 0);
> /* EMIFF or SDRC */
> diff --git a/drivers/usb/host/ohci-omap.c b/drivers/usb/host/ohci-omap.c
> index 71fb9fd..c2fa621 100644
> --- a/drivers/usb/host/ohci-omap.c
> +++ b/drivers/usb/host/ohci-omap.c
> @@ -208,7 +208,7 @@ static int ohci_omap_init(struct usb_hcd *hcd)
> if (cpu_is_omap16xx())
> ocpi_enable();
>
> -#ifdef CONFIG_ARCH_OMAP_OTG
> +#ifdef CONFIG_USB_OTG /* CHECKME */
> if (need_transceiver) {
> ohci->transceiver = otg_get_transceiver();
> if (ohci->transceiver) {
> @@ -343,7 +343,7 @@ static int usb_hcd_omap_probe (const struct hc_driver *driver,
> goto err1;
> }
>
> - hcd->regs = (void __iomem *) (int) IO_ADDRESS(hcd->rsrc_start);
> + hcd->regs = IO_ADDRESS(hcd->rsrc_start); /* WBNI: why not ioremap? */
>
> ohci = hcd_to_ohci(hcd);
> ohci_hcd_init(ohci);
> diff --git a/drivers/video/omap/dispc.c b/drivers/video/omap/dispc.c
> index 00ad6b2..3e802b7 100644
> --- a/drivers/video/omap/dispc.c
> +++ b/drivers/video/omap/dispc.c
> @@ -156,7 +156,7 @@ struct resmap {
> };
>
> static struct {
> - u32 base;
> + void __iomem *base;
>
> struct omapfb_mem_desc mem_desc;
> struct resmap *res_map[DISPC_MEMTYPE_NUM];
> @@ -212,9 +212,9 @@ static void enable_rfbi_mode(int enable)
> dispc_write_reg(DISPC_CONTROL, l);
>
> /* Set bypass mode in RFBI module */
> - l = __raw_readl(io_p2v(RFBI_CONTROL));
> + l = __raw_readl(IO_ADDRESS(RFBI_CONTROL));
> l |= enable ? 0 : (1 << 1);
> - __raw_writel(l, io_p2v(RFBI_CONTROL));
> + __raw_writel(l, IO_ADDRESS(RFBI_CONTROL));
> }
>
> static void set_lcd_data_lines(int data_lines)
> @@ -1353,7 +1353,7 @@ static int omap_dispc_init(struct omapfb_device *fbdev, int ext_mode,
>
> memset(&dispc, 0, sizeof(dispc));
>
> - dispc.base = io_p2v(DISPC_BASE);
> + dispc.base = IO_ADDRESS(DISPC_BASE);
> dispc.fbdev = fbdev;
> dispc.ext_mode = ext_mode;
>
> @@ -1417,7 +1417,7 @@ static int omap_dispc_init(struct omapfb_device *fbdev, int ext_mode,
> }
>
> /* L3 firewall setting: enable access to OCM RAM */
> - __raw_writel(0x402000b0, io_p2v(0x680050a0));
> + __raw_writel(0x402000b0, IO_ADDRESS(0x680050a0));
>
> if ((r = alloc_palette_ram()) < 0)
> goto fail2;
> diff --git a/drivers/video/omap/rfbi.c b/drivers/video/omap/rfbi.c
> index 470920d..59f857e 100644
> --- a/drivers/video/omap/rfbi.c
> +++ b/drivers/video/omap/rfbi.c
> @@ -59,7 +59,7 @@
> #define DISPC_CONTROL 0x0040
>
> static struct {
> - u32 base;
> + void __iomem *base;
> void (*lcdc_callback)(void *data);
> void *lcdc_callback_data;
> unsigned long l4_khz;
> @@ -518,7 +518,7 @@ static int rfbi_init(struct omapfb_device *fbdev)
> int r;
>
> rfbi.fbdev = fbdev;
> - rfbi.base = io_p2v(RFBI_BASE);
> + rfbi.base = IO_ADDRESS(RFBI_BASE);
>
> if ((r = rfbi_get_clocks()) < 0)
> return r;
> diff --git a/drivers/video/omap/sossi.c b/drivers/video/omap/sossi.c
> index b4f67fc..9839d35 100644
> --- a/drivers/video/omap/sossi.c
> +++ b/drivers/video/omap/sossi.c
> @@ -574,7 +574,7 @@ static int sossi_init(struct omapfb_device *fbdev)
> struct clk *dpll1out_ck;
> int r;
>
> - sossi.base = (void __iomem *)IO_ADDRESS(OMAP_SOSSI_BASE);
> + sossi.base = IO_ADDRESS(OMAP_SOSSI_BASE);
> sossi.fbdev = fbdev;
> spin_lock_init(&sossi.lock);
>
>
>
>
> --
> Russell King
> Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
> maintainer of:
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Eduardo Bezerra Valentin
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 7:55 ` Russell King - ARM Linux
@ 2008-09-03 16:40 ` Tony Lindgren
2008-09-03 19:34 ` Russell King - ARM Linux
0 siblings, 1 reply; 58+ messages in thread
From: Tony Lindgren @ 2008-09-03 16:40 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: David Brownell, linux-omap, Eduardo Valentin
* Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 00:56]:
> On Tue, Sep 02, 2008 at 03:15:10PM -0700, Tony Lindgren wrote:
> > Back online now. These changes look ggood to me in general, except
> > I suggest that we use the following standard:
> >
> > - Keep OMAP1_IO_ADDRESS() and OMAP2_IO_ADDRESS() as I have some
> > experimental patches to compile in both omap1 and omap2 into the
> > same binary. Booting the kernel currently still requires some
> > Makefile.boot patching, but at least compiling everything in makes
> > things easier to maintain in the long run.
>
> Already decided to do that.
OK
> > - Use io_p2v() for initializing dynamic stuff as it can be a function
> > for non-optimized multiboot binaries.
>
> It can't become a function - it's used in structure initialization so
> must be constant.
Outside the drivers we can use XXX_IO_ADDRESS() or set it during resource
init with io_p2v().
> There's a few places where it's used where resources are passed into
> drivers - in which case if the device is at a different physical address
> it's the resources which should be changed, not the translation macro.
Yeah. Luckily it looks like there are only few drivers that would need
patching for the resources.
Other than that, do you see other reasons why io_p2v() could not be a
function?
> Anyway, I've put a modified version in my git tree.
OK, will also apply to l-o tree after shortly.
> I still want to hear on the other build fix in the patch, and there's
> also a missing function for mmc stuff which I've not looked into yet.
The mpuio build fix? That is a correct fix.
Tony
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-02 22:15 ` Tony Lindgren
2008-09-03 7:55 ` Russell King - ARM Linux
2008-09-03 15:07 ` Eduardo Valentin
@ 2008-09-03 18:01 ` Tony Lindgren
2008-09-04 0:16 ` David Brownell
2 siblings, 1 reply; 58+ messages in thread
From: Tony Lindgren @ 2008-09-03 18:01 UTC (permalink / raw)
To: David Brownell; +Cc: Russell King, linux-omap, Eduardo Valentin
* Tony Lindgren <tony@atomide.com> [080902 15:16]:
> Hi,
>
> * David Brownell <david-b@pacbell.net> [080831 14:47]:
> > On Wednesday 27 August 2008, Russell King wrote:
<snip>
> >
> > So I can try my patch for the omap_udc thing ... even though for
> > OSK the mainline kernel ** DOES NOT BOOT ** and dies even before
> > the kernel "hello, I'm RC5!" banner prints. Someone with a JTAG
> > to throw at this might be able to fix that regression quickly.
>
> I'll take a look at the OSK boot problem unless somebody else is
> already working on it.
My OSK is booting just fine here with current mainline and the
omap_osk_5912_defconfig. This is the current head at
2.6.27-rc5-00100-gec0c15a. Have you checked if yours boots with
the mainline omap_osk_5912_defconfig?
Tony
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 15:33 ` Eduardo Valentin
@ 2008-09-03 18:48 ` Russell King
2008-09-03 19:33 ` Eduardo Valentin
2008-09-04 9:46 ` Russell King - ARM Linux
1 sibling, 1 reply; 58+ messages in thread
From: Russell King @ 2008-09-03 18:48 UTC (permalink / raw)
To: Eduardo Valentin; +Cc: linux-omap
On Wed, Sep 03, 2008 at 11:33:51AM -0400, Eduardo Valentin wrote:
> > @@ -159,7 +159,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
> > #ifdef CONFIG_ARCH_OMAP15XX
> > static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> > {
> > - .virt_base = OMAP1510_MCBSP1_BASE,
> > + .virt_base = OMAP1510_MCBSP1_BASE, /* FIXME: virtual or physical */
> AFAIK, OMAP1510_MCBSP1_BASE is physical. So, I'd say:
> + .virt_base = IO_ADDRESS(OMAP1510_MCBSP1_BASE),
>
> Because, plat-omap/mcbsp.c expect .virt_base to be a virtual address.
Ok, I'll fix these which you've confirmed in my version for mainline.
> > @@ -701,6 +702,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
> > omap_set_dma_dest_params(mcbsp->dma_tx_lch,
> > src_port,
> > OMAP_DMA_AMODE_CONSTANT,
> > + /* FIXME: this is a virtual address */
> > mcbsp->io_base + OMAP_MCBSP_REG_DXR1,
>
> yes, that's true. This is expected to be virtual (mcbsp->io_base).
Don't you mean that it is expected to be physical?
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 18:48 ` Russell King
@ 2008-09-03 19:33 ` Eduardo Valentin
2008-09-03 19:48 ` Russell King - ARM Linux
0 siblings, 1 reply; 58+ messages in thread
From: Eduardo Valentin @ 2008-09-03 19:33 UTC (permalink / raw)
To: Russell King; +Cc: linux-omap
Hi,
On Wed, Sep 3, 2008 at 2:48 PM, Russell King <rmk@arm.linux.org.uk> wrote:
> On Wed, Sep 03, 2008 at 11:33:51AM -0400, Eduardo Valentin wrote:
>> > @@ -159,7 +159,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
>> > #ifdef CONFIG_ARCH_OMAP15XX
>> > static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
>> > {
>> > - .virt_base = OMAP1510_MCBSP1_BASE,
>> > + .virt_base = OMAP1510_MCBSP1_BASE, /* FIXME: virtual or physical */
>> AFAIK, OMAP1510_MCBSP1_BASE is physical. So, I'd say:
>> + .virt_base = IO_ADDRESS(OMAP1510_MCBSP1_BASE),
>>
>> Because, plat-omap/mcbsp.c expect .virt_base to be a virtual address.
>
> Ok, I'll fix these which you've confirmed in my version for mainline.
>
>> > @@ -701,6 +702,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
>> > omap_set_dma_dest_params(mcbsp->dma_tx_lch,
>> > src_port,
>> > OMAP_DMA_AMODE_CONSTANT,
>> > + /* FIXME: this is a virtual address */
>> > mcbsp->io_base + OMAP_MCBSP_REG_DXR1,
>>
>> yes, that's true. This is expected to be virtual (mcbsp->io_base).
>
> Don't you mean that it is expected to be physical?
>
Well, correct me if I'm wrong, but I meant that the result of
mcbsp->io_base + OMAP_MCBSP_REG_DXR1
is supposed to be virtual.
Because the mcbsp->io_base reference comes directly from virt_base field of platform_data.
At least it is what says line 895 of plat-omap/mcbsp.c.
And that's why we need to use IO_ADDRESS while filling up platform_data,
as mentioned above.
> --
> Russell King
> Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
> maintainer of:
>
Cheers,
--
Eduardo Bezerra Valentin
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 16:40 ` Tony Lindgren
@ 2008-09-03 19:34 ` Russell King - ARM Linux
2008-09-03 19:48 ` Tony Lindgren
2008-09-03 19:58 ` Woodruff, Richard
0 siblings, 2 replies; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-03 19:34 UTC (permalink / raw)
To: Tony Lindgren; +Cc: David Brownell, linux-omap, Eduardo Valentin
On Wed, Sep 03, 2008 at 09:40:08AM -0700, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 00:56]:
> > > - Use io_p2v() for initializing dynamic stuff as it can be a function
> > > for non-optimized multiboot binaries.
> >
> > It can't become a function - it's used in structure initialization so
> > must be constant.
>
> Outside the drivers we can use XXX_IO_ADDRESS() or set it during resource
> init with io_p2v().
The question is why do we need it? If the correct physical address
is passed, then things should work out just fine anyway, especially
if drivers start to use ioremap rather than relying on all these fixed
translations.
> > I still want to hear on the other build fix in the patch, and there's
> > also a missing function for mmc stuff which I've not looked into yet.
>
> The mpuio build fix? That is a correct fix.
No, see drivers/usb/host/ohci-omap.c:
-#ifdef CONFIG_ARCH_OMAP_OTG
+#ifdef CONFIG_USB_OTG /* CHECKME */
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 19:33 ` Eduardo Valentin
@ 2008-09-03 19:48 ` Russell King - ARM Linux
2008-09-03 20:04 ` Eduardo Valentin
2008-09-03 20:37 ` Tony Lindgren
0 siblings, 2 replies; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-03 19:48 UTC (permalink / raw)
To: Eduardo Valentin; +Cc: linux-omap
On Wed, Sep 03, 2008 at 03:33:55PM -0400, Eduardo Valentin wrote:
> Hi,
>
> On Wed, Sep 3, 2008 at 2:48 PM, Russell King <rmk@arm.linux.org.uk> wrote:
> > On Wed, Sep 03, 2008 at 11:33:51AM -0400, Eduardo Valentin wrote:
> >> > @@ -159,7 +159,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
> >> > #ifdef CONFIG_ARCH_OMAP15XX
> >> > static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> >> > {
> >> > - .virt_base = OMAP1510_MCBSP1_BASE,
> >> > + .virt_base = OMAP1510_MCBSP1_BASE, /* FIXME: virtual or physical */
> >> AFAIK, OMAP1510_MCBSP1_BASE is physical. So, I'd say:
> >> + .virt_base = IO_ADDRESS(OMAP1510_MCBSP1_BASE),
> >>
> >> Because, plat-omap/mcbsp.c expect .virt_base to be a virtual address.
> >
> > Ok, I'll fix these which you've confirmed in my version for mainline.
> >
> >> > @@ -701,6 +702,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
> >> > omap_set_dma_dest_params(mcbsp->dma_tx_lch,
> >> > src_port,
> >> > OMAP_DMA_AMODE_CONSTANT,
> >> > + /* FIXME: this is a virtual address */
> >> > mcbsp->io_base + OMAP_MCBSP_REG_DXR1,
> >>
> >> yes, that's true. This is expected to be virtual (mcbsp->io_base).
> >
> > Don't you mean that it is expected to be physical?
> >
>
> Well, correct me if I'm wrong, but I meant that the result of
> mcbsp->io_base + OMAP_MCBSP_REG_DXR1
>
> is supposed to be virtual.
Yes, that will be virtual. But what does it mean to call:
omap_set_dma_dest_params()
specifying a virtual address? Can the DMA controller cope with DMAing
to virtual addresses? My hunch is that the DMA controller can't cope
with that, so giving it a virtual address is a bug.
Let me change the question: does omap_set_dma_dest_params()'s 4th
argument take a virtual or a physical address? If the former, it's
prototype is wrong, and its 4th argument needs to be typed as
'void __iomem *' rather than 'unsigned long'. If the latter, the code
above is wrong.
Do you see what I'm getting at now?
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 19:34 ` Russell King - ARM Linux
@ 2008-09-03 19:48 ` Tony Lindgren
2008-09-03 21:09 ` David Brownell
2008-09-03 19:58 ` Woodruff, Richard
1 sibling, 1 reply; 58+ messages in thread
From: Tony Lindgren @ 2008-09-03 19:48 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: David Brownell, linux-omap, Eduardo Valentin
* Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 12:34]:
> On Wed, Sep 03, 2008 at 09:40:08AM -0700, Tony Lindgren wrote:
> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 00:56]:
> > > > - Use io_p2v() for initializing dynamic stuff as it can be a function
> > > > for non-optimized multiboot binaries.
> > >
> > > It can't become a function - it's used in structure initialization so
> > > must be constant.
> >
> > Outside the drivers we can use XXX_IO_ADDRESS() or set it during resource
> > init with io_p2v().
>
> The question is why do we need it? If the correct physical address
> is passed, then things should work out just fine anyway, especially
> if drivers start to use ioremap rather than relying on all these fixed
> translations.
Hmm, that means fixing up resource init a bit in few places to avoid
sprinkling tests for cpu_class_is_omap1() to set the physical address.
Anyways, your approach sounds cleaner in the long run.
Eventually the whole io_p2v() can be removed as it's redundant and all
the arch stuff could use XXX_IO_ADDRESS().
> > > I still want to hear on the other build fix in the patch, and there's
> > > also a missing function for mmc stuff which I've not looked into yet.
> >
> > The mpuio build fix? That is a correct fix.
>
> No, see drivers/usb/host/ohci-omap.c:
>
> -#ifdef CONFIG_ARCH_OMAP_OTG
> +#ifdef CONFIG_USB_OTG /* CHECKME */
>
That's a Dave question.
Tony
^ permalink raw reply [flat|nested] 58+ messages in thread
* RE: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 19:34 ` Russell King - ARM Linux
2008-09-03 19:48 ` Tony Lindgren
@ 2008-09-03 19:58 ` Woodruff, Richard
2008-09-03 20:30 ` Russell King - ARM Linux
` (2 more replies)
1 sibling, 3 replies; 58+ messages in thread
From: Woodruff, Richard @ 2008-09-03 19:58 UTC (permalink / raw)
To: Russell King - ARM Linux, Tony Lindgren
Cc: David Brownell, linux-omap@vger.kernel.org, Eduardo Valentin
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Russell King - ARM Linux
> Sent: Wednesday, September 03, 2008 2:34 PM
<snip>
> The question is why do we need it? If the correct physical address
> is passed, then things should work out just fine anyway, especially
> if drivers start to use ioremap rather than relying on all these fixed
> translations.
Fixed translations do have some benefits. You can ensure that you are using section or super section descriptors to cover large areas. This does result in better TLB usage. Along with freeing up TLB entries you also generally avoid TLB misses on IO calls which touch a variety of internal spaces as part of the IRQ sequence.
With in a family of chips like 2420/22/23 or 3410/20/30/40 the internal space is mapped the same.
Frankly I've never been convinced that a multi OMAP1/2/3 image makes much sense apart forcing better code structure and being kind of cool. Each chip has very different performance targets and is really better built with an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing multi-boots with in the same architecture family seems really good but across seems less so.
Regards,
Richard W.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 19:48 ` Russell King - ARM Linux
@ 2008-09-03 20:04 ` Eduardo Valentin
2008-09-03 20:45 ` Russell King - ARM Linux
2008-09-03 20:37 ` Tony Lindgren
1 sibling, 1 reply; 58+ messages in thread
From: Eduardo Valentin @ 2008-09-03 20:04 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: linux-omap
Hi Russell,
On Wed, Sep 3, 2008 at 3:48 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Sep 03, 2008 at 03:33:55PM -0400, Eduardo Valentin wrote:
>> Hi,
>>
>> On Wed, Sep 3, 2008 at 2:48 PM, Russell King <rmk@arm.linux.org.uk> wrote:
>> > On Wed, Sep 03, 2008 at 11:33:51AM -0400, Eduardo Valentin wrote:
>> >> > @@ -159,7 +159,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
>> >> > #ifdef CONFIG_ARCH_OMAP15XX
>> >> > static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
>> >> > {
>> >> > - .virt_base = OMAP1510_MCBSP1_BASE,
>> >> > + .virt_base = OMAP1510_MCBSP1_BASE, /* FIXME: virtual or physical */
>> >> AFAIK, OMAP1510_MCBSP1_BASE is physical. So, I'd say:
>> >> + .virt_base = IO_ADDRESS(OMAP1510_MCBSP1_BASE),
>> >>
>> >> Because, plat-omap/mcbsp.c expect .virt_base to be a virtual address.
>> >
>> > Ok, I'll fix these which you've confirmed in my version for mainline.
>> >
>> >> > @@ -701,6 +702,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
>> >> > omap_set_dma_dest_params(mcbsp->dma_tx_lch,
>> >> > src_port,
>> >> > OMAP_DMA_AMODE_CONSTANT,
>> >> > + /* FIXME: this is a virtual address */
>> >> > mcbsp->io_base + OMAP_MCBSP_REG_DXR1,
>> >>
>> >> yes, that's true. This is expected to be virtual (mcbsp->io_base).
>> >
>> > Don't you mean that it is expected to be physical?
>> >
>>
>> Well, correct me if I'm wrong, but I meant that the result of
>> mcbsp->io_base + OMAP_MCBSP_REG_DXR1
>>
>> is supposed to be virtual.
>
> Yes, that will be virtual. But what does it mean to call:
>
> omap_set_dma_dest_params()
>
> specifying a virtual address? Can the DMA controller cope with DMAing
> to virtual addresses? My hunch is that the DMA controller can't cope
> with that, so giving it a virtual address is a bug.
>
> Let me change the question: does omap_set_dma_dest_params()'s 4th
> argument take a virtual or a physical address? If the former, it's
> prototype is wrong, and its 4th argument needs to be typed as
> 'void __iomem *' rather than 'unsigned long'. If the latter, the code
> above is wrong.
>
> Do you see what I'm getting at now?
Ok. I got your point. I took a look at it and it expects a virtual address
on its 4th argument. I uses it with dma_write() macro, which uses it
with __raw_write() (arch/arm/plat-omap/dma.c).
And yes, that's true, in this case omap_set_dma_dest_params needs
a refactor to accomplish what you are achiving with this patch.
omap_set_dma_dest_params uses unsigned long on its 4th argument
and also uses u32 in some io addressing.
>
--
Eduardo Bezerra Valentin
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 19:58 ` Woodruff, Richard
@ 2008-09-03 20:30 ` Russell King - ARM Linux
2008-09-03 21:19 ` Woodruff, Richard
2008-09-03 20:32 ` Tony Lindgren
2008-09-03 21:18 ` David Brownell
2 siblings, 1 reply; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-03 20:30 UTC (permalink / raw)
To: Woodruff, Richard
Cc: Tony Lindgren, David Brownell, linux-omap@vger.kernel.org,
Eduardo Valentin
On Wed, Sep 03, 2008 at 02:58:10PM -0500, Woodruff, Richard wrote:
> Fixed translations do have some benefits. You can ensure that you are
> using section or super section descriptors to cover large areas. This
> does result in better TLB usage. Along with freeing up TLB entries you
> also generally avoid TLB misses on IO calls which touch a variety of
> internal spaces as part of the IRQ sequence.
These are valid points, and you can arrange for ioremap to give you the
same effect. That way, you end up using _standard_ Linux interfaces
which everyone understands and still having your cake.
> Each chip has very different performance targets and is really better
> built with an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing
> multi-boots with in the same architecture family seems really good
> but across seems less so.
This thread isn't about multi-boots across differnet architecture;
please take that discussion elsewhere - it's off topic.
This thread is about fixing up the crappy state that the OMAP tree is
in with respect to typechecking of virtual vs physical address spaces
without regressing the current state of the code.
And "regressing the current state of the code" includes not wilfully
undoing the work which Tony has put in place towards his goal, *even*
if you may not agree with it.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 19:58 ` Woodruff, Richard
2008-09-03 20:30 ` Russell King - ARM Linux
@ 2008-09-03 20:32 ` Tony Lindgren
2008-09-03 21:32 ` Woodruff, Richard
2008-09-03 21:18 ` David Brownell
2 siblings, 1 reply; 58+ messages in thread
From: Tony Lindgren @ 2008-09-03 20:32 UTC (permalink / raw)
To: Woodruff, Richard
Cc: Russell King - ARM Linux, David Brownell,
linux-omap@vger.kernel.org, Eduardo Valentin
* Woodruff, Richard <r-woodruff2@ti.com> [080903 12:58]:
>
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of Russell King - ARM Linux
> > Sent: Wednesday, September 03, 2008 2:34 PM
> <snip>
> > The question is why do we need it? If the correct physical address
> > is passed, then things should work out just fine anyway, especially
> > if drivers start to use ioremap rather than relying on all these fixed
> > translations.
>
> Fixed translations do have some benefits. You can ensure that you are using section or super section descriptors to cover large areas. This does result in better TLB usage. Along with freeing up TLB entries you also generally avoid TLB misses on IO calls which touch a variety of internal spaces as part of the IRQ sequence.
>
> With in a family of chips like 2420/22/23 or 3410/20/30/40 the internal space is mapped the same.
I guess there's no advantage of using ioremap if the area is
already mapped. For drivers that are shared across multiple
archs or buses it makes sense.
> Frankly I've never been convinced that a multi OMAP1/2/3 image makes much sense apart forcing better code structure and being kind of cool. Each chip has very different performance targets and is really better built with an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing multi-boots with in the same architecture family seems really good but across seems less so.
It definitely makes sense from distro and maintenance point of view. If
we did not work towards multi-omap we would already have the same code
duplicated many times over.
Tony
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 19:48 ` Russell King - ARM Linux
2008-09-03 20:04 ` Eduardo Valentin
@ 2008-09-03 20:37 ` Tony Lindgren
2008-09-03 21:04 ` Russell King - ARM Linux
1 sibling, 1 reply; 58+ messages in thread
From: Tony Lindgren @ 2008-09-03 20:37 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Eduardo Valentin, linux-omap
* Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 12:49]:
> On Wed, Sep 03, 2008 at 03:33:55PM -0400, Eduardo Valentin wrote:
> > Hi,
> >
> > On Wed, Sep 3, 2008 at 2:48 PM, Russell King <rmk@arm.linux.org.uk> wrote:
> > > On Wed, Sep 03, 2008 at 11:33:51AM -0400, Eduardo Valentin wrote:
> > >> > @@ -159,7 +159,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
> > >> > #ifdef CONFIG_ARCH_OMAP15XX
> > >> > static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> > >> > {
> > >> > - .virt_base = OMAP1510_MCBSP1_BASE,
> > >> > + .virt_base = OMAP1510_MCBSP1_BASE, /* FIXME: virtual or physical */
> > >> AFAIK, OMAP1510_MCBSP1_BASE is physical. So, I'd say:
> > >> + .virt_base = IO_ADDRESS(OMAP1510_MCBSP1_BASE),
> > >>
> > >> Because, plat-omap/mcbsp.c expect .virt_base to be a virtual address.
> > >
> > > Ok, I'll fix these which you've confirmed in my version for mainline.
> > >
> > >> > @@ -701,6 +702,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
> > >> > omap_set_dma_dest_params(mcbsp->dma_tx_lch,
> > >> > src_port,
> > >> > OMAP_DMA_AMODE_CONSTANT,
> > >> > + /* FIXME: this is a virtual address */
> > >> > mcbsp->io_base + OMAP_MCBSP_REG_DXR1,
> > >>
> > >> yes, that's true. This is expected to be virtual (mcbsp->io_base).
> > >
> > > Don't you mean that it is expected to be physical?
> > >
> >
> > Well, correct me if I'm wrong, but I meant that the result of
> > mcbsp->io_base + OMAP_MCBSP_REG_DXR1
> >
> > is supposed to be virtual.
>
> Yes, that will be virtual. But what does it mean to call:
>
> omap_set_dma_dest_params()
>
> specifying a virtual address? Can the DMA controller cope with DMAing
> to virtual addresses? My hunch is that the DMA controller can't cope
> with that, so giving it a virtual address is a bug.
>
> Let me change the question: does omap_set_dma_dest_params()'s 4th
> argument take a virtual or a physical address? If the former, it's
> prototype is wrong, and its 4th argument needs to be typed as
> 'void __iomem *' rather than 'unsigned long'. If the latter, the code
> above is wrong.
The dma src and dest functions take physical addresses so the prototype
should be void __iomem *.
Tony
> Do you see what I'm getting at now?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 20:04 ` Eduardo Valentin
@ 2008-09-03 20:45 ` Russell King - ARM Linux
2008-09-03 20:50 ` Tony Lindgren
2008-09-03 21:00 ` Koen Kooi
0 siblings, 2 replies; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-03 20:45 UTC (permalink / raw)
To: Eduardo Valentin; +Cc: linux-omap
On Wed, Sep 03, 2008 at 04:04:11PM -0400, Eduardo Valentin wrote:
> Hi Russell,
>
> On Wed, Sep 3, 2008 at 3:48 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > Yes, that will be virtual. But what does it mean to call:
> >
> > omap_set_dma_dest_params()
> >
> > specifying a virtual address? Can the DMA controller cope with DMAing
> > to virtual addresses? My hunch is that the DMA controller can't cope
> > with that, so giving it a virtual address is a bug.
> >
> > Let me change the question: does omap_set_dma_dest_params()'s 4th
> > argument take a virtual or a physical address? If the former, it's
> > prototype is wrong, and its 4th argument needs to be typed as
> > 'void __iomem *' rather than 'unsigned long'. If the latter, the code
> > above is wrong.
> >
> > Do you see what I'm getting at now?
>
> Ok. I got your point. I took a look at it and it expects a virtual address
> on its 4th argument.
I'm sorry, but I don't think you have.
> I uses it with dma_write() macro, which uses it with __raw_write()
> (arch/arm/plat-omap/dma.c).
That's true, but probably not in the way which would require it to be
a virtual address that you're thinking.
void omap_set_dma_dest_params(int lch, int dest_port, int dest_amode,
unsigned long dest_start,
int dst_ei, int dst_fi)
{
if (cpu_class_is_omap1()) {
dma_write(dest_start >> 16, CDSA_U(lch));
dma_write(dest_start, CDSA_L(lch));
}
if (cpu_class_is_omap2())
dma_write(dest_start, CDSA(lch));
}
'dest_start' is a value to be written to the hardware registers in
the DMA controller. So, the question now is: do the CDSA registers
expect a virtual or a physical address to be written to them?
I would look up this CDSA register in the OMAP manuals, if they were
accessible... Are they not publically available? (Google isn't
helping, neither is searching on ti.com...)
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 20:45 ` Russell King - ARM Linux
@ 2008-09-03 20:50 ` Tony Lindgren
2008-09-03 20:56 ` Tony Lindgren
2008-09-03 21:00 ` Koen Kooi
1 sibling, 1 reply; 58+ messages in thread
From: Tony Lindgren @ 2008-09-03 20:50 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Eduardo Valentin, linux-omap
* Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 13:46]:
> On Wed, Sep 03, 2008 at 04:04:11PM -0400, Eduardo Valentin wrote:
> > Hi Russell,
> >
> > On Wed, Sep 3, 2008 at 3:48 PM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > Yes, that will be virtual. But what does it mean to call:
> > >
> > > omap_set_dma_dest_params()
> > >
> > > specifying a virtual address? Can the DMA controller cope with DMAing
> > > to virtual addresses? My hunch is that the DMA controller can't cope
> > > with that, so giving it a virtual address is a bug.
> > >
> > > Let me change the question: does omap_set_dma_dest_params()'s 4th
> > > argument take a virtual or a physical address? If the former, it's
> > > prototype is wrong, and its 4th argument needs to be typed as
> > > 'void __iomem *' rather than 'unsigned long'. If the latter, the code
> > > above is wrong.
> > >
> > > Do you see what I'm getting at now?
> >
> > Ok. I got your point. I took a look at it and it expects a virtual address
> > on its 4th argument.
>
> I'm sorry, but I don't think you have.
>
> > I uses it with dma_write() macro, which uses it with __raw_write()
> > (arch/arm/plat-omap/dma.c).
>
> That's true, but probably not in the way which would require it to be
> a virtual address that you're thinking.
>
> void omap_set_dma_dest_params(int lch, int dest_port, int dest_amode,
> unsigned long dest_start,
> int dst_ei, int dst_fi)
> {
> if (cpu_class_is_omap1()) {
> dma_write(dest_start >> 16, CDSA_U(lch));
> dma_write(dest_start, CDSA_L(lch));
> }
>
> if (cpu_class_is_omap2())
> dma_write(dest_start, CDSA(lch));
> }
>
> 'dest_start' is a value to be written to the hardware registers in
> the DMA controller. So, the question now is: do the CDSA registers
> expect a virtual or a physical address to be written to them?
C[DS][DS]A register need physical addresses for the DMA. The DMA
controller does not know anything about virtual addresses.
> I would look up this CDSA register in the OMAP manuals, if they were
> accessible... Are they not publically available? (Google isn't
> helping, neither is searching on ti.com...)
There is a 3450 public TRM somewhere AFAIK, anybody got a link for that?
At least 5912 TRM is available somewhere.
Tony
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 20:50 ` Tony Lindgren
@ 2008-09-03 20:56 ` Tony Lindgren
2008-09-03 21:07 ` Russell King - ARM Linux
0 siblings, 1 reply; 58+ messages in thread
From: Tony Lindgren @ 2008-09-03 20:56 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Eduardo Valentin, linux-omap
* Tony Lindgren <tony@atomide.com> [080903 13:50]:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 13:46]:
> > On Wed, Sep 03, 2008 at 04:04:11PM -0400, Eduardo Valentin wrote:
> > > Hi Russell,
> > >
> > > On Wed, Sep 3, 2008 at 3:48 PM, Russell King - ARM Linux
> > > <linux@arm.linux.org.uk> wrote:
> > > > Yes, that will be virtual. But what does it mean to call:
> > > >
> > > > omap_set_dma_dest_params()
> > > >
> > > > specifying a virtual address? Can the DMA controller cope with DMAing
> > > > to virtual addresses? My hunch is that the DMA controller can't cope
> > > > with that, so giving it a virtual address is a bug.
> > > >
> > > > Let me change the question: does omap_set_dma_dest_params()'s 4th
> > > > argument take a virtual or a physical address? If the former, it's
> > > > prototype is wrong, and its 4th argument needs to be typed as
> > > > 'void __iomem *' rather than 'unsigned long'. If the latter, the code
> > > > above is wrong.
> > > >
> > > > Do you see what I'm getting at now?
> > >
> > > Ok. I got your point. I took a look at it and it expects a virtual address
> > > on its 4th argument.
> >
> > I'm sorry, but I don't think you have.
> >
> > > I uses it with dma_write() macro, which uses it with __raw_write()
> > > (arch/arm/plat-omap/dma.c).
> >
> > That's true, but probably not in the way which would require it to be
> > a virtual address that you're thinking.
> >
> > void omap_set_dma_dest_params(int lch, int dest_port, int dest_amode,
> > unsigned long dest_start,
> > int dst_ei, int dst_fi)
> > {
> > if (cpu_class_is_omap1()) {
> > dma_write(dest_start >> 16, CDSA_U(lch));
> > dma_write(dest_start, CDSA_L(lch));
> > }
> >
> > if (cpu_class_is_omap2())
> > dma_write(dest_start, CDSA(lch));
> > }
> >
> > 'dest_start' is a value to be written to the hardware registers in
> > the DMA controller. So, the question now is: do the CDSA registers
> > expect a virtual or a physical address to be written to them?
>
> C[DS][DS]A register need physical addresses for the DMA. The DMA
> controller does not know anything about virtual addresses.
>
> > I would look up this CDSA register in the OMAP manuals, if they were
> > accessible... Are they not publically available? (Google isn't
> > helping, neither is searching on ti.com...)
>
> There is a 3450 public TRM somewhere AFAIK, anybody got a link for that?
> At least 5912 TRM is available somewhere.
Sorry I meant 3550 public TRM. Here's the link to the 5912 TRM,
which covers 16xx.
http://focus.ti.com/dsp/docs/dspsupporttechdocsc.tsp?abstractName=spru742§ionId=3&tabId=409
>
> Tony
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 20:45 ` Russell King - ARM Linux
2008-09-03 20:50 ` Tony Lindgren
@ 2008-09-03 21:00 ` Koen Kooi
1 sibling, 0 replies; 58+ messages in thread
From: Koen Kooi @ 2008-09-03 21:00 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: linux-omap@vger.kernel.org Mailing List
[-- Attachment #1: Type: text/plain, Size: 335 bytes --]
Op 3 sep 2008, om 22:45 heeft Russell King - ARM Linux het volgende
geschreven:
> I would look up this CDSA register in the OMAP manuals, if they were
> accessible... Are they not publically available? (Google isn't
> helping, neither is searching on ti.com...)
Try http://www.ti.com/litv/pdf/spruf98a for omap3
regards,
Koen
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 20:37 ` Tony Lindgren
@ 2008-09-03 21:04 ` Russell King - ARM Linux
2008-09-03 21:26 ` Eduardo Valentin
2008-09-03 21:35 ` David Brownell
0 siblings, 2 replies; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-03 21:04 UTC (permalink / raw)
To: Tony Lindgren; +Cc: Eduardo Valentin, linux-omap
On Wed, Sep 03, 2008 at 01:37:21PM -0700, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 12:49]:
> > Yes, that will be virtual. But what does it mean to call:
> >
> > omap_set_dma_dest_params()
> >
> > specifying a virtual address? Can the DMA controller cope with DMAing
> > to virtual addresses? My hunch is that the DMA controller can't cope
> > with that, so giving it a virtual address is a bug.
> >
> > Let me change the question: does omap_set_dma_dest_params()'s 4th
> > argument take a virtual or a physical address? If the former, it's
> > prototype is wrong, and its 4th argument needs to be typed as
> > 'void __iomem *' rather than 'unsigned long'. If the latter, the code
> > above is wrong.
>
> The dma src and dest functions take physical addresses so the prototype
> should be void __iomem *.
Grr. No. Let me repeat the rule:
- virtual addresses are pointer like.
- physical addresses are integer like.
So, if it's a physical address, it should be stored in an integer type
large enough to contain it, and that means something like u32, or
unsigned long.
If it's a virtual MMIO address, then it should be something like
'void __iomem *', or if you want to play roulette with compiler padding,
'struct foo __iomem *'.
Okay, so lets accept that the 4th argument to omap_set_dma_dest_params()
is a physical address. It should be typed as 'unsigned long' (it is)
and it then means that:
diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index d084405..d9b1a42 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -652,6 +652,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
omap_set_dma_dest_params(mcbsp[id].dma_tx_lch,
src_port,
OMAP_DMA_AMODE_CONSTANT,
+ /* FIXME: this is a virtual address */
mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1,
0, 0);
@@ -713,6 +714,7 @@ int omap_mcbsp_recv_buffer(unsigned int id, dma_addr_t buffer,
omap_set_dma_src_params(mcbsp[id].dma_rx_lch,
src_port,
OMAP_DMA_AMODE_CONSTANT,
+ /* FIXME: this is a virtual address */
mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1,
0, 0);
are broken because they're passing a virtual address into a function
requiring a physical address.
Now, to fix this in the right way isn't going to be easy, because
arch/arm/plat-omap/mcbsp.c doesn't know what the physical address of
the mcbsp actually is - it's only passed the virtual address via
platform data (eww, yuck yuck yuck)...
If this was a properly reviewed platform driver, and on *any* *other*
ARM platform, it would take the resources containing the physical
addresses and ioremap them... and this would be a trivial bug fix.
I'll cook a patch up.
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 20:56 ` Tony Lindgren
@ 2008-09-03 21:07 ` Russell King - ARM Linux
2008-09-03 21:13 ` Tony Lindgren
0 siblings, 1 reply; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-03 21:07 UTC (permalink / raw)
To: Tony Lindgren; +Cc: Eduardo Valentin, linux-omap
On Wed, Sep 03, 2008 at 01:56:18PM -0700, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [080903 13:50]:
> > There is a 3450 public TRM somewhere AFAIK, anybody got a link for that?
> > At least 5912 TRM is available somewhere.
>
> Sorry I meant 3550 public TRM. Here's the link to the 5912 TRM,
> which covers 16xx.
>
> http://focus.ti.com/dsp/docs/dspsupporttechdocsc.tsp?abstractName=spru742§ionId=3&tabId=409
Hmm, claims to only be 106K, and xpdf 3 claims the file is unreadable.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 19:48 ` Tony Lindgren
@ 2008-09-03 21:09 ` David Brownell
2008-09-03 23:02 ` Russell King - ARM Linux
0 siblings, 1 reply; 58+ messages in thread
From: David Brownell @ 2008-09-03 21:09 UTC (permalink / raw)
To: Tony Lindgren; +Cc: Russell King - ARM Linux, linux-omap, Eduardo Valentin
On Wednesday 03 September 2008, Tony Lindgren wrote:
> > > > I still want to hear on the other build fix in the patch, and there's
> > > > also a missing function for mmc stuff which I've not looked into yet.
> > >
> > > The mpuio build fix? That is a correct fix.
> >
> > No, see drivers/usb/host/ohci-omap.c:
> >
> > -#ifdef CONFIG_ARCH_OMAP_OTG
> > +#ifdef CONFIG_USB_OTG /* CHECKME */
> >
>
> That's a Dave question.
That stuff's been whacked around a bunch, so I'm not sure
what its latest incarnation is expected to do. Looks like
that change (to USB_OTG) couldn't hurt ... go for it.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 21:07 ` Russell King - ARM Linux
@ 2008-09-03 21:13 ` Tony Lindgren
0 siblings, 0 replies; 58+ messages in thread
From: Tony Lindgren @ 2008-09-03 21:13 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Eduardo Valentin, linux-omap
* Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 14:07]:
> On Wed, Sep 03, 2008 at 01:56:18PM -0700, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [080903 13:50]:
> > > There is a 3450 public TRM somewhere AFAIK, anybody got a link for that?
> > > At least 5912 TRM is available somewhere.
> >
> > Sorry I meant 3550 public TRM. Here's the link to the 5912 TRM,
> > which covers 16xx.
> >
> > http://focus.ti.com/dsp/docs/dspsupporttechdocsc.tsp?abstractName=spru742§ionId=3&tabId=409
>
> Hmm, claims to only be 106K, and xpdf 3 claims the file is unreadable.
Grr. I've added working links for 5912 and 35xxx TRM's to my omap
page:
http://muru.com/linux/omap/
Tony
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 19:58 ` Woodruff, Richard
2008-09-03 20:30 ` Russell King - ARM Linux
2008-09-03 20:32 ` Tony Lindgren
@ 2008-09-03 21:18 ` David Brownell
2008-09-03 21:40 ` Woodruff, Richard
2 siblings, 1 reply; 58+ messages in thread
From: David Brownell @ 2008-09-03 21:18 UTC (permalink / raw)
To: Woodruff, Richard
Cc: Russell King - ARM Linux, Tony Lindgren,
linux-omap@vger.kernel.org, Eduardo Valentin
On Wednesday 03 September 2008, Woodruff, Richard wrote:
> Fixed translations do have some benefits. You can ensure that you
> are using section or super section descriptors to cover large areas.
> This does result in better TLB usage. Along with freeing up TLB
> entries you also generally avoid TLB misses on IO calls which
> touch a variety of internal spaces as part of the IRQ sequence.
... which is exactly why some linux/arch/... code makes sure ioremap()
can return fixed mappings instead of always requiring dynamic ones.
> Frankly I've never been convinced that a multi OMAP1/2/3 image makes much
> sense apart forcing better code structure
Which is actually a pretty big thing. Along with "structure", it
helps avoid #define collisions ... and also ensures that test
builds can cover increasing fractions of the code base, giving a
big win for maintainability.
Maintainability is a *big thing* ... without it, you ensure that
lots of code will begin bitrotting long before it needs to.
> and being kind of cool. Each
> chip has very different performance targets and is really better built with
> an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing multi-boots with in
> the same architecture family seems really good but across seems less so.
True enough, but that decision can be in the hands of whoever
builds a kernel. If you want to deploy an A8-optimized system,
the kernel will be only one part of the equation.
- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 58+ messages in thread
* RE: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 20:30 ` Russell King - ARM Linux
@ 2008-09-03 21:19 ` Woodruff, Richard
0 siblings, 0 replies; 58+ messages in thread
From: Woodruff, Richard @ 2008-09-03 21:19 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Tony Lindgren, David Brownell, linux-omap@vger.kernel.org,
Eduardo Valentin
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Sent: Wednesday, September 03, 2008 3:30 PM
<snip>
> On Wed, Sep 03, 2008 at 02:58:10PM -0500, Woodruff, Richard wrote:
> > Fixed translations do have some benefits. You can ensure that you are
> > using section or super section descriptors to cover large areas. This
> > does result in better TLB usage. Along with freeing up TLB entries
> you
> > also generally avoid TLB misses on IO calls which touch a variety of
> > internal spaces as part of the IRQ sequence.
>
> These are valid points, and you can arrange for ioremap to give you the
> same effect. That way, you end up using _standard_ Linux interfaces
> which everyone understands and still having your cake.
Is there some example in current code which does this? I recall ioremap's only using 4k and not combining.
Also, fixed maps are a lot easier to debug.
> > Each chip has very different performance targets and is really better
> > built with an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing
> > multi-boots with in the same architecture family seems really good
> > but across seems less so.
>
> This thread isn't about multi-boots across differnet architecture;
> please take that discussion elsewhere - it's off topic.
No but it is a consideration in how fix up is done.
Regards,
Richard W.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 21:04 ` Russell King - ARM Linux
@ 2008-09-03 21:26 ` Eduardo Valentin
2008-09-03 21:48 ` Tony Lindgren
2008-09-03 21:35 ` David Brownell
1 sibling, 1 reply; 58+ messages in thread
From: Eduardo Valentin @ 2008-09-03 21:26 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Tony Lindgren, linux-omap
Hi Russell,
On Wed, Sep 3, 2008 at 5:04 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Sep 03, 2008 at 01:37:21PM -0700, Tony Lindgren wrote:
>> * Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 12:49]:
>> > Yes, that will be virtual. But what does it mean to call:
>> >
>> > omap_set_dma_dest_params()
>> >
>> > specifying a virtual address? Can the DMA controller cope with DMAing
>> > to virtual addresses? My hunch is that the DMA controller can't cope
>> > with that, so giving it a virtual address is a bug.
>> >
>> > Let me change the question: does omap_set_dma_dest_params()'s 4th
>> > argument take a virtual or a physical address? If the former, it's
>> > prototype is wrong, and its 4th argument needs to be typed as
>> > 'void __iomem *' rather than 'unsigned long'. If the latter, the code
>> > above is wrong.
>>
>> The dma src and dest functions take physical addresses so the prototype
>> should be void __iomem *.
>
> Grr. No. Let me repeat the rule:
>
> - virtual addresses are pointer like.
> - physical addresses are integer like.
>
> So, if it's a physical address, it should be stored in an integer type
> large enough to contain it, and that means something like u32, or
> unsigned long.
>
> If it's a virtual MMIO address, then it should be something like
> 'void __iomem *', or if you want to play roulette with compiler padding,
> 'struct foo __iomem *'.
>
> Okay, so lets accept that the 4th argument to omap_set_dma_dest_params()
> is a physical address. It should be typed as 'unsigned long' (it is)
> and it then means that:
>
> diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
> index d084405..d9b1a42 100644
> --- a/arch/arm/plat-omap/mcbsp.c
> +++ b/arch/arm/plat-omap/mcbsp.c
> @@ -652,6 +652,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
> omap_set_dma_dest_params(mcbsp[id].dma_tx_lch,
> src_port,
> OMAP_DMA_AMODE_CONSTANT,
> + /* FIXME: this is a virtual address */
> mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1,
> 0, 0);
>
> @@ -713,6 +714,7 @@ int omap_mcbsp_recv_buffer(unsigned int id, dma_addr_t buffer,
> omap_set_dma_src_params(mcbsp[id].dma_rx_lch,
> src_port,
> OMAP_DMA_AMODE_CONSTANT,
> + /* FIXME: this is a virtual address */
> mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1,
> 0, 0);
>
>
> are broken because they're passing a virtual address into a function
> requiring a physical address.
Yes, the code above is wrong. Now I understood that I was messing up
with what you meant.
>
> Now, to fix this in the right way isn't going to be easy, because
> arch/arm/plat-omap/mcbsp.c doesn't know what the physical address of
> the mcbsp actually is - it's only passed the virtual address via
> platform data (eww, yuck yuck yuck)...
>
> If this was a properly reviewed platform driver, and on *any* *other*
> ARM platform, it would take the resources containing the physical
> addresses and ioremap them... and this would be a trivial bug fix.
Yes, I agreed here.
>
> I'll cook a patch up.
>
--
Eduardo Bezerra Valentin
^ permalink raw reply [flat|nested] 58+ messages in thread
* RE: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 20:32 ` Tony Lindgren
@ 2008-09-03 21:32 ` Woodruff, Richard
2008-09-03 21:35 ` Tony Lindgren
2008-09-03 21:38 ` Russell King - ARM Linux
0 siblings, 2 replies; 58+ messages in thread
From: Woodruff, Richard @ 2008-09-03 21:32 UTC (permalink / raw)
To: Tony Lindgren
Cc: Russell King - ARM Linux, David Brownell,
linux-omap@vger.kernel.org, Eduardo Valentin
> From: Tony Lindgren [mailto:tony@atomide.com]
> Sent: Wednesday, September 03, 2008 3:33 PM
<snip>
> > Fixed translations do have some benefits. You can ensure that you are
> using section or super section descriptors to cover large areas. This
> does result in better TLB usage. Along with freeing up TLB entries you
> also generally avoid TLB misses on IO calls which touch a variety of
> internal spaces as part of the IRQ sequence.
> >
> > With in a family of chips like 2420/22/23 or 3410/20/30/40 the
> internal space is mapped the same.
>
> I guess there's no advantage of using ioremap if the area is
> already mapped. For drivers that are shared across multiple
> archs or buses it makes sense.
That has been my general feeling.
Many drivers have some ISA'ish way to get address or they have dynamic ways. While many probably hate the old fixed systems in embedded it doesn't always seem so offensive.
You still can abstract resources into platform specific data areas.
> > Frankly I've never been convinced that a multi OMAP1/2/3 image makes
> much sense apart forcing better code structure and being kind of cool.
> Each chip has very different performance targets and is really better
> built with an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing multi-
> boots with in the same architecture family seems really good but across
> seems less so.
>
> It definitely makes sense from distro and maintenance point of view. If
> we did not work towards multi-omap we would already have the same code
> duplicated many times over.
Yes. But this also adds a high level of coupling. Sometimes this great like an internal MUSB driver other times it complicates development like for power aspects.
For example domain OFF mode is not compelling given how good retention is at the process node used for all OMAP2s. But it is compelling on an OMAP3. Pushing back OMAP3 needed code on to OMAP2 in this area ends up being not so beneficial for OMAP2. It causes regressions and slows down OMAP3 code adoption. If its down nicely like Paul has been working at it may turn out well. But the time to do this is extended.
Regards,
Richard W.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 21:32 ` Woodruff, Richard
@ 2008-09-03 21:35 ` Tony Lindgren
2008-09-03 21:38 ` Russell King - ARM Linux
1 sibling, 0 replies; 58+ messages in thread
From: Tony Lindgren @ 2008-09-03 21:35 UTC (permalink / raw)
To: Woodruff, Richard
Cc: Russell King - ARM Linux, David Brownell,
linux-omap@vger.kernel.org, Eduardo Valentin
* Woodruff, Richard <r-woodruff2@ti.com> [080903 14:32]:
>
> > From: Tony Lindgren [mailto:tony@atomide.com]
> > Sent: Wednesday, September 03, 2008 3:33 PM
> <snip>
> > > Fixed translations do have some benefits. You can ensure that you are
> > using section or super section descriptors to cover large areas. This
> > does result in better TLB usage. Along with freeing up TLB entries you
> > also generally avoid TLB misses on IO calls which touch a variety of
> > internal spaces as part of the IRQ sequence.
> > >
> > > With in a family of chips like 2420/22/23 or 3410/20/30/40 the
> > internal space is mapped the same.
> >
> > I guess there's no advantage of using ioremap if the area is
> > already mapped. For drivers that are shared across multiple
> > archs or buses it makes sense.
>
> That has been my general feeling.
>
> Many drivers have some ISA'ish way to get address or they have dynamic ways. While many probably hate the old fixed systems in embedded it doesn't always seem so offensive.
>
> You still can abstract resources into platform specific data areas.
>
>
> > > Frankly I've never been convinced that a multi OMAP1/2/3 image makes
> > much sense apart forcing better code structure and being kind of cool.
> > Each chip has very different performance targets and is really better
> > built with an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing multi-
> > boots with in the same architecture family seems really good but across
> > seems less so.
> >
> > It definitely makes sense from distro and maintenance point of view. If
> > we did not work towards multi-omap we would already have the same code
> > duplicated many times over.
>
> Yes. But this also adds a high level of coupling. Sometimes this great like an internal MUSB driver other times it complicates development like for power aspects.
>
> For example domain OFF mode is not compelling given how good retention is at the process node used for all OMAP2s. But it is compelling on an OMAP3. Pushing back OMAP3 needed code on to OMAP2 in this area ends up being not so beneficial for OMAP2. It causes regressions and slows down OMAP3 code adoption. If its down nicely like Paul has been working at it may turn out well. But the time to do this is extended.
Well we already have pm24xx.c and pm34xx.c. AFAIK the code compiles
for both 24xx and 34xx, but is only used on 34xx.
Tony
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 21:04 ` Russell King - ARM Linux
2008-09-03 21:26 ` Eduardo Valentin
@ 2008-09-03 21:35 ` David Brownell
2008-09-03 23:16 ` Russell King - ARM Linux
1 sibling, 1 reply; 58+ messages in thread
From: David Brownell @ 2008-09-03 21:35 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Tony Lindgren, Eduardo Valentin, linux-omap
On Wednesday 03 September 2008, Russell King - ARM Linux wrote:
> - virtual addresses are pointer like.
> - physical addresses are integer like.
>
> So, if it's a physical address, it should be stored in an integer type
> large enough to contain it, and that means something like u32, or
> unsigned long.
And to complete the puzzle: in the absense of IOMMU technologies
what's the difference between a "physical address" and a dma_addr_t?
(DMA addreses being integer-like.)
I would have expected omap_dma_set_{dest,src}_params() to accept a
dma_addr_t instead of a "physical address", on general principles.
And for those address types to be effectively the same, although it
would be allowed (albeit annoying) to have some trivial mapping
between the two (XOR some value etc).
- Dave
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 21:32 ` Woodruff, Richard
2008-09-03 21:35 ` Tony Lindgren
@ 2008-09-03 21:38 ` Russell King - ARM Linux
2008-09-03 21:46 ` Multi-Boot: Was " Woodruff, Richard
1 sibling, 1 reply; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-03 21:38 UTC (permalink / raw)
To: Woodruff, Richard
Cc: Tony Lindgren, David Brownell, linux-omap@vger.kernel.org,
Eduardo Valentin
On Wed, Sep 03, 2008 at 04:32:10PM -0500, Woodruff, Richard wrote:
>
> > From: Tony Lindgren [mailto:tony@atomide.com]
> > Sent: Wednesday, September 03, 2008 3:33 PM
> <snip>
> > > Fixed translations do have some benefits. You can ensure that you are
> > using section or super section descriptors to cover large areas. This
> > does result in better TLB usage. Along with freeing up TLB entries you
> > also generally avoid TLB misses on IO calls which touch a variety of
> > internal spaces as part of the IRQ sequence.
> > >
> > > With in a family of chips like 2420/22/23 or 3410/20/30/40 the
> > internal space is mapped the same.
> >
> > I guess there's no advantage of using ioremap if the area is
> > already mapped. For drivers that are shared across multiple
> > archs or buses it makes sense.
>
> That has been my general feeling.
>
> Many drivers have some ISA'ish way to get address or they have dynamic ways. While many probably hate the old fixed systems in embedded it doesn't always seem so offensive.
>
> You still can abstract resources into platform specific data areas.
>
>
> > > Frankly I've never been convinced that a multi OMAP1/2/3 image makes
> > much sense apart forcing better code structure and being kind of cool.
> > Each chip has very different performance targets and is really better
> > built with an optimized tool chain (ARMv5, ARMv6, ARMv7). Doing multi-
> > boots with in the same architecture family seems really good but across
> > seems less so.
> >
> > It definitely makes sense from distro and maintenance point of view. If
> > we did not work towards multi-omap we would already have the same code
> > duplicated many times over.
>
> Yes. But this also adds a high level of coupling. Sometimes this great like an internal MUSB driver other times it complicates development like for power aspects.
>
> For example domain OFF mode is not compelling given how good retention is at the process node used for all OMAP2s. But it is compelling on an OMAP3. Pushing back OMAP3 needed code on to OMAP2 in this area ends up being not so beneficial for OMAP2. It causes regressions and slows down OMAP3 code adoption. If its down nicely like Paul has been working at it may turn out well. But the time to do this is extended.
Can we _PLEASE_ take this discussion to a different thread. OFF TOPIC.
Thank you.
^ permalink raw reply [flat|nested] 58+ messages in thread
* RE: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 21:18 ` David Brownell
@ 2008-09-03 21:40 ` Woodruff, Richard
2008-09-03 22:05 ` David Brownell
0 siblings, 1 reply; 58+ messages in thread
From: Woodruff, Richard @ 2008-09-03 21:40 UTC (permalink / raw)
To: David Brownell
Cc: Russell King - ARM Linux, Tony Lindgren,
linux-omap@vger.kernel.org, Eduardo Valentin
> From: David Brownell [mailto:david-b@pacbell.net]
<snip>
> On Wednesday 03 September 2008, Woodruff, Richard wrote:
> > Fixed translations do have some benefits. You can ensure that you
> > are using section or super section descriptors to cover large areas.
> > This does result in better TLB usage. Along with freeing up TLB
> > entries you also generally avoid TLB misses on IO calls which
> > touch a variety of internal spaces as part of the IRQ sequence.
>
> ... which is exactly why some linux/arch/... code makes sure ioremap()
> can return fixed mappings instead of always requiring dynamic ones.
That does sound like a nice consistent way to do it. Which implementation of this would you recommend looking at? Do any ARMs you know of do it today?
> True enough, but that decision can be in the hands of whoever
> builds a kernel. If you want to deploy an A8-optimized system,
> the kernel will be only one part of the equation.
As long as the option exists to make the optimized build, yours is a pretty reasonable stance.
Practically, I wonder about user communities you are trying to support. If there are tons of active users at each generation then it seems to make sense to cater to them all. If generations are quick flashes then coupling seems less compelling.
Regards,
Richard W.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Multi-Boot: Was RE: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 21:38 ` Russell King - ARM Linux
@ 2008-09-03 21:46 ` Woodruff, Richard
0 siblings, 0 replies; 58+ messages in thread
From: Woodruff, Richard @ 2008-09-03 21:46 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Tony Lindgren, David Brownell, linux-omap@vger.kernel.org,
Eduardo Valentin
> From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> Can we _PLEASE_ take this discussion to a different thread. OFF TOPIC.
> Thank you.
Ok for me. Sorry if it defocused.
Regards,
Richard W.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 21:26 ` Eduardo Valentin
@ 2008-09-03 21:48 ` Tony Lindgren
0 siblings, 0 replies; 58+ messages in thread
From: Tony Lindgren @ 2008-09-03 21:48 UTC (permalink / raw)
To: Eduardo Valentin; +Cc: Russell King - ARM Linux, linux-omap
* Eduardo Valentin <edubezval@gmail.com> [080903 14:27]:
> Hi Russell,
>
> On Wed, Sep 3, 2008 at 5:04 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Wed, Sep 03, 2008 at 01:37:21PM -0700, Tony Lindgren wrote:
> >> * Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 12:49]:
> >> > Yes, that will be virtual. But what does it mean to call:
> >> >
> >> > omap_set_dma_dest_params()
> >> >
> >> > specifying a virtual address? Can the DMA controller cope with DMAing
> >> > to virtual addresses? My hunch is that the DMA controller can't cope
> >> > with that, so giving it a virtual address is a bug.
> >> >
> >> > Let me change the question: does omap_set_dma_dest_params()'s 4th
> >> > argument take a virtual or a physical address? If the former, it's
> >> > prototype is wrong, and its 4th argument needs to be typed as
> >> > 'void __iomem *' rather than 'unsigned long'. If the latter, the code
> >> > above is wrong.
> >>
> >> The dma src and dest functions take physical addresses so the prototype
> >> should be void __iomem *.
> >
> > Grr. No. Let me repeat the rule:
> >
> > - virtual addresses are pointer like.
> > - physical addresses are integer like.
> >
> > So, if it's a physical address, it should be stored in an integer type
> > large enough to contain it, and that means something like u32, or
> > unsigned long.
> >
> > If it's a virtual MMIO address, then it should be something like
> > 'void __iomem *', or if you want to play roulette with compiler padding,
> > 'struct foo __iomem *'.
Oops sorry, that's correct.
> > Okay, so lets accept that the 4th argument to omap_set_dma_dest_params()
> > is a physical address. It should be typed as 'unsigned long' (it is)
> > and it then means that:
> >
> > diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
> > index d084405..d9b1a42 100644
> > --- a/arch/arm/plat-omap/mcbsp.c
> > +++ b/arch/arm/plat-omap/mcbsp.c
> > @@ -652,6 +652,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
> > omap_set_dma_dest_params(mcbsp[id].dma_tx_lch,
> > src_port,
> > OMAP_DMA_AMODE_CONSTANT,
> > + /* FIXME: this is a virtual address */
> > mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1,
> > 0, 0);
> >
> > @@ -713,6 +714,7 @@ int omap_mcbsp_recv_buffer(unsigned int id, dma_addr_t buffer,
> > omap_set_dma_src_params(mcbsp[id].dma_rx_lch,
> > src_port,
> > OMAP_DMA_AMODE_CONSTANT,
> > + /* FIXME: this is a virtual address */
> > mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1,
> > 0, 0);
> >
> >
> > are broken because they're passing a virtual address into a function
> > requiring a physical address.
>
> Yes, the code above is wrong. Now I understood that I was messing up
> with what you meant.
>
> >
> > Now, to fix this in the right way isn't going to be easy, because
> > arch/arm/plat-omap/mcbsp.c doesn't know what the physical address of
> > the mcbsp actually is - it's only passed the virtual address via
> > platform data (eww, yuck yuck yuck)...
> >
> > If this was a properly reviewed platform driver, and on *any* *other*
> > ARM platform, it would take the resources containing the physical
> > addresses and ioremap them... and this would be a trivial bug fix.
>
> Yes, I agreed here.
> >
> > I'll cook a patch up.
> >
Heh, I don't see how we still have working audio on at least 24xx :)
Maybe the DMA src and dest addresses are really only 28-bit and it's
ignoring the rest.
Tony
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 21:40 ` Woodruff, Richard
@ 2008-09-03 22:05 ` David Brownell
2008-09-03 22:56 ` Russell King - ARM Linux
0 siblings, 1 reply; 58+ messages in thread
From: David Brownell @ 2008-09-03 22:05 UTC (permalink / raw)
To: Woodruff, Richard
Cc: Russell King - ARM Linux, Tony Lindgren,
linux-omap@vger.kernel.org, Eduardo Valentin
On Wednesday 03 September 2008, Woodruff, Richard wrote:
> > > ... [ big/inexpensive fixed mappings can be virtuous ... ]
> >
> > ... which is exactly why some linux/arch/... code makes sure ioremap()
> > can return fixed mappings instead of always requiring dynamic ones.
>
> That does sound like a nice consistent way to do it. Which implementation
> of this would you recommend looking at? Do any ARMs you know of do it today?
I was just calling attention to the possibility. ISTR seeing
that AVR32 does that, and I think some MIPS hardware has the
same kind of logic. Not just those, either.
The ARM stuff seems to have changed since last I looked!
In arch/arm/include/asm/io.h there's an __arch_ioremap()
hook; it could be used instead of __arm_ioremap() ... and
it seems <mach/io.h> would define __arch_ioremap().
So at a first glance, OMAPs _could_ define that arch function
to look at the physical address to see if it's covered by a
fixed mapping, then reuse it ... else use __arm_ioremap().
According to Mr. Grep, there are at least 10 ARMs that work
like that in mainline. Maybe Russell can recommend one of
them as a preferred model.
- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 22:05 ` David Brownell
@ 2008-09-03 22:56 ` Russell King - ARM Linux
2008-09-04 0:28 ` Tony Lindgren
` (2 more replies)
0 siblings, 3 replies; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-03 22:56 UTC (permalink / raw)
To: David Brownell
Cc: Woodruff, Richard, Tony Lindgren, linux-omap@vger.kernel.org,
Eduardo Valentin
On Wed, Sep 03, 2008 at 03:05:59PM -0700, David Brownell wrote:
> According to Mr. Grep, there are at least 10 ARMs that work
> like that in mainline. Maybe Russell can recommend one of
> them as a preferred model.
As I've been trying to say, I see this as a separate issue for the near
future. At the moment, I'm trying to concentrate on one aspect only.
That is, getting OMAP to the point that we're using the compiler to
warn us when we do something silly, like passing a virtual address
to a function which takes a physical address, and fixing the places
which are currently wrong.
Anything else like changing the ioremap behaviour is actually
completely orthogonal to that, and *is* a distraction. Rather than
actually fixing the mcbsp.c issue, I've spent the last hour or so
composing various replies to Richard's emails, and then postponing
them, reading more of this thread, creating more replies, postponing
those as well, etc.
Anyway, what I've done now is committed the minimal set of fixes so
far to go into mainline for the current -rc so we can at least improve
the situation there. That's not to say that the other patch won't be
going in - it will in some form or other.
It can also be applied to the omap tree by saving this message as
"whateverpatchfile", running:
sed -i 's,\[id\]\.,->,' whateverpatchfile
and then applying "whateverpatchfile". Expect some offsets.
Arun - can you please test this patch on your 5912 OSK board to see if
it resolves your problem please?
diff --git a/arch/arm/mach-omap1/mcbsp.c b/arch/arm/mach-omap1/mcbsp.c
index 826010d..b6ffbab 100644
--- a/arch/arm/mach-omap1/mcbsp.c
+++ b/arch/arm/mach-omap1/mcbsp.c
@@ -184,7 +184,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
#ifdef CONFIG_ARCH_OMAP15XX
static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
{
- .virt_base = OMAP1510_MCBSP1_BASE,
+ .virt_base = io_p2v(OMAP1510_MCBSP1_BASE),
.dma_rx_sync = OMAP_DMA_MCBSP1_RX,
.dma_tx_sync = OMAP_DMA_MCBSP1_TX,
.rx_irq = INT_McBSP1RX,
@@ -201,7 +201,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
.ops = &omap1_mcbsp_ops,
},
{
- .virt_base = OMAP1510_MCBSP3_BASE,
+ .virt_base = io_p2v(OMAP1510_MCBSP3_BASE),
.dma_rx_sync = OMAP_DMA_MCBSP3_RX,
.dma_tx_sync = OMAP_DMA_MCBSP3_TX,
.rx_irq = INT_McBSP3RX,
@@ -219,7 +219,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
#ifdef CONFIG_ARCH_OMAP16XX
static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
{
- .virt_base = OMAP1610_MCBSP1_BASE,
+ .virt_base = io_p2v(OMAP1610_MCBSP1_BASE),
.dma_rx_sync = OMAP_DMA_MCBSP1_RX,
.dma_tx_sync = OMAP_DMA_MCBSP1_TX,
.rx_irq = INT_McBSP1RX,
@@ -236,7 +236,7 @@ static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
.ops = &omap1_mcbsp_ops,
},
{
- .virt_base = OMAP1610_MCBSP3_BASE,
+ .virt_base = io_p2v(OMAP1610_MCBSP3_BASE),
.dma_rx_sync = OMAP_DMA_MCBSP3_RX,
.dma_tx_sync = OMAP_DMA_MCBSP3_TX,
.rx_irq = INT_McBSP3RX,
diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index d084405..5245a2a 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -651,7 +651,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
omap_set_dma_dest_params(mcbsp[id].dma_tx_lch,
src_port,
OMAP_DMA_AMODE_CONSTANT,
- mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1,
+ io_v2p(mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1),
0, 0);
omap_set_dma_src_params(mcbsp[id].dma_tx_lch,
@@ -712,7 +712,7 @@ int omap_mcbsp_recv_buffer(unsigned int id, dma_addr_t buffer,
omap_set_dma_src_params(mcbsp[id].dma_rx_lch,
src_port,
OMAP_DMA_AMODE_CONSTANT,
- mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1,
+ io_v2p(mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1),
0, 0);
omap_set_dma_dest_params(mcbsp[id].dma_rx_lch,
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 21:09 ` David Brownell
@ 2008-09-03 23:02 ` Russell King - ARM Linux
0 siblings, 0 replies; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-03 23:02 UTC (permalink / raw)
To: David Brownell; +Cc: Tony Lindgren, linux-omap, Eduardo Valentin
On Wed, Sep 03, 2008 at 02:09:14PM -0700, David Brownell wrote:
> On Wednesday 03 September 2008, Tony Lindgren wrote:
> > > > > I still want to hear on the other build fix in the patch, and there's
> > > > > also a missing function for mmc stuff which I've not looked into yet.
> > > >
> > > > The mpuio build fix? That is a correct fix.
> > >
> > > No, see drivers/usb/host/ohci-omap.c:
> > >
> > > -#ifdef CONFIG_ARCH_OMAP_OTG
> > > +#ifdef CONFIG_USB_OTG /* CHECKME */
> > >
> >
> > That's a Dave question.
>
> That stuff's been whacked around a bunch, so I'm not sure
> what its latest incarnation is expected to do. Looks like
> that change (to USB_OTG) couldn't hurt ... go for it.
Thanks.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 21:35 ` David Brownell
@ 2008-09-03 23:16 ` Russell King - ARM Linux
0 siblings, 0 replies; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-03 23:16 UTC (permalink / raw)
To: David Brownell; +Cc: Tony Lindgren, Eduardo Valentin, linux-omap
On Wed, Sep 03, 2008 at 02:35:52PM -0700, David Brownell wrote:
> On Wednesday 03 September 2008, Russell King - ARM Linux wrote:
> > - virtual addresses are pointer like.
> > - physical addresses are integer like.
> >
> > So, if it's a physical address, it should be stored in an integer type
> > large enough to contain it, and that means something like u32, or
> > unsigned long.
>
> And to complete the puzzle: in the absense of IOMMU technologies
> what's the difference between a "physical address" and a dma_addr_t?
> (DMA addreses being integer-like.)
That's a valid point, and related, but one which I don't wish to get
into at present (mainly because it isn't a problem on current hardware.)
I'm not out to solve world hunger at this point, just to shake out some
of the real problems that OMAP currently suffers from, which we can use
the standard compiler to find. That's why I'm trying to keep this thread
narrowly focussed.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 18:01 ` Tony Lindgren
@ 2008-09-04 0:16 ` David Brownell
0 siblings, 0 replies; 58+ messages in thread
From: David Brownell @ 2008-09-04 0:16 UTC (permalink / raw)
To: Tony Lindgren; +Cc: Russell King, linux-omap, Eduardo Valentin
On Wednesday 03 September 2008, Tony Lindgren wrote:
> My OSK is booting just fine here with current mainline and the
> omap_osk_5912_defconfig. This is the current head at
> 2.6.27-rc5-00100-gec0c15a. Have you checked if yours boots with
> the mainline omap_osk_5912_defconfig?
Today's root seems to work: no "sudden death" before the kernel
banner shows. I'll see if it still works after I turn it into
something more useful. For now I'm assuming one of the recent
ARM merges helped somehow.
- Dave
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 22:56 ` Russell King - ARM Linux
@ 2008-09-04 0:28 ` Tony Lindgren
2008-09-04 1:06 ` David Brownell
2008-09-04 7:25 ` Arun KS
2 siblings, 0 replies; 58+ messages in thread
From: Tony Lindgren @ 2008-09-04 0:28 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: David Brownell, Woodruff, Richard, linux-omap@vger.kernel.org,
Eduardo Valentin
* Russell King - ARM Linux <linux@arm.linux.org.uk> [080903 15:57]:
> On Wed, Sep 03, 2008 at 03:05:59PM -0700, David Brownell wrote:
> > According to Mr. Grep, there are at least 10 ARMs that work
> > like that in mainline. Maybe Russell can recommend one of
> > them as a preferred model.
>
> As I've been trying to say, I see this as a separate issue for the near
> future. At the moment, I'm trying to concentrate on one aspect only.
>
> That is, getting OMAP to the point that we're using the compiler to
> warn us when we do something silly, like passing a virtual address
> to a function which takes a physical address, and fixing the places
> which are currently wrong.
>
> Anything else like changing the ioremap behaviour is actually
> completely orthogonal to that, and *is* a distraction. Rather than
> actually fixing the mcbsp.c issue, I've spent the last hour or so
> composing various replies to Richard's emails, and then postponing
> them, reading more of this thread, creating more replies, postponing
> those as well, etc.
>
> Anyway, what I've done now is committed the minimal set of fixes so
> far to go into mainline for the current -rc so we can at least improve
> the situation there. That's not to say that the other patch won't be
> going in - it will in some form or other.
>
> It can also be applied to the omap tree by saving this message as
> "whateverpatchfile", running:
>
> sed -i 's,\[id\]\.,->,' whateverpatchfile
>
> and then applying "whateverpatchfile". Expect some offsets.
>
> Arun - can you please test this patch on your 5912 OSK board to see if
> it resolves your problem please?
Audio still works with n810 after this patch, so I'll apply it also
to l-o tree.
Can anybody from TI confirm that the DMA is using only 28 bits of the
src/dest address? At least that's the best explanation so far why it
was working earlier even without this patch.
Acked-by: Tony Lindgren <tony@atomide.com>
> diff --git a/arch/arm/mach-omap1/mcbsp.c b/arch/arm/mach-omap1/mcbsp.c
> index 826010d..b6ffbab 100644
> --- a/arch/arm/mach-omap1/mcbsp.c
> +++ b/arch/arm/mach-omap1/mcbsp.c
> @@ -184,7 +184,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
> #ifdef CONFIG_ARCH_OMAP15XX
> static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> {
> - .virt_base = OMAP1510_MCBSP1_BASE,
> + .virt_base = io_p2v(OMAP1510_MCBSP1_BASE),
> .dma_rx_sync = OMAP_DMA_MCBSP1_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP1_TX,
> .rx_irq = INT_McBSP1RX,
> @@ -201,7 +201,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> .ops = &omap1_mcbsp_ops,
> },
> {
> - .virt_base = OMAP1510_MCBSP3_BASE,
> + .virt_base = io_p2v(OMAP1510_MCBSP3_BASE),
> .dma_rx_sync = OMAP_DMA_MCBSP3_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP3_TX,
> .rx_irq = INT_McBSP3RX,
> @@ -219,7 +219,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> #ifdef CONFIG_ARCH_OMAP16XX
> static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
> {
> - .virt_base = OMAP1610_MCBSP1_BASE,
> + .virt_base = io_p2v(OMAP1610_MCBSP1_BASE),
> .dma_rx_sync = OMAP_DMA_MCBSP1_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP1_TX,
> .rx_irq = INT_McBSP1RX,
> @@ -236,7 +236,7 @@ static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
> .ops = &omap1_mcbsp_ops,
> },
> {
> - .virt_base = OMAP1610_MCBSP3_BASE,
> + .virt_base = io_p2v(OMAP1610_MCBSP3_BASE),
> .dma_rx_sync = OMAP_DMA_MCBSP3_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP3_TX,
> .rx_irq = INT_McBSP3RX,
> diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
> index d084405..5245a2a 100644
> --- a/arch/arm/plat-omap/mcbsp.c
> +++ b/arch/arm/plat-omap/mcbsp.c
> @@ -651,7 +651,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
> omap_set_dma_dest_params(mcbsp[id].dma_tx_lch,
> src_port,
> OMAP_DMA_AMODE_CONSTANT,
> - mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1,
> + io_v2p(mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1),
> 0, 0);
>
> omap_set_dma_src_params(mcbsp[id].dma_tx_lch,
> @@ -712,7 +712,7 @@ int omap_mcbsp_recv_buffer(unsigned int id, dma_addr_t buffer,
> omap_set_dma_src_params(mcbsp[id].dma_rx_lch,
> src_port,
> OMAP_DMA_AMODE_CONSTANT,
> - mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1,
> + io_v2p(mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1),
> 0, 0);
>
> omap_set_dma_dest_params(mcbsp[id].dma_rx_lch,
>
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 22:56 ` Russell King - ARM Linux
2008-09-04 0:28 ` Tony Lindgren
@ 2008-09-04 1:06 ` David Brownell
2008-09-04 7:25 ` Arun KS
2 siblings, 0 replies; 58+ messages in thread
From: David Brownell @ 2008-09-04 1:06 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Woodruff, Richard, Tony Lindgren, linux-omap@vger.kernel.org,
Eduardo Valentin
On Wednesday 03 September 2008, Russell King - ARM Linux wrote:
> On Wed, Sep 03, 2008 at 03:05:59PM -0700, David Brownell wrote:
> > According to Mr. Grep, there are at least 10 ARMs that work
> > like that [__arch_ioremap] in mainline. Maybe Russell can
> > recommend one of them as a preferred model.
>
> As I've been trying to say, I see this as a separate issue for the near
> future. At the moment, I'm trying to concentrate on one aspect only.
OK ... if you think that's a "near future" thing, great! I think
most bad usage in this area came from:
- Intentional "performance hacks", achieving what can
better be done with an arch_ioremap and fixed mappings;
- Recently introduced goofage;
- Accidents/misunderstandings.
Those latter two will become much less common when GCC starts to
report errors which previously required a separate "sparse" run.
> That is, getting OMAP to the point that we're using the compiler to
> warn us when we do something silly, like passing a virtual address
> to a function which takes a physical address, and fixing the places
> which are currently wrong.
Yeah. I did a pass like that over a lot of OMAP1 drivers a few years
back (before OMAP2/OMAP3); "sparse" was a big help. If we're now ready
to have GCC tell us that stuff, that's a lot better.
- Dave
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 22:56 ` Russell King - ARM Linux
2008-09-04 0:28 ` Tony Lindgren
2008-09-04 1:06 ` David Brownell
@ 2008-09-04 7:25 ` Arun KS
2 siblings, 0 replies; 58+ messages in thread
From: Arun KS @ 2008-09-04 7:25 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: David Brownell, Woodruff, Richard, Tony Lindgren,
linux-omap@vger.kernel.org, Eduardo Valentin
Hi Russel,
Sorry for late reply.
I applied the patch and tested, but it didn't resolves my problem.
I found that code hangs at this point.
File : arch/arm/plat-omap/mcbsp.c
Function: omap_mcbsp_request
if (mcbsp->pdata && mcbsp->pdata->ops && mcbsp->pdata->ops->request)
mcbsp->pdata->ops->request(id);
I am requesting mcbsp1.
Arun KS
On Thu, Sep 4, 2008 at 4:26 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Sep 03, 2008 at 03:05:59PM -0700, David Brownell wrote:
>> According to Mr. Grep, there are at least 10 ARMs that work
>> like that in mainline. Maybe Russell can recommend one of
>> them as a preferred model.
>
> As I've been trying to say, I see this as a separate issue for the near
> future. At the moment, I'm trying to concentrate on one aspect only.
>
> That is, getting OMAP to the point that we're using the compiler to
> warn us when we do something silly, like passing a virtual address
> to a function which takes a physical address, and fixing the places
> which are currently wrong.
>
> Anything else like changing the ioremap behaviour is actually
> completely orthogonal to that, and *is* a distraction. Rather than
> actually fixing the mcbsp.c issue, I've spent the last hour or so
> composing various replies to Richard's emails, and then postponing
> them, reading more of this thread, creating more replies, postponing
> those as well, etc.
>
> Anyway, what I've done now is committed the minimal set of fixes so
> far to go into mainline for the current -rc so we can at least improve
> the situation there. That's not to say that the other patch won't be
> going in - it will in some form or other.
>
> It can also be applied to the omap tree by saving this message as
> "whateverpatchfile", running:
>
> sed -i 's,\[id\]\.,->,' whateverpatchfile
>
> and then applying "whateverpatchfile". Expect some offsets.
>
> Arun - can you please test this patch on your 5912 OSK board to see if
> it resolves your problem please?
>
> diff --git a/arch/arm/mach-omap1/mcbsp.c b/arch/arm/mach-omap1/mcbsp.c
> index 826010d..b6ffbab 100644
> --- a/arch/arm/mach-omap1/mcbsp.c
> +++ b/arch/arm/mach-omap1/mcbsp.c
> @@ -184,7 +184,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
> #ifdef CONFIG_ARCH_OMAP15XX
> static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> {
> - .virt_base = OMAP1510_MCBSP1_BASE,
> + .virt_base = io_p2v(OMAP1510_MCBSP1_BASE),
> .dma_rx_sync = OMAP_DMA_MCBSP1_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP1_TX,
> .rx_irq = INT_McBSP1RX,
> @@ -201,7 +201,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> .ops = &omap1_mcbsp_ops,
> },
> {
> - .virt_base = OMAP1510_MCBSP3_BASE,
> + .virt_base = io_p2v(OMAP1510_MCBSP3_BASE),
> .dma_rx_sync = OMAP_DMA_MCBSP3_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP3_TX,
> .rx_irq = INT_McBSP3RX,
> @@ -219,7 +219,7 @@ static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> #ifdef CONFIG_ARCH_OMAP16XX
> static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
> {
> - .virt_base = OMAP1610_MCBSP1_BASE,
> + .virt_base = io_p2v(OMAP1610_MCBSP1_BASE),
> .dma_rx_sync = OMAP_DMA_MCBSP1_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP1_TX,
> .rx_irq = INT_McBSP1RX,
> @@ -236,7 +236,7 @@ static struct omap_mcbsp_platform_data omap16xx_mcbsp_pdata[] = {
> .ops = &omap1_mcbsp_ops,
> },
> {
> - .virt_base = OMAP1610_MCBSP3_BASE,
> + .virt_base = io_p2v(OMAP1610_MCBSP3_BASE),
> .dma_rx_sync = OMAP_DMA_MCBSP3_RX,
> .dma_tx_sync = OMAP_DMA_MCBSP3_TX,
> .rx_irq = INT_McBSP3RX,
> diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
> index d084405..5245a2a 100644
> --- a/arch/arm/plat-omap/mcbsp.c
> +++ b/arch/arm/plat-omap/mcbsp.c
> @@ -651,7 +651,7 @@ int omap_mcbsp_xmit_buffer(unsigned int id, dma_addr_t buffer,
> omap_set_dma_dest_params(mcbsp[id].dma_tx_lch,
> src_port,
> OMAP_DMA_AMODE_CONSTANT,
> - mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1,
> + io_v2p(mcbsp[id].io_base + OMAP_MCBSP_REG_DXR1),
> 0, 0);
>
> omap_set_dma_src_params(mcbsp[id].dma_tx_lch,
> @@ -712,7 +712,7 @@ int omap_mcbsp_recv_buffer(unsigned int id, dma_addr_t buffer,
> omap_set_dma_src_params(mcbsp[id].dma_rx_lch,
> src_port,
> OMAP_DMA_AMODE_CONSTANT,
> - mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1,
> + io_v2p(mcbsp[id].io_base + OMAP_MCBSP_REG_DRR1),
> 0, 0);
>
> omap_set_dma_dest_params(mcbsp[id].dma_rx_lch,
>
>
>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-03 15:33 ` Eduardo Valentin
2008-09-03 18:48 ` Russell King
@ 2008-09-04 9:46 ` Russell King - ARM Linux
2008-09-04 16:10 ` Tony Lindgren
1 sibling, 1 reply; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-04 9:46 UTC (permalink / raw)
To: Eduardo Valentin; +Cc: linux-omap
On Wed, Sep 03, 2008 at 11:33:51AM -0400, Eduardo Valentin wrote:
> > @@ -159,7 +159,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
> > #ifdef CONFIG_ARCH_OMAP15XX
> > static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> > {
> > - .virt_base = OMAP1510_MCBSP1_BASE,
> > + .virt_base = OMAP1510_MCBSP1_BASE, /* FIXME: virtual or physical */
> AFAIK, OMAP1510_MCBSP1_BASE is physical. So, I'd say:
> + .virt_base = IO_ADDRESS(OMAP1510_MCBSP1_BASE),
>
> Because, plat-omap/mcbsp.c expect .virt_base to be a virtual address.
And today, the story is completely different, having looked through more
of the code and some documentation.
- OMAPxxxx_MCBSPx_BASE are all physical addresses. Fine.
- physical addresses > 0xfffb0000 are subject to an offset (IO_OFFSET)
but others for the DSP located and DSP shared peripherals aren't.
So, applying the IO_OFFSET via IO_ADDRESS() or io_p2v() to all addresses
breaks. Meaning it's completely random whether something should be put
through IO_ADDRESS() and similar.
This isn't obvious. It isn't readable. It isn't maintainable. It doesn't
lend itself to compile time checking.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-04 9:46 ` Russell King - ARM Linux
@ 2008-09-04 16:10 ` Tony Lindgren
2008-09-04 16:12 ` Russell King - ARM Linux
0 siblings, 1 reply; 58+ messages in thread
From: Tony Lindgren @ 2008-09-04 16:10 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Eduardo Valentin, linux-omap
* Russell King - ARM Linux <linux@arm.linux.org.uk> [080904 02:47]:
> On Wed, Sep 03, 2008 at 11:33:51AM -0400, Eduardo Valentin wrote:
> > > @@ -159,7 +159,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
> > > #ifdef CONFIG_ARCH_OMAP15XX
> > > static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> > > {
> > > - .virt_base = OMAP1510_MCBSP1_BASE,
> > > + .virt_base = OMAP1510_MCBSP1_BASE, /* FIXME: virtual or physical */
> > AFAIK, OMAP1510_MCBSP1_BASE is physical. So, I'd say:
> > + .virt_base = IO_ADDRESS(OMAP1510_MCBSP1_BASE),
> >
> > Because, plat-omap/mcbsp.c expect .virt_base to be a virtual address.
>
> And today, the story is completely different, having looked through more
> of the code and some documentation.
>
> - OMAPxxxx_MCBSPx_BASE are all physical addresses. Fine.
> - physical addresses > 0xfffb0000 are subject to an offset (IO_OFFSET)
> but others for the DSP located and DSP shared peripherals aren't.
>
> So, applying the IO_OFFSET via IO_ADDRESS() or io_p2v() to all addresses
> breaks. Meaning it's completely random whether something should be put
> through IO_ADDRESS() and similar.
>
> This isn't obvious. It isn't readable. It isn't maintainable. It doesn't
> lend itself to compile time checking.
Ouch. Looking at the history of the mcbsp.c the MCBSPX_BASE defines have
been a mix of virt and phys addresses to start with.
Looks like in the short term we need to define both virt_base and phys_base
for omap_mcbsp_platform_data.
Then define __arch_ioremap that understands also the DSP mappings and get
rid of the remaining hardcoded virtual DSP defines.
At that point we can also remove the virt_base from omap_mcbsp_platform_data.
Tony
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-04 16:10 ` Tony Lindgren
@ 2008-09-04 16:12 ` Russell King - ARM Linux
2008-09-04 16:29 ` Tony Lindgren
0 siblings, 1 reply; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-04 16:12 UTC (permalink / raw)
To: Tony Lindgren; +Cc: Eduardo Valentin, linux-omap
On Thu, Sep 04, 2008 at 09:10:34AM -0700, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [080904 02:47]:
> > On Wed, Sep 03, 2008 at 11:33:51AM -0400, Eduardo Valentin wrote:
> > > > @@ -159,7 +159,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
> > > > #ifdef CONFIG_ARCH_OMAP15XX
> > > > static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> > > > {
> > > > - .virt_base = OMAP1510_MCBSP1_BASE,
> > > > + .virt_base = OMAP1510_MCBSP1_BASE, /* FIXME: virtual or physical */
> > > AFAIK, OMAP1510_MCBSP1_BASE is physical. So, I'd say:
> > > + .virt_base = IO_ADDRESS(OMAP1510_MCBSP1_BASE),
> > >
> > > Because, plat-omap/mcbsp.c expect .virt_base to be a virtual address.
> >
> > And today, the story is completely different, having looked through more
> > of the code and some documentation.
> >
> > - OMAPxxxx_MCBSPx_BASE are all physical addresses. Fine.
> > - physical addresses > 0xfffb0000 are subject to an offset (IO_OFFSET)
> > but others for the DSP located and DSP shared peripherals aren't.
> >
> > So, applying the IO_OFFSET via IO_ADDRESS() or io_p2v() to all addresses
> > breaks. Meaning it's completely random whether something should be put
> > through IO_ADDRESS() and similar.
> >
> > This isn't obvious. It isn't readable. It isn't maintainable. It doesn't
> > lend itself to compile time checking.
>
> Ouch. Looking at the history of the mcbsp.c the MCBSPX_BASE defines have
> been a mix of virt and phys addresses to start with.
>
> Looks like in the short term we need to define both virt_base and phys_base
> for omap_mcbsp_platform_data.
>
> Then define __arch_ioremap that understands also the DSP mappings and get
> rid of the remaining hardcoded virtual DSP defines.
>
> At that point we can also remove the virt_base from omap_mcbsp_platform_data.
Already done. ;)
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-04 16:12 ` Russell King - ARM Linux
@ 2008-09-04 16:29 ` Tony Lindgren
2008-09-04 17:07 ` Russell King - ARM Linux
0 siblings, 1 reply; 58+ messages in thread
From: Tony Lindgren @ 2008-09-04 16:29 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Eduardo Valentin, linux-omap
* Russell King - ARM Linux <linux@arm.linux.org.uk> [080904 09:13]:
> On Thu, Sep 04, 2008 at 09:10:34AM -0700, Tony Lindgren wrote:
> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [080904 02:47]:
> > > On Wed, Sep 03, 2008 at 11:33:51AM -0400, Eduardo Valentin wrote:
> > > > > @@ -159,7 +159,7 @@ static struct omap_mcbsp_platform_data omap730_mcbsp_pdata[] = {
> > > > > #ifdef CONFIG_ARCH_OMAP15XX
> > > > > static struct omap_mcbsp_platform_data omap15xx_mcbsp_pdata[] = {
> > > > > {
> > > > > - .virt_base = OMAP1510_MCBSP1_BASE,
> > > > > + .virt_base = OMAP1510_MCBSP1_BASE, /* FIXME: virtual or physical */
> > > > AFAIK, OMAP1510_MCBSP1_BASE is physical. So, I'd say:
> > > > + .virt_base = IO_ADDRESS(OMAP1510_MCBSP1_BASE),
> > > >
> > > > Because, plat-omap/mcbsp.c expect .virt_base to be a virtual address.
> > >
> > > And today, the story is completely different, having looked through more
> > > of the code and some documentation.
> > >
> > > - OMAPxxxx_MCBSPx_BASE are all physical addresses. Fine.
> > > - physical addresses > 0xfffb0000 are subject to an offset (IO_OFFSET)
> > > but others for the DSP located and DSP shared peripherals aren't.
> > >
> > > So, applying the IO_OFFSET via IO_ADDRESS() or io_p2v() to all addresses
> > > breaks. Meaning it's completely random whether something should be put
> > > through IO_ADDRESS() and similar.
> > >
> > > This isn't obvious. It isn't readable. It isn't maintainable. It doesn't
> > > lend itself to compile time checking.
> >
> > Ouch. Looking at the history of the mcbsp.c the MCBSPX_BASE defines have
> > been a mix of virt and phys addresses to start with.
> >
> > Looks like in the short term we need to define both virt_base and phys_base
> > for omap_mcbsp_platform_data.
> >
> > Then define __arch_ioremap that understands also the DSP mappings and get
> > rid of the remaining hardcoded virtual DSP defines.
> >
> > At that point we can also remove the virt_base from omap_mcbsp_platform_data.
>
> Already done. ;)
That was fast :) Sounds we'll have a patch to test soon.
Tony
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-04 16:29 ` Tony Lindgren
@ 2008-09-04 17:07 ` Russell King - ARM Linux
2008-09-04 17:58 ` Tony Lindgren
0 siblings, 1 reply; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-04 17:07 UTC (permalink / raw)
To: Tony Lindgren; +Cc: Eduardo Valentin, linux-omap
On Thu, Sep 04, 2008 at 09:29:19AM -0700, Tony Lindgren wrote:
> That was fast :) Sounds we'll have a patch to test soon.
Of course. In my tree. The master branch contains the minimal fixes,
and the devel branch contains everything. If you want the individual
patches, either pull the tree or grab the mboxes.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-04 17:07 ` Russell King - ARM Linux
@ 2008-09-04 17:58 ` Tony Lindgren
2008-09-04 21:01 ` Russell King - ARM Linux
2008-09-05 5:17 ` Paul Walmsley
0 siblings, 2 replies; 58+ messages in thread
From: Tony Lindgren @ 2008-09-04 17:58 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Eduardo Valentin, linux-omap
* Russell King - ARM Linux <linux@arm.linux.org.uk> [080904 10:07]:
> On Thu, Sep 04, 2008 at 09:29:19AM -0700, Tony Lindgren wrote:
> > That was fast :) Sounds we'll have a patch to test soon.
>
> Of course. In my tree. The master branch contains the minimal fixes,
> and the devel branch contains everything. If you want the individual
> patches, either pull the tree or grab the mboxes.
Hey looks good! I'll cherry pick them to l-o tree, then let's make
sure things work as expected.
Tony
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-04 17:58 ` Tony Lindgren
@ 2008-09-04 21:01 ` Russell King - ARM Linux
2008-09-04 21:20 ` Tony Lindgren
2008-09-05 5:17 ` Paul Walmsley
1 sibling, 1 reply; 58+ messages in thread
From: Russell King - ARM Linux @ 2008-09-04 21:01 UTC (permalink / raw)
To: Tony Lindgren; +Cc: Eduardo Valentin, linux-omap
On Thu, Sep 04, 2008 at 10:58:13AM -0700, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [080904 10:07]:
> > On Thu, Sep 04, 2008 at 09:29:19AM -0700, Tony Lindgren wrote:
> > > That was fast :) Sounds we'll have a patch to test soon.
> >
> > Of course. In my tree. The master branch contains the minimal fixes,
> > and the devel branch contains everything. If you want the individual
> > patches, either pull the tree or grab the mboxes.
>
> Hey looks good! I'll cherry pick them to l-o tree, then let's make
> sure things work as expected.
BTW, having an ack (or even an ok) for that last patch in the master
branch would be useful:
[ARM] omap: fix virtual vs physical address space confusions
Planning to send them off tonight.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-04 21:01 ` Russell King - ARM Linux
@ 2008-09-04 21:20 ` Tony Lindgren
2008-09-05 1:07 ` Tony Lindgren
0 siblings, 1 reply; 58+ messages in thread
From: Tony Lindgren @ 2008-09-04 21:20 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Eduardo Valentin, linux-omap
* Russell King - ARM Linux <linux@arm.linux.org.uk> [080904 14:01]:
> On Thu, Sep 04, 2008 at 10:58:13AM -0700, Tony Lindgren wrote:
> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [080904 10:07]:
> > > On Thu, Sep 04, 2008 at 09:29:19AM -0700, Tony Lindgren wrote:
> > > > That was fast :) Sounds we'll have a patch to test soon.
> > >
> > > Of course. In my tree. The master branch contains the minimal fixes,
> > > and the devel branch contains everything. If you want the individual
> > > patches, either pull the tree or grab the mboxes.
> >
> > Hey looks good! I'll cherry pick them to l-o tree, then let's make
> > sure things work as expected.
>
> BTW, having an ack (or even an ok) for that last patch in the master
> branch would be useful:
>
> [ARM] omap: fix virtual vs physical address space confusions
Here you are:
Acked-by: Tony Lindgren <tony@atomide.com>
> Planning to send them off tonight.
Great.
Also, I'm about to push your devel patches to l-o tree once I get
done transplanting them. So far things are working just fine.
Tony
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-04 21:20 ` Tony Lindgren
@ 2008-09-05 1:07 ` Tony Lindgren
0 siblings, 0 replies; 58+ messages in thread
From: Tony Lindgren @ 2008-09-05 1:07 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Eduardo Valentin, linux-omap
* Tony Lindgren <tony@atomide.com> [080904 14:20]:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [080904 14:01]:
> > On Thu, Sep 04, 2008 at 10:58:13AM -0700, Tony Lindgren wrote:
> > > * Russell King - ARM Linux <linux@arm.linux.org.uk> [080904 10:07]:
> > > > On Thu, Sep 04, 2008 at 09:29:19AM -0700, Tony Lindgren wrote:
> > > > > That was fast :) Sounds we'll have a patch to test soon.
> > > >
> > > > Of course. In my tree. The master branch contains the minimal fixes,
> > > > and the devel branch contains everything. If you want the individual
> > > > patches, either pull the tree or grab the mboxes.
> > >
> > > Hey looks good! I'll cherry pick them to l-o tree, then let's make
> > > sure things work as expected.
> >
> > BTW, having an ack (or even an ok) for that last patch in the master
> > branch would be useful:
> >
> > [ARM] omap: fix virtual vs physical address space confusions
>
> Here you are:
>
> Acked-by: Tony Lindgren <tony@atomide.com>
>
> > Planning to send them off tonight.
>
> Great.
>
> Also, I'm about to push your devel patches to l-o tree once I get
> done transplanting them. So far things are working just fine.
Your devel patches are pushed now too to l-o. Here's my ack for the
remaining patches:
[ARM] omap: improve is_omap_port()
[SERIAL] 8250: serial8250_port_size() - omap ports are larger
[ARM] omap: remove an io_v2p() usage
[ARM] omap: allow ioremap() to use our fixed IO mappings
[ARM] omap: convert OMAP drivers to use ioremap()
[ARM] omap: convert mcbsp to use ioremap()
omap: Fix IO_ADDRESS() macros
[ARM] omap: make sure virtual mmio addresses are __iomem pointer-like
Acked-by: Tony Lindgren <tony@atomide.com>
Thanks,
Tony
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-04 17:58 ` Tony Lindgren
2008-09-04 21:01 ` Russell King - ARM Linux
@ 2008-09-05 5:17 ` Paul Walmsley
2008-09-05 5:58 ` Paul Walmsley
1 sibling, 1 reply; 58+ messages in thread
From: Paul Walmsley @ 2008-09-05 5:17 UTC (permalink / raw)
To: Tony Lindgren; +Cc: Russell King - ARM Linux, linux-omap
Hello Russell, Tony,
in arch/arm/plat-omap/include/mach/io.h, shouldn't these casts be
"__force void __iomem *" ?
#define IO_ADDRESS(pa) ((void __iomem *)__IO_ADDRESS(pa))
#define OMAP1_IO_ADDRESS(pa) ((void __iomem *)__OMAP1_IO_ADDRESS(pa))
#define OMAP2_IO_ADDRESS(pa) ((void __iomem *)__OMAP2_IO_ADDRESS(pa))
sparse is emitting warnings again - i.e.,
arch/arm/mach-omap2/irq.c:150:21: warning: cast adds address space to
expression (<asn:2>)
- Paul
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-05 5:17 ` Paul Walmsley
@ 2008-09-05 5:58 ` Paul Walmsley
2008-09-29 5:16 ` Arun KS
0 siblings, 1 reply; 58+ messages in thread
From: Paul Walmsley @ 2008-09-05 5:58 UTC (permalink / raw)
To: Tony Lindgren; +Cc: Russell King - ARM Linux, linux-omap
On Thu, 4 Sep 2008, Paul Walmsley wrote:
> Hello Russell, Tony,
>
> in arch/arm/plat-omap/include/mach/io.h, shouldn't these casts be
> "__force void __iomem *" ?
>
> #define IO_ADDRESS(pa) ((void __iomem *)__IO_ADDRESS(pa))
> #define OMAP1_IO_ADDRESS(pa) ((void __iomem *)__OMAP1_IO_ADDRESS(pa))
> #define OMAP2_IO_ADDRESS(pa) ((void __iomem *)__OMAP2_IO_ADDRESS(pa))
>
> sparse is emitting warnings again - i.e.,
>
> arch/arm/mach-omap2/irq.c:150:21: warning: cast adds address space to
> expression (<asn:2>)
Eh, never mind, most of these warnings will go away after ioremap()
conversion...
- Paul
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-05 5:58 ` Paul Walmsley
@ 2008-09-29 5:16 ` Arun KS
2008-09-29 7:44 ` Jarkko Nikula
0 siblings, 1 reply; 58+ messages in thread
From: Arun KS @ 2008-09-29 5:16 UTC (permalink / raw)
To: linux-omap; +Cc: Tony Lindgren, Russell King - ARM Linux, Paul Walmsley
Hi All,
I am finished with the ASOC driver for tlvaic23b codec on osk.
I tested the playback and capture.
But have certain issue which stops me from sending the patches.
The system hangs when there is a request for mcbsp in the platform driver
of omap ie.. at
File:sound/soc/omap/omap-mcbsp.c
Function: omap_mcbsp_dai_startup
omap_mcbsp_request(mcbsp_data->bus_id);
This is executed when we do an aplay.
In the ALSA driver(sound/arm/omap/omap-alsa.c:513) the
omap_mcbsp_request() is during the boot time.
There no issues.
So during my testing of the ASOC driver i comment out
omap_mcbsp_request(mcbsp_data->bus_id); from
platform driver (sound/soc/omap/omap-mcbsp.c) and insted requested the
omap_mcbsp in my
moudule_init of machine driver(ie sound/soc/omap/osk.c).
Write which causes this hang is at
File: arch/arm/mach-omap1/mcbsp.c
Function: omap1_mcbsp_request
__raw_writew(__raw_readw(DSP_RSTCT2) | DPS_RSTCT2_PER_EN |
DSP_RSTCT2_WD_PER_EN, DSP_RSTCT2);
Anybody has some clues to fix this.
Thanks
Arun
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-29 5:16 ` Arun KS
@ 2008-09-29 7:44 ` Jarkko Nikula
2008-09-29 9:24 ` Arun KS
0 siblings, 1 reply; 58+ messages in thread
From: Jarkko Nikula @ 2008-09-29 7:44 UTC (permalink / raw)
To: ext Arun KS
Cc: linux-omap, Tony Lindgren, Russell King - ARM Linux,
Paul Walmsley
On Mon, 29 Sep 2008 10:46:33 +0530
"ext Arun KS" <arunks@mistralsolutions.com> wrote:
> The system hangs when there is a request for mcbsp in the platform
> driver of omap ie.. at
> File:sound/soc/omap/omap-mcbsp.c
> Function: omap_mcbsp_dai_startup
> omap_mcbsp_request(mcbsp_data->bus_id);
> This is executed when we do an aplay.
>
Was this patch included?
commit f9d06c24f43b7620b14e4dd6dbf667fa68457766
Author: Jarkko Nikula <jarkko.nikula@nokia.com>
Date: Mon Sep 22 16:47:51 2008 +0300
ARM: OMAP: Fixes to omap_mcbsp_request function
Bootloader may let McBSP logic running so make sure that block is
idle before requesting IRQs. Also make sure that TX and RX waitqueues
are initialized before request_irq.
Also you can get some debug messages from mcbsp by patch below and
"echo 8 > /proc/sys/kernel/printk" in your target before executing
aplay.
---
diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
index 798c91e..2bfb177 100644
--- a/arch/arm/plat-omap/mcbsp.c
+++ b/arch/arm/plat-omap/mcbsp.c
@@ -12,6 +12,7 @@
* Multichannel mode not supported.
*/
+#define DEBUG
#include <linux/module.h>
#include <linux/init.h>
#include <linux/device.h>
---
> In the ALSA driver(sound/arm/omap/omap-alsa.c:513) the
> omap_mcbsp_request() is during the boot time.
> There no issues.
>
> So during my testing of the ASOC driver i comment out
> omap_mcbsp_request(mcbsp_data->bus_id); from
> platform driver (sound/soc/omap/omap-mcbsp.c) and insted requested the
> omap_mcbsp in my
> moudule_init of machine driver(ie sound/soc/omap/osk.c).
>
Legacy driver is doing some clock setup in snd_omap_alsa_post_probe
before omap_mcbsp_request.
alsa_codec_config->codec_clock_setup();
alsa_codec_config->codec_clock_on();
omap_mcbsp_request(AUDIO_MCBSP);
If my patch wasn't included I can understand that it would explain why
legacy driver is working. If it was, then there still might be some
registers which must be set to reset default before requesting irqs in
omap_mcbsp_request.
Jarkko
^ permalink raw reply related [flat|nested] 58+ messages in thread
* Re: FOR COMMENT: void __iomem * and similar casts are Bad News
2008-09-29 7:44 ` Jarkko Nikula
@ 2008-09-29 9:24 ` Arun KS
0 siblings, 0 replies; 58+ messages in thread
From: Arun KS @ 2008-09-29 9:24 UTC (permalink / raw)
To: Jarkko Nikula
Cc: linux-omap, Tony Lindgren, Russell King - ARM Linux,
Paul Walmsley
On Mon, Sep 29, 2008 at 1:14 PM, Jarkko Nikula <jarkko.nikula@nokia.com> wrote:
> On Mon, 29 Sep 2008 10:46:33 +0530
> "ext Arun KS" <arunks@mistralsolutions.com> wrote:
>
>> The system hangs when there is a request for mcbsp in the platform
>> driver of omap ie.. at
>> File:sound/soc/omap/omap-mcbsp.c
>> Function: omap_mcbsp_dai_startup
>> omap_mcbsp_request(mcbsp_data->bus_id);
>> This is executed when we do an aplay.
>>
> Was this patch included?
>
> commit f9d06c24f43b7620b14e4dd6dbf667fa68457766
> Author: Jarkko Nikula <jarkko.nikula@nokia.com>
> Date: Mon Sep 22 16:47:51 2008 +0300
>
> ARM: OMAP: Fixes to omap_mcbsp_request function
>
> Bootloader may let McBSP logic running so make sure that block is
> idle before requesting IRQs. Also make sure that TX and RX waitqueues
> are initialized before request_irq.
This patch is included. I am using latest l-o tree.
>
> Also you can get some debug messages from mcbsp by patch below and
> "echo 8 > /proc/sys/kernel/printk" in your target before executing
> aplay.
>
> ---
> diff --git a/arch/arm/plat-omap/mcbsp.c b/arch/arm/plat-omap/mcbsp.c
> index 798c91e..2bfb177 100644
> --- a/arch/arm/plat-omap/mcbsp.c
> +++ b/arch/arm/plat-omap/mcbsp.c
> @@ -12,6 +12,7 @@
> * Multichannel mode not supported.
> */
>
> +#define DEBUG
> #include <linux/module.h>
> #include <linux/init.h>
> #include <linux/device.h>
> ---
>
>> In the ALSA driver(sound/arm/omap/omap-alsa.c:513) the
>> omap_mcbsp_request() is during the boot time.
>> There no issues.
>>
>> So during my testing of the ASOC driver i comment out
>> omap_mcbsp_request(mcbsp_data->bus_id); from
>> platform driver (sound/soc/omap/omap-mcbsp.c) and insted requested the
>> omap_mcbsp in my
>> moudule_init of machine driver(ie sound/soc/omap/osk.c).
>>
> Legacy driver is doing some clock setup in snd_omap_alsa_post_probe
> before omap_mcbsp_request.
>
> alsa_codec_config->codec_clock_setup();
> alsa_codec_config->codec_clock_on();
>
> omap_mcbsp_request(AUDIO_MCBSP);
All these setting i had taken care off in the machine driver osk.c
>
> If my patch wasn't included I can understand that it would explain why
> legacy driver is working. If it was, then there still might be some
> registers which must be set to reset default before requesting irqs in
> omap_mcbsp_request.
Write which causes this hang is at
File: arch/arm/mach-omap1/mcbsp.c
Function: omap1_mcbsp_request
__raw_writew(__raw_readw(DSP_RSTCT2) | DPS_RSTCT2_PER_EN |
DSP_RSTCT2_WD_PER_EN, DSP_RSTCT2);
This is the exact place where the system hangs. This function is
called from omap_mcbsp_request().
If i call omap_mcbsp_request in init funciton of machine dirver(ie
osk.c), there is no problem.
Is there any memory mapping happening after booting that may result
this write to hit
some other registers?
Arun
>
>
> Jarkko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 58+ messages in thread
end of thread, other threads:[~2008-09-29 9:24 UTC | newest]
Thread overview: 58+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-08-27 22:08 FOR COMMENT: void __iomem * and similar casts are Bad News Russell King
2008-08-31 21:47 ` David Brownell
2008-09-02 22:15 ` Tony Lindgren
2008-09-03 7:55 ` Russell King - ARM Linux
2008-09-03 16:40 ` Tony Lindgren
2008-09-03 19:34 ` Russell King - ARM Linux
2008-09-03 19:48 ` Tony Lindgren
2008-09-03 21:09 ` David Brownell
2008-09-03 23:02 ` Russell King - ARM Linux
2008-09-03 19:58 ` Woodruff, Richard
2008-09-03 20:30 ` Russell King - ARM Linux
2008-09-03 21:19 ` Woodruff, Richard
2008-09-03 20:32 ` Tony Lindgren
2008-09-03 21:32 ` Woodruff, Richard
2008-09-03 21:35 ` Tony Lindgren
2008-09-03 21:38 ` Russell King - ARM Linux
2008-09-03 21:46 ` Multi-Boot: Was " Woodruff, Richard
2008-09-03 21:18 ` David Brownell
2008-09-03 21:40 ` Woodruff, Richard
2008-09-03 22:05 ` David Brownell
2008-09-03 22:56 ` Russell King - ARM Linux
2008-09-04 0:28 ` Tony Lindgren
2008-09-04 1:06 ` David Brownell
2008-09-04 7:25 ` Arun KS
2008-09-03 15:07 ` Eduardo Valentin
2008-09-03 18:01 ` Tony Lindgren
2008-09-04 0:16 ` David Brownell
2008-09-03 15:33 ` Eduardo Valentin
2008-09-03 18:48 ` Russell King
2008-09-03 19:33 ` Eduardo Valentin
2008-09-03 19:48 ` Russell King - ARM Linux
2008-09-03 20:04 ` Eduardo Valentin
2008-09-03 20:45 ` Russell King - ARM Linux
2008-09-03 20:50 ` Tony Lindgren
2008-09-03 20:56 ` Tony Lindgren
2008-09-03 21:07 ` Russell King - ARM Linux
2008-09-03 21:13 ` Tony Lindgren
2008-09-03 21:00 ` Koen Kooi
2008-09-03 20:37 ` Tony Lindgren
2008-09-03 21:04 ` Russell King - ARM Linux
2008-09-03 21:26 ` Eduardo Valentin
2008-09-03 21:48 ` Tony Lindgren
2008-09-03 21:35 ` David Brownell
2008-09-03 23:16 ` Russell King - ARM Linux
2008-09-04 9:46 ` Russell King - ARM Linux
2008-09-04 16:10 ` Tony Lindgren
2008-09-04 16:12 ` Russell King - ARM Linux
2008-09-04 16:29 ` Tony Lindgren
2008-09-04 17:07 ` Russell King - ARM Linux
2008-09-04 17:58 ` Tony Lindgren
2008-09-04 21:01 ` Russell King - ARM Linux
2008-09-04 21:20 ` Tony Lindgren
2008-09-05 1:07 ` Tony Lindgren
2008-09-05 5:17 ` Paul Walmsley
2008-09-05 5:58 ` Paul Walmsley
2008-09-29 5:16 ` Arun KS
2008-09-29 7:44 ` Jarkko Nikula
2008-09-29 9:24 ` Arun KS
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox