Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <b7d29bf2-f7b6-1d53-2d59-9c1bb5ee4d84@gmail.com>
Date:   Thu, 30 Sep 2021 14:19:53 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.1.0
Content-Language: en-US
To:     Tony Lindgren <tony@atomide.com>, linux-omap@vger.kernel.org
Cc:     Dave Gerlach <d-gerlach@ti.com>, Faiz Abbas <faiz_abbas@ti.com>,
        Santosh Shilimkar <ssantosh@kernel.org>,
        Suman Anna <s-anna@ti.com>, Tero Kristo <kristo@kernel.org>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        Drew Fustini <pdp7pdp7@gmail.com>,
        Grygorii Strashko <grygorii.strashko@ti.com>,
        "H. Nikolaus Schaller" <hns@goldelico.com>,
        Robert Nelson <robertcnelson@gmail.com>,
        Yongqin Liu <yongqin.liu@linaro.org>
References: <20210930080100.56820-1-tony@atomide.com>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH] soc: ti: omap-prm: Fix external abort for am335x pruss
In-Reply-To: <20210930080100.56820-1-tony@atomide.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Thanks Tony!

This was _much_ appreciated :)

On 9/30/21 11:01, Tony Lindgren wrote:
> Starting with v5.15-rc1, we may now see some am335x beaglebone black
> device produce the following error on pruss probe:
> 
> Unhandled fault: external abort on non-linefetch (0x1008) at 0xe0326000
> 
> This has started with the enabling of pruss for am335x in the dts files.
> 
> Turns out the is caused by the PRM reset handling not waiting for the
> reset bit to clear. To fix the issue, let's always wait for the reset
> bit to clear, even if there is a separate reset status register.
> 
> We attempted to fix a similar issue for dra7 iva with a udelay() in
> commit effe89e40037 ("soc: ti: omap-prm: Fix occasional abort on reset
> deassert for dra7 iva"). There is no longer a need for the udelay()
> for dra7 iva reset either with the check added for reset bit clearing.
> 
> Cc: Drew Fustini <pdp7pdp7@gmail.com>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Cc: "H. Nikolaus Schaller" <hns@goldelico.com>
> Cc: Robert Nelson <robertcnelson@gmail.com>
> Cc: Yongqin Liu <yongqin.liu@linaro.org>
> Reported-by: Matti Vaittinen <mazziesaccount@gmail.com>
> Fixes: effe89e40037 ("soc: ti: omap-prm: Fix occasional abort on reset deassert for dra7 iva")

Tested-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   drivers/soc/ti/omap_prm.c | 27 +++++++++++++++------------
>   1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> --- a/drivers/soc/ti/omap_prm.c
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -825,25 +825,28 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>   	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctrl);
>   	spin_unlock_irqrestore(&reset->lock, flags);
>   
> -	if (!has_rstst)
> -		goto exit;
> +	/* wait for the reset bit to clear */
> +	ret = readl_relaxed_poll_timeout_atomic(reset->prm->base +
> +						reset->prm->data->rstctrl,
> +						v, !(v & BIT(id)), 1,
> +						OMAP_RESET_MAX_WAIT);
> +	if (ret)
> +		pr_err("%s: timedout waiting for %s:%lu\n", __func__,
> +		       reset->prm->data->name, id);

If I was writing this I might drop the __func__. AFAIR dyndbg allows 
enabling the functipn names to be printed by +f. This is just a 'nit' 
though - I am happy if this fix gets in no matter how this print 
eventually looks like. I just thought I mention this as the __func__ 
catched my eye.

>   
>   	/* wait for the status to be set */
> -	ret = readl_relaxed_poll_timeout_atomic(reset->prm->base +
> +	if (has_rstst) {
> +		ret = readl_relaxed_poll_timeout_atomic(reset->prm->base +
>   						 reset->prm->data->rstst,
>   						 v, v & BIT(st_bit), 1,
>   						 OMAP_RESET_MAX_WAIT);
> -	if (ret)
> -		pr_err("%s: timedout waiting for %s:%lu\n", __func__,
> -		       reset->prm->data->name, id);
> +		if (ret)
> +			pr_err("%s: timedout waiting for %s:%lu\n", __func__,
> +			       reset->prm->data->name, id);

Same here (although that would be unrelated change as the print exists 
prior this patch).

I tested this patch on v5.15-rc3 using my BBB Rev C - it seems to fix 
the boot issue on my board! Thanks a bunch!


Best Regards
	--Matti Vaittinen