Re: [PATCH 1/1] soc: qcom: geni: Provide parameter error checking

[Lee Jones]

On Wed, 04 Sep 2019, Bjorn Andersson wrote:

> On Wed 04 Sep 13:01 PDT 2019, Lee Jones wrote:
> 
> > On Wed, 04 Sep 2019, Bjorn Andersson wrote:
> > 
> > > On Wed 04 Sep 01:45 PDT 2019, Lee Jones wrote:
> > > 
> > > > On Tue, 03 Sep 2019, Bjorn Andersson wrote:
> > > > 
> > > > > On Tue 03 Sep 06:50 PDT 2019, Lee Jones wrote:
> [..]
> > > > With this simple parameter checking patch, the SE falls back to using
> > > > FIFO mode to transmit data and continues to work flawlessly.  IMHO
> > > > this should be applied in the first instance, as it fixes a real (null
> > > > dereference) bug which currently resides in the Mainline kernel.
> > > > 
> > > 
> > > Per the current driver design the wrapper device is the parent of the
> > > SE, I should have seen that 8bc529b25354 was the beginning of a game of
> > > whac-a-mole circumventing this design. Sorry for not spotting this
> > > earlier.
> > 
> > Right, but that doesn't mean that the current driver design is
> > correct.  ACPI, which is in theory a description of the hardware
> > doesn't seem to think so.  It looks more like we do this in Linux as a
> > convenience function to link the devices.  Instead this 'parent' seems
> > to be represented as a very small register space at the end of the SE
> > banks.
> 
> There's a larger register window containing one block of common
> registers followed by register blocks for each serial engine.
> 
> I don't know if we will need more of the common registers in the future,
> but for now you at least have the requirement that in order to operate
> the SEs you need to clock the wrapper. So the current DT model
> represents the hardware and the power/clocking topology.
> 
> The fact that you managed to boot the system with just ignoring all
> clocks is a surprise to me.

That is a prerequisite of UEFI/ACPI.  All clocks, registers, phys,
resets, etc must be configured by the firmware.  We should only need
to play with them for Power Management purposes (which requires the
PEP to be enabled).

> > > But if this is the one whack left to get the thing to boot then I think
> > > we should merge it.
> > 
> > Amazing, thank you!
> > 
> > Do you know how we go about getting this merged?  We only potentially
> > have 0.5 weeks (1.5 weeks if there is an -rc8 [doubtful]), so we need
> > to move fast.  Would you be prepared to send it to Linus for -fixes?
> > I'd do it myself, but this is a little out of my remit.
> > 
> 
> The "offending" commit was picked up mid June and no one noticed that it
> doesn't work until this week?

I think you're viewing this from the wrong angle.  The "ACPI
enablement" commit(s) merely prevented the use of the Clock and
Regulator APIs, as per the ACPI guidelines.  See [0].

Right.  Unfortunately, I developed this on top of our DT enablement
patches, which also included your Geni SE DMA and Regulator status
hacks, which meant I missed this and the above until now.

It's only now I've had the chance to attempt to boot raw Mainline that
these came to light, hence the last minute panic to try and fix them.

> Let's slap a Cc: stable@ on it and get it into v5.4-rc1 and it will show
> up in v5.3.1.

We *can* do that, however my fear is that the distros are going to be
releasing their new versions (next month) based on v5.3.  Which would
mean they will not boot on these platforms until they backport these
patches, which might be some months later.

This is the only issue that prevents ACPI from booting, meaning we can
at least make use of the distro installers when they are released.
The patch is very simple and low-risk, so ideally it would go into
-rc7.

[0] Documentation/arm64/arm-acpi.rst

-- 
Lee Jones [æçæ]
Linaro Services Technical Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog