| Message ID | 51F9E4CB.9040004@denx.de |
|---|---|
| State | Not Applicable |
| Headers | show |
On 07/31/2013 10:32 PM, Heiko Schocher wrote: > Hello Stephen, > > Am 31.07.2013 21:31, schrieb Stephen Warren: >> On 07/30/2013 11:46 PM, Heiko Schocher wrote: >> ... >>> Hmm.. each i2c adapter has its own init function ... why the tegra >>> driver do not use it? And do the necessary inits in it? So we >>> initialize an adapater only if we use it, which is also a rule >>> for u-boot ... >>> >>> I have no hw, but it should be possible to add to process_nodes() >>> a parameter "controller_number" and call >>> process_nodes(controller_number, ...) from the i2c drivers init >>> function ... >> >> There are two steps to initializing I2C on a Tegra system: >> >> 1) Parsing the DT to determine which/how-many I2C adapters exist in the >> SoC. This will yield information such as the register address of the I2C >> adapters, which clock/reset signal they rely on, perhaps the max bus >> clock rate, etc. >> >> This is a single global operation that needs to happen one single time >> before anything else. > > Why? That's simply the way the code is written. >> This stage initializes the i2c_controllers[] array, since that's what >> stores all the data parsed from DT. >> >> 2) Actually initializing or using the I2C HW. That could certainly be >> part of the per-I2C-controller init() function you mention. > > For that purpose is the i2c_init function. > > And why we could not do step 1 when we do step 2? Why doing step 1 > for hw we later do not use? I suppose you could. It seems conceptually /far/ simpler to just scan the DT once up-front rather than having to defer all this stuff until you actually use an I2C bus. If you do that, how can you know how many I2C buses exist without trying to use every possible one and seeing which fail? Also, the mapping from I2C bus number to register address is only created by actually scanning the whole DT; there's no need to every I2C DT node to have a /aliases entry that dictates its U-Boot device ID, so you really do have to scan everything completely up-front before you can determine which registers to use. > saying something like this (just as an example, should be tested): > > diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c > index 9ac3969..7bbd3c7 100644 > --- a/drivers/i2c/tegra_i2c.c > +++ b/drivers/i2c/tegra_i2c.c > @@ -378,81 +378,91 @@ static int i2c_get_config(const void *blob, int > node, struct i2c_bus *i2c_bus) > * @param is_scs 1 if this HW uses a single clock source (T114+) > * @return 0 if ok, -1 on error > */ > -static int process_nodes(const void *blob, int node_list[], int count, > +static int process_nodes(const void *blob, int node_list[], > + struct i2c_adapter *adap, int count, > int is_dvc, int is_scs) > { > struct i2c_bus *i2c_bus; > - int i; > - > - /* build the i2c_controllers[] for each controller */ > - for (i = 0; i < count; i++) { > - int node = node_list[i]; > + int node = node_list[i]; How do you determine i here? There's no guarantee that all I2C DT nodes end up being processed in a single call to process_nodes(); there might be some "Tegra20 I2C", some "Tegra20 I2C DVC", some "Tegra30 I2C" modules in the same SoC.
Hello Stephen, Am 01.08.2013 07:39, schrieb Stephen Warren: > On 07/31/2013 10:32 PM, Heiko Schocher wrote: >> Hello Stephen, >> >> Am 31.07.2013 21:31, schrieb Stephen Warren: >>> On 07/30/2013 11:46 PM, Heiko Schocher wrote: >>> ... >>>> Hmm.. each i2c adapter has its own init function ... why the tegra >>>> driver do not use it? And do the necessary inits in it? So we >>>> initialize an adapater only if we use it, which is also a rule >>>> for u-boot ... >>>> >>>> I have no hw, but it should be possible to add to process_nodes() >>>> a parameter "controller_number" and call >>>> process_nodes(controller_number, ...) from the i2c drivers init >>>> function ... >>> >>> There are two steps to initializing I2C on a Tegra system: >>> >>> 1) Parsing the DT to determine which/how-many I2C adapters exist in the >>> SoC. This will yield information such as the register address of the I2C >>> adapters, which clock/reset signal they rely on, perhaps the max bus >>> clock rate, etc. >>> >>> This is a single global operation that needs to happen one single time >>> before anything else. >> >> Why? > > That's simply the way the code is written. Ok. >>> This stage initializes the i2c_controllers[] array, since that's what >>> stores all the data parsed from DT. >>> >>> 2) Actually initializing or using the I2C HW. That could certainly be >>> part of the per-I2C-controller init() function you mention. >> >> For that purpose is the i2c_init function. >> >> And why we could not do step 1 when we do step 2? Why doing step 1 >> for hw we later do not use? > > I suppose you could. It seems conceptually /far/ simpler to just scan > the DT once up-front rather than having to defer all this stuff until on the other hand we ring for every ms boot time ... and here we want to scan a complete dt with maybe a lot of nodes, we do not want to use? > you actually use an I2C bus. If you do that, how can you know how many > I2C buses exist without trying to use every possible one and seeing Do I really need to know that? > which fail? Also, the mapping from I2C bus number to register address is Hmm.. there are max TEGRA_I2C_NUM_CONTROLLERS. If one gets used, it scans the dt ... if only 1 adapter is used, only one is activated through i2c_init ... > only created by actually scanning the whole DT; there's no need to every > I2C DT node to have a /aliases entry that dictates its U-Boot device ID, > so you really do have to scan everything completely up-front before you > can determine which registers to use. But the i2c bus number is coded static all over the u-boot code (Should be changed) in the code. Saying a board has an i2c eeprom on bus "2", it calls i2c_set_bus_number(2) ... this "2" must be somewhere in the dt or? If this "2" is used on different boards with the same u-boot code only different in dt, bus 2 maybe not exist ... or if you change the order of i2c nodes in the dt "2" is no longer "2" ... or? >> saying something like this (just as an example, should be tested): >> >> diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c >> index 9ac3969..7bbd3c7 100644 >> --- a/drivers/i2c/tegra_i2c.c >> +++ b/drivers/i2c/tegra_i2c.c >> @@ -378,81 +378,91 @@ static int i2c_get_config(const void *blob, int >> node, struct i2c_bus *i2c_bus) >> * @param is_scs 1 if this HW uses a single clock source (T114+) >> * @return 0 if ok, -1 on error >> */ >> -static int process_nodes(const void *blob, int node_list[], int count, >> +static int process_nodes(const void *blob, int node_list[], >> + struct i2c_adapter *adap, int count, >> int is_dvc, int is_scs) >> { >> struct i2c_bus *i2c_bus; >> - int i; >> - >> - /* build the i2c_controllers[] for each controller */ >> - for (i = 0; i< count; i++) { >> - int node = node_list[i]; >> + int node = node_list[i]; > > How do you determine i here? There's no guarantee that all I2C DT nodes adap->hwadapnr as in drivers/i2c/tegra_i2c.c: static struct i2c_bus *tegra_i2c_get_bus(struct i2c_adapter *adap) { struct i2c_bus *bus; bus = &i2c_controllers[adap->hwadapnr]; [...] > end up being processed in a single call to process_nodes(); there might > be some "Tegra20 I2C", some "Tegra20 I2C DVC", some "Tegra30 I2C" > modules in the same SoC. bye, Heiko
Hi Heiko, On Thu, 01 Aug 2013 08:02:42 +0200, Heiko Schocher <hs@denx.de> wrote: > > I suppose you could. It seems conceptually /far/ simpler to just scan > > the DT once up-front rather than having to defer all this stuff until > > on the other hand we ring for every ms boot time ... and here we want > to scan a complete dt with maybe a lot of nodes, we do not want to > use? Scanning all of DT seems to imply it has no strict or standard ordering. Could we mandate, suggest, of make it so that all entries in the DT needed at _f time are put first, and even maybe place an "end of _f" custom marker in DT to delimit them? (I assume that, for the sake of Postel-ism, anything in DT which is not understandable is skipped, so other users of the DT than us would not even be annoyed by such a marker) This way, we'd avoid wasting time scanning most of the DT in this case. Note: I confess I don't even know at the moment how DT is structured, so I may have talked complete nonsense above. If so, please forgive me and point me to some DT 101 course for me to avoid shame (at least on this topic) in the future. > bye, > Heiko Amicalement,
Hello Albert, Am 01.08.2013 08:53, schrieb Albert ARIBAUD: > Hi Heiko, > > On Thu, 01 Aug 2013 08:02:42 +0200, Heiko Schocher<hs@denx.de> wrote: > >>> I suppose you could. It seems conceptually /far/ simpler to just scan >>> the DT once up-front rather than having to defer all this stuff until >> >> on the other hand we ring for every ms boot time ... and here we want >> to scan a complete dt with maybe a lot of nodes, we do not want to >> use? > > Scanning all of DT seems to imply it has no strict or standard > ordering. Could we mandate, suggest, of make it so that all entries in > the DT needed at _f time are put first, and even maybe place an "end of > _f" custom marker in DT to delimit them? (I assume that, for the sake of I do not know, if this is possible, as I think the DT used in U-Boot should be the same as used in linux ... or? > Postel-ism, anything in DT which is not understandable is skipped, so > other users of the DT than us would not even be annoyed by such a > marker) > > This way, we'd avoid wasting time scanning most of the DT in this case. Hmm.. why should we introduce such things, instead of scanning the node only if we need it? We have "only" the problem, that we could not write to data at this moment ... but this problem should be solved in a seperate topic. I2C is usable before relocation, the problem is in conjunction with dt, that we can not save for example the base address of the controller, which we get from the DT ... If I understand it correct! So we need an option when using dt, that we have (small ram) in which we can write some parameters parsed from dt ... I think this problem have all subsystems used before relocation. (for example: environment on a spi flash?) As Wolfgang said: "Agreed - actually we're entering an area hear that smells pretty strong like device model reorganization :-)" BTW: How is this problem solved with the device model approach? > Note: I confess I don't even know at the moment how DT is structured, so > I may have talked complete nonsense above. If so, please forgive me and > point me to some DT 101 course for me to avoid shame (at least on this > topic) in the future. bye, Heiko
Hi, On Thu, Aug 1, 2013 at 2:38 AM, Heiko Schocher <hs@denx.de> wrote: > Hello Albert, > > Am 01.08.2013 08:53, schrieb Albert ARIBAUD: > > Hi Heiko, >> >> On Thu, 01 Aug 2013 08:02:42 +0200, Heiko Schocher<hs@denx.de> wrote: >> >> I suppose you could. It seems conceptually /far/ simpler to just scan >>>> the DT once up-front rather than having to defer all this stuff until >>>> >>> >>> on the other hand we ring for every ms boot time ... and here we want >>> to scan a complete dt with maybe a lot of nodes, we do not want to >>> use? >>> >> >> Scanning all of DT seems to imply it has no strict or standard >> ordering. Could we mandate, suggest, of make it so that all entries in >> the DT needed at _f time are put first, and even maybe place an "end of >> _f" custom marker in DT to delimit them? (I assume that, for the sake of >> > > I do not know, if this is possible, as I think the DT used in U-Boot > should be the same as used in linux ... or? > > > Postel-ism, anything in DT which is not understandable is skipped, so >> other users of the DT than us would not even be annoyed by such a >> marker) >> >> This way, we'd avoid wasting time scanning most of the DT in this case. >> > > Hmm.. why should we introduce such things, instead of scanning the > node only if we need it? > > We have "only" the problem, that we could not write to data at this > moment ... but this problem should be solved in a seperate topic. > I2C is usable before relocation, the problem is in conjunction with > dt, that we can not save for example the base address of the controller, > which we get from the DT ... If I understand it correct! > > So we need an option when using dt, that we have (small ram) in which > we can write some parameters parsed from dt ... > > I think this problem have all subsystems used before relocation. > (for example: environment on a spi flash?) > > I think using a small RAM is a good approach. At least it is better than pretending there is no RAM at all. We currently have no facility to allocate RAM before relocation, other than to use the .data section. We can use global_data but that won't scale well for many drivers adding their own stuff in there. Samsung's driver uses .data, I don't think it is a big deal. One option I should mention is to decode the I2C FDT nodes each time it is needed prior to relocation (i.e. to the stack), use it for the transaction, and throw it away. This is quite painful IMO this it involves putting calls in the driver to check if we are pre-reloc and have a stack space used every time. On tegra20 this would be 130 bytes or so. I mention it because console working this way for a while (decoding the console again on every byte). Options as I see it: - just fudge it for now and use .data (deal with it later with driver model) - change the meaning of board_init_f() such that memory may be available (e.g. if run from SPL) - decode the FDT nodes every time we need them (ick) - adjust the ordering so that I2C normally happens post reloc except for specific platforms with a CONFIG defined (Heiko, the difference as I understand it is that with your patch CONFIG_HARD_I2C or CONFIG_SOFT_I2C are now defined, and so I2C happens prior to reloc now) > > As Wolfgang said: > "Agreed - actually we're entering an area hear that smells pretty > strong like device model reorganization :-)" > > BTW: How is this problem solved with the device model approach? More that we need to solve it, probably with a limited malloc() pre-reloc. Regards, Simon
Hello Simon, Am 01.08.2013 16:22, schrieb Simon Glass: > Hi, > > On Thu, Aug 1, 2013 at 2:38 AM, Heiko Schocher<hs@denx.de> wrote: > >> Hello Albert, >> >> Am 01.08.2013 08:53, schrieb Albert ARIBAUD: >> >> Hi Heiko, >>> >>> On Thu, 01 Aug 2013 08:02:42 +0200, Heiko Schocher<hs@denx.de> wrote: >>> >>> I suppose you could. It seems conceptually /far/ simpler to just scan >>>>> the DT once up-front rather than having to defer all this stuff until >>>>> >>>> >>>> on the other hand we ring for every ms boot time ... and here we want >>>> to scan a complete dt with maybe a lot of nodes, we do not want to >>>> use? >>>> >>> >>> Scanning all of DT seems to imply it has no strict or standard >>> ordering. Could we mandate, suggest, of make it so that all entries in >>> the DT needed at _f time are put first, and even maybe place an "end of >>> _f" custom marker in DT to delimit them? (I assume that, for the sake of >>> >> >> I do not know, if this is possible, as I think the DT used in U-Boot >> should be the same as used in linux ... or? >> >> >> Postel-ism, anything in DT which is not understandable is skipped, so >>> other users of the DT than us would not even be annoyed by such a >>> marker) >>> >>> This way, we'd avoid wasting time scanning most of the DT in this case. >>> >> >> Hmm.. why should we introduce such things, instead of scanning the >> node only if we need it? >> >> We have "only" the problem, that we could not write to data at this >> moment ... but this problem should be solved in a seperate topic. >> I2C is usable before relocation, the problem is in conjunction with >> dt, that we can not save for example the base address of the controller, >> which we get from the DT ... If I understand it correct! >> >> So we need an option when using dt, that we have (small ram) in which >> we can write some parameters parsed from dt ... >> >> I think this problem have all subsystems used before relocation. >> (for example: environment on a spi flash?) >> >> > I think using a small RAM is a good approach. At least it is better than > pretending there is no RAM at all. We currently have no facility to > allocate RAM before relocation, other than to use the .data section. We can > use global_data but that won't scale well for many drivers adding their own > stuff in there. Samsung's driver uses .data, I don't think it is a big deal. > > One option I should mention is to decode the I2C FDT nodes each time it is > needed prior to relocation (i.e. to the stack), use it for the transaction, > and throw it away. This is quite painful IMO this it involves putting calls > in the driver to check if we are pre-reloc and have a stack space used > every time. On tegra20 this would be 130 bytes or so. I mention it because > console working this way for a while (decoding the console again on every > byte). > > Options as I see it: > > - just fudge it for now and use .data (deal with it later with driver model) > - change the meaning of board_init_f() such that memory may be available > (e.g. if run from SPL) > - decode the FDT nodes every time we need them (ick) Why? after relocation it is needed exactly once. You can do something like that in the i2c_init function: + bus = tegra_i2c_get_bus(adap); + if (bus) + return 0; If you get a bus with tegra_i2c_get_bus(adap), it is "fdt initialized" If you not get a bus ... init it ... only once The problem before relocation could be solved with .data or later with the driver model ... > - adjust the ordering so that I2C normally happens post reloc except for > specific platforms with a CONFIG defined (Heiko, the difference as I > understand it is that with your patch CONFIG_HARD_I2C or CONFIG_SOFT_I2C > are now defined, and so I2C happens prior to reloc now) Hmm.. BTW: CONFIG_SOFT_I2C is no longer in the code :-) And there are a lot of boards that use i2c before relocation, that is not a new feature. The problem pop up, because in arch/arm/lib/board.c@init_func_i2c() the new code calls i2c_init_all() instead i2c_init() ... This i2c_init_all() was introduced to init fix the SPD EEPROM bus and call i2c_init_board() before it... So we can call here again i2c_init() I think ... but, i2c_init() also calls i2c_init_bus() and this i2c_init from the tegra driver, which sets speed and return 0 ... what is not really clean, it just works, because tegra_i2c_set_bus_speed() checks if the bus is valid, if not only returns ... but i2c_init_board is not called so early, which is the cause for our problem with the tegra driver and the new framework, as i2c_init_board() hysterically was introduced to unblock blocked i2c busses (Correct me, if I am wrong) My prefered way is still: - replace i2c_init_all() through i2c_init() in arch/arm/lib/board.c@init_func_i2c() to get tegra again working In the long term when all i2c drivers use the new framework init_func_i2c() will no longer necessary :-) As the goal is to change the i2c api, so all i2c functions pass "bus" as a parameter, and i2c_set_bus_num() get a static function in drivers/i2c/i2c_core.c no need longer for storing the old bus, setting the new bus, doing i2c work, set the old bus, as this sequence is spread over the hole u-boot code. - decoding fdt node in tegra_i2c_init, as I think it is the right place for it. optional more or less, but to prevent in future again such "order" problems ... >> As Wolfgang said: >> "Agreed - actually we're entering an area hear that smells pretty >> strong like device model reorganization :-)" >> >> BTW: How is this problem solved with the device model approach? > > > More that we need to solve it, probably with a limited malloc() pre-reloc. bye, Heiko
Hi Heiko, On Thu, 01 Aug 2013 10:38:15 +0200, Heiko Schocher <hs@denx.de> wrote: > Hello Albert, > > Am 01.08.2013 08:53, schrieb Albert ARIBAUD: > > Hi Heiko, > > > > On Thu, 01 Aug 2013 08:02:42 +0200, Heiko Schocher<hs@denx.de> wrote: > > > >>> I suppose you could. It seems conceptually /far/ simpler to just scan > >>> the DT once up-front rather than having to defer all this stuff until > >> > >> on the other hand we ring for every ms boot time ... and here we want > >> to scan a complete dt with maybe a lot of nodes, we do not want to > >> use? > > > > Scanning all of DT seems to imply it has no strict or standard > > ordering. Could we mandate, suggest, of make it so that all entries in > > the DT needed at _f time are put first, and even maybe place an "end of > > _f" custom marker in DT to delimit them? (I assume that, for the sake of > > I do not know, if this is possible, as I think the DT used in U-Boot > should be the same as used in linux ... or? > > > Postel-ism, anything in DT which is not understandable is skipped, so > > other users of the DT than us would not even be annoyed by such a > > marker) > > > > This way, we'd avoid wasting time scanning most of the DT in this case. > > Hmm.. why should we introduce such things, instead of scanning the > node only if we need it? From the "we want to scan a complete dt with maybe a lot of nodes, we do not want to use?" above, I inferred that one problem here was having to waste time going through the whole DT looking only for info needed at _f stage. This is why I made the suggestion above for that problem. Sorry if I did not understand this properly. > We have "only" the problem, that we could not write to data at this > moment ... but this problem should be solved in a seperate topic. > I2C is usable before relocation, the problem is in conjunction with > dt, that we can not save for example the base address of the controller, > which we get from the DT ... If I understand it correct! > > So we need an option when using dt, that we have (small ram) in which > we can write some parameters parsed from dt ... > > I think this problem have all subsystems used before relocation. > (for example: environment on a spi flash?) > > As Wolfgang said: > "Agreed - actually we're entering an area hear that smells pretty > strong like device model reorganization :-)" > > BTW: How is this problem solved with the device model approach? > > > Note: I confess I don't even know at the moment how DT is structured, so > > I may have talked complete nonsense above. If so, please forgive me and > > point me to some DT 101 course for me to avoid shame (at least on this > > topic) in the future. > > bye, > Heiko Amicalement,
Hi Simon, On Thu, 1 Aug 2013 08:22:55 -0600, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On Thu, Aug 1, 2013 at 2:38 AM, Heiko Schocher <hs@denx.de> wrote: > > > Hello Albert, > > > > Am 01.08.2013 08:53, schrieb Albert ARIBAUD: > > > > Hi Heiko, > >> > >> On Thu, 01 Aug 2013 08:02:42 +0200, Heiko Schocher<hs@denx.de> wrote: > >> > >> I suppose you could. It seems conceptually /far/ simpler to just scan > >>>> the DT once up-front rather than having to defer all this stuff until > >>>> > >>> > >>> on the other hand we ring for every ms boot time ... and here we want > >>> to scan a complete dt with maybe a lot of nodes, we do not want to > >>> use? > >>> > >> > >> Scanning all of DT seems to imply it has no strict or standard > >> ordering. Could we mandate, suggest, of make it so that all entries in > >> the DT needed at _f time are put first, and even maybe place an "end of > >> _f" custom marker in DT to delimit them? (I assume that, for the sake of > >> > > > > I do not know, if this is possible, as I think the DT used in U-Boot > > should be the same as used in linux ... or? > > > > > > Postel-ism, anything in DT which is not understandable is skipped, so > >> other users of the DT than us would not even be annoyed by such a > >> marker) > >> > >> This way, we'd avoid wasting time scanning most of the DT in this case. > >> > > > > Hmm.. why should we introduce such things, instead of scanning the > > node only if we need it? > > > > We have "only" the problem, that we could not write to data at this > > moment ... but this problem should be solved in a seperate topic. > > I2C is usable before relocation, the problem is in conjunction with > > dt, that we can not save for example the base address of the controller, > > which we get from the DT ... If I understand it correct! > > > > So we need an option when using dt, that we have (small ram) in which > > we can write some parameters parsed from dt ... > > > > I think this problem have all subsystems used before relocation. > > (for example: environment on a spi flash?) > > > > > I think using a small RAM is a good approach. At least it is better than > pretending there is no RAM at all. We currently have no facility to > allocate RAM before relocation, other than to use the .data section. We can > use global_data but that won't scale well for many drivers adding their own > stuff in there. Samsung's driver uses .data, I don't think it is a big deal. What about targets which do not have such a small RAM available? > One option I should mention is to decode the I2C FDT nodes each time it is > needed prior to relocation (i.e. to the stack), use it for the transaction, > and throw it away. This is quite painful IMO this it involves putting calls > in the driver to check if we are pre-reloc and have a stack space used > every time. On tegra20 this would be 130 bytes or so. I mention it because > console working this way for a while (decoding the console again on every > byte). > > Options as I see it: > > - just fudge it for now and use .data (deal with it later with driver model) > - change the meaning of board_init_f() such that memory may be available > (e.g. if run from SPL) > - decode the FDT nodes every time we need them (ick) > - adjust the ordering so that I2C normally happens post reloc except for > specific platforms with a CONFIG defined (Heiko, the difference as I > understand it is that with your patch CONFIG_HARD_I2C or CONFIG_SOFT_I2C > are now defined, and so I2C happens prior to reloc now) > > > > > > As Wolfgang said: > > "Agreed - actually we're entering an area hear that smells pretty > > strong like device model reorganization :-)" > > > > BTW: How is this problem solved with the device model approach? > > > More that we need to solve it, probably with a limited malloc() pre-reloc. > > Regards, > Simon Amicalement,
On 08/01/2013 12:02 AM, Heiko Schocher wrote: > Am 01.08.2013 07:39, schrieb Stephen Warren: >> On 07/31/2013 10:32 PM, Heiko Schocher wrote: >>> Am 31.07.2013 21:31, schrieb Stephen Warren: >>>> On 07/30/2013 11:46 PM, Heiko Schocher wrote: ... >>>> 2) Actually initializing or using the I2C HW. That could certainly be >>>> part of the per-I2C-controller init() function you mention. >>> >>> For that purpose is the i2c_init function. >>> >>> And why we could not do step 1 when we do step 2? Why doing step 1 >>> for hw we later do not use? >> >> I suppose you could. It seems conceptually /far/ simpler to just scan >> the DT once up-front rather than having to defer all this stuff until > > on the other hand we ring for every ms boot time ... and here we want > to scan a complete dt with maybe a lot of nodes, we do not want to > use? > >> you actually use an I2C bus. If you do that, how can you know how many >> I2C buses exist without trying to use every possible one and seeing > > Do I really need to know that? Well, with the recent patches, U-Boot prints out a list of valid I2C adapters at boot. So, I suppose someone must have thought it a good idea! Perhaps the most relevant reason: why on earth wouldn't a user want to run e.g. "i2c list" or "i2c dev" and get back a list of valid I2C adapters, as opposed to poking around in the dark to see which exist? >> which fail? Also, the mapping from I2C bus number to register address is > > Hmm.. there are max TEGRA_I2C_NUM_CONTROLLERS. If one gets used, > it scans the dt ... if only 1 adapter is used, only one is activated > through i2c_init ... > >> only created by actually scanning the whole DT; there's no need to every >> I2C DT node to have a /aliases entry that dictates its U-Boot device ID, >> so you really do have to scan everything completely up-front before you >> can determine which registers to use. > > But the i2c bus number is coded static all over the u-boot code (Should > be changed) in the code. Saying a board has an i2c eeprom on bus "2", > it calls i2c_set_bus_number(2) ... this "2" must be somewhere in the dt > or? Yes, the "2" must come from DT, or DT is pointless. We simply have to get rid of all the hard-coding. If we can't do that, then I'd strongly prefer to revert all the DT mess, and put the list of HW modules back into the source code. DT isn't adding anything at all unless we can actually use it exclusively. Given how long this discussion is going on, can we please just revert the commit so that the code works for everyone who's trying to use it, then fix the problem later?
On 08/01/2013 12:53 AM, Albert ARIBAUD wrote: > Hi Heiko, > > On Thu, 01 Aug 2013 08:02:42 +0200, Heiko Schocher <hs@denx.de> wrote: > >>> I suppose you could. It seems conceptually /far/ simpler to just scan >>> the DT once up-front rather than having to defer all this stuff until >> >> on the other hand we ring for every ms boot time ... and here we want >> to scan a complete dt with maybe a lot of nodes, we do not want to >> use? > > Scanning all of DT seems to imply it has no strict or standard > ordering. Could we mandate, suggest, of make it so that all entries in > the DT needed at _f time are put first, and even maybe place an "end of > _f" custom marker in DT to delimit them? (I assume that, for the sake of > Postel-ism, anything in DT which is not understandable is skipped, so > other users of the DT than us would not even be annoyed by such a > marker) DT nodes are explicitly unordered, and there is and should be no guaranteed correlation between the order of entries in DT and when drivers that handle them are probed. In particular, for the Tegra DTs, nodes are supposed to be sorted by register address to keep things neat.
Hi Albert, On Thu, Aug 1, 2013 at 2:16 PM, Albert ARIBAUD <albert.u.boot@aribaud.net>wrote: > Hi Simon, > > On Thu, 1 Aug 2013 08:22:55 -0600, Simon Glass <sjg@chromium.org> wrote: > > > Hi, > > > > On Thu, Aug 1, 2013 at 2:38 AM, Heiko Schocher <hs@denx.de> wrote: > > > > > Hello Albert, > > > > > > Am 01.08.2013 08:53, schrieb Albert ARIBAUD: > > > > > > Hi Heiko, > > >> > > >> On Thu, 01 Aug 2013 08:02:42 +0200, Heiko Schocher<hs@denx.de> > wrote: > > >> > > >> I suppose you could. It seems conceptually /far/ simpler to just scan > > >>>> the DT once up-front rather than having to defer all this stuff > until > > >>>> > > >>> > > >>> on the other hand we ring for every ms boot time ... and here we want > > >>> to scan a complete dt with maybe a lot of nodes, we do not want to > > >>> use? > > >>> > > >> > > >> Scanning all of DT seems to imply it has no strict or standard > > >> ordering. Could we mandate, suggest, of make it so that all entries in > > >> the DT needed at _f time are put first, and even maybe place an "end > of > > >> _f" custom marker in DT to delimit them? (I assume that, for the sake > of > > >> > > > > > > I do not know, if this is possible, as I think the DT used in U-Boot > > > should be the same as used in linux ... or? > > > > > > > > > Postel-ism, anything in DT which is not understandable is skipped, so > > >> other users of the DT than us would not even be annoyed by such a > > >> marker) > > >> > > >> This way, we'd avoid wasting time scanning most of the DT in this > case. > > >> > > > > > > Hmm.. why should we introduce such things, instead of scanning the > > > node only if we need it? > > > > > > We have "only" the problem, that we could not write to data at this > > > moment ... but this problem should be solved in a seperate topic. > > > I2C is usable before relocation, the problem is in conjunction with > > > dt, that we can not save for example the base address of the > controller, > > > which we get from the DT ... If I understand it correct! > > > > > > So we need an option when using dt, that we have (small ram) in which > > > we can write some parameters parsed from dt ... > > > > > > I think this problem have all subsystems used before relocation. > > > (for example: environment on a spi flash?) > > > > > > > > I think using a small RAM is a good approach. At least it is better than > > pretending there is no RAM at all. We currently have no facility to > > allocate RAM before relocation, other than to use the .data section. We > can > > use global_data but that won't scale well for many drivers adding their > own > > stuff in there. Samsung's driver uses .data, I don't think it is a big > deal. > > What about targets which do not have such a small RAM available? > Presumably such targets do not use SPL, and perhaps don't use FDT at present? > > > One option I should mention is to decode the I2C FDT nodes each time it > is > > needed prior to relocation (i.e. to the stack), use it for the > transaction, > > and throw it away. This is quite painful IMO this it involves putting > calls > > in the driver to check if we are pre-reloc and have a stack space used > > every time. On tegra20 this would be 130 bytes or so. I mention it > because > > console working this way for a while (decoding the console again on every > > byte). > > > > Options as I see it: > > > > - just fudge it for now and use .data (deal with it later with driver > model) > > - change the meaning of board_init_f() such that memory may be available > > (e.g. if run from SPL) > > - decode the FDT nodes every time we need them (ick) > > - adjust the ordering so that I2C normally happens post reloc except for > > specific platforms with a CONFIG defined (Heiko, the difference as I > > understand it is that with your patch CONFIG_HARD_I2C or CONFIG_SOFT_I2C > > are now defined, and so I2C happens prior to reloc now) > > > > > > > > > > As Wolfgang said: > > > "Agreed - actually we're entering an area hear that smells pretty > > > strong like device model reorganization :-)" > > > > > > BTW: How is this problem solved with the device model approach? > > > > > > More that we need to solve it, probably with a limited malloc() > pre-reloc. > > Regards, Simon
diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c index 9ac3969..7bbd3c7 100644 --- a/drivers/i2c/tegra_i2c.c +++ b/drivers/i2c/tegra_i2c.c @@ -378,81 +378,91 @@ static int i2c_get_config(const void *blob, int node, struct i2c_bus *i2c_bus) * @param is_scs 1 if this HW uses a single clock source (T114+) * @return 0 if ok, -1 on error */ -static int process_nodes(const void *blob, int node_list[], int count, +static int process_nodes(const void *blob, int node_list[], + struct i2c_adapter *adap, int count, int is_dvc, int is_scs) { struct i2c_bus *i2c_bus; - int i; - - /* build the i2c_controllers[] for each controller */ - for (i = 0; i < count; i++) { - int node = node_list[i]; + int node = node_list[i]; - if (node <= 0) - continue; + bus = &i2c_controllers[adap->hwadapnr]; + i2c_bus->id = adap->hwadapnr; - i2c_bus = &i2c_controllers[i]; - i2c_bus->id = i; + if (node <= 0) + continue; - if (i2c_get_config(blob, node, i2c_bus)) { - printf("i2c_init_board: failed to decode bus %d\n", i); - return -1; - } + if (i2c_get_config(blob, node, i2c_bus)) { + printf("i2c_init_board: failed to decode bus %d\n", i); + return -1; + } - i2c_bus->is_scs = is_scs; + i2c_bus->is_scs = is_scs; - i2c_bus->is_dvc = is_dvc; - if (is_dvc) { - i2c_bus->control = - &((struct dvc_ctlr *)i2c_bus->regs)->control; - } else { - i2c_bus->control = &i2c_bus->regs->control; - } - debug("%s: controller bus %d at %p, periph_id %d, speed %d: ", - is_dvc ? "dvc" : "i2c", i, i2c_bus->regs, - i2c_bus->periph_id, i2c_bus->speed); - i2c_init_controller(i2c_bus); - debug("ok\n"); - i2c_bus->inited = 1; - - /* Mark position as used */ - node_list[i] = -1; + i2c_bus->is_dvc = is_dvc; + if (is_dvc) { + i2c_bus->control = + &((struct dvc_ctlr *)i2c_bus->regs)->control; + } else { + i2c_bus->control = &i2c_bus->regs->control; } + debug("%s: controller bus %d at %p, periph_id %d, speed %d: ", + is_dvc ? "dvc" : "i2c", i, i2c_bus->regs, + i2c_bus->periph_id, i2c_bus->speed); + i2c_init_controller(i2c_bus); + debug("ok\n"); + i2c_bus->inited = 1; + + /* Mark position as used */ + node_list[i] = -1; return 0; } /* Sadly there is no error return from this function */ -void i2c_init_board(void) +static int tegra_i2c_init_board(struct i2c_adapter *adap) { + struct i2c_bus *i2c_bus; int node_list[TEGRA_I2C_NUM_CONTROLLERS]; const void *blob = gd->fdt_blob; int count; + bus = tegra_i2c_get_bus(adap); + if (bus) + return 0; + /* First check for newer (T114+) I2C ports */ count = fdtdec_find_aliases_for_id(blob, "i2c", COMPAT_NVIDIA_TEGRA114_I2C, node_list, TEGRA_I2C_NUM_CONTROLLERS); - if (process_nodes(blob, node_list, count, 0, 1)) - return; + if (process_nodes(blob, node_list, adap, count, 0, 1)) + return -1; /* Now get the older (T20/T30) normal I2C ports */ count = fdtdec_find_aliases_for_id(blob, "i2c", COMPAT_NVIDIA_TEGRA20_I2C, node_list, TEGRA_I2C_NUM_CONTROLLERS); - if (process_nodes(blob, node_list, count, 0, 0)) - return; + if (process_nodes(blob, node_list, adap, count, 0, 0)) + return -1; /* Now look for dvc ports */ count = fdtdec_add_aliases_for_id(blob, "i2c", COMPAT_NVIDIA_TEGRA20_DVC, node_list, TEGRA_I2C_NUM_CONTROLLERS); - if (process_nodes(blob, node_list, count, 1, 0)) - return; + if (process_nodes(blob, node_list, adap, count, 1, 0)) + return -1; + + return 0; } static void tegra_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr) { + int ret; + + ret = tegra_i2c_init_board(adap); + if (ret) { + printf("Could not decode bus: %d\n", adap->hwadapnr); + return; + } /* This will override the speed selected in the fdt for that port */ debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr); i2c_set_bus_speed(speed);