Re: [U-Boot] [PATCH] ARM: compiler options cleanup - improve tool chain support
On Mon, 17 Aug 2009, Wolfgang Denk wrote: Ack-by: Sergey Kubushyn --- > For some time there have been repeated reports about build problems > with some ARM (cross) tool chains. Especially issues about > (in)compatibility with the tool chain provided runtime support > library libgcc.a caused to add and support a private implementation > of such runtime support code in U-Boot. A closer look at the code > indicated that some of these issues are actually home-made. This > patch attempts to clean up some of the most obvious problems and make > building of U-Boot with different tool chains easier: > > - Even though all ARM systems basicy used the same compiler options > to select a specific ABI from the tool chain, the code for this was > distributed over all cpu/*/config.mk files. We move this one level > up into lib_arm/config.mk instead. > > - So far, we only checked if "-mapcs-32" was supported by the tool > chain; if yes, this was used, if not, "-mabi=apcs-gnu" was > selected, no matter if the tool chain actually understood this > option. There was no support for EABI conformant tool chains. > This patch implements the following logic: > > 1) If the tool chain supports > "-mabi=aapcs-linux -mno-thumb-interwork" > we use these options (EABI conformant tool chain). > 2) Otherwise, we check first if > "-mapcs-32" > is supported, and then check for > "-mabi=apcs-gnu" > If one test succeeds, we use the first found option. > 3) In case 2), we also test if "-mno-thumb-interwork", and use > this if the test succeeds. [For "-mabi=aapcs-linux" we set > "-mno-thumb-interwork" mandatorily.] > > This way we use a similar logic for the compile options as the > Linux kenrel does. > > - Some EABI conformant tool chains cause external references to > utility functions like raise(); such functions are provided in the > new file lib_arm/eabi_compat.c > > Note that lib_arm/config.mk gets parsed several times, so we must > make sure to add eabi_compat.o only once to the linker list. > > Signed-off-by: Wolfgang Denk > Cc: Jean-Christophe PLAGNIOL-VILLARD > Cc: Dirk Behme > Cc: Magnus Lilja > Cc: Tom Rix > Cc: Prafulla Wadaskar > --- --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] davinci_emac.c:*: warning: duplicate `volatile'
On Mon, 17 Aug 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > what compiler do you use? I just compiled all the davinci boards with > > GNUEABI GCC 4.4.1 (binutils ver.2.19.1) and I didn't get a single compiler > > warning. > > I was playing with old tool chains actually. That was with gcc 3.3.x. Ah, that's ancient... I don't think we need to something for this. I betcha it won't compile at all with something like 2.x.x but who cares? :) > > There are 2 separate issues but they are not related to the above > > description. > > > > First is that it fails to compile with GNUEABI GCC but that is addressed in > > your recent arm build patch that works OK. > > Thanks for the confirmation! Much appreciated. Could you please send > an Ack-by or Tested-by for that patch? Thanks in advance. Sure I will. > > The second one is that GCC 4.4.1 (don't know about the older ones, don't > > have those on my machine now) generates an error for inline weak functions > > in lib_arm/board.c (and I think it is totally correct -- what is an inline > > weak function?) Here is the failed build log excertpt: > > A patch for this has been posted some time ago. See > http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/65397/focus=65559 > > we're just waiting for the ARM custodian to take appropriate action. OK. > > As for those "duplicate `volatile'" warnings I don't see them (and have > > never seen them before) so I don't know what to fix. > > > > I can send you my build logs off the list if you want. > > Well, here is an example - building davinci_schmoogie using ELDK 3.1.1 > (gcc version 3.3.3): > > davinci_emac.c:82: warning: duplicate `volatile' > davinci_emac.c:83: warning: duplicate `volatile' > davinci_emac.c:84: warning: duplicate `volatile' > davinci_emac.c:85: warning: duplicate `volatile' > davinci_emac.c: In function `davinci_eth_open': > davinci_emac.c:257: warning: duplicate `volatile' > davinci_emac.c: In function `davinci_eth_rcv_packet': > davinci_emac.c:475: warning: duplicate `volatile' > davinci_emac.c:476: warning: duplicate `volatile' > davinci_emac.c:477: warning: duplicate `volatile' > davinci_emac.c:496: warning: duplicate `volatile' Once again -- I don't think it requires any action. It builds without a single warning with newer compilers, even with 4.2.x less for 4.4.1... --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] davinci_emac.c:*: warning: duplicate `volatile'
On Sun, 16 Aug 2009, Wolfgang Denk wrote: > Dear Sergey, > > in include/asm-arm/arch-davinci/emac_defs.h you declare emac_desc with > the "volatile" attribute, but when using it, "volatile" is frequently > added again, resulting in complier warnings like these: > > Configuring for davinci_sffsdr board... > davinci_emac.c:82: warning: duplicate `volatile' > davinci_emac.c:83: warning: duplicate `volatile' > davinci_emac.c:84: warning: duplicate `volatile' > davinci_emac.c:85: warning: duplicate `volatile' > davinci_emac.c: In function `davinci_eth_open': > davinci_emac.c:257: warning: duplicate `volatile' > davinci_emac.c: In function `davinci_eth_rcv_packet': > davinci_emac.c:475: warning: duplicate `volatile' > davinci_emac.c:476: warning: duplicate `volatile' > davinci_emac.c:477: warning: duplicate `volatile' > davinci_emac.c:496: warning: duplicate `volatile' > > > Can you please provide a fix? Thanks in advance. Hey, what compiler do you use? I just compiled all the davinci boards with GNUEABI GCC 4.4.1 (binutils ver.2.19.1) and I didn't get a single compiler warning. There are 2 separate issues but they are not related to the above description. First is that it fails to compile with GNUEABI GCC but that is addressed in your recent arm build patch that works OK. The second one is that GCC 4.4.1 (don't know about the older ones, don't have those on my machine now) generates an error for inline weak functions in lib_arm/board.c (and I think it is totally correct -- what is an inline weak function?) Here is the failed build log excertpt: === Cut === board.c:127: error: inline function 'coloured_LED_init' cannot be declared weak board.c:129: error: inline function 'red_LED_on' cannot be declared weak board.c:131: error: inline function 'red_LED_off' cannot be declared weak board.c:133: error: inline function 'green_LED_on' cannot be declared weak board.c:135: error: inline function 'green_LED_off' cannot be declared weak board.c:137: error: inline function 'yellow_LED_on' cannot be declared weak board.c:139: error: inline function 'yellow_LED_off' cannot be declared weak board.c:141: error: inline function 'blue_LED_on' cannot be declared weak board.c:143: error: inline function 'blue_LED_off' cannot be declared weak make[1]: *** [board.o] Error 1 make[1]: Leaving directory /usr/src/U-Boot/Davinci/u-boot/lib_arm' make: *** [lib_arm/libarm.a] Error 2 === Cut === Do you want me to send a fix for those? It hardly makes sense, you can do it yourself by removing "inline" qualifier at those 9 lines but I can send a patch if you want me to. As for those "duplicate `volatile'" warnings I don't see them (and have never seen them before) so I don't know what to fix. I can send you my build logs off the list if you want. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] davinci_emac.c:*: warning: duplicate `volatile'
On Sun, 16 Aug 2009, Wolfgang Denk wrote: OK, I'll try to fix it tomorrow or Monday, very busy right now... > Dear Sergey, > > in include/asm-arm/arch-davinci/emac_defs.h you declare emac_desc with > the "volatile" attribute, but when using it, "volatile" is frequently > added again, resulting in complier warnings like these: > > Configuring for davinci_sffsdr board... > davinci_emac.c:82: warning: duplicate `volatile' > davinci_emac.c:83: warning: duplicate `volatile' > davinci_emac.c:84: warning: duplicate `volatile' > davinci_emac.c:85: warning: duplicate `volatile' > davinci_emac.c: In function `davinci_eth_open': > davinci_emac.c:257: warning: duplicate `volatile' > davinci_emac.c: In function `davinci_eth_rcv_packet': > davinci_emac.c:475: warning: duplicate `volatile' > davinci_emac.c:476: warning: duplicate `volatile' > davinci_emac.c:477: warning: duplicate `volatile' > davinci_emac.c:496: warning: duplicate `volatile' > > > Can you please provide a fix? Thanks in advance. > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de > Drawing on my fine command of language, I said nothing. > --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] RFC: "make DESTDIR=xxx install" ?
On Thu, 13 Aug 2009, Ulf Samuelsson wrote:
> Many packages support installing the resulting binary in another
> location, but U-Boot does not.
>
> When you use buildsystems like buildroot and openembedded,
> you want to collect the end result in a target directory,
> and while you can use internal knowledge about u-boot
> to do so, it seems cleaner to me, to do a "make DESTDIR install".
>
> Since you may want to put the binaries for several
> boards in the same directory (like /tftpboot)
> it is not always good to call the binary simply u-boot.bin.
>
> I guess "make DESTDIR= TARGET= install" would work
>
> Alternatively, we collect the final binary from several variables,
>
> openembedded typically calls the end binary:
> ${MACHINE}-u-boot-${U_BOOT_VERSION}-${REVISION}.bin
>
> Feedback?
IMHO it is not worth the effort... U-Boot builds its binary in source root
and there is no "make install" at all. When one builds RPM or whatever
packages (as I do) it is not big deal to move the resulting binary elsewhere
with a single "mv" command.
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] 85xx: MPC8536DS board does not build
On Mon, 10 Aug 2009, Peter Tyser wrote: > On Mon, 2009-08-10 at 15:27 -0400, Jerry Van Baren wrote: > > Kumar Gala wrote: > > > On Aug 10, 2009, at 1:59 PM, Zang Roy-R61911 wrote: > > > > > >> > > >>> -Original Message- > > >>> From: Kumar Gala [mailto:ga...@kernel.crashing.org] > > >>> Sent: Monday, August 10, 2009 13:41 PM > > >>> To: Wolfgang Denk > > >>> Cc: U-Boot-Users ML; Zang Roy-R61911 > > >>> Subject: Re: 85xx: MPC8536DS board does not build > > >>> > > >>> > > >>> On Aug 10, 2009, at 1:22 PM, Wolfgang Denk wrote: > > >>> > > Dear Kumar Gala, > > > > In message <0EB7516A-2F14-42F7- > > a6ed-555adfab3...@kernel.crashing.org> you wrote: > > >> Allocate more space for U-Boot? > > > I might turn of BEDBUG as its never been properly enabled on > > > e500/85xx > > > platforms. > > Is there any problem with the bigger image which I don't > understand > > yet? Normally we just move down the TEXT_BASE by a sector, > > >>> and that's > > it. > > >>> Not specifically, its just that ever 85xx image to date has been > > >>> 512k. I'm just trying to avoid this being the first one that > > >>> changes > > >>> that historic fact. Especially since compilers like gcc-4.3 seem > to > > >>> be able to fit the size in 512k. > > >> We may have more requirements to support graphic in u-boot. > > >> Sooner and later, the size will exceed 512K. Should we have some > plan > > >> for this? > > > > > > So if we are going to increase the limit from 512k do we go to 768k > or > > > 1M? (Sector size on the board appears to 128k) > > > > > > I would also like to know how big the flashes are on some of the > other > > > 85xx boards that u-boot supports. > > > > > > - k > > > > Hi Kumar, Roy, > > > > 512K is pretty big for u-boot (not unheard of, but still...). Is it > > really 512K or is it using a full page to hold the boot page (top 4K > of > > memory) and one page for the env (unavoidable): > > > > + > 0x1__ > > | One sector dedicated for the power up page (only using 4K) > > + > 0x0_F800_ > > | One sector dedicated for the env > > + > 0x0_F000_ > > | Two sectors of u-boot > > + > 0x0_E800_ > > | > > + > 0x0_E000_ > > > > > > If that is the case, you can gain a sector (less 4K) by rearranging > your > > memory map: > > + > 0x1__ > > | One page (4K) of power up vector, the rest is u-boot > > + > 0x0_F800_ > > | > > + > 0x0_F000_ > > | Three sectors (less 4K) of u-boot > > + > 0x0_E800_ > > | One sector dedicated for the env > > + > 0x0_E000_ > > > > This also makes reprogramming u-boot nicer because your power up > vector > > and u-boot itself are contiguous. > > Hi Jerry, > Currently a sector shouldn't be wasted just for the 4K boot page. Your > second diagram above is similar to current operation - a chunk of the 4k > bootpage is wasted/unused, but other u-boot code shares the same flash > sector with the 4K boot page. So a little space may be wasted, but not > too much (ie less than 4K). That is where top boot block flashes come handy... It is not just that 128K sector is a huge waste for 4K boot block, the same is true for environment... --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] 85xx: MPC8536DS board does not build
On Mon, 10 Aug 2009, Kumar Gala wrote: > > On Aug 10, 2009, at 1:22 PM, Wolfgang Denk wrote: > > > Dear Kumar Gala, > > > > In message <0EB7516A-2F14-42F7- > > a6ed-555adfab3...@kernel.crashing.org> you wrote: > >> > >>> Allocate more space for U-Boot? > >> > >> I might turn of BEDBUG as its never been properly enabled on > >> e500/85xx > >> platforms. > > > > Is there any problem with the bigger image which I don't understand > > yet? Normally we just move down the TEXT_BASE by a sector, and that's > > it. > > Not specifically, its just that ever 85xx image to date has been > 512k. I'm just trying to avoid this being the first one that changes > that historic fact. Especially since compilers like gcc-4.3 seem to > be able to fit the size in 512k. Off of 512K something like 1/3 is empty in e.g. MPC8548CDS. The very last sector contains fixed location power-on boot vector, the beginning of those 512K has actual U-Boot code and the hole between them is big enough to fit an entire sector for environment. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] i2c: rework multibus/multiadapter functionality
On Sat, 18 Jul 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you > wrote: > > > > > Will you post patches for this merge window? > > > > Are we back at square one or we're gonna implement that weirdo with > > accessing global variables from object methods? > > I suggest you re-read Heiko's posting from end of March [1], and have > a look at the code he provided. Constructive comments are welcome, > then. > > [1] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/56416 Ah, OK, that's where I quit. I think this entire undertaking is long dead. I seriously doubt somebody will join a discussion if one happened to start. And even if it resurrected it should probably be started from scratch because even I'm having difficulties recalling the details. But anyway, let's see if somebody have something to say on the subject... --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] i2c: rework multibus/multiadapter functionality
On Sat, 18 Jul 2009, Wolfgang Denk wrote: > Dear Heiko Schocher, > > In message <49c89574.9040...@denx.de> you wrote: > > > > I want now, because the merge window is open again, restart the > > the multibus/multiadapter discussion. > > No negative feedback has been posted, as far as I can tell. > > Will you post patches for this merge window? Are we back at square one or we're gonna implement that weirdo with accessing global variables from object methods? I have the entire I2C layer under my $(BOARD)/ now because it won't fit in the existing U-Boot tree. And we have exactly the same problem with USB (I wouldn't even start bragging about EHCI.) --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] U-book and GPLv3?
On Mon, 6 Jul 2009, Jon Smirl wrote: > On Mon, Jul 6, 2009 at 6:55 AM, Wolfgang Denk wrote: > > Dear Richard, > > > > In message you wrote: > >> > >> Have you considered moving U-boot to "GPLv3-or-later"? > > If u-boot goes GPLv3 it will simply cause the people that need secure > boot to switch boot loaders. That will result in a loss of u-boot > developers. It is also a lot of pointless administrative work changing > licenses and rewriting code. Even worse, you'll could cause a u-boot > fork at the point of the license change since the code in front of the > change will still be licensed GPLv2 and it can't be retracted. > > Why do you want to take on a bunch of pointless administrative work > that is going to result in losing developers? That time could be spent > productively writing code. I totally agree with all the above. And if this happened I will be among those who move to the new forked code. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] U-book and GPLv3? (fwd)
On Sat, 27 Jun 2009, Jean-Christophe PLAGNIOL-VILLARD wrote: > > > > and this is why i dislike the GPLv3. the GPLv2 was all about the > source, so > > the conversation between developers and everyone else was "you can > take my > > source and modify it all you want, but i want to see the changes". > sounds > > fair. > > > > GPLv3 (ignoring the fix for the loophole with web applications) adds > *nothing* > > to this premise. instead, it's used as an ideological club such that > the > > conversation is now "i have all these ideas about how software should > and > > shouldnt be utilized, so if you want to use my software, you too now > have to > > subscribe to my way of thinking and you have to show me the changes". > > > > so what does moving from GPLv2 to GPLv3 gain us in terms of > protections ? > > nothing. it does however allow us to restrict the people who want to > use u- > > boot to using it in only ways we've "blessed". that's plain wrong in > my eyes > > and none of our business in the first place. > > > > > I think it is not a coincidence that devices which can be updated > with > > > arbitrary firmware sells pretty good in the meantime. Who buys > routers > > > capable of running OpenWRT because of their original firmware? > > > > then let your wallet/politicians do the talking. i certainly do -- i > avoid > > purchasing any music/games encumbered with DRM, or companies that > employ such > > methods. but i'm above going around and forcing people to think the > way i do > > with licenses. > agreed with Mike. Second that. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] U-book and GPLv3? (fwd)
On Thu, 25 Jun 2009, Jean-Christian de Rivaz wrote: > k...@koi8.net a ?crit : > > > > > I downloaded the one I suspect is the more relevant: > > > > > http://gaming.nv.gov/stats_regs/reg14_tech_stnds.pdf > > > > > And I cannot found "secure boot" into it. > > > > Are you looking for a precise phrase? > > > I want to look deeper into the subject. I think that if a regulation > > > make a technical point as a requirement, then it must more or less > > > describe the technical point so that it can be implemented is a way > it > > > work as expected. As an engineer, I think that a "secure boot" is > only a > > > buzz word: if the system can be physically modified, it can't be > > > secured. If it can't be physically modified, then you don't need a > > > secure boot. > > > > It is not just technical measures; it is a complex of them and > different > > operating procedures. > > Yes, I known that. But here we specifically talk about u-boot. You still > failed to show a description of how u-boot can be modified to secure a > system > and why this must be a hidden proprietary code. > > > > > > > > I failed to understand how a secure booted machine can be > > > updated by > > > > > the > > > > > > > manufacturer to fix a bug for example, but not by a > customer. > > > > > > The manufacturer can _NOT_ update his machine at will. _EACH > AND > > > > > EVERY_ > > > > > > change goes through the same approval process. > > > > > Still, technically the hardware have only two possibility: > > > > > 1) it can be reprogrammed. > > > > > 2) it can't be reprogrammed. > > > > > > > > > > If 1), I dont' see how the a boot loader can't be replaced by a > less > > > > > secure one and let boot anything. > > > > > > > > > > if 2), there is not point as nobody can possibly make any > update, so > > > the > > > > > firmware don't have to be secured. > [...] > > Ah, that's absolutely orthogonal issue... We do NOT do something > stupid from > > engineering standpoint because it makes sense (and quite often it > doesn't) > > but because the regulations and the Commission's understanding of them > > requires that. > > > > Yes, many of those are stupid and outdated but they do a good job > anyways; > > there is not that much cheating in our casinos. > > You seem to agree that a "secure boot" is maybe not more that only a > marketing > word... No, this does not have the same strict meaning as "#6-32x1/2" slotted head steel zinc plated machine screw." It is a set of different features. Here is e.g. a Freescale's whitepaper on one of their SoCs: http://www.freescale.com/files/32bit/doc/white_paper/IMX31SECURITYWP.pdf > [...] > > > Why do you think I want to fight regulation ? I actually be more > > > concerned about understanding how a proprietary hidden piece of code > > > into u-boot can possibly make a system satisfy a security > regulation. > > > > It is not just hardware/software. The latter is only a part of > solution. It > > is NOT the machine that pays that jackpot, it is real humans. There is > no > > way to make the system unbreakable and impossible to cheat on. That's > why an > > additional layer of security is being able to DETECT that system had > been > > cheated on. > > So why using open source at all if you think that hidden code is a way > to make > a system more secure ? It highly not consistent ! Who is talking about hidden code? It can be open source. And quite often it is. And most of that code, BTW, is written by the people who are paid to do it. If you want to make us drop U-Boot and write our own firmware no problems, that's just additional job security for us. But don't expect all those people to do anything on U-Boot and forget about their contributions. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] U-book and GPLv3? (fwd)
On Thu, 25 Jun 2009, Jean-Christian de Rivaz wrote: > k...@koi8.net a ?crit : > > On Thu, 25 Jun 2009, Jean-Christian de Rivaz wrote: > > > > > k...@koi8.net a ?crit : > > > > > Please point out precisely the regulations that require secure > boot. > > > > > Should be > > > > > trivial as regulations are by definition public. > > > > Do you happen to know what "Google" is? > > > Yes, thanks :-) > > > > > > For example this document have the term "secure boot": > > > http://www.dcg.virginia.gov/supplier/sup-rules/standards.shtm > > > The wording is this one: > > > "D. Electronic Bingo > > > [...] > > > 3. > > > [...] Security measures that may be employed to comply with these > > > provisions include, but are not limited to the use of dongles, > digital > > > signature comparison hardware and software; secure boot loaders, > > > encryption, and key and callback password systems." > > > > > > The term "secure boot" is listed as a possibility, not as a > requirement. > > > > > > Now I don't have the time to parse every possible document that > Google > > > propose. This is why I politely ask a precise example, as I was > under > > > the impression that some peoples know very well this subject. > > > > > > > This is our Nevada regulations: > > > > > > > > http://gaming.nv.gov/stats_regs.htm > > > I don't have the time to parse all the documents listed at this URL, > but > > > I downloaded the one I suspect is the more relevant: > > > http://gaming.nv.gov/stats_regs/reg14_tech_stnds.pdf > > > And I cannot found "secure boot" into it. > > > > Are you looking for a precise phrase? > > I want to look deeper into the subject. I think that if a regulation > make a technical point as a requirement, then it must more or less > describe the technical point so that it can be implemented is a way it > work as expected. As an engineer, I think that a "secure boot" is only a > buzz word: if the system can be physically modified, it can't be > secured. If it can't be physically modified, then you don't need a > secure boot. It is not just technical measures; it is a complex of them and different operating procedures. When you hit a jackpot the machine should be immediately stopped (hang) in that state and nobody should touch it. Then a controller comes into the scene. He pulls all the EPROM chips from the machine and checks them with MD5 or whatever is approved and checks every single piece of programmable hardware with some procedure approved for this particular model. That would not prevent a cheating casino employee from replacing some EPROM chip (or whatever) with his own one but it will NOT allow for stuffing the original one back once the jackpot is hit so the cheating will be detected. That's only one example... > > > > > I failed to understand how a secure booted machine can be > updated by > > > the > > > > > manufacturer to fix a bug for example, but not by a customer. > > > > The manufacturer can _NOT_ update his machine at will. _EACH AND > > > EVERY_ > > > > change goes through the same approval process. > > > Still, technically the hardware have only two possibility: > > > 1) it can be reprogrammed. > > > 2) it can't be reprogrammed. > > > > > > If 1), I dont' see how the a boot loader can't be replaced by a less > > > secure one and let boot anything. > > > > > > if 2), there is not point as nobody can possibly make any update, so > the > > > firmware don't have to be secured. > > > > You are trying to make sense out of the regulations. It doesn't work > this > > way. If regulations say "one must use a screwdriver with a red handle > on > > this screw" one must use the red screwdriver. No matter if it makes > sense or > > not. If you feel it's bullshit you should fight for the regulation to > change > > that is a very long (years, not months) and very difficult process. In > the > > meantime you _MUST_ use that red screwdriver. > > > > Then you should read not only technical part but also a procedural one > on > > how approvals are given. You must persuade the Commision to give you > an > > approval. And they give them at their discretion. And you can NOT sue > them. > > In this second part, I don't make reference to regulation. I only talk > about the technical problem of reprogramming a system. Ah, that's absolutely orthogonal issue... We do NOT do something stupid from engineering standpoint because it makes sense (and quite often it doesn't) but because the regulations and the Commission's understanding of them requires that. Yes, many of those are stupid and outdated but they do a good job anyways; there is not that much cheating in our casinos. > > Finally don't forget that your employees all want to get their salary > paid > > and that comes from your business revenues. No approval == No > business. Good > > luck fighting regulations. > > Why do you think I want to fight regulation ? I actually be more > concerned about understanding how a proprietary hidden piece of code > into u
Re: [U-Boot] U-book and GPLv3? (fwd)
On Thu, 25 Jun 2009, Jean-Christian de Rivaz wrote: > k...@koi8.net a ?crit : > > > Please point out precisely the regulations that require secure boot. > > > Should be > > > trivial as regulations are by definition public. > > > > Do you happen to know what "Google" is? > > Yes, thanks :-) > > For example this document have the term "secure boot": > http://www.dcg.virginia.gov/supplier/sup-rules/standards.shtm > The wording is this one: > "D. Electronic Bingo > [...] > 3. > [...] Security measures that may be employed to comply with these > provisions include, but are not limited to the use of dongles, digital > signature comparison hardware and software; secure boot loaders, > encryption, and key and callback password systems." > > The term "secure boot" is listed as a possibility, not as a requirement. > > Now I don't have the time to parse every possible document that Google > propose. This is why I politely ask a precise example, as I was under > the impression that some peoples know very well this subject. > > > This is our Nevada regulations: > > > > http://gaming.nv.gov/stats_regs.htm > > I don't have the time to parse all the documents listed at this URL, but > I downloaded the one I suspect is the more relevant: > http://gaming.nv.gov/stats_regs/reg14_tech_stnds.pdf > And I cannot found "secure boot" into it. Are you looking for a precise phrase? > > > I failed to understand how a secure booted machine can be updated by > the > > > manufacturer to fix a bug for example, but not by a customer. > > > > The manufacturer can _NOT_ update his machine at will. _EACH AND > EVERY_ > > change goes through the same approval process. > > Still, technically the hardware have only two possibility: > 1) it can be reprogrammed. > 2) it can't be reprogrammed. > > If 1), I dont' see how the a boot loader can't be replaced by a less > secure one and let boot anything. > > if 2), there is not point as nobody can possibly make any update, so the > firmware don't have to be secured. You are trying to make sense out of the regulations. It doesn't work this way. If regulations say "one must use a screwdriver with a red handle on this screw" one must use the red screwdriver. No matter if it makes sense or not. If you feel it's bullshit you should fight for the regulation to change that is a very long (years, not months) and very difficult process. In the meantime you _MUST_ use that red screwdriver. Then you should read not only technical part but also a procedural one on how approvals are given. You must persuade the Commision to give you an approval. And they give them at their discretion. And you can NOT sue them. Finally don't forget that your employees all want to get their salary paid and that comes from your business revenues. No approval == No business. Good luck fighting regulations. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] U-book and GPLv3? (fwd)
On Thu, 25 Jun 2009, Thomas Doerfler wrote: > Hi, > > since this threads gets more and more interesting, just a question out > of my curiosity: > > which operating systems, that get typically booted using U-Boot are > already under GPL3? > > I know that the license of the Boot Loader has nothing to do with the > license of the booted software, what is the "political benefit" to put > the boot loader under GPLv3, when the major OS of the software (e.g. > linux) are under GPLv2? There is none. It is plain and simple case of paranoia that begs for clinical treatment. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] U-book and GPLv3? (fwd)
On Thu, 25 Jun 2009, Jean-Christian de Rivaz wrote: > k...@koi8.net a ?crit : > > On Thu, 25 Jun 2009, Mike Frysinger wrote: > > > > > On Wednesday 24 June 2009 20:59:11 Richard Stallman wrote: > > > > The principal purpose of these products is to restrict the > public's > > > > freedom. So it is natural that their means involve restricting > our > > > > freedom too. > > > it sure is nice to make generalities as it makes your resulting > argument > > > so much easier to digest. the companies ive worked with could give > two > > > sh*ts about end customers tinkering with their products. they're > > > interested > > > in keeping their product secure from other people in their > respective > > > industry and from malicious tampering for regulation/safety > purposes. > > > > I would like to add that sometimes regulations EXPLICITELY require > secure > > boot. No product can be approved without it. And this does not have > anything > > to do with public's freedom. Just one example is gambling industry > which I > > happen to work right now. Nobody cares about cloning or public's > freedom > > here. What they care about is that nobody can cheat on those nice > shiny > > machines that sometimes let a lucky person to win a multimillion > jackpot. > > Please point out precisely the regulations that require secure boot. > Should be > trivial as regulations are by definition public. Do you happen to know what "Google" is? This is our Nevada regulations: http://gaming.nv.gov/stats_regs.htm > I failed to understand how a secure booted machine can be updated by the > manufacturer to fix a bug for example, but not by a customer. The manufacturer can _NOT_ update his machine at will. _EACH AND EVERY_ change goes through the same approval process. And one more hint--external hackers is _NOT_ the primary concern here. The most important task is to make cheating by casino _EMPLOYEES_ as difficult as it's possible. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] U-book and GPLv3? (fwd)
On Thu, 25 Jun 2009, Mike Frysinger wrote: > On Wednesday 24 June 2009 20:59:11 Richard Stallman wrote: > > Embedded systems using core soc silicon from a number of > manufacturers > > have started to use what is known as 'secure boot'. This is > typically > > the case in applications which utilise conditional access system > software > > to protect content. The emphasis on using secure boot is largely > driven by > > the conditional access industry itself. > > > > The principal purpose of these products is to restrict the public's > > freedom. So it is natural that their means involve restricting our > > freedom too. > > it sure is nice to make generalities as it makes your resulting argument > so > much easier to digest. the companies ive worked with could give two > sh*ts > about end customers tinkering with their products. they're interested > in > keeping their product secure from other people in their respective > industry > and from malicious tampering for regulation/safety purposes. I would like to add that sometimes regulations EXPLICITELY require secure boot. No product can be approved without it. And this does not have anything to do with public's freedom. Just one example is gambling industry which I happen to work right now. Nobody cares about cloning or public's freedom here. What they care about is that nobody can cheat on those nice shiny machines that sometimes let a lucky person to win a multimillion jackpot. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] BDIxxxx and others...
On Wed, 17 Jun 2009, Wolfgang Denk wrote: > Dear David Hawkins, > > In message <4a382998.4010...@ovro.caltech.edu> you wrote: > > > > The USB-TAP has a PowerPC processor in it ... so even if you > > had to blow away the original firmware, I'm sure it wouldn't > > be too hard to figure out what code would be required to > > make the device look like a USB debugger, and then create > > a set of USB commands to generate JTAG transactions. > > > > I think the main problem is getting the JTAG TAP codes > > for manipulating JTAG. I've never seen a document relating > > to that, so they are obviously NDA. > > Right. Implementing some JTAG access routines is not that difficult. > Implementing debug support requires you to sign a NDA which prevents > you from developing any free software from it. OK, so it is going to be a door stop :) --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] BDIxxxx and others...
I see you guys talking about BDI3000 and I decided to ask a related question. Those who happen to own MPC8548CDS or something like this know it comes with a small box called CodeWarrior USB TAP. It is supposed to work with their software one has to pay for. I never used anything but GCC suite for anything GCC supports and always tried to avoid commercial tools like a plague. So I want to ask if somebody knows if there are any free tools for that thing... Not that I really need it for something but it is sitting in the box gathering dust and it would be nice ot somehow put it to do something... --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Multiple device support -- sorry state :(
On Wed, 8 Apr 2009, Robert Schwebel wrote: > On Wed, Apr 08, 2009 at 02:25:57PM -0700, k...@koi8.net wrote: > > > OK, thanks. Cloning now :) > > > > OK, got a look at it. Looks promising but it is in very early stage > yet... I > > wouldn't say in pre-conception stage, but definitely on a very > beginning of > > the first trimester :) > > Well, you are free to send patches. We use it mainly for ARM systems > which happen to be flash based (NAND, NOR), without PCI. So what was > implemented was what we needed. No reason for you not to send us your > favourite and fancy new features :-) Sure I will :) Ah, ARM, my old love, still remeber that talk -- ldmia, stmfd... Romantic times, eh... > > I have the first prototype of MPC8548E-based big motherboard sitting > > on my desk right now and it is in full bringup stage. All the hardware > > is checked, bunch of smoked parts replaced and fixed, all clocks are > > ticking, CPU fetching etc. > > > > I will definitely join V2 development but for now I'll probably make a > > huge set of horrible hacks over V1 to test everything and get my > > revision 2 started. Then I will be able to work on U-Boot V2 while my > > revision 2 is made (schematic rework, re-layout, PCB, missing parts > > sourcing, assembly etc.) > > Go ahead, we definitely are looking forward to more community > contribution. OK, I probably will. V1, IMHO, is beyond repair; I even contemplated forking off my own... --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Multiple device support -- sorry state :(
On Wed, 8 Apr 2009, k...@koi8.net wrote: > On Wed, 8 Apr 2009, Robert Schwebel wrote: > > > On Wed, Apr 08, 2009 at 01:18:45PM -0700, k...@koi8.net wrote: > > > > I suppose you didn't look in the right place. We don't even have > > support > > > > for i2c and spi in v2 :-) > > > > > > Ah, that's that forked one! Sorry, my bad... I thought about the new > > version > > > of a legacy one that just shuffled source files to put device > drivers > > under > > > devices/*... > > > > > > Can you tell me where it lives to clone the repository? I'd rather > > give up > > > on v1 entirely because it is not going anywhere... > > > > git clone git://www.denx.de/git/u-boot-v2 > > > > > Do you have a separate mailing list for v2? > > > > No, just use this one. As long as the traffic keeps low, I suppose > there > > is no need for a separate list. However, it's probably a good idea to > > have Sascha on Cc: :-) > > OK, thanks. Cloning now :) OK, got a look at it. Looks promising but it is in very early stage yet... I wouldn't say in pre-conception stage, but definitely on a very beginning of the first trimester :) I have the first prototype of MPC8548E-based big motherboard sitting on my desk right now and it is in full bringup stage. All the hardware is checked, bunch of smoked parts replaced and fixed, all clocks are ticking, CPU fetching etc. I will definitely join V2 development but for now I'll probably make a huge set of horrible hacks over V1 to test everything and get my revision 2 started. Then I will be able to work on U-Boot V2 while my revision 2 is made (schematic rework, re-layout, PCB, missing parts sourcing, assembly etc.) --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Multiple device support -- sorry state :(
On Wed, 8 Apr 2009, Robert Schwebel wrote: > On Wed, Apr 08, 2009 at 01:18:45PM -0700, k...@koi8.net wrote: > > > I suppose you didn't look in the right place. We don't even have > support > > > for i2c and spi in v2 :-) > > > > Ah, that's that forked one! Sorry, my bad... I thought about the new > version > > of a legacy one that just shuffled source files to put device drivers > under > > devices/*... > > > > Can you tell me where it lives to clone the repository? I'd rather > give up > > on v1 entirely because it is not going anywhere... > > git clone git://www.denx.de/git/u-boot-v2 > > > Do you have a separate mailing list for v2? > > No, just use this one. As long as the traffic keeps low, I suppose there > is no need for a separate list. However, it's probably a good idea to > have Sascha on Cc: :-) OK, thanks. Cloning now :) --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Multiple device support -- sorry state :(
On Wed, 8 Apr 2009, Robert Schwebel wrote: > On Wed, Apr 08, 2009 at 12:25:16PM -0700, k...@koi8.net wrote: > > On Wed, 8 Apr 2009, Jerry Van Baren wrote: > > > > > k...@koi8.net wrote: > > > > OK, this is _NOT_ just multiple I2C adapters... The entire thing > is > > > > fundamentally broken. > > > > > > > > One supposed to have _THE_ device and only this device is somehow > > > supported. > > > > > > > > Now it is USB. Each and every USB driver exports the same set of > > > functions, > > > > submit_XXX_msg(...) That means there can be one and only one USB > > > device in > > > > the system. > > > > > > [snip the badly scorched spot ;-)] > > > > > > > USB keyboard is another grand kludge deserving its own chapter... > As > > > of now > > > > one can only switch to it from command line because USB is not > even > > > > initialized until do_usb() from cmd_usb.c is called... What if we > do > > > NOT > > > > have a serial console at all? > > > > > > Dumb question because I have not looked seriously at the v2 fork of > > > u-boot: > > > how does the v2 fork handle this? Better? Since the v2 fork uses > (or > > > is > > > close to) the linux driver model, I would expect it to be better. > > > > Nothing's changed here... > > > > And that is _NOT_ just USB, it is everywhere -- I2C, SPI, etc. It is > stiff, > > inflexible, everything written in stone and that stone is permanently > set in > > concrete. > > I suppose you didn't look in the right place. We don't even have support > for i2c and spi in v2 :-) Ah, that's that forked one! Sorry, my bad... I thought about the new version of a legacy one that just shuffled source files to put device drivers under devices/*... Can you tell me where it lives to clone the repository? I'd rather give up on v1 entirely because it is not going anywhere... Do you have a separate mailing list for v2? --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Multiple device support -- sorry state :(
On Wed, 8 Apr 2009, Jerry Van Baren wrote: > k...@koi8.net wrote: > > OK, this is _NOT_ just multiple I2C adapters... The entire thing is > > fundamentally broken. > > > > One supposed to have _THE_ device and only this device is somehow > supported. > > > > Now it is USB. Each and every USB driver exports the same set of > functions, > > submit_XXX_msg(...) That means there can be one and only one USB > device in > > the system. > > [snip the badly scorched spot ;-)] > > > USB keyboard is another grand kludge deserving its own chapter... As > of now > > one can only switch to it from command line because USB is not even > > initialized until do_usb() from cmd_usb.c is called... What if we do > NOT > > have a serial console at all? > > Dumb question because I have not looked seriously at the v2 fork of > u-boot: > how does the v2 fork handle this? Better? Since the v2 fork uses (or > is > close to) the linux driver model, I would expect it to be better. Nothing's changed here... And that is _NOT_ just USB, it is everywhere -- I2C, SPI, etc. It is stiff, inflexible, everything written in stone and that stone is permanently set in concrete. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] Multiple device support -- sorry state :(
OK, this is _NOT_ just multiple I2C adapters... The entire thing is fundamentally broken. One supposed to have _THE_ device and only this device is somehow supported. Now it is USB. Each and every USB driver exports the same set of functions, submit_XXX_msg(...) That means there can be one and only one USB device in the system. Let's take e.g. widely used ISP1563 PCI USB controller. It has _TWO_ OHCI controllers. Existing usb_ohci.c code only supports one. That means that one can only use odd numbered ports (1 and 3) for any USB devices. Ports 2 and 4 are not usable at all. Not just together with odd numbered ones but even _INSTEAD_ of those. But wait, there is more... This very same controller also has a 4-port EHCI controller for USB 2.0 support. This is not supported at all because usb_ehci_pci.c is a mere stub that does NOT support _ANY_ PCI EHCI controller. But it would not do any good if it did support ISP1563 -- there should be one and only one USB controller, _THE_ USB controller... EHCI driver, usb_ehci_core.c exports exactly the same submit_XXX_msg() set of function so it can _NOT_ coexist with anything else. So what's the choice? Let's say we somehow made that usb_ehci_pci.c stub in a real driver and got ISP1563 (or _ANY_ other USB2.0 PCI adapter) recognized and initialized. This hypothetically gives us all 4 ports because all 4 are supported by EHCI vs. only 2 by OHCI. Would it do any good? The answer is _NO_. EHCI does _NOT_ support slower USB1.X ports so one can _NOT_ hook his e.g. USB keyboard to EHCI controller. It is only for hi-speed devices. USB2.0 controller actually consists of an EHCI and set of slower USB1.X controllers that share the same USB pins. If a connected device supports high speed, EHCI is used. If it does NOT support high speed, driver should made EHCI controller to give up ownership of that particular USB port to OHCI (or whatever it is) and pass control to OHCI driver. That is how USB works. Now we suddenly have _THREE_ USB controllers while our software only supports one. And that is _NOT_ some exotic situation with multiple chips or other strange design -- e.g. MPC8548 does NOT have a built-in USB controller so I added a single PCI chip to provide for a USB interface. One can recommend using only the first OHCI controller in U-Boot somehow tolerating it being painfully slow with storage devices etc. and limited with what ports can be used but that is also _NOT_ a solution... OHCI only allows for 2 ports and in our case they are both already used (KBD + MOUSE.) Sure we don't need a mouse in U-Boot but we _DO_ need it to be connected anyways because we boot full-blown Fedora with X that uses mouse. That means we do NOT have any USB ports available for storage or anything else... USB keyboard is another grand kludge deserving its own chapter... As of now one can only switch to it from command line because USB is not even initialized until do_usb() from cmd_usb.c is called... What if we do NOT have a serial console at all? Grrr... --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Porting uboot to Broadcom 47xx (MIPS)
On Wed, 8 Apr 2009, Michael Buesch wrote: > Hi, > > I'm the author of the Linux SSB subsystem, which implements 95% of the > system > bootup code for the Broadcom 47xx MIPS architecture. > > I'd like to start porting u-boot to the bcm47xx. (I guess nobody started > with this already?) Did I miss something or Broadcom (or Marvell) already started giving out their datasheets, less for something more useful without 37 NDAs per each BGA pin? I don't understand why we accept any code for all those closed platforms that no one can even get a rudimentary data without all those NDAs into a GPL-licensed U-Boot? Especially for those companies that make helluva secret even of mere PHYs, less something more complex... If it had been up to me I wouldn't've accepted any of such code into U-Boot. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] Noisily disable the legacy NAND subsystem.
On Sat, 4 Apr 2009, Wolfgang Denk wrote: > Dear Jean-Christophe PLAGNIOL-VILLARD, > > In message <20090404163654.ge32...@game.jcrosoft.org> you wrote: > > > > Scoot you brake the rm9200dk > > No, he did not. He just made the problem clearly visible - the code > was has been broken before (just silently), if I understand > correctly. > > > could you convert your #error in #warning for this release > > as it's said in the commit message it will be remove for the next > release > > I think we should leave this an #error, so people are really forced to > react. Second that. That ancient code should've long died. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 10/13] at91: move usb driver to drivers/usb
On Wed, 1 Apr 2009, Stefan Roese wrote: > On Wednesday 01 April 2009, Michael Trimarchi wrote: > > It is just a glue code in linux. Maybe it will be simple if you create > a > > core direcory > > and a host directory under the usb. An move the specific part in the > host. > > Yes, I like this idea. This gets us closer to the Linux directory > structure. I agree. Drivers are going under drivers/ so it is OK. Generic ones should be kept right under drivers/something, platform specific go under drivers/something/host. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 10/13] at91: move usb driver to drivers/usb
On Wed, 1 Apr 2009, Remy Bohmer wrote: > Hello Stefan, > > > From what I remember we all agreed to move the device drivers (e.g. > ethernet, > > NAND, USB, serial etc) from the architecture/board (cpu/... board/...) > to the > > drivers directories at some time. > > > > Speaking for PPC4xx, the 4xx ethernet driver has recently been moved > from > > cpu/ppc4xx to drivers/net. And I'm planning to move the 4xx NAND > driver (and > > others) soon too. > > > > So if this atmel_usb.c driver isn't just platform USB init code, but a > real > > USB driver, then I'm voting to move it to drivers/usb as well. > > And that is exactly the issue here. The discussion is about moving > 'platform USB init code' to generic driver code. > I would have no problems with moving real USB drivers to the generic > driver section, if they are cleaned from board specific code, of > course. Board specific code belongs to $(BOARD).c. It does _NOT_ belong to cpu/* either so it should be moved to $(BOARD).c no matter where that platform driver resides. That is why we should put something like platform_$(DRIVER)_init() in all those drivers (most of them do already have it, see e.g. I2C drivers) and move that portion in board specific functions in $(BOARD).c. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 10/13] at91: move usb driver to drivers/usb
On Wed, 1 Apr 2009, Stefan Roese wrote: > On Tuesday 31 March 2009, Wolfgang Denk wrote: > > In message <20090331192117.gf24...@game.jcrosoft.org> you wrote: > > > > >  drivers/usb/Makefile             >    |   1 + > > > > >  .../at91/usb.c => drivers/usb/atmel_usb.c      |  >  0 > > > > >  rename cpu/arm926ejs/at91/usb.c => drivers/usb/atmel_usb.c > (100%) > > > > > > > > Same here, this is architecture specific code, why move it to > generic > > > > cod> e? > > > > > > it's the at91 usb drivers and we need to have it in the driver/usb > > > > Why do we need to have it in the driver/usb ? > > > > Please explain in detail. > > >From what I remember we all agreed to move the device drivers (e.g. > ethernet, > NAND, USB, serial etc) from the architecture/board (cpu/... board/...) > to the > drivers directories at some time. > > Speaking for PPC4xx, the 4xx ethernet driver has recently been moved > from > cpu/ppc4xx to drivers/net. And I'm planning to move the 4xx NAND driver > (and > others) soon too. > > So if this atmel_usb.c driver isn't just platform USB init code, but a > real > USB driver, then I'm voting to move it to drivers/usb as well. I also vote for moving _ALL_ the drivers (i2c, usb, net, etc.) to appropriate directories under drivers/ no matter architecture specific they are or not. This will make the tree more logical and one wouldn't have to chase say USB driver all over the source tree. Also it is a first step to general overhaul that would allow for multiple drivers support. The fact some SoC has a built-in, say USB controller does _NOT_ mean there is no more USB controllers on the same board. Some can be on PCI bus etc. The same is true for each and every other driver. And we should _NOT_ treat some drivers (e.g. SPI) as marginal. AT91RM9200 for example can _NOT_ boot off of parallel flash because of silicon error so it boots off of SPI DataFlash thus making SPI driver essential for the system. To contain drivers is a reason for drivers/* to exist, isn't it? --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * **___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Multiple device support - none at all?
On Thu, 12 Mar 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > That's true but... It is not that unusual to have several similar interfaces > > on a board. There is nothing obscure or weird in that. But allowing for only > > one of those interfaces (whichever one chooses) supported by a given binary > > is what is weird. > > When U-Boot was started, it was for MPC860 only. Well, actually for > MPC8xx. > > It took until Stefan Roese submitted the firt port to another pro- > cessor family (4xx) - before that, supporting other processors was > never a goal. > > You see the same happen in another area. That I understand, nobody can predict the future... But now it overgrew itself so some radical changes are in order. No matter how much grease we stuff in the bearings of a horse-cart wheels it can not cope with modern vehicles any more. Iron horse is coming to replace the old peasant's mare :) > > And it is _NOT_ that existing U-Boot does not provide a solution for a > > particular board. It is that it does not provide _MEANS_ to make a solution. > > You mean the code cannot be changed? C'me on... It can be. But the changes will be really extensive. I might be wrong but I think that all board-specific stuff must go under $(BOARD)/* with the rest of U-Boot proper untouched. As for now I don't have any other choice than duplicate USB, I2C, and serial drivers (not sure yet what else) under my $(BOARD)/ and add wrappers there. I can _NOT_ use standard drivers because they export access functions that must go through a wrapper in my case with the wrapper itself exporting them. That makes my $(BOARD)/* really big and a lot of source code duplication is required. I do also have to make some changes to U-Boot proper (e.g. to add switching USB controllers to common/cmd_usb.c) but those are relatively small. > > > Wrong. You can switch console devices on the fly. Including assigning > > > it to the null device. Of course you better know exactly what you are > > > doing. > > > > So what? Let's say I'm trying to read a file from a USB device with some > > interactive command. I do NOT have a serial console and I want to get a > > result somehow... > > Use stdin on USB keyboard, if you like (I'd prefer netconsole any- > way); then define an environment variable that 1) switches stdin to > nulldev; 2) reads the file from USB ; 3) switches stdin to USB key- > board. > > I do not claim that this is especially elegant, but it's a simple > workaround that allows you to do what you ask for and takes less than > 3 seconds to implement. There are numerous ways to skin a cat :) Sure, everything can be done eventually but we are developers, not mere end users, aren't we? :) We can make it elegant, that's what we are for... > You are welcoem to provide patches for a more elegant (and more > expensive) solution. > > > > > > controller permanently enabled if we use USB keyboard that in turn > > > > means the > > > > boot device absolutely must reside on that same controller in current > > > > architecture. > > > > > > Wrong. You can switch on the fly. > > > > Yes, I can. Will it do any good is totally different question. > > It can be used as a quick (even if considered somewhat dirty) > workaround. > > > > As such, I wonder why you waste time for the messages in this current > > > thread. > > > > It was _NOT_ a discussion. It ceased to be one after a couple of days. You > > guys somehow got scared by innocent CPP tricks and then discussion degraded > > to junk. I didn't even tell that there _IS_ a whole bunch of very similar > > CPP-generated code already in U-Boot source. Look at e.g. drivers/pci/pci.c > > or drivers/pci/pci_indirect.c... > > Well, I think you can blame both sides. You also provided your share > of unproven claims, ignoring other opinions, etc. No, I gave a logical analysis. I did not build several versions and compared them but it is not required. It is only needed if there is no logic or theoretical solution and result can not be calculated. There is nothing more practical than a good theory. Numerous experiments in the dark is not an effective way to do things. It might be the only one if we deal with something totally unknown but there is no need for experiments to tell if a sledgehammer would deliver a stronger blow than a small hammer. And it doesn't require experimenting to tell which one is easier on an operator. Once upon a time Soviet military did not have QA departments for software. Nobody tested programs by pushing each and every button in different combinations in hope to find some bug. Every piece of software had to be _PROVEN_ i.e. it's source code was analyzed for dead states, logical deficiencies etc. Every single line of it. That resulted in almost no bugs, everything worked as it supposed to. > I suggest we let this rest for now, and start again in the next > release cycle, hoping the minds are quieter tehn. > > > > >
Re: [U-Boot] Multiple device support - none at all?
On Thu, 12 Mar 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > It is supposed to be a "Universal" bootloader. Here is what wiki says: > > There is a certain difference between "universal" and "omnipotent". > Note that it's called "U-Boot", not "O-Boot". That's true but... It is not that unusual to have several similar interfaces on a board. There is nothing obscure or weird in that. But allowing for only one of those interfaces (whichever one chooses) supported by a given binary is what is weird. > > === Cut === > > Das U-Boot (Universal Bootloader, short for "Das Unterseeboot", German for > > "the submarine") is a boot loader for a number of different computer > > architectures, including PPC, ARM, AVR32, MIPS, x86, 68k, Nios, and > > MicroBlaze. > > === Cut === > > And this is 100% correct, isn't it? Yes. It is truth, only truth and nothing but truth. But it is not the entire truth :) There is no "embedded" here... > > I'm not even talking about using several USB/I2C/SPI/Serial/whatever devices > > concurently; it is only about being able to pick one of several at run > > time... > > You seem to be the first to express such a requirement. And U-Boot > provides you with all the means (free software etc.) that enables you > to extend it and to add such a feature. > > U-Boot never claimed to provide a turnkey solution for any obscure > requirement. It is not obscure. E.g. ISP1563 PCI USB controller (that is somehow supported with drivers/usb/usb_ohci.c) has 3 devices, not one. They are 2 OHCI and 1 EHCI controllers. For EHCI, that allows for all 4 ports (but HS only, will not work with e.g. USB keyboard) we do _NOT_ have any support; drivers/usb/usb_ehci_pci.c is just a stub that does _NOT_ support _ANY_ EHCI controller. What we are left with is _TWO_ OHCI controllers, each serving 2 ports (odd for one controller and even for another one.) Our existing driver only supports _ONE_ controller. That means not just we are not able to use HS USB 2.0 at all having to stick with USB 1.1 but also that we must use 2 specific ports out of available 4 because only one OHCI controller is detected and supported. That _MIGHT_ be not an issue for an embedded board where everything is hardwired and we _DO_ know where anything is connected to but it _IS_ an issue for a regular motherboard with something like 6 or 8 USB connectors on it's back panel. All USB are born equal, aren't they? And it is _NOT_ that existing U-Boot does not provide a solution for a particular board. It is that it does not provide _MEANS_ to make a solution. > > That is actually not sufficient because we have one device that must be > > enabled at all times -- the console. That means we must have one USB > > Wrong. You can switch console devices on the fly. Including assigning > it to the null device. Of course you better know exactly what you are > doing. So what? Let's say I'm trying to read a file from a USB device with some interactive command. I do NOT have a serial console and I want to get a result somehow... > > controller permanently enabled if we use USB keyboard that in turn means the > > boot device absolutely must reside on that same controller in current > > architecture. > > Wrong. You can switch on the fly. Yes, I can. Will it do any good is totally different question. > > Eh, I did offer such a model for I2C :) And that model can be extended to > > We might come bck on this after the next release is out. Heiko has all > your stuff in his repository. And some more. > > > into the U-Boot proper. I wasted 2+ weeks for I2C without any result (that > > is not counting another couple of weeks I spent on that before starting > > sending patches to the list.) > > It's a pity when you consider discussion the principle parts of a new > design wasted time. > > As such, I wonder why you waste time for the messages in this current > thread. It was _NOT_ a discussion. It ceased to be one after a couple of days. You guys somehow got scared by innocent CPP tricks and then discussion degraded to junk. I didn't even tell that there _IS_ a whole bunch of very similar CPP-generated code already in U-Boot source. Look at e.g. drivers/pci/pci.c or drivers/pci/pci_indirect.c... > > The model can be really simple. Just use an array of structures with > > function pointers to several adapter drivers and make a simple wrapper > > calling an appropriate function depending on device chosen (i.e. "current" > > device.) Don't call all e.g. USB adapter submit message functions > > "submit_XXX_msg" and link that only one that is chosen at compile time and > > exported. Make them e.g. "adapter_submit_XXX_msg" instead, then do something > > like this: > > Submit a patch? Then we can see the code, the size impact, etc. I did it for I2C, it got nothing. There is absolutely no reason to make a whole bunch of similar patches for other interfaces, they are almost identical. The design is exactly the same,
Re: [U-Boot] Multiple device support - none at all?
On Thu, 12 Mar 2009, Detlev Zundel wrote:
> Hello ksi (so I'll leave it at that),
>
> > First of all, there are several bootable devices on a motherboard itself.
> > These days almost every motherboard has PATA and some kind of SATA RAID
> > controller onboard. Then, it can usually boot off of USB while having USB
> > keyboard/mouse. As for add-on devices, it usually CAN boot off of those
> > devices. They have their own BIOS for that that gets attached to the
> > motherboard's one when it initializes and those devices are added to the
> > boot table.
>
> It may come as a surprise to you, but I actually also use such system :)
>
> What I wanted to say is that your blatant "it allows me to choose from
> which device to boot from" does not coincide with my experiences in real
> life. Moreover what you call "to boot from" is in the BIOS world the
> transfer of usually one block followed by a jump. I have yet to see a
> BIOS booting Linux in a flexible way. This is hardly comparably to the
> support (just think filesystems) that we have for ages in U-Boot.
Yes, that's great, but that filesystem _MUST_ reside on a device connected
to _ONE_ particular controller. And it becomes even more complicated if you
have a keyboard (or console) on USB...
> But actually I think that this is not news to most people on the ML so
> I'll stop here.
>
> >> As Jerry pointed out - booting embedded hardware used to be a comparably
> >> simple operation ;)
> >
> > There is a whole world beyond that keyhole... Embedded devices yes, but
> > there are other boards that don't fall in that category. My current MPC8548
> > based board is not an embedded one, it is full-blown mATX motherboard with
> > several USB controllers, PATA, SATA, Video Capture, VGA/LCD controller,
> > PCI-X/PCI/PCIe connectors etc.
>
> Yes, and again, you may notice that U-Boot was meant for "embedded
> devices" so the "impedance mismatch" you now realize for "full-blown
> motherboards" was kind of to be expected...
It is supposed to be a "Universal" bootloader. Here is what wiki says:
=== Cut ===
Das U-Boot (Universal Bootloader, short for "Das Unterseeboot", German for
"the submarine") is a boot loader for a number of different computer
architectures, including PPC, ARM, AVR32, MIPS, x86, 68k, Nios, and
MicroBlaze.
=== Cut ===
I'm not even talking about using several USB/I2C/SPI/Serial/whatever devices
concurently; it is only about being able to pick one of several at run
time...
That is actually not sufficient because we have one device that must be
enabled at all times -- the console. That means we must have one USB
controller permanently enabled if we use USB keyboard that in turn means the
boot device absolutely must reside on that same controller in current
architecture.
> > **
> > * k...@homeKOI8 Net < > The impossible we do immediately. *
> > * Las Vegas NV, USA < > Miracles require 24-hour notice. *
> > **
>
> Miracles in 24 hours? Ok, I'm looking forward to a device model
> implementation tomorrow evening then ;)
Eh, I did offer such a model for I2C :) And that model can be extended to
anything else. That is what I'm actually implementing for my new board. But
that goes into $(BOARD)/* because I simply don't have time for pushing it
into the U-Boot proper. I wasted 2+ weeks for I2C without any result (that
is not counting another couple of weeks I spent on that before starting
sending patches to the list.)
The model can be really simple. Just use an array of structures with
function pointers to several adapter drivers and make a simple wrapper
calling an appropriate function depending on device chosen (i.e. "current"
device.) Don't call all e.g. USB adapter submit message functions
"submit_XXX_msg" and link that only one that is chosen at compile time and
exported. Make them e.g. "adapter_submit_XXX_msg" instead, then do something
like this:
=== Cut ===
Header file:
.
.
.
typedef struct usb_functions_mux {
int (*submit_bulk_msg)(struct usb_device *dev,
unsigned long pipe,
void *buffer,
int transfer_len);
int (*sumbit_control_msg)(struct usb_device *dev,
unsigned long pipe,
void *buffer,
int transfer_len,
struct de
Re: [U-Boot] Multiple device support - none at all?
On Thu, 12 Mar 2009, Detlev Zundel wrote: > Hi ksi (was it Sergei?), > > >> The fundamental concept of u-boot isn't to handle all your devices, it is > >> to > >> boot linux (or other OS) and use the OS to handle the multiple devices. > > > > The same is true for a PC BIOS, but it DOES handle all of my devices. And it > > allows me to choose which device to boot from. And it is extendable so I can > > add a PCI card with e.g. RAID controller and boot off of that RAID. And it > > is not handling of all devices, it is _BASIC_ functionality... > > Actually I don't know what you're talking about in the PC BIOS area > here. Usually the computers I own cannot boot from the devices that are > bought somewhat after the motherboard. But maybe you have different > instances of computers... First of all, there are several bootable devices on a motherboard itself. These days almost every motherboard has PATA and some kind of SATA RAID controller onboard. Then, it can usually boot off of USB while having USB keyboard/mouse. As for add-on devices, it usually CAN boot off of those devices. They have their own BIOS for that that gets attached to the motherboard's one when it initializes and those devices are added to the boot table. > > I don't mind reconfiguring and rebuilding the thing for my parcticular > > configuration instead of just changing some settings in interactive setup > > utility, but the current implementation won't allow even that... > > Actually we are thinking about "going generic" here also for a while > already. Look for example at the BOF session notes of OLS 2007[1] > "Device Tree Run Time Configuration". > > We are all aware that a "proper" device model will be needed at a > certain stage, only nobody started into that direction yet. > > As Jerry pointed out - booting embedded hardware used to be a comparably > simple operation ;) There is a whole world beyond that keyhole... Embedded devices yes, but there are other boards that don't fall in that category. My current MPC8548 based board is not an embedded one, it is full-blown mATX motherboard with several USB controllers, PATA, SATA, Video Capture, VGA/LCD controller, PCI-X/PCI/PCIe connectors etc. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] Multiple device support - none at all?
On Tue, 10 Mar 2009, Jerry Van Baren wrote: > k...@koi8.net wrote: > > Hi, everyone. > > > > I wonder if I'm dreaming or the U-Boot is fundamentally broken... > > Well... broken can be a relative term. I like to think of it as an > opportunity to stand on the shoulders of giants. ;-) :) > > It looks like there is no support for multiple devices of a same kind at > > all. > > > > I'm porting U-Boot to my new MPC8548-based board and it pops up > > everywhere... > > > > The first case was I2C subsystem that does not provide for several I2C > > adapters except rudimentary hacks for fsl_i2c. > > > > Now it is USB. I have 2 OHCI USB controllers on the board, OHCI module in > > NXP ISP1563 and OHCI controller in Silicon Motion SM502 MFD. The former is > > supported in drivers/usb/usb_ohci.c, the latter is kinda trivial to add, > > BUT... > > > > It looks like nobody even considered the case when a board can have several > > controllers... Looking at usb_lowlevel_init() I can see it only works for > > the first device found, there is absolutely no provision for several > > devices. And things are getting even nastier if there are devices of the > > same type with different interfaces (e.g. on-SoC USB controller and a PCI > > one.) > > > > And this seems to be the case for each and every device type. Am I missing > > something or U-Boot is actually flawed in FUNDAMENTAL way and it is time to > > start a new one from scratch? > > U-Boot started life as a boot loader. You know, simple. It has gotten more > complex over time, but the modus operandi of u-boot is and shall remain that. > Simple. (quoting our benevolent dictator, WD) > > The counter-question for your application is to challenge what you are trying > to use u-boot for: do you actually need to use both USB controllers > (simultaneously or even sequentially) to boot linux? Do you really need to > use multiple I2C controllers to boot linux? Can you simplify your system to > get linux running without supporting multiple USB or I2C adapters? Yes, I _DO_ need both controllers. There are numerous scenarios for that -- e.g. I can have a keyboard on a slow controller and a storage device or network adapter on another one. I can have multiple storage devices and want to choose which to boot from and so on. > If you really need multiple adapter support, you are going to have to blaze > new ground (or build out some hacks into more acceptable levels of support). > That is the cost of being the leader. :-/ Thanks, I already tried this for I2C... Now it is a hack (separate driver and core) in my $(BOARD) directory. USB is next, then I have additional serial ports... > The fundamental concept of u-boot isn't to handle all your devices, it is to > boot linux (or other OS) and use the OS to handle the multiple devices. The same is true for a PC BIOS, but it DOES handle all of my devices. And it allows me to choose which device to boot from. And it is extendable so I can add a PCI card with e.g. RAID controller and boot off of that RAID. And it is not handling of all devices, it is _BASIC_ functionality... I don't mind reconfiguring and rebuilding the thing for my parcticular configuration instead of just changing some settings in interactive setup utility, but the current implementation won't allow even that... --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] Multiple device support - none at all?
Hi, everyone. I wonder if I'm dreaming or the U-Boot is fundamentally broken... It looks like there is no support for multiple devices of a same kind at all. I'm porting U-Boot to my new MPC8548-based board and it pops up everywhere... The first case was I2C subsystem that does not provide for several I2C adapters except rudimentary hacks for fsl_i2c. Now it is USB. I have 2 OHCI USB controllers on the board, OHCI module in NXP ISP1563 and OHCI controller in Silicon Motion SM502 MFD. The former is supported in drivers/usb/usb_ohci.c, the latter is kinda trivial to add, BUT... It looks like nobody even considered the case when a board can have several controllers... Looking at usb_lowlevel_init() I can see it only works for the first device found, there is absolutely no provision for several devices. And things are getting even nastier if there are devices of the same type with different interfaces (e.g. on-SoC USB controller and a PCI one.) And this seems to be the case for each and every device type. Am I missing something or U-Boot is actually flawed in FUNDAMENTAL way and it is time to start a new one from scratch? --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] [85xx] Add eTSEC 1/2 IO override control definition
On Mon, 23 Feb 2009, Kumar Gala wrote:
>
> On Feb 23, 2009, at 12:42 PM, k...@koi8.net wrote:
>
> > This adds missing tsec12ioovcr to include/asm-ppc/immap_85xx.h. It was
> > named "res14" (for "reserved") while tsec34ioovcr was properly there.
> >
> > Signed-off-by: Sergey Kubushyn
> > ---
> > diff -purN u-boot.orig/include/asm-ppc/immap_85xx.h
> > u-boot/include/asm-ppc/immap_85xx.h
> > --- u-boot.orig/include/asm-ppc/immap_85xx.h2009-02-19
> > 13:39:31.0
> > -0800
> > +++ u-boot/include/asm-ppc/immap_85xx.h 2009-02-23 10:32:48.0
> > -0800
> > @@ -1667,7 +1667,11 @@ typedef struct ccsr_gur {
> > uintlbiuiplldcr0; /* 0xe0f1c -- LBIU PLL Debug Reg 0 */
> > uintlbiuiplldcr1; /* 0xe0f20 -- LBIU PLL Debug Reg 1 */
> > uintddrioovcr; /* 0xe0f24 - DDR IO Override Control */
> > +#ifdef CONFIG_MPC8548
> > + uinttsec12ioovcr; /* 0xe0f28 - eTSEC 1/2 IO override control */
> > +#else
> > uintres14; /* 0xe0f28 */
> > +#endif
>
> don't bother with the ifdef if the field was previously reserved.
Corrected patch posted.
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] [85xx] Add eTSEC 1/2 IO override control (corrected)
This adds tsec12ioovcr to include/asm-ppc/immap_85xx.h (was reserved.)
Signed-off-by: Sergey Kubushyn
---
diff -purN u-boot.orig/include/asm-ppc/immap_85xx.h
u-boot/include/asm-ppc/immap_85xx.h
--- u-boot.orig/include/asm-ppc/immap_85xx.h2009-02-19 13:39:31.0
-0800
+++ u-boot/include/asm-ppc/immap_85xx.h 2009-02-23 10:49:09.0 -0800
@@ -1667,7 +1667,7 @@ typedef struct ccsr_gur {
uintlbiuiplldcr0; /* 0xe0f1c -- LBIU PLL Debug Reg 0 */
uintlbiuiplldcr1; /* 0xe0f20 -- LBIU PLL Debug Reg 1 */
uintddrioovcr; /* 0xe0f24 - DDR IO Override Control */
- uintres14; /* 0xe0f28 */
+ uinttsec12ioovcr; /* 0xe0f28 - eTSEC 1/2 IO override control */
uinttsec34ioovcr; /* 0xe0f2c - eTSEC 3/4 IO override control */
charres15[61648]; /* 0xe0f30 to 0xef */
} ccsr_gur_t;
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] [85xx] Add eTSEC 1/2 IO override control definition
This adds missing tsec12ioovcr to include/asm-ppc/immap_85xx.h. It was
named "res14" (for "reserved") while tsec34ioovcr was properly there.
Signed-off-by: Sergey Kubushyn
---
diff -purN u-boot.orig/include/asm-ppc/immap_85xx.h
u-boot/include/asm-ppc/immap_85xx.h
--- u-boot.orig/include/asm-ppc/immap_85xx.h2009-02-19 13:39:31.0
-0800
+++ u-boot/include/asm-ppc/immap_85xx.h 2009-02-23 10:32:48.0 -0800
@@ -1667,7 +1667,11 @@ typedef struct ccsr_gur {
uintlbiuiplldcr0; /* 0xe0f1c -- LBIU PLL Debug Reg 0 */
uintlbiuiplldcr1; /* 0xe0f20 -- LBIU PLL Debug Reg 1 */
uintddrioovcr; /* 0xe0f24 - DDR IO Override Control */
+#ifdef CONFIG_MPC8548
+ uinttsec12ioovcr; /* 0xe0f28 - eTSEC 1/2 IO override control */
+#else
uintres14; /* 0xe0f28 */
+#endif
uinttsec34ioovcr; /* 0xe0f2c - eTSEC 3/4 IO override control */
charres15[61648]; /* 0xe0f30 to 0xef */
} ccsr_gur_t;
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Davinci I2C code cleanup
On Mon, 23 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > > I am sure you could come up with an efficient *and* clean solution - if > > > you really wanted to try. > > > > There is no way you can avoid that very same code repeated several times (7 > > or so? didn't count them) -- that is how the driver works. > > See above - I wrote "if you really wanted to try." You don't. > > > > > The code as is is not acceptable and needs to be cleaned up. > > > > Why not? What is wrong with that code? > > I will not explain it again. We are splitting hairs here... --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Davinci I2C code cleanup
On Sun, 22 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > > I suggest that "tmp" gets passed as argument to the macro. > > > > It is _NOT_ a macro working on some variable. It is simply repeating > > Oh yes, of course it is. It references the variable "tmp", and this is > nasty as it is not clear to the reader of the code. > > If I think it would be better to rename "tmp" into "foo" the code will > magically break. It won't magically break, it will fail to compile. It is totally different case. This macro does not come from some header file, it is in the same relatively small source file so it is easy to trace if compilation broke. Also it is not used anywhere else so there is no risk it could break anything else. And there is absolutely no reason to rename tmp to foo. > > _LITERAL_ text that not only accesses a local variable tmp but even > > performs a function return with an error if condition is not met. That is, > > You are right. That's even worse. Yep. That's if we are to follow those dogmas to the letter, blindly. But we don't have to. > > why, BTW, it should not be made into an inline function because it would > > require additional checking of its return code where it is used (lot of > > places in Davinci I2C driver) that, in turn, would've added a lot of > > unnecessary overhead for local variable, register loads, second conditional > > checking etc. > > I am sure you could come up with an efficient *and* clean solution - if > you really wanted to try. There is no way you can avoid that very same code repeated several times (7 or so? didn't count them) -- that is how the driver works. > > It is _NOT_ supposed to be any function-like macro. It is just a fragment of > > _LITERAL_ text. > > The code as is is not acceptable and needs to be cleaned up. Why not? What is wrong with that code? --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Davinci I2C code cleanup
On Sun, 22 Feb 2009, Wolfgang Denk wrote: > Dear Jean-Christophe PLAGNIOL-VILLARD, > > In message <20090222124436.ga9...@game.jcrosoft.org> you wrote: > > On 13:52 Thu 19 Feb , k...@koi8.net wrote: > > > Removed CHECK_NACK macro from Davinci I2C driver for code cleanup. > > > > > > Signed-off-by: Sergey Kubushyn > > > --- > > > cpu/arm926ejs/davinci/i2c.c | 62 +- > > > 1 files changed, 45 insertions(+), 17 deletions(-) > > NACK > > > > please explain why > > Please see previous discussion about multibus I2C support. > > > IMHO duplicate code is really wrong > > I agree. The macro should indeed NOT be deleted, but it needs fixing. > It is magically accessing the local variable "tmp" which [Quoting the > CodingStyle] "...might look like a good thing, but it's confusing as > hell when one reads the code and it's prone to breakage from > seemingly innocent changes." > > I suggest that "tmp" gets passed as argument to the macro. It is _NOT_ a macro working on some variable. It is simply repeating _LITERAL_ text that not only accesses a local variable tmp but even performs a function return with an error if condition is not met. That is, why, BTW, it should not be made into an inline function because it would require additional checking of its return code where it is used (lot of places in Davinci I2C driver) that, in turn, would've added a lot of unnecessary overhead for local variable, register loads, second conditional checking etc. It is _NOT_ supposed to be any function-like macro. It is just a fragment of _LITERAL_ text. I don't think it is a right thing to blindly apply "Coding Style" to everything no matter if it applies to it or not. I also NACK this :) --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Sat, 21 Feb 2009, Heiko Schocher wrote: > Hello ksi, > > k...@koi8.net wrote: > > On Fri, 20 Feb 2009, Heiko Schocher wrote: > >> k...@koi8.net wrote: > >>> On Thu, 19 Feb 2009, Heiko Schocher wrote: > >>>> k...@koi8.net wrote: > >>>>> On Wed, 18 Feb 2009, Heiko Schocher wrote: > >>>>>> k...@koi8.net wrote: > >>>>>>> On Mon, 16 Feb 2009, Wolfgang Denk wrote: > [...] > > > Why would we need _ADDITIONAL_ ifs in extremely simple macros that just > > access some memory/io location? They are so simple and small that you can > > not make them any smaller. And _ADDING_ ifs or whatever to them will _NOT_ > > make them any smaller. You can _NOT_ exclude actual code from them, you can > > only _ADD_ to it. If you want e.g. I2C_SDA for 4 interfaces you will have > > _ALL_ the code of 4 separate macros (you can _NOT_ remove a single line from > > them) _PLUS_ those ifs. One more time, it goes _ON TOP_ of whatever is in > > those macros. > > I know this, but as Wolfgang mentioned, we wouldn;t have such a lot of > defines in soft-i2c.c. So that is those defines that look scary, right? As I already said we could have them replaced with just repeated text. It is quite a strange position - make everything ugly, make it bloated, violate every taboo just because something look scary. And why is it just that soft_i2c.c, there are other multiadapter drivers (fsl_i2c.c, mxc_i2c.c, omap*.) Should we also apply those kludges to them? > [...] > > > Here is the path to the real action in my case: > > > > bus_no -> adap_no -> function -> action > > precisly: > i2c_adap[i2c_bus[(bus)].adapter]-> function -> action > > > > Here is yours: > > > > bus_no -> adap_no -> function -> GLOBAL(adap_no) -> hwadap_no -> action > > precisly: > cur_ptr -> function -> if (cur_ptr -> hwadap_no) -> action > > Hmm.. you got an idea, why i want a cur_ptr? No, I don't. That scary looking underlined construction IS your cur_ptr. And the variable itself is an integer, bus number, that doesn't have to be adjusted when the code is relocated and i2c_get_bus_num() just returns this integer. And it is file scope. > Just saying we don;t want in soft-i2c.c this hwadap_no > > cur_ptr -> function -> action > > Which _is_ faster than your actual implementation, or? My actual implementation. > And you need not to do: > > include/i2c.h: > #ifndef CONFIG_SYS_I2C_DIRECT_BUS > #define ADAP(bus) i2c_adap[i2c_bus[(bus)].adapter] > #else > #define ADAP(bus) i2c_adap[(bus)] > #endif > > it is always just cur_ptr-> OK, I will not do a lengthy analysis again, I do not have more time to waste. With your cur_ptr you introduce a whole bunch of problems, violate rules of good engineering and do not provide _ANY_ benefits. > > Not only that makes the path longer, not only it requires a blasphemy of > > using a global variable in object methods, but it also defeats a purpose of > > having a separate object for each adapter. Why do we need those separate > > objects if they contain exactly the same poiners to the very same functions? > > > > There is already an index into the objects array and it is already used to > > choose a proper object. Why should we use that index again in the already > > chosen object? Why go twice over the same rake? And even if we were going > > that way what do we gain by using such horrible kludges? Is there any > > benefit? > > Only to save SourceCode as Wolfgang mentioned. Don;t ignore this. You do NOT save any SourceCode. And there is no reason to save SourceCode even if you did. > >>>>> pointers to those functions to appropriate adapter struct members at > >>>>> _COMPLILE_ time. And that's all to it. > >>>>> > >>>>> In your case you stick those functions in one monstrous i2c_write() > >>>>> where > >>>>> you still have those N functions as "case" bodies of some switch so you > >>>> No, just the SDA,SCL accesses have such a switch. > >>> So we should make a monster with switches off of each and every > >>> send_start() > >>> and friends and pass them a value to decide which set to use? > >> It is not in send_start(), it is in every SDA,SCL fn/macro, again, I sent > >> you a soft-i2c driver proposal. If you look in this you see, no fn > >> in it is changed! > >> > >> We want this, because we don;t need to change everything in
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Fri, 20 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> k...@koi8.net wrote:
> > On Thu, 19 Feb 2009, Heiko Schocher wrote:
> >
> >> Hello ksi,
> >>
> >> k...@koi8.net wrote:
> >>> On Wed, 18 Feb 2009, Heiko Schocher wrote:
> >>>
> >>>> Hello ksi,
> >>>>
> >>>> k...@koi8.net wrote:
> >>>>> On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> >>>>>
> >>>>>> Dear k...@koi8.net,
> >>>>>>
> >>>>>> In message you
> >>>>>> wrote:
> >>>>>>> That means you have to make changes in two places instead of one --
> >>>>>>> config
> >>>>>>> file AND $(BOARD).c. Also you use functions instead of macros and you
> >>>>>>> can
> >>>>>>> NOT make them inline because they come from a separate object file.
> >>>>>>> This
> >>>>>>> essentially defeats the very purpose of that common soft_i2c.c
> >>>>>>> driver. If
> >>>>>>> you want to make functions for bitbanged I2C into the $(BOARD).c
> >>>>>>> there is no
> >>>>>>> reason to have them as a base for that driver. It is much more
> >>>>>>> logical to do
> >>>>>>> everything in reverse, i.e. instead of having soft_i2c.c as a bona
> >>>>>>> fide
> >>>>>>> drivers and those I2C_SDA and friends as its building blocks make
> >>>>>>> those
> >>>>>>> i2c_soft_sda() etc. in each and every $(BOARD).c into primary
> >>>>>>> entities and
> >>>>>>> build the actual driver in the $(BOARD).c itself. Just convert that
> >>>>>>> soft_i2c.c into a header file with macros for real functions
> >>>>>>> (soft_i2c_read
> >>>>>>> etc.) and instantiate them in the $(BOARD).c.
> >>>>>> Ecept that the code you posted is unreadable and you will need lots of
> >>>>>> very good arguments to make me accept it.
> >>>>> What is unreadable in that code?
> >>>> I wouldn;t say unreadable but unnecessary swollen.
> >>>>
> >>>>> Take e.g. this:
> >>>>>
> >>>>> === Cut ===
> >>>>> #define I2C_SOFT_SEND_START(n) \
> >>>>> static void send_start##n(void) \
> >>>>> { \
> >>>> [...]
> >>>>> I2C_DELAY2;
> >>>>> I2C_SDA2(0);
> >>>>> I2C_DELAY2;
> >>>>> }
> >>>>> === Cut ===
> >>>>>
> >>>>> This will be generated at compile time and fed to gcc.
> >>>>>
> >>>>> What is so unreadable here?
> >>>>>
> >>>>> Sure I can make all the instances manually and avoid those #define's
> >>>>> but it
> >>>>> will not make that source file any more readable by simply repeating
> >>>>> those
> >>>>> functions several times with just that "##n" different. And it will make
> >>>>> that source file 4 times bigger with 4 instances or twice as big if
> >>>>> there
> >>>>> are only two of them.
> >>>> Again, if you use, as i proposed, this cur_adap_nr pointer, you didn;t
> >>>> have to change anything in this driver (I posted such a patch as a
> >>>> proposal)
> >>>>
> >>>> And again, you don;t need to do, as i did in this proposal, make this
> >>>> I2C_SDA, ... in function. You can of course make this in macros. OK, you
> >>>> have one more if but that shouldn;t be such a problem!
> >>> What problem? Look, for a generic case _ALL_ those I2C_SDA etc. building
> >>> blocks are _ABSOLUTELY_ different for different adapters. You can _NOT_
> >>> parameterize them, you will have _ABSOLUTELY_ different i2c_write() etc.
> >>> for
> >>> different adapters. There is _NOTHING_ common between them.
> >> How SDA, SCL are get/set is common, just how SDA and SCL are accessed is
> >> different! So there is no need for different i2c_write(), ... only SDA,SCL
> >> accessors are different.
> >
> > Argh... Do you understand
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Thu, 19 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > Argh... Do you understand that those send_start etc. are _NOT_ the > > functions? One more time -- they are _NOT_ functions, they are _TEMPLATES_. > > > Please, please: calm down. > > I think there is no reason to claim that Heiko did not understand > this. Yeah, that was a bit too far from my side, my apologies for that. > On the other side, I, too, feel that you should (just for the > theoretical possibility of another, probably completey stupid and > braindead and whatever, but still another approach to solve the same > set of problems) try to follow Heiko's thoughts and see where that > would take us? > > If you continue to perseverate on the position that your code is the > only possible solution, we might as well stop the discussion here. Eh, I did try and I analyzed that approach in several postings. Please trust me, I'm not sticking to my own approach and I do not hesitate to borrow anything I could. Nobody's perfect and one have to run very fast just to stay in place in our profession less for moving forward... We have to learn constantly and I love to learn new things. That replaces me drugs, alcohol, whatever. I wouldn't've object for a second if that've had any benefits. But that did not have any and handicaps however small they are were aplenty. > > Hm... Please explain how are you going to use 2 different sets of pins with > > different access methods with one function? > > Been there before. See for example > http://lists.denx.de/pipermail/u-boot/2009-February/047937.html Yep. It is possible but it is not any better. It doesn't provide any benefits. I did already explain this in several postings. If it did give some benefits I would definitely use that way. > > So we should make a monster with switches off of each and every send_start() > > and friends and pass them a value to decide which set to use? > > No, of course not. > > > That makes everything more messy and has a performance penalty -- instead of > > simple branch to those simple blocks you are adding at least one register > > load per call to load that argument. > > That seams a _terrible_ overhead to me: one register load per call. > How fast is that I2C bus? Well below 50 kByte per second, right? > > Please stay serious. It is not big but it IS an overhead. One register load here, one function call there and soon we are bloated up to our ears. But the main factor is NOT that overhead. It simply doesn't give any benefits so it is not any better even if didn't have any overhead. I wouldn't hesitate to go that way if there were any benefits. > > > No problem with this. > > > > There IS a problem First, that means you have _ABSOLUTELY_ no way to access > > any other adapter than default one until that variable made writable. > > Please let's stop considering this as a problem. > > Let's accept that this is a self-imposed and accepted restriction. Eh, this is not about switching busses while running from flash, I was not going to support this either. But it gives at least some means for a simple hack if one or two boards really need this. Using global variable for this a.) does not give any benefits and b.) removes any possibility for accessing any adapter but the default one at all, hackish or not. > > Second, all class functions must be self-sufficient, they should NOT rely on > > some external global variable to work. That might sound C++ese but no matter > > how I don't like C++ it does many things right. > > We don't use C++ here, and we are not that strict if it allows for > smaller code or solves other problems. But it does not make code smaller or solve any problems. I would definitely consider it if it did. > > I can call a member function for any adapter directly if needed with > > adap[N]->function(). You can NOT because in your case that "N" does not have > > any effect and your functions rely on an external global variable that you > > can not change. > > Regards from the rat race. I am really tired of that. > > It makes no sense to continue this discussion with you when you are > not willing to even discuss the possibility of an alternative > implementation that allows for a (writable!) global variable, and > then you continue claiming we could not use writable variables > (especially when we will not even require it because we don;t need > bus switching before relocation). > > There are better things I can do in my spare time. Eh... I AM discussing it here. And that is NOT about switching busses while running from flash... Guys, why are you not listening?... That variable, put aside all the ugliness and blasphemy, what problem it solves and what benefits it gives? Please show me some, pull my eyelids and open my eyes and may be I'll see the light... Where are the benefits? > > That's a separate issue. I can offer N other ways to achieve this other
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Thu, 19 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > > For what you will do this, when you can;t use the adapter, when running > > > from > > > flash? See Later, why you cannot use it. > > > > Using multiple adapters while running from flash is an exception. I can let > > myself call adapter-specific functions directly if needed, without changing > > busses. > > Maybe we all can save some time here? > > For the following discussion, please let's assume that we have > reached an agreement that we do NOT support multiple adapters while > running from flash. > > OK? Thanks. As a matter of fact I wasn't going to support it either :) That bus_num variable is not writable before relocation and I deliberately made i2c_set_bus() fail when running from flash. But if somebody wants to use several adapters it can do it by calling adapter methods directly. This provides necessary means for a simple hack if a board or two would require such access thus saving us from extensive and complex hack. But those are clear exceptions and they are NOT covered by regular API. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Thu, 19 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > > in board config file ... OK, we are worser against your approach, because > > > we have for all I2C_SDA, I2C_SCL accesses + 1 switch, but I don;t think > > > this is such a problem. > > > > First of all, you are using an external global variable for object methods. > > That is a VERY BAD practice and I can't even imagine a use case that would > > justify this. > > We are pretty pragmatic here. If it solves a problem efficiently, we > use even global variables. But not in object methods... And it must serve some purpose. I'm a sinner myself and I have to confess to even using goto's sometimes but it must have some reason... > > That means you'll have to rewrite the entire U-Boot. 99% of the boards have > > only one bus so they did not switch busses. That means they never called > > that i2c_set_bus_num() relying on i2c_init() in libxxx/board.c instead. > > I cannot follow your argument. > > Yes, the status quo is as you describe, it relies on i2c_init() and > is simple-minded and does not support an arbitry number of > arbitrarily complex I2C bus trees and multiplexors and expanders and > what else. But it was sufficient for the first 10 years and 500 > boards of U-Boot development. > > Now we are discussion a major redesign, so what is the big problem of > changing this part? "rewrite the entire U-Boot"? Please stay serious. > Compared to the other changes you suggest, this is not that big a > part. No, my changes are limited. Look, somebody must initialize an adapter. As for now it is done with a single i2c_init() usually in libxxx/board.c. Then the entire code assumes adapter is already initialized and just issues i2c_read/write() as it see fits. 99% of this code is written on assumption that there is only one I2C bus so it doesn't use i2c_set_bus_num() or whatever, it just fires up i2c_read() and that's it. This would perfectly work with my changes without modifying that code -- the only bus is bus number 0 so there is nothing wrong with not setting the bus for each I2C access; it is already at that only bus. Now, if we have adapter initialization moved to i2c_set_bus() all that code will cease to work because i2c_set_bus() is never called thus adapter will never be initialized and all those i2c_read() and friends will fail. That means that we should read through each and every board's code to find where i2c functions are used and add i2c_set_bus() calls as needed. That is not INSTEAD of that big rewrite, that is _IN ADDITION_ to it. That is a very sizeable chunk of additional changes. I DID think of adding adapter initialization to i2c_set_bus() initially but then it turned out it generated more problems than it solved (and it solved none) so I dropped that idea. > > Sorry guys, I do not have THAT much free time that my employer would let me > > to spend on this. > > Well, you at least have some commercial motivation to spend time for > this code and discussion, while for me it's all my "free" time (and I > better don't tell you what my wife says of my interpretation "free" > here). Eh, you won't believe what constitutes my "free" time and for how long ahead that "free" time is planned out. I don't think mere mortals live that long... :) --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 6/12 Multiadapter/multibus I2C, drivers part 3
On Thu, 19 Feb 2009, Wolfgang Denk wrote:
> Dear k...@koi8.net,
>
> In message you wrote:
> >
> > > Macros with magic side effects (here on the variable "tmp" are stronly
> > > deprecated. Please fix this.
> >
> > There is exactly the same code in Davinci I2C driver that had been accepted
> > and made it in the main tree. The reason behind this macro is to make source
>
> Sorry that the code slipped through the review then.
>
> > text smaller and easier to read. It is not used anywhere outside the driver
>
> That's not easier to read. Macros with side effects are dangerous.
> Quoting the CodingStyle: "...might look like a good thing, but it's
> confusing as hell when one reads the code and it's prone to breakage
> from seemingly innocent changes."
>
> > file. Sure I can remove it and put that text as is anywhere where it is used
> > (3 places in sm502_i2c.c) but will it make it any better or more readable? I
> > seriously doubt it.
>
> I am sure about it.
>
> And while you are at it, I would appreciate if you could also post a
> (separate) patch that fixes the same thing in the Davinci I2C driver
> you were referring to above. Thanks.
>
> > Are we going to do the same for Davinci I2C driver for consistency?
>
> Yes, please.
OK, posted a patch.
> > > Also, instead of register offsets ("...base + SM501_I2C") please use
> > > a proper data structure.
> >
> > Are we dropping Linux compatibility or what? That sm501-regs.h file with
> > those offsets has been directly stolen from Linux kernel source and that's
> > the way they do it in the kernel. I don't have any problems with making a
>
> I am aware that there are areas in the Linux kernel that could need a
> thourough rework - like in any bigger project.
>
> Borrowing good code from Linux is excellent. But there is no reason
> not to improve poor code when we run into it. Who knows, maybe one
> day Linux might copy code from U-Boot - now would that be a bad
> thing?
OK, will replace it with a structure, no problems. I do personally prefer a
structure too; that code was against my nature but I tried to keep Linux
kernel files intact.
[dd]
> > What's wrong with bracing a single line statement? I personally think it
> > makes code more readable and less prone for errors. But sure, I can change
> > this, no problems...
>
> I agree with you - my personal preference is quite often to use
> braces there, too. But we try all to stick to a common coding style,
> so everybody has to swallow his own set of bitter pills.
OK, no problems.
> > > And "return" is not a function - please omit the parens (here and
> > > elsewhere).
> >
> > There is a lot of places where parens are used with "return" in U-Boot (and
> > elsewhere.) C does not REQUIRE parens but also does not forbid or discourage
> > them. I personally think it is better to use excessive parens than not to
> > use them when they are required.
> >
> > But sure I can change it if it really matters...
>
> We try to be good citicens and follow the CodingStyle, that's all...
OK.
> > > > + while ((read_i2c_reg(SM501_I2C_STATUS) & SM501_I2C_BUS_BUSY) &&
> > > > timeout--) {
> > > ...
> > > > diff -purN u-boot-i2c.orig/include/sm501-regs.h
> > > > u-boot-i2c/include/sm501-regs.h
> > > > --- u-boot-i2c.orig/include/sm501-regs.h1969-12-31
> > > > 16:00:00.0 -0800
> > > > +++ u-boot-i2c/include/sm501-regs.h 2009-02-12 10:46:00.0
> > > > -0800
> > > > @@ -0,0 +1,394 @@
> > > > +/* sm501-regs.h
> > >
> > > Incorrect multiline comment style.
> >
> > This again comes from Linux kernel source. Should I fix such borrowed code?
>
> Since more changes are required to that file anyway: yes, please.
OK.
> > > Please do not use register offsets, define a C structure instead.
> >
> > Again, this came from Linux kernel. Should I throw it away and make my own
> > header file? Please advise.
>
> Yes, please.
OK, not a big deal, will do.
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] Davinci I2C code cleanup
Removed CHECK_NACK macro from Davinci I2C driver for code cleanup.
Signed-off-by: Sergey Kubushyn
---
cpu/arm926ejs/davinci/i2c.c | 62 +-
1 files changed, 45 insertions(+), 17 deletions(-)
diff --git a/cpu/arm926ejs/davinci/i2c.c b/cpu/arm926ejs/davinci/i2c.c
index 3ba20ef..f48aae2 100644
--- a/cpu/arm926ejs/davinci/i2c.c
+++ b/cpu/arm926ejs/davinci/i2c.c
@@ -32,14 +32,6 @@
#include
#include
-#define CHECK_NACK() \
- do {\
- if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) {\
- REG(I2C_CON) = 0;\
- return(1);\
- }\
- } while (0)
-
static int wait_for_bus(void)
{
@@ -179,7 +171,11 @@ int i2c_read(u_int8_t chip, u_int32_t addr, int alen,
u_int8_t *buf, int len)
tmp = poll_i2c_irq(I2C_STAT_XRDY | I2C_STAT_NACK);
- CHECK_NACK();
+ /* Check for NACK */
+ if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) {
+ REG(I2C_CON) = 0;
+ return(1);
+ }
switch (alen) {
case 2:
@@ -193,7 +189,11 @@ int i2c_read(u_int8_t chip, u_int32_t addr, int alen,
u_int8_t *buf, int len)
tmp = poll_i2c_irq(I2C_STAT_XRDY |
I2C_STAT_NACK);
- CHECK_NACK();
+ /* Check for NACK */
+ if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) {
+ REG(I2C_CON) = 0;
+ return(1);
+ }
/* No break, fall through */
case 1:
/* Send address LSByte */
@@ -206,7 +206,11 @@ int i2c_read(u_int8_t chip, u_int32_t addr, int alen,
u_int8_t *buf, int len)
tmp = poll_i2c_irq(I2C_STAT_XRDY |
I2C_STAT_NACK | I2C_STAT_ARDY);
- CHECK_NACK();
+ /* Check for NACK */
+ if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) {
+ REG(I2C_CON) = 0;
+ return(1);
+ }
if (!(tmp & I2C_STAT_ARDY)) {
REG(I2C_CON) = 0;
@@ -224,7 +228,11 @@ int i2c_read(u_int8_t chip, u_int32_t addr, int alen,
u_int8_t *buf, int len)
for (i = 0; i < len; i++) {
tmp = poll_i2c_irq(I2C_STAT_RRDY | I2C_STAT_NACK |
I2C_STAT_ROVR);
- CHECK_NACK();
+ /* Check for NACK */
+ if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) {
+ REG(I2C_CON) = 0;
+ return(1);
+ }
if (tmp & I2C_STAT_RRDY) {
buf[i] = REG(I2C_DRR);
@@ -236,7 +244,11 @@ int i2c_read(u_int8_t chip, u_int32_t addr, int alen,
u_int8_t *buf, int len)
tmp = poll_i2c_irq(I2C_STAT_SCD | I2C_STAT_NACK);
- CHECK_NACK();
+ /* Check for NACK */
+ if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) {
+ REG(I2C_CON) = 0;
+ return(1);
+ }
if (!(tmp & I2C_STAT_SCD)) {
REG(I2C_CON) = 0;
@@ -279,7 +291,11 @@ int i2c_write(u_int8_t chip, u_int32_t addr, int alen,
u_int8_t *buf, int len)
/* Send address MSByte */
tmp = poll_i2c_irq(I2C_STAT_XRDY | I2C_STAT_NACK);
- CHECK_NACK();
+ /* Check for NACK */
+ if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) {
+ REG(I2C_CON) = 0;
+ return(1);
+ }
if (tmp & I2C_STAT_XRDY) {
REG(I2C_DXR) = (addr >> 8) & 0xff;
@@ -292,7 +308,11 @@ int i2c_write(u_int8_t chip, u_int32_t addr, int alen,
u_int8_t *buf, int len)
/* Send address LSByte */
tmp = poll_i2c_irq(I2C_STAT_XRDY | I2C_STAT_NACK);
- CHECK_NACK();
+ /* Check for NACK */
+ if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) {
+ REG(I2C_CON) = 0;
+ return(1);
+ }
if (tmp & I2C_STAT_XRDY) {
REG(I2C_DXR) = addr & 0xff;
@@ -305,7 +325,11 @@ int i2c_write(u_int8_t chip, u_int32_t addr, int alen,
u_int8_t *buf, int len)
for (i = 0; i < len; i++) {
tmp = poll_i2c_irq(I2C_STAT_XRDY | I2C_STAT_NACK);
- CHECK_NACK();
+ /* Check for NACK */
+ if (tmp & (I2C_TIMEOUT | I2C_STAT_NACK)) {
+ REG(I2C_CON) = 0;
+
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Thu, 19 Feb 2009, Heiko Schocher wrote:
> Hello Wolfgang,
>
> Wolfgang Denk wrote:
> > Dear k...@koi8.net,
> >
> > In message you wrote:
> >> How would you know what to initialize and what not to? We were initializing
> >
> > I don't know. You probably need some way to encode some kind of
>
> Look at my proposal for i2c_set_bus_num().
>
> http://lists.denx.de/pipermail/u-boot/2009-February/047860.html
>
> This function is the central point for switching between busses. It
> _must_ be called, when switching busses, so this function can decide,
> if the hardware adapter is initialized or not. If not, it will
> call the init function ... and there is no problem for calling
> the init function, when running in flash. We _first_ call the
> adapter specific init() function and _then_ switching the muxes.
>
> Also we can do in this function the adapterspecific deinit()
> function ... one central point for doing init(), deinit()
> sounds good to me.
>
> We just have to look, that boards who uses i2c_* functions,
> now call before accessing the I2C bus, i2c_set_bus_num().
> But this is with every implemntation for multibussuport a ToDo,
> because there probably now more then one bus, so we must
> take care of this.
>
> >> It is easy to initialize just a selected set of adapters in the new code
> >> but
> >> how do we decide what to initialize and what not to?
> >
> > Good question. Probably each I2C device will have some list of
> > bus/adapter ID's that need to be up to access it, and that will get
> > shut down afterwards.
>
> No need for this i2c_set_bus_num() will do that automagically.
>
> >> Should we add a config option like CONFIG_I2C_INIT_ADAPTERS {1,3,5,9} ? But
> >
> > No, because we probably do not need to activate all tehse adapters at
> > the same time.
>
> We don;t need this, because there is i2c_set_bus_number, which can
> do this for us.
>
> >> There is also no way of DE-initilizing those interfaces so that init is
> >> like
> >> a gun trigger -- once it is pulled, there is no way to bring the bullet
> >> back.
> >
> > That needs to be changed, too.
>
> Ack.
Sorry guys, I simply do not have THAT much time... I have my board coming
from assembly house so I'll probably end up with some board-specific hack;
that will be much faster.
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Thu, 19 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> k...@koi8.net wrote:
> > On Thu, 19 Feb 2009, Heiko Schocher wrote:
> >
> >> Hello ksi,
> >>
> >> k...@koi8.net wrote:
> >>> On Wed, 18 Feb 2009, Heiko Schocher wrote:
> >>>
> >>>> Hello Wolfgang,
> >>>>
> >>>> Wolfgang Denk wrote:
> >>>>> Dear k...@koi8.net,
> >>>>>
> >>>>> In message you
> >>>>> wrote:
> >>>> [...]
> >>>>>>> What makes you insist that we cannot change a variable if we need to
> >>>>>>> be able to change one?
> >>>>>> It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And
> >>>>>> number of busses can be bigger than number of adapters (e.g. when some
> >>>>>> busses a reached via muxes or switches.) When doing
> >>>>>> i2c_set_current_bus()
> >>>>>> you are switching _NOT_ adapters, but busses. That involves not only
> >>>>>> changing that global variable but also reprogramming muxes/switches for
> >>>>>> i2c_set_current_bus() to be consistent and hardware independent.
> >>>>>> Otherwise
> >>>>>> your code should know if that particular bus it is switching to is
> >>>>>> directly
> >>>>>> connected or switched and check the bus it is switching from for
> >>>>>> muxes. If
> >>>>>> they are switched, your code should disconnect the current bus
> >>>>>> switches,
> >>>>>> then do that i2c_set_current_bus() and connect the switches to the new
> >>>>>> bus
> >>>>>> after that.
> >>>>>>
> >>>>>> That means that code MUST somehow know the topology to take appropriate
> >>>>>> actions and properly configure those switches. That means you should
> >>>>>> somehow
> >>>>>> describe that topology for each and every board in CONFIG_* terms and
> >>>>>> make
> >>>>>> each and every place at U-Boot that invokes _ANY_ i2c function to take
> >>>>>> care
> >>>>>> of that switching.
> >>>>> You convinced me. This code must not be used before relocation to RAM,
> >>>>> then.
> >>>> But is is possible to use that code when running from flash, if
> >>>> this current pointer is writeable ...
> >>> It is not the pointer that must be writeable, it's what it is pointing
> >>> to...
> >> But in your approach this is also not writeable! So we lost nothing, when
> >> using such a pointer.
> >
> > No, we did NOT. I can still cal adap[N]->init() and it will init the proper
>
> For what you will do this, when you can;t use the adapter, when running from
> flash? See Later, why you cannot use it.
Using multiple adapters while running from flash is an exception. I can let
myself call adapter-specific functions directly if needed, without changing
busses.
> > adapter. It does NOT require any global variables for it, it is
> > self-sufficient.
>
> But you could not switch busses, nor work with it, first because
> i2c_set_bus_num() don;t work for you and i2c_cur_bus is not writeable,
> so _all_ accesses with the API in i2c-core.c like for example
>
> int i2c_read(u_int8_t chip, unsigned int addr, int alen,
> u_int8_t *buffer, int len)
> {
> return(ADAP(i2c_cur_bus)->read(chip, addr, alen, buffer, len));
> }
>
> _will_ fail, when running from flash, because i2c_cur_bus is not
> writeable!
>
> So why will you initialize all adapters when running from flash?
>
> But this is no problem, when we make i2c_cur_bus or this pointer
> I would like to see, writeable. And think about my proposal for
> i2c_set_bus_num() and we have there also no problem with calling
> init() for an adapter.
I'm against using global variables at all. And I will never ever use them in
object's member functions, that's a blasphemy.
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Thu, 19 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> k...@koi8.net wrote:
> > On Wed, 18 Feb 2009, Wolfgang Denk wrote:
> >
> >> Dear k...@koi8.net,
> >>
> >> In message you
> >> wrote:
> >>>> Duplicating the source code (and thus the object code, too) to create
> >>>> additional instances of basicly the same driver seems to be the wrong
> >>>> approach to me.
> >>>>
> >>>> It doesn't scale well, to say the least.
> >>> It is _NOT_ the same driver. Every bitbaged I2C driver is _DIFFERENT_.
> >>> Only
> >>> the _NAME_ is the same.
> >> This is one way to implement it, but not the only one.
> >>
> >> Just to give an example of a different implementation (without
> >> claiming that that would be a better one): we could provide an array
> >> of functions (instead of macros) for each such adapter, so we could
> >> use just a single instance of the driver which takes the address of
> >> the respective array as argument.
> >
> > Yes, it is possible, but it is not the best approach. Most of those macros
> > translate in 2 to 3 words. If you use functions you should add at very least
> > 2 more words to it, one for a function call another for a function return.
> > That is without array, just directly linked functions. If you are to use an
> > array it adds another 2 words -- one for the array element (the pointer)
> > itself and another one for the pointer to that array in i2c function code.
> >
> > Linux uses functions for that but they are _INLINE_ ones in headers included
> > into the driver file i.e. they are essentially macros.
>
> >From which Linux and which driver you speak?
>
> If you use from actual 2.6 Linux the drivers/i2c/algos/i2c-algo-bit.c
> bitbang driver with gpio support (drivers/i2c/busses/i2c-gpio.c)
> you get called your gpio_get/set_value (pin, state) function in your
> board code (or gpio code)!
>
> And in this function you have to switch dependent on the pin, and
> then again to switch dependent on the state, before you can do
> any action ... so tell me, where we are worser, when we making
>
> #define I2C_SDA(bit) \
> switch(cur_adap_nr->hw_adapnr) { \
> case 0: \
> if (bit) { \
> /* set pin for adapter 0 = 1*/ \
> } else { \
> /* set pin for adapter 0 = 0*/ \
> } \
> [...] \
> } \
> } \
>
> in board config file ... OK, we are worser against your approach, because
> we have for all I2C_SDA, I2C_SCL accesses + 1 switch, but I don;t think
> this is such a problem.
First of all, you are using an external global variable for object methods.
That is a VERY BAD practice and I can't even imagine a use case that would
justify this.
Second, what do you gain by commiting such a blasphemy?
> > That means that implementation is much worse than _EXISTING_ one. And out of
> > decent and much worse one which one would you choose?
> >
> >>> The soft_i2c.c is _NOT_ a driver _SOURCE_. It is a _TEMPLATE_ that makes a
> >> This is your implementation. It is not the only possible implemen-
> >> tation. Please try and open your mind to discuss alternative
> >> possibilities as well.
> >
> > No, it is NOT my implementation. It is _EXISTING_ driver in the main tree. I
> > did _NOT_ change the driver, I just made several copies of it.
> >
> > And, BTW, you can see something very similar in Linux kernel
> > (i2c-algo-bit.c.)
>
> Please have a deeper look in it, see above comment.
I did.
> > I'm open to any alternative possibilities but I can not see anything better.
> > That _EXISTING_ soft_i2c.c we have in the current tree is a little miracle
> > that was there since the start of times so I can't see any reason to throw
> > it away and reinvent the wheel.
>
> Nobody wants to throw it away!
You want...
> >>> The former does not require additional adapter struct member, hwadap_no.
> >>> And, unlike the latter, it is self-contained, it doesn't require any
> >>> external global variable to decide what to do. One can initialize all
> >>> adapters by:
> >>>
> >>> for (i = 0; i < NUM_I2C_ADAPTERS; i++)
> >>> i2c_adap[i]->init(...);
> >> Most probably we never *want* to initialize all adapters...
> >
> > It is easier that way and wouldn't do any harm in most cases...
>
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Thu, 19 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> k...@koi8.net wrote:
> > On Wed, 18 Feb 2009, Heiko Schocher wrote:
> >
> >> Hello ksi,
> >>
> >> k...@koi8.net wrote:
> >>> On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> >>>
> >>>> Dear k...@koi8.net,
> >>>>
> >>>> In message you
> >>>> wrote:
> >> [...]
> >>>>> And remember, the devil is in details. How are you going to assign
> >>>>> (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you
> >>>>> going to work on an adapter other that "current" in a situation when
> >>>>> you can
> >>>>> NOT change "current" adapter (e.g. perform all I2C layer initialization
> >>>>> while still running from flash?) Remember, this is plain C and there is
> >>>>> no
> >>>> What makes you insist that we cannot change a variable if we need to
> >>>> be able to change one?
> >>> It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And
> >>> number of busses can be bigger than number of adapters (e.g. when some
> >>> busses a reached via muxes or switches.) When doing i2c_set_current_bus()
> >>> you are switching _NOT_ adapters, but busses. That involves not only
> >>> changing that global variable but also reprogramming muxes/switches for
> >>> i2c_set_current_bus() to be consistent and hardware independent. Otherwise
> >> You have no i2c_set_current_bus() in your code! I think you mean
> >> i2c_set_current_bus(), right?
> >>
> >> And this function fails when running from flash! So, how can you switch
> >> busses with your patches when running from flash?
> >>
> >> Here your function:
> >>
> >> int i2c_set_bus_num(unsigned int bus)
> >> {
> >> #ifndef CONFIG_SYS_I2C_DIRECT_BUS
> >> int i;
> >> u_int8_tbuf;
> >> #endif
> >>
> >> if ((bus >= CONFIG_SYS_NUM_I2C_BUSSES) || !(gd->flags &
> >> GD_FLG_RELOC))
> >> return(-1);
> >> [...]
> >>
> >> This function wouldn;t work from flash ...
> >
> > So what? I don't need that function to initialize adapters.
>
> Read my EMail!
>
> But to switch busses, right? And how you switch busses, when running
> from flash, and you do a:
>
> if ((bus >= CONFIG_SYS_NUM_I2C_BUSSES) || !(gd->flags &
> GD_FLG_RELOC))
> return(-1);
>
> in it??
I'm not going to switch busses while running from flash with that function.
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Thu, 19 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> k...@koi8.net wrote:
> > On Wed, 18 Feb 2009, Heiko Schocher wrote:
> >
> >> Hello ksi,
> >>
> >> k...@koi8.net wrote:
> >>> On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> >>>
> >>>> Dear k...@koi8.net,
> >>>>
> >>>> In message you
> >>>> wrote:
> >>>>> That means you have to make changes in two places instead of one --
> >>>>> config
> >>>>> file AND $(BOARD).c. Also you use functions instead of macros and you
> >>>>> can
> >>>>> NOT make them inline because they come from a separate object file. This
> >>>>> essentially defeats the very purpose of that common soft_i2c.c driver.
> >>>>> If
> >>>>> you want to make functions for bitbanged I2C into the $(BOARD).c there
> >>>>> is no
> >>>>> reason to have them as a base for that driver. It is much more logical
> >>>>> to do
> >>>>> everything in reverse, i.e. instead of having soft_i2c.c as a bona fide
> >>>>> drivers and those I2C_SDA and friends as its building blocks make those
> >>>>> i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities
> >>>>> and
> >>>>> build the actual driver in the $(BOARD).c itself. Just convert that
> >>>>> soft_i2c.c into a header file with macros for real functions
> >>>>> (soft_i2c_read
> >>>>> etc.) and instantiate them in the $(BOARD).c.
> >>>> Ecept that the code you posted is unreadable and you will need lots of
> >>>> very good arguments to make me accept it.
> >>> What is unreadable in that code?
> >> I wouldn;t say unreadable but unnecessary swollen.
> >>
> >>> Take e.g. this:
> >>>
> >>> === Cut ===
> >>> #define I2C_SOFT_SEND_START(n) \
> >>> static void send_start##n(void) \
> >>> { \
> >> [...]
> >>> I2C_DELAY2;
> >>> I2C_SDA2(0);
> >>> I2C_DELAY2;
> >>> }
> >>> === Cut ===
> >>>
> >>> This will be generated at compile time and fed to gcc.
> >>>
> >>> What is so unreadable here?
> >>>
> >>> Sure I can make all the instances manually and avoid those #define's but
> >>> it
> >>> will not make that source file any more readable by simply repeating those
> >>> functions several times with just that "##n" different. And it will make
> >>> that source file 4 times bigger with 4 instances or twice as big if there
> >>> are only two of them.
> >> Again, if you use, as i proposed, this cur_adap_nr pointer, you didn;t
> >> have to change anything in this driver (I posted such a patch as a
> >> proposal)
> >>
> >> And again, you don;t need to do, as i did in this proposal, make this
> >> I2C_SDA, ... in function. You can of course make this in macros. OK, you
> >> have one more if but that shouldn;t be such a problem!
> >
> > What problem? Look, for a generic case _ALL_ those I2C_SDA etc. building
> > blocks are _ABSOLUTELY_ different for different adapters. You can _NOT_
> > parameterize them, you will have _ABSOLUTELY_ different i2c_write() etc. for
> > different adapters. There is _NOTHING_ common between them.
>
> How SDA, SCL are get/set is common, just how SDA and SCL are accessed is
> different! So there is no need for different i2c_write(), ... only SDA,SCL
> accessors are different.
Argh... Do you understand that those send_start etc. are _NOT_ the
functions? One more time -- they are _NOT_ functions, they are _TEMPLATES_.
Template is _NOT_ a code, it is a _TOOL_ that generates code.
> > In my case you simply make N i2c_write() functions for N adapters and assign
>
> Thats not needed.
Hm... Please explain how are you going to use 2 different sets of pins with
different access methods with one function?
> > pointers to those functions to appropriate adapter struct members at
> > _COMPLILE_ time. And that's all to it.
> >
> > In your case you stick those functions in one monstrous i2c_write() where
> > you still have those N functions as "case" bodies of some switch so you
>
> No, just the SDA,SCL accesses have such a switch.
So we should make a monster with switches off of each and every send_start()
and friends and pass the
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Thu, 19 Feb 2009, Heiko Schocher wrote: > Hello ksi, > > k...@koi8.net wrote: > > On Wed, 18 Feb 2009, Heiko Schocher wrote: > > > >> Hello Wolfgang, > >> > >> Wolfgang Denk wrote: > >>> Dear k...@koi8.net, > >>> > >>> In message you > >>> wrote: > >> [...] > >>>>> What makes you insist that we cannot change a variable if we need to > >>>>> be able to change one? > >>>> It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And > >>>> number of busses can be bigger than number of adapters (e.g. when some > >>>> busses a reached via muxes or switches.) When doing i2c_set_current_bus() > >>>> you are switching _NOT_ adapters, but busses. That involves not only > >>>> changing that global variable but also reprogramming muxes/switches for > >>>> i2c_set_current_bus() to be consistent and hardware independent. > >>>> Otherwise > >>>> your code should know if that particular bus it is switching to is > >>>> directly > >>>> connected or switched and check the bus it is switching from for muxes. > >>>> If > >>>> they are switched, your code should disconnect the current bus switches, > >>>> then do that i2c_set_current_bus() and connect the switches to the new > >>>> bus > >>>> after that. > >>>> > >>>> That means that code MUST somehow know the topology to take appropriate > >>>> actions and properly configure those switches. That means you should > >>>> somehow > >>>> describe that topology for each and every board in CONFIG_* terms and > >>>> make > >>>> each and every place at U-Boot that invokes _ANY_ i2c function to take > >>>> care > >>>> of that switching. > >>> You convinced me. This code must not be used before relocation to RAM, > >>> then. > >> But is is possible to use that code when running from flash, if > >> this current pointer is writeable ... > > > > It is not the pointer that must be writeable, it's what it is pointing to... > > But in your approach this is also not writeable! So we lost nothing, when > using such a pointer. No, we did NOT. I can still cal adap[N]->init() and it will init the proper adapter. It does NOT require any global variables for it, it is self-sufficient. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Thu, 19 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > Yes, it is possible, but it is not the best approach. Most of those macros > ... > > That means that implementation is much worse than _EXISTING_ one. And out of > > decent and much worse one which one would you choose? > > Define "decent". It seems we have a different opintion abot such a > definition. > > And define "much worse", please. How much wors is this (in bytes) for > a single adapter, and for 2, 3, or 5? I already gave an estimate. It is at very least not better. It doesn't give any benefits and introduces some problems. Yes, there is already a tradeof and all our weaknesses are just an extension of our advantages but in this case there is no advantages... > > > This is your implementation. It is not the only possible implemen- > > > tation. Please try and open your mind to discuss alternative > > > possibilities as well. > > > > No, it is NOT my implementation. It is _EXISTING_ driver in the main tree. I > > did _NOT_ change the driver, I just made several copies of it. > > Yes, and this is what I'm complaining about. The whole idea behind > software engineering is not to copy code. > > > Well, it looks like it is you are Russian, not me :) It is well known > > Maybe I am; kind of, at least :-) :) > > Russian passion to scrap everything and design a Universal Server Of > > Everything from scratch :) I'm trying to avoid that... > > Me too. > > > > Call it what you want, I call it duplication of code. > > > > It might be a duplication of SOURCE TEXT, but not CODE. > > Source text compiles to object code, doesn't it? Not always. E.g. defines never compile to object code. Function declarations don't compile to object code. And so on... > > > > > Is this theory or did you perform code size measurements? > > > > > > > > It is obvious. Furthermore, it doesn't make sence to count size > > > > difference > > > > here because it is miniscule -- how many I2C adapters do we have on a > > > > board? > > > > > > It is obvious. Famous last words again. > > > > Eh, do you think anyone really has time to make such comparisons to find out > > that size difference is zero? > > You have the code base ready available and can compile and measure > it, can't you? > > If you base your argumentation on such claims, you better have prove > for it. There is a threshhold. Yes, I should've proved it if it had been very different. But in this case the difference is minimal and not worth an effort. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
> Dear k...@koi8.net,
>
> In message you wrote:
> >
> > How would you know what to initialize and what not to? We were initializing
>
> I don't know. You probably need some way to encode some kind of
> routing information that tells you which adapter(s) need to be
> initialized to reach some specific device.
It's already there. ADAP(bus_no) macro takes care of this returning the
right adapter .
> > _ALL_ I2C adapters up to today with a single i2c_init() function. It just
>
> Yes, that's the status quo. And it is not good as is, and shall be
> changed.
>
> > It is easy to initialize just a selected set of adapters in the new code but
> > how do we decide what to initialize and what not to?
>
> Good question. Probably each I2C device will have some list of
> bus/adapter ID's that need to be up to access it, and that will get
> shut down afterwards.
Eh, that is what I call overcomplicated... And it definitely does not belong
to I2C subsystem per se, it's up to the board developer to know which bus
each device is connected to. I2C system can not now where that "afterwards"
happen. If we are reading something from a device, checking some bit,
writing something else back depending on that bit and then repeat the entire
cycle (let's say we are polling for something and until that something
happened we are sending a "please continue" command back to that device,
then we tell it "thanks, stop now",) where in this sequence that
"afterwards" happens?
> > Should we add a config option like CONFIG_I2C_INIT_ADAPTERS {1,3,5,9} ? But
>
> No, because we probably do not need to activate all tehse adapters at
> the same time.
>
> > Or should we remove i2c_init() from _ALL_ common places and let board
> > developers to call i2c_adap[X]->init() as they see fit? But that is a big
> > rewrite... And there is another place, cmd_i2c.c that must be taken care
> > of...
>
> You will always call i2c_init() for a specific I2C device.
>
> The code should automatically know which adapters need to be initia-
> lized to "talk" to that device. Yes, you must somehow describe the
> I2C bus topology, but a single one-way description for the path from
> the specific device to the CPU should be sufficient.
Topology is already there, this is not a problem. That is that
initinialization that is. The a.m. approach means the I2C layer should not
initialize anything at all, it should only provide API to do it (that it
does.) It is up to each board developer to call appropriate init function
when needed. That means huge, no, even _HUGE_ rewrite with entire U-Boot
affected. And this is not grep-and-replace kind of job, it requires intimate
knowledge of the boards affected...
Sorry, I do not have _THAT_ much time, my board is already on a
pick-and-place machine and I will have to start the bringup process really
soon...
>
> > There is also no way of DE-initilizing those interfaces so that init is like
> > a gun trigger -- once it is pulled, there is no way to bring the bullet
> > back.
>
> That needs to be changed, too.
Again, most of the drivers disable the adapter when transfer is complete but
that does not involve turning of its power or clock or reconfiguring GPIOs
or whatever else it was before initial adapter init. Then, it is all
different for different chips. E.g. OMAP can only turn power on/off for all
I2C adapters so initializing any of them should turn the power on and it
must not be turned back off until the last adapter is shutdown. Pinmuxes are
usually configured as a part of board setup etc.
This means a whole lot of, IMHO, absolutely unneeded work.
> > And the final question -- what is wrong with initializing all I2C adapters
> > for a handful of boards that have more than one? What is the problem? I can
>
> Initializing things that are not actually needed is bad for many
> reasons. It takes time, and boot time is critical on many systems. It
> increases the power consumption, and we have a growing number of
> mobile devices where power consumption is critical. It carries big
> potential for "unexpected" behaviour (read: nasty failure modes), and
> so on.
>
> There are projets around that paid a bite price for not deinitia-
> lizing devices after use. I2C may be relatively harmless, but we
> thought the same about USB until it bit. And it bit hard. That's a
> lesson learned.
I'd say it should not be treated as "never do it again" but rather as "take
caution"...
> > make that init_all function a weak alias so if there's some problem with
> > performing total init it can be replaced with board-specific function. But
> > frankly I can not see any problems with initializing all 2-3 adapters for
> > a few multiadapter boards...
>
> A rule is a rule is a rule.
Eh, never say never...
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Wed, 18 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > > Duplicating the source code (and thus the object code, too) to create > > > additional instances of basicly the same driver seems to be the wrong > > > approach to me. > > > > > > It doesn't scale well, to say the least. > > > > It is _NOT_ the same driver. Every bitbaged I2C driver is _DIFFERENT_. Only > > the _NAME_ is the same. > > This is one way to implement it, but not the only one. > > Just to give an example of a different implementation (without > claiming that that would be a better one): we could provide an array > of functions (instead of macros) for each such adapter, so we could > use just a single instance of the driver which takes the address of > the respective array as argument. Yes, it is possible, but it is not the best approach. Most of those macros translate in 2 to 3 words. If you use functions you should add at very least 2 more words to it, one for a function call another for a function return. That is without array, just directly linked functions. If you are to use an array it adds another 2 words -- one for the array element (the pointer) itself and another one for the pointer to that array in i2c function code. Linux uses functions for that but they are _INLINE_ ones in headers included into the driver file i.e. they are essentially macros. That means that implementation is much worse than _EXISTING_ one. And out of decent and much worse one which one would you choose? > > The soft_i2c.c is _NOT_ a driver _SOURCE_. It is a _TEMPLATE_ that makes a > > This is your implementation. It is not the only possible implemen- > tation. Please try and open your mind to discuss alternative > possibilities as well. No, it is NOT my implementation. It is _EXISTING_ driver in the main tree. I did _NOT_ change the driver, I just made several copies of it. And, BTW, you can see something very similar in Linux kernel (i2c-algo-bit.c.) I'm open to any alternative possibilities but I can not see anything better. That _EXISTING_ soft_i2c.c we have in the current tree is a little miracle that was there since the start of times so I can't see any reason to throw it away and reinvent the wheel. > > And it can _NOT_ scale because it is impossible to make a generic driver > > _SOURCE_ for each and every hardware configuration imaginable. That existing > > Impossible. Famous last words. Well, it looks like it is you are Russian, not me :) It is well known Russian passion to scrap everything and design a Universal Server Of Everything from scratch :) I'm trying to avoid that... > > soft_i2c.c makes _GENERATION_ of such a driver trivial. The only problem > > with it is it only makes _ONE_ such interface. > > And it duplicates the source code for each additional instance that is > needed. > > > And no, we are _NOT_ duplicating source code. Source code is _DIFFERENT_ for > > different adapters. We just creating several _INSTANCES_ using that template > > with _DIFFERENT_ parameters. And those instances are all different. The > > template itself does _NOT_ go into the final code. > > Call it what you want, I call it duplication of code. It might be a duplication of SOURCE TEXT, but not CODE. > > > Is this theory or did you perform code size measurements? > > > > It is obvious. Furthermore, it doesn't make sence to count size difference > > here because it is miniscule -- how many I2C adapters do we have on a board? > > It is obvious. Famous last words again. Eh, do you think anyone really has time to make such comparisons to find out that size difference is zero? > > The former does not require additional adapter struct member, hwadap_no. > > And, unlike the latter, it is self-contained, it doesn't require any > > external global variable to decide what to do. One can initialize all > > adapters by: > > > > for (i = 0; i < NUM_I2C_ADAPTERS; i++) > > i2c_adap[i]->init(...); > > Most probably we never *want* to initialize all adapters... It is easier that way and wouldn't do any harm in most cases... --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
> Dear k...@koi8.net,
>
> In message you wrote:
> >
> > You are multiplying entities. i2c_init() is invoked as a part of system
> > bootup process in libXXX/board.c anyways. There is no need for any global
> > variables, even non-writable for proposed code to initialize adapters.
>
> Please keep in mind that (even if it should be different at the
> moment), I2C should only be initialized when needed, i. e. when
> U-Boot is running any code that needs to access the I2C bus, but not
> always after each reset on all systems that have I2C enabled.
>
> This is a mandatory requirement for a rewrite.
How would you know what to initialize and what not to? We were initializing
_ALL_ I2C adapters up to today with a single i2c_init() function. It just
happened that in most cases the total number of adapters was 1 but not
always (fsl_i2c.c is a notable example.) There was no mechanism for
selective initialization in current U-Boot.
It is easy to initialize just a selected set of adapters in the new code but
how do we decide what to initialize and what not to?
Should we add a config option like CONFIG_I2C_INIT_ADAPTERS {1,3,5,9} ? But
that is simply ridiculous and adds _ABSOLUTELY_ unnecessary code.
Or should we remove i2c_init() from _ALL_ common places and let board
developers to call i2c_adap[X]->init() as they see fit? But that is a big
rewrite... And there is another place, cmd_i2c.c that must be taken care
of...
We can easily add such an initialization to i2c_set_current_bus() (or
however it is called) so it will be initializing adapters as bus is changed
but that does _NOT_ solve all the problems and requires messing with the
entire U-Boot source -- that set_bus call should be added everywhere where
I2C is used and it is not a simple grep-and-replace because there is
absolutely no reason to add set_bus to each of e.g. 3 i2c_write() calls
executed in a row...
Please remember, that even with the new code we don't have to modify
anything but config file (trivial one-time changes) for 99% of supported
boards that only use one I2C bus -- no set_bus needed at all.
There is also no way of DE-initilizing those interfaces so that init is like
a gun trigger -- once it is pulled, there is no way to bring the bullet
back.
And the final question -- what is wrong with initializing all I2C adapters
for a handful of boards that have more than one? What is the problem? I can
make that init_all function a weak alias so if there's some problem with
performing total init it can be replaced with board-specific function. But
frankly I can not see any problems with initializing all 2-3 adapters for
a few multiadapter boards...
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] Fix Freescale link scripts for newer GCCs
On Wed, 18 Feb 2009, Wolfgang Denk wrote: > Dear Jon, > > In message <499c58fa.20...@freescale.com> you wrote: > > > > > Trent - you have obviously already spend a lot of effort into > > > analyzing this code - most probably more time than any other of us. > > > Based on your experience and the effort you already spent I think it > > > would be best if you could provided a patch to fix all boards. > > > > Just as a clarification, are you asking Trent to *fix* it > > for all boards, or to generate a patch that will apply to > > all boards? > > He. Of course I can only ask to provide patch that, to the best of > Trent's experience, is supposed to fix the problems. I am aware > thatthere is a chance that it will not work on some boards, but then > it's up to the board maintainers to complain and to help fixing this > (rpobabmly small) number of remaining issues. > > > Specifically, I recall Trent's hesitation was he doesn't have > > access to most boards (not even all PPC boards), and he wasn't > > even sure it was a problem on some other ARCHes in the first place. > > Good point. But which other options do we have? > > We can: > > 1) apply the patch to a small subset of the boards that are known to >be affected (at least all PowerPC boards) > > 2) apply the patch to all boards that are known to be affected (all >PowerPC boards) > > 3) apply the patch to all boards that are most probably affected (all >boards) > > If we go for 1) or even 2), ther eis a pretty high chance that the > problem will byte us later, and until somebody remembers that this > was raised (and fixed) before in another architecture, lots of > efforts will be wasted. > > However, if we now apply a patch that breaks a number of boards we > will quickly learn where the change works and where not. > > In my point of view the risk of a potential damage involved with 3) is > smaller than the probability for damage and the amount of effort cause > by 2) and especially by 1). I would say apply it to all boards (option 3) and see what broke than fix it. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Wed, 18 Feb 2009, Wolfgang Denk wrote:
> Dear k...@koi8.net,
>
> In message you wrote:
> >
> > OK, for bitbang driver it is just a source file size reduction. We can
> > simply duplicate (triplicate etc.) that file for more than one adapter. What
> > I did makes CPP make that duplication instead of us. But I can simply do it
> > manually. It will make soft_i2c.c 4 times bigger (for 4 adapters) and the
> > resulting object code will remain exactly the same, byte-to-byte.
> >
> > The soft_i2c.c source file makes ONE adapter. If we want 2 adapters we
> > should have 2 such files. If we want 4 adapters there should be 4 such
> > files. It is that simple.
>
> Duplicating the source code (and thus the object code, too) to create
> additional instances of basicly the same driver seems to be the wrong
> approach to me.
>
> It doesn't scale well, to say the least.
It is _NOT_ the same driver. Every bitbaged I2C driver is _DIFFERENT_. Only
the _NAME_ is the same.
The soft_i2c.c is _NOT_ a driver _SOURCE_. It is a _TEMPLATE_ that makes a
source from set of macros that come from an external file (in our case it is
board config file.) And that is a beauty of this driver -- one doesn't have
to write his own driver for each and every board; he just defines a set of
basic operations and CPP takes it from there.
And it can _NOT_ scale because it is impossible to make a generic driver
_SOURCE_ for each and every hardware configuration imaginable. That existing
soft_i2c.c makes _GENERATION_ of such a driver trivial. The only problem
with it is it only makes _ONE_ such interface.
And no, we are _NOT_ duplicating source code. Source code is _DIFFERENT_ for
different adapters. We just creating several _INSTANCES_ using that template
with _DIFFERENT_ parameters. And those instances are all different. The
template itself does _NOT_ go into the final code.
> > For the functions that can be parameterized I'm adding a simple wrapper
> > function that simply passes additional argument to that parameterizable
> > function and calls it. That is in no way worse, or bigger, or more complex
> > than accessing some pointer, then getting adapter number, then branching to
> > the appropriate function. The wrapper is simpler and smaller. Codewise it is
> > even smaller if all arguments fit into registers because when you are
> > preparing that actual function call from a wrapper function ALL arguments
> > are already loaded into appropriate registers; you only have to load one
> > additional register with an immediate constant (channel number) and perform
> > a branch.
>
> Is this theory or did you perform code size measurements?
It is obvious. Furthermore, it doesn't make sence to count size difference
here because it is miniscule -- how many I2C adapters do we have on a board?
> > Look what I do:
> >
> > === Cut ===
> > static int _i2c_probe(chip, adap_no)
> > {
> > ...
> > }
> >
> > static int xx_i2c_probe1(chip)
> > {
> > return _i2c_probe(chip, 0);
> > }
> >
> > static int xx_i2c_probe2(chip)
> > {
> > return _i2c_probe(chip, 1);
> > }
> > === Cut ===
>
> Looks like overhead to me.
Sure it is. There is no way how you can avoid overhead without writing a
totally custom U-Boot for each and every board. But it is no worse than
this:
=== Cut ===
static int _i2c_probe(chip)
{
adap_no = ADAP(i2c_cur_bus)->hwadap_no;
}
xx_i2c_probe1 = xx_i2c_probe2 = _i2c_probe;
=== Cut ===
The former does not require additional adapter struct member, hwadap_no.
And, unlike the latter, it is self-contained, it doesn't require any
external global variable to decide what to do. One can initialize all
adapters by:
for (i = 0; i < NUM_I2C_ADAPTERS; i++)
i2c_adap[i]->init(...);
and there is no Catch 22 with changing current bus number to initialize
adapter that requires that adapter must be initialized to change the current
bus number.
As a rule of thumb, member functions should _NEVER_ use any global
variables. It is up to higher layer to work on those if they are required.
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Wed, 18 Feb 2009, Wolfgang Denk wrote: > Dear Heiko, > > In message <499bc39c.9050...@denx.de> you wrote: > > > > >> But is is possible to use that code when running from flash, if > > >> this current pointer is writeable ... > > > > > > Yes, it is possible, but then - ther eis no need for it. > > > > For what is no need? > > For dynamic reconfiguration of I2C busses before relocation. Yep, totally agree. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Wed, 18 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> Heiko Schocher schrieb:
> > Hello ksi,
> >
> > k...@koi8.net wrote:
> >> On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> >>
> >>> Dear k...@koi8.net,
> >>>
> >>> In message you
> >>> wrote:
> > [...]
> >>>> And remember, the devil is in details. How are you going to assign
> >>>> (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you
> >>>> going to work on an adapter other that "current" in a situation when you
> >>>> can
> >>>> NOT change "current" adapter (e.g. perform all I2C layer initialization
> >>>> while still running from flash?) Remember, this is plain C and there is
> >>>> no
> >>> What makes you insist that we cannot change a variable if we need to
> >>> be able to change one?
> >> It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And
> >> number of busses can be bigger than number of adapters (e.g. when some
> >> busses a reached via muxes or switches.) When doing i2c_set_current_bus()
> >> you are switching _NOT_ adapters, but busses. That involves not only
> >> changing that global variable but also reprogramming muxes/switches for
> >> i2c_set_current_bus() to be consistent and hardware independent. Otherwise
> >
> > You have no i2c_set_current_bus() in your code! I think you mean
> > i2c_set_current_bus(), right?
> >
> > And this function fails when running from flash! So, how can you switch
> > busses with your patches when running from flash?
> >
> > Here your function:
> >
> > int i2c_set_bus_num(unsigned int bus)
> > {
> > #ifndef CONFIG_SYS_I2C_DIRECT_BUS
> > int i;
> > u_int8_tbuf;
> > #endif
> >
> > if ((bus >= CONFIG_SYS_NUM_I2C_BUSSES) || !(gd->flags &
> > GD_FLG_RELOC))
> > return(-1);
> > [...]
> >
> > This function wouldn;t work from flash ...
>
> And one more reason, why your function will not work from flash:
>
> later in your i2c_set_bus_num function:
> [...]
> if ((i2c_bus[i2c_cur_bus].next_hop[0].chip != 0) &&
> (ADAP(i2c_cur_bus)->init_done != 0)) {
>
> You only switch muxes if init_done != 0 ... but init_done is
> not writeable when running from flash, so it is "= 0" when
> code is not relocated ... how work this for you?
>
> But this can be solved, if we always init the hardwareadapter
> when switching to it and running from flash ... and if we are
> relocated, we can analyse this flag, if init_done = 0, we init
> this hardware adapter ...
>
> My proposal for the i2c_set_bus_num(unsigned int bus) function:
>
> int i2c_set_bus_num(unsigned int bus)
> {
> #ifndef CONFIG_SYS_I2C_DIRECT_BUS
> int i;
> u_int8_tbuf;
> #endif
>
> if (bus >= CONFIG_SYS_NUM_I2C_BUSSES)
> return(-1);
>
> #ifndef CONFIG_SYS_I2C_DIRECT_BUS
> /* Disconnect current bus (turn off muxes if any) */
> if ((i2c_bus[i2c_cur_bus].next_hop[0].chip != 0) &&
> (ADAP(i2c_cur_bus)->init_done != 0)) {
>
> i = CONFIG_SYS_I2C_MAX_HOPS;
>
> do {
> u_int8_tchip;
>
> if ((chip = i2c_bus[i2c_cur_bus].next_hop[--i].chip) ==
> 0)
> continue;
>
> ADAP(i2c_cur_bus)->write(chip, 0, 0, &buf, 1);
>
> } while (i > 0);
> }
> #endif
>
> cur_adap_nr = (i2c_adap_t *)&ADAP(bus);
> if (ADAP(bus)->init_done == 0) {
> i2c_init_bus(bus, ADAP(bus)->speed, ADAP(bus)->slaveaddr);
> }
>
> #ifndef CONFIG_SYS_I2C_DIRECT_BUS
> /* Connect requested bus if behind muxes */
> if (i2c_bus[bus].next_hop[0].chip != 0) {
>
> /* Set all muxes along the path to that bus */
> for (i = 0; i < CONFIG_SYS_I2C_MAX_HOPS; i++) {
>
> if (i2c_bus[bus].next_hop[i].chip == 0) break;
>
> i2c_mux_set(i2c_bus[bus].adapter,
> i2c_bus[bus].next_hop[i].mux.id,
> i2c_bus[bus].next_hop[i].chip,
> i2c_bus[bus].next_hop[i].channel);
> }
> }
> #e
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Wed, 18 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> k...@koi8.net wrote:
> > On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> >
> >> Dear k...@koi8.net,
> >>
> >> In message you
> >> wrote:
> [...]
> >>> And remember, the devil is in details. How are you going to assign
> >>> (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you
> >>> going to work on an adapter other that "current" in a situation when you
> >>> can
> >>> NOT change "current" adapter (e.g. perform all I2C layer initialization
> >>> while still running from flash?) Remember, this is plain C and there is no
> >> What makes you insist that we cannot change a variable if we need to
> >> be able to change one?
> >
> > It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And
> > number of busses can be bigger than number of adapters (e.g. when some
> > busses a reached via muxes or switches.) When doing i2c_set_current_bus()
> > you are switching _NOT_ adapters, but busses. That involves not only
> > changing that global variable but also reprogramming muxes/switches for
> > i2c_set_current_bus() to be consistent and hardware independent. Otherwise
>
> You have no i2c_set_current_bus() in your code! I think you mean
> i2c_set_current_bus(), right?
>
> And this function fails when running from flash! So, how can you switch
> busses with your patches when running from flash?
>
> Here your function:
>
> int i2c_set_bus_num(unsigned int bus)
> {
> #ifndef CONFIG_SYS_I2C_DIRECT_BUS
> int i;
> u_int8_tbuf;
> #endif
>
> if ((bus >= CONFIG_SYS_NUM_I2C_BUSSES) || !(gd->flags & GD_FLG_RELOC))
> return(-1);
> [...]
>
> This function wouldn;t work from flash ...
So what? I don't need that function to initialize adapters.
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Wed, 18 Feb 2009, Wolfgang Denk wrote: > Dear Heiko Schocher, > > In message <499bb9c6.6010...@denx.de> you wrote: > > > > > You convinced me. This code must not be used before relocation to RAM, > > > then. > > > > But is is possible to use that code when running from flash, if > > this current pointer is writeable ... > > Yes, it is possible, but then - ther eis no need for it. > > > > Yes, it is, because none of them needs any such switching before > > > relocation. And switching is really simple so far. > > > > They use it before relocation, because the DTTs are read before relocation. > > I am not aware that any piece of code in the init sequence makes use > of the information read from the DTT's, so why is this performaned > before relocation? > > More - why is this performed at all for each reset cycle? Normally we > should not even initialize interfaces that are nt used for U-Boot's > own operation. > > I think the automatic DTT checking should be dropped. Exactly. At least it must be postponed until relocation is done. There is nothing required for relocation in DTT. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Wed, 18 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> k...@koi8.net wrote:
> > On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> >
> >> Dear k...@koi8.net,
> >>
> >> In message you
> >> wrote:
> >>> That means you have to make changes in two places instead of one -- config
> >>> file AND $(BOARD).c. Also you use functions instead of macros and you can
> >>> NOT make them inline because they come from a separate object file. This
> >>> essentially defeats the very purpose of that common soft_i2c.c driver. If
> >>> you want to make functions for bitbanged I2C into the $(BOARD).c there is
> >>> no
> >>> reason to have them as a base for that driver. It is much more logical to
> >>> do
> >>> everything in reverse, i.e. instead of having soft_i2c.c as a bona fide
> >>> drivers and those I2C_SDA and friends as its building blocks make those
> >>> i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and
> >>> build the actual driver in the $(BOARD).c itself. Just convert that
> >>> soft_i2c.c into a header file with macros for real functions
> >>> (soft_i2c_read
> >>> etc.) and instantiate them in the $(BOARD).c.
> >> Ecept that the code you posted is unreadable and you will need lots of
> >> very good arguments to make me accept it.
> >
> > What is unreadable in that code?
>
> I wouldn;t say unreadable but unnecessary swollen.
>
> > Take e.g. this:
> >
> > === Cut ===
> > #define I2C_SOFT_SEND_START(n) \
> > static void send_start##n(void) \
> > { \
> [...]
> > I2C_DELAY2;
> > I2C_SDA2(0);
> > I2C_DELAY2;
> > }
> > === Cut ===
> >
> > This will be generated at compile time and fed to gcc.
> >
> > What is so unreadable here?
> >
> > Sure I can make all the instances manually and avoid those #define's but it
> > will not make that source file any more readable by simply repeating those
> > functions several times with just that "##n" different. And it will make
> > that source file 4 times bigger with 4 instances or twice as big if there
> > are only two of them.
>
> Again, if you use, as i proposed, this cur_adap_nr pointer, you didn;t
> have to change anything in this driver (I posted such a patch as a proposal)
>
> And again, you don;t need to do, as i did in this proposal, make this
> I2C_SDA, ... in function. You can of course make this in macros. OK, you
> have one more if but that shouldn;t be such a problem!
What problem? Look, for a generic case _ALL_ those I2C_SDA etc. building
blocks are _ABSOLUTELY_ different for different adapters. You can _NOT_
parameterize them, you will have _ABSOLUTELY_ different i2c_write() etc. for
different adapters. There is _NOTHING_ common between them.
In my case you simply make N i2c_write() functions for N adapters and assign
pointers to those functions to appropriate adapter struct members at
_COMPLILE_ time. And that's all to it.
In your case you stick those functions in one monstrous i2c_write() where
you still have those N functions as "case" bodies of some switch so you
still have that same code size _PLUS_ switch. Than, you assign a pointer to
that monstrous function to i2c_write() member of _ALL_ adapter structures so
a call to i2c_write() ends up calling that function. Now you have to somehow
find out which switch case to execute. For that you need an additional
global variable and additional member in each adapter struct. And those must
be writeable because you don't have any means of executing something like
"adap[N]->i2c_write()", you must prepend it with i2c_set_bus_num(M)
because in your case that "N" in "adap[N]" has absolutely no effect...
Please explain how it is better than simple and straightforward function per
adapter? Which one is more complex?
It looks like I simply don't understand something. You must've meant
something else that I didn't get because the above comparison is SO obvious
that it is almost impossible to somehow misunderstand it...
> > Why should we avoid using CPP feature that is SPECIALLY made for cases like
> > this?
>
> What CPP feature?
Source file preprocessing.
> > Not rocket science and not much of black magic, just simple and
> > straightforward token pasting...
> >
> >>> The only problem with that is it breaks uniformity and makes another mess.
> >>> The whole idea was to bring _ALL_ I2C drivers to a single place and make
> >>> them totally transparent and uniform. Something like
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Wed, 18 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> k...@koi8.net wrote:
> > On Wed, 18 Feb 2009, Wolfgang Denk wrote:
> >
> >> Dear k...@koi8.net,
> >>
> >> In message you
> >> wrote:
> [...]
> >>> OK, this is not about using multiple I2C busses before relocation and
> >>> using
> >>> them right now for any of existing boards (some of which are actually dead
> >>> or vaporware.)
> >> ...
> >>
> >> We agree that there are cases where multiple busses are needed. But do
> >> we need such complexity before relocation?
> >
> > It is not before relocation. I don't see a reason to do this before
> > relocation either. But if we want to have such a possibility after
> > relocation we also need a mechanism to do this.
>
> But, if this current pointer is writeable, it is also usable before
> relocation!
Once again -- it is not the pointer itself that should be writeable, it is
what it is pointing to.
> > And it is not all that complex, I can't understand why you think it is.
>
> We actually mix things:
>
> - complex: There we (Wolfgang and I) talk about your implentation
> of the bitbang driver
>
> not about your i2c-core ...
OK, for bitbang driver it is just a source file size reduction. We can
simply duplicate (triplicate etc.) that file for more than one adapter. What
I did makes CPP make that duplication instead of us. But I can simply do it
manually. It will make soft_i2c.c 4 times bigger (for 4 adapters) and the
resulting object code will remain exactly the same, byte-to-byte.
The soft_i2c.c source file makes ONE adapter. If we want 2 adapters we
should have 2 such files. If we want 4 adapters there should be 4 such
files. It is that simple.
> > As for now we have a set of regular i2c functions (i2c_init/i2c_read/etc.)
> > in each and every i2c driver. Those functions are exported so when we pick
> > up a driver (with e.g. CONFIG_HARD_I2C that metastasize through the entire
> > U-Boot source choosing different drivers for different platforms) those
> > functions get called where i2c_read() etc. are used.
> >
> > I make all those functions static so they are not exported and add a simple
> > exported structure with pointers to those functions. Then there is one
> > wrapper, i2c_core.c that holds global bus number and exports that usual set
> > of i2c_read() and friends. Those functions in turn just play a dispatcher
> > role calling appropriate functions from an array of those driver structures
> > using current bus number as an index into that array. There is nothing more
> > complex than that.
>
> Perfectly.
>
> > The i2c_set_bus_number() is a bit more complex than just changing that
> > global variable to accomodate multiplexed busses but not rocket science
> > either -- if the current bus is multiplexed, it switches off all the muxes
> > in the path starting with the farthest one (if it is multihop, but I doubt
> > we'll see such a beast so it will be just one chip) and turn on the muxes
> > for the new bus (if it is multiplexed) starting with the closest one. The
> > companion i2c_get_bus_num() just returns that global variable.
>
> Ok.
>
> > What is that complex with such a design?
>
> Nothing. And there is not even more complexity, when we add a
> current pointer, or? We just can use this current pointer, to make
> driver code more simpler by using this current pointer and hwadapnr ...
It will NOT make code any simpler. And it will NOT make it any smaller.
For the functions that can be parameterized I'm adding a simple wrapper
function that simply passes additional argument to that parameterizable
function and calls it. That is in no way worse, or bigger, or more complex
than accessing some pointer, then getting adapter number, then branching to
the appropriate function. The wrapper is simpler and smaller. Codewise it is
even smaller if all arguments fit into registers because when you are
preparing that actual function call from a wrapper function ALL arguments
are already loaded into appropriate registers; you only have to load one
additional register with an immediate constant (channel number) and perform
a branch.
Your current pointer adds additional member to the adapter structure and lot
of complexity for such an easy task.
Look what I do:
=== Cut ===
static int _i2c_probe(chip, adap_no)
{
...
}
static int xx_i2c_probe1(chip)
{
return _i2c_probe(chip, 0);
}
static int xx_i2c_probe2(chip)
{
return _i2c_probe(chip, 1);
}
=== Cut ===
Is it any bigger or more complex than using additional structure members,
accessing
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Wed, 18 Feb 2009, Heiko Schocher wrote: > Hello Wolfgang, > > Wolfgang Denk wrote: > > Dear k...@koi8.net, > > > > In message you > > wrote: > [...] > >>> What makes you insist that we cannot change a variable if we need to > >>> be able to change one? > >> It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And > >> number of busses can be bigger than number of adapters (e.g. when some > >> busses a reached via muxes or switches.) When doing i2c_set_current_bus() > >> you are switching _NOT_ adapters, but busses. That involves not only > >> changing that global variable but also reprogramming muxes/switches for > >> i2c_set_current_bus() to be consistent and hardware independent. Otherwise > >> your code should know if that particular bus it is switching to is directly > >> connected or switched and check the bus it is switching from for muxes. If > >> they are switched, your code should disconnect the current bus switches, > >> then do that i2c_set_current_bus() and connect the switches to the new bus > >> after that. > >> > >> That means that code MUST somehow know the topology to take appropriate > >> actions and properly configure those switches. That means you should > >> somehow > >> describe that topology for each and every board in CONFIG_* terms and make > >> each and every place at U-Boot that invokes _ANY_ i2c function to take care > >> of that switching. > > > > You convinced me. This code must not be used before relocation to RAM, > > then. > > But is is possible to use that code when running from flash, if > this current pointer is writeable ... It is not the pointer that must be writeable, it's what it is pointing to... > >> And yes, we DO have some boards with switched I2C busses in U-Boot main > >> tree > >> so this is NOT a hypothetical situation. > > > > Yes, it is, because none of them needs any such switching before > > relocation. And switching is really simple so far. > > They use it before relocation, because the DTTs are read before relocation. > But this is another approach. Actually, the "way" to the DTTs is in > an environment variable. When running from flash, it get directly > parsed, if running from RAM, the var gets analyzed and this "new bus" > is added with the "i2c bus" command resp. by calling the underlying > C function. I don't think there is any need to access DTT before relocation. The only device that absolutely must be read is SPD EEPROM[s]. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Wed, 18 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > > [On the other hand I still wonder why I have never seen any such > > > board appear to me in real life yet. None of the 500+ boards > > > supported in U-Boot uses any such configuration.] > > > > Eh, for those 500+ (actually 472 judging on the number of files in > > include/configs) boards there are at least twice that many that were never > > There are some boards that share one config file... Yes, I know. > > OK, this is not about using multiple I2C busses before relocation and using > > them right now for any of existing boards (some of which are actually dead > > or vaporware.) > ... > > We agree that there are cases where multiple busses are needed. But do > we need such complexity before relocation? It is not before relocation. I don't see a reason to do this before relocation either. But if we want to have such a possibility after relocation we also need a mechanism to do this. And it is not all that complex, I can't understand why you think it is. As for now we have a set of regular i2c functions (i2c_init/i2c_read/etc.) in each and every i2c driver. Those functions are exported so when we pick up a driver (with e.g. CONFIG_HARD_I2C that metastasize through the entire U-Boot source choosing different drivers for different platforms) those functions get called where i2c_read() etc. are used. I make all those functions static so they are not exported and add a simple exported structure with pointers to those functions. Then there is one wrapper, i2c_core.c that holds global bus number and exports that usual set of i2c_read() and friends. Those functions in turn just play a dispatcher role calling appropriate functions from an array of those driver structures using current bus number as an index into that array. There is nothing more complex than that. The i2c_set_bus_number() is a bit more complex than just changing that global variable to accomodate multiplexed busses but not rocket science either -- if the current bus is multiplexed, it switches off all the muxes in the path starting with the farthest one (if it is multihop, but I doubt we'll see such a beast so it will be just one chip) and turn on the muxes for the new bus (if it is multiplexed) starting with the closest one. The companion i2c_get_bus_num() just returns that global variable. And that's all to it. What is that complex with such a design? --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Tue, 17 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > What is unreadable in that code? > > It gets out of control. You don;t see any more how much code gets > generated from that. Why not? But anyways, I can make those instances manually, without CPP participations. Would it be better? > > > This is a boot loader with limited resources, not a general purpose > > > OS. > > > > It doesn't matter. It is much better to have a uniform API for all the > > future developers to use than to multiply horrible hacks and reinventing the > > wheel again and again. > > Well, I tell you again that size does matter, and may even be a > killing point when deciding abouut a patch. There is no much size increase here. Also it is still possible to simply include just those adapters that are required for SPD and thus reduce size if several I2C drivers make it too big. > > > What makes you insist that we cannot change a variable if we need to > > > be able to change one? > > > > It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And > > number of busses can be bigger than number of adapters (e.g. when some > > busses a reached via muxes or switches.) When doing i2c_set_current_bus() > > you are switching _NOT_ adapters, but busses. That involves not only > > changing that global variable but also reprogramming muxes/switches for > > i2c_set_current_bus() to be consistent and hardware independent. Otherwise > > your code should know if that particular bus it is switching to is directly > > connected or switched and check the bus it is switching from for muxes. If > > they are switched, your code should disconnect the current bus switches, > > then do that i2c_set_current_bus() and connect the switches to the new bus > > after that. > > > > That means that code MUST somehow know the topology to take appropriate > > actions and properly configure those switches. That means you should somehow > > describe that topology for each and every board in CONFIG_* terms and make > > each and every place at U-Boot that invokes _ANY_ i2c function to take care > > of that switching. > > You convinced me. This code must not be used before relocation to RAM, > then. > > [On the other hand I still wonder why I have never seen any such > board appear to me in real life yet. None of the 500+ boards > supported in U-Boot uses any such configuration.] Eh, for those 500+ (actually 472 judging on the number of files in include/configs) boards there are at least twice that many that were never submitted to the U-Boot tree... I have ported U-Boot/Linux to at least 10 different boards that were never submitted anywhere myself. You know, company only pays for making their boards work, not for pushing them in the official tree. This is the second company where I have a luxury of crafting patches for submission (the first one was that now defunct that allowed me to push that Davinci port.) And one can not even imagine what runaway hardware designers can design... I still remember some Brasilian MPC8560/Fujitsu ARM boards that were pure nightmare to work on :( > > And yes, we DO have some boards with switched I2C busses in U-Boot main tree > > so this is NOT a hypothetical situation. > > Yes, it is, because none of them needs any such switching before > relocation. And switching is really simple so far. No, it is not easy. You simply did not dig deep enough :) > > > > And the million dollar question -- what is the potential gain? > > > > > > Indeed. The same question goes to you - where is this added > > > complexity really needed? So far nowhere. Are we just talking about > > > hypothetical cases, or about a real design? How many such designs? > > > Just a single one? > > > > Please see above. > > You did not answer my question. OK, this is not about using multiple I2C busses before relocation and using them right now for any of existing boards (some of which are actually dead or vaporware.) This is about consistent logical API for multiple I2C busses. As for multiple busses I personally need that right now for my new board. That board has 2 ST Micro M41ST87 chips on it (braindead chip, I wouldn't've used such a weirdo but it is "certified" by our licensing authorities so I have to) and DS1307 RTT. M41ST87 is a Tamper Detector with RTT but I can not use those RTTs for timekeeping because they are frozen to give a tamper event timestamp hence the separate RTT. Operation procedure requires to check for tamper events on bootup and if one's detected the system must log the current bootup time, give some indication to the user and halt for proper authorities to investigate. The problem is M41ST87 uses a fixed I2C address, 0x68 that is also used by RTT. That means I need 3 I2C busses to talk to those 3 chips... And this is just a first such board out of a whole bunch that we have in various stages. The next one, OMAP3-based, is almost ready, we should have pro
Re: [U-Boot] [PATCH] Multiadapter/multibus I2C, i2c.h fix
On Mon, 16 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > Add missing #endif in include/i2c.h > > > > Signed-off-by: Sergey Kubushyn > > --- > > diff -purN u-boot-i2c.ORIG/include/i2c.h u-boot-i2c/include/i2c.h > > --- u-boot-i2c.ORIG/include/i2c.h 2009-02-13 16:34:36.0 -0800 > > +++ u-boot-i2c/include/i2c.h2009-02-13 16:25:52.0 -0800 > > @@ -226,6 +226,7 @@ void i2c_reloc_fixup(void); > > # else > > # define I2C_SOFT_DEFS > > # endif > > +#endif > > Such a patch must NEVER, never ever be part of a patch series. It > guarantess that we have un-bisectable code in the tree. Please rebase > your branch and squash related patches before posting them here. Sure. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 1/12 Multiadapter/multibus I2C, common part 1, fixed
On Mon, 16 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > Initial multiadapter/multibus I2C support. > > > > Signed-off-by: Sergey Kubushyn > > Please start versioning your patches, and make sure it is clear which > parts belong to which series. > > Do not add such comments as "fixed" to the subject as this will go > into the commit message. > > Note that (as far as I can see) all my comments to the repvious patche > series still apply. Sure. I will keep reposting the entire set until we had something in that i2c-git. And only then I will start to make incremental targeted patches. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Mon, 16 Feb 2009, Wolfgang Denk wrote: > Dear Heiko Schocher, > > In message <49992bce.5020...@denx.de> you wrote: > > > > > You can have, e.g. TWO SPD EEPROMs on different busses. And please > > > remember > > > that infamous "640K ought to be enough for anybody..." > > > > OK, if we really need this. > > But - do we? Really? > > We never promised that U-Boot will support each and every broken > hardware design ;-) Yep. But it would've been nice if it did :) --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Mon, 16 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > > Yes, good point. But do we need more then one i2c adapter when running > > > from flash? I see only one reason to use i2c when running from flash: > > > accessing SPD EEprom ... and this "bus" could always be the first > > > hw adapter. All other accesses to i2c should be moved to run when > > > we are in ram. > > > > You can have, e.g. TWO SPD EEPROMs on different busses. And please remember > > that infamous "640K ought to be enough for anybody..." > > This is a very unlikely hypothetical case. Based on such assumptions > you can design any arbitrary complexity in any code. > > For now, please lets assume that there is no such usage mode, and > that we do not add complexity to support it. > > > > I don't think there is a viable reason to object unless somebody came up > > with something better. > > Well, complex code that is difficult to read and difficult to > understand is a very viable reason to object, me thinks. The code is not all that complex. Most of the code is the same, there is just a thin HAL over it. And it is _NOT_ for reading two SPDs on different busses, it is for consistency and uniformity. Reading two SPDs is a free bonus, there is no a single line of additional code for this. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> Dear k...@koi8.net,
>
> In message you wrote:
> >
> > That means you have to make changes in two places instead of one -- config
> > file AND $(BOARD).c. Also you use functions instead of macros and you can
> > NOT make them inline because they come from a separate object file. This
> > essentially defeats the very purpose of that common soft_i2c.c driver. If
> > you want to make functions for bitbanged I2C into the $(BOARD).c there is no
> > reason to have them as a base for that driver. It is much more logical to do
> > everything in reverse, i.e. instead of having soft_i2c.c as a bona fide
> > drivers and those I2C_SDA and friends as its building blocks make those
> > i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and
> > build the actual driver in the $(BOARD).c itself. Just convert that
> > soft_i2c.c into a header file with macros for real functions (soft_i2c_read
> > etc.) and instantiate them in the $(BOARD).c.
>
> Ecept that the code you posted is unreadable and you will need lots of
> very good arguments to make me accept it.
What is unreadable in that code?
Take e.g. this:
=== Cut ===
#define I2C_SOFT_SEND_START(n) \
static void send_start##n(void) \
{ \
I2C_SOFT_DECLARATIONS##n \
I2C_DELAY##n; \
I2C_SDA##n(1); \
I2C_ACTIVE##n; \
I2C_DELAY##n; \
I2C_SCL##n(1); \
I2C_DELAY##n; \
I2C_SDA##n(0); \
I2C_DELAY##n; \
}
...
I2C_SOFT_SEND_START()
I2C_SOFT_SEND_START(2)
=== Cut ===
That will make two functions replacing "##n" with literal argument:
=== Cut ===
static void send_start(void)
{
I2C_SOFT_DECLARATIONS
I2C_DELAY;
I2C_SDA(1);
I2C_ACTIVE;
I2C_DELAY;
I2C_SCL(1);
I2C_DELAY;
I2C_SDA(0);
I2C_DELAY;
}
static void send_start2(void)
{
I2C_SOFT_DECLARATIONS2
I2C_DELAY2;
I2C_SDA2(1);
I2C_ACTIVE2;
I2C_DELAY2;
I2C_SCL2(1);
I2C_DELAY2;
I2C_SDA2(0);
I2C_DELAY2;
}
=== Cut ===
This will be generated at compile time and fed to gcc.
What is so unreadable here?
Sure I can make all the instances manually and avoid those #define's but it
will not make that source file any more readable by simply repeating those
functions several times with just that "##n" different. And it will make
that source file 4 times bigger with 4 instances or twice as big if there
are only two of them.
Why should we avoid using CPP feature that is SPECIALLY made for cases like
this?
Not rocket science and not much of black magic, just simple and
straightforward token pasting...
> > The only problem with that is it breaks uniformity and makes another mess.
> > The whole idea was to bring _ALL_ I2C drivers to a single place and make
> > them totally transparent and uniform. Something like e.g. Linux VFS.
>
> This is a boot loader with limited resources, not a general purpose
> OS.
It doesn't matter. It is much better to have a uniform API for all the
future developers to use than to multiply horrible hacks and reinventing the
wheel again and again.
> > And remember, the devil is in details. How are you going to assign
> > (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you
> > going to work on an adapter other that "current" in a situation when you can
> > NOT change "current" adapter (e.g. perform all I2C layer initialization
> > while still running from flash?) Remember, this is plain C and there is no
>
> What makes you insist that we cannot change a variable if we need to
> be able to change one?
It is NOT just variable. My approach uses i2c _BUS_, not _ADAPTER_. And
number of busses can be bigger than number of adapters (e.g. when some
busses a reached via muxes or switches.) When doing i2c_set_current_bus()
you are switching _NOT_ adapters, but busses. That involves not only
changing that global variable but also reprogramming muxes/switches for
i2c_set_current_bus() to be consistent and hardware independent. Otherwise
your code should know if that particular bus it is switching to is directly
connected or switched and check the bus it is switching from for muxes. If
they are switched, your code should disconnect the current bus switches,
then do that i2c_set_current_bus() and connect the switches to the new bus
after that.
That means that code MUST somehow know the topology to take appropriate
actions and properly configure those switches. That means you should somehow
describe that topology for each and every board in CONFIG_* terms and make
each and every place at U-Boot that invokes _ANY_ i2c function to take care
of that switching.
My code does it transparently in the single place, i2c_set_current_bus() so
higher level code doesn't have to bother with details.
Then, all those I2C multiplexers/switches are I2C devices theirself that
means you can NOT talk to them if the adapter they connected to is not
initia
Re: [U-Boot] [PATCH] 9/12 Multiadapter/multibus I2C, configs part 2
On Mon, 16 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > Signed-off-by: Sergey Kubushyn > > --- > > diff -purN u-boot-i2c.orig/include/configs/MHPC.h > > u-boot-i2c/include/configs/MHPC.h > > --- u-boot-i2c.orig/include/configs/MHPC.h 2009-02-12 10:43:41.0 > > -0800 > > +++ u-boot-i2c/include/configs/MHPC.h 2009-02-12 10:46:00.0 > > -0800 > > @@ -72,7 +72,12 @@ > > Here again patches are split such that the resutling tree will not be > bisectable. It looks like I didn't get you. Do you want me to make relatively small changes to 472 config files into 472 separate patches? --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 8/12 Multiadapter/multibus I2C, configs part 1
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> Dear k...@koi8.net,
>
> In message you wrote:
> >
> > -#define CONFIG_HARD_I2C/* I2C with hardware support*/
> > -#undef CONFIG_SOFT_I2C /* I2C bit-banged */
> > -#define CONFIG_SYS_I2C_SPEED 40 /* I2C speed and slave
> > address */
> > -#define CONFIG_SYS_I2C_EEPROM_ADDR 0x57
> > -#define CONFIG_SYS_I2C_SLAVE 0x7F
> > -#define CONFIG_SYS_I2C_NOPROBES{0x69} /* Don't probe these addrs */
> > -#define CONFIG_SYS_I2C_OFFSET 0x3000
> > +#define CONFIG_NEW_I2C /* New I2C code */
> > +#define CONFIG_FSL_I2C /* Use FSL common I2C driver */
> > +#define CONFIG_SYS_FSL_I2C_SPEED 40 /* I2C speed and slave address
> > */
> > +#define CONFIG_SYS_FSL_I2C_EEPROM_ADDR 0x57
> > +#define CONFIG_SYS_FSL_I2C_SLAVE 0x7F
> > +#define CONFIG_SYS_I2C_NOPROBES{0x69} /* Don't probe these
> > addrs */
> > +#define CONFIG_SYS_FSL_I2C_OFFSET 0x3000
> > +#define CONFIG_SYS_I2C_ADAPTERS{&fsl_i2c_adap[0]}
>
> Do we really need sucheven more unreadable names?
>
> Why do you have to add the FSL_, SOFT_, ... everywhere?
>
> I think this should be redundant. Omit it.
Because you can have several DIFFERENT adapters in the system. Each of those
adapters should have its own set of configuration parameters. That's why
adapter-specific prefixes are used. There are no prefixes on options common
to the entire I2C sybsystem, e.g. CONFIG_SYS_I2C_ADAPTERS.
Once more - one can have both hardware adapter able to run at 400 kHz and a
bitbanged one only capable of 20 kHz. If we used a generic
CONFIG_SYS_I2C_SPEED, what should we put there? 2? But why that slow
bitbanged interface be a handicap on a fast hardware one? 40? But
bitbanged one can not run at that speed... The same is true for slaveaddr.
CONFIG_SYS_FSL_I2C_EEPROM_ADDR is an error, it should NOT have FSL_ prefix.
That's a typo I will fix.
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> Dear k...@koi8.net,
>
> In message you wrote:
> >
> > +#define I2C_SOFT_SEND_START(n) \
> > +static void send_start##n(void) \
> > +{ \
> > + I2C_SOFT_DECLARATIONS##n \
> > + I2C_DELAY##n; \
> > + I2C_SDA##n(1); \
> > + I2C_ACTIVE##n; \
> > + I2C_DELAY##n; \
> > + I2C_SCL##n(1); \
> > + I2C_DELAY##n; \
> > + I2C_SDA##n(0); \
> > + I2C_DELAY##n; \
> > }
>
> Sorry, but that's becoming an unreadable mess. I don't think I'm going
> to ACK such code.
There is nothing unreadable there. The only alternative I see is to put
several instances of the same functions with those "##n" expanded by hand
instead of letting CPP do it. No problems, I can do that if you want me to.
But it will not make code any more readable or smaller and the final result
will be exactly the same, byte-to-byte.
I can't see any problems with that code--it is in the same single file,
not used anywhere else, cpp magic used is trivial ("##n" is replaced with
literal "n" parameter in instances,) the functions look and feel almost like
original ones--but sure, I can do CPP work myself and put several instances
manually.
Please let me know if you really want me to do this.
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 6/12 Multiadapter/multibus I2C, drivers part 3
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> Dear k...@koi8.net,
>
> In message you wrote:
> ...
> > +/* I2C registers field definitions */
> > +/* */
> > +/* I2C_CONTROL (R/W) */
>
> Incorrect multi-line comment style. Please fix.
OK.
> > +#define SM501_CHECK_NACK() \
> > + do {\
> > + if (tmp & (SM501_I2C_NACK | SM501_I2C_BUS_ERROR)) {\
> > + tmp = read_i2c_reg(SM501_I2C_CONTROL) &
> > SM501_I2C_SPEED;\
> > + write_i2c_reg(SM501_I2C_CONTROL, tmp);\
> > + return(1);\
> > + }\
> > + } while (0)
>
> Macros with magic side effects (here on the variable "tmp" are stronly
> deprecated. Please fix this.
There is exactly the same code in Davinci I2C driver that had been accepted
and made it in the main tree. The reason behind this macro is to make source
text smaller and easier to read. It is not used anywhere outside the driver
file. Sure I can remove it and put that text as is anywhere where it is used
(3 places in sm502_i2c.c) but will it make it any better or more readable? I
seriously doubt it.
And that side effect on tmp is accounted for so there is no magic. Nothing
in the code relies on tmp content after that code fragment is executed. This
is just a text fragment that gets inserted literally at appropriate places.
Please let me know if you still want that changed. I will remove that
standard "do {...} while(0)" wrapper and paste the "if{...}" block at
appropriate places.
Are we going to do the same for Davinci I2C driver for consistency?
> > +static __inline__ void write_i2c_reg(unsigned long offset, u_int8_t data)
> > +{
> > + writeb(data, sm501_iomem_base + SM501_I2C + offset);
> > + __asm__("sync;isync;msync");
>
> Are you sure the "sync;isync;msync" is needed? The accessor functions
> should make sure this is not necessary.
OK. It would do any harm anyways but I will remove that.
> Also, instead of register offsets ("...base + SM501_I2C") please use
> a proper data structure.
Are we dropping Linux compatibility or what? That sm501-regs.h file with
those offsets has been directly stolen from Linux kernel source and that's
the way they do it in the kernel. I don't have any problems with making a
data structure for SM501/502 I2C controller but aren't we supposed to keep
parts stolen from Linux kernel as close to the source as possible?
Please advise.
> > + do {
> > + stat = read_i2c_reg(SM501_I2C_STATUS);
> > + if (stat & mask) {
> > + return(stat);
> > + }
>
> No curly braces for single line statements. Same goes everywhere esle,
> too.
What's wrong with bracing a single line statement? I personally think it
makes code more readable and less prone for errors. But sure, I can change
this, no problems...
> And "return" is not a function - please omit the parens (here and
> elsewhere).
There is a lot of places where parens are used with "return" in U-Boot (and
elsewhere.) C does not REQUIRE parens but also does not forbid or discourage
them. I personally think it is better to use excessive parens than not to
use them when they are required.
But sure I can change it if it really matters...
> > + while ((read_i2c_reg(SM501_I2C_STATUS) & SM501_I2C_BUS_BUSY) &&
> > timeout--) {
> ...
> > diff -purN u-boot-i2c.orig/include/sm501-regs.h
> > u-boot-i2c/include/sm501-regs.h
> > --- u-boot-i2c.orig/include/sm501-regs.h1969-12-31 16:00:00.0
> > -0800
> > +++ u-boot-i2c/include/sm501-regs.h 2009-02-12 10:46:00.0 -0800
> > @@ -0,0 +1,394 @@
> > +/* sm501-regs.h
>
> Incorrect multiline comment style.
This again comes from Linux kernel source. Should I fix such borrowed code?
> ...
> > +#define SM501_GPIO31_0_CONTROL (0x08)
> > +#define SM501_GPIO63_32_CONTROL(0x0C)
> > +#define SM501_DRAM_CONTROL (0x10)
> > +
> > +/* command list */
> > +#define SM501_ARBTRTN_CONTROL (0x14)
> > +
> > +/* command list */
> > +#define SM501_COMMAND_LIST_STATUS (0x24)
> > +
> > +/* interrupt debug */
> > +#define SM501_RAW_IRQ_STATUS (0x28)
> > +#define SM501_RAW_IRQ_CLEAR(0x28)
> > +#define SM501_IRQ_STATUS (0x2C)
> > +#define SM501_IRQ_MASK (0x30)
> > +#define SM501_DEBUG_CONTROL(0x34)
>
> Please do not use register offsets, define a C structure instead.
Again, this came from Linux kernel. Should I throw it away and make my own
header file? Please advise.
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 5/12 Multiadapter/multibus I2C, drivers part 2
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> Dear k...@koi8.net,
>
> In message you wrote:
> >
> ...
> > -int i2c_read (uchar chip, uint addr, int alen, uchar * buffer, int len)
> > +static int omap1510_i2c_read (uchar chip, uint addr, int alen, uchar *
> > buffer, int len)
> > {
> > int i;
> >
> > if (alen > 1) {
> > - printf ("I2C read: addr len %d not supported\n", alen);
> > + printf ("%s: addr len %d not supported\n", __FUNCTION__, alen);
> > return 1;
> > }
> >
> > if (addr + len > 256) {
> > - printf ("I2C read: address out of range\n");
> > + printf ("%s: address out of range\n", __FUNCTION__);
>
> These are changes really to the worse: instead of
>
> I2C read: address out of range
>
> we now get
>
> omap1510_i2c_read: address out of range
>
> More difficult to read, higher memory footprint.
>
> Please revert this.
No problems, will do.
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 2/12 Multiadapter/multibus I2C, common part 2
On Mon, 16 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > Signed-off-by: Sergey Kubushyn > > --- > > diff -purN u-boot-i2c.orig/cpu/mpc8xx/video.c u-boot-i2c/cpu/mpc8xx/video.c > > --- u-boot-i2c.orig/cpu/mpc8xx/video.c 2009-02-12 10:43:41.0 > > -0800 > > +++ u-boot-i2c/cpu/mpc8xx/video.c 2009-02-12 10:46:00.0 -0800 > > Looking close at this "common part 2" patch I think your patches are > not split orthogonally - to me it seems as if "common part 1" and > "common part 2" cannot be separated without causing bisect problems. > > Please fix - and check the rmaining patches for other, similar issues. I don't fully understand what is this but I will try to fix it when posting a next version. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 3/12 Multiadapter/multibus I2C, boards
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> Dear k...@koi8.net,
>
> In message you wrote:
> >
> ...
> > - if (eeprom_read (dev_addr, 0, i2c_buffer,
> > CONFIG_SYS_IVM_EEPROM_MAX_LEN) != 0) {
> > - printf ("Error reading EEprom\n");
> > - return -2;
> > + if (eeprom_read (tmp, 0, i2c_buffer, CONFIG_SYS_IVM_EEPROM_MAX_LEN) !=
> > 0) {
>^^ space kept
> > + printf("Error reading EEprom\n");
> ^^ space deleted.
> > + return(-2);
>
> Please do not mix coding style changes with functional changes in one
> patch. And please do not change style at random - leaving spaces here
> but removing them there. At least use a consistent style.
OK, I won't touch those.
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 2/12 Multiadapter/multibus I2C, common part 2
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> Dear k...@koi8.net,
>
> In message you wrote:
> > Signed-off-by: Sergey Kubushyn
> > ---
> > diff -purN u-boot-i2c.orig/cpu/mpc8xx/video.c u-boot-i2c/cpu/mpc8xx/video.c
> > --- u-boot-i2c.orig/cpu/mpc8xx/video.c 2009-02-12 10:43:41.0
> > -0800
> > +++ u-boot-i2c/cpu/mpc8xx/video.c 2009-02-12 10:46:00.0 -0800
> > @@ -809,7 +809,11 @@ static void video_encoder_init (void)
> >
> > /* Initialize the I2C */
> > debug ("[VIDEO ENCODER] Initializing I2C bus...\n");
> > +#ifdef CONFIG_NEW_I2C
> > + i2c_init_all();
>
> As mentioned before, I think this is a design problem.
>
> You must not gloablly initialize all I2C busses / adapters.
>
> You are only permitted to initialize those devices that are actually
> needed by U-Boot itself to perform certain operations, and you are
> supposed to deactivate these after use.
What is wrong with activating ALL i2c adapters? Is there any viable reason
for this?
Then, please point me to a single board that deactivates an I2C adapter. And
please tell me what function one can use to do that. I haven't seen anything
like i2c_deinit() or i2c_disable().
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 2/12 Multiadapter/multibus I2C, common part 2
On Mon, 16 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > ... > > diff -purN u-boot-i2c.orig/lib_arm/board.c u-boot-i2c/lib_arm/board.c > > --- u-boot-i2c.orig/lib_arm/board.c 2009-02-12 10:43:41.0 -0800 > > +++ u-boot-i2c/lib_arm/board.c 2009-02-12 10:46:00.0 -0800 > > @@ -81,7 +81,8 @@ extern void rtl8019_get_enetaddr (uchar > > #endif > > > > #if defined(CONFIG_HARD_I2C) || \ > > -defined(CONFIG_SOFT_I2C) > > +defined(CONFIG_SOFT_I2C) || \ > > +defined(CONFIG_SYS_I2C_ADAPTERS) > > It seems you define a set of new CONFIG_SYS_ options, but I did not > see any respective updates to the README file. PLease add. I will definitely make a new README file. But it takes time and not everything can be done at once. This thing is big and I'm not gonna spend all my time making huge patches that are not going to make it in the main tree. Let's eat an elephant in chunks. When the core is accepted I can make the rest. It is just waste of time otherwise. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 1/12 Multiadapter/multibus I2C, common part 1
On Mon, 16 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > Signed-off-by: Sergey Kubushyn > > --- > > diff -purN u-boot-i2c.orig/common/cmd_date.c u-boot-i2c/common/cmd_date.c > > --- u-boot-i2c.orig/common/cmd_date.c 2009-02-12 10:43:41.0 > > -0800 > > +++ u-boot-i2c/common/cmd_date.c2009-02-12 10:46:00.0 -0800 > > @@ -46,8 +46,13 @@ int do_date (cmd_tbl_t *cmdtp, int flag, > > int old_bus; > > One more global comment: can you please use git and especially > git-format-patch to prepare your patches? For example, it is really > helpful to see at a glance which files are affected by a patch. OK, I will. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 1/12 Multiadapter/multibus I2C, common part 1
On Mon, 16 Feb 2009, Wolfgang Denk wrote:
> Dear k...@koi8.net,
>
> In message you wrote:
> > Signed-off-by: Sergey Kubushyn
> > ---
> > diff -purN u-boot-i2c.orig/common/cmd_date.c u-boot-i2c/common/cmd_date.c
> > --- u-boot-i2c.orig/common/cmd_date.c 2009-02-12 10:43:41.0
> > -0800
> > +++ u-boot-i2c/common/cmd_date.c2009-02-12 10:46:00.0 -0800
> > @@ -46,8 +46,13 @@ int do_date (cmd_tbl_t *cmdtp, int flag,
> > int old_bus;
> >
> > /* switch to correct I2C bus */
> > +#ifdef CONFIG_NEW_I2C
> > + old_bus = i2c_get_bus_num();
> > + i2c_set_bus_num(CONFIG_SYS_RTC_BUS_NUM);
> > +#else
> > old_bus = I2C_GET_BUS();
> > I2C_SET_BUS(CONFIG_SYS_RTC_BUS_NUM);
> > +#endif
> >
> > switch (argc) {
> > case 2: /* set date & time */
> > @@ -94,7 +99,11 @@ int do_date (cmd_tbl_t *cmdtp, int flag,
> > }
> >
> > /* switch back to original I2C bus */
> > +#ifdef CONFIG_NEW_I2C
> > + i2c_set_bus_num(old_bus);
> > +#else
> > I2C_SET_BUS(old_bus);
> > +#endif
>
> Just a global note:
>
> This makes no sense to me. If we assume that the new code works, then
> it will replace the old code. These #ifdef's make no sense.
>
> We will probably hold this code in some testing branch (for a longer
> period of time to allow for extensive testing), but I don;t see the
> need to support both the old and the new code at the same time. This
> makes the code and your patches only more difficult to read.
I totally agree with you. That was done before a separate branch has been
created so I tried to make something working in the main tree. The idea was
make everything with that CONFIG_NEW_I2C first, one thing at a time and
then, when everything was converted to the new format, remove all those
stubs with a single patch.
You are totally right, it is not needed any more. I will rebase those
patches and get rid of all old (legacy) code.
Ok, it's getting late here in Vegas so I will reply the rest of postings
tomorrow when I'm back in the office.
I'm glad that there is interest in getting that I2C thing done...
---
**
* k...@homeKOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
**
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 0/12 Multiadapter/multibus I2C
On Mon, 16 Feb 2009, Wolfgang Denk wrote: > Dear Heiko, > > In message <49992beb.30...@denx.de> you wrote: > > > > >> Yes, thats a point. But do we need this before running from ram (except > > >> one hardwareadapter)? > > > > > > Yes, see above. > > > > Yes, thats is a problem in my approach, and if we need more then one > > i2c bus when running from flash, maybe a no go. > > It is not an unsolvable problem. We do have the global data structure, > and it has been (mis-) used for worse purposes before. Not so fast... And remember, there are other similar interfaces that are waiting in the line (e.g. SPI...) I personally do not think we should use that global data structure for such obscure purposes. And it must be writable for this to work. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 0/12 Multiadapter/multibus I2C
On Mon, 16 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > Yep. But nobody's perfect and you can have a situation when you need to > > access several busses before relocation. It is not hardware for U-Boot, it > > is U-Boot for hardware. When hardware designers design their hardware they > > don't make their decisions based on U-Boot limitation. That is us who should > > accomodate what they designed. > > We don't have to make the mainline U-Boot implementation unnecessary > complex just for the small chance that there is sombeody stupid > enough to design a broken system. > > It is our task as software engineers to tell the hardware designers > which designs are easy to support, nd which will cause problems in > the software. It is not complex. Quite in the contrary, it is much simpler and more straightforward to have 2 sets of functions for 2 adapters in one place than to have one that includes both of them in one place switchable using some global variable at another place used by wrappers in a third place. > > > There is also another consideration -- when having several adapters which > > one should be initialized at boot time, before relocation? Another problem > > The one that is needed there, if any at all. You will have to pick that one for each and every board somehow... > > is init() function that can be unique for each adapter. To make the lower > > layer transparent I'm reprogramming muxes if any when switching busses. It > > is necessary to make I2C API simple and uniform between muxed and non-muxed > > busses. That essentially means that we can NOT do i2c_set_bus_num() to > > execute init() for a particular adapter -- adapter MUST be initialized for > > i2c_set_bus_num() to succeed. > > > > Your suggestion requires total LOGIC change. > > Maybe. But that's why we're discussing this here. > > > > Is this needed? If so, you must before call a i2c_set_bus_num(), and after > > > you finished call it again with the old busnumber. So it is done for > > > example > > > in do_date () common/cmd_date.c > > > > You can not do it before all adapters are initialized. And you WON'T be able > > to initialize adapters because you will not be able to switch busses. > > This sounds like a design problem to me, then. > > Please keep in mind that according to U-Boot philosophy it is > forbidden to always initialize all adapters. Only those actually used > by U-Boot may be initialized, and shall be de-initialized after use. Eh, I don't see any problem initializing all I2C controllers at the same time. What is the problem with that? Then, we _DO_ already initialize _ALL_ controllers in U-Boot. Most of the time the total number of controllers is 1 and we do initialize all of them. In those rare cases when that number is not 1, we _DO_ initialize all of them (look at fsl_i2c.c as it is in the main tree right now.) Existing i2c_init() function does not have any provision for initializing any particular controller; it initializes them all. > > > Yes, thats a point. But do we need this before running from ram (except > > > one hardwareadapter)? > > > > Yes, see above. > > Um... Maybe I missed something - did you give an example (except for > broken designs) where this really might be needed? We did not have such boards as of now but that does not mean they can't exist. I would've agreed for making a handicapped version if it had been difficult to make a full-featured one but it is not. Furthermore, additional efforts required to put such a handicap on existing code. > > > Yes, I know. But again, do we need this? > > > > We do. Otherwise we can essentially throw everything to trash and start > > over. This requires changing the logical design, architecture. And this is > > that logic that is most difficult and takes most thinking. Coding is easy. > > You say: "we do [need that]". I ask: why? what for? > > > Another reason why macros are used is speed. Not everyone is running U-Boot > > on 10 GHz Pentium-9 with gigabyte of cache. In bitbanged I2C every > > instruction counts if you want to run a bus at a decent speed (I won't even > > start with regular 100kHz less for 400kHz; 50kHz would be very good.) Your > > Are you sure? If I remember correctly soft-I2C can even run 400 KHz on > a slow 50 MHz MPC8xx system. > > Do you have other numbers? Is something wrong with those numbers? > > must be executed. And remember, there is probably no instruction cache and > > you're running off of flash so every instruction fetch is something like > > I tend to say that an U-Boot port where instruction cache is disabled > is misconfigured and should be fixed :-) Eh, everything is easy when you have your RAM running and all handicaps dropped off... Just try to run 100 meters with heavy weights on your legs and you will see you can not compete with sprinters :) --- ** * k...@hom
Re: [U-Boot] [PATCH] 0/12 Multiadapter/multibus I2C
On Mon, 16 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > OK, please explain how that cur_adap_nr->hwadapnr gets assigned. Please also > > explain how can one invoke a function on other adapter than "current". > > Remember, i2c_init is quite often called BEFORE the code is relocated to RAM > > so you can NOT change "current" adapter. > > We could assign an entry in the global data for it. > > But then - how often will it bbe necessary to switch adapters before > relocation? What is I2C being used for? To read the SPD data for the > RAM init code. Which other adapter would be needed? I can not tell it right away. There might be several RAM DIMMs with their own SPD EPROMS sitting on different busses or something else. It is not like we absolutely need this feature right now, urgently but it is so easy to implement that it would've been a crime to not to... :) There is another problem with choosing a proper i2c_init() function with i2c_set_bus_num() -- the latter also reprograms I2C multiplexers/switches if there are any so you essentially need an initialized adapter to switch to it to initialize it. Catch 22... > > Please also note that you will loose a capability of working with more than > > one adapter before the code is relocated to RAM. > > I don't see this. Why? There is no writable global variables before relocation... --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 0/12 Multiadapter/multibus I2C
On Mon, 16 Feb 2009, Wolfgang Denk wrote: > Dear k...@koi8.net, > > In message you wrote: > > > > > Can you please send your patches with some better commit messages. > > > You only send your Signed-off-by, without any explanation. Please > > > change this. > > > > There is not much sense in extensive commit messages in this case, IMHO. It > > is not a bug fix or added feature at one particular place; it is a major > > rework. The only message I can give is something like "Changes for > > multiadapter/multibus I2C support..." > > This sounds very much as if there was a danger that the patches are > not logically cut - did you make sure that eahc of the commits is > still bisectable? I will rebase them and slice 'em in smaller pieces. Unfortunately this is a major rework so there is no way one can make small patches really independent. Changes to include/i2c.h, e.g. make the entire old code uncompilable so they only make sence together with some other parts that constitute the core of the new code. Everything else is much easier when the core is in. --- ** * k...@homeKOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ** ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] 0/12 Multiadapter/multibus I2C
On Sun, 15 Feb 2009, Heiko Schocher wrote: > Hello ksi, > > k...@koi8.net wrote: > > On Sat, 14 Feb 2009, Heiko Schocher wrote: > > > > > >> Hello ksi, > >> > >> k...@koi8.net wrote: > >> > >>> On Fri, 13 Feb 2009, Heiko Schocher wrote: > >>> > >>>> k...@koi8.net wrote: > >>>> > >>>>> Here is the second attempt for initial portion of multibus/multiadapter > >>>>> I2C support. > >>>>> > >>>>> > >>>> Can you please send your patches with some better commit messages. > >>>> You only send your Signed-off-by, without any explanation. Please > >>>> change this. > >>>> > >>> There is not much sense in extensive commit messages in this case, IMHO. > >>> It > >>> is not a bug fix or added feature at one particular place; it is a major > >>> rework. The only message I can give is something like "Changes for > >>> multiadapter/multibus I2C support..." > >>> > >>> I'll add it to the second attempt that I will make later today. > >>> > >>> > >>>>> This includes a set of common files, all drivers in drivers/i2c and all > >>>>> boards affected by these changes (config files, board files, and lib_xx > >>>>> files.) > >>>>> > >>>>> There is an illustrative example of multiadapter multibus I2C config in > >>>>> MPC8548CDS.h config file (#if 0'd.) Definitions in that example are > >>>>> bogus so please don't expect it to work. It will compile though... > >>>>> > >>>>> This set also includes big rework for soft_i2c.c that makes it template > >>>>> version that allows up to 4 bitbanged adapters. This number can be > >>>>> > >>>>> > >>>> Didn;t you try my suggestion? This is a really big define monster now, > >>>> which I think, we can avoid, and without to change nearly all lines of > >>>> the existing driver. > >>>> > >>> We can not avoid it. At least I can not see an easy way to do this. > >>> SOFT_I2C > >>> > >> Yes, we can. Look again deeper in my approach, this _is_ an easy way! > >> > >> I also looked again in your changes in the fsl_i2c.c driver, and we > >> can make this also simplier, by using cur_adap_nr->hwadapnr! > >> > > > > OK, please explain how that cur_adap_nr->hwadapnr gets assigned. Please also > > > > When running from ram, this is no problem. It should be set in > i2c_set_bus_num(). Yep. But nobody's perfect and you can have a situation when you need to access several busses before relocation. It is not hardware for U-Boot, it is U-Boot for hardware. When hardware designers design their hardware they don't make their decisions based on U-Boot limitation. That is us who should accomodate what they designed. There is also another consideration -- when having several adapters which one should be initialized at boot time, before relocation? Another problem is init() function that can be unique for each adapter. To make the lower layer transparent I'm reprogramming muxes if any when switching busses. It is necessary to make I2C API simple and uniform between muxed and non-muxed busses. That essentially means that we can NOT do i2c_set_bus_num() to execute init() for a particular adapter -- adapter MUST be initialized for i2c_set_bus_num() to succeed. Your suggestion requires total LOGIC change. > > explain how can one invoke a function on other adapter than "current". > > > > Is this needed? If so, you must before call a i2c_set_bus_num(), and after > you finished call it again with the old busnumber. So it is done for example > in do_date () common/cmd_date.c You can not do it before all adapters are initialized. And you WON'T be able to initialize adapters because you will not be able to switch busses. > > Remember, i2c_init is quite often called BEFORE the code is relocated to RAM > > so you can NOT change "current" adapter. > > > > Yes, thats a point. But do we need this before running from ram (except > one hardwareadapter)? Yes, see above. > > Please also note that you will loose a capability of working with more than > > one adapter before the code is relocated to RAM. > > > > Do we really need this? Eh, "640K o
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Sun, 15 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> k...@koi8.net wrote:
> > On Sat, 14 Feb 2009, Heiko Schocher wrote:
> >
> >
> >> Hello ksi,
> >>
> >> k...@koi8.net wrote:
> >>
> >>> On Fri, 13 Feb 2009, Heiko Schocher wrote:
> >>>
> >>>> k...@koi8.net wrote:
> >>>>
> >>>>> Signed-off-by: Sergey Kubushyn
> >>>>> ---
> >>>>> diff -purN u-boot-i2c.orig/drivers/i2c/soft_i2c.c
> >>>>> u-boot-i2c/drivers/i2c/soft_i2c.c
> >>>>> --- u-boot-i2c.orig/drivers/i2c/soft_i2c.c 2009-02-12
> >>>>> 10:43:41.0 -0800
> >>>>> +++ u-boot-i2c/drivers/i2c/soft_i2c.c 2009-02-12 10:46:00.0
> >>>>> -0800
> >>>>> @@ -1,4 +1,8 @@
> >>>>> /*
> >>>>> + * Copyright (c) 2009 Sergey Kubushyn
> >>>>> + *
> >>>>> + * Changes for multibus/multiadapter I2C support.
> >>>>> + *
> >>>>> * (C) Copyright 2001, 2002
> >>>>> * Wolfgang Denk, DENX Software Engineering, w...@denx.de.
> >>>>>
> >>>> [...]
> >>>>
> >>>> The following patch is based on your patches without 7/12 and
> >>>> adds multibus support for the soft_i2c driver without doing such
> >>>> a big change as you did. Maybe it is not yet perfect, because
> >>>> it is just a fast try, but I think we should go this way. What
> >>>> do you/others think?
> >>>>
> >>> The reason behind this patch is making SEVERAL different SOFT_I2C ADAPTERS
> >>> available. Not BUSSES but separate PHYSICAL I2C ADAPTERS made of different
> >>> pin pairs from different chips.
> >>>
> >> This you can also do with "my" suggestion ...
> >>
> >>
> >>> OK, please explain how are you going to make different functions for
> >>> different adapters? Let's say you want to use 2 on-SoC GPIO pins for
> >>>
> >> You can do now the following for example in your
> >> include/configs/MPC8548CDS.h example:
> >>
> >> you only have to define
> >>
> >> #define I2C_SDA(bit) (printf("hwadap: %d sda1: %d",
> >> cur_adap_nr->hwadapnr, bit))
> >>
> >> if this is a real driver you can make a function in your board code
> >> say (just a fast thought):
> >>
> >> void i2c_soft_sda (int bit)
> >> {
> >>switch(cur_adap_nr->hwadapnr) {
> >>case 0:
> >>/* adapter specfic code 0 */
> >>break;
> >>case 1:
> >>/* adapter specfic code 1 */
> >>break;
> >>[...]
> >>}
> >> }
> >>
> >> and define in config file
> >>
> >> #define I2C_SDA(bit) i2c_soft_sda (bit)
> >>
> >
> > That means you have to make changes in two places instead of one -- config
> > file AND $(BOARD).c. Also you use functions instead of macros and you can
> >
>
> Yes. But that was just a thought, it should be possible to do this also
> in macros.
Not if you want to make functions in $(BOARD).c.
> > NOT make them inline because they come from a separate object file. This
> > essentially defeats the very purpose of that common soft_i2c.c driver. If
> > you want to make functions for bitbanged I2C into the $(BOARD).c there is no
> > reason to have them as a base for that driver. It is much more logical to do
> >
>
> Maybe more logical, but not needed.
>
> > everything in reverse, i.e. instead of having soft_i2c.c as a bona fide
> > drivers and those I2C_SDA and friends as its building blocks make those
> > i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and
> > build the actual driver in the $(BOARD).c itself. Just convert that
> > soft_i2c.c into a header file with macros for real functions (soft_i2c_read
> > etc.) and instantiate them in the $(BOARD).c.
> >
> > The only problem with that is it breaks uniformity and makes another mess.
> >
>
> Just, if we do this, but we don;t need to do it so.
>
> > The whole idea was to bring _ALL_ I2C drivers to a single place and make
> > them totally transparent and uniform. Something like e.g. Linux VFS
Re: [U-Boot] [PATCH] 0/12 Multiadapter/multibus I2C
On Sat, 14 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> k...@koi8.net wrote:
> > On Fri, 13 Feb 2009, Heiko Schocher wrote:
> >> k...@koi8.net wrote:
> >>> Here is the second attempt for initial portion of multibus/multiadapter
> >>> I2C support.
> >>>
> >> Can you please send your patches with some better commit messages.
> >> You only send your Signed-off-by, without any explanation. Please
> >> change this.
> >
> > There is not much sense in extensive commit messages in this case, IMHO. It
> > is not a bug fix or added feature at one particular place; it is a major
> > rework. The only message I can give is something like "Changes for
> > multiadapter/multibus I2C support..."
> >
> > I'll add it to the second attempt that I will make later today.
> >
> >>> This includes a set of common files, all drivers in drivers/i2c and all
> >>> boards affected by these changes (config files, board files, and lib_xx
> >>> files.)
> >>>
> >>> There is an illustrative example of multiadapter multibus I2C config in
> >>> MPC8548CDS.h config file (#if 0'd.) Definitions in that example are
> >>> bogus so please don't expect it to work. It will compile though...
> >>>
> >>> This set also includes big rework for soft_i2c.c that makes it template
> >>> version that allows up to 4 bitbanged adapters. This number can be
> >>>
> >> Didn;t you try my suggestion? This is a really big define monster now,
> >> which I think, we can avoid, and without to change nearly all lines of
> >> the existing driver.
> >
> > We can not avoid it. At least I can not see an easy way to do this. SOFT_I2C
>
> Yes, we can. Look again deeper in my approach, this _is_ an easy way!
>
> I also looked again in your changes in the fsl_i2c.c driver, and we
> can make this also simplier, by using cur_adap_nr->hwadapnr!
OK, please explain how that cur_adap_nr->hwadapnr gets assigned. Please also
explain how can one invoke a function on other adapter than "current".
Remember, i2c_init is quite often called BEFORE the code is relocated to RAM
so you can NOT change "current" adapter.
Please also note that you will loose a capability of working with more than
one adapter before the code is relocated to RAM.
> We have not to define for both hardware adapter each function in
> i2c_adap_t! For example:
>
> You did:
> static void fsl_i2c1_init(int speed, int slaveadd)
> {
> __i2c_init(0, speed, slaveadd);
> }
>
> instead we only need
>
> i2c_init(cur_adap_nr->hwadapnr, speed, slaveadd);
>
> with
>
> i2c_adap_tfsl_i2c_adap[] = {
> {
> .init = i2c_init,
> [...]
> .hwadapnr = 0,
> .name = FSL_NAME(CONFIG_SYS_FSL_I2C_OFFSET)
> },
> #ifdef CONFIG_SYS_FSL_I2C2_OFFSET
> {
> .init = i2c_init,
> [...]
> .hwadapnr = 1,
> .name = FSL_NAME(CONFIG_SYS_FSL_I2C2_OFFSET)
> },
> #endif
It would've been easy if we had had "this" pointer. That would allow us to
find out what adapter we are running on by using something like
"this->hwadapnr." Unfortunately we do NOT have such a pointer, we're plain
C. Function in a structure does not have a way to find out how to access a
member of that structure. The only way to somehow find which "hwadapnr" we
are running at is using a global variable, cur_i2c_bus as a starting point.
But that is meaningless until the code is relocated to RAM and that variable
became writable. And that robs us of added possibility of using any adapter
other than a single one preset in config file before relocating to RAM.
That is if we want to keep the original I2C API. The other, simpler way is
to add an argument to each and every function, a pointer to i2c_adap_t
structure or its index or something similar. But that defeats the entire
purpose of this code by requiring to find and change each and every call to
any I2C function in the entire U-Boot source thus totally breaking ALL
existing code 99.99% of which only use single I2C adapter/bus...
> Please change this driver also!
I can't. Please read above.
> If I think more, we never even need to change the function parameters
> like you did for example for i2c_int ()! We can use at the beginning
> of every function who go in i2c_adap_t, the "cur_adap_nr->hwadapnr"
> and make the settings we need for this function... and wow we saved
> one function p
Re: [U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
On Sat, 14 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> k...@koi8.net wrote:
> > On Fri, 13 Feb 2009, Heiko Schocher wrote:
> >> k...@koi8.net wrote:
> >>> Signed-off-by: Sergey Kubushyn
> >>> ---
> >>> diff -purN u-boot-i2c.orig/drivers/i2c/soft_i2c.c
> >>> u-boot-i2c/drivers/i2c/soft_i2c.c
> >>> --- u-boot-i2c.orig/drivers/i2c/soft_i2c.c2009-02-12
> >>> 10:43:41.0 -0800
> >>> +++ u-boot-i2c/drivers/i2c/soft_i2c.c 2009-02-12 10:46:00.0
> >>> -0800
> >>> @@ -1,4 +1,8 @@
> >>> /*
> >>> + * Copyright (c) 2009 Sergey Kubushyn
> >>> + *
> >>> + * Changes for multibus/multiadapter I2C support.
> >>> + *
> >>> * (C) Copyright 2001, 2002
> >>> * Wolfgang Denk, DENX Software Engineering, w...@denx.de.
> >> [...]
> >>
> >> The following patch is based on your patches without 7/12 and
> >> adds multibus support for the soft_i2c driver without doing such
> >> a big change as you did. Maybe it is not yet perfect, because
> >> it is just a fast try, but I think we should go this way. What
> >> do you/others think?
> >
> > The reason behind this patch is making SEVERAL different SOFT_I2C ADAPTERS
> > available. Not BUSSES but separate PHYSICAL I2C ADAPTERS made of different
> > pin pairs from different chips.
>
> This you can also do with "my" suggestion ...
>
> > OK, please explain how are you going to make different functions for
> > different adapters? Let's say you want to use 2 on-SoC GPIO pins for
>
> You can do now the following for example in your
> include/configs/MPC8548CDS.h example:
>
> you only have to define
>
> #define I2C_SDA(bit) (printf("hwadap: %d sda1: %d", cur_adap_nr->hwadapnr,
> bit))
>
> if this is a real driver you can make a function in your board code
> say (just a fast thought):
>
> void i2c_soft_sda (int bit)
> {
> switch(cur_adap_nr->hwadapnr) {
> case 0:
> /* adapter specfic code 0 */
> break;
> case 1:
> /* adapter specfic code 1 */
> break;
> [...]
> }
> }
>
> and define in config file
>
> #define I2C_SDA(bit) i2c_soft_sda (bit)
That means you have to make changes in two places instead of one -- config
file AND $(BOARD).c. Also you use functions instead of macros and you can
NOT make them inline because they come from a separate object file. This
essentially defeats the very purpose of that common soft_i2c.c driver. If
you want to make functions for bitbanged I2C into the $(BOARD).c there is no
reason to have them as a base for that driver. It is much more logical to do
everything in reverse, i.e. instead of having soft_i2c.c as a bona fide
drivers and those I2C_SDA and friends as its building blocks make those
i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and
build the actual driver in the $(BOARD).c itself. Just convert that
soft_i2c.c into a header file with macros for real functions (soft_i2c_read
etc.) and instantiate them in the $(BOARD).c.
The only problem with that is it breaks uniformity and makes another mess.
The whole idea was to bring _ALL_ I2C drivers to a single place and make
them totally transparent and uniform. Something like e.g. Linux VFS.
And remember, the devil is in details. How are you going to assign
(initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you
going to work on an adapter other that "current" in a situation when you can
NOT change "current" adapter (e.g. perform all I2C layer initialization
while still running from flash?) Remember, this is plain C and there is no
"this" pointer... And that is just a tip of an iceberg...
And the million dollar question -- what is the potential gain?
> > adapter #0, 2 GPIOs from a PCI-PCI bridge for adapter #1, and 2 pins from
> > some chip sitting behind that bridge for adapter #2 if all those pin sets
> > are accessed totally different. I won't even start about using pins from
> > different chips for SDA and SCL (let's say you only have one GPIO available
> > on your SoC and another one on PCI Bridge.)
> >
> > What your patch creates is just aliases to the SAME physical adapter.
>
> No, it is not! I only use the same functions, but in the board
> specific code it is possible to made a switch and access
> the Pins where ever they are.
You are adding unnecessary complexity to the code. And you break unif
[U-Boot] [PATCH] Multiadapter/multibus I2C, at91rm9200 and friends
Initial multiadapter/multibus I2C support.
Atmel AT91RM9200 based boards.
This goes on top of previous patches.
Signed-off-by: Sergey Kubushyn
---
diff -purN u-boot-i2c.ORIG/board/cmc_pu2/load_sernum_ethaddr.c
u-boot-i2c/board/cmc_pu2/load_sernum_ethaddr.c
--- u-boot-i2c.ORIG/board/cmc_pu2/load_sernum_ethaddr.c 2009-02-12
10:43:40.0 -0800
+++ u-boot-i2c/board/cmc_pu2/load_sernum_ethaddr.c 2009-02-13
16:29:00.0 -0800
@@ -75,7 +75,7 @@ void load_sernum_ethaddr (void)
unsigned char *p;
unsigned short i, is, id;
-#if !defined(CONFIG_HARD_I2C) && !defined(CONFIG_SOFT_I2C)
+#if !defined(CONFIG_HAS_I2C)
#error you must define some I2C support (CONFIG_HARD_I2C or CONFIG_SOFT_I2C)
#endif
if (i2c_read(I2C_CHIP, I2C_OFFSET, I2C_ALEN, (unsigned char *)&data,
diff -purN u-boot-i2c.ORIG/cpu/arm920t/at91rm9200/i2c.c
u-boot-i2c/cpu/arm920t/at91rm9200/i2c.c
--- u-boot-i2c.ORIG/cpu/arm920t/at91rm9200/i2c.c2009-02-12
10:43:41.0 -0800
+++ u-boot-i2c/cpu/arm920t/at91rm9200/i2c.c 1969-12-31 16:00:00.0
-0800
@@ -1,202 +0,0 @@
-/*
- * i2c Support for Atmel's AT91RM9200 Two-Wire Interface
- *
- * (c) Rick Bronson
- *
- * Borrowed heavily from original work by:
- * Copyright (c) 2000 Philip Edelbrock
- *
- * Modified to work with u-boot by (C) 2004 Gary Jennejohn ga...@denx.de
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
- *
-*/
-#include
-
-#ifdef CONFIG_HARD_I2C
-
-#include
-#include
-#include
-
-#include
-
-/* define DEBUG */
-
-/*
- * Poll the i2c status register until the specified bit is set.
- * Returns 0 if timed out (100 msec)
- */
-static short at91_poll_status(AT91PS_TWI twi, unsigned long bit) {
- int loop_cntr = 1;
- do {
- udelay(10);
- } while (!(twi->TWI_SR & bit) && (--loop_cntr > 0));
-
- return (loop_cntr > 0);
-}
-
-/*
- * Generic i2c master transfer entrypoint
- *
- * rw == 1 means that this is a read
- */
-static int
-at91_xfer(unsigned char chip, unsigned int addr, int alen,
- unsigned char *buffer, int len,
int rw)
-{
- AT91PS_TWI twi = (AT91PS_TWI) AT91_TWI_BASE;
- int length;
- unsigned char *buf;
- /* Set the TWI Master Mode Register */
- twi->TWI_MMR = (chip << 16) | (alen << 8)
- | ((rw == 1) ? AT91C_TWI_MREAD : 0);
-
- /* Set TWI Internal Address Register with first messages data field */
- if (alen > 0)
- twi->TWI_IADR = addr;
-
- length = len;
- buf = buffer;
- if (length && buf) {/* sanity check */
- if (rw) {
- twi->TWI_CR = AT91C_TWI_START;
- while (length--) {
- if (!length)
- twi->TWI_CR = AT91C_TWI_STOP;
- /* Wait until transfer is finished */
- if (!at91_poll_status(twi, AT91C_TWI_RXRDY)) {
- debug ("at91_i2c: timeout 1\n");
- return 1;
- }
- *buf++ = twi->TWI_RHR;
- }
- if (!at91_poll_status(twi, AT91C_TWI_TXCOMP)) {
- debug ("at91_i2c: timeout 2\n");
- return 1;
- }
- } else {
- twi->TWI_CR = AT91C_TWI_START;
- while (length--) {
- twi->TWI_THR = *buf++;
- if (!length)
- twi->TWI_CR = AT91C_TWI_STOP;
- if (!at91_poll_status(twi, AT91C_TWI_TXRDY)) {
- debug ("at91_i2c: timeout 3\n");
- return 1;
- }
- }
- /* Wait until transfer is finished */
- if (!at91_poll_status(twi, AT91C_TWI_TXCOMP)) {
- debug ("at91_i2c: timeout 4\n");
- return 1;
- }
- }
- }
- retu
[U-Boot] [PATCH] Multiadapter/multibus I2C, i2c.h fix
Add missing #endif in include/i2c.h Signed-off-by: Sergey Kubushyn --- diff -purN u-boot-i2c.ORIG/include/i2c.h u-boot-i2c/include/i2c.h --- u-boot-i2c.ORIG/include/i2c.h 2009-02-13 16:34:36.0 -0800 +++ u-boot-i2c/include/i2c.h2009-02-13 16:25:52.0 -0800 @@ -226,6 +226,7 @@ void i2c_reloc_fixup(void); # else # define I2C_SOFT_DEFS # endif +#endif /* * Initialization, must be called once on start up, may be called ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] 9/12 Multiadapter/multibus I2C, configs part 2, fixed
Initial multiadapter/multibus I2C support.
Signed-off-by: Sergey Kubushyn
---
diff -purN u-boot-i2c.orig/include/configs/MHPC.h
u-boot-i2c/include/configs/MHPC.h
--- u-boot-i2c.orig/include/configs/MHPC.h 2009-02-12 10:43:41.0
-0800
+++ u-boot-i2c/include/configs/MHPC.h 2009-02-12 10:46:00.0 -0800
@@ -72,7 +72,12 @@
/* enable I2C and select the hardware/software driver */
#undef CONFIG_HARD_I2C /* I2C with hardware support*/
-#define CONFIG_SOFT_I2C1 /* I2C bit-banged
*/
+#define CONFIG_NEW_I2C
+#define CONFIG_SOFT_I2C/* I2C bit-banged */
+#define I2C_SOFT_DECLARATIONS I2C_SOFT_DEFS
+#define CONFIG_SYS_SOFT_I2C_SPEED 5
+#define CONFIG_SYS_SOFT_I2C_SLAVE 0xFE
+#define CONFIG_SYS_I2C_ADAPTERS{&soft_i2c_adap[0]}
/*
* Software (bit-bang) I2C driver configuration
*/
@@ -89,8 +94,6 @@
elseimmr->im_cpm.cp_pbdat &= ~PB_SCL
#define I2C_DELAY udelay(5) /* 1/4 I2C clock duration */
-#define CONFIG_SYS_I2C_SPEED 5
-#define CONFIG_SYS_I2C_SLAVE 0xFE
#define CONFIG_SYS_I2C_EEPROM_ADDR 0x50/* EEPROM X24C04
*/
#define CONFIG_SYS_I2C_EEPROM_ADDR_LEN 1 /* bytes of address
*/
/* mask of address bits that overflow into the "EEPROM chip address" */
diff -purN u-boot-i2c.orig/include/configs/MPC8313ERDB.h
u-boot-i2c/include/configs/MPC8313ERDB.h
--- u-boot-i2c.orig/include/configs/MPC8313ERDB.h 2009-02-12
10:43:41.0 -0800
+++ u-boot-i2c/include/configs/MPC8313ERDB.h2009-02-12 10:46:00.0
-0800
@@ -318,15 +318,17 @@
#define CONFIG_SYS_PROMPT_HUSH_PS2 "> "
/* I2C */
-#define CONFIG_HARD_I2C/* I2C with hardware support*/
+#define CONFIG_NEW_I2C
+#define CONFIG_SYS_NUM_I2C_ADAPTERS2
#define CONFIG_FSL_I2C
-#define CONFIG_I2C_MULTI_BUS
-#define CONFIG_I2C_CMD_TREE
-#define CONFIG_SYS_I2C_SPEED 40 /* I2C speed and slave address
*/
-#define CONFIG_SYS_I2C_SLAVE 0x7F
-#define CONFIG_SYS_I2C_NOPROBES{{0,0x69}} /* Don't probe these addrs */
-#define CONFIG_SYS_I2C_OFFSET 0x3000
-#define CONFIG_SYS_I2C2_OFFSET 0x3100
+#define CONFIG_SYS_FSL_I2C_SPEED 40 /* I2C speed and slave address
*/
+#define CONFIG_SYS_FSL_I2C_SLAVE 0x7F
+#define CONFIG_SYS_FSL_I2C_OFFSET 0x3000
+#define CONFIG_SYS_FSL_I2C2_SPEED 40 /* I2C speed and slave address
*/
+#define CONFIG_SYS_FSL_I2C2_SLAVE 0x7F
+#define CONFIG_SYS_FSL_I2C2_OFFSET 0x3100
+#define CONFIG_SYS_I2C_ADAPTERS{&fsl_i2c_adap[0],
&fsl_i2c_adap[1]}
+#define CONFIG_SYS_I2C_NOPROBES{{0,0x69}} /* Don't probe these
addrs */
/*
* General PCI
diff -purN u-boot-i2c.orig/include/configs/MPC8315ERDB.h
u-boot-i2c/include/configs/MPC8315ERDB.h
--- u-boot-i2c.orig/include/configs/MPC8315ERDB.h 2009-02-12
10:43:41.0 -0800
+++ u-boot-i2c/include/configs/MPC8315ERDB.h2009-02-12 10:46:00.0
-0800
@@ -272,13 +272,13 @@
#define CONFIG_OF_STDOUT_VIA_ALIAS 1
/* I2C */
-#define CONFIG_HARD_I2C/* I2C with hardware support */
+#define CONFIG_NEW_I2C
#define CONFIG_FSL_I2C
-#define CONFIG_SYS_I2C_SPEED 40 /* I2C speed and slave address */
-#define CONFIG_SYS_I2C_SLAVE 0x7F
-#define CONFIG_SYS_I2C_NOPROBES{0x51} /* Don't probe these addrs */
-#define CONFIG_SYS_I2C_OFFSET 0x3000
-#define CONFIG_SYS_I2C2_OFFSET 0x3100
+#define CONFIG_SYS_FSL_I2C_SPEED 40 /* I2C speed and slave address
*/
+#define CONFIG_SYS_FSL_I2C_SLAVE 0x7F
+#define CONFIG_SYS_FSL_I2C_OFFSET 0x3000
+#define CONFIG_SYS_I2C_ADAPTERS{&fsl_i2c_adap[0]}
+#define CONFIG_SYS_I2C_NOPROBES{0x51} /* Don't probe these
addrs */
/*
* Board info - revision and where boot from
diff -purN u-boot-i2c.orig/include/configs/MPC8323ERDB.h
u-boot-i2c/include/configs/MPC8323ERDB.h
--- u-boot-i2c.orig/include/configs/MPC8323ERDB.h 2009-02-12
10:43:41.0 -0800
+++ u-boot-i2c/include/configs/MPC8323ERDB.h2009-02-12 10:46:00.0
-0800
@@ -313,13 +313,13 @@
#define CONFIG_OF_STDOUT_VIA_ALIAS 1
/* I2C */
-#define CONFIG_HARD_I2C/* I2C with hardware support */
-#undef CONFIG_SOFT_I2C /* I2C bit-banged */
+#define CONFIG_NEW_I2C
#define CONFIG_FSL_I2C
-#define CONFIG_SYS_I2C_SPEED 40 /* I2C speed and slave address */
-#define CONFIG_SYS_I2C_SLAVE 0x7F
-#define CONFIG_SYS_I2C_NOPROBES{0x51} /* Don't probe these addrs */
-#define CONFIG_SYS_I2C_OFFSET 0x3000
+#define CONFIG_SYS_FSL_I2C_SPEED 40 /* I2C speed and slave address
*/
+#define CONFIG_SYS_FSL_I2C_SLAVE 0x7F
+#define CONFIG_SYS_FSL_I2C_OFFSET 0x3000
+#define CONFIG_SYS_I2C_ADAPTERS
[U-Boot] [PATCH] 6/12 Multiadapter/multibus I2C, drivers part 3, fixed
Initial multiadapter/multibus I2C support.
Signed-off-by: Sergey Kubushyn
---
diff -purN u-boot-i2c.orig/drivers/i2c/sm502_i2c.c
u-boot-i2c/drivers/i2c/sm502_i2c.c
--- u-boot-i2c.orig/drivers/i2c/sm502_i2c.c 1969-12-31 16:00:00.0
-0800
+++ u-boot-i2c/drivers/i2c/sm502_i2c.c 2009-02-12 10:46:00.0 -0800
@@ -0,0 +1,340 @@
+/*
+ * Copyright (C) 2009 Sergey Kubushyn
+ *
+ * Driver for Silicon Motion SM501/SM502 I2C interface.
+ */
+
+#include
+
+#ifdef CONFIG_SM502_I2C
+
+#include
+#include
+
+#include
+#include
+
+/* I2C registers field definitions */
+/* */
+/* I2C_CONTROL (R/W) */
+#define SM501_I2C_ENABLE (1<<0)
+#define SM501_I2C_SPEED(1<<1)
+#define SM501_I2C_START(1<<2)
+#define SM501_I2C_IRQ_ENA (1<<4)
+#define SM501_I2C_IRQ_ACK (1<<5)
+#define SM501_I2C_RPT_START_ENA(1<<6)
+/* I2C_STATUS (R/O) */
+#define SM501_I2C_BUS_BUSY (1<<0)
+#define SM501_I2C_NACK (1<<1)
+#define SM501_I2C_BUS_ERROR(1<<2)
+#define SM501_I2C_XFER_DONE(1<<3)
+/* I2C_RESET (W/O) */
+#define SM501_I2C_CLEAR(1<<2)
+
+#defineSM501_I2C_MAX_COUNT 16
+
+#define SM501_I2C_READ 1
+#define SM501_I2C_WRITE0
+
+#define SM501_I2C_TIMEOUT (1<<7)
+
+#define SM501_CHECK_NACK() \
+ do {\
+ if (tmp & (SM501_I2C_NACK | SM501_I2C_BUS_ERROR)) {\
+ tmp = read_i2c_reg(SM501_I2C_CONTROL) &
SM501_I2C_SPEED;\
+ write_i2c_reg(SM501_I2C_CONTROL, tmp);\
+ return(1);\
+ }\
+ } while (0)
+
+
+i2c_adap_t sm501_i2c_adap;
+
+DECLARE_GLOBAL_DATA_PTR;
+
+
+static __inline__ u_int8_t read_i2c_reg(unsigned long offset)
+{
+ return(readb(sm501_iomem_base + SM501_I2C + offset));
+}
+
+static __inline__ void write_i2c_reg(unsigned long offset, u_int8_t data)
+{
+ writeb(data, sm501_iomem_base + SM501_I2C + offset);
+ __asm__("sync;isync;msync");
+#ifndef CONFIG_SYS_SM501_BASEADDR
+ /* Dummy read to push it through PCI */
+ readb(sm501_iomem_base + SM501_I2C + offset);
+#endif
+}
+
+static int poll_i2c_status(u_int8_t mask, int timeout)
+{
+ u_int8_tstat;
+ int i;
+
+ i = 0;
+
+ do {
+ stat = read_i2c_reg(SM501_I2C_STATUS);
+ if (stat & mask) {
+ return(stat);
+ }
+ udelay(1000);
+
+ } while (i++ < timeout);
+
+ return(stat | SM501_I2C_TIMEOUT);
+}
+
+static int wait_for_bus(void)
+{
+ int timeout = 10;
+
+ while ((read_i2c_reg(SM501_I2C_STATUS) & SM501_I2C_BUS_BUSY) &&
timeout--) {
+ udelay (1000);
+ }
+
+ if (timeout <= 0) {
+ printf ("SM501_I2C timed out in wait_for_bb: I2C_STAT=%x\n",
+ read_i2c_reg(SM501_I2C_STATUS));
+ return(1);
+ }
+
+ return(0);
+}
+
+static void sm501_i2c_init(int speed, int slaveadd)
+{
+ unsigned long tmpl;
+
+ /* Set GPIO pins for hardware I2C */
+ tmpl = readl(sm501_iomem_base + SM501_GPIO + SM501_GPIO_DATA_HIGH);
+ tmpl |= 0xc000;
+ writel(tmpl, sm501_iomem_base + SM501_GPIO + SM501_GPIO_DATA_HIGH);
+ __asm__("sync;isync;msync");
+#ifndef CONFIG_SYS_SM501_BASEADDR
+ /* Dummy read to push is through PCI */
+ readl(sm501_iomem_base + SM501_GPIO + SM501_GPIO_DATA_HIGH);
+#endif
+
+ /* SM501/502 only allows 100 or 400 KHz speed (standard or fast) */
+ if (speed <= 10)
+ write_i2c_reg(SM501_I2C_CONTROL, SM501_I2C_ENABLE);
+ else
+ write_i2c_reg(SM501_I2C_CONTROL, SM501_I2C_ENABLE |
SM501_I2C_SPEED);
+
+ if (gd->flags & GD_FLG_RELOC) {
+ sm501_i2c_adap.speed = speed <= 10 ? 10 : 40;
+ sm501_i2c_adap.init_done = 1;
+ }
+}
+
+
+static int sm501_i2c_probe(u_int8_t chip)
+{
+ u_int8_ttmp;
+
+ /* Fail if I2C address is invalid */
+ if (chip > 0x7f) {return(1);}
+
+ tmp = read_i2c_reg(SM501_I2C_CONTROL);
+ write_i2c_reg(SM501_I2C_CONTROL, tmp & ~(SM501_I2C_START |
SM501_I2C_ENABLE));
+ if (wait_for_bus()) {return(1);}
+
+ /* try to read one byte from current (or only) address */
+ write_i2c_reg(SM501_I2C_CONTROL, tmp | SM501_I2C_ENABLE);
+ write_i2c_reg(SM501_I2C_BYTE_COUNT, 0);
+ write_i2c_reg(SM501_I2C_SLAVE_ADDRESS, (chip << 1) | SM501_I2C_READ);
+ tmp = read_i2c_reg(SM501_I2C_CONTROL);
+ write_i2c_reg(SM501_I2C_CONTROL, tmp | SM501_I2C_START);
+ udelay (1000);
+
+ if (!(read_i2c_reg(SM501_I2C_STATUS) & (SM501_I2C_NACK |
SM501_I2C_BUS_ERROR))) {
+ tmp = read_i2c_reg(SM501_I2C_CONTROL);
+ write_i2c_reg(SM501_I2C_CONTROL, tmp & ~(SM501_I2C_START |
SM501_I2C_ENABLE));
+ if (wait_for_bus()) {return(1);}
[U-Boot] [PATCH] 5/12 Multiadapter/multibus I2C, drivers part 2, fixed
Initial multiadapter/multibus I2C support.
Signed-off-by: Sergey Kubushyn
---
diff -purN u-boot-i2c.orig/drivers/i2c/omap1510_i2c.c
u-boot-i2c/drivers/i2c/omap1510_i2c.c
--- u-boot-i2c.orig/drivers/i2c/omap1510_i2c.c 2009-02-12 10:43:41.0
-0800
+++ u-boot-i2c/drivers/i2c/omap1510_i2c.c 2009-02-12 10:46:00.0
-0800
@@ -1,4 +1,8 @@
/*
+ * Copyright (c) 2009 Sergey Kubushyn
+ *
+ * Changes for multibus/multiadapter I2C support.
+ *
* Basic I2C functions
*
* Copyright (c) 2003 Texas Instruments
@@ -19,11 +23,51 @@
*/
#include
+#include
+
+i2c_adap_t omap1510_i2c_adap;
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static void wait_for_bb (void)
+{
+ int timeout = 10;
+
+ while ((inw (I2C_STAT) & I2C_STAT_BB) && timeout--) {
+ inw (I2C_IV);
+ udelay (1000);
+ }
+
+ if (timeout <= 0) {
+ printf ("timed out in wait_for_bb: I2C_STAT=%x\n",
+ inw (I2C_STAT));
+ }
+}
+
+static u16 wait_for_pin (void)
+{
+ u16 status, iv;
+ int timeout = 10;
+
+ do {
+ udelay (1000);
+ status = inw (I2C_STAT);
+ iv = inw (I2C_IV);
+ } while (!iv &&
+!(status &
+ (I2C_STAT_ROVR | I2C_STAT_XUDF | I2C_STAT_XRDY |
+ I2C_STAT_RRDY | I2C_STAT_ARDY | I2C_STAT_NACK |
+ I2C_STAT_AL)) && timeout--);
+
+ if (timeout <= 0) {
+ printf ("timed out in wait_for_pin: I2C_STAT=%x\n",
+ inw (I2C_STAT));
+ }
-static void wait_for_bb (void);
-static u16 wait_for_pin (void);
+ return status;
+}
-void i2c_init (int speed, int slaveadd)
+static void omap1510_i2c_init (int speed, int slaveadd)
{
u16 scl;
@@ -46,6 +90,10 @@ void i2c_init (int speed, int slaveadd)
outw (slaveadd, I2C_OA);
outw (0, I2C_CNT);
udelay (1000);
+
+ if (gd->flags & GD_FLG_RELOC) {
+ omap1510_i2c_adap.init_done = 1;
+ }
}
static int i2c_read_byte (u8 devaddr, u8 regoffset, u8 * value)
@@ -158,7 +206,7 @@ static int i2c_write_byte (u8 devaddr, u
return i2c_error;
}
-int i2c_probe (uchar chip)
+static int omap1510_i2c_probe (uchar chip)
{
int res = 1;
@@ -188,24 +236,24 @@ int i2c_probe (uchar chip)
return res;
}
-int i2c_read (uchar chip, uint addr, int alen, uchar * buffer, int len)
+static int omap1510_i2c_read (uchar chip, uint addr, int alen, uchar * buffer,
int len)
{
int i;
if (alen > 1) {
- printf ("I2C read: addr len %d not supported\n", alen);
+ printf ("%s: addr len %d not supported\n", __FUNCTION__, alen);
return 1;
}
if (addr + len > 256) {
- printf ("I2C read: address out of range\n");
+ printf ("%s: address out of range\n", __FUNCTION__);
return 1;
}
for (i = 0; i < len; i++) {
if (i2c_read_byte (chip, addr + i, &buffer[i])) {
- printf ("I2C read: I/O error\n");
- i2c_init (CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
+ printf ("%s: I/O error\n", __FUNCTION__);
+ i2c_init (CONFIG_SYS_OMAP1510_I2C_SPEED,
CONFIG_SYS_OMAP1510_I2C_SLAVE);
return 1;
}
}
@@ -213,24 +261,24 @@ int i2c_read (uchar chip, uint addr, int
return 0;
}
-int i2c_write (uchar chip, uint addr, int alen, uchar * buffer, int len)
+static int omap1510_i2c_write (uchar chip, uint addr, int alen, uchar *
buffer, int len)
{
int i;
if (alen > 1) {
- printf ("I2C read: addr len %d not supported\n", alen);
+ printf ("%s: addr len %d not supported\n", __FUNCTION__, alen);
return 1;
}
if (addr + len > 256) {
- printf ("I2C read: address out of range\n");
+ printf ("%s: address out of range\n", __FUNCTION__);
return 1;
}
for (i = 0; i < len; i++) {
if (i2c_write_byte (chip, addr + i, buffer[i])) {
- printf ("I2C read: I/O error\n");
- i2c_init (CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
+ printf ("%s: I/O error\n", __FUNCTION__);
+ i2c_init (CONFIG_SYS_OMAP1510_I2C_SPEED,
CONFIG_SYS_OMAP1510_I2C_SLAVE);
return 1;
}
}
@@ -238,40 +286,25 @@ int i2c_write (uchar chip, uint addr, in
return 0;
}
-static void wait_for_bb (void)
+static unsigned int omap1510_i2c_set_bus_speed(unsigned int speed)
{
- int timeout = 10;
-
- while ((inw (I2C_STAT) & I2C_STAT_BB) && timeout--) {
- inw (I2C_IV);
- udelay (1000);
- }
-
- if (timeout <= 0) {
- printf ("
[U-Boot] [PATCH] 1/12 Multiadapter/multibus I2C, common part 1, fixed
Initial multiadapter/multibus I2C support.
Signed-off-by: Sergey Kubushyn
---
diff -purN u-boot-i2c.orig/common/cmd_date.c u-boot-i2c/common/cmd_date.c
--- u-boot-i2c.orig/common/cmd_date.c 2009-02-12 10:43:41.0 -0800
+++ u-boot-i2c/common/cmd_date.c2009-02-12 10:46:00.0 -0800
@@ -46,8 +46,13 @@ int do_date (cmd_tbl_t *cmdtp, int flag,
int old_bus;
/* switch to correct I2C bus */
+#ifdef CONFIG_NEW_I2C
+ old_bus = i2c_get_bus_num();
+ i2c_set_bus_num(CONFIG_SYS_RTC_BUS_NUM);
+#else
old_bus = I2C_GET_BUS();
I2C_SET_BUS(CONFIG_SYS_RTC_BUS_NUM);
+#endif
switch (argc) {
case 2: /* set date & time */
@@ -94,7 +99,11 @@ int do_date (cmd_tbl_t *cmdtp, int flag,
}
/* switch back to original I2C bus */
+#ifdef CONFIG_NEW_I2C
+ i2c_set_bus_num(old_bus);
+#else
I2C_SET_BUS(old_bus);
+#endif
return rcode;
}
diff -purN u-boot-i2c.orig/common/cmd_dtt.c u-boot-i2c/common/cmd_dtt.c
--- u-boot-i2c.orig/common/cmd_dtt.c2009-02-12 10:43:41.0 -0800
+++ u-boot-i2c/common/cmd_dtt.c 2009-02-12 10:46:00.0 -0800
@@ -35,8 +35,13 @@ int do_dtt (cmd_tbl_t * cmdtp, int flag,
int old_bus;
/* switch to correct I2C bus */
+#ifdef CONFIG_NEW_I2C
+ old_bus = i2c_get_bus_num();
+ i2c_set_bus_num(CONFIG_SYS_DTT_BUS_NUM);
+#else
old_bus = I2C_GET_BUS();
I2C_SET_BUS(CONFIG_SYS_DTT_BUS_NUM);
+#endif
/*
* Loop through sensors, read
@@ -46,7 +51,11 @@ int do_dtt (cmd_tbl_t * cmdtp, int flag,
printf ("DTT%d: %i C\n", i + 1, dtt_get_temp (sensors[i]));
/* switch back to original I2C bus */
+#ifdef CONFIG_NEW_I2C
+ i2c_set_bus_num(old_bus);
+#else
I2C_SET_BUS(old_bus);
+#endif
return 0;
} /* do_dtt() */
diff -purN u-boot-i2c.orig/common/cmd_i2c.c u-boot-i2c/common/cmd_i2c.c
--- u-boot-i2c.orig/common/cmd_i2c.c2009-02-12 10:43:41.0 -0800
+++ u-boot-i2c/common/cmd_i2c.c 2009-02-12 10:46:00.0 -0800
@@ -1,4 +1,9 @@
/*
+ * (C) Copyright 2009
+ * Sergey Kubushyn, himself, k...@koi8.net
+ *
+ * Changes for unified multibus/multiadapter I2C support.
+ *
* (C) Copyright 2001
* Gerald Van Baren, Custom IDEAS, vanba...@cideas.com.
*
@@ -106,7 +111,7 @@ static uint i2c_mm_last_alen;
* pairs. The following macros take care of this */
#if defined(CONFIG_SYS_I2C_NOPROBES)
-#if defined(CONFIG_I2C_MULTI_BUS)
+#if CONFIG_SYS_NUM_I2C_BUSSES > 1
static struct
{
uchar bus;
@@ -122,19 +127,11 @@ static uchar i2c_no_probes[] = CONFIG_SY
#define COMPARE_BUS(b,i) ((b) == 0) /* Make compiler happy */
#define COMPARE_ADDR(a,i) (i2c_no_probes[(i)] == (a))
#define NO_PROBE_ADDR(i) i2c_no_probes[(i)]
-#endif /* CONFIG_MULTI_BUS */
+#endif /* CONFIG_SYS_NUM_I2C_BUSSES > 1 */
#define NUM_ELEMENTS_NOPROBE (sizeof(i2c_no_probes)/sizeof(i2c_no_probes[0]))
#endif
-#if defined(CONFIG_I2C_MUX)
-static I2C_MUX_DEVICE *i2c_mux_devices = NULL;
-static int i2c_mux_busid = CONFIG_SYS_MAX_I2C_BUS;
-
-DECLARE_GLOBAL_DATA_PTR;
-
-#endif
-
static int
mod_i2c_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char *argv[]);
@@ -548,10 +545,10 @@ mod_i2c_mem(cmd_tbl_t *cmdtp, int incrfl
*/
int do_i2c_probe (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
{
- int j;
+ int j;
#if defined(CONFIG_SYS_I2C_NOPROBES)
- int k, skip;
- uchar bus = GET_BUS_NUM;
+ int k, skip;
+ unsigned intbus = GET_BUS_NUM;
#endif /* NOPROBES */
puts ("Valid chip addresses:");
@@ -1189,59 +1186,79 @@ int do_sdram (cmd_tbl_t * cmdtp, int fla
#if defined(CONFIG_I2C_CMD_TREE)
int do_i2c_reset(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
{
- i2c_init (CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
+ i2c_init (ADAP(i2c_get_bus_num())->speed,
ADAP(i2c_get_bus_num())->slaveaddr);
return 0;
}
-#if defined(CONFIG_I2C_MUX)
-int do_i2c_add_bus(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
+int do_i2c_show_bus(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
{
- int ret=0;
+ int i;
+#ifndef CONFIG_SYS_I2C_DIRECT_BUS
+ int j;
+#endif
if (argc == 1) {
/* show all busses */
- I2C_MUX *mux;
- I2C_MUX_DEVICE *device = i2c_mux_devices;
-
- printf ("Busses reached over muxes:\n");
- while (device != NULL) {
- printf ("Bus ID: %x\n", device->busid);
- printf (" reached over Mux(es):\n");
- mux = device->mux;
- while (mux != NULL) {
- printf ("%...@%x ch: %x\n", mux->name,
mux->chip, mux->channel);
- mux = mux->next;
+ for (i = 0; i < CONFIG_SYS_NUM_I2C_BUSSES; i
