Re: [PATCH 00/27] Console code consolidation

[Sascha Hauer <s.hauer@pengutronix.de>]

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@pengutronix.de> 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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox