Re: [PATCH] spi: bcm2835: do not unregister controller in shutdown handler

[Florian Fainelli]

On 10/4/21 9:52 AM, Mark Brown wrote:
> On Mon, Oct 04, 2021 at 09:36:37AM -0700, Florian Fainelli wrote:
>> On 10/4/21 9:31 AM, Mark Brown wrote:
> 
>>> an issue, someone could press a button or whatever.  Frankly for SPI the
>>> quiescing part doesn't seem like logic that should be implemented in
>>> drivers, it's a subsystem level thing since there's nothing driver
>>> specific about it.
> 
>> Surely the SPI subsystem can help avoid queuing new transfers towards
>> the SPI controller while the controller can shut down the resources that
>> only it knows about.
> 
> Yes, that's what I was saying.
> 
>>> In the case of this specific driver I'm still not clear that the best
>>> thing isn't just to delete the shutdown callback and let any ongoing
>>> transfers complete, though I guess there'd be issues in kexec cases with
>>> long enough tansfers.
> 
>> No please don't, I should have arguably justified the reasons why
>> better, but the main reason is that one of the platforms on which this
>> driver is used has received extensive power management analysis and
>> changes, and shutting down every bit of hardware, including something as
>> small as a SPI controller, and its clock (and its PLL) helped meet
>> stringent power targets.
> 
> OK, so it's similar to a lot of the other embedded cases where it's for
> a power down that doesn't cut as much power as would be desirable -
> that's reasonable.  Like you say you didn't mention it at all in the
> changelog.  Ideally the hardware would just cut all power to the SoC in
> shutdown but then IIRC those boards don't have a PMIC so...  

Yes, that's is what we do on other types of SoCs, this particular one
however only has a single power domain and so software must come to the
rescue to shut down as much as it can. Newer boards do have a PMIC that
can help us with that, but not with everything, still.

> 
>> TBH, I still wonder why we have .shutdown() and we simply don't use
>> .remove() which would reduce the amount of work that people have to do
>> validate that the hardware is put in a low power state and would also
>> reduce the amount of burden on the various subsystems.
> 
> Yeah, it does seem a bit odd - I'd figured it was for speed reasons.
> 


-- 
Florian