| Message ID | 1412259444-11959-3-git-send-email-nikita@compulab.co.il |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Simon Glass |
| Headers | show |
Hi Nikita, On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote: > 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 <grinberg@compulab.co.il> > Cc: Simon Glass <sjg@chromium.org> > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> 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. Regards, Simon
Hi Nikita, On 10/02/14 17:17, Nikita Kiryanov wrote: > 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 <grinberg@compulab.co.il> > Cc: Simon Glass <sjg@chromium.org> > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> This looks much better. I really don't like sticking the "has_been_inited" style of plugs, while the same can be done by design and w/o such plugs. Thanks Nikita. Acked-by Igor Grinberg <grinberg@compulab.co.il>
Hi Simon, On 10/02/14 22:22, Simon Glass wrote: > Hi Nikita, > > On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote: >> 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 <grinberg@compulab.co.il> >> Cc: Simon Glass <sjg@chromium.org> >> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > > 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). > > 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. I think this is case dependent. Really we're in the boot loader world, things here are board specific in many cases. > > 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. > > 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. > > 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. > > 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. > > 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, On 3 October 2014 01:41, Igor Grinberg <grinberg@compulab.co.il> wrote: > Hi Simon, > > On 10/02/14 22:22, Simon Glass wrote: >> Hi Nikita, >> >> On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote: >>> 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 <grinberg@compulab.co.il> >>> Cc: Simon Glass <sjg@chromium.org> >>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> >> >> 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). > >> >> 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. > > 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. > >> >> 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. 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. > >> >> 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. 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. > >> >> 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. Yes. My point was that pre-claiming would still result in an error in this 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. > > 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'. > >> >> 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, On 10/03/14 17:04, Simon Glass wrote: > Hi Igor, > > > On 3 October 2014 01:41, Igor Grinberg <grinberg@compulab.co.il> wrote: >> Hi Simon, >> >> On 10/02/14 22:22, Simon Glass wrote: >>> Hi Nikita, >>> >>> On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote: >>>> 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 <grinberg@compulab.co.il> >>>> Cc: Simon Glass <sjg@chromium.org> >>>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> >>> >>> 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). >> >>> >>> 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. >> >> 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. > >> >>> >>> 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. > > 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. > >> >>> >>> 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. > > 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... > > 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... > >> >>> >>> 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. > > 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. > >> >>> >>> 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. > > 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'. > >> >>> >>> 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 Igor, On 5 October 2014 04:52, Igor Grinberg <grinberg@compulab.co.il> wrote: > Hi Simon, > > On 10/03/14 17:04, Simon Glass wrote: >> Hi Igor, >> >> >> On 3 October 2014 01:41, Igor Grinberg <grinberg@compulab.co.il> wrote: >>> Hi Simon, >>> >>> On 10/02/14 22:22, Simon Glass wrote: >>>> Hi Nikita, >>>> >>>> On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote: >>>>> 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 <grinberg@compulab.co.il> >>>>> Cc: Simon Glass <sjg@chromium.org> >>>>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> >>>> >>>> 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). >>> >>>> >>>> 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. >>> >>> 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. >> >>> >>>> >>>> 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. >> >> 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. OK good. > >> >>> >>>> >>>> 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. >> >> 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... 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? > >> >> 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... > >> >>> >>>> >>>> 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. >> >> 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. > >> >>> >>>> >>>> 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. >> >> 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'. >> >>> >>>> >>>> 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
On 10/05/14 21:32, Simon Glass wrote: > Hi Igor, > > On 5 October 2014 04:52, Igor Grinberg <grinberg@compulab.co.il> wrote: >> Hi Simon, >> >> On 10/03/14 17:04, Simon Glass wrote: >>> Hi Igor, >>> >>> >>> On 3 October 2014 01:41, Igor Grinberg <grinberg@compulab.co.il> wrote: >>>> Hi Simon, >>>> >>>> On 10/02/14 22:22, Simon Glass wrote: >>>>> Hi Nikita, >>>>> >>>>> On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote: [...] >>>>> 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. >>> >>> 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... > > 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. Revision GPIOs is only one case of encoding, there are multiple possibilities for revision encoding. Nevertheless, from my experience, only 3rd option is scaling well enough and sorts out various production/field update by customer/etc. needs. > > For base boards (and maybe even board revs) I wonder if the DT merging > feature might be useful? Yes. I think it is useful indeed. There were talks about "cape bus" (or whatever it was called) in Linux kernel and dynamic DT blobs loading for extending the one the kernel was booted with. Though I don't know what the status of this, but it can be very useful for extension boards and hot plugging. Less for the base boards concept (at least as we use it now). For the base boards DT merging (if I understand the meaning of this correctly) can prove useful, but should be handled by U-Boot as base boards might have a serious impact on the boot process. > >> >>> >>> 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... >> >>> >>>> >>>>> >>>>> 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. >>> >>> 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. No objection on this one. The objection was only on how to handle the current situation with cm-fx6 CoM. e.g. current driver does not handle the GPIO request stuff. Once it does, in the same patch, we should remove it from the board(s) code. It is just like Nikita said, until we have these in the drivers, we prefer to solve the request problem in a better way (IMO, this is a better way - to request once instead of request and free each time or depend on a "inited" variable).
On 2 October 2014 08:17, Nikita Kiryanov <nikita@compulab.co.il> wrote: > 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 <grinberg@compulab.co.il> > Cc: Simon Glass <sjg@chromium.org> > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > --- > board/compulab/cm_fx6/cm_fx6.c | 133 +++++++++++++++++++++++++++++------------ > 1 file changed, 94 insertions(+), 39 deletions(-) > Applied to u-boot-dm/master, thanks!
diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c index 9c6e686..f0edc8f 100644 --- a/board/compulab/cm_fx6/cm_fx6.c +++ b/board/compulab/cm_fx6/cm_fx6.c @@ -69,16 +69,23 @@ static iomux_v3_cfg_t const sata_pads[] = { IOMUX_PADS(PAD_EIM_BCLK__GPIO6_IO31 | MUX_PAD_CTRL(NO_PAD_CTRL)), }; -static void cm_fx6_setup_issd(void) +static int cm_fx6_setup_issd(void) { + int ret, i; + SETUP_IOMUX_PADS(sata_pads); - /* Make sure this gpio has logical 0 value */ - gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0); - udelay(100); - cm_fx6_sata_power(0); - mdelay(250); - cm_fx6_sata_power(1); + for (i = 0; i < ARRAY_SIZE(cm_fx6_issd_gpios); i++) { + ret = gpio_request(cm_fx6_issd_gpios[i], "sata"); + if (ret) + return ret; + } + + ret = gpio_request(CM_FX6_SATA_PWLOSS_INT, "sata_pwloss_int"); + if (ret) + return ret; + + return 0; } #define CM_FX6_SATA_INIT_RETRIES 10 @@ -86,7 +93,14 @@ int sata_initialize(void) { int err, i; - cm_fx6_setup_issd(); + /* Make sure this gpio has logical 0 value */ + gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0); + udelay(100); + + cm_fx6_sata_power(0); + mdelay(250); + cm_fx6_sata_power(1); + for (i = 0; i < CM_FX6_SATA_INIT_RETRIES; i++) { err = setup_sata(); if (err) { @@ -109,6 +123,8 @@ int sata_initialize(void) return err; } +#else +static int cm_fx6_setup_issd(void) { return 0; } #endif #ifdef CONFIG_SYS_I2C_MXC @@ -177,35 +193,32 @@ static int cm_fx6_setup_i2c(void) { return 0; } #define WEAK_PULLDOWN (PAD_CTL_PUS_100K_DOWN | \ PAD_CTL_SPEED_MED | PAD_CTL_DSE_40ohm | \ PAD_CTL_HYS | PAD_CTL_SRE_SLOW) +#define MX6_USBNC_BASEADDR 0x2184800 +#define USBNC_USB_H1_PWR_POL (1 << 9) -static int cm_fx6_usb_hub_reset(void) +static int cm_fx6_setup_usb_host(void) { int err; err = gpio_request(CM_FX6_USB_HUB_RST, "usb hub rst"); - if (err) { - printf("USB hub rst gpio request failed: %d\n", err); - return -1; - } + if (err) + return err; + SETUP_IOMUX_PAD(PAD_GPIO_0__USB_H1_PWR | MUX_PAD_CTRL(NO_PAD_CTRL)); SETUP_IOMUX_PAD(PAD_SD3_RST__GPIO7_IO08 | MUX_PAD_CTRL(NO_PAD_CTRL)); - gpio_direction_output(CM_FX6_USB_HUB_RST, 0); - udelay(10); - gpio_direction_output(CM_FX6_USB_HUB_RST, 1); - mdelay(1); return 0; } -static int cm_fx6_init_usb_otg(void) +static int cm_fx6_setup_usb_otg(void) { - int ret; + int err; struct iomuxc *iomux = (struct iomuxc *)IOMUXC_BASE_ADDR; - ret = gpio_request(SB_FX6_USB_OTG_PWR, "usb-pwr"); - if (ret) { - printf("USB OTG pwr gpio request failed: %d\n", ret); - return ret; + err = gpio_request(SB_FX6_USB_OTG_PWR, "usb-pwr"); + if (err) { + printf("USB OTG pwr gpio request failed: %d\n", err); + return err; } SETUP_IOMUX_PAD(PAD_EIM_D22__GPIO3_IO22 | MUX_PAD_CTRL(NO_PAD_CTRL)); @@ -216,25 +229,27 @@ static int cm_fx6_init_usb_otg(void) return gpio_direction_output(SB_FX6_USB_OTG_PWR, 0); } -#define MX6_USBNC_BASEADDR 0x2184800 -#define USBNC_USB_H1_PWR_POL (1 << 9) int board_ehci_hcd_init(int port) { + int ret; u32 *usbnc_usb_uh1_ctrl = (u32 *)(MX6_USBNC_BASEADDR + 4); - switch (port) { - case 0: - return cm_fx6_init_usb_otg(); - case 1: - SETUP_IOMUX_PAD(PAD_GPIO_0__USB_H1_PWR | - MUX_PAD_CTRL(NO_PAD_CTRL)); + /* Only 1 host controller in use. port 0 is OTG & needs no attention */ + if (port != 1) + return 0; - /* Set PWR polarity to match power switch's enable polarity */ - setbits_le32(usbnc_usb_uh1_ctrl, USBNC_USB_H1_PWR_POL); - return cm_fx6_usb_hub_reset(); - default: - break; - } + /* Set PWR polarity to match power switch's enable polarity */ + setbits_le32(usbnc_usb_uh1_ctrl, USBNC_USB_H1_PWR_POL); + ret = gpio_direction_output(CM_FX6_USB_HUB_RST, 0); + if (ret) + return ret; + + udelay(10); + ret = gpio_direction_output(CM_FX6_USB_HUB_RST, 1); + if (ret) + return ret; + + mdelay(1); return 0; } @@ -246,6 +261,9 @@ int board_ehci_power(int port, int on) return 0; } +#else +static int cm_fx6_setup_usb_otg(void) { return 0; } +static int cm_fx6_setup_usb_host(void) { return 0; } #endif #ifdef CONFIG_FEC_MXC @@ -340,12 +358,17 @@ static int handle_mac_address(void) int board_eth_init(bd_t *bis) { - int res = handle_mac_address(); - if (res) + int err; + + err = handle_mac_address(); + if (err) puts("No MAC address found\n"); SETUP_IOMUX_PADS(enet_pads); /* phy reset */ + err = gpio_request(CM_FX6_ENET_NRST, "enet_nrst"); + if (err) + printf("Etnernet NRST gpio request failed: %d\n", err); gpio_direction_output(CM_FX6_ENET_NRST, 0); udelay(500); gpio_set_value(CM_FX6_ENET_NRST, 1); @@ -416,6 +439,16 @@ int board_mmc_init(bd_t *bis) } #endif +#ifdef CONFIG_MXC_SPI +int cm_fx6_setup_ecspi(void) +{ + cm_fx6_set_ecspi_iomux(); + return gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0"); +} +#else +int cm_fx6_setup_ecspi(void) { return 0; } +#endif + #ifdef CONFIG_OF_BOARD_SETUP void ft_board_setup(void *blob, bd_t *bd) { @@ -436,6 +469,28 @@ int board_init(void) gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100; cm_fx6_setup_gpmi_nand(); + ret = cm_fx6_setup_ecspi(); + if (ret) + printf("Warning: ECSPI setup failed: %d\n", ret); + + ret = cm_fx6_setup_usb_otg(); + if (ret) + printf("Warning: USB OTG setup failed: %d\n", ret); + + ret = cm_fx6_setup_usb_host(); + if (ret) + printf("Warning: USB host setup failed: %d\n", ret); + + /* + * cm-fx6 may have iSSD not assembled and in this case it has + * bypasses for a (m)SATA socket on the baseboard. The socketed + * device is not controlled by those GPIOs. So just print a warning + * if the setup fails. + */ + ret = cm_fx6_setup_issd(); + if (ret) + printf("Warning: iSSD setup failed: %d\n", ret); + /* Warn on failure but do not abort boot */ ret = cm_fx6_setup_i2c(); if (ret)
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 <grinberg@compulab.co.il> Cc: Simon Glass <sjg@chromium.org> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> --- board/compulab/cm_fx6/cm_fx6.c | 133 +++++++++++++++++++++++++++++------------ 1 file changed, 94 insertions(+), 39 deletions(-)