Re: [PATCHv2] reset: socfpga: add an early reset driver for SoCFPGA

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Dinh,

On Wed, 2018-10-17 at 10:02 -0500, Dinh Nguyen wrote:
[...]
> > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > > index 13d28fdbdbb5..f10de5ce4753 100644
> > > --- a/drivers/reset/Kconfig
> > > +++ b/drivers/reset/Kconfig
> > > @@ -100,7 +100,7 @@ config RESET_QCOM_AOSS
> > >  
> > >  config RESET_SIMPLE
> > >  	bool "Simple Reset Controller Driver" if COMPILE_TEST
> > > -	default ARCH_SOCFPGA || ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX || ARCH_ASPEED
> > > +	default ARCH_STM32 || ARCH_STRATIX10 || ARCH_SUNXI || ARCH_ZX || ARCH_ASPEED
> > >  	help
> > >  	  This enables a simple reset controller driver for reset lines that
> > >  	  that can be asserted and deasserted by toggling bits in a contiguous,
> > > @@ -119,6 +119,13 @@ config RESET_STM32MP157
> > >  	help
> > >  	  This enables the RCC reset controller driver for STM32 MPUs.
> > >  
> > > +config RESET_SOCFPGA
> > > +	bool "SoCFPGA Reset Driver" if COMPILE_TEST && !ARCH_SOCFPGA
> > > +	default ARCH_SOCFPGA && !ARCH_STRATIX10
> > > +	select RESET_SIMPLE
[...]
> > > +static const struct of_device_id socfpga_early_reset_dt_ids[] __initconst = {
> > > +	{ .compatible = "altr,rst-mgr", },
> > 
> > That doesn't seem right. If you don't remove the altr,rst-mgr compatible
> > from reset-simple.c anymore, we suddenly have two device drivers for the
> > same compatible.
> 
> You're right, and that's why I am not building reset-simple for
> ARCH_SOCFPGA anymore. I am only building reset-simple for ARCH_STRATIX10.

RESET_SOCFPGA still selects RESET_SIMPLE, so reset-simple should be
built in both cases.

> > (Also I liked removing altr,modrst-offset from reset-simple.c)
> 
> I can remove it since it's just 0x20 of ARCH_STRATIX10.

As long as there is another driver handling that compatible,
altr,rst-mgr support should be removed from the platform driver in
reset-simple.c altogether.

> > Would there be any issue with calling socfpga_reset_init() on Stratix10
> > as well?
> 
> I don't see any place in the arm64 common code where I can do this.

This is one of the reasons why there should always be a SoC specific
compatible as well as the generic one. If the device trees contained
something like

	compatible = "altr,socfpga-a10-rst-mgr", "altr,rst-mgr";

and

	compatible = "altr,socfpga-s10-rst-mgr", "altr,rst-mgr";

we could now easily match the specific compatibles for the different
cases (s10 in reset-simple, a10 in reset-socfpga).

I suppose one way to handle this with the shared compatible would be to
add a platform driver for s10 to reset-socfpga.c, but register it only
if socfpga_reset_init() was not called earlier.

regards
Philipp