2007-08-08 03:38:11

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 00/12] Blackfin arch GPIO updating

As David mentioned, I send out these series patch to LKML for review.
These patches are related Blackfin arch GPIO updating, not big change at all.
I think it is OK for git-pull in -RC2 or later.

Thanks
- Bryan Wu


2007-08-08 03:35:59

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 03/12] Blackfin arch: fix PORT_J BUG for BF537/6 EMAC driver

From: Michael Hennerich <[email protected]>

Signed-off-by: Michael Hennerich <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
arch/blackfin/kernel/bfin_gpio.c | 19 +++++++++------
include/asm-blackfin/mach-bf537/portmux.h | 35 ++++++++++++++++++++++++++++-
2 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/arch/blackfin/kernel/bfin_gpio.c b/arch/blackfin/kernel/bfin_gpio.c
index 9f30948..5d488ef 100644
--- a/arch/blackfin/kernel/bfin_gpio.c
+++ b/arch/blackfin/kernel/bfin_gpio.c
@@ -180,11 +180,13 @@ static int cmp_label(unsigned short ident, const char *label)
#ifdef BF537_FAMILY
static void port_setup(unsigned short gpio, unsigned short usage)
{
- if (usage == GPIO_USAGE) {
- *port_fer[gpio_bank(gpio)] &= ~gpio_bit(gpio);
- } else
- *port_fer[gpio_bank(gpio)] |= gpio_bit(gpio);
- SSYNC();
+ if (!check_gpio(gpio)) {
+ if (usage == GPIO_USAGE) {
+ *port_fer[gpio_bank(gpio)] &= ~gpio_bit(gpio);
+ } else
+ *port_fer[gpio_bank(gpio)] |= gpio_bit(gpio);
+ SSYNC();
+ }
}
#else
# define port_setup(...) do { } while (0)
@@ -644,11 +646,10 @@ int peripheral_request(unsigned short per, const char *label)
if (!(per & P_DEFINED))
return -ENODEV;

- if (check_gpio(ident) < 0)
- return -EINVAL;
-
local_irq_save(flags);

+ if (!check_gpio(ident)) {
+
if (unlikely(reserved_gpio_map[gpio_bank(ident)] & gpio_bit(ident))) {
printk(KERN_ERR
"%s: Peripheral %d is already reserved as GPIO by %s !\n",
@@ -658,6 +659,8 @@ int peripheral_request(unsigned short per, const char *label)
return -EBUSY;
}

+ }
+
if (unlikely(reserved_peri_map[gpio_bank(ident)] & gpio_bit(ident))) {

/*
diff --git a/include/asm-blackfin/mach-bf537/portmux.h b/include/asm-blackfin/mach-bf537/portmux.h
index 7daa247..ae6c53b 100644
--- a/include/asm-blackfin/mach-bf537/portmux.h
+++ b/include/asm-blackfin/mach-bf537/portmux.h
@@ -106,4 +106,37 @@
#define P_SPI0_SSEL2 (P_DEFINED | P_IDENT(PORT_PJ11) | P_FUNCT(1))
#define P_SPI0_SSEL7 (P_DEFINED | P_IDENT(PORT_PJ5) | P_FUNCT(2))

-#endif /* _MACH_PORTMUX_H_ */
+#define P_MII0 {\
+ P_MII0_ETxD0, \
+ P_MII0_ETxD1, \
+ P_MII0_ETxD2, \
+ P_MII0_ETxD3, \
+ P_MII0_ETxEN, \
+ P_MII0_TxCLK, \
+ P_MII0_PHYINT, \
+ P_MII0_COL, \
+ P_MII0_ERxD0, \
+ P_MII0_ERxD1, \
+ P_MII0_ERxD2, \
+ P_MII0_ERxD3, \
+ P_MII0_ERxDV, \
+ P_MII0_ERxCLK, \
+ P_MII0_ERxER, \
+ P_MII0_CRS, \
+ P_MDC, \
+ P_MDIO, 0}
+
+
+#define P_RMII0 {\
+ P_MII0_ETxD0, \
+ P_MII0_ETxD1, \
+ P_MII0_ETxEN, \
+ P_MII0_ERxD0, \
+ P_MII0_ERxD1, \
+ P_MII0_ERxER, \
+ P_RMII0_REF_CLK, \
+ P_RMII0_MDINT, \
+ P_RMII0_CRS_DV, \
+ P_MDC, \
+ P_MDIO, 0}
+#endif /* _MACH_PORTMUX_H_ */
--
1.5.2

2007-08-08 03:36:33

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 07/12] Blackfin arch: bug fixing, add missing BF533_FAMILY GPIO_PFx definition

Signed-off-by: Bryan Wu <[email protected]>
---
include/asm-blackfin/gpio.h | 18 ++++++++++++++++++
1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/include/asm-blackfin/gpio.h b/include/asm-blackfin/gpio.h
index e714363..dd203cd 100644
--- a/include/asm-blackfin/gpio.h
+++ b/include/asm-blackfin/gpio.h
@@ -144,6 +144,24 @@

#ifdef BF533_FAMILY
#define MAX_BLACKFIN_GPIOS 16
+
+#define GPIO_PF0 0
+#define GPIO_PF1 1
+#define GPIO_PF2 2
+#define GPIO_PF3 3
+#define GPIO_PF4 4
+#define GPIO_PF5 5
+#define GPIO_PF6 6
+#define GPIO_PF7 7
+#define GPIO_PF8 8
+#define GPIO_PF9 9
+#define GPIO_PF10 10
+#define GPIO_PF11 11
+#define GPIO_PF12 12
+#define GPIO_PF13 13
+#define GPIO_PF14 14
+#define GPIO_PF15 15
+
#endif

#ifdef BF537_FAMILY
--
1.5.2

2007-08-08 03:36:56

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 08/12] Blackfin arch: add missing gpio error handling to make sure we roll back requests in case one fails

From: Michael Hennerich <[email protected]>

Signed-off-by: Michael Hennerich <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
arch/blackfin/kernel/bfin_gpio.c | 10 ++++++++--
arch/blackfin/mach-bf548/gpio.c | 11 +++++++++--
2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/blackfin/kernel/bfin_gpio.c b/arch/blackfin/kernel/bfin_gpio.c
index 5d488ef..f712772 100644
--- a/arch/blackfin/kernel/bfin_gpio.c
+++ b/arch/blackfin/kernel/bfin_gpio.c
@@ -711,9 +711,15 @@ int peripheral_request_list(unsigned short per[], const char *label)
int ret;

for (cnt = 0; per[cnt] != 0; cnt++) {
+
ret = peripheral_request(per[cnt], label);
- if (ret < 0)
- return ret;
+
+ if (ret < 0) {
+ for ( ; cnt > 0; cnt--) {
+ peripheral_free(per[cnt - 1]);
+ }
+ return ret;
+ }
}

return 0;
diff --git a/arch/blackfin/mach-bf548/gpio.c b/arch/blackfin/mach-bf548/gpio.c
index c073ab3..f3b9dea 100644
--- a/arch/blackfin/mach-bf548/gpio.c
+++ b/arch/blackfin/mach-bf548/gpio.c
@@ -212,11 +212,18 @@ int peripheral_request_list(unsigned short per[], const char *label)
int ret;

for (cnt = 0; per[cnt] != 0; cnt++) {
+
ret = peripheral_request(per[cnt], label);
- if (ret < 0)
- return ret;
+
+ if (ret < 0) {
+ for ( ; cnt > 0; cnt--) {
+ peripheral_free(per[cnt - 1]);
+ }
+ return ret;
+ }
}

+
return 0;
}
EXPORT_SYMBOL(peripheral_request_list);
--
1.5.2

2007-08-08 03:37:42

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 12/12] Blackfin serial driver: use new GPIO API

From: Michael Hennerich <[email protected]>

Signed-off-by: Michael Hennerich <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
include/asm-blackfin/mach-bf548/bfin_serial_5xx.h | 39 ++++++++++-----------
1 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/include/asm-blackfin/mach-bf548/bfin_serial_5xx.h b/include/asm-blackfin/mach-bf548/bfin_serial_5xx.h
index 2f4afc9..f21a162 100644
--- a/include/asm-blackfin/mach-bf548/bfin_serial_5xx.h
+++ b/include/asm-blackfin/mach-bf548/bfin_serial_5xx.h
@@ -1,5 +1,6 @@
#include <linux/serial.h>
#include <asm/dma.h>
+#include <asm/portmux.h>

#define NR_PORTS 4

@@ -143,50 +144,48 @@ struct bfin_serial_res bfin_serial_resource[] = {

int nr_ports = ARRAY_SIZE(bfin_serial_resource);

+#define DRIVER_NAME "bfin-uart"
+
static void bfin_serial_hw_init(struct bfin_serial_port *uart)
{
#ifdef CONFIG_SERIAL_BFIN_UART0
- /* Enable UART0 RX and TX on pin 7 & 8 of PORT E */
- bfin_write_PORTE_FER(0x180 | bfin_read_PORTE_FER());
- bfin_write_PORTE_MUX(0x3C000 | bfin_read_PORTE_MUX());
+ peripheral_request(P_UART0_TX, DRIVER_NAME);
+ peripheral_request(P_UART0_RX, DRIVER_NAME);
#endif

#ifdef CONFIG_SERIAL_BFIN_UART1
- /* Enable UART1 RX and TX on pin 0 & 1 of PORT H */
- bfin_write_PORTH_FER(0x3 | bfin_read_PORTH_FER());
- bfin_write_PORTH_MUX(~0xF & bfin_read_PORTH_MUX());
+ peripheral_request(P_UART1_TX, DRIVER_NAME);
+ peripheral_request(P_UART1_RX, DRIVER_NAME);
+
#ifdef CONFIG_BFIN_UART1_CTSRTS
- /* Enable UART1 RTS and CTS on pin 9 & 10 of PORT E */
- bfin_write_PORTE_FER(0x600 | bfin_read_PORTE_FER());
- bfin_write_PORTE_MUX(~0x3C0000 & bfin_read_PORTE_MUX());
+ peripheral_request(P_UART1_RTS, DRIVER_NAME);
+ peripheral_request(P_UART1_CTS DRIVER_NAME);
#endif
#endif

#ifdef CONFIG_SERIAL_BFIN_UART2
- /* Enable UART2 RX and TX on pin 4 & 5 of PORT B */
- bfin_write_PORTB_FER(0x30 | bfin_read_PORTB_FER());
- bfin_write_PORTB_MUX(~0xF00 & bfin_read_PORTB_MUX());
+ peripheral_request(P_UART2_TX, DRIVER_NAME);
+ peripheral_request(P_UART2_RX, DRIVER_NAME);
#endif

#ifdef CONFIG_SERIAL_BFIN_UART3
- /* Enable UART3 RX and TX on pin 6 & 7 of PORT B */
- bfin_write_PORTB_FER(0xC0 | bfin_read_PORTB_FER());
- bfin_write_PORTB_MUX(~0xF000 | bfin_read_PORTB_MUX());
+ peripheral_request(P_UART3_TX, DRIVER_NAME);
+ peripheral_request(P_UART3_RX, DRIVER_NAME);
+
#ifdef CONFIG_BFIN_UART3_CTSRTS
- /* Enable UART3 RTS and CTS on pin 2 & 3 of PORT B */
- bfin_write_PORTB_FER(0xC | bfin_read_PORTB_FER());
- bfin_write_PORTB_MUX(~0xF0 | bfin_read_PORTB_MUX());
+ peripheral_request(P_UART3_RTS, DRIVER_NAME);
+ peripheral_request(P_UART3_CTS DRIVER_NAME);
#endif
#endif
SSYNC();
#ifdef CONFIG_SERIAL_BFIN_CTSRTS
if (uart->cts_pin >= 0) {
- gpio_request(uart->cts_pin, NULL);
+ gpio_request(uart->cts_pin, DRIVER_NAME);
gpio_direction_input(uart->cts_pin);
}

if (uart->rts_pin >= 0) {
- gpio_request(uart->rts_pin, NULL);
+ gpio_request(uart->rts_pin, DRIVER_NAME);
gpio_direction_output(uart->rts_pin);
}
#endif
--
1.5.2

2007-08-08 03:38:40

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 01/12] Blackfin arch: add peripheral resource allocation support

From: Michael Hennerich <[email protected]>

Signed-off-by: Michael Hennerich <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
arch/blackfin/kernel/bfin_gpio.c | 272 ++++++++++++++++++---
include/asm-blackfin/mach-bf533/bfin_serial_5xx.h | 11 +-
include/asm-blackfin/mach-bf537/bfin_serial_5xx.h | 23 +-
include/asm-blackfin/mach-bf537/portmux.h | 2 +-
include/asm-blackfin/mach-bf561/bfin_serial_5xx.h | 11 +-
5 files changed, 274 insertions(+), 45 deletions(-)

diff --git a/arch/blackfin/kernel/bfin_gpio.c b/arch/blackfin/kernel/bfin_gpio.c
index bafcfa5..9f30948 100644
--- a/arch/blackfin/kernel/bfin_gpio.c
+++ b/arch/blackfin/kernel/bfin_gpio.c
@@ -84,6 +84,7 @@
#include <linux/err.h>
#include <asm/blackfin.h>
#include <asm/gpio.h>
+#include <asm/portmux.h>
#include <linux/irq.h>

#ifdef BF533_FAMILY
@@ -115,7 +116,11 @@ static struct gpio_port_t *gpio_bankb[gpio_bank(MAX_BLACKFIN_GPIOS)] = {
};
#endif

-static unsigned short reserved_map[gpio_bank(MAX_BLACKFIN_GPIOS)];
+static unsigned short reserved_gpio_map[gpio_bank(MAX_BLACKFIN_GPIOS)];
+static unsigned short reserved_peri_map[gpio_bank(MAX_BLACKFIN_GPIOS + 16)];
+char *str_ident = NULL;
+
+#define RESOURCE_LABEL_SIZE 16

#ifdef CONFIG_PM
static unsigned short wakeup_map[gpio_bank(MAX_BLACKFIN_GPIOS)];
@@ -143,13 +148,39 @@ inline int check_gpio(unsigned short gpio)
return 0;
}

+static void set_label(unsigned short ident, const char *label)
+{
+
+ if (label && str_ident) {
+ strncpy(str_ident + ident * RESOURCE_LABEL_SIZE, label,
+ RESOURCE_LABEL_SIZE);
+ str_ident[ident * RESOURCE_LABEL_SIZE +
+ RESOURCE_LABEL_SIZE - 1] = 0;
+ }
+}
+
+static char *get_label(unsigned short ident)
+{
+ if (!str_ident)
+ return "UNKNOWN";
+
+ return (str_ident[ident * RESOURCE_LABEL_SIZE] ?
+ (str_ident + ident * RESOURCE_LABEL_SIZE) : "UNKNOWN");
+}
+
+static int cmp_label(unsigned short ident, const char *label)
+{
+ if (label && str_ident)
+ return strncmp(str_ident + ident * RESOURCE_LABEL_SIZE,
+ label, strlen(label));
+ else
+ return -EINVAL;
+}
+
#ifdef BF537_FAMILY
static void port_setup(unsigned short gpio, unsigned short usage)
{
if (usage == GPIO_USAGE) {
- if (*port_fer[gpio_bank(gpio)] & gpio_bit(gpio))
- printk(KERN_WARNING "bfin-gpio: Possible Conflict with Peripheral "
- "usage and GPIO %d detected!\n", gpio);
*port_fer[gpio_bank(gpio)] &= ~gpio_bit(gpio);
} else
*port_fer[gpio_bank(gpio)] |= gpio_bit(gpio);
@@ -159,6 +190,56 @@ static void port_setup(unsigned short gpio, unsigned short usage)
# define port_setup(...) do { } while (0)
#endif

+#ifdef BF537_FAMILY
+
+#define PMUX_LUT_RES 0
+#define PMUX_LUT_OFFSET 1
+#define PMUX_LUT_ENTRIES 41
+#define PMUX_LUT_SIZE 2
+
+static unsigned short port_mux_lut[PMUX_LUT_ENTRIES][PMUX_LUT_SIZE] = {
+ {P_PPI0_D13, 11}, {P_PPI0_D14, 11}, {P_PPI0_D15, 11},
+ {P_SPORT1_TFS, 11}, {P_SPORT1_TSCLK, 11}, {P_SPORT1_DTPRI, 11},
+ {P_PPI0_D10, 10}, {P_PPI0_D11, 10}, {P_PPI0_D12, 10},
+ {P_SPORT1_RSCLK, 10}, {P_SPORT1_RFS, 10}, {P_SPORT1_DRPRI, 10},
+ {P_PPI0_D8, 9}, {P_PPI0_D9, 9}, {P_SPORT1_DRSEC, 9},
+ {P_SPORT1_DTSEC, 9}, {P_TMR2, 8}, {P_PPI0_FS3, 8}, {P_TMR3, 7},
+ {P_SPI0_SSEL4, 7}, {P_TMR4, 6}, {P_SPI0_SSEL5, 6}, {P_TMR5, 5},
+ {P_SPI0_SSEL6, 5}, {P_UART1_RX, 4}, {P_UART1_TX, 4}, {P_TMR6, 4},
+ {P_TMR7, 4}, {P_UART0_RX, 3}, {P_UART0_TX, 3}, {P_DMAR0, 3},
+ {P_DMAR1, 3}, {P_SPORT0_DTSEC, 1}, {P_SPORT0_DRSEC, 1},
+ {P_CAN0_RX, 1}, {P_CAN0_TX, 1}, {P_SPI0_SSEL7, 1},
+ {P_SPORT0_TFS, 0}, {P_SPORT0_DTPRI, 0}, {P_SPI0_SSEL2, 0},
+ {P_SPI0_SSEL3, 0}
+};
+
+static void portmux_setup(unsigned short per, unsigned short function)
+{
+ u16 y, muxreg, offset;
+
+ for (y = 0; y < PMUX_LUT_ENTRIES; y++) {
+ if (port_mux_lut[y][PMUX_LUT_RES] == per) {
+
+ /* SET PORTMUX REG */
+
+ offset = port_mux_lut[y][PMUX_LUT_OFFSET];
+ muxreg = bfin_read_PORT_MUX();
+
+ if (offset != 1) {
+ muxreg &= ~(1 << offset);
+ } else {
+ muxreg &= ~(3 << 1);
+ }
+
+ muxreg |= (function << offset);
+ bfin_write_PORT_MUX(muxreg);
+ }
+ }
+}
+
+#else
+# define portmux_setup(...) do { } while (0)
+#endif

static void default_gpio(unsigned short gpio)
{
@@ -179,22 +260,15 @@ static void default_gpio(unsigned short gpio)

static int __init bfin_gpio_init(void)
{
- int i;
-
- printk(KERN_INFO "Blackfin GPIO Controller\n");

- for (i = 0; i < MAX_BLACKFIN_GPIOS; i += GPIO_BANKSIZE)
- reserved_map[gpio_bank(i)] = 0;
+ str_ident = kzalloc(RESOURCE_LABEL_SIZE * 256, GFP_KERNEL);
+ if (!str_ident)
+ return -ENOMEM;

-#if defined(BF537_FAMILY) && (defined(CONFIG_BFIN_MAC) || defined(CONFIG_BFIN_MAC_MODULE))
-# if defined(CONFIG_BFIN_MAC_RMII)
- reserved_map[gpio_bank(PORT_H)] = 0xC373;
-# else
- reserved_map[gpio_bank(PORT_H)] = 0xFFFF;
-# endif
-#endif
+ printk(KERN_INFO "Blackfin GPIO Controller\n");

return 0;
+
}

arch_initcall(bfin_gpio_init);
@@ -223,7 +297,7 @@ arch_initcall(bfin_gpio_init);
void set_gpio_ ## name(unsigned short gpio, unsigned short arg) \
{ \
unsigned long flags; \
- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio))); \
+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio))); \
local_irq_save(flags); \
if (arg) \
gpio_bankb[gpio_bank(gpio)]->name |= gpio_bit(gpio); \
@@ -243,7 +317,7 @@ SET_GPIO(both)
#define SET_GPIO_SC(name) \
void set_gpio_ ## name(unsigned short gpio, unsigned short arg) \
{ \
- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio))); \
+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio))); \
if (arg) \
gpio_bankb[gpio_bank(gpio)]->name ## _set = gpio_bit(gpio); \
else \
@@ -258,7 +332,7 @@ SET_GPIO_SC(maskb)
void set_gpio_data(unsigned short gpio, unsigned short arg)
{
unsigned long flags;
- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
local_irq_save(flags);
if (arg)
gpio_bankb[gpio_bank(gpio)]->data_set = gpio_bit(gpio);
@@ -277,7 +351,7 @@ SET_GPIO_SC(data)
void set_gpio_toggle(unsigned short gpio)
{
unsigned long flags;
- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
local_irq_save(flags);
gpio_bankb[gpio_bank(gpio)]->toggle = gpio_bit(gpio);
bfin_read_CHIPID();
@@ -286,7 +360,7 @@ void set_gpio_toggle(unsigned short gpio)
#else
void set_gpio_toggle(unsigned short gpio)
{
- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
gpio_bankb[gpio_bank(gpio)]->toggle = gpio_bit(gpio);
}
#endif
@@ -350,7 +424,7 @@ unsigned short get_gpio_data(unsigned short gpio)
{
unsigned long flags;
unsigned short ret;
- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
local_irq_save(flags);
ret = 0x01 & (gpio_bankb[gpio_bank(gpio)]->data >> gpio_sub_n(gpio));
bfin_read_CHIPID();
@@ -494,13 +568,14 @@ u32 gpio_pm_setup(void)
gpio_bank_saved[bank].dir = gpio_bankb[bank]->dir;
gpio_bank_saved[bank].edge = gpio_bankb[bank]->edge;
gpio_bank_saved[bank].both = gpio_bankb[bank]->both;
- gpio_bank_saved[bank].reserved = reserved_map[bank];
+ gpio_bank_saved[bank].reserved =
+ reserved_gpio_map[bank];

gpio = i;

while (mask) {
if (mask & 1) {
- reserved_map[gpio_bank(gpio)] |=
+ reserved_gpio_map[gpio_bank(gpio)] |=
gpio_bit(gpio);
bfin_gpio_wakeup_type(gpio,
wakeup_flags_map[gpio]);
@@ -540,7 +615,8 @@ void gpio_pm_restore(void)
gpio_bankb[bank]->edge = gpio_bank_saved[bank].edge;
gpio_bankb[bank]->both = gpio_bank_saved[bank].both;

- reserved_map[bank] = gpio_bank_saved[bank].reserved;
+ reserved_gpio_map[bank] =
+ gpio_bank_saved[bank].reserved;

}

@@ -550,6 +626,140 @@ void gpio_pm_restore(void)

#endif

+
+
+
+int peripheral_request(unsigned short per, const char *label)
+{
+ unsigned long flags;
+ unsigned short ident = P_IDENT(per);
+
+ /*
+ * Don't cares are pins with only one dedicated function
+ */
+
+ if (per & P_DONTCARE)
+ return 0;
+
+ if (!(per & P_DEFINED))
+ return -ENODEV;
+
+ if (check_gpio(ident) < 0)
+ return -EINVAL;
+
+ local_irq_save(flags);
+
+ if (unlikely(reserved_gpio_map[gpio_bank(ident)] & gpio_bit(ident))) {
+ printk(KERN_ERR
+ "%s: Peripheral %d is already reserved as GPIO by %s !\n",
+ __FUNCTION__, ident, get_label(ident));
+ dump_stack();
+ local_irq_restore(flags);
+ return -EBUSY;
+ }
+
+ if (unlikely(reserved_peri_map[gpio_bank(ident)] & gpio_bit(ident))) {
+
+ /*
+ * Pin functions like AMC address strobes my
+ * be requested and used by several drivers
+ */
+
+ if (!(per & P_MAYSHARE)) {
+
+ /*
+ * Allow that the identical pin function can
+ * be requested from the same driver twice
+ */
+
+ if (cmp_label(ident, label) == 0)
+ goto anyway;
+
+ printk(KERN_ERR
+ "%s: Peripheral %d function %d is already"
+ "reserved by %s !\n",
+ __FUNCTION__, ident, P_FUNCT2MUX(per),
+ get_label(ident));
+ dump_stack();
+ local_irq_restore(flags);
+ return -EBUSY;
+ }
+
+ }
+
+anyway:
+
+
+ portmux_setup(per, P_FUNCT2MUX(per));
+
+ port_setup(ident, PERIPHERAL_USAGE);
+
+ reserved_peri_map[gpio_bank(ident)] |= gpio_bit(ident);
+ local_irq_restore(flags);
+ set_label(ident, label);
+
+ return 0;
+}
+EXPORT_SYMBOL(peripheral_request);
+
+int peripheral_request_list(unsigned short per[], const char *label)
+{
+ u16 cnt;
+ int ret;
+
+ for (cnt = 0; per[cnt] != 0; cnt++) {
+ ret = peripheral_request(per[cnt], label);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL(peripheral_request_list);
+
+void peripheral_free(unsigned short per)
+{
+ unsigned long flags;
+ unsigned short ident = P_IDENT(per);
+
+ if (per & P_DONTCARE)
+ return;
+
+ if (!(per & P_DEFINED))
+ return;
+
+ if (check_gpio(ident) < 0)
+ return;
+
+ local_irq_save(flags);
+
+ if (unlikely(!(reserved_peri_map[gpio_bank(ident)]
+ & gpio_bit(ident)))) {
+ local_irq_restore(flags);
+ return;
+ }
+
+ if (!(per & P_MAYSHARE)) {
+ port_setup(ident, GPIO_USAGE);
+ }
+
+ reserved_peri_map[gpio_bank(ident)] &= ~gpio_bit(ident);
+
+ local_irq_restore(flags);
+}
+EXPORT_SYMBOL(peripheral_free);
+
+void peripheral_free_list(unsigned short per[])
+{
+ u16 cnt;
+
+ for (cnt = 0; per[cnt] != 0; cnt++) {
+ peripheral_free(per[cnt]);
+ }
+
+}
+EXPORT_SYMBOL(peripheral_free_list);
+
/***********************************************************
*
* FUNCTIONS: Blackfin GPIO Driver
@@ -574,13 +784,13 @@ int gpio_request(unsigned short gpio, const char *label)

local_irq_save(flags);

- if (unlikely(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio))) {
+ if (unlikely(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio))) {
printk(KERN_ERR "bfin-gpio: GPIO %d is already reserved!\n", gpio);
dump_stack();
local_irq_restore(flags);
return -EBUSY;
}
- reserved_map[gpio_bank(gpio)] |= gpio_bit(gpio);
+ reserved_gpio_map[gpio_bank(gpio)] |= gpio_bit(gpio);

local_irq_restore(flags);

@@ -599,7 +809,7 @@ void gpio_free(unsigned short gpio)

local_irq_save(flags);

- if (unlikely(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)))) {
+ if (unlikely(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)))) {
printk(KERN_ERR "bfin-gpio: GPIO %d wasn't reserved!\n", gpio);
dump_stack();
local_irq_restore(flags);
@@ -608,7 +818,7 @@ void gpio_free(unsigned short gpio)

default_gpio(gpio);

- reserved_map[gpio_bank(gpio)] &= ~gpio_bit(gpio);
+ reserved_gpio_map[gpio_bank(gpio)] &= ~gpio_bit(gpio);

local_irq_restore(flags);
}
@@ -618,7 +828,7 @@ void gpio_direction_input(unsigned short gpio)
{
unsigned long flags;

- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));

local_irq_save(flags);
gpio_bankb[gpio_bank(gpio)]->dir &= ~gpio_bit(gpio);
@@ -631,7 +841,7 @@ void gpio_direction_output(unsigned short gpio)
{
unsigned long flags;

- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));

local_irq_save(flags);
gpio_bankb[gpio_bank(gpio)]->inen &= ~gpio_bit(gpio);
diff --git a/include/asm-blackfin/mach-bf533/bfin_serial_5xx.h b/include/asm-blackfin/mach-bf533/bfin_serial_5xx.h
index e043caf..69b9f8e 100644
--- a/include/asm-blackfin/mach-bf533/bfin_serial_5xx.h
+++ b/include/asm-blackfin/mach-bf533/bfin_serial_5xx.h
@@ -1,5 +1,6 @@
#include <linux/serial.h>
#include <asm/dma.h>
+#include <asm/portmux.h>

#define NR_PORTS 1

@@ -92,18 +93,24 @@ struct bfin_serial_res bfin_serial_resource[] = {
}
};

+#define DRIVER_NAME "bfin-uart"

int nr_ports = NR_PORTS;
static void bfin_serial_hw_init(struct bfin_serial_port *uart)
{

+#ifdef CONFIG_SERIAL_BFIN_UART0
+ peripheral_request(P_UART0_TX, DRIVER_NAME);
+ peripheral_request(P_UART0_RX, DRIVER_NAME);
+#endif
+
#ifdef CONFIG_SERIAL_BFIN_CTSRTS
if (uart->cts_pin >= 0) {
- gpio_request(uart->cts_pin, NULL);
+ gpio_request(uart->cts_pin, DRIVER_NAME);
gpio_direction_input(uart->cts_pin);
}
if (uart->rts_pin >= 0) {
- gpio_request(uart->rts_pin, NULL);
+ gpio_request(uart->rts_pin, DRIVER_NAME);
gpio_direction_input(uart->rts_pin);
}
#endif
diff --git a/include/asm-blackfin/mach-bf537/bfin_serial_5xx.h b/include/asm-blackfin/mach-bf537/bfin_serial_5xx.h
index 8f5d9c4..6fb328f 100644
--- a/include/asm-blackfin/mach-bf537/bfin_serial_5xx.h
+++ b/include/asm-blackfin/mach-bf537/bfin_serial_5xx.h
@@ -1,5 +1,6 @@
#include <linux/serial.h>
#include <asm/dma.h>
+#include <asm/portmux.h>

#define NR_PORTS 2

@@ -122,25 +123,29 @@ struct bfin_serial_res bfin_serial_resource[] = {

int nr_ports = ARRAY_SIZE(bfin_serial_resource);

+#define DRIVER_NAME "bfin-uart"
+
static void bfin_serial_hw_init(struct bfin_serial_port *uart)
{
- unsigned short val;
- val = bfin_read16(BFIN_PORT_MUX);
- val &= ~(PFDE | PFTE);
- bfin_write16(BFIN_PORT_MUX, val);

- val = bfin_read16(PORTF_FER);
- val |= 0xF;
- bfin_write16(PORTF_FER, val);
+#ifdef CONFIG_SERIAL_BFIN_UART0
+ peripheral_request(P_UART0_TX, DRIVER_NAME);
+ peripheral_request(P_UART0_RX, DRIVER_NAME);
+#endif
+
+#ifdef CONFIG_SERIAL_BFIN_UART1
+ peripheral_request(P_UART1_TX, DRIVER_NAME);
+ peripheral_request(P_UART1_RX, DRIVER_NAME);
+#endif

#ifdef CONFIG_SERIAL_BFIN_CTSRTS
if (uart->cts_pin >= 0) {
- gpio_request(uart->cts_pin, NULL);
+ gpio_request(uart->cts_pin, DRIVER_NAME);
gpio_direction_input(uart->cts_pin);
}

if (uart->rts_pin >= 0) {
- gpio_request(uart->rts_pin, NULL);
+ gpio_request(uart->rts_pin, DRIVER_NAME);
gpio_direction_output(uart->rts_pin);
}
#endif
diff --git a/include/asm-blackfin/mach-bf537/portmux.h b/include/asm-blackfin/mach-bf537/portmux.h
index 23e13c5..7daa247 100644
--- a/include/asm-blackfin/mach-bf537/portmux.h
+++ b/include/asm-blackfin/mach-bf537/portmux.h
@@ -106,4 +106,4 @@
#define P_SPI0_SSEL2 (P_DEFINED | P_IDENT(PORT_PJ11) | P_FUNCT(1))
#define P_SPI0_SSEL7 (P_DEFINED | P_IDENT(PORT_PJ5) | P_FUNCT(2))

-#endif /* _MACH_PORTMUX_H_ */
+#endif /* _MACH_PORTMUX_H_ */
diff --git a/include/asm-blackfin/mach-bf561/bfin_serial_5xx.h b/include/asm-blackfin/mach-bf561/bfin_serial_5xx.h
index e043caf..69b9f8e 100644
--- a/include/asm-blackfin/mach-bf561/bfin_serial_5xx.h
+++ b/include/asm-blackfin/mach-bf561/bfin_serial_5xx.h
@@ -1,5 +1,6 @@
#include <linux/serial.h>
#include <asm/dma.h>
+#include <asm/portmux.h>

#define NR_PORTS 1

@@ -92,18 +93,24 @@ struct bfin_serial_res bfin_serial_resource[] = {
}
};

+#define DRIVER_NAME "bfin-uart"

int nr_ports = NR_PORTS;
static void bfin_serial_hw_init(struct bfin_serial_port *uart)
{

+#ifdef CONFIG_SERIAL_BFIN_UART0
+ peripheral_request(P_UART0_TX, DRIVER_NAME);
+ peripheral_request(P_UART0_RX, DRIVER_NAME);
+#endif
+
#ifdef CONFIG_SERIAL_BFIN_CTSRTS
if (uart->cts_pin >= 0) {
- gpio_request(uart->cts_pin, NULL);
+ gpio_request(uart->cts_pin, DRIVER_NAME);
gpio_direction_input(uart->cts_pin);
}
if (uart->rts_pin >= 0) {
- gpio_request(uart->rts_pin, NULL);
+ gpio_request(uart->rts_pin, DRIVER_NAME);
gpio_direction_input(uart->rts_pin);
}
#endif
--
1.5.2

2007-08-08 03:39:04

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

From: Michael Hennerich <[email protected]>

Signed-off-by: Michael Hennerich <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
arch/blackfin/mach-common/ints-priority-dc.c | 4 ++--
arch/blackfin/mach-common/ints-priority-sc.c | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/blackfin/mach-common/ints-priority-dc.c b/arch/blackfin/mach-common/ints-priority-dc.c
index 660f881..d5d9e57 100644
--- a/arch/blackfin/mach-common/ints-priority-dc.c
+++ b/arch/blackfin/mach-common/ints-priority-dc.c
@@ -221,7 +221,7 @@ static unsigned int bf561_gpio_irq_startup(unsigned int irq)

if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {

- ret = gpio_request(gpionr, NULL);
+ ret = gpio_request(gpionr, "IRQ");
if (ret)
return ret;

@@ -261,7 +261,7 @@ static int bf561_gpio_irq_type(unsigned int irq, unsigned int type)

if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {

- ret = gpio_request(gpionr, NULL);
+ ret = gpio_request(gpionr, "IRQ");
if (ret)
return ret;

diff --git a/arch/blackfin/mach-common/ints-priority-sc.c b/arch/blackfin/mach-common/ints-priority-sc.c
index 4708023..505b948 100644
--- a/arch/blackfin/mach-common/ints-priority-sc.c
+++ b/arch/blackfin/mach-common/ints-priority-sc.c
@@ -343,7 +343,7 @@ static unsigned int bfin_gpio_irq_startup(unsigned int irq)
u16 gpionr = irq - IRQ_PF0;

if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
- ret = gpio_request(gpionr, NULL);
+ ret = gpio_request(gpionr, "IRQ");
if (ret)
return ret;
}
@@ -377,7 +377,7 @@ static int bfin_gpio_irq_type(unsigned int irq, unsigned int type)
if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) {
if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
- ret = gpio_request(gpionr, NULL);
+ ret = gpio_request(gpionr, "IRQ");
if (ret)
return ret;
}
@@ -587,7 +587,7 @@ static unsigned int bfin_gpio_irq_startup(unsigned int irq)
}

if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
- ret = gpio_request(gpionr, NULL);
+ ret = gpio_request(gpionr, "IRQ");
if (ret)
return ret;
}
@@ -627,7 +627,7 @@ static int bfin_gpio_irq_type(unsigned int irq, unsigned int type)
if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) {
if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
- ret = gpio_request(gpionr, NULL);
+ ret = gpio_request(gpionr, "IRQ");
if (ret)
return ret;
}
--
1.5.2

2007-08-08 03:39:37

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 06/12] Blackfin arch: Add PORT_J.High (needed for BF548-EZkit Touchscreen interrupts) - remove PORT_C.H

From: Michael Hennerich <[email protected]>

Signed-off-by: Michael Hennerich <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
arch/blackfin/mach-bf548/Kconfig | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/blackfin/mach-bf548/Kconfig b/arch/blackfin/mach-bf548/Kconfig
index e78b03d..3976b7f 100644
--- a/arch/blackfin/mach-bf548/Kconfig
+++ b/arch/blackfin/mach-bf548/Kconfig
@@ -282,7 +282,7 @@ menu "Assignment"

config PINTx_REASSIGN
bool "Reprogram PINT Assignment"
- default n
+ default y
help
The interrupt assignment registers controls the pin-to-interrupt
assignment in a byte-wide manner. Each option allows you to select
@@ -303,7 +303,7 @@ config PINT1_ASSIGN
config PINT2_ASSIGN
hex "PINT2_ASSIGN"
depends on PINTx_REASSIGN
- default 0x00000101
+ default 0x07000101
config PINT3_ASSIGN
hex "PINT3_ASSIGN"
depends on PINTx_REASSIGN
--
1.5.2

2007-08-08 03:39:58

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 05/12] Blackfin arch: Advertise GENERIC_GPIO and remove duplicated GENERIC_CALIBRATE_DELAY

From: Michael Hennerich <[email protected]>

Signed-off-by: Michael Hennerich <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
arch/blackfin/Kconfig | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/blackfin/Kconfig b/arch/blackfin/Kconfig
index 017defa..5c1e215 100644
--- a/arch/blackfin/Kconfig
+++ b/arch/blackfin/Kconfig
@@ -57,7 +57,7 @@ config GENERIC_TIME
bool
default n

-config GENERIC_CALIBRATE_DELAY
+config GENERIC_GPIO
bool
default y

--
1.5.2

2007-08-08 03:40:49

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 11/12] Blackfin arch: after removing fs.h from mm.h, fix the broken on Blackfin arch.

Cc: Alexey Dobriyan <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
arch/blackfin/kernel/init_task.c | 1 +
arch/blackfin/kernel/process.c | 2 ++
arch/blackfin/kernel/sys_bfin.c | 1 +
arch/blackfin/kernel/traps.c | 1 +
4 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/blackfin/kernel/init_task.c b/arch/blackfin/kernel/init_task.c
index b45188f..673c860 100644
--- a/arch/blackfin/kernel/init_task.c
+++ b/arch/blackfin/kernel/init_task.c
@@ -31,6 +31,7 @@
#include <linux/module.h>
#include <linux/init_task.h>
#include <linux/mqueue.h>
+#include <linux/fs.h>

static struct fs_struct init_fs = INIT_FS;
static struct files_struct init_files = INIT_FILES;
diff --git a/arch/blackfin/kernel/process.c b/arch/blackfin/kernel/process.c
index 5a51dd6..6a7aefe 100644
--- a/arch/blackfin/kernel/process.c
+++ b/arch/blackfin/kernel/process.c
@@ -33,6 +33,8 @@
#include <linux/user.h>
#include <linux/a.out.h>
#include <linux/uaccess.h>
+#include <linux/fs.h>
+#include <linux/err.h>

#include <asm/blackfin.h>
#include <asm/fixed_code.h>
diff --git a/arch/blackfin/kernel/sys_bfin.c b/arch/blackfin/kernel/sys_bfin.c
index f5e1ae3..abcd148 100644
--- a/arch/blackfin/kernel/sys_bfin.c
+++ b/arch/blackfin/kernel/sys_bfin.c
@@ -37,6 +37,7 @@
#include <linux/syscalls.h>
#include <linux/mman.h>
#include <linux/file.h>
+#include <linux/fs.h>
#include <linux/uaccess.h>
#include <linux/ipc.h>
#include <linux/unistd.h>
diff --git a/arch/blackfin/kernel/traps.c b/arch/blackfin/kernel/traps.c
index 8766bd6..792a841 100644
--- a/arch/blackfin/kernel/traps.c
+++ b/arch/blackfin/kernel/traps.c
@@ -31,6 +31,7 @@
#include <linux/interrupt.h>
#include <linux/module.h>
#include <linux/kallsyms.h>
+#include <linux/fs.h>
#include <asm/traps.h>
#include <asm/cacheflush.h>
#include <asm/blackfin.h>
--
1.5.2

2007-08-08 03:41:24

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 09/12] Blackfin arch: scrub remaining ASSEMBLY usage since the switch to __ASSEMBLY__

From: Mike Frysinger <[email protected]>

Signed-off-by: Mike Frysinger <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
include/asm-blackfin/mach-bf533/blackfin.h | 2 +-
include/asm-blackfin/mach-bf537/blackfin.h | 2 +-
include/asm-blackfin/mach-bf548/blackfin.h | 2 +-
include/asm-blackfin/mach-bf561/blackfin.h | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/asm-blackfin/mach-bf533/blackfin.h b/include/asm-blackfin/mach-bf533/blackfin.h
index e438449..f3b240a 100644
--- a/include/asm-blackfin/mach-bf533/blackfin.h
+++ b/include/asm-blackfin/mach-bf533/blackfin.h
@@ -38,7 +38,7 @@
#include "defBF532.h"
#include "anomaly.h"

-#if !(defined(__ASSEMBLY__) || defined(ASSEMBLY))
+#if !defined(__ASSEMBLY__)
#include "cdefBF532.h"
#endif

diff --git a/include/asm-blackfin/mach-bf537/blackfin.h b/include/asm-blackfin/mach-bf537/blackfin.h
index bbd9705..f196588 100644
--- a/include/asm-blackfin/mach-bf537/blackfin.h
+++ b/include/asm-blackfin/mach-bf537/blackfin.h
@@ -43,7 +43,7 @@
#include "defBF537.h"
#endif

-#if !(defined(__ASSEMBLY__) || defined(ASSEMBLY))
+#if !defined(__ASSEMBLY__)
#include "cdefBF534.h"

/* UART 0*/
diff --git a/include/asm-blackfin/mach-bf548/blackfin.h b/include/asm-blackfin/mach-bf548/blackfin.h
index 791218f..19e84dd 100644
--- a/include/asm-blackfin/mach-bf548/blackfin.h
+++ b/include/asm-blackfin/mach-bf548/blackfin.h
@@ -54,7 +54,7 @@
#include "defBF549.h"
#endif

-#if !(defined(__ASSEMBLY__) || defined(ASSEMBLY))
+#if !defined(__ASSEMBLY__)
#ifdef CONFIG_BF542
#include "cdefBF542.h"
#endif
diff --git a/include/asm-blackfin/mach-bf561/blackfin.h b/include/asm-blackfin/mach-bf561/blackfin.h
index 2537c84..562aee3 100644
--- a/include/asm-blackfin/mach-bf561/blackfin.h
+++ b/include/asm-blackfin/mach-bf561/blackfin.h
@@ -38,7 +38,7 @@
#include "defBF561.h"
#include "anomaly.h"

-#if !(defined(__ASSEMBLY__) || defined(ASSEMBLY))
+#if !defined(__ASSEMBLY__)
#include "cdefBF561.h"
#endif

--
1.5.2

2007-08-08 03:41:50

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 04/12] Blackfin arch: Finalize the generic gpio support - add gpio_to_irq and irq_to_gpio

From: Michael Hennerich <[email protected]>

Signed-off-by: Michael Hennerich <[email protected]>
Signed-off-by: Bryan Wu <[email protected]>
---
include/asm-blackfin/gpio.h | 13 +++++++++++++
include/asm-blackfin/mach-bf533/irq.h | 2 ++
include/asm-blackfin/mach-bf537/irq.h | 2 ++
include/asm-blackfin/mach-bf548/irq.h | 2 ++
include/asm-blackfin/mach-bf561/irq.h | 2 ++
5 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/include/asm-blackfin/gpio.h b/include/asm-blackfin/gpio.h
index 7480cfa..e714363 100644
--- a/include/asm-blackfin/gpio.h
+++ b/include/asm-blackfin/gpio.h
@@ -421,6 +421,19 @@ unsigned short gpio_get_value(unsigned short gpio);
void gpio_direction_input(unsigned short gpio);
void gpio_direction_output(unsigned short gpio);

+#include <asm-generic/gpio.h> /* cansleep wrappers */
+#include <asm/irq.h>
+
+static inline int gpio_to_irq(unsigned gpio)
+{
+ return (gpio + GPIO_IRQ_BASE);
+}
+
+static inline int irq_to_gpio(unsigned irq)
+{
+ return (irq - GPIO_IRQ_BASE);
+}
+
#endif /* __ASSEMBLY__ */

#endif /* __ARCH_BLACKFIN_GPIO_H__ */
diff --git a/include/asm-blackfin/mach-bf533/irq.h b/include/asm-blackfin/mach-bf533/irq.h
index 9879e68..452fb82 100644
--- a/include/asm-blackfin/mach-bf533/irq.h
+++ b/include/asm-blackfin/mach-bf533/irq.h
@@ -128,6 +128,8 @@ Core Emulation **
#define IRQ_PF14 47
#define IRQ_PF15 48

+#define GPIO_IRQ_BASE IRQ_PF0
+
#ifdef CONFIG_IRQCHIP_DEMUX_GPIO
#define NR_IRQS (IRQ_PF15+1)
#else
diff --git a/include/asm-blackfin/mach-bf537/irq.h b/include/asm-blackfin/mach-bf537/irq.h
index 8af2a83..36c44bc 100644
--- a/include/asm-blackfin/mach-bf537/irq.h
+++ b/include/asm-blackfin/mach-bf537/irq.h
@@ -160,6 +160,8 @@ Core Emulation **
#define IRQ_PH14 96
#define IRQ_PH15 97

+#define GPIO_IRQ_BASE IRQ_PF0
+
#ifdef CONFIG_IRQCHIP_DEMUX_GPIO
#define NR_IRQS (IRQ_PH15+1)
#else
diff --git a/include/asm-blackfin/mach-bf548/irq.h b/include/asm-blackfin/mach-bf548/irq.h
index e548d3c..21f06f7 100644
--- a/include/asm-blackfin/mach-bf548/irq.h
+++ b/include/asm-blackfin/mach-bf548/irq.h
@@ -337,6 +337,8 @@ Events (highest priority) EMU 0
#define IRQ_PJ14 BFIN_PJ_IRQ(14) /* N/A */
#define IRQ_PJ15 BFIN_PJ_IRQ(15) /* N/A */

+#define GPIO_IRQ_BASE IRQ_PA0
+
#ifdef CONFIG_IRQCHIP_DEMUX_GPIO
#define NR_IRQS (IRQ_PJ15+1)
#else
diff --git a/include/asm-blackfin/mach-bf561/irq.h b/include/asm-blackfin/mach-bf561/irq.h
index a753ce7..1278992 100644
--- a/include/asm-blackfin/mach-bf561/irq.h
+++ b/include/asm-blackfin/mach-bf561/irq.h
@@ -289,6 +289,8 @@
#define IRQ_PF46 119
#define IRQ_PF47 120

+#define GPIO_IRQ_BASE IRQ_PF0
+
#ifdef CONFIG_IRQCHIP_DEMUX_GPIO
#define NR_IRQS (IRQ_PF47 + 1)
#else
--
1.5.2

2007-08-08 03:42:25

by Bryan Wu

[permalink] [raw]
Subject: [PATCH 10/12] Blackfin arch: update platform driver resource information to the ezkitBF548 board file

Signed-off-by: Bryan Wu <[email protected]>
---
arch/blackfin/mach-bf548/boards/ezkit.c | 367 ++++++++++++++++++++++++++++++-
1 files changed, 366 insertions(+), 1 deletions(-)

diff --git a/arch/blackfin/mach-bf548/boards/ezkit.c b/arch/blackfin/mach-bf548/boards/ezkit.c
index 96ad95f..59e64c5 100644
--- a/arch/blackfin/mach-bf548/boards/ezkit.c
+++ b/arch/blackfin/mach-bf548/boards/ezkit.c
@@ -35,9 +35,13 @@
#include <linux/spi/spi.h>
#include <linux/spi/flash.h>
#include <linux/irq.h>
-#include <linux/irq.h>
#include <linux/interrupt.h>
#include <asm/bfin5xx_spi.h>
+#include <asm/dma.h>
+#include <asm/gpio.h>
+#include <asm/mach/nand.h>
+#include <asm/mach/bf54x_keys.h>
+#include <linux/spi/ad7877.h>

/*
* Name the Board for the /proc/cpuinfo
@@ -48,6 +52,87 @@ char *bfin_board_name = "ADSP-BF548-EZKIT";
* Driver needs to know address, irq and flag pin.
*/

+#if defined(CONFIG_FB_BF54X_LQ043) || defined(CONFIG_FB_BF54X_LQ043_MODULE)
+
+#include <asm/mach/bf54x-lq043.h>
+
+static struct bfin_bf54xfb_mach_info bf54x_lq043_data = {
+ .width = 480,
+ .height = 272,
+ .xres = {480, 480, 480},
+ .yres = {272, 272, 272},
+ .bpp = {24, 24, 24},
+ .disp = GPIO_PE3,
+};
+
+static struct resource bf54x_lq043_resources[] = {
+ {
+ .start = IRQ_EPPI0_ERR,
+ .end = IRQ_EPPI0_ERR,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct platform_device bf54x_lq043_device = {
+ .name = "bf54x-lq043",
+ .id = -1,
+ .num_resources = ARRAY_SIZE(bf54x_lq043_resources),
+ .resource = bf54x_lq043_resources,
+ .dev = {
+ .platform_data = &bf54x_lq043_data,
+ },
+};
+#endif
+
+#if defined(CONFIG_KEYBOARD_BFIN) || defined(CONFIG_KEYBOARD_BFIN_MODULE)
+static int bf548_keymap[] = {
+ KEYVAL(0, 0, KEY_ENTER),
+ KEYVAL(0, 1, KEY_HELP),
+ KEYVAL(0, 2, KEY_0),
+ KEYVAL(0, 3, KEY_BACKSPACE),
+ KEYVAL(1, 0, KEY_TAB),
+ KEYVAL(1, 1, KEY_9),
+ KEYVAL(1, 2, KEY_8),
+ KEYVAL(1, 3, KEY_7),
+ KEYVAL(2, 0, KEY_DOWN),
+ KEYVAL(2, 1, KEY_6),
+ KEYVAL(2, 2, KEY_5),
+ KEYVAL(2, 3, KEY_4),
+ KEYVAL(3, 0, KEY_UP),
+ KEYVAL(3, 1, KEY_3),
+ KEYVAL(3, 2, KEY_2),
+ KEYVAL(3, 3, KEY_1),
+};
+
+static struct bfin_kpad_platform_data bf54x_kpad_data = {
+ .rows = 4,
+ .cols = 4,
+ .keymap = bf548_keymap,
+ .keymapsize = ARRAY_SIZE(bf548_keymap),
+ .debounce_time = 5000, /* ns (5ms) */
+ .coldrive_time = 1000, /* ns (1ms) */
+ .keyup_test_interval = 50, /* ms (50ms) */
+};
+
+static struct resource bf54x_kpad_resources[] = {
+ {
+ .start = IRQ_KEY,
+ .end = IRQ_KEY,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct platform_device bf54x_kpad_device = {
+ .name = "bf54x-keys",
+ .id = -1,
+ .num_resources = ARRAY_SIZE(bf54x_kpad_resources),
+ .resource = bf54x_kpad_resources,
+ .dev = {
+ .platform_data = &bf54x_kpad_data,
+ },
+};
+#endif
+
#if defined(CONFIG_RTC_DRV_BFIN) || defined(CONFIG_RTC_DRV_BFIN_MODULE)
static struct platform_device rtc_device = {
.name = "rtc-bfin",
@@ -94,6 +179,248 @@ static struct platform_device bfin_uart_device = {
};
#endif

+#if defined(CONFIG_SMSC911X) || defined(CONFIG_SMSC911X_MODULE)
+static struct resource smsc911x_resources[] = {
+ {
+ .name = "smsc911x-memory",
+ .start = 0x24000000,
+ .end = 0x24000000 + 0xFF,
+ .flags = IORESOURCE_MEM,
+ },
+ {
+ .start = IRQ_PE8,
+ .end = IRQ_PE8,
+ .flags = IORESOURCE_IRQ | IORESOURCE_IRQ_LOWLEVEL,
+ },
+};
+static struct platform_device smsc911x_device = {
+ .name = "smsc911x",
+ .id = 0,
+ .num_resources = ARRAY_SIZE(smsc911x_resources),
+ .resource = smsc911x_resources,
+};
+#endif
+
+#if defined(CONFIG_USB_BF54x_HCD) || defined(CONFIG_USB_BF54x_HCD_MODULE)
+static struct resource bf54x_hcd_resources[] = {
+ {
+ .start = 0xFFC03C00,
+ .end = 0xFFC040FF,
+ .flags = IORESOURCE_MEM,
+ },
+};
+
+static struct platform_device bf54x_hcd = {
+ .name = "bf54x-hcd",
+ .id = 0,
+ .num_resources = ARRAY_SIZE(bf54x_hcd_resources),
+ .resource = bf54x_hcd_resources,
+};
+#endif
+
+#if defined(CONFIG_PATA_BF54X) || defined(CONFIG_PATA_BF54X_MODULE)
+static struct resource bfin_atapi_resources[] = {
+ {
+ .start = 0xFFC03800,
+ .end = 0xFFC0386F,
+ .flags = IORESOURCE_MEM,
+ },
+ {
+ .start = IRQ_ATAPI_ERR,
+ .end = IRQ_ATAPI_ERR,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct platform_device bfin_atapi_device = {
+ .name = "bf54x-atapi",
+ .id = -1,
+ .num_resources = ARRAY_SIZE(bfin_atapi_resources),
+ .resource = bfin_atapi_resources,
+};
+#endif
+
+#if defined(CONFIG_MTD_NAND_BF54X) || defined(CONFIG_MTD_NAND_BF54X_MODULE)
+static struct mtd_partition partition_info[] = {
+ {
+ .name = "Linux Kernel",
+ .offset = 0,
+ .size = 4 * SIZE_1M,
+ },
+ {
+ .name = "File System",
+ .offset = 4 * SIZE_1M,
+ .size = (256 - 4) * SIZE_1M,
+ },
+};
+
+static struct bf54x_nand_platform bf54x_nand_platform = {
+ .page_size = NFC_PG_SIZE_256,
+ .data_width = NFC_NWIDTH_8,
+ .partitions = partition_info,
+ .nr_partitions = ARRAY_SIZE(partition_info),
+ .rd_dly = 3,
+ .wr_dly = 3,
+};
+
+static struct resource bf54x_nand_resources[] = {
+ {
+ .start = 0xFFC03B00,
+ .end = 0xFFC03B4F,
+ .flags = IORESOURCE_MEM,
+ },
+ {
+ .start = CH_NFC,
+ .end = CH_NFC,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct platform_device bf54x_nand_device = {
+ .name = "bf54x-nand",
+ .id = 0,
+ .num_resources = ARRAY_SIZE(bf54x_nand_resources),
+ .resource = bf54x_nand_resources,
+ .dev = {
+ .platform_data = &bf54x_nand_platform,
+ },
+};
+#endif
+
+#if defined(CONFIG_SPI_BFIN) || defined(CONFIG_SPI_BFIN_MODULE)
+/* all SPI peripherals info goes here */
+#if defined(CONFIG_MTD_M25P80) \
+ || defined(CONFIG_MTD_M25P80_MODULE)
+/* SPI flash chip (m25p16) */
+static struct mtd_partition bfin_spi_flash_partitions[] = {
+ {
+ .name = "bootloader",
+ .size = 0x00030000,
+ .offset = 0,
+ .mask_flags = MTD_CAP_ROM
+ }, {
+ .name = "linux kernel",
+ .size = 0x1d0000,
+ .offset = 0x30000
+ }
+};
+
+static struct flash_platform_data bfin_spi_flash_data = {
+ .name = "m25p80",
+ .parts = bfin_spi_flash_partitions,
+ .nr_parts = ARRAY_SIZE(bfin_spi_flash_partitions),
+ .type = "m25p16",
+};
+
+static struct bfin5xx_spi_chip spi_flash_chip_info = {
+ .enable_dma = 0, /* use dma transfer with this chip*/
+ .bits_per_word = 8,
+ .cs_change_per_word = 0,
+};
+#endif
+
+#if defined(CONFIG_TOUCHSCREEN_AD7877) || defined(CONFIG_TOUCHSCREEN_AD7877_MODULE)
+static struct bfin5xx_spi_chip spi_ad7877_chip_info = {
+ .cs_change_per_word = 1,
+ .enable_dma = 0,
+ .bits_per_word = 16,
+};
+
+static const struct ad7877_platform_data bfin_ad7877_ts_info = {
+ .model = 7877,
+ .vref_delay_usecs = 50, /* internal, no capacitor */
+ .x_plate_ohms = 419,
+ .y_plate_ohms = 486,
+ .pressure_max = 1000,
+ .pressure_min = 0,
+ .stopacq_polarity = 1,
+ .first_conversion_delay = 3,
+ .acquisition_time = 1,
+ .averaging = 1,
+ .pen_down_acc_interval = 1,
+};
+#endif
+
+static struct spi_board_info bf54x_spi_board_info[] __initdata = {
+#if defined(CONFIG_MTD_M25P80) \
+ || defined(CONFIG_MTD_M25P80_MODULE)
+ {
+ /* the modalias must be the same as spi device driver name */
+ .modalias = "m25p80", /* Name of spi_driver for this device */
+ .max_speed_hz = 25000000, /* max spi clock (SCK) speed in HZ */
+ .bus_num = 0, /* Framework bus number */
+ .chip_select = 1, /* SPI_SSEL1*/
+ .platform_data = &bfin_spi_flash_data,
+ .controller_data = &spi_flash_chip_info,
+ .mode = SPI_MODE_3,
+ },
+#endif
+#if defined(CONFIG_TOUCHSCREEN_AD7877) || defined(CONFIG_TOUCHSCREEN_AD7877_MODULE)
+{
+ .modalias = "ad7877",
+ .platform_data = &bfin_ad7877_ts_info,
+ .irq = IRQ_PJ11,
+ .max_speed_hz = 12500000, /* max spi clock (SCK) speed in HZ */
+ .bus_num = 1,
+ .chip_select = 2,
+ .controller_data = &spi_ad7877_chip_info,
+},
+#endif
+};
+
+/* SPI (0) */
+static struct resource bfin_spi0_resource[] = {
+ [0] = {
+ .start = SPI0_REGBASE,
+ .end = SPI0_REGBASE + 0xFF,
+ .flags = IORESOURCE_MEM,
+ },
+ [1] = {
+ .start = CH_SPI0,
+ .end = CH_SPI0,
+ .flags = IORESOURCE_IRQ,
+ }
+};
+
+/* SPI controller data */
+static struct bfin5xx_spi_master bf54x_spi_master_info = {
+ .num_chipselect = 8,
+ .enable_dma = 1, /* master has the ability to do dma transfer */
+};
+
+static struct platform_device bf54x_spi_master_device = {
+ .name = "bfin-spi",
+ .id = 0, /* Bus number */
+ .num_resources = ARRAY_SIZE(bfin_spi0_resource),
+ .resource = bfin_spi0_resource,
+ .dev = {
+ .platform_data = &bf54x_spi_master_info, /* Passed to driver */
+ },
+};
+#endif /* spi master and devices */
+
+#if defined(CONFIG_I2C_BLACKFIN_TWI) || defined(CONFIG_I2C_BLACKFIN_TWI_MODULE)
+static struct resource bfin_twi0_resource[] = {
+ [0] = {
+ .start = 0xFFC01400,
+ .end = 0xFFC014FF,
+ .flags = IORESOURCE_MEM,
+ },
+ [1] = {
+ .start = IRQ_TWI0,
+ .end = IRQ_TWI0,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct platform_device i2c_bfin_twi0_device = {
+ .name = "i2c-bfin-twi",
+ .id = 0,
+ .num_resources = ARRAY_SIZE(bfin_twi0_resource),
+ .resource = bfin_twi0_resource,
+};
+#endif
+
static struct platform_device *ezkit_devices[] __initdata = {
#if defined(CONFIG_RTC_DRV_BFIN) || defined(CONFIG_RTC_DRV_BFIN_MODULE)
&rtc_device,
@@ -102,12 +429,50 @@ static struct platform_device *ezkit_devices[] __initdata = {
#if defined(CONFIG_SERIAL_BFIN) || defined(CONFIG_SERIAL_BFIN_MODULE)
&bfin_uart_device,
#endif
+
+#if defined(CONFIG_FB_BF54X_LQ043) || defined(CONFIG_FB_BF54X_LQ043_MODULE)
+ &bf54x_lq043_device,
+#endif
+
+#if defined(CONFIG_SMSC911X) || defined(CONFIG_SMSC911X_MODULE)
+ &smsc911x_device,
+#endif
+
+#if defined(CONFIG_USB_BF54x_HCD) || defined(CONFIG_USB_BF54x_HCD_MODULE)
+ &bf54x_hcd,
+#endif
+
+#if defined(CONFIG_PATA_BF54X) || defined(CONFIG_PATA_BF54X_MODULE)
+ &bfin_atapi_device,
+#endif
+
+#if defined(CONFIG_MTD_NAND_BF54X) || defined(CONFIG_MTD_NAND_BF54X_MODULE)
+ &bf54x_nand_device,
+#endif
+
+#if defined(CONFIG_SPI_BFIN) || defined(CONFIG_SPI_BFIN_MODULE)
+ &bf54x_spi_master_device,
+#endif
+
+#if defined(CONFIG_KEYBOARD_BFIN) || defined(CONFIG_KEYBOARD_BFIN_MODULE)
+ &bf54x_kpad_device,
+#endif
+
+#if defined(CONFIG_I2C_BLACKFIN_TWI) || defined(CONFIG_I2C_BLACKFIN_TWI_MODULE)
+ &i2c_bfin_twi0_device,
+#endif
};

static int __init stamp_init(void)
{
printk(KERN_INFO "%s(): registering device resources\n", __FUNCTION__);
platform_add_devices(ezkit_devices, ARRAY_SIZE(ezkit_devices));
+
+#if defined(CONFIG_SPI_BFIN) || defined(CONFIG_SPI_BFIN_MODULE)
+ spi_register_board_info(bf54x_spi_board_info,
+ ARRAY_SIZE(bf54x_spi_board_info));
+#endif
+
return 0;
}

--
1.5.2

2007-08-08 07:20:27

by Hennerich, Michael

[permalink] [raw]
Subject: RE: [PATCH 01/12] Blackfin arch: add peripheral resource allocation support

Bryan,

This patch doesn't seem to be up to date.
It doesn't include the changes made based on feedback from Joe Perches.

Please see our SVN:
Modified: trunk/arch/blackfin/kernel/bfin_gpio.c (3489 => 3490)

-Michael

>-----Original Message-----
>From: Bryan Wu [mailto:[email protected]]
>Sent: Mittwoch, 8. August 2007 05:35
>To: [email protected]; [email protected];
>[email protected]
>Cc: [email protected]; Michael Hennerich; Bryan Wu
>Subject: [PATCH 01/12] Blackfin arch: add peripheral resource
allocation
>support
>
>From: Michael Hennerich <[email protected]>
>
>Signed-off-by: Michael Hennerich <[email protected]>
>Signed-off-by: Bryan Wu <[email protected]>
>---
> arch/blackfin/kernel/bfin_gpio.c | 272
>++++++++++++++++++---
> include/asm-blackfin/mach-bf533/bfin_serial_5xx.h | 11 +-
> include/asm-blackfin/mach-bf537/bfin_serial_5xx.h | 23 +-
> include/asm-blackfin/mach-bf537/portmux.h | 2 +-
> include/asm-blackfin/mach-bf561/bfin_serial_5xx.h | 11 +-
> 5 files changed, 274 insertions(+), 45 deletions(-)
>
>diff --git a/arch/blackfin/kernel/bfin_gpio.c
>b/arch/blackfin/kernel/bfin_gpio.c
>index bafcfa5..9f30948 100644
>--- a/arch/blackfin/kernel/bfin_gpio.c
>+++ b/arch/blackfin/kernel/bfin_gpio.c
>@@ -84,6 +84,7 @@
> #include <linux/err.h>
> #include <asm/blackfin.h>
> #include <asm/gpio.h>
>+#include <asm/portmux.h>
> #include <linux/irq.h>
>
> #ifdef BF533_FAMILY
>@@ -115,7 +116,11 @@ static struct gpio_port_t
>*gpio_bankb[gpio_bank(MAX_BLACKFIN_GPIOS)] = {
> };
> #endif
>
>-static unsigned short reserved_map[gpio_bank(MAX_BLACKFIN_GPIOS)];
>+static unsigned short
reserved_gpio_map[gpio_bank(MAX_BLACKFIN_GPIOS)];
>+static unsigned short reserved_peri_map[gpio_bank(MAX_BLACKFIN_GPIOS +
>16)];
>+char *str_ident = NULL;
>+
>+#define RESOURCE_LABEL_SIZE 16
>
> #ifdef CONFIG_PM
> static unsigned short wakeup_map[gpio_bank(MAX_BLACKFIN_GPIOS)];
>@@ -143,13 +148,39 @@ inline int check_gpio(unsigned short gpio)
> return 0;
> }
>
>+static void set_label(unsigned short ident, const char *label)
>+{
>+
>+ if (label && str_ident) {
>+ strncpy(str_ident + ident * RESOURCE_LABEL_SIZE, label,
>+ RESOURCE_LABEL_SIZE);
>+ str_ident[ident * RESOURCE_LABEL_SIZE +
>+ RESOURCE_LABEL_SIZE - 1] = 0;
>+ }
>+}
>+
>+static char *get_label(unsigned short ident)
>+{
>+ if (!str_ident)
>+ return "UNKNOWN";
>+
>+ return (str_ident[ident * RESOURCE_LABEL_SIZE] ?
>+ (str_ident + ident * RESOURCE_LABEL_SIZE) : "UNKNOWN");
>+}
>+
>+static int cmp_label(unsigned short ident, const char *label)
>+{
>+ if (label && str_ident)
>+ return strncmp(str_ident + ident * RESOURCE_LABEL_SIZE,
>+ label, strlen(label));
>+ else
>+ return -EINVAL;
>+}
>+
> #ifdef BF537_FAMILY
> static void port_setup(unsigned short gpio, unsigned short usage)
> {
> if (usage == GPIO_USAGE) {
>- if (*port_fer[gpio_bank(gpio)] & gpio_bit(gpio))
>- printk(KERN_WARNING "bfin-gpio: Possible
Conflict with
>Peripheral "
>- "usage and GPIO %d detected!\n", gpio);
> *port_fer[gpio_bank(gpio)] &= ~gpio_bit(gpio);
> } else
> *port_fer[gpio_bank(gpio)] |= gpio_bit(gpio);
>@@ -159,6 +190,56 @@ static void port_setup(unsigned short gpio,
unsigned
>short usage)
> # define port_setup(...) do { } while (0)
> #endif
>
>+#ifdef BF537_FAMILY
>+
>+#define PMUX_LUT_RES 0
>+#define PMUX_LUT_OFFSET 1
>+#define PMUX_LUT_ENTRIES 41
>+#define PMUX_LUT_SIZE 2
>+
>+static unsigned short port_mux_lut[PMUX_LUT_ENTRIES][PMUX_LUT_SIZE] =
{
>+ {P_PPI0_D13, 11}, {P_PPI0_D14, 11}, {P_PPI0_D15, 11},
>+ {P_SPORT1_TFS, 11}, {P_SPORT1_TSCLK, 11}, {P_SPORT1_DTPRI, 11},
>+ {P_PPI0_D10, 10}, {P_PPI0_D11, 10}, {P_PPI0_D12, 10},
>+ {P_SPORT1_RSCLK, 10}, {P_SPORT1_RFS, 10}, {P_SPORT1_DRPRI, 10},
>+ {P_PPI0_D8, 9}, {P_PPI0_D9, 9}, {P_SPORT1_DRSEC, 9},
>+ {P_SPORT1_DTSEC, 9}, {P_TMR2, 8}, {P_PPI0_FS3, 8}, {P_TMR3, 7},
>+ {P_SPI0_SSEL4, 7}, {P_TMR4, 6}, {P_SPI0_SSEL5, 6}, {P_TMR5, 5},
>+ {P_SPI0_SSEL6, 5}, {P_UART1_RX, 4}, {P_UART1_TX, 4}, {P_TMR6,
4},
>+ {P_TMR7, 4}, {P_UART0_RX, 3}, {P_UART0_TX, 3}, {P_DMAR0, 3},
>+ {P_DMAR1, 3}, {P_SPORT0_DTSEC, 1}, {P_SPORT0_DRSEC, 1},
>+ {P_CAN0_RX, 1}, {P_CAN0_TX, 1}, {P_SPI0_SSEL7, 1},
>+ {P_SPORT0_TFS, 0}, {P_SPORT0_DTPRI, 0}, {P_SPI0_SSEL2, 0},
>+ {P_SPI0_SSEL3, 0}
>+};
>+
>+static void portmux_setup(unsigned short per, unsigned short function)
>+{
>+ u16 y, muxreg, offset;
>+
>+ for (y = 0; y < PMUX_LUT_ENTRIES; y++) {
>+ if (port_mux_lut[y][PMUX_LUT_RES] == per) {
>+
>+ /* SET PORTMUX REG */
>+
>+ offset = port_mux_lut[y][PMUX_LUT_OFFSET];
>+ muxreg = bfin_read_PORT_MUX();
>+
>+ if (offset != 1) {
>+ muxreg &= ~(1 << offset);
>+ } else {
>+ muxreg &= ~(3 << 1);
>+ }
>+
>+ muxreg |= (function << offset);
>+ bfin_write_PORT_MUX(muxreg);
>+ }
>+ }
>+}
>+
>+#else
>+# define portmux_setup(...) do { } while (0)
>+#endif
>
> static void default_gpio(unsigned short gpio)
> {
>@@ -179,22 +260,15 @@ static void default_gpio(unsigned short gpio)
>
> static int __init bfin_gpio_init(void)
> {
>- int i;
>-
>- printk(KERN_INFO "Blackfin GPIO Controller\n");
>
>- for (i = 0; i < MAX_BLACKFIN_GPIOS; i += GPIO_BANKSIZE)
>- reserved_map[gpio_bank(i)] = 0;
>+ str_ident = kzalloc(RESOURCE_LABEL_SIZE * 256, GFP_KERNEL);
>+ if (!str_ident)
>+ return -ENOMEM;
>
>-#if defined(BF537_FAMILY) && (defined(CONFIG_BFIN_MAC) ||
>defined(CONFIG_BFIN_MAC_MODULE))
>-# if defined(CONFIG_BFIN_MAC_RMII)
>- reserved_map[gpio_bank(PORT_H)] = 0xC373;
>-# else
>- reserved_map[gpio_bank(PORT_H)] = 0xFFFF;
>-# endif
>-#endif
>+ printk(KERN_INFO "Blackfin GPIO Controller\n");
>
> return 0;
>+
> }
>
> arch_initcall(bfin_gpio_init);
>@@ -223,7 +297,7 @@ arch_initcall(bfin_gpio_init);
> void set_gpio_ ## name(unsigned short gpio, unsigned short arg) \
> { \
> unsigned long flags; \
>- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio))); \
>+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
\
> local_irq_save(flags); \
> if (arg) \
> gpio_bankb[gpio_bank(gpio)]->name |= gpio_bit(gpio); \
>@@ -243,7 +317,7 @@ SET_GPIO(both)
> #define SET_GPIO_SC(name) \
> void set_gpio_ ## name(unsigned short gpio, unsigned short arg) \
> { \
>- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio))); \
>+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
\
> if (arg) \
> gpio_bankb[gpio_bank(gpio)]->name ## _set =
gpio_bit(gpio); \
> else \
>@@ -258,7 +332,7 @@ SET_GPIO_SC(maskb)
> void set_gpio_data(unsigned short gpio, unsigned short arg)
> {
> unsigned long flags;
>- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
>+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> local_irq_save(flags);
> if (arg)
> gpio_bankb[gpio_bank(gpio)]->data_set = gpio_bit(gpio);
>@@ -277,7 +351,7 @@ SET_GPIO_SC(data)
> void set_gpio_toggle(unsigned short gpio)
> {
> unsigned long flags;
>- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
>+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> local_irq_save(flags);
> gpio_bankb[gpio_bank(gpio)]->toggle = gpio_bit(gpio);
> bfin_read_CHIPID();
>@@ -286,7 +360,7 @@ void set_gpio_toggle(unsigned short gpio)
> #else
> void set_gpio_toggle(unsigned short gpio)
> {
>- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
>+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> gpio_bankb[gpio_bank(gpio)]->toggle = gpio_bit(gpio);
> }
> #endif
>@@ -350,7 +424,7 @@ unsigned short get_gpio_data(unsigned short gpio)
> {
> unsigned long flags;
> unsigned short ret;
>- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
>+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> local_irq_save(flags);
> ret = 0x01 & (gpio_bankb[gpio_bank(gpio)]->data >>
gpio_sub_n(gpio));
> bfin_read_CHIPID();
>@@ -494,13 +568,14 @@ u32 gpio_pm_setup(void)
> gpio_bank_saved[bank].dir =
gpio_bankb[bank]->dir;
> gpio_bank_saved[bank].edge =
gpio_bankb[bank]->edge;
> gpio_bank_saved[bank].both =
gpio_bankb[bank]->both;
>- gpio_bank_saved[bank].reserved =
reserved_map[bank];
>+ gpio_bank_saved[bank].reserved =
>+ reserved_gpio_map[bank];
>
> gpio = i;
>
> while (mask) {
> if (mask & 1) {
>- reserved_map[gpio_bank(gpio)] |=
>+
reserved_gpio_map[gpio_bank(gpio)] |=
> gpio_bit(gpio);
> bfin_gpio_wakeup_type(gpio,
> wakeup_flags_map[gpio]);
>@@ -540,7 +615,8 @@ void gpio_pm_restore(void)
> gpio_bankb[bank]->edge =
gpio_bank_saved[bank].edge;
> gpio_bankb[bank]->both =
gpio_bank_saved[bank].both;
>
>- reserved_map[bank] =
gpio_bank_saved[bank].reserved;
>+ reserved_gpio_map[bank] =
>+ gpio_bank_saved[bank].reserved;
>
> }
>
>@@ -550,6 +626,140 @@ void gpio_pm_restore(void)
>
> #endif
>
>+
>+
>+
>+int peripheral_request(unsigned short per, const char *label)
>+{
>+ unsigned long flags;
>+ unsigned short ident = P_IDENT(per);
>+
>+ /*
>+ * Don't cares are pins with only one dedicated function
>+ */
>+
>+ if (per & P_DONTCARE)
>+ return 0;
>+
>+ if (!(per & P_DEFINED))
>+ return -ENODEV;
>+
>+ if (check_gpio(ident) < 0)
>+ return -EINVAL;
>+
>+ local_irq_save(flags);
>+
>+ if (unlikely(reserved_gpio_map[gpio_bank(ident)] &
gpio_bit(ident)))
>{
>+ printk(KERN_ERR
>+ "%s: Peripheral %d is already reserved as GPIO by
%s
>!\n",
>+ __FUNCTION__, ident, get_label(ident));
>+ dump_stack();
>+ local_irq_restore(flags);
>+ return -EBUSY;
>+ }
>+
>+ if (unlikely(reserved_peri_map[gpio_bank(ident)] &
gpio_bit(ident)))
>{
>+
>+ /*
>+ * Pin functions like AMC address strobes my
>+ * be requested and used by several drivers
>+ */
>+
>+ if (!(per & P_MAYSHARE)) {
>+
>+ /*
>+ * Allow that the identical pin function can
>+ * be requested from the same driver twice
>+ */
>+
>+ if (cmp_label(ident, label) == 0)
>+ goto anyway;
>+
>+ printk(KERN_ERR
>+ "%s: Peripheral %d function %d is
already"
>+ "reserved by %s !\n",
>+ __FUNCTION__, ident, P_FUNCT2MUX(per),
>+ get_label(ident));
>+ dump_stack();
>+ local_irq_restore(flags);
>+ return -EBUSY;
>+ }
>+
>+ }
>+
>+anyway:
>+
>+
>+ portmux_setup(per, P_FUNCT2MUX(per));
>+
>+ port_setup(ident, PERIPHERAL_USAGE);
>+
>+ reserved_peri_map[gpio_bank(ident)] |= gpio_bit(ident);
>+ local_irq_restore(flags);
>+ set_label(ident, label);
>+
>+ return 0;
>+}
>+EXPORT_SYMBOL(peripheral_request);
>+
>+int peripheral_request_list(unsigned short per[], const char *label)
>+{
>+ u16 cnt;
>+ int ret;
>+
>+ for (cnt = 0; per[cnt] != 0; cnt++) {
>+ ret = peripheral_request(per[cnt], label);
>+ if (ret < 0)
>+ return ret;
>+ }
>+
>+ return 0;
>+}
>+EXPORT_SYMBOL(peripheral_request_list);
>+
>+void peripheral_free(unsigned short per)
>+{
>+ unsigned long flags;
>+ unsigned short ident = P_IDENT(per);
>+
>+ if (per & P_DONTCARE)
>+ return;
>+
>+ if (!(per & P_DEFINED))
>+ return;
>+
>+ if (check_gpio(ident) < 0)
>+ return;
>+
>+ local_irq_save(flags);
>+
>+ if (unlikely(!(reserved_peri_map[gpio_bank(ident)]
>+ & gpio_bit(ident)))) {
>+ local_irq_restore(flags);
>+ return;
>+ }
>+
>+ if (!(per & P_MAYSHARE)) {
>+ port_setup(ident, GPIO_USAGE);
>+ }
>+
>+ reserved_peri_map[gpio_bank(ident)] &= ~gpio_bit(ident);
>+
>+ local_irq_restore(flags);
>+}
>+EXPORT_SYMBOL(peripheral_free);
>+
>+void peripheral_free_list(unsigned short per[])
>+{
>+ u16 cnt;
>+
>+ for (cnt = 0; per[cnt] != 0; cnt++) {
>+ peripheral_free(per[cnt]);
>+ }
>+
>+}
>+EXPORT_SYMBOL(peripheral_free_list);
>+
> /***********************************************************
> *
> * FUNCTIONS: Blackfin GPIO Driver
>@@ -574,13 +784,13 @@ int gpio_request(unsigned short gpio, const char
>*label)
>
> local_irq_save(flags);
>
>- if (unlikely(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio))) {
>+ if (unlikely(reserved_gpio_map[gpio_bank(gpio)] &
gpio_bit(gpio))) {
> printk(KERN_ERR "bfin-gpio: GPIO %d is already
reserved!\n",
>gpio);
> dump_stack();
> local_irq_restore(flags);
> return -EBUSY;
> }
>- reserved_map[gpio_bank(gpio)] |= gpio_bit(gpio);
>+ reserved_gpio_map[gpio_bank(gpio)] |= gpio_bit(gpio);
>
> local_irq_restore(flags);
>
>@@ -599,7 +809,7 @@ void gpio_free(unsigned short gpio)
>
> local_irq_save(flags);
>
>- if (unlikely(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio))))
{
>+ if (unlikely(!(reserved_gpio_map[gpio_bank(gpio)] &
gpio_bit(gpio))))
>{
> printk(KERN_ERR "bfin-gpio: GPIO %d wasn't reserved!\n",
gpio);
> dump_stack();
> local_irq_restore(flags);
>@@ -608,7 +818,7 @@ void gpio_free(unsigned short gpio)
>
> default_gpio(gpio);
>
>- reserved_map[gpio_bank(gpio)] &= ~gpio_bit(gpio);
>+ reserved_gpio_map[gpio_bank(gpio)] &= ~gpio_bit(gpio);
>
> local_irq_restore(flags);
> }
>@@ -618,7 +828,7 @@ void gpio_direction_input(unsigned short gpio)
> {
> unsigned long flags;
>
>- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
>+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
>
> local_irq_save(flags);
> gpio_bankb[gpio_bank(gpio)]->dir &= ~gpio_bit(gpio);
>@@ -631,7 +841,7 @@ void gpio_direction_output(unsigned short gpio)
> {
> unsigned long flags;
>
>- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
>+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
>
> local_irq_save(flags);
> gpio_bankb[gpio_bank(gpio)]->inen &= ~gpio_bit(gpio);
>diff --git a/include/asm-blackfin/mach-bf533/bfin_serial_5xx.h
>b/include/asm-blackfin/mach-bf533/bfin_serial_5xx.h
>index e043caf..69b9f8e 100644
>--- a/include/asm-blackfin/mach-bf533/bfin_serial_5xx.h
>+++ b/include/asm-blackfin/mach-bf533/bfin_serial_5xx.h
>@@ -1,5 +1,6 @@
> #include <linux/serial.h>
> #include <asm/dma.h>
>+#include <asm/portmux.h>
>
> #define NR_PORTS 1
>
>@@ -92,18 +93,24 @@ struct bfin_serial_res bfin_serial_resource[] = {
> }
> };
>
>+#define DRIVER_NAME "bfin-uart"
>
> int nr_ports = NR_PORTS;
> static void bfin_serial_hw_init(struct bfin_serial_port *uart)
> {
>
>+#ifdef CONFIG_SERIAL_BFIN_UART0
>+ peripheral_request(P_UART0_TX, DRIVER_NAME);
>+ peripheral_request(P_UART0_RX, DRIVER_NAME);
>+#endif
>+
> #ifdef CONFIG_SERIAL_BFIN_CTSRTS
> if (uart->cts_pin >= 0) {
>- gpio_request(uart->cts_pin, NULL);
>+ gpio_request(uart->cts_pin, DRIVER_NAME);
> gpio_direction_input(uart->cts_pin);
> }
> if (uart->rts_pin >= 0) {
>- gpio_request(uart->rts_pin, NULL);
>+ gpio_request(uart->rts_pin, DRIVER_NAME);
> gpio_direction_input(uart->rts_pin);
> }
> #endif
>diff --git a/include/asm-blackfin/mach-bf537/bfin_serial_5xx.h
>b/include/asm-blackfin/mach-bf537/bfin_serial_5xx.h
>index 8f5d9c4..6fb328f 100644
>--- a/include/asm-blackfin/mach-bf537/bfin_serial_5xx.h
>+++ b/include/asm-blackfin/mach-bf537/bfin_serial_5xx.h
>@@ -1,5 +1,6 @@
> #include <linux/serial.h>
> #include <asm/dma.h>
>+#include <asm/portmux.h>
>
> #define NR_PORTS 2
>
>@@ -122,25 +123,29 @@ struct bfin_serial_res bfin_serial_resource[] = {
>
> int nr_ports = ARRAY_SIZE(bfin_serial_resource);
>
>+#define DRIVER_NAME "bfin-uart"
>+
> static void bfin_serial_hw_init(struct bfin_serial_port *uart)
> {
>- unsigned short val;
>- val = bfin_read16(BFIN_PORT_MUX);
>- val &= ~(PFDE | PFTE);
>- bfin_write16(BFIN_PORT_MUX, val);
>
>- val = bfin_read16(PORTF_FER);
>- val |= 0xF;
>- bfin_write16(PORTF_FER, val);
>+#ifdef CONFIG_SERIAL_BFIN_UART0
>+ peripheral_request(P_UART0_TX, DRIVER_NAME);
>+ peripheral_request(P_UART0_RX, DRIVER_NAME);
>+#endif
>+
>+#ifdef CONFIG_SERIAL_BFIN_UART1
>+ peripheral_request(P_UART1_TX, DRIVER_NAME);
>+ peripheral_request(P_UART1_RX, DRIVER_NAME);
>+#endif
>
> #ifdef CONFIG_SERIAL_BFIN_CTSRTS
> if (uart->cts_pin >= 0) {
>- gpio_request(uart->cts_pin, NULL);
>+ gpio_request(uart->cts_pin, DRIVER_NAME);
> gpio_direction_input(uart->cts_pin);
> }
>
> if (uart->rts_pin >= 0) {
>- gpio_request(uart->rts_pin, NULL);
>+ gpio_request(uart->rts_pin, DRIVER_NAME);
> gpio_direction_output(uart->rts_pin);
> }
> #endif
>diff --git a/include/asm-blackfin/mach-bf537/portmux.h b/include/asm-
>blackfin/mach-bf537/portmux.h
>index 23e13c5..7daa247 100644
>--- a/include/asm-blackfin/mach-bf537/portmux.h
>+++ b/include/asm-blackfin/mach-bf537/portmux.h
>@@ -106,4 +106,4 @@
> #define P_SPI0_SSEL2 (P_DEFINED | P_IDENT(PORT_PJ11) | P_FUNCT(1))
> #define P_SPI0_SSEL7 (P_DEFINED | P_IDENT(PORT_PJ5) | P_FUNCT(2))
>
>-#endif /* _MACH_PORTMUX_H_ */
>+#endif /* _MACH_PORTMUX_H_ */
>diff --git a/include/asm-blackfin/mach-bf561/bfin_serial_5xx.h
>b/include/asm-blackfin/mach-bf561/bfin_serial_5xx.h
>index e043caf..69b9f8e 100644
>--- a/include/asm-blackfin/mach-bf561/bfin_serial_5xx.h
>+++ b/include/asm-blackfin/mach-bf561/bfin_serial_5xx.h
>@@ -1,5 +1,6 @@
> #include <linux/serial.h>
> #include <asm/dma.h>
>+#include <asm/portmux.h>
>
> #define NR_PORTS 1
>
>@@ -92,18 +93,24 @@ struct bfin_serial_res bfin_serial_resource[] = {
> }
> };
>
>+#define DRIVER_NAME "bfin-uart"
>
> int nr_ports = NR_PORTS;
> static void bfin_serial_hw_init(struct bfin_serial_port *uart)
> {
>
>+#ifdef CONFIG_SERIAL_BFIN_UART0
>+ peripheral_request(P_UART0_TX, DRIVER_NAME);
>+ peripheral_request(P_UART0_RX, DRIVER_NAME);
>+#endif
>+
> #ifdef CONFIG_SERIAL_BFIN_CTSRTS
> if (uart->cts_pin >= 0) {
>- gpio_request(uart->cts_pin, NULL);
>+ gpio_request(uart->cts_pin, DRIVER_NAME);
> gpio_direction_input(uart->cts_pin);
> }
> if (uart->rts_pin >= 0) {
>- gpio_request(uart->rts_pin, NULL);
>+ gpio_request(uart->rts_pin, DRIVER_NAME);
> gpio_direction_input(uart->rts_pin);
> }
> #endif
>--
>1.5.2

2007-08-08 07:50:55

by Bryan Wu

[permalink] [raw]
Subject: RE: [PATCH 01/12] Blackfin arch: add peripheral resource allocation support

On Wed, 2007-08-08 at 08:18 +0100, Hennerich, Michael wrote:
> Bryan,
>
> This patch doesn't seem to be up to date.
> It doesn't include the changes made based on feedback from Joe Perches.
>
> Please see our SVN:
> Modified: trunk/arch/blackfin/kernel/bfin_gpio.c (3489 => 3490)
>

Yes, I got all changes in my local git-tree.
Actually I send out the patches ordered by time, next time I will send
out patch based on Joe's idea.

Please don't worry about this, I'll try my best to make sure never
missing the changes.

Thanks
- Bryan Wu

> -Michael
>
> >-----Original Message-----
> >From: Bryan Wu [mailto:[email protected]]
> >Sent: Mittwoch, 8. August 2007 05:35
> >To: [email protected]; [email protected];
> >[email protected]
> >Cc: [email protected]; Michael Hennerich; Bryan Wu
> >Subject: [PATCH 01/12] Blackfin arch: add peripheral resource allocation
> >support
> >
> >From: Michael Hennerich <[email protected]>
> >
> >Signed-off-by: Michael Hennerich <[email protected]>
> >Signed-off-by: Bryan Wu <[email protected]>
> >---
> > arch/blackfin/kernel/bfin_gpio.c | 272
> >++++++++++++++++++---
> > include/asm-blackfin/mach-bf533/bfin_serial_5xx.h | 11 +-
> > include/asm-blackfin/mach-bf537/bfin_serial_5xx.h | 23 +-
> > include/asm-blackfin/mach-bf537/portmux.h | 2 +-
> > include/asm-blackfin/mach-bf561/bfin_serial_5xx.h | 11 +-
> > 5 files changed, 274 insertions(+), 45 deletions(-)
> >
> >diff --git a/arch/blackfin/kernel/bfin_gpio.c
> >b/arch/blackfin/kernel/bfin_gpio.c
> >index bafcfa5..9f30948 100644
> >--- a/arch/blackfin/kernel/bfin_gpio.c
> >+++ b/arch/blackfin/kernel/bfin_gpio.c
> >@@ -84,6 +84,7 @@
> > #include <linux/err.h>
> > #include <asm/blackfin.h>
> > #include <asm/gpio.h>
> >+#include <asm/portmux.h>
> > #include <linux/irq.h>
> >
> > #ifdef BF533_FAMILY
> >@@ -115,7 +116,11 @@ static struct gpio_port_t
> >*gpio_bankb[gpio_bank(MAX_BLACKFIN_GPIOS)] = {
> > };
> > #endif
> >
> >-static unsigned short reserved_map[gpio_bank(MAX_BLACKFIN_GPIOS)];
> >+static unsigned short reserved_gpio_map[gpio_bank(MAX_BLACKFIN_GPIOS)];
> >+static unsigned short reserved_peri_map[gpio_bank(MAX_BLACKFIN_GPIOS +
> >16)];
> >+char *str_ident = NULL;
> >+
> >+#define RESOURCE_LABEL_SIZE 16
> >
> > #ifdef CONFIG_PM
> > static unsigned short wakeup_map[gpio_bank(MAX_BLACKFIN_GPIOS)];
> >@@ -143,13 +148,39 @@ inline int check_gpio(unsigned short gpio)
> > return 0;
> > }
> >
> >+static void set_label(unsigned short ident, const char *label)
> >+{
> >+
> >+ if (label && str_ident) {
> >+ strncpy(str_ident + ident * RESOURCE_LABEL_SIZE, label,
> >+ RESOURCE_LABEL_SIZE);
> >+ str_ident[ident * RESOURCE_LABEL_SIZE +
> >+ RESOURCE_LABEL_SIZE - 1] = 0;
> >+ }
> >+}
> >+
> >+static char *get_label(unsigned short ident)
> >+{
> >+ if (!str_ident)
> >+ return "UNKNOWN";
> >+
> >+ return (str_ident[ident * RESOURCE_LABEL_SIZE] ?
> >+ (str_ident + ident * RESOURCE_LABEL_SIZE) : "UNKNOWN");
> >+}
> >+
> >+static int cmp_label(unsigned short ident, const char *label)
> >+{
> >+ if (label && str_ident)
> >+ return strncmp(str_ident + ident * RESOURCE_LABEL_SIZE,
> >+ label, strlen(label));
> >+ else
> >+ return -EINVAL;
> >+}
> >+
> > #ifdef BF537_FAMILY
> > static void port_setup(unsigned short gpio, unsigned short usage)
> > {
> > if (usage == GPIO_USAGE) {
> >- if (*port_fer[gpio_bank(gpio)] & gpio_bit(gpio))
> >- printk(KERN_WARNING "bfin-gpio: Possible Conflict with
> >Peripheral "
> >- "usage and GPIO %d detected!\n", gpio);
> > *port_fer[gpio_bank(gpio)] &= ~gpio_bit(gpio);
> > } else
> > *port_fer[gpio_bank(gpio)] |= gpio_bit(gpio);
> >@@ -159,6 +190,56 @@ static void port_setup(unsigned short gpio, unsigned
> >short usage)
> > # define port_setup(...) do { } while (0)
> > #endif
> >
> >+#ifdef BF537_FAMILY
> >+
> >+#define PMUX_LUT_RES 0
> >+#define PMUX_LUT_OFFSET 1
> >+#define PMUX_LUT_ENTRIES 41
> >+#define PMUX_LUT_SIZE 2
> >+
> >+static unsigned short port_mux_lut[PMUX_LUT_ENTRIES][PMUX_LUT_SIZE] = {
> >+ {P_PPI0_D13, 11}, {P_PPI0_D14, 11}, {P_PPI0_D15, 11},
> >+ {P_SPORT1_TFS, 11}, {P_SPORT1_TSCLK, 11}, {P_SPORT1_DTPRI, 11},
> >+ {P_PPI0_D10, 10}, {P_PPI0_D11, 10}, {P_PPI0_D12, 10},
> >+ {P_SPORT1_RSCLK, 10}, {P_SPORT1_RFS, 10}, {P_SPORT1_DRPRI, 10},
> >+ {P_PPI0_D8, 9}, {P_PPI0_D9, 9}, {P_SPORT1_DRSEC, 9},
> >+ {P_SPORT1_DTSEC, 9}, {P_TMR2, 8}, {P_PPI0_FS3, 8}, {P_TMR3, 7},
> >+ {P_SPI0_SSEL4, 7}, {P_TMR4, 6}, {P_SPI0_SSEL5, 6}, {P_TMR5, 5},
> >+ {P_SPI0_SSEL6, 5}, {P_UART1_RX, 4}, {P_UART1_TX, 4}, {P_TMR6, 4},
> >+ {P_TMR7, 4}, {P_UART0_RX, 3}, {P_UART0_TX, 3}, {P_DMAR0, 3},
> >+ {P_DMAR1, 3}, {P_SPORT0_DTSEC, 1}, {P_SPORT0_DRSEC, 1},
> >+ {P_CAN0_RX, 1}, {P_CAN0_TX, 1}, {P_SPI0_SSEL7, 1},
> >+ {P_SPORT0_TFS, 0}, {P_SPORT0_DTPRI, 0}, {P_SPI0_SSEL2, 0},
> >+ {P_SPI0_SSEL3, 0}
> >+};
> >+
> >+static void portmux_setup(unsigned short per, unsigned short function)
> >+{
> >+ u16 y, muxreg, offset;
> >+
> >+ for (y = 0; y < PMUX_LUT_ENTRIES; y++) {
> >+ if (port_mux_lut[y][PMUX_LUT_RES] == per) {
> >+
> >+ /* SET PORTMUX REG */
> >+
> >+ offset = port_mux_lut[y][PMUX_LUT_OFFSET];
> >+ muxreg = bfin_read_PORT_MUX();
> >+
> >+ if (offset != 1) {
> >+ muxreg &= ~(1 << offset);
> >+ } else {
> >+ muxreg &= ~(3 << 1);
> >+ }
> >+
> >+ muxreg |= (function << offset);
> >+ bfin_write_PORT_MUX(muxreg);
> >+ }
> >+ }
> >+}
> >+
> >+#else
> >+# define portmux_setup(...) do { } while (0)
> >+#endif
> >
> > static void default_gpio(unsigned short gpio)
> > {
> >@@ -179,22 +260,15 @@ static void default_gpio(unsigned short gpio)
> >
> > static int __init bfin_gpio_init(void)
> > {
> >- int i;
> >-
> >- printk(KERN_INFO "Blackfin GPIO Controller\n");
> >
> >- for (i = 0; i < MAX_BLACKFIN_GPIOS; i += GPIO_BANKSIZE)
> >- reserved_map[gpio_bank(i)] = 0;
> >+ str_ident = kzalloc(RESOURCE_LABEL_SIZE * 256, GFP_KERNEL);
> >+ if (!str_ident)
> >+ return -ENOMEM;
> >
> >-#if defined(BF537_FAMILY) && (defined(CONFIG_BFIN_MAC) ||
> >defined(CONFIG_BFIN_MAC_MODULE))
> >-# if defined(CONFIG_BFIN_MAC_RMII)
> >- reserved_map[gpio_bank(PORT_H)] = 0xC373;
> >-# else
> >- reserved_map[gpio_bank(PORT_H)] = 0xFFFF;
> >-# endif
> >-#endif
> >+ printk(KERN_INFO "Blackfin GPIO Controller\n");
> >
> > return 0;
> >+
> > }
> >
> > arch_initcall(bfin_gpio_init);
> >@@ -223,7 +297,7 @@ arch_initcall(bfin_gpio_init);
> > void set_gpio_ ## name(unsigned short gpio, unsigned short arg) \
> > { \
> > unsigned long flags; \
> >- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio))); \
> >+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio))); \
> > local_irq_save(flags); \
> > if (arg) \
> > gpio_bankb[gpio_bank(gpio)]->name |= gpio_bit(gpio); \
> >@@ -243,7 +317,7 @@ SET_GPIO(both)
> > #define SET_GPIO_SC(name) \
> > void set_gpio_ ## name(unsigned short gpio, unsigned short arg) \
> > { \
> >- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio))); \
> >+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio))); \
> > if (arg) \
> > gpio_bankb[gpio_bank(gpio)]->name ## _set = gpio_bit(gpio); \
> > else \
> >@@ -258,7 +332,7 @@ SET_GPIO_SC(maskb)
> > void set_gpio_data(unsigned short gpio, unsigned short arg)
> > {
> > unsigned long flags;
> >- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> >+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> > local_irq_save(flags);
> > if (arg)
> > gpio_bankb[gpio_bank(gpio)]->data_set = gpio_bit(gpio);
> >@@ -277,7 +351,7 @@ SET_GPIO_SC(data)
> > void set_gpio_toggle(unsigned short gpio)
> > {
> > unsigned long flags;
> >- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> >+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> > local_irq_save(flags);
> > gpio_bankb[gpio_bank(gpio)]->toggle = gpio_bit(gpio);
> > bfin_read_CHIPID();
> >@@ -286,7 +360,7 @@ void set_gpio_toggle(unsigned short gpio)
> > #else
> > void set_gpio_toggle(unsigned short gpio)
> > {
> >- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> >+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> > gpio_bankb[gpio_bank(gpio)]->toggle = gpio_bit(gpio);
> > }
> > #endif
> >@@ -350,7 +424,7 @@ unsigned short get_gpio_data(unsigned short gpio)
> > {
> > unsigned long flags;
> > unsigned short ret;
> >- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> >+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> > local_irq_save(flags);
> > ret = 0x01 & (gpio_bankb[gpio_bank(gpio)]->data >> gpio_sub_n(gpio));
> > bfin_read_CHIPID();
> >@@ -494,13 +568,14 @@ u32 gpio_pm_setup(void)
> > gpio_bank_saved[bank].dir = gpio_bankb[bank]->dir;
> > gpio_bank_saved[bank].edge = gpio_bankb[bank]->edge;
> > gpio_bank_saved[bank].both = gpio_bankb[bank]->both;
> >- gpio_bank_saved[bank].reserved = reserved_map[bank];
> >+ gpio_bank_saved[bank].reserved =
> >+ reserved_gpio_map[bank];
> >
> > gpio = i;
> >
> > while (mask) {
> > if (mask & 1) {
> >- reserved_map[gpio_bank(gpio)] |=
> >+ reserved_gpio_map[gpio_bank(gpio)] |=
> > gpio_bit(gpio);
> > bfin_gpio_wakeup_type(gpio,
> > wakeup_flags_map[gpio]);
> >@@ -540,7 +615,8 @@ void gpio_pm_restore(void)
> > gpio_bankb[bank]->edge = gpio_bank_saved[bank].edge;
> > gpio_bankb[bank]->both = gpio_bank_saved[bank].both;
> >
> >- reserved_map[bank] = gpio_bank_saved[bank].reserved;
> >+ reserved_gpio_map[bank] =
> >+ gpio_bank_saved[bank].reserved;
> >
> > }
> >
> >@@ -550,6 +626,140 @@ void gpio_pm_restore(void)
> >
> > #endif
> >
> >+
> >+
> >+
> >+int peripheral_request(unsigned short per, const char *label)
> >+{
> >+ unsigned long flags;
> >+ unsigned short ident = P_IDENT(per);
> >+
> >+ /*
> >+ * Don't cares are pins with only one dedicated function
> >+ */
> >+
> >+ if (per & P_DONTCARE)
> >+ return 0;
> >+
> >+ if (!(per & P_DEFINED))
> >+ return -ENODEV;
> >+
> >+ if (check_gpio(ident) < 0)
> >+ return -EINVAL;
> >+
> >+ local_irq_save(flags);
> >+
> >+ if (unlikely(reserved_gpio_map[gpio_bank(ident)] & gpio_bit(ident)))
> >{
> >+ printk(KERN_ERR
> >+ "%s: Peripheral %d is already reserved as GPIO by %s
> >!\n",
> >+ __FUNCTION__, ident, get_label(ident));
> >+ dump_stack();
> >+ local_irq_restore(flags);
> >+ return -EBUSY;
> >+ }
> >+
> >+ if (unlikely(reserved_peri_map[gpio_bank(ident)] & gpio_bit(ident)))
> >{
> >+
> >+ /*
> >+ * Pin functions like AMC address strobes my
> >+ * be requested and used by several drivers
> >+ */
> >+
> >+ if (!(per & P_MAYSHARE)) {
> >+
> >+ /*
> >+ * Allow that the identical pin function can
> >+ * be requested from the same driver twice
> >+ */
> >+
> >+ if (cmp_label(ident, label) == 0)
> >+ goto anyway;
> >+
> >+ printk(KERN_ERR
> >+ "%s: Peripheral %d function %d is already"
> >+ "reserved by %s !\n",
> >+ __FUNCTION__, ident, P_FUNCT2MUX(per),
> >+ get_label(ident));
> >+ dump_stack();
> >+ local_irq_restore(flags);
> >+ return -EBUSY;
> >+ }
> >+
> >+ }
> >+
> >+anyway:
> >+
> >+
> >+ portmux_setup(per, P_FUNCT2MUX(per));
> >+
> >+ port_setup(ident, PERIPHERAL_USAGE);
> >+
> >+ reserved_peri_map[gpio_bank(ident)] |= gpio_bit(ident);
> >+ local_irq_restore(flags);
> >+ set_label(ident, label);
> >+
> >+ return 0;
> >+}
> >+EXPORT_SYMBOL(peripheral_request);
> >+
> >+int peripheral_request_list(unsigned short per[], const char *label)
> >+{
> >+ u16 cnt;
> >+ int ret;
> >+
> >+ for (cnt = 0; per[cnt] != 0; cnt++) {
> >+ ret = peripheral_request(per[cnt], label);
> >+ if (ret < 0)
> >+ return ret;
> >+ }
> >+
> >+ return 0;
> >+}
> >+EXPORT_SYMBOL(peripheral_request_list);
> >+
> >+void peripheral_free(unsigned short per)
> >+{
> >+ unsigned long flags;
> >+ unsigned short ident = P_IDENT(per);
> >+
> >+ if (per & P_DONTCARE)
> >+ return;
> >+
> >+ if (!(per & P_DEFINED))
> >+ return;
> >+
> >+ if (check_gpio(ident) < 0)
> >+ return;
> >+
> >+ local_irq_save(flags);
> >+
> >+ if (unlikely(!(reserved_peri_map[gpio_bank(ident)]
> >+ & gpio_bit(ident)))) {
> >+ local_irq_restore(flags);
> >+ return;
> >+ }
> >+
> >+ if (!(per & P_MAYSHARE)) {
> >+ port_setup(ident, GPIO_USAGE);
> >+ }
> >+
> >+ reserved_peri_map[gpio_bank(ident)] &= ~gpio_bit(ident);
> >+
> >+ local_irq_restore(flags);
> >+}
> >+EXPORT_SYMBOL(peripheral_free);
> >+
> >+void peripheral_free_list(unsigned short per[])
> >+{
> >+ u16 cnt;
> >+
> >+ for (cnt = 0; per[cnt] != 0; cnt++) {
> >+ peripheral_free(per[cnt]);
> >+ }
> >+
> >+}
> >+EXPORT_SYMBOL(peripheral_free_list);
> >+
> > /***********************************************************
> > *
> > * FUNCTIONS: Blackfin GPIO Driver
> >@@ -574,13 +784,13 @@ int gpio_request(unsigned short gpio, const char
> >*label)
> >
> > local_irq_save(flags);
> >
> >- if (unlikely(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio))) {
> >+ if (unlikely(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio))) {
> > printk(KERN_ERR "bfin-gpio: GPIO %d is already reserved!\n",
> >gpio);
> > dump_stack();
> > local_irq_restore(flags);
> > return -EBUSY;
> > }
> >- reserved_map[gpio_bank(gpio)] |= gpio_bit(gpio);
> >+ reserved_gpio_map[gpio_bank(gpio)] |= gpio_bit(gpio);
> >
> > local_irq_restore(flags);
> >
> >@@ -599,7 +809,7 @@ void gpio_free(unsigned short gpio)
> >
> > local_irq_save(flags);
> >
> >- if (unlikely(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)))) {
> >+ if (unlikely(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio))))
> >{
> > printk(KERN_ERR "bfin-gpio: GPIO %d wasn't reserved!\n", gpio);
> > dump_stack();
> > local_irq_restore(flags);
> >@@ -608,7 +818,7 @@ void gpio_free(unsigned short gpio)
> >
> > default_gpio(gpio);
> >
> >- reserved_map[gpio_bank(gpio)] &= ~gpio_bit(gpio);
> >+ reserved_gpio_map[gpio_bank(gpio)] &= ~gpio_bit(gpio);
> >
> > local_irq_restore(flags);
> > }
> >@@ -618,7 +828,7 @@ void gpio_direction_input(unsigned short gpio)
> > {
> > unsigned long flags;
> >
> >- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> >+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> >
> > local_irq_save(flags);
> > gpio_bankb[gpio_bank(gpio)]->dir &= ~gpio_bit(gpio);
> >@@ -631,7 +841,7 @@ void gpio_direction_output(unsigned short gpio)
> > {
> > unsigned long flags;
> >
> >- BUG_ON(!(reserved_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> >+ BUG_ON(!(reserved_gpio_map[gpio_bank(gpio)] & gpio_bit(gpio)));
> >
> > local_irq_save(flags);
> > gpio_bankb[gpio_bank(gpio)]->inen &= ~gpio_bit(gpio);
> >diff --git a/include/asm-blackfin/mach-bf533/bfin_serial_5xx.h
> >b/include/asm-blackfin/mach-bf533/bfin_serial_5xx.h
> >index e043caf..69b9f8e 100644
> >--- a/include/asm-blackfin/mach-bf533/bfin_serial_5xx.h
> >+++ b/include/asm-blackfin/mach-bf533/bfin_serial_5xx.h
> >@@ -1,5 +1,6 @@
> > #include <linux/serial.h>
> > #include <asm/dma.h>
> >+#include <asm/portmux.h>
> >
> > #define NR_PORTS 1
> >
> >@@ -92,18 +93,24 @@ struct bfin_serial_res bfin_serial_resource[] = {
> > }
> > };
> >
> >+#define DRIVER_NAME "bfin-uart"
> >
> > int nr_ports = NR_PORTS;
> > static void bfin_serial_hw_init(struct bfin_serial_port *uart)
> > {
> >
> >+#ifdef CONFIG_SERIAL_BFIN_UART0
> >+ peripheral_request(P_UART0_TX, DRIVER_NAME);
> >+ peripheral_request(P_UART0_RX, DRIVER_NAME);
> >+#endif
> >+
> > #ifdef CONFIG_SERIAL_BFIN_CTSRTS
> > if (uart->cts_pin >= 0) {
> >- gpio_request(uart->cts_pin, NULL);
> >+ gpio_request(uart->cts_pin, DRIVER_NAME);
> > gpio_direction_input(uart->cts_pin);
> > }
> > if (uart->rts_pin >= 0) {
> >- gpio_request(uart->rts_pin, NULL);
> >+ gpio_request(uart->rts_pin, DRIVER_NAME);
> > gpio_direction_input(uart->rts_pin);
> > }
> > #endif
> >diff --git a/include/asm-blackfin/mach-bf537/bfin_serial_5xx.h
> >b/include/asm-blackfin/mach-bf537/bfin_serial_5xx.h
> >index 8f5d9c4..6fb328f 100644
> >--- a/include/asm-blackfin/mach-bf537/bfin_serial_5xx.h
> >+++ b/include/asm-blackfin/mach-bf537/bfin_serial_5xx.h
> >@@ -1,5 +1,6 @@
> > #include <linux/serial.h>
> > #include <asm/dma.h>
> >+#include <asm/portmux.h>
> >
> > #define NR_PORTS 2
> >
> >@@ -122,25 +123,29 @@ struct bfin_serial_res bfin_serial_resource[] = {
> >
> > int nr_ports = ARRAY_SIZE(bfin_serial_resource);
> >
> >+#define DRIVER_NAME "bfin-uart"
> >+
> > static void bfin_serial_hw_init(struct bfin_serial_port *uart)
> > {
> >- unsigned short val;
> >- val = bfin_read16(BFIN_PORT_MUX);
> >- val &= ~(PFDE | PFTE);
> >- bfin_write16(BFIN_PORT_MUX, val);
> >
> >- val = bfin_read16(PORTF_FER);
> >- val |= 0xF;
> >- bfin_write16(PORTF_FER, val);
> >+#ifdef CONFIG_SERIAL_BFIN_UART0
> >+ peripheral_request(P_UART0_TX, DRIVER_NAME);
> >+ peripheral_request(P_UART0_RX, DRIVER_NAME);
> >+#endif
> >+
> >+#ifdef CONFIG_SERIAL_BFIN_UART1
> >+ peripheral_request(P_UART1_TX, DRIVER_NAME);
> >+ peripheral_request(P_UART1_RX, DRIVER_NAME);
> >+#endif
> >
> > #ifdef CONFIG_SERIAL_BFIN_CTSRTS
> > if (uart->cts_pin >= 0) {
> >- gpio_request(uart->cts_pin, NULL);
> >+ gpio_request(uart->cts_pin, DRIVER_NAME);
> > gpio_direction_input(uart->cts_pin);
> > }
> >
> > if (uart->rts_pin >= 0) {
> >- gpio_request(uart->rts_pin, NULL);
> >+ gpio_request(uart->rts_pin, DRIVER_NAME);
> > gpio_direction_output(uart->rts_pin);
> > }
> > #endif
> >diff --git a/include/asm-blackfin/mach-bf537/portmux.h b/include/asm-
> >blackfin/mach-bf537/portmux.h
> >index 23e13c5..7daa247 100644
> >--- a/include/asm-blackfin/mach-bf537/portmux.h
> >+++ b/include/asm-blackfin/mach-bf537/portmux.h
> >@@ -106,4 +106,4 @@
> > #define P_SPI0_SSEL2 (P_DEFINED | P_IDENT(PORT_PJ11) | P_FUNCT(1))
> > #define P_SPI0_SSEL7 (P_DEFINED | P_IDENT(PORT_PJ5) | P_FUNCT(2))
> >
> >-#endif /* _MACH_PORTMUX_H_ */
> >+#endif /* _MACH_PORTMUX_H_ */
> >diff --git a/include/asm-blackfin/mach-bf561/bfin_serial_5xx.h
> >b/include/asm-blackfin/mach-bf561/bfin_serial_5xx.h
> >index e043caf..69b9f8e 100644
> >--- a/include/asm-blackfin/mach-bf561/bfin_serial_5xx.h
> >+++ b/include/asm-blackfin/mach-bf561/bfin_serial_5xx.h
> >@@ -1,5 +1,6 @@
> > #include <linux/serial.h>
> > #include <asm/dma.h>
> >+#include <asm/portmux.h>
> >
> > #define NR_PORTS 1
> >
> >@@ -92,18 +93,24 @@ struct bfin_serial_res bfin_serial_resource[] = {
> > }
> > };
> >
> >+#define DRIVER_NAME "bfin-uart"
> >
> > int nr_ports = NR_PORTS;
> > static void bfin_serial_hw_init(struct bfin_serial_port *uart)
> > {
> >
> >+#ifdef CONFIG_SERIAL_BFIN_UART0
> >+ peripheral_request(P_UART0_TX, DRIVER_NAME);
> >+ peripheral_request(P_UART0_RX, DRIVER_NAME);
> >+#endif
> >+
> > #ifdef CONFIG_SERIAL_BFIN_CTSRTS
> > if (uart->cts_pin >= 0) {
> >- gpio_request(uart->cts_pin, NULL);
> >+ gpio_request(uart->cts_pin, DRIVER_NAME);
> > gpio_direction_input(uart->cts_pin);
> > }
> > if (uart->rts_pin >= 0) {
> >- gpio_request(uart->rts_pin, NULL);
> >+ gpio_request(uart->rts_pin, DRIVER_NAME);
> > gpio_direction_input(uart->rts_pin);
> > }
> > #endif
> >--
> >1.5.2
>

2007-08-17 18:27:50

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 12/12] Blackfin serial driver: use new GPIO API

On Tuesday 07 August 2007, Bryan Wu wrote:
> -???????/* Enable UART0 RX and TX on pin 7 & 8 of PORT E */
> -???????bfin_write_PORTE_FER(0x180 | bfin_read_PORTE_FER());
> -???????bfin_write_PORTE_MUX(0x3C000 | bfin_read_PORTE_MUX());
> +???????peripheral_request(P_UART0_TX, DRIVER_NAME);
> +???????peripheral_request(P_UART0_RX, DRIVER_NAME);


This is not a GPIO API, so the patch summary and description
are wrong. That's pin muxing. You didn't change any of
the GPIO related calls.


2007-08-17 18:28:09

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 01/12] Blackfin arch: add peripheral resource allocation support

On Tuesday 07 August 2007, Bryan Wu wrote:
> From: Michael Hennerich <[email protected]>

The patch description here is IMO misleading, and is clearly
weak-to-nonexistent ... what this patch does is

* Start tracking the label strings provided by gpio_request()
* Provide a new portmux mechanisms
* Start using those in the serial support code

When I read "resource allocation" I think of "struct resource"
from <linux/ioport.h>, allocate_resource(), and so on. So while
it's true there are other kinds of driver resource, it's rather
unnatural for me to think about pin mux and gpio issues in any
terms other than chip and board setup.


> +static int cmp_label(unsigned short ident, const char *label)
> +{
> + if (label && str_ident)
> + return strncmp(str_ident + ident * RESOURCE_LABEL_SIZE,
> + label, strlen(label));
> + else
> + return -EINVAL;
> +}

GRPIO labels are purely for diagnostics. There's no reason to
compare one to another. You seem to be using these for purposes
in addition to GPIOs though ... probably worth commenting on that
unusual scheme.


> +int peripheral_request(unsigned short per, const char *label)
> +{
> + ...
> +
> + if (unlikely(reserved_peri_map[gpio_bank(ident)] & gpio_bit(ident))) {
> +
> + /*
> + * Pin functions like AMC address strobes my
> + * be requested and used by several drivers
> + */
> +
> + if (!(per & P_MAYSHARE)) {

Goofy indentation. And as a rule, drivers have been kept out of
the business of configuring pin usage. It's simpler that way;
they don't need to try coping with configuration errors like two
drivers wanting conflicting usage ... or as you say above, needing
some explicit sharing mechanism ...


> +
> + /*
> + * Allow that the identical pin function can
> + * be requested from the same driver twice
> + */

... or as you say here, needing to structure themselves so they
don't configure the same usage more than once ...


That said, how you handle pinmux on Blackfin is your business.

But you should know that this approach seems idiosyncratic and
more complex than needed: when pin config is done early and as
part of board setup, drivers don't need to care about it or to
handle any pinmux errors. And heck, products can sometimes be
shipped with the bootloader having done all pinmux setup, so
Linux won't need to worry about it at all. That can help ship
multiple board revisions using the same kernel.

- Dave



2007-08-17 18:28:31

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

Again, the patch descriptions need work. This changes the
IRQ code (to add those labels). $SUBJECT doesn't mention IRQs,
neither does the description ...


On Tuesday 07 August 2007, Bryan Wu wrote:
> --- a/arch/blackfin/mach-common/ints-priority-dc.c
> +++ b/arch/blackfin/mach-common/ints-priority-dc.c
> @@ -221,7 +221,7 @@ static unsigned int bf561_gpio_irq_startup(unsigned int irq)
> ?
> ????????if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
> ?
> -???????????????ret = gpio_request(gpionr, NULL);
> +???????????????ret = gpio_request(gpionr, "IRQ");
> ????????????????if (ret)
> ????????????????????????return ret;
> ?
> @@ -261,7 +261,7 @@ static int bf561_gpio_irq_type(unsigned int irq, unsigned int type)
> ?
> ????????????????if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
> ?
> -???????????????????????ret = gpio_request(gpionr, NULL);
> +???????????????????????ret = gpio_request(gpionr, "IRQ");
> ????????????????????????if (ret)
> ????????????????????????????????return ret;
> ?

Just for the record, this is an unusual way to use these calls.

Other platforms completely decouple these issues from the
IRQ infrastructure ... doing the pinmux and gpio claiming
separately from the request_irq()/free_irq() paths, mostly
as part of board setup. Doing all of that "early":

- keeps those error returns from causing hard-to-track-down
runtime bugs;

- works always, even on platforms where a given IRQ may
appear on any of several pins/balls;

- makes it easier to cross-check against board schematics,
by keeping most board-specific setup in one source file;

- shrinks the kernel's runtime footprint;

- allows the label to be more descriptive ... describeing
exactly *which* IRQ, so that using the labels for better
diagnostics actually gives better diagnostics.

Again, not "wrong"; but probably sub-optimal. You might
want to move towards earlier binding now, while Linux is
still young on Blackfin and you don't have legacy code to
worry about.

- Dave

2007-08-17 19:45:47

by Mike Frysinger

[permalink] [raw]
Subject: Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

On 8/17/07, David Brownell <[email protected]> wrote:
> Again, the patch descriptions need work. This changes the
> IRQ code (to add those labels). $SUBJECT doesn't mention IRQs,
> neither does the description ...
>
>
> On Tuesday 07 August 2007, Bryan Wu wrote:
> > --- a/arch/blackfin/mach-common/ints-priority-dc.c
> > +++ b/arch/blackfin/mach-common/ints-priority-dc.c
> > @@ -221,7 +221,7 @@ static unsigned int bf561_gpio_irq_startup(unsigned int irq)
> >
> > if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
> >
> > -ret = gpio_request(gpionr, NULL);
> > +ret = gpio_request(gpionr, "IRQ");
> > if (ret)
> > return ret;
> >
> > @@ -261,7 +261,7 @@ static int bf561_gpio_irq_type(unsigned int irq, unsigned int type)
> >
> > if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
> >
> > -ret = gpio_request(gpionr, NULL);
> > +ret = gpio_request(gpionr, "IRQ");
> > if (ret)
> > return ret;
> >
>
> Just for the record, this is an unusual way to use these calls.
>
> Other platforms completely decouple these issues from the
> IRQ infrastructure ... doing the pinmux and gpio claiming
> separately from the request_irq()/free_irq() paths, mostly
> as part of board setup. Doing all of that "early":
>
> - keeps those error returns from causing hard-to-track-down
> runtime bugs;
>
> - works always, even on platforms where a given IRQ may
> appear on any of several pins/balls;
>
> - makes it easier to cross-check against board schematics,
> by keeping most board-specific setup in one source file;
>
> - shrinks the kernel's runtime footprint;
>
> - allows the label to be more descriptive ... describeing
> exactly *which* IRQ, so that using the labels for better
> diagnostics actually gives better diagnostics.
>
> Again, not "wrong"; but probably sub-optimal. You might
> want to move towards earlier binding now, while Linux is
> still young on Blackfin and you don't have legacy code to
> worry about.

in the Blackfin port, if you want to use a pin as an IRQ rather than
GPIO, you use the normal request_irq/free_irq API ... those functions
will call back into the proper GPIO/PORTMUX code so that the pin is
setup properly. this is done so that code isnt duplicated across
files and so that we can easily detect if someone does something
incorrect like try to take the same pin and use it as
irq/gpio/whatever at the same time ...

are you saying that other ports dont unify the backend code paths at all ?
-mike

2007-08-17 20:02:42

by Hennerich, Michael

[permalink] [raw]
Subject: RE: [PATCH 01/12] Blackfin arch: add peripheral resource allocation support

Hi Dave,

>-----Original Message-----
>From: David Brownell [mailto:[email protected]]
>Sent: Freitag, 17. August 2007 20:12
>To: Bryan Wu
>Cc: [email protected]; [email protected];
>[email protected]; Michael Hennerich
>Subject: Re: [PATCH 01/12] Blackfin arch: add peripheral resource
>allocation support
>
>On Tuesday 07 August 2007, Bryan Wu wrote:
>> From: Michael Hennerich <[email protected]>
>
>The patch description here is IMO misleading, and is clearly
>weak-to-nonexistent ... what this patch does is
>
> * Start tracking the label strings provided by gpio_request()
> * Provide a new portmux mechanisms
> * Start using those in the serial support code
>

Right - our patch descriptions needs to be worked on.


>When I read "resource allocation" I think of "struct resource"
>from <linux/ioport.h>, allocate_resource(), and so on. So while
>it's true there are other kinds of driver resource, it's rather
>unnatural for me to think about pin mux and gpio issues in any
>terms other than chip and board setup.

Let me explain a bit. On some Blackfin derivatives almost all PINs can
be GPIOs besides up to 4 alternative functions. For a well experienced
systems engineer being the same time the same guy who does the Hardware
and the Software this is not an issue.
We provide all kind of drivers utilizing almost any peripheral on
Blackfin.
While potentially causing conflicting usage, for someone without
detailed hardware knowledge. The platform device board file is a good
thing to track conflicting memory or IO space resources as well as IRQs.
We also utilize platform device files for exactly these purposes.

The dynamic resource allocation for pinmux and gpio seems to us the best
way to handle things. The "resource allocation" mechanism will spill an
error and dump in case conflicting usage is detected. It'll also tell
you who is causing the conflicting usage.

>
>
>> +static int cmp_label(unsigned short ident, const char *label)
>> +{
>> + if (label && str_ident)
>> + return strncmp(str_ident + ident * RESOURCE_LABEL_SIZE,
>> + label, strlen(label));
>> + else
>> + return -EINVAL;
>> +}
>
>GRPIO labels are purely for diagnostics. There's no reason to
>compare one to another. You seem to be using these for purposes
>in addition to GPIOs though ... probably worth commenting on that
>unusual scheme.

You are right - diagnostics:
Telling who claimed my resource.
In addition getting a signature, allowing double allocation.

Some drives provide the option for a simple callback function exported
though the platform device file, in order to toggle a GPIO powering up
some external device. Without some additional global external flag it's
pretty had to maintain whether this gpio was allocated before.
In this case I prefer to allow double allocation, for the same purpose.


>
>
>> +int peripheral_request(unsigned short per, const char *label)
>> +{
>> + ...
>> +
>> + if (unlikely(reserved_peri_map[gpio_bank(ident)] &
gpio_bit(ident)))
>{
>> +
>> + /*
>> + * Pin functions like AMC address strobes my
>> + * be requested and used by several drivers
>> + */
>> +
>> + if (!(per & P_MAYSHARE)) {
>
>Goofy indentation. And as a rule, drivers have been kept out of
>the business of configuring pin usage. It's simpler that way;
>they don't need to try coping with configuration errors like two
>drivers wanting conflicting usage ... or as you say above, needing
>some explicit sharing mechanism ...


We define some PINs or better single PIN functions to be may shared.
This is only for PINs where the sharing of the function is in nature.
Think about an address strobe or a bus (Busy/Wait) signal, used by
several
drivers/devices sharing the same bus.


>
>
>> +
>> + /*
>> + * Allow that the identical pin function can
>> + * be requested from the same driver twice
>> + */
>
>... or as you say here, needing to structure themselves so they
>don't configure the same usage more than once ...


Same as explained above - this is only for these spots where the
request/free scheme doesn't work.

>
>
>That said, how you handle pinmux on Blackfin is your business.
>
>But you should know that this approach seems idiosyncratic and
>more complex than needed: when pin config is done early and as
>part of board setup, drivers don't need to care about it or to
>handle any pinmux errors. And heck, products can sometimes be
>shipped with the bootloader having done all pinmux setup, so
>Linux won't need to worry about it at all. That can help ship
>multiple board revisions using the same kernel.

This works for fixed function boards. But not for development boards
where we provide lego like add on cards, and allow people to connect
their homebrewn hardware.

Most people/customers I cope with, use the boot loader to only boot the
Linux kernel. The hardware setup we default the processor in the boot
loader might not fit their applications needs.

-Michael

>
>- Dave
>

2007-08-17 20:09:37

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

On Friday 17 August 2007, Mike Frysinger wrote:
> On 8/17/07, David Brownell <[email protected]> wrote:
> > ...
> > Just for the record, this is an unusual way to use these calls.
> >
> > Other platforms completely decouple these issues from the
> > IRQ infrastructure ... doing the pinmux and gpio claiming
> > separately from the request_irq()/free_irq() paths, mostly
> > as part of board setup. Doing all of that "early":
> >
> > - keeps those error returns from causing hard-to-track-down
> > runtime bugs;
> >
> > - works always, even on platforms where a given IRQ may
> > appear on any of several pins/balls;
> >
> > - makes it easier to cross-check against board schematics,
> > by keeping most board-specific setup in one source file;
> >
> > - shrinks the kernel's runtime footprint;
> >
> > - allows the label to be more descriptive ... describeing
> > exactly *which* IRQ, so that using the labels for better
> > diagnostics actually gives better diagnostics.
> >
> > Again, not "wrong"; but probably sub-optimal. You might
> > want to move towards earlier binding now, while Linux is
> > still young on Blackfin and you don't have legacy code to
> > worry about.
>
> in the Blackfin port, if you want to use a pin as an IRQ rather than
> GPIO, you use the normal request_irq/free_irq API ... those functions
> will call back into the proper GPIO/PORTMUX code so that the pin is
> setup properly. this is done so that code isnt duplicated across
> files and so that we can easily detect if someone does something
> incorrect like try to take the same pin and use it as
> irq/gpio/whatever at the same time ...
>
> are you saying that other ports dont unify the backend code paths at all ?

Some platforms try to "unify" the pin setup in the boot loader.
Most of them cope with bogus bootloaders by doing it in the board
setup code.

I don't know of any who try to do it "late" as you summarized.

See above why "late" unification is not necessarily as good as
"early" unification.

And then there's the OMAP1 example, where for example you might
know that you want MPUIO-0 but that's insufficient to tell whether
you must mux ball F18 or R13 ... so it's impossible to do the kind
of "late" unification done here, and pinmux *MUST* be separate from
IRQ setup.

- Dave


2007-08-17 20:19:47

by Mike Frysinger

[permalink] [raw]
Subject: Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

On 8/17/07, David Brownell <[email protected]> wrote:
> On Friday 17 August 2007, Mike Frysinger wrote:
> > On 8/17/07, David Brownell <[email protected]> wrote:
> > > ...
> > > Just for the record, this is an unusual way to use these calls.
> > >
> > > Other platforms completely decouple these issues from the
> > > IRQ infrastructure ... doing the pinmux and gpio claiming
> > > separately from the request_irq()/free_irq() paths, mostly
> > > as part of board setup. Doing all of that "early":
> > >
> > > - keeps those error returns from causing hard-to-track-down
> > > runtime bugs;
> > >
> > > - works always, even on platforms where a given IRQ may
> > > appear on any of several pins/balls;
> > >
> > > - makes it easier to cross-check against board schematics,
> > > by keeping most board-specific setup in one source file;
> > >
> > > - shrinks the kernel's runtime footprint;
> > >
> > > - allows the label to be more descriptive ... describeing
> > > exactly *which* IRQ, so that using the labels for better
> > > diagnostics actually gives better diagnostics.
> > >
> > > Again, not "wrong"; but probably sub-optimal. You might
> > > want to move towards earlier binding now, while Linux is
> > > still young on Blackfin and you don't have legacy code to
> > > worry about.
> >
> > in the Blackfin port, if you want to use a pin as an IRQ rather than
> > GPIO, you use the normal request_irq/free_irq API ... those functions
> > will call back into the proper GPIO/PORTMUX code so that the pin is
> > setup properly. this is done so that code isnt duplicated across
> > files and so that we can easily detect if someone does something
> > incorrect like try to take the same pin and use it as
> > irq/gpio/whatever at the same time ...
> >
> > are you saying that other ports dont unify the backend code paths at all ?
>
> Some platforms try to "unify" the pin setup in the boot loader.
> Most of them cope with bogus bootloaders by doing it in the board
> setup code.
>
> I don't know of any who try to do it "late" as you summarized.
>
> See above why "late" unification is not necessarily as good as
> "early" unification.
>
> And then there's the OMAP1 example, where for example you might
> know that you want MPUIO-0 but that's insufficient to tell whether
> you must mux ball F18 or R13 ... so it's impossible to do the kind
> of "late" unification done here, and pinmux *MUST* be separate from
> IRQ setup.

sorry, i'm not familiar at all with anything OMAP, but the Blackfin
architecture is such that all of its pins can be flipped on the fly
between modes ... i have seen once or twice some drivers which
actually needed this ability at runtime as they were muxing things
(maybe our fault there ... give someone the ability and they'll use
it).

as Michael pointed out, in the Blackfin world we tend to keep things
very dynamic as we have dev systems which allow for dropping in of
optional cards at will, so doing this in the bootloader is way too
inflexible.
-mike

2007-08-17 20:21:51

by Mike Frysinger

[permalink] [raw]
Subject: Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

On 8/17/07, Mike Frysinger <[email protected]> wrote:
> as Michael pointed out, in the Blackfin world we tend to keep things
> very dynamic as we have dev systems which allow for dropping in of
> optional cards at will, so doing this in the bootloader is way too
> inflexible.

oh, and another [smallish] data point. the Blackfin processor has a
small bootrom on it that could be likened to a very micro bios. so
it's possible to actually boot the linux kernel straight without a
boot loader. send the kernel over the UART to a Blackfin and watch it
go go go :)
-mike

2007-08-17 20:26:43

by Hennerich, Michael

[permalink] [raw]
Subject: RE: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API



>-----Original Message-----
>From: David Brownell [mailto:[email protected]]
>
>On Friday 17 August 2007, Mike Frysinger wrote:
>> On 8/17/07, David Brownell <[email protected]> wrote:
>> > ...
>> > Just for the record, this is an unusual way to use these calls.
>> >
>> > Other platforms completely decouple these issues from the
>> > IRQ infrastructure ... doing the pinmux and gpio claiming
>> > separately from the request_irq()/free_irq() paths, mostly
>> > as part of board setup. Doing all of that "early":
>> >
>> > - keeps those error returns from causing hard-to-track-down
>> > runtime bugs;
>> >
>> > - works always, even on platforms where a given IRQ may
>> > appear on any of several pins/balls;
>> >
>> > - makes it easier to cross-check against board schematics,
>> > by keeping most board-specific setup in one source file;
>> >
>> > - shrinks the kernel's runtime footprint;
>> >
>> > - allows the label to be more descriptive ... describeing
>> > exactly *which* IRQ, so that using the labels for better
>> > diagnostics actually gives better diagnostics.
>> >
>> > Again, not "wrong"; but probably sub-optimal. You might
>> > want to move towards earlier binding now, while Linux is
>> > still young on Blackfin and you don't have legacy code to
>> > worry about.
>>
>> in the Blackfin port, if you want to use a pin as an IRQ rather than
>> GPIO, you use the normal request_irq/free_irq API ... those functions
>> will call back into the proper GPIO/PORTMUX code so that the pin is
>> setup properly. this is done so that code isnt duplicated across
>> files and so that we can easily detect if someone does something
>> incorrect like try to take the same pin and use it as
>> irq/gpio/whatever at the same time ...
>>
>> are you saying that other ports dont unify the backend code paths at
all
>?
>
>Some platforms try to "unify" the pin setup in the boot loader.
>Most of them cope with bogus bootloaders by doing it in the board
>setup code.
>
>I don't know of any who try to do it "late" as you summarized.
>
>See above why "late" unification is not necessarily as good as
>"early" unification.
>
>And then there's the OMAP1 example, where for example you might
>know that you want MPUIO-0 but that's insufficient to tell whether
>you must mux ball F18 or R13 ... so it's impossible to do the kind
>of "late" unification done here, and pinmux *MUST* be separate from
>IRQ setup.

Dave,

We are not talking about PIN routing. One physical PIN/BALL can only
have one dedicated function the same time. It's more about telling about
possible conflicts, on a development board level.

What Mike wants to point out is that a external IRQ is first a GPIO and
needs to be configured like an INPUT GPIO and then a specific bit needs
to be set unmask it as IRQ.

So why not use the GPIO infrastructure to setup this pin as GPIO?

-Michael




>
>- Dave

2007-08-17 21:30:33

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 01/12] Blackfin arch: add peripheral resource allocation support

On Friday 17 August 2007, Hennerich, Michael wrote:
> Hi Dave,
>

> Right - our patch descriptions needs to be worked on.

Yes, please ... that makes reviewing easier!


> For a well experienced
> systems engineer being the same time the same guy who does the Hardware
> and the Software this is not an issue.

I guess I've rarely come across job descriptions like that.
Lowlevel software folk need to be able to use schematics and
often test equipment, but not design product circuits ... and
circuit designers rarely have responsibility to ship software.

On the other hand, maybe you want your "typical" customer to
be more of a systems integrator than anything else.


> We provide all kind of drivers utilizing almost any peripheral on
> Blackfin.

Chip vendors supporting Linux drivers for all their hardware.
What a pleasant change! :)


> While potentially causing conflicting usage, for someone without
> detailed hardware knowledge. The platform device board file is a good
> thing to track conflicting memory or IO space resources as well as IRQs.
> We also utilize platform device files for exactly these purposes.
>
> The dynamic resource allocation for pinmux and gpio seems to us the best
> way to handle things. The "resource allocation" mechanism will spill an
> error and dump in case conflicting usage is detected. It'll also tell
> you who is causing the conflicting usage.

That's your call, of course. I was pointing out why the "early"
binding of pin resources is the more usual strategy with Linux.
A "late" strategy is a bit surprising, and has its own issues.


> >That said, how you handle pinmux on Blackfin is your business.
> >
> >But you should know that this approach seems idiosyncratic and
> >more complex than needed: when pin config is done early and as
> >part of board setup, drivers don't need to care about it or to
> >handle any pinmux errors. And heck, products can sometimes be
> >shipped with the bootloader having done all pinmux setup, so
> >Linux won't need to worry about it at all. That can help ship
> >multiple board revisions using the same kernel.
>
> This works for fixed function boards.

That is, for typical products embedding Linux...


> But not for development boards
> where we provide lego like add on cards, and allow people to connect
> their homebrewn hardware.

Development boards are usually run differently than product
boards. All that flexibility in the development boards is
not necessarily a feature in the product version; it costs
space and time, which the application may need. Being able
to shift costs *early* and then drop them at runtime is a
useful strategy to apply in most places.

And heck -- most development setups get used only with one
card stack at a time, and it's easy to install new kernels
for new stacks.

- Dave

2007-08-17 21:30:56

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

On Friday 17 August 2007, Mike Frysinger wrote:

> as Michael pointed out, in the Blackfin world we tend to keep things
> very dynamic as we have dev systems which allow for dropping in of
> optional cards at will, so doing this in the bootloader is way too
> inflexible.

That's the tradeoff: optimize for development boards, or
instead for more fixed-function product boards.


> oh, and another [smallish] data point. ?the Blackfin processor has a
> small bootrom on it that could be likened to a very micro bios. ?so
> it's possible to actually boot the linux kernel straight without a
> boot loader. ?send the kernel over the UART to a Blackfin and watch it
> go go go :)

That's not uncommon, although I'm more used to seeing the
on-chip ROMs relying on a second stage loader for stuff like
getting memory and other clocks going at optimal speeds,
and loading "big" images (that won't fit on-chip SRAM).

- Dave

2007-08-17 21:31:26

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

On Friday 17 August 2007, Hennerich, Michael wrote:
> What Mike wants to point out is that a external IRQ is first a GPIO and
> needs to be configured like an INPUT GPIO and then a specific bit needs
> to be set unmask it as IRQ.
>
> So why not use the GPIO infrastructure to setup this pin as GPIO?

My comments about the advantages of using that infrastructure
for *early* binding captured the key points ... it's "failfast".

For IRQs you're probably on decently firm ground, since it's
extremely rare that people not handle request_irq() errors.

Remember, I just pointed out that the "late fail" strategy
is unusual. That doesn't mean it's wrong ... just it'll be
a bit of surprise, some cognitive dissonance to developers
picking up a Blackfin project, potentially more error prone.

- Dave

2007-08-17 21:54:19

by Robin Getz

[permalink] [raw]
Subject: Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

On Fri 17 Aug 2007 14:24, David Brownell pondered:
> Just for the record, this is an unusual way to use these calls.

That is part of the natural evolution of the kernel isn't it - per James's
keynote at OLS - you release something, and see how people [ab]use it until
it either grows, evolves, or it dies.

> Other platforms completely decouple these issues from the
> IRQ infrastructure ... doing the pinmux and gpio claiming
> separately from the request_irq()/free_irq() paths, mostly
> as part of board setup. Doing all of that "early":

is early:
- early in the kernel?
- early before the kernel? (in the bootloader).

> - keeps those error returns from causing hard-to-track-down
> runtime bugs;

The current Blackfin implementation causes a run time message:
"the pin xxxx driver requested, was already claimed by yyy driver".

I don't think that is too bad?

> - works always, even on platforms where a given IRQ may
> appear on any of several pins/balls;

But requires custom bootloaders or board setup for every hardware platform?
Most of our users would not like that, since they do as you say - use the
same kernel - with different drivers on multiple platforms.

> - makes it easier to cross-check against board schematics,
> by keeping most board-specific setup in one source file;

Yes - but we are not talking about muxing a common peripheral (like a single
UART) out many different pins (A or B or C). The UART pins are fixed. If you
want the UART, you need to use pin A. If you want to use the I2C that also
sits on pin A, you will get the message:
"pin A, requested by I2C, was already claimed by UART driver".

> - shrinks the kernel's runtime footprint;

I agree - making things more flexible/easier to use - is normally more
complex/larger/slower. (I know - easier to use is a matter of opinion). Since
this is normally done once, in _init functions, I'm not sure that makes much
of a difference here.

> - allows the label to be more descriptive ... describeing
> exactly *which* IRQ, so that using the labels for better
> diagnostics actually gives better diagnostics.

I'm not sure what you mean?

> Again, not "wrong"; but probably sub-optimal. You might
> want to move towards earlier binding now, while Linux is
> still young on Blackfin and you don't have legacy code to
> worry about.

Our overall goal is to keep as much code - including bootloader - platform
agnostic, and not require people to write any of code/configuration data to
boot up something, and get things working in a semi-standard manner.

This still has it's limits - which is why we publish all our hardware designs.
If you implement things the similar way (because for the most part it is
fixed by the processor designer) - the bootloader/kernel/driver will just
work.

I would rather force a little extra complexity on me (as a kernel developer)
than have to answer thousands of questions from end users, who are trying to
move the kernel onto their hardware.

-Robin

2007-08-17 22:02:57

by Hennerich, Michael

[permalink] [raw]
Subject: RE: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API



>-----Original Message-----
>From: David Brownell [mailto:[email protected]]
>
>On Friday 17 August 2007, Hennerich, Michael wrote:
>> What Mike wants to point out is that a external IRQ is first a GPIO
and
>> needs to be configured like an INPUT GPIO and then a specific bit
needs
>> to be set unmask it as IRQ.
>>
>> So why not use the GPIO infrastructure to setup this pin as GPIO?
>
>My comments about the advantages of using that infrastructure
>for *early* binding captured the key points ... it's "failfast".
>
>For IRQs you're probably on decently firm ground, since it's
>extremely rare that people not handle request_irq() errors.
>
>Remember, I just pointed out that the "late fail" strategy
>is unusual. That doesn't mean it's wrong ... just it'll be
>a bit of surprise, some cognitive dissonance to developers
>picking up a Blackfin project, potentially more error prone.
>

Dave,

Thanks - we really appreciate your feedback.
Please believe me - since a great while we have similar internal
discussion how we should handle these things.

Things need to be DAU proof.

We rather prefer having some verbal runtime messages, than having a
system that doesn't do what expected and being silent.
(The bootloader doesn't know what kernel modules are being loaded
requiring specific HW setup)

We also don't fear the memory overhead (compared to the support
overhead), the runtime overhead is almost neglectable since these
functions are only called once, best case twice (module remove).

I see your points - I would prefer having a fix function board suiting
all our customers' needs - or something like an x86 system where
everything is fixed or dedicated and abstracted by IO/Memory and IRQ.

-Michael




>- Dave

2007-08-17 22:16:11

by Robin Getz

[permalink] [raw]
Subject: Re: [PATCH 01/12] Blackfin arch: add peripheral resource allocation support

On Fri 17 Aug 2007 17:10, David Brownell pondered:
> On the other hand, maybe you want your "typical" customer to
> be more of a systems integrator than anything else.

We are getting yelled at by our customers (I was on the phone yesterday),
because the kernel build environment we distribute (the default) was not
inside Eclipse, and someone couldn't push a button on a GUI rather than
typing "make".

While Linux and other open source software is free, for some taking advantage
of its benefits can require a significant investment in time. Linux is
powerful, rich, and flexible. It is these very characteristics that make
Linux so appealing that also create a significant level of complexity.

We are seeing more and more "first time buyers" jumping from bare metal - no
OS, to Linux. For them - the ease of not having to write configuration files
gives them a warm fuzzy feeling when they boot their board, and it
just "works".

> > While potentially causing conflicting usage, for someone without
> > detailed hardware knowledge. The platform device board file is a good
> > thing to track conflicting memory or IO space resources as well as
> > IRQs. We also utilize platform device files for exactly these purposes.
> >
> > The dynamic resource allocation for pinmux and gpio seems to us the
> > best way to handle things. The "resource allocation" mechanism will
> > spill an error and dump in case conflicting usage is detected. It'll
> > also tell you who is causing the conflicting usage.
>
> That's your call, of course. I was pointing out why the "early"
> binding of pin resources is the more usual strategy with Linux.
> A "late" strategy is a bit surprising, and has its own issues.

Agreed - Every implementation has its own issues, but based on what we were
being asked to do - it was the best way we could accommodate everyone.

> > >That said, how you handle pinmux on Blackfin is your business.
> > >
> > >But you should know that this approach seems idiosyncratic and
> > >more complex than needed: when pin config is done early and as
> > >part of board setup, drivers don't need to care about it or to
> > >handle any pinmux errors. And heck, products can sometimes be
> > >shipped with the bootloader having done all pinmux setup, so
> > >Linux won't need to worry about it at all. That can help ship
> > >multiple board revisions using the same kernel.
> >
> > This works for fixed function boards.
>
> That is, for typical products embedding Linux...

We have multiple customers shipping the same bootloader/kernel binary on
different products, and the only difference is the /etc/rc file - which
drivers they install, and a few things in userspace.

Could this be smaller - sure - but NAND is cheap (according to them) compared
to the effort and cost of maintaining and testing multiple kernel versions
for every product.

Could this be faster - sure - but it is done at init - and then never again.
We have a ~2-3 second boot time - maybe we shave off a few ms - things go
pretty fast at 600MHz.

-Robin

2007-08-17 22:34:44

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

On Friday 17 August 2007, Robin Getz wrote:
> On Fri 17 Aug 2007 14:24, David Brownell pondered:
> > Just for the record, this is an unusual way to use these calls.
>
> That is part of the natural evolution of the kernel isn't it - per James's
> keynote at OLS - you release something, and see how people [ab]use it until
> it either grows, evolves, or it dies.

Yep ... and it's worth knowing when you're doing
something different. Different isn't always worse,
isn't always better.


> > Other platforms completely decouple these issues from the
> > IRQ infrastructure ... doing the pinmux and gpio claiming
> > separately from the request_irq()/free_irq() paths, mostly
> > as part of board setup. Doing all of that "early":
>
> is early:
> - early in the kernel?
> - early before the kernel? (in the bootloader).

Both of those are "earlier", yes. Different product developers
may argue for either placement.


> > - keeps those error returns from causing hard-to-track-down
> > runtime bugs;
>
> The current Blackfin implementation causes a run time message:
> "the pin xxxx driver requested, was already claimed by yyy driver".
>
> I don't think that is too bad?

Given some product with a Blackfin chip, would you expect a
customer -- who may not even see the Linux bits!! -- to be
able to solve such problems? If it's not possible for such
problems to crop up in the field, product support (and field
troubleshooting) gets easier...


> > - works always, even on platforms where a given IRQ may
> > appear on any of several pins/balls;
>
> But requires custom bootloaders or board setup for every hardware platform?

One or both, yes. That's typical in embedded setups.
They're not necessarily all that different, but that
code does need to handle the hardware differences.


> Most of our users would not like that, since they do as you say - use the
> same kernel - with different drivers on multiple platforms.

I thought I referred to different revisions of one platform... :)


> > - makes it easier to cross-check against board schematics,
> > by keeping most board-specific setup in one source file;
>
> Yes - but we are not talking about muxing a common peripheral (like a single
> UART) out many different pins (A or B or C). The UART pins are fixed. If you
> want the UART, you need to use pin A. If you want to use the I2C that also
> sits on pin A, you will get the message:
> "pin A, requested by I2C, was already claimed by UART driver".

Not all platforms work that way though. There can often be several
options for where a given signal gets routed.

And then there are the errors where someone accidentally copies
something like "GPIO 29" to two places ... invariants like "only
one GPIO requestor at a time" are needed to turn up such stuff.


> > - shrinks the kernel's runtime footprint;
>
> I agree - making things more flexible/easier to use - is normally more
> complex/larger/slower. (I know - easier to use is a matter of opinion). Since
> this is normally done once, in _init functions, I'm not sure that makes much
> of a difference here.
>
> > - allows the label to be more descriptive ... describeing
> > exactly *which* IRQ, so that using the labels for better
> > diagnostics actually gives better diagnostics.
>
> I'm not sure what you mean?

The $SUBJECT patch uses the string "IRQ" in all cases.
But "smc_irq" and "codec_irq" would be more informative
as entries in a list of even just a handful of GPIOs.
And with a few dozen, I'd find "IRQ" not at all helpful.


> > Again, not "wrong"; but probably sub-optimal. You might
> > want to move towards earlier binding now, while Linux is
> > still young on Blackfin and you don't have legacy code to
> > worry about.
>
> Our overall goal is to keep as much code - including bootloader - platform
> agnostic, and not require people to write any of code/configuration data to
> boot up something, and get things working in a semi-standard manner.

The issue is just where those limits lie. IMO it's not at
all unreasonable to require board-specific code. External
chips will need board-specfic glue data in most cases (how
they're addressed, what IRQs they use, and so on); and you
may have drivers available that correspond to devices that
are not wired up on that particular hardware.


> This still has it's limits - which is why we publish all our hardware designs.
> If you implement things the similar way (because for the most part it is
> fixed by the processor designer) - the bootloader/kernel/driver will just
> work.

Sure ... you'd need to say "this board uses <these devices>"
and if integrated in the SOC that's often enough. External
devices need more configuration. Even for integrated ones,
that knowledge doesn't belong in the driver ... "which of the
many UARTS to use as console" isn't standard, and neither
is "what hardware handshaking pins are in use".


> I would rather force a little extra complexity on me (as a kernel developer)
> than have to answer thousands of questions from end users, who are trying to
> move the kernel onto their hardware.

Just remember that "Aunt Tilly" doesn't configure kernels.
And that even deeply technical people who do configure
them may not have the details fresh in mind a few months
later. ;)

- Dave


2007-08-17 22:46:38

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 01/12] Blackfin arch: add peripheral resource allocation support

On Friday 17 August 2007, Robin Getz wrote:
> On Fri 17 Aug 2007 17:10, David Brownell pondered:
> > On the other hand, maybe you want your "typical" customer to
> > be more of a systems integrator than anything else.
>
> We are getting yelled at by our customers (I was on the phone yesterday),
> because the kernel build environment we distribute (the default) was not
> inside Eclipse, and someone couldn't push a button on a GUI rather than
> typing "make".

Press the "enter" button after typing "make". ;)


> While Linux and other open source software is free, for some taking advantage
> of its benefits can require a significant investment in time. Linux is
> powerful, rich, and flexible. It is these very characteristics that make
> Linux so appealing that also create a significant level of complexity.

Yes.


> We are seeing more and more "first time buyers" jumping from bare metal - no
> OS, to Linux. For them - the ease of not having to write configuration files
> gives them a warm fuzzy feeling when they boot their board, and it
> just "works".

There's certainly a lot to be said for having things
just work the first time when you're making such a big
transition in tools. On the other hand, at some point
the training wheels need to come off!



> > > >That said, how you handle pinmux on Blackfin is your business.
> > > >
> > > >But you should know that this approach seems idiosyncratic and
> > > >more complex than needed: when pin config is done early and as
> > > >part of board setup, drivers don't need to care about it or to
> > > >handle any pinmux errors. And heck, products can sometimes be
> > > >shipped with the bootloader having done all pinmux setup, so
> > > >Linux won't need to worry about it at all. That can help ship
> > > >multiple board revisions using the same kernel.
> > >
> > > This works for fixed function boards.
> >
> > That is, for typical products embedding Linux...
>
> We have multiple customers shipping the same bootloader/kernel binary on
> different products, and the only difference is the /etc/rc file - which
> drivers they install, and a few things in userspace.

Be careful there. Remember that the driver model is predicated
on knowing the devices first, and *then* matching drivers. I
expect you *will* see problems if you get people thinking system
config comes from a "which driver" selection rather than "here's
the exact hardware that's available". Maybe configfs should be
used for device config.


> Could this be smaller - sure - but NAND is cheap (according to them) compared
> to the effort and cost of maintaining and testing multiple kernel versions
> for every product.
>
> Could this be faster - sure - but it is done at init - and then never again.
> We have a ~2-3 second boot time - maybe we shave off a few ms - things go
> pretty fast at 600MHz.

I'm more used to clock rates less than 1/3 that much. :)

- Dave


2007-08-18 19:07:37

by Robin Getz

[permalink] [raw]
Subject: Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

On Fri 17 Aug 2007 18:34, David Brownell pondered:
> On Friday 17 August 2007, Robin Getz wrote:
> > On Fri 17 Aug 2007 14:24, David Brownell pondered:
> > > Just for the record, this is an unusual way to use these calls.
> >
> > That is part of the natural evolution of the kernel isn't it - per
> > James's keynote at OLS - you release something, and see how people
> > [ab]use it until it either grows, evolves, or it dies.
>
> Yep ... and it's worth knowing when you're doing
> something different. Different isn't always worse,
> isn't always better.

No disagreements here.

> > > Other platforms completely decouple these issues from the
> > > IRQ infrastructure ... doing the pinmux and gpio claiming
> > > separately from the request_irq()/free_irq() paths, mostly
> > > as part of board setup. Doing all of that "early":
> >
> > is early:
> > - early in the kernel?
> > - early before the kernel? (in the bootloader).
>
> Both of those are "earlier", yes. Different product developers
> may argue for either placement.

Just like we say things are better/easier for us later.

> > > - keeps those error returns from causing hard-to-track-down
> > > runtime bugs;
> >
> > The current Blackfin implementation causes a run time message:
> > "the pin xxxx driver requested, was already claimed by yyy driver".
> >
> > I don't think that is too bad?
>
> Given some product with a Blackfin chip, would you expect a
> customer -- who may not even see the Linux bits!! -- to be
> able to solve such problems? If it's not possible for such
> problems to crop up in the field, product support (and field
> troubleshooting) gets easier...

Typically customers who are not familiar with the linux bits are not doing
modprobe either...

I don't see how early/late makes the problem easier/worse to debug. No matter
when you do it - the driver refuses to install (or at least should).

> > > - works always, even on platforms where a given IRQ may
> > > appear on any of several pins/balls;
> >
> > But requires custom bootloaders or board setup for every hardware
> > platform?
>
> One or both, yes. That's typical in embedded setups.
> They're not necessarily all that different, but that
> code does need to handle the hardware differences.

Right - for us - the code handing the hardware differences is easier in the
drivers, rather than the bootloaders.

For other systems - where you can have a UART on any pin - I completely
understand your point.

> > Most of our users would not like that, since they do as you say - use
> > the same kernel - with different drivers on multiple platforms.
>
> I thought I referred to different revisions of one platform... :)

You did - I was just saying that some of our customers don't do it the way you
were thinking.

> > > - makes it easier to cross-check against board schematics,
> > > by keeping most board-specific setup in one source file;
> >
> > Yes - but we are not talking about muxing a common peripheral (like a
> > single UART) out many different pins (A or B or C). The UART pins are
> > fixed. If you want the UART, you need to use pin A. If you want to use
> > the I2C that also sits on pin A, you will get the message:
> > "pin A, requested by I2C, was already claimed by UART driver".
>
> Not all platforms work that way though. There can often be several
> options for where a given signal gets routed.

But this is how it _always_ works on Blackfin. For other systems - like ARM,
where n+1 silicon manufactures are all implementing things differently - I
can understand your comments.


> > > - allows the label to be more descriptive ... describeing
> > > exactly *which* IRQ, so that using the labels for better
> > > diagnostics actually gives better diagnostics.
> >
> > I'm not sure what you mean?
>
> The $SUBJECT patch uses the string "IRQ" in all cases.
> But "smc_irq" and "codec_irq" would be more informative
> as entries in a list of even just a handful of GPIOs.
> And with a few dozen, I'd find "IRQ" not at all helpful.

I agree - things can always be more descriptive.

> > > Again, not "wrong"; but probably sub-optimal. You might
> > > want to move towards earlier binding now, while Linux is
> > > still young on Blackfin and you don't have legacy code to
> > > worry about.
> >
> > Our overall goal is to keep as much code - including bootloader -
> > platform agnostic, and not require people to write any of
> > code/configuration data to boot up something, and get things
> > working in a semi-standard manner.
>
> The issue is just where those limits lie. IMO it's not at
> all unreasonable to require board-specific code. External
> chips will need board-specfic glue data in most cases (how
> they're addressed, what IRQs they use, and so on); and you
> may have drivers available that correspond to devices that
> are not wired up on that particular hardware.
>
>
> > This still has it's limits - which is why we publish all our hardware
> > designs. If you implement things the similar way (because for the
> > most part it is fixed by the processor designer) - the
> > bootloader/kernel/driver will just work.
>
> Sure ... you'd need to say "this board uses <these devices>"
> and if integrated in the SOC that's often enough.

with the kernel .config - that is what happens. If you have 2 serial drivers
connected - you enable 2 serial drivers in Kconfig.

> External
> devices need more configuration. Even for integrated ones,
> that knowledge doesn't belong in the driver ... "which of the
> many UARTS to use as console" isn't standard, and neither
> is "what hardware handshaking pins are in use".

When hardware handshaking pins are fixed - it sure is. When they are not (when
the hardware doesn't support hardware handshaking, and you need to do it in
software) - we still allow you do to it via Kconfig.

linux-2.6.x/drivers/serial/Kconfig:

config UART0_CTS_PIN
int "UART0 CTS pin"
depends on BFIN_UART0_CTSRTS
default 23
help
The default pin is GPIO_GP7.
Refer to ./include/asm-blackfin/gpio.h to see the GPIO map.

config UART0_RTS_PIN
int "UART0 RTS pin"
depends on BFIN_UART0_CTSRTS
default 22
help
The default pin is GPIO_GP6.
Refer to ./include/asm-blackfin/gpio.h to see the GPIO map.

Board configs are in one place - under source control - the kernel .config

> > I would rather force a little extra complexity on me (as a kernel
> > developer) than have to answer thousands of questions from end
> > users, who are trying to move the kernel onto their hardware.
>
> Just remember that "Aunt Tilly" doesn't configure kernels.
> And that even deeply technical people who do configure
> them may not have the details fresh in mind a few months
> later. ;)

I wish some of our customers where as good as my aunt Tilly when it comes to
kernel config, or could remember as well as she can.

I guess we thought it was easier for people to select a few things in config,
rather than have to write C code/include files for board specific
implementations options - It is like you said - everything is all in one
place...

-Robin

2007-08-19 21:54:31

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

On Saturday 18 August 2007, Robin Getz wrote:

> I don't see how early/late makes the problem easier/worse to debug. No matter
> when you do it - the driver refuses to install (or at least should).

If you arrange to *reliably* detect the pinmux/setup problems by
the time the system starts ""init" (early), that means one large
class of hard-to-sort problems never needs runtime troubleshooting.

Think of it this way: folk have observed that pin setup issues can
be painful to sort out. So they adopt a strategy ("failfast"/"early")
which helps surface them early and basically removes them as issues
in later debugging. I think you're hoping that by adding extra
resource tracking code, you can make that later debugging easier
even though, by "late" binding, you've introduced extra error paths.


> Right - for us - the code handing the hardware differences is easier in the
> drivers, rather than the bootloaders.

Remember that I didn't argue in favor of putting that code into
boot loaders ... I just pointed out that some product lines work
that way, so Linux needs to cope with that strategy. (One of the
many examples involves OpenFirmware device tables.)

But regardless: I can't buy any argument that it's better to put
lots of board-specific code into drivers. That adds up quickly,
making maintaining the drivers painful. "Real" updates (bugfixes,
new features, API updates, cleanup, and so on) regularly end up
in conflict with patches to support a few more boards, and board
support patches must then always involve those driver maintainers.
So merging new boards involves many more people than necessary...


> For other systems - where you can have a UART on any pin - I completely
> understand your point.

UART on any pin? Few kernels dynamically reprogram FPGAs! :)


> > Sure ... you'd need to say "this board uses <these devices>"
> > and if integrated in the SOC that's often enough.
>
> with the kernel .config - that is what happens. If you have 2 serial drivers
> connected - you enable 2 serial drivers in Kconfig.

Your language is incorrect here. What your Kconfig does is
not configure two different *drivers* ... but some number of
different serial *devices* handled by the same driver.

One obvious downside of that is that making it needlessly hard
to support several boards with one kernel. As a rule, those
boards can have different serial devices, and the devices can
be configured differently. Yet you said you wanted to make it
easy to support many boards with one kernel...


> > External
> > devices need more configuration. Even for integrated ones,
> > that knowledge doesn't belong in the driver ... "which of the
> > many UARTs to use as console" isn't standard, and neither
> > is "what hardware handshaking pins are in use".
>
> When hardware handshaking pins are fixed - it sure is.

Not unless the UART for some odd reason *requires* those pins to work.

There's almost always support for pure software handshaking (XON/XOFF),
with one board option being "don't handshake". Board A might use two
pins for UART2 RTS/CTS; board B might use UART as well, but use those
two pins for another I2C bus. The differences belong in board-specific
configuration, not in drivers.


> When they are not (when
> the hardware doesn't support hardware handshaking, and you need to do it in
> software) - we still allow you do to it via Kconfig.
>
> linux-2.6.x/drivers/serial/Kconfig:

That can work, at least for *single-board* kernel builds. Of course,
that gets into territory some people will say is Kconfig abuse ...
and the need for many ugly #ifdefs is very obvious. :)

In fact one could argue that those bits of Kconfig syntax are really
just support for one Blackfin board (ezkit), and so they don't belong
in that Kconfig file or with those names...

Plus, that approach only works with fairly simple types of device glue.
It's routine to find chip hookups that can't fit smoothly into some
pre-planned Kconfig, since they require board-specific function hooks.
(Sometimes even with UARTs, but clearly not in this case.)


> Board configs are in one place - under source control - the kernel .config

And in arch/blackfin/mach-*/boards/*.c ... all that stuff you set up
in Kconfig could as easily have been coded in those files, without any
need for #ifdefs or confusing Kconfig. Still under source control,
plus it's a lot harder to break. :)


> I guess we thought it was easier for people to select a few things in config,
> rather than have to write C code/include files for board specific
> implementations options - It is like you said - everything is all in one
> place...

Doing that in Kconfig is atypical ... it may well be a bit easier to
pick up at the beginning of a developer's learning curve, but I think
it doesn't scale very well as multi-board product lines evolve.

- Dave

2007-08-20 01:56:13

by Robin Getz

[permalink] [raw]
Subject: Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

On Sun 19 Aug 2007 17:54, David Brownell pondered:
> On Saturday 18 August 2007, Robin Getz wrote:
>
> > I don't see how early/late makes the problem easier/worse to debug. No
> > matter when you do it - the driver refuses to install (or at least
> > should).
>
> If you arrange to *reliably* detect the pinmux/setup problems by
> the time the system starts ""init" (early), that means one large
> class of hard-to-sort problems never needs runtime troubleshooting.

Sure it does - it just needs to do it in the bootloader, not the kernel. You
haven't eliminated the problem - or made it any easier to debug - it is just
moved somewhere else. Again - this is not bad. We decided/are attempting to
do it in the kernel. Normally what we find is there are more kernel people on
a project than bootloader (whether this makes this easier - or not - is still
TDB :)

> Think of it this way: folk have observed that pin setup issues can
> be painful to sort out.

Absolutely - the problem that everyone is trying to solve - is how to do plug
and play, with no enumeration?

Doing it early in the bootloader is akin to historical PC solution where early
ISA PNP (shudder) filled out a table in memory, and passed it to the OS.

> So they adopt a strategy ("failfast"/"early")
> which helps surface them early and basically removes them as issues
> in later debugging.

When the kernel engineer runs into a problem because a driver won't load, is
it better that he can fix it himself (hopefully), or that they have to call
the person maintaining the bootloader? There are pro/cons of either. I can
see the value in both.

We choose to do it in the driver, not the bootloader.

> > Right - for us - the code handing the hardware differences is easier
> > in the drivers, rather than the bootloaders.
>
> Remember that I didn't argue in favor of putting that code into
> boot loaders ... I just pointed out that some product lines work
> that way, so Linux needs to cope with that strategy. (One of the
> many examples involves OpenFirmware device tables.)
>
> But regardless: I can't buy any argument that it's better to put
> lots of board-specific code into drivers.

I don't think we are putting board specific code in drivers (or if there is -
it should get removed).

I did a quick look, and the only place this has happened is in some of our
drivers that have not made it to main line yet - where we accidently put some
mtd_partitions in the drivers, rather than the boards file. I know we are
working on fixing this.

> That adds up quickly,
> making maintaining the drivers painful. "Real" updates (bugfixes,
> new features, API updates, cleanup, and so on) regularly end up
> in conflict with patches to support a few more boards, and board
> support patches must then always involve those driver maintainers.
> So merging new boards involves many more people than necessary...

I agree - which is why don't do this either. Board specific info does not go
into drivers. I think this is something that Michael, Bryan and others
working on the Blackfin arch & drivers will agree to 100%.

What we do is try to make the driver agnostic to the hardware as much as
possible, where hardware specifics (chip selects, IRQ, GPIO, etc) are managed
in Kconfig.

I can see the value of doing the initialisation in the bootloader - since this
would allow you have a common driver - and hardware customisation is done in
the bootloader.

>
> >
> > linux-2.6.x/drivers/serial/Kconfig:
>
> That can work, at least for *single-board* kernel builds. Of course,
> that gets into territory some people will say is Kconfig abuse ...
> and the need for many ugly #ifdefs is very obvious. :)

We have been trying to minimise that - and I think we have been doing a pretty
good job. There doesn't seem to be any platform specific ifdefs in our
drivers that are not abstracted out.

> Doing that in Kconfig is atypical ... it may well be a bit easier to
> pick up at the beginning of a developer's learning curve, but I think
> it doesn't scale very well as multi-board product lines evolve.

We have lots of end users (obviously not as many as ARM or PPC yet), and they
have not been complaining, in fact some say that this is easier to deploy.

But - I think we both agree - that what we are doing is just an alternative
implemention of hardware abstraction - that is different than the way that
some others are doing it. Not better/worse (from what I can tell) - it just
has different tradeoffs.

-Robin

2007-08-20 03:42:16

by David Brownell

[permalink] [raw]
Subject: Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

On Sunday 19 August 2007, Robin Getz wrote:
> On Sun 19 Aug 2007 17:54, David Brownell pondered:
> > On Saturday 18 August 2007, Robin Getz wrote:
> >
> > > I don't see how early/late makes the problem easier/worse to debug. No
> > > matter when you do it - the driver refuses to install (or at least
> > > should).
> >
> > If you arrange to *reliably* detect the pinmux/setup problems by
> > the time the system starts ""init" (early), that means one large
> > class of hard-to-sort problems never needs runtime troubleshooting.
>
> Sure it does - it just needs to do it in the bootloader, not the kernel.

Kernel arch/.../board-X.c code is the place to "reliably" handle
that. Bootloader is out-of-scope here; it's a separate codebase.


> You
> haven't eliminated the problem - or made it any easier to debug - it is just
> moved somewhere else.

Moved it out of runtime, to a small window before "init" sections
get discarded and "init" is invoked. So if it ever *could* show
up, it'll show up then ... every time the system boots, a message
will appear. Easy to notice while the bug is fresh, just from a
developer's routine scan of bootup messages.

The alternative lets the problem appear later, almost randomly,
based on whether the conflicting drivers happen to be loaded
at the same time. Which *IS* harder to debug.


I still remember the time a board had to be respun because the
hardware guys goofed on use of one signal. Two different drivers
both needed it ... but until later in system integration, they
were never tested together, so that conflict never showed up.
(It was a non-obvious thing, for on-chip signal routing not the
on-board type verified by module tests in hardware QA and design
review.)

Doing that setup "early" would have prevented that problem, and
avoided one annoying schedule slip and hardware respin.


> Again - this is not bad. We decided/are attempting to
> do it in the kernel. Normally what we find is there are more kernel people on
> a project than bootloader (whether this makes this easier - or not - is still
> TDB :)

I don't know why you're reading what I write as any kind of
preference for doing that in the bootloader. It's nothing
more than an *acknowledgement* that some vendors work that
way, so Linux is better off with an approach which won't be
broken when they do. (That is, always having finished pin
mux setup before drivers get probed.)

When Linux does that setup in arch/..../board-X.c files, mostly
before drivers come into play, it's normal for driver code to
ignore it ... which means the bootloader might also have done
it, for those kinds of product.

Usually I'd expect one person on the bootloader, plus ideally
understudies. Not a full time job; that person might mostly
do kernel development, hardware test/bringup, or something
else depending on how the product team was structured.


> > Remember that I didn't argue in favor of putting that code into
> > boot loaders ... I just pointed out that some product lines work
> > that way, so Linux needs to cope with that strategy. (One of the
> > many examples involves OpenFirmware device tables.)
> >
> > But regardless: I can't buy any argument that it's better to put
> > lots of board-specific code into drivers.
>
> I don't think we are putting board specific code in drivers (or if there is -
> it should get removed).
>
> I did a quick look, and the only place this has happened is in some of our
> drivers that have not made it to main line yet

Good!! If it's not in drivers, I suspect you'll agree that it
will probably all land in that arch/..../board-X.c setup code.


> I agree - which is why don't do this either. Board specific info does not go
> into drivers. I think this is something that Michael, Bryan and others
> working on the Blackfin arch & drivers will agree to 100%.
>
> What we do is try to make the driver agnostic to the hardware as much as
> possible,

Good ...

> where hardware specifics (chip selects, IRQ, GPIO, etc) are managed
> in Kconfig.

... IMO less good, but that's all your worry. I suspect that
by the time you handle a few dozen boards, you'll find Kconfig
is not well suited to this. (Because of the way it prevents one
kernel from handling multiple boards, unless they really aren't
very different.)


> > Doing that in Kconfig is atypical ... it may well be a bit easier to
> > pick up at the beginning of a developer's learning curve, but I think
> > it doesn't scale very well as multi-board product lines evolve.
>
> We have lots of end users (obviously not as many as ARM or PPC yet), and they
> have not been complaining, in fact some say that this is easier to deploy.

These are people already familiar with ARM or PPC embedded Linux?
I guess one question is "easier than what".

Kconfig doesn't try to help with the "give me a minimal .config
for <THIS> hardware" problem; say, by reading "lspci -n" output
or some other system description. It expects people to know their
way around. Maybe a bit of a "20 questions" style interface
comes across as providing useful hand-holding; some of that can
be done in Kconfig, but it's still a kind of menu maze.

I'd hope people don't still get degrees for writing automagic
system configuration tools. Even though it's still not easy. :)


> But - I think we both agree - that what we are doing is just an alternative
> implemention of hardware abstraction - that is different than the way that
> some others are doing it. Not better/worse (from what I can tell) - it just
> has different tradeoffs.

We could have that discussion in a few years. I suspect you'll
find less going into Kconfig and more into C code, if for no
other reason than to make sure things scale sanely. Or if lots
of stuff is in Kconfig ... it'll be in the arch part of the tree
associated with boards, not the driver parts.

- Dave