From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Wed, 31 Jul 2013 15:03:16 +0200 Subject: [U-Boot] [PATCH v3 8/9] tegra: i2c: Enable new CONFIG_SYS_I2C framework In-Reply-To: References: <1367668903-29653-1-git-send-email-hs@denx.de> <1367668903-29653-9-git-send-email-hs@denx.de> <51F6946F.8010500@wwwdotorg.org> <51F740FD.4080505@denx.de> <51F81261.7020008@wwwdotorg.org> <51F81B51.6010305@wwwdotorg.org> <51F83104.5090508@wwwdotorg.org> <51F83570.1010909@wwwdotorg.org> <20130731000921.724f5c71@lilith> <20130731051821.DDA1C380480@gemini.denx.de> <20130731090644.2b4ce215@lilith> <51F8BE73.2010101@denx.de> <20130731101636.059ad6de@lilith> <51F8CB50.4070004@denx.de> <20130731113831.379156ef@lilith> Message-ID: <51F90B14.1060104@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Simon, Am 31.07.2013 14:30, schrieb Simon Glass: > Hi, > > On Wed, Jul 31, 2013 at 3:38 AM, Albert ARIBAUD > wrote: > >> Hi Heiko, >> >> On Wed, 31 Jul 2013 10:31:12 +0200, Heiko Schocher wrote: >> >>> Hello Albert, >>> >>> Am 31.07.2013 10:16, schrieb Albert ARIBAUD: >>>> Hi Heiko, >>>> >>>> On Wed, 31 Jul 2013 09:36:19 +0200, Heiko Schocher wrote: >>>> >>>>>> On the other hand, it may be hard to immediately know what functions >>>>>> throughout U-boot are safe to call from within board_init_f(); maybe >> we >>>>>> should start thinking about checking and marking these, the simplest >>>>>> way being to suffix them with "_f" once we have made sure they are >> safe >>>>>> to call from within board_init_f(). >>>>> >>>>> Hmmm... Maybe instead we should think (also in thinking common bring >>>>> up for all boards) about: >>>>> >>>>> getting rid of board_init_f in u-boot code, instead use for all >>>>> boards spl code to init needed things and copy and relocate u-boot >>>>> to ram in spl code ... so we have in u-boot no longer such >>>>> restictions ... but thats just an idea which whirs in my head ... >>>>> without thinking to deep in it. >>>>> >>>>> But this approach would have some advantages ... >>>> >>>> Well, the original SPL was basically board_init_f() plus some code to >>>> copy U-Boot from wherever it was to DDR, so it was tightly linked to >>>> board_init_f(). But... first, SPL has evolved into a "U-Boot lite" >>>> where much can happen beyond board_init_f() -- think Falcon mode, for >>>> instance -- and second, there are boards which do not have SPL at all, >>>> and their board_init_f() can thus not be "moved to SPL". >>> >>> Hmm... all boards use board_init_f ... and spl do pieces from >> board_init_f >>> So why should it not be possible to do all init things in spl code? >>> Code beyond board_init_f is optional ... >> >> It is, in the original "SPL is just board_init_f plus some copying" >> view. In the current "SPL is U-boot only not full-featured", it becomes >> false. >> >>> And yes, there are a lot of boards, which have no spl, but they >>> can execute spl code (thinking of the lof of powerpc boards which >>> booting from nor flash ... spl code can also run from nor ... and >>> copy the u-boot piece of the image to ram, relocate it ...) >>> >>> And yes, a side effect could be, that all boards can use Falcon boot >> mode. >>> >>> ;-) >>> >>>> So no, I don't think we can move U-Boot's design from "_f/_r" to >>>> "SPL/U-Boot". >>> >>> I am not sure ... >> >> I see this approach of likening SPL to _f and U-boot to _r as forcing a >> dual-binary model onto all boards whereas not all boards require it. I >> prefer a model where _f can exist, _r can exist, and for each target, >> the maintainer decides which binaries are built and for each one, >> whether _f and/or _r is present and what _r does. >> > > I am not really any clearer as to what should be done here. > > Previously on ARM i2c init generally happened in board_init_r(). This has really? The init_func_i2c() call is not introduced through the i2c multibus approach ... > changed now, and so boards which need to do some init (e.g. reading and > storing DT information) to make i2c work are going to have problems if they > cannot access any memory (yes we could put it in global_data I suppose). > > It sounds like the need for early i2c is rare, so we could perhaps create > an option like CONFIG_SYS_EARLY_I2C to enable this? > > While I agree that minimising code in board_init_f() is a great idea, if we > have one case that needs it, then we need to deal with the problem. From my side, yes. But this different to powerpc! > Although did I mention that it does seem silly to me to solve what is an > entirely hypothetical problem on Tegra and (I think) any other modern ARM > SOC that uses SPL? After all, SDRAM is fully available on these SOCs and in > fact setting that up and getting U-Boot loaded into RAM is the purpose of > the SPL stage! To my mind, SPL has taken over this responsibility of > board_init_f(). Thoughts? Maybe a minor rethink of > SPL/board_init_f()/board_init_r() is in order? Yes, but board_init_f is used in spl code, or? And it is not only ram settings, maybe read baudrate setting in an i2c epprom ... We can make init_func_i2c() weak, and in the first step a dummy function and see, which boards really need it. Nevertheless, why the tegra i2c driver do not call process_node() from the i2c_init function? This must be fixed I think, as it is not good to do some setup needed for i2c in board_init() and other in i2c_init to have a working i2c adapter ... bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany