On Fri, Jun 15, 2018 at 05:11:17AM -0700, Andrey Smirnov wrote: > On Fri, Jun 15, 2018 at 2:28 AM Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> wrote: > > > > Hi Andrey, > > > > On Thu, Jun 14, 2018 at 09:11:06PM -0700, Andrey Smirnov wrote: > > > Everyone: > > > > > > While debugging the reason behind print_hex_dump() not producing > > > carriage return properly, when used in PBL, I realised that current > > > codebase contained: > > > > > > - at least 5 places where '\n' was replaced with '\n\r' > > > - at least 3 almost identical implementations of puts() > > > - at least 3 almost identical implementations of printf() > > > > > > so this patcheset is an attempt to consolidate, share and simplify > > > console related code. > > > > The console support really deserves some cleanup. We have the LL console > > support, PBL console support, regular console support and simple console > > support, all mixed into a single codebase so that it's sometimes really > > hard to understand what is going on. > > > > Instead of optimizing the different variants for better code sharing I > > wonder if we could not consolidate some of the console types to reduce > > the number of variants in the code. PBL console works by calling > > pbl_set_putc() to specify a putc function. PUTC_LL instead works by > > putting a PUTC_LL function into a SoC specific header function. Instead > > each board could provide its own putc function, say board_putc() or so. > > This would be enough to replace the DEBUG_LL and the PBL console > > support. > > > > - That still leaves psci_set_putc(), which currently is handled the > same way pbl_set_putc() does > - Dropping pbl_set_putc(), would require making direct changes to the > code for boards I don't have access to (as opposed to indirect API > changes that I can test with on boards that I do have) > > IMHO, what you are proposing is orthogonal to the work in this > patchset. One can unify PUTC_LL and pbl_set_putc() usecases, but it > wouldn't change the fact that PBL code has it's own, yet another, > implementation of puts() and printf(). Ok, I buy this argument. > > Then I don't like weak functions. It can provide nifty solutions to some > > problems, but I think it also often leads to situations where you don't > > really know if something has been overwritten or with what is has been > > overwritten with. > > And with #ifdefs you do? I regularly find myself having to inject > #error statements or look at the final disassembly to make sure I > interpret the preprocessor logic right, but that might be my unique > experience. I know this problem and in fact it was one problem that has driven me away from U-Boot. No, you are right, #fdefs are not better than weak functions. I just have the feeling that allowing weak functions is yet another can of worms, also admittedly a less ugly one than #ifdefs. > > OK, do you want me to get rid of all of the uses of __weak in this > patch set or does your comment apply only to "console: Drop > ARCH_HAS_CTRLC"? Leave them in for now. So far I haven't seen the major selling point for this series. It makes some things better, but others just look different. One thing I noticed is that the resulting barebox.bin for imx_v7_defconfig gets 5k bigger with this series. I have no idea why this happens, nothing obvious pops up, but that's not what I expect from a series which tries to consolidate things. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/barebox
