Re: [PATCH 4/6] mips: bmips: setup: make CBR address configurable

[Christian Marangi <ansuelsmth@gmail.com>]

On Fri, May 03, 2024 at 12:09:02PM -0700, Florian Fainelli wrote:
> On 5/3/24 06:54, Christian Marangi wrote:
> > Add support to provide CBR address from DT to handle broken
> > SoC/Bootloader that doesn't correctly init it. This permits to use the
> > RAC flush even in these condition.
> > 
> > To provide a CBR address from DT, the property "mips-cbr-reg" needs to
> > be set in the "cpus" node. On DT init, this property presence will be
> > checked and will set the bmips_cbr_addr value accordingly. Also
> > bmips_rac_flush_disable will be set to false as RAC flush can be
> > correctly supported.
> > 
> > The CBR address from DT will be applied only if the CBR address from the
> > registers is 0, if the CBR address from the registers is not 0 and
> > is not equal to the one set in DT (if provided) a WARN is printed.
> > 
> > To ALWAYS overwrite the CBR address the additional property
> > "mips-broken-cbr-reg" needs to be set.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >   arch/mips/bmips/setup.c | 30 +++++++++++++++++++++++++++---
> >   1 file changed, 27 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/mips/bmips/setup.c b/arch/mips/bmips/setup.c
> > index 18561d426f89..bef84677248e 100644
> > --- a/arch/mips/bmips/setup.c
> > +++ b/arch/mips/bmips/setup.c
> > @@ -34,7 +34,11 @@
> >   #define REG_BCM6328_OTP		((void __iomem *)CKSEG1ADDR(0x1000062c))
> >   #define BCM6328_TP1_DISABLED	BIT(9)
> > -/* CBR addr doesn't change and we can cache it */
> > +/*
> > + * CBR addr doesn't change and we can cache it.
> > + * For broken SoC/Bootloader CBR addr might also be provided via DT
> > + * with "mips-cbr-reg" in the "cpus" node.
> > + */
> >   void __iomem *bmips_cbr_addr;
> >   extern bool bmips_rac_flush_disable;
> > @@ -212,8 +216,28 @@ void __init device_tree_init(void)
> >   	/* Disable SMP boot unless both CPUs are listed in DT and !disabled */
> >   	np = of_find_node_by_name(NULL, "cpus");
> > -	if (np && of_get_available_child_count(np) <= 1)
> > -		bmips_smp_enabled = 0;
> > +	if (np) {
> 
> Please reduce the indentation with early return/gotos. There might also be a
> need to do some validation that the CBR is at least outside of the DRAM
> window, that is we cannot blindly trust the DT to have gotten the CBR right
> IMHO.

Do you have any hint on how to do the check if we are outside DRAM?

-- 
	Ansuel