Re: [PATCH v2 0/8] ARM: at91: Remove mach/ includes from the reset driver

[Maxime Ripard]

Hi,

On Tue, Oct 28, 2014 at 09:52:03AM +0100, Alexandre Belloni wrote:
> Hi,
> 
> On 28/10/2014 at 08:50:53 +0100, Boris Brezillon wrote :
> > Hi,
> > 
> > On Tue, 28 Oct 2014 00:09:29 +0100
> > Alexandre Belloni <alexandre.belloni@xxxxxxxxxxxxxxxxxx> wrote:
> > 
> > > This series removes the mach/ headers dependency from the reset driver. It is
> > > also laying some groundwork for the necessary power management support rework.
> > > 
> > > The first patch adds and export a function to shutdown the sdram from the sdramc
> > > driver. That function also take the RSTC CR register and a value as parameters
> > > to be able to reset the chip. This is a hackish way of doing it but it ensures
> > > that all the code fits in one cache line. We already have plan to start using
> > > the sram to have a cleaner way to execute that code safely as soon as that
> > > series goes in:
> > > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-September/198778.html
> > > 
> > > The second patch makes the sdramc driver usable from the board files.
> > > 
> > > The third patch actually registers the sdramc driver from the boards files.
> > > The fourth patch does the same, only for sam9g45 and sam9rl to simplify future
> > > merging as the board files have been removed. Simply drop that patch.
> > > 
> > > The fifth patch makes the at91-reset driver use the newly created
> > > at91_ramc_shutdown() function and removes the mach/ headers inclusion.
> > 
> > I'm not a big fan of this approach.
> > 
> > I definitely think each step of the reset process should be handled in
> > the appropriate block (and the patch series you pointed out would
> > definitely help in achieving this goal), but you're just moving all the
> > stuff done in the reset driver into the SDRAM one, which means you're
> > solving one design issue by introducing a new one.
> > 
> > Moreover, the errata at the origin of this hack is attached to the RSTC
> > (Rest Controller) block in the datasheets.
> > 
> 
> I agree it is still not clean but it is a step in the good direction.
> The sdram shutdown will have to be down in the sdram driver at some
> point, even if the errata is attached to the reset controller. At least,
> the series introduces a function to do the sdram shutdown.

Not really. AFAICS, this adds a function to reset the CPU. Actually,
it doesn't add anything at all, it just copies what was done in the
reset driver.

> > I'd rather keep the reset driver as is and move SDRAM related macros
> > into a specific header (include/linux/memory/atmel-sdram.h or
> > include/soc/atmel/memory.h as you proposed) so that the reset driver
> > can reference them without including mach headers.
> > 
> 
> My personal opinion is that it is better to hide the registers/bits from
> the reset driver right now as we have two different IPs and the sdram
> driver already knows how to make the difference between them.

The reset driver doesn't do anything anymore with these patches. Why
not just remove it altogether?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachment: signature.asc
Description: Digital signature