Re: [PATCH 2/2] reset: brcm-pmc: add driver for Broadcom's PMC

[Florian Fainelli <f.fainelli@gmail.com>]

On 11/17/20 10:44 AM, Rafał Miłecki wrote:
> On 12.11.2020 15:55, Rafał Miłecki wrote:
>> *** PMB ***
>>
>> It seems like PMB Master is reset controller on its own. Even though
>> we don't
>> have actual driver for the (documented) "brcm,bcm63138-pmb" binding,
>> arch code
>> treates it like a reset.
>>
>> So it seems that a single PMB is capable at least of:
>> 1. Resetting ARM CPU core (id by 8b addr)
>> 2. Resetting devices (id by 8b addr) by (en|dis)abling zones
>>
>> Above operations are performed in a different (programming) ways.
>>
>>
>> *** PMC ***
>>
>> PMC seems to be a reset controller with firmware capable at least of:
>> 1. Resetting devices (id by 8b addr) by (en|dis)abling zones
>>
>> that fallback to using PMB blocks when needed / required.
> 
> This just got more complex as I started playing with PMC / PMB and PCIe
> controller.
> 
> 1. BPCM_CAPABILITIES reg for PCIe controller reports 0 num_zones
> 2. Enabling PCIe requires powering on zone 0 manually
> 3. After powering on zone, PCIe requires setting SR_CONTROL
> 
> It means that PMB driver (and so PMC one as it fallback to the PMB)
> needs to
> know what type of device we're addressing.

From prior attempts at getting the 63138 supported which was a fairly
early silicon revision with a mix of PMB and PMC ultimately, the reset
driver does need to know what kind of device ID is being reset/powered
on. I stopped after seeing that SATA, USB, PCIe, Switch would all need
to be treated differently, and the board I had should have been upgraded
to a B0:

https://github.com/ffainelli/linux/commit/50f1dfb17b624ee41a11dda01bc899e6756869b7

There are way too many quirks for Device Tree to express all of them,
and the whole point of the BPCM was to design a re-usable and
self-discoverable  power/reset/clocking module. As you found out, if the
capabilities are wrong, it defeats the purpose.

> 
> Anything simple like:
> 
> resets = <&pmb0 14>;
> resets = <&pmc 1 14>;
> 
> won't work (unless we hardcode ids in driver - which is unwanted for DTS).
> 
> 
> So I guess that after all we'll need something like:
> 
> cpu@1 {
>     resets = <&pmb0 PMB_ARM 4>; /* ARM CPU */
> }
> 
> foo {
>     resets = <&pmb1 PMB_USB_DEVICE 17>; /* Reset USB using PMB directly */
> }
> 
> bar {
>     resets = <&pmb1 PMB_PCIE_DEVICE 15>; /* Reset PCIE using PMB
> directly */
> }

What is the second reset cell mapping to? Maybe you can define a C
preprocessor macro that expressed both the PMB bus + device ID?

> 
> and also
> 
> qux {
>     resets = <&pmc PMB_PCIE_DEVICE 1 15>; /* Reset PCIe using PMC */
> }


-- 
Florian