dm: imx: Add driver model support for GPIO and serial on cm_fx6

The existing ll_entry_declare() permits a single element of the list to be added to a linker list. Sometimes we want to add several objects at once. To avoid lots of messy declarations, add a macro to support this.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 include/linker_lists.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

The U_BOOT_DEVICE macro allows the declaration of a single U-Boot device. Add an equivalent macro to declare an array of devices, for convenience.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 include/dm/platdata.h | 4 ++++
 1 file changed, 4 insertions(+)

Now that some initcall functions return a useful error number, display it when something goes wrong.

Signed-off-by Simon Glass <[email protected]>

Acked-by Igor Grinberg <[email protected]>

---

Changes in v4: None
Changes in v3: None
Changes in v2:
- Add patch to display error number when an error occurs in initcall

 lib/initcall.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Avoid duplicating the code which deals with getc() and putc(). It is fairly simple, but may expand later.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/serial/serial-uclass.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

Since this function can fail, check its return value.

Signed-off-by Simon Glass <[email protected]>

Tested-by Nikita Kiryanov <[email protected]>

---

Changes in v4:
- Move SATA changes into the next patch

Changes in v3:
- Just warn when one of the board init stages fails

Changes in v2:
- Add new patch to add error checking to setup_i2c()

 arch/arm/imx-common/i2c-mxv7.c            | 24 +++++++++++++++----
 arch/arm/include/asm/imx-common/mxc_i2c.h |  4 ++--
 board/compulab/cm_fx6/cm_fx6.c            | 40 ++++++++++++++++++++++++++-----
 3 files changed, 55 insertions(+), 13 deletions(-)

arch/arm/imx-common/i2c-mxv7.c +19 -5

index a580873..70cff5c 100644
@@ -69,15 +69,29 @@ static void * const i2c_bases[] = {
 };
 
 /* i2c_index can be from 0 - 2 */
-void setup_i2c(unsigned i2c_index, int speed, int slave_addr,
-               struct i2c_pads_info *p)
+int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
+             struct i2c_pads_info *p)
 {
+       int ret;
+
        if (i2c_index >= ARRAY_SIZE(i2c_bases))
-               return;
+               return -EINVAL;
        /* Enable i2c clock */
-       enable_i2c_clk(1, i2c_index);
+       ret = enable_i2c_clk(1, i2c_index);
+       if (ret)
+               goto err_clk;
+
        /* Make sure bus is idle */
-       force_idle_bus(p);
+       ret = force_idle_bus(p);
+       if (ret)
+               goto err_idle;
+
        bus_i2c_init(i2c_bases[i2c_index], speed, slave_addr,
                        force_idle_bus, p);
+
+       return 0;
+
+err_idle:
+err_clk:
+       return ret;
 }

arch/arm/include/asm/imx-common/mxc_i2c.h +2 -2

index 182c2f3..af86163 100644
@@ -52,8 +52,8 @@ struct i2c_pads_info {
                                        &mx6q_##name : &mx6s_##name
 #endif
 
-void setup_i2c(unsigned i2c_index, int speed, int slave_addr,
-               struct i2c_pads_info *p);
+int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
+             struct i2c_pads_info *p);
 void bus_i2c_init(void *base, int speed, int slave_addr,
                int (*idle_bus_fn)(void *p), void *p);
 int bus_i2c_read(void *base, uchar chip, uint addr, int alen, uchar *buf,

board/compulab/cm_fx6/cm_fx6.c +34 -7

index fdb8ebf..9c6e686 100644
@@ -141,14 +141,36 @@ I2C_PADS(i2c2_pads,
         IMX_GPIO_NR(1, 6));
 
 
-static void cm_fx6_setup_i2c(void)
+static int cm_fx6_setup_one_i2c(int busnum, struct i2c_pads_info *pads)
 {
-       setup_i2c(0, CONFIG_SYS_I2C_SPEED, 0x7f, I2C_PADS_INFO(i2c0_pads));
-       setup_i2c(1, CONFIG_SYS_I2C_SPEED, 0x7f, I2C_PADS_INFO(i2c1_pads));
-       setup_i2c(2, CONFIG_SYS_I2C_SPEED, 0x7f, I2C_PADS_INFO(i2c2_pads));
+       int ret;
+
+       ret = setup_i2c(busnum, CONFIG_SYS_I2C_SPEED, 0x7f, pads);
+       if (ret)
+               printf("Warning: I2C%d setup failed: %d\n", busnum, ret);
+
+       return ret;
+}
+
+static int cm_fx6_setup_i2c(void)
+{
+       int ret = 0, err;
+
+       /* i2c<x>_pads are wierd macro variables; we can't use an array */
+       err = cm_fx6_setup_one_i2c(0, I2C_PADS_INFO(i2c0_pads));
+       if (err)
+               ret = err;
+       err = cm_fx6_setup_one_i2c(1, I2C_PADS_INFO(i2c1_pads));
+       if (err)
+               ret = err;
+       err = cm_fx6_setup_one_i2c(2, I2C_PADS_INFO(i2c2_pads));
+       if (err)
+               ret = err;
+
+       return ret;
 }
 #else
-static void cm_fx6_setup_i2c(void) { }
+static int cm_fx6_setup_i2c(void) { return 0; }
 #endif
 
 #ifdef CONFIG_USB_EHCI_MX6

@@ -409,9 +431,15 @@ void ft_board_setup(void *blob, bd_t *bd)
 
 int board_init(void)
 {
+       int ret;
+
        gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100;
        cm_fx6_setup_gpmi_nand();
-       cm_fx6_setup_i2c();
+
+       /* Warn on failure but do not abort boot */
+       ret = cm_fx6_setup_i2c();
+       if (ret)
+               printf("Warning: I2C setup failed: %d\n", ret);
 
        return 0;
 }
--

2.1.0.rc2.206.gedb03e5

From: Simon Glass <[email protected]>

GPIOs should be requested before use. Without this, driver model will not permit the GPIO to be used.

Cc Igor Grinberg <[email protected]>

Signed-off-by Simon Glass <[email protected]>

Signed-off-by Nikita Kiryanov <[email protected]>

Quoted 5 lines · Nikita Kiryanov

From: Simon Glass <[email protected]>

GPIOs should be requested before use. Without this, driver model will not permit the GPIO to be used.

Quoted 3 lines

Cc Igor Grinberg <[email protected]>

Signed-off-by Simon Glass <[email protected]>

Signed-off-by Nikita Kiryanov <[email protected]>

I like the "small functionality oriented changes" much more.

Acked-by Igor Grinberg <[email protected]>

Hi Stefan,

Quoted 11 lines · Igor Grinberg

Quoted 8 lines · Nikita Kiryanov

From: Simon Glass <[email protected]>

GPIOs should be requested before use. Without this, driver model will not permit the GPIO to be used.

Cc Igor Grinberg <[email protected]>

Signed-off-by Simon Glass <[email protected]>

Signed-off-by Nikita Kiryanov <[email protected]>

Quoted 3 lines

I like the "small functionality oriented changes" much more.

Acked-by Igor Grinberg <[email protected]>

I plan to pull these two patches from Nikita into u-boot-dm along with my other IMX and DM patches. It needs to come after SPI and there are about 8 series in the line. Is that OK with you?

Hi Simon,

I'm sorry for that late reply, I've had email problems during the conference...
Finally, I have the email working again and going through the stuff... I guess, it is better late than never..

Quoted 16 lines · Simon Glass

Hi Stefan,

Quoted 13 lines · Igor Grinberg

Quoted 8 lines · Nikita Kiryanov

From: Simon Glass <[email protected]>

GPIOs should be requested before use. Without this, driver model will not permit the GPIO to be used.

Cc Igor Grinberg <[email protected]>

Signed-off-by Simon Glass <[email protected]>

Signed-off-by Nikita Kiryanov <[email protected]>

I like the "small functionality oriented changes" much more.

Quoted 5 lines

Quoted 1 line

Acked-by Igor Grinberg <[email protected]>

I plan to pull these two patches from Nikita into u-boot-dm along with my other IMX and DM patches. It needs to come after SPI and there are about 8 series in the line. Is that OK with you?

Yep, this is fine. Thanks!

Use gpio_request for all the gpios that are utilized by various subsystems in cm-fx6, and refactor the relevant init functions so that all gpios are requested during board_init(), not during subsystem init, thus avoiding the need to manage gpio ownership each time a subsystem is initialized.

The new division of labor is:
During board_init() muxes are setup and gpios are requested. During subsystem init gpios are toggled.

Cc Igor Grinberg <[email protected]>

Cc Simon Glass <[email protected]>

Signed-off-by Nikita Kiryanov <[email protected]>

Hi Nikita,

Quoted 10 lines · Nikita Kiryanov

Use gpio_request for all the gpios that are utilized by various subsystems in cm-fx6, and refactor the relevant init functions so that all gpios are requested during board_init(), not during subsystem init, thus avoiding the need to manage gpio ownership each time a subsystem is initialized.

The new division of labor is:
During board_init() muxes are setup and gpios are requested. During subsystem init gpios are toggled.

Quoted 3 lines

Cc Igor Grinberg <[email protected]>

Cc Simon Glass <[email protected]>

Signed-off-by Nikita Kiryanov <[email protected]>

Copying my comments from the other patch:

I've thought about that quite a lot as part of the driver model work. Claiming GPIOs in board code doesn't feel right to me:

1. If using device tree, the GPIOs are in there, and it means the board code needs to go looking there as well as the driver. The board code actually needs to sniff around in the driver's device tree nodes. That just seems honky.

2. In the driver model world, we hope that board init will fade away to a large extent. Certainly it should be possible to specify most of what a driver needs in device tree or platform data. Getting rid of the explicit init calls in U-Boot (board_init_f(), board_init(), board_late_init(), board_early_init_f(), ...) is a nice effect of driver model I hope.

3. Even if not using device tree, and using platform data, where the board code may specify the platform data, it still feels honky for the board to be parsing its own data (designed for use by the driver) to claim GPIOs.

4. I don't really see why pre-claiming enforces anything. If two subsystems claim the same GPIO you are going to get an error somewhere in any case.

5. If you look at the calls that drivers make to find out information from the board file, or perform some init (mmc does this, USB, ethernet, etc.) it is mostly because the driver has no idea of the specifics of the board. Device tree and platform data fix exactly this problem. The driver has the best idea of when it needs to start up, when it needs this resource of that. In some cases the driver have the ability to operate in two modes (e.g. 4 bit or 8 bit SDIO, UART with/without flow control) and this means the init depends on the driver's mode.

Having said that, it's your board and if you really want to go this way in the interim, then I'm not going to strongly object.

Hi Simon,

Quoted 13 lines · Simon Glass

Hi Nikita,

Quoted 10 lines · Nikita Kiryanov

Use gpio_request for all the gpios that are utilized by various subsystems in cm-fx6, and refactor the relevant init functions so that all gpios are requested during board_init(), not during subsystem init, thus avoiding the need to manage gpio ownership each time a subsystem is initialized.

The new division of labor is:
During board_init() muxes are setup and gpios are requested. During subsystem init gpios are toggled.

Quoted 5 lines

Quoted 3 lines

Cc Igor Grinberg <[email protected]>

Cc Simon Glass <[email protected]>

Signed-off-by Nikita Kiryanov <[email protected]>

Copying my comments from the other patch:

Please, don't get me wrong, we have of course read and thought about this also considering your comments.
The thing is that currently, some of those GPIOs are way too board specific and may be the right thing would be to leave it that way. Also some are not board specific, and I will be very glad to pass them to the drivers.
Yet, I think Nikita's patch is very sane for now.
Once we can pass the GPIOs to the drivers we will of course do this. We will also be glad to help working on this as we always did (when the schedule permits us).

Quoted 4 lines

I've thought about that quite a lot as part of the driver model work. Claiming GPIOs in board code doesn't feel right to me:

Quoted 4 lines

1. If using device tree, the GPIOs are in there, and it means the board code needs to go looking there as well as the driver. The board code actually needs to sniff around in the driver's device tree nodes. That just seems honky.

I think this is case dependent. Really we're in the boot loader world, things here are board specific in many cases.

Quoted 8 lines

2. In the driver model world, we hope that board init will fade away to a large extent. Certainly it should be possible to specify most of what a driver needs in device tree or platform data. Getting rid of the explicit init calls in U-Boot (board_init_f(), board_init(), board_late_init(), board_early_init_f(), ...) is a nice effect of driver model I hope.

I don't think it is possible.
There will always be boards that are not by the reference design and there will have to be stuff done in the board files as it will not concern any other boards or drivers.

Quoted 6 lines

3. Even if not using device tree, and using platform data, where the board code may specify the platform data, it still feels honky for the board to be parsing its own data (designed for use by the driver) to claim GPIOs.

Why even? Not using DT is not a bad practice at all!
DT has been designed as an API/ABI to the OS and not for the boot loader! Boot loaders are board specific, period.
I don't mind using DT in the boot loader for ease and abstraction, but don't be obsessed with it, because it will only lead to another, pre-bootloader boot loader which will accommodate all the stuff you are trying to avoid.

Regarding your feeling honky about parsing data by the board code:
There are so many cases, that I don't think you have considered, where using DT _instead_ of run time initializations is a total madness.
Here is one:
Same board, different configuration/revision/extension/variation/etc. Instead of parsing stuff at runtime and adjusting things according to detection, you propose an army of DT blobs? This sounds insane.

Quoted 5 lines

4. I don't really see why pre-claiming enforces anything. If two subsystems claim the same GPIO you are going to get an error somewhere in any case.

Two subsystems should never own the same GPIO at the same time. If you follow that rule, there will be errors only in case when there should errors.

Quoted 11 lines

5. If you look at the calls that drivers make to find out information from the board file, or perform some init (mmc does this, USB, ethernet, etc.) it is mostly because the driver has no idea of the specifics of the board. Device tree and platform data fix exactly this problem. The driver has the best idea of when it needs to start up, when it needs this resource of that. In some cases the driver have the ability to operate in two modes (e.g. 4 bit or 8 bit SDIO, UART with/without flow control) and this means the init depends on the driver's mode.

This is correct. No doubt about this.
Yet, generic driver may have prerequisite on a custom board and don't even know about this prerequisite.

Quoted 4 lines

Having said that, it's your board and if you really want to go this way in the interim, then I'm not going to strongly object.

Thanks!
I do really like the idea of DM and I think this should be developed year ago. Yet, any framework should be flexible enough to give some place on the stage to the board specific code.

Hi Igor,

Quoted 37 lines · Igor Grinberg

Hi Simon,

Quoted 13 lines · Simon Glass

Hi Nikita,

Quoted 10 lines · Nikita Kiryanov

Use gpio_request for all the gpios that are utilized by various subsystems in cm-fx6, and refactor the relevant init functions so that all gpios are requested during board_init(), not during subsystem init, thus avoiding the need to manage gpio ownership each time a subsystem is initialized.

The new division of labor is:
During board_init() muxes are setup and gpios are requested. During subsystem init gpios are toggled.

Quoted 5 lines

Quoted 3 lines

Cc Igor Grinberg <[email protected]>

Cc Simon Glass <[email protected]>

Signed-off-by Nikita Kiryanov <[email protected]>

Copying my comments from the other patch:

Please, don't get me wrong, we have of course read and thought about this also considering your comments.
The thing is that currently, some of those GPIOs are way too board specific and may be the right thing would be to leave it that way. Also some are not board specific, and I will be very glad to pass them to the drivers.
Yet, I think Nikita's patch is very sane for now.
Once we can pass the GPIOs to the drivers we will of course do this. We will also be glad to help working on this as we always did (when the schedule permits us).

Quoted 4 lines

I've thought about that quite a lot as part of the driver model work. Claiming GPIOs in board code doesn't feel right to me:

Quoted 7 lines

Quoted 4 lines

1. If using device tree, the GPIOs are in there, and it means the board code needs to go looking there as well as the driver. The board code actually needs to sniff around in the driver's device tree nodes. That just seems honky.

I think this is case dependent. Really we're in the boot loader world, things here are board specific in many cases.

Yes it is important to retain that flexibility.

Quoted 9 lines

Quoted 8 lines

2. In the driver model world, we hope that board init will fade away to a large extent. Certainly it should be possible to specify most of what a driver needs in device tree or platform data. Getting rid of the explicit init calls in U-Boot (board_init_f(), board_init(), board_late_init(), board_early_init_f(), ...) is a nice effect of driver model I hope.

Quoted 4 lines

I don't think it is possible.
There will always be boards that are not by the reference design and there will have to be stuff done in the board files as it will not concern any other boards or drivers.

Let's see how this pans out. I feel we can more the pendulum a long way towards less board-specific init. For example, for MMC we have a card detect. If we just specify which GPIO it is on, then we don't need all the callbacks.

Quoted 16 lines

Quoted 7 lines

3. Even if not using device tree, and using platform data, where the board code may specify the platform data, it still feels honky for the board to be parsing its own data (designed for use by the driver) to claim GPIOs.

Why even? Not using DT is not a bad practice at all!
DT has been designed as an API/ABI to the OS and not for the boot loader! Boot loaders are board specific, period.
I don't mind using DT in the boot loader for ease and abstraction, but don't be obsessed with it, because it will only lead to another, pre-bootloader boot loader which will accommodate all the stuff you are trying to avoid.

Quoted 8 lines

Regarding your feeling honky about parsing data by the board code:
There are so many cases, that I don't think you have considered, where using DT _instead_ of run time initializations is a total madness.
Here is one:
Same board, different configuration/revision/extension/variation/etc. Instead of parsing stuff at runtime and adjusting things according to detection, you propose an army of DT blobs? This sounds insane.

We are talking here about the code/data trade-off. I feel that U-Boot currently requires lots of code to be written where with device tree/platform data it could be data, and the benefit is fewer code paths and easier integration of new boards. What are these revisions doing? If they are changing the MMC card detect line then you can have two different platform data blobs for the board, or two device trees. It's not mandatory but it certainly works.

If it is easier to check a few GPIOs to find the board ID and then adjust things at runtime then that works too. That sort of code should live in the board files though, not the drivers.

Quoted 7 lines

Quoted 6 lines

4. I don't really see why pre-claiming enforces anything. If two subsystems claim the same GPIO you are going to get an error somewhere in any case.

Quoted 3 lines

Two subsystems should never own the same GPIO at the same time. If you follow that rule, there will be errors only in case when there should errors.

Yes. My point was that pre-claiming would still result in an error in this case.

Quoted 13 lines

Quoted 12 lines

5. If you look at the calls that drivers make to find out information from the board file, or perform some init (mmc does this, USB, ethernet, etc.) it is mostly because the driver has no idea of the specifics of the board. Device tree and platform data fix exactly this problem. The driver has the best idea of when it needs to start up, when it needs this resource of that. In some cases the driver have the ability to operate in two modes (e.g. 4 bit or 8 bit SDIO, UART with/without flow control) and this means the init depends on the driver's mode.

Quoted 3 lines

This is correct. No doubt about this.
Yet, generic driver may have prerequisite on a custom board and don't even know about this prerequisite.

Or in fact the driver may request that the board be set up a particular way. This is effectively what happens with MMC - 'please set up the card-detect line for me to use'.

Quoted 9 lines

Quoted 4 lines

Having said that, it's your board and if you really want to go this way in the interim, then I'm not going to strongly object.

Thanks!
I do really like the idea of DM and I think this should be developed year ago. Yet, any framework should be flexible enough to give some place on the stage to the board specific code.

I strongly agree with this statement and have been careful so far to maintain this flexibility in the framework. Let's keep an eye on it.

Regards,
Simon

Hi Simon,

Quoted 65 lines · Simon Glass

Hi Igor,

Quoted 37 lines · Igor Grinberg

Hi Simon,

Quoted 13 lines · Simon Glass

Hi Nikita,

Quoted 10 lines · Nikita Kiryanov

Use gpio_request for all the gpios that are utilized by various subsystems in cm-fx6, and refactor the relevant init functions so that all gpios are requested during board_init(), not during subsystem init, thus avoiding the need to manage gpio ownership each time a subsystem is initialized.

The new division of labor is:
During board_init() muxes are setup and gpios are requested. During subsystem init gpios are toggled.

Quoted 5 lines

Quoted 3 lines

Cc Igor Grinberg <[email protected]>

Cc Simon Glass <[email protected]>

Signed-off-by Nikita Kiryanov <[email protected]>

Copying my comments from the other patch:

Please, don't get me wrong, we have of course read and thought about this also considering your comments.
The thing is that currently, some of those GPIOs are way too board specific and may be the right thing would be to leave it that way. Also some are not board specific, and I will be very glad to pass them to the drivers.
Yet, I think Nikita's patch is very sane for now.
Once we can pass the GPIOs to the drivers we will of course do this. We will also be glad to help working on this as we always did (when the schedule permits us).

Quoted 4 lines

I've thought about that quite a lot as part of the driver model work. Claiming GPIOs in board code doesn't feel right to me:

Quoted 7 lines

Quoted 4 lines

1. If using device tree, the GPIOs are in there, and it means the board code needs to go looking there as well as the driver. The board code actually needs to sniff around in the driver's device tree nodes. That just seems honky.

I think this is case dependent. Really we're in the boot loader world, things here are board specific in many cases.

Yes it is important to retain that flexibility.

Quoted 13 lines

Quoted 8 lines

2. In the driver model world, we hope that board init will fade away to a large extent. Certainly it should be possible to specify most of what a driver needs in device tree or platform data. Getting rid of the explicit init calls in U-Boot (board_init_f(), board_init(), board_late_init(), board_early_init_f(), ...) is a nice effect of driver model I hope.

I don't think it is possible.
There will always be boards that are not by the reference design and there will have to be stuff done in the board files as it will not concern any other boards or drivers.

Quoted 4 lines

Let's see how this pans out. I feel we can more the pendulum a long way towards less board-specific init. For example, for MMC we have a card detect. If we just specify which GPIO it is on, then we don't need all the callbacks.

That is totally agreed for cases where it is possible.

Quoted 26 lines

Quoted 17 lines

Quoted 8 lines

3. Even if not using device tree, and using platform data, where the board code may specify the platform data, it still feels honky for the board to be parsing its own data (designed for use by the driver) to claim GPIOs.

Why even? Not using DT is not a bad practice at all!
DT has been designed as an API/ABI to the OS and not for the boot loader! Boot loaders are board specific, period.
I don't mind using DT in the boot loader for ease and abstraction, but don't be obsessed with it, because it will only lead to another, pre-bootloader boot loader which will accommodate all the stuff you are trying to avoid.

Quoted 8 lines

Regarding your feeling honky about parsing data by the board code:
There are so many cases, that I don't think you have considered, where using DT _instead_ of run time initializations is a total madness.
Here is one:
Same board, different configuration/revision/extension/variation/etc. Instead of parsing stuff at runtime and adjusting things according to detection, you propose an army of DT blobs? This sounds insane.

Quoted 7 lines

We are talking here about the code/data trade-off. I feel that U-Boot currently requires lots of code to be written where with device tree/platform data it could be data, and the benefit is fewer code paths and easier integration of new boards. What are these revisions doing? If they are changing the MMC card detect line then you can have two different platform data blobs for the board, or two device trees. It's not mandatory but it certainly works.

Right. Yet, you still need code that will detect the revision and make the correct choice for platform data/DT blob. What I'm saying is that in this case, you don't really need several DT blobs or platform data structs, but only one which can be adjusted by the same (or not the same) revision detection code.

Sometimes, it can be 3, 4, 5 revisions of the CoM, plus even more of a base board. You can't really have that much DT blobs in a pocket to pull out...

Quoted 5 lines

If it is easier to check a few GPIOs to find the board ID and then adjust things at runtime then that works too. That sort of code should live in the board files though, not the drivers.

So, it merely repeats what I'm saying...

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

4. I don't really see why pre-claiming enforces anything. If two subsystems claim the same GPIO you are going to get an error somewhere in any case.

Two subsystems should never own the same GPIO at the same time. If you follow that rule, there will be errors only in case when there should errors.

Quoted 1 line

Yes. My point was that pre-claiming would still result in an error in this case.

It will have the benefit of _not_ playing the request - free game. IMO, request should be done once (for GPIOs that are designed to be used in only one subsystem) and then the subsystem can toggle the GPIO as it likes - w/o the need for requesting once again.

Quoted 27 lines

Quoted 14 lines

Quoted 4 lines

Quoted 9 lines

5. If you look at the calls that drivers make to find out information from the board file, or perform some init (mmc does this, USB, ethernet, etc.) it is mostly because the driver has no idea of the specifics of the board. Device tree and platform data fix exactly this problem. The driver has the best idea of when it needs to start up, when it needs this resource of that. In some cases the driver have the ability to operate in two modes (e.g. 4 bit or 8 bit SDIO, UART with/without flow control) and this means the init depends on the driver's mode.

Quoted 3 lines

This is correct. No doubt about this.
Yet, generic driver may have prerequisite on a custom board and don't even know about this prerequisite.

Or in fact the driver may request that the board be set up a particular way. This is effectively what happens with MMC - 'please set up the card-detect line for me to use'.

Quoted 5 lines

Quoted 4 lines

Having said that, it's your board and if you really want to go this way in the interim, then I'm not going to strongly object.

Quoted 7 lines

Quoted 4 lines

Thanks!
I do really like the idea of DM and I think this should be developed year ago. Yet, any framework should be flexible enough to give some place on the stage to the board specific code.

I strongly agree with this statement and have been careful so far to maintain this flexibility in the framework. Let's keep an eye on it.

Hi Igor,

Quoted 13 lines · Igor Grinberg

Hi Simon,

Quoted 10 lines · Simon Glass

Hi Igor,

Quoted 6 lines · Igor Grinberg

Hi Simon,

Quoted 3 lines · Simon Glass

Hi Nikita,

On 2 October 2014 08:17, Nikita Kiryanov <[email protected]> wrote:

Quoted 36 lines

Quoted 22 lines

Quoted 13 lines

Quoted 4 lines

3. Even if not using device tree, and using platform data, where the board code may specify the platform data, it still feels honky for the board to be parsing its own data (designed for use by the driver) to claim GPIOs.

Why even? Not using DT is not a bad practice at all!
DT has been designed as an API/ABI to the OS and not for the boot loader! Boot loaders are board specific, period.
I don't mind using DT in the boot loader for ease and abstraction, but don't be obsessed with it, because it will only lead to another, pre-bootloader boot loader which will accommodate all the stuff you are trying to avoid.

Quoted 8 lines

Regarding your feeling honky about parsing data by the board code:
There are so many cases, that I don't think you have considered, where using DT _instead_ of run time initializations is a total madness.
Here is one:
Same board, different configuration/revision/extension/variation/etc. Instead of parsing stuff at runtime and adjusting things according to detection, you propose an army of DT blobs? This sounds insane.

Quoted 7 lines

We are talking here about the code/data trade-off. I feel that U-Boot currently requires lots of code to be written where with device tree/platform data it could be data, and the benefit is fewer code paths and easier integration of new boards. What are these revisions doing? If they are changing the MMC card detect line then you can have two different platform data blobs for the board, or two device trees. It's not mandatory but it certainly works.

Right. Yet, you still need code that will detect the revision and make the correct choice for platform data/DT blob. What I'm saying is that in this case, you don't really need several DT blobs or platform data structs, but only one which can be adjusted by the same (or not the same) revision detection code.

Quoted 3 lines

Sometimes, it can be 3, 4, 5 revisions of the CoM, plus even more of a base board. You can't really have that much DT blobs in a pocket to pull out...

Sure you can. But if you have board rev GPIOs you can do at least three things:

1. Use them only during production to configure the image to flash (an image that only works with a single board rev)
2. Use them at run time to select which platform data or device tree info to use
3. Use them at run-time to adjust the platform data or device tree (even if just to change the 'status' property from "okay" to "disabled")

I feel all of these have their uses depending on the situation. There a lots of trade-offs to be had.

For base boards (and maybe even board revs) I wonder if the DT merging feature might be useful?

Quoted 20 lines

Quoted 6 lines

If it is easier to check a few GPIOs to find the board ID and then adjust things at runtime then that works too. That sort of code should live in the board files though, not the drivers.

So, it merely repeats what I'm saying...

Quoted 11 lines

Quoted 10 lines

Quoted 6 lines

4. I don't really see why pre-claiming enforces anything. If two subsystems claim the same GPIO you are going to get an error somewhere in any case.

Two subsystems should never own the same GPIO at the same time. If you follow that rule, there will be errors only in case when there should errors.

Quoted 6 lines

Quoted 1 line

Yes. My point was that pre-claiming would still result in an error in this case.

It will have the benefit of _not_ playing the request - free game. IMO, request should be done once (for GPIOs that are designed to be used in only one subsystem) and then the subsystem can toggle the GPIO as it likes - w/o the need for requesting once again.

OK. In general I prefer the device to claim the GPIOs rather than the board. With driver model this is pretty easy as there is a formal 'probe' function which will only be called once (until after 'remove' is called at least). I think request/free is is only a problem because of the ad-hoc driver approach. But I see that as a general direction to take, and not a hard and fast rule.

Use gpio_request for all the gpios that are utilized by various subsystems in cm-fx6, and refactor the relevant init functions so that all gpios are requested during board_init(), not during subsystem init, thus avoiding the need to manage gpio ownership each time a subsystem is initialized.

The new division of labor is:
During board_init() muxes are setup and gpios are requested. During subsystem init gpios are toggled.

Cc Igor Grinberg <[email protected]>

Cc Simon Glass <[email protected]>

Signed-off-by Nikita Kiryanov <[email protected]>

Use gpio_request for all the gpios that are utilized by various subsystems in cm-fx6, and refactor the relevant init functions so that all gpios are requested during board_init(), not during subsystem init, thus avoiding the need to manage gpio ownership each time a subsystem is initialized.

The new division of labor is:
During board_init() muxes are setup and gpios are requested. During subsystem init gpios are toggled.

Cc Igor Grinberg <[email protected]>

Cc Simon Glass <[email protected]>

Signed-off-by Nikita Kiryanov <[email protected]>

---
 board/compulab/cm_fx6/cm_fx6.c | 133 +++++++++++++++++++++++++++++------------
 1 file changed, 94 insertions(+), 39 deletions(-)

GPIOs should be requested before use. Without this, driver model will not permit the GPIO to be used.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v4:
- Adjust error checking to permit calling gpio_request() multiple times
- Avoid doing low-level SATA init multiple times
- Move SATA changes into this patch

Changes in v3:
- Add a check for the Ethernet gpio_request() also
- Add a comment for the CONFIG_SPL_BUILD #ifdef
- Just warn when one of the board init stages fails

Changes in v2:
- Check return values of gpio_request()

  arch/arm/imx-common/i2c-mxv7.c | 24 +++++++++++++++++
  board/compulab/cm_fx6/cm_fx6.c | 61 ++++++++++++++++++++++++++++++++----------
  board/compulab/cm_fx6/common.c | 14 +++++++++-
  3 files changed, 84 insertions(+), 15 deletions(-)

arch/arm/imx-common/i2c-mxv7.c +1 -0

index 70cff5c..aaf6936 100644
@@ -4,6 +4,7 @@
   * SPDX-License-Identifier:	GPL-2.0+
   */
  #include <common.h>
+#include <malloc.h>
  #include <asm/arch/clock.h>
  #include <asm/arch/imx-regs.h>
  #include <asm/errno.h>

@@ -72,10 +73,26 @@ static void * const i2c_bases[] = {
  int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
  	      struct i2c_pads_info *p)
  {

Hi Simon,

Quoted 46 lines · Simon Glass

GPIOs should be requested before use. Without this, driver model will not permit the GPIO to be used.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v4:
- Adjust error checking to permit calling gpio_request() multiple times
- Avoid doing low-level SATA init multiple times
- Move SATA changes into this patch

Changes in v3:
- Add a check for the Ethernet gpio_request() also
- Add a comment for the CONFIG_SPL_BUILD #ifdef
- Just warn when one of the board init stages fails

Changes in v2:
- Check return values of gpio_request()

  arch/arm/imx-common/i2c-mxv7.c | 24 +++++++++++++++++
  board/compulab/cm_fx6/cm_fx6.c | 61
++++++++++++++++++++++++++++++++----------
  board/compulab/cm_fx6/common.c | 14 +++++++++-
  3 files changed, 84 insertions(+), 15 deletions(-)

arch/arm/imx-common/i2c-mxv7.c +2 -0

index 70cff5c..aaf6936 100644
@@ -4,6 +4,7 @@
   * SPDX-License-Identifier:   GPL-2.0+
   */
  #include <common.h>
+#include <malloc.h>
  #include <asm/arch/clock.h>
  #include <asm/arch/imx-regs.h>
  #include <asm/errno.h>

@@ -72,10 +73,26 @@ static void * const i2c_bases[] = {
  int setup_i2c(unsigned i2c_index, int speed, int slave_addr,
              struct i2c_pads_info *p)
  {
+       char *name1, *name2;
        int ret;
 

arch/arm/imx-common/i2c-mxv7.c static void * const i2c_bases[] = { · line 85

Quoted 7 lines

arch/arm/imx-common/i2c-mxv7.c +5 -0

        if (i2c_index >= ARRAY_SIZE(i2c_bases))
                return -EINVAL;
+
+       name1 = malloc(9);
+       name2 = malloc(9);
+       if (!name1 || !name2)
+               return -ENOMEM;

You have a memory leak here if name1 is allocated but name2 is not.

Quoted 32 lines

arch/arm/imx-common/i2c-mxv7.c +17 -0

+       sprintf(name1, "i2c_sda%d", i2c_index);
+       sprintf(name2, "i2c_scl%d", i2c_index);
+       ret = gpio_request(p->sda.gp, name1);
+       if (ret)
+               goto err_req1;
+
+       ret = gpio_request(p->scl.gp, name2);
+       if (ret)
+               goto err_req2;
+
        /* Enable i2c clock */
        ret = enable_i2c_clk(1, i2c_index);
        if (ret)

@@ -93,5 +110,12 @@ int setup_i2c(unsigned i2c_index, int speed, int
 slave_addr,
 
  err_idle:
  err_clk:
+       gpio_free(p->scl.gp);
+err_req2:
+       gpio_free(p->sda.gp);
+err_req1:
+       free(name1);
+       free(name2);
+
        return ret;

  }

board/compulab/cm_fx6/cm_fx6.c +0 -0

index 9c6e686..53681d4 100644

board/compulab/cm_fx6/cm_fx6.c

Quoted 2 lines

board/compulab/cm_fx6/cm_fx6.c +0 -0

While this patch addresses the errors I mentioned in V3, I think that the amount of additional checks this required demonstrates that these init functions (which can be called multiple times) are not the best place to do gpio requests.

I'm open to the idea that these requests will be handled by the respective drivers (where applicable), but until that functionality is implemented I think it's best to do them in board_init().

I'm attaching 2 patches, which split this patch into two, one for i2c-mxv7.c, and the other for cm_fx6.c.

Add driver model support with this driver. In this case the platform data is in the driver. It would be better to put this into an SOC-specific file, but this is best attempted when more boards are moved over to use driver model.

Signed-off-by Simon Glass <[email protected]>

Acked-by Igor Grinberg <[email protected]>

---

Changes in v4: None
Changes in v3:
- Use gpio_is_requested() in one more place

Changes in v2:
- Add an internal function to check if a GPIO is requested
- Change 'reserved' to 'requested'
- Tidy up confusing code that creates names for gpio_request()

 drivers/gpio/mxc_gpio.c | 304 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 303 insertions(+), 1 deletion(-)

Add driver model support with this driver. Boards which use this driver should define platform data in their board files.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/serial/serial_mxc.c | 170 +++++++++++++++++++++++++++++++++++++-------
 include/serial_mxc.h        |  14 ++++
 2 files changed, 159 insertions(+), 25 deletions(-)
 create mode 100644 include/serial_mxc.h

Now that serial and GPIO are available for iMX.6, move cm_fx6 over as an example.

Acked-by Igor Grinberg <[email protected]>

Signed-off-by Simon Glass <[email protected]>

Now that serial and GPIO are available for iMX.6, move cm_fx6 over as an example.

Acked-by Igor Grinberg <[email protected]>

Signed-off-by Simon Glass <[email protected]>

---

Changes in v4: None
Changes in v3: None
Changes in v2:
- Use the correct namespace for the platform data

 board/compulab/cm_fx6/cm_fx6.c | 10 ++++++++++
 include/configs/cm_fx6.h       | 11 +++++++++++
 2 files changed, 21 insertions(+)

This series adjusts the IMX serial and GPIO drivers to support driver model. As an example of its use, the recently-added cm_fx6 board is converted over to driver model.

Some minor driver model core changed are required to make this work and these are included with this series.

Thanks to the Compulab people for helping with this.

Changes in v4:
- Adjust error checking to permit calling gpio_request() multiple times
- Avoid doing low-level SATA init multiple times
- Move SATA changes into the next patch
- Move SATA changes into this patch