[U-Boot] [PATCH 03/11] DM: add block controller core

[Pavel Herrmann]

On Friday 21 of September 2012 20:01:27 Marek Vasut wrote:
> Dear Pavel Herrmann,
> 
> > On Friday 21 of September 2012 18:08:13 Marek Vasut wrote:
> > > Dear Pavel Herrmann,
> > > 
> > > > On Friday 21 of September 2012 17:39:21 Marek Vasut wrote:
> > > > > Dear Pavel Herrmann,
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > > Can't the old driver just have a compat section in them? Like I
> > > > > > > did with serial stuff:
> > > > > > > 
> > > > > > > 1) rename the internal functions to ${driver}_${function_name}
> > > > > > > from pure ${function_name} and introduce section which behaves
> > > > > > > as a wrapper (implement ${function_name} calling
> > > > > > > ${driver}_${function_name} ). 2) Add your DM goo, implement
> > > > > > > #ifdef around it so either the compat section or DM section is
> > > > > > > enabled.
> > > > > > 
> > > > > > I actually did something of this sort, see [4/11], with less
> > > > > > touching.
> > > > > > 
> > > > > > the problem is that while SATA drivers are easy to convert, IDE
> > > > > > ones are not. I would actually propose to do a ide_legacy driver
> > > > > > (mostly out of the code currently in common/cmd_ide.c), and keep
> > > > > > it as the only option until IDE dies completely.
> > > > > 
> > > > > IDE will be around for a LONG time.
> > > > > 
> > > > > You introduce that CONFIG_SYS_SATA_LEGACY for no reason, if you did
> > > > > it as
> > > > > said above, simple CONFIG_DM would suffice as the drivers would be
> > > > > intacts with DM disabled. Note the compiler will opt-out these proxy
> > > > > calls.
> > > > > 
> > > > > Besides, with this approach of yours, you need to enable SATA_LEGACY
> > > > > for every single board now, introducing a lot of churn into the
> > > > > patches and if it's not defined, every board using SATA is broken,
> > > > > right?
> > > > 
> > > > No, if you dont define CONFIG_DM, you get the old way of interacting
> > > > with disks. only if you define CONFIG_DM you only need
> > > > CONFIG_SATA_LEGACY to plug old SATA drivers into DM codepaths
> > > 
> > > So if you enable DM for a board, you also need to enable this compat
> > > layer? My opinion is, that you either enable DM on the board and the
> > > board is ready for it or don't enable it.
> > 
> > if you enable DM on a board, you can either use the compat layer and the
> > old driver, or use a ported driver and get rid of the compat layer
> > 
> > > Would you need the compat layer at all were you to follow my advice?
> > 
> > if i understand what you meant, i would build this compat layer into every
> > one of the drivers currently in tree (with some cleanup). in that case, i
> > would not need it
> 
> That's correct. You would prepare every driver to be DM-ish and the final
> deployment of DM would be mechanical. So would be the removal of the non-DM
> part later on.

that would lead to massive code duplication. im not talking about the SATA 
drivers now, there its just about ready for DM already (static for all 
functions, remove dependency on static block_dev_desc array, include driver 
registration functions, done), but about IDE drivers, where the drivers have 
~200 lines, whereas cmd_ide.c has ~2000 lines. as for the SCSI drivers, ahci.c 
is not really SCSI, more like a SATA driver with SCSI wrapper built in, the 
other one is a bit more complex, and would require some wrapper code addition 
(cmd_scsi is ~600 lines)

> > > The whole idea goes deeper, see if you prepended this patchset with such
> > > a conversion as above, you'd already have a readily defined structure of
> > > blockdev operations in each driver to use in this patchset. This
> > > patchset would then really only be the DM stuff. The DM-part addition to
> > > the drivers would be mechanical.
> > > 
> > > > > > > How does that work? It's much cleaner.
> > > > > > > 
> > > > > > > > Pavel Herrmann
> > > > > > > 
> > > > > > > Best regards,
> > > > > > > Marek Vasut
> > > > > 
> > > > > Best regards,
> > > > > Marek Vasut
> > > 
> > > Best regards,
> > > Marek Vasut
> 
> Best regards,
> Marek Vasut