Received-SPF: pass (google.com: domain of linux-kernel+bounces-168251-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <663556cb.5d0a0220.4b13a.04d5@mx.google.com>
Date: Fri, 3 May 2024 23:27:39 +0200
From: Christian Marangi <ansuelsmth@gmail.com>
To: Florian Fainelli <f.fainelli@gmail.com>
Cc: Florian Fainelli <florian.fainelli@broadcom.com>,
	Hauke Mehrtens <hauke@hauke-m.de>,
	=?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>,
	Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>,
	=?iso-8859-1?Q?=C1lvaro_Fern=E1ndez?= Rojas <noltari@gmail.com>,
	linux-mips@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Daniel =?iso-8859-1?Q?Gonz=E1lez?= Cabanelas <dgcbueu@gmail.com>
Subject: Re: [PATCH 4/6] mips: bmips: setup: make CBR address configurable
References: <20240503135455.966-1-ansuelsmth@gmail.com>
 <20240503135455.966-5-ansuelsmth@gmail.com>
 <bf20e911-26db-4291-95d4-c91cc1d7f000@broadcom.com>
 <66353c92.050a0220.fd42f.7cfb@mx.google.com>
 <6ca4f556-28ea-4f51-a8dc-a10a94d726f0@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6ca4f556-28ea-4f51-a8dc-a10a94d726f0@gmail.com>

On Fri, May 03, 2024 at 02:24:49PM -0700, Florian Fainelli wrote:
> On 5/3/24 12:35, Christian Marangi wrote:
> > 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?
> > 
> 
> I was going to suggest the use of memblock_start_of_DRAM() and
> memblock_end_of_DRAM() but before I sent that out your v2 already had it!

Ehehe, I was initially using mem_map and totalram_pages but then I inspected
memblock header and notice those good API.

-- 
	Ansuel