Re: [PATCH v4] amba: Remove deferred device addition

From: Saravana Kannan <saravanak@google.com>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>,
	Russell King <linux@armlinux.org.uk>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Rob Herring <robh@kernel.org>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	John Stultz <john.stultz@linaro.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Kefeng Wang <wangkefeng.wang@huawei.com>,
	kernel-team@android.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4] amba: Remove deferred device addition
Date: Tue, 19 Jul 2022 09:57:07 -0700	[thread overview]
Message-ID: <CAGETcx-YSVunJv+xpHd3PD4O8m=C5JjBh5b+h+6WnEcUAr=5_g@mail.gmail.com> (raw)
In-Reply-To: <20220719133931.7dkcejvc6s4a7y4z@bogus>

On Tue, Jul 19, 2022 at 6:39 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Mon, Jul 18, 2022 at 06:55:23PM -0700, Saravana Kannan wrote:
> > On Wed, Jul 13, 2022 at 7:58 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 12:38:33PM -0700, Saravana Kannan wrote:
> > > > Sudeep,
> > > >
> > > > This makes me think the issue you are seeing is related to your
> > > > hardware drivers. Can you look into those please? I'm leaning towards
> > > > merging this amba clean up and adding delays (say 1ms) to your
> > > > clock/power domain drivers to avoid the crash you are seeing. And then
> > > > you can figure out the actual delays needed and update it.
> > >
> > > I haven't got a chance to debug the issue on Juno much further. One thing
> > > about the platform is that we can't turn off the debug power domain that
> > > most of the coresight devices share.
> > >
> > > One thing I also observed with -next+this patch is that with a little log
> > > it can access the registers while adding first few devices and then crash
> > > which doesn't align with platform behaviour as we can't turn off the domain
> > > though we attached and turn on in amba_read_periphid and then turn off and
> > > detach the power domain. Ideally if first device amba_read_periphid was
> > > successful, it must be the case for all, but I see different behaviour.
> > >
> > > I need to check again to confirm if it is issue with platform power domain
> > > driver. It is based on SCMI so there is some role played by the f/w as well.
> >
> > Yeah, this log timing based behavior is what makes me suspect it's not
> > a problem with this patch itself.
> >
> > However, just to rule it out, can you try making this change on top of
> > v4 and give it a shot? This is related to the issue Marek reported,
> > but those are more about permanent probe failures. Not a crash.
> >
>
> This patch(version v4, fails to apply on -next but the conflict is trivial
> and in the deleted code so I just retained your copy of all the functions)
> plus the below change fixes the issue I reported on Juno.

What the heck? I didn't expect it to fix the issue at all. Without the
fix some amba devices could end up not getting probed. But that
shouldn't have caused crashes. So that still indicates some issue in
your driver you might want to look into.

With that said, I'll just roll this fix into a v5 and send it out.
While the revert would fix it, I don't want this to be blocked on that
or be fragile enough to be broken in the future.

-Saravana

>
> I won't give you tested-by yet as you have plans to revert some things
> and I resolved the conflict here though trivial, I prefer to apply the
> patch as is with all associated changes and test once more.
>
> Thanks for digging this and fixing it.
>
> --
> Regards,
> Sudeep