Re: [PATCH 1/2] dt-bindings: reset: document Broadcom's BCM4908 USB reset binding

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

On 12/9/20 3:35 PM, Rob Herring wrote:
> On Fri, Dec 04, 2020 at 05:39:14PM +0100, Rafał Miłecki wrote:
>> On 04.12.2020 17:32, Florian Fainelli wrote:
>>> On 12/4/2020 1:37 AM, Rafał Miłecki wrote:
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>
>>>> Document binding of block responsible for initializing USB controllers
>>>> (OHCI, EHCI, XHCI).
>>>>
>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>> ---
>>>>   .../reset/brcm,bcm4908-usb-reset.yaml         | 60 +++++++++++++++++++
>>>>   1 file changed, 60 insertions(+)
>>>>   create mode 100644 Documentation/devicetree/bindings/reset/brcm,bcm4908-usb-reset.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/reset/brcm,bcm4908-usb-reset.yaml b/Documentation/devicetree/bindings/reset/brcm,bcm4908-usb-reset.yaml
>>>> new file mode 100644
>>>> index 000000000000..31beb1c8f3cd
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/reset/brcm,bcm4908-usb-reset.yaml
>>>> @@ -0,0 +1,60 @@
>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/reset/brcm,bcm4908-usb-reset.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Broadcom BCM4908 USB host controller reset
>>>> +
>>>> +description: >
>>>> +  BCM4908 has a separated block controlling all USB controllers. It handles the
>>>> +  whole setup process and takes care of initializing PHYs at the right time
>>>> +  (state).
>>>> +
>>>> +maintainers:
>>>> +  - Rafał Miłecki <rafal@milecki.pl>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - brcm,bcm4908-usb-reset
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  resets:
>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>> +
>>>> +  phys:
>>>> +    minItems: 2
>>>> +    maxItems: 2
>>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>>> +
>>>> +  phy-names:
>>>> +    items:
>>>> +      - const: usb2
>>>> +      - const: usb3
>>>> +
>>>> +  "#reset-cells":
>>>> +    const: 0
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - phys
>>>> +  - phy-names
>>>> +  - "#reset-cells"
>>>> +
>>>> +additionalProperties: true
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    reset-controller@8000c200 {
>>>> +        compatible = "brcm,bcm4908-usb-reset";
>>>> +        reg = <0x8000c200 0x100>;
>>>> +
>>>> +        phys = <&usb2_phy>, <&usb3_phy>;
>>>> +        phy-names = "usb2", "usb3";
>>>
>>> This looks quite unusual, usually the *HCI controllers would be
>>> consumers of the PHY and the PHY may be a consumer of the reset controller.
>>>
>>> (still going through my emails have not fully read your separate email
>>> on the topic, so pardon me if this is being discussed twice).
>>
>> I agree, it's the the best solution I found for this specific design.
>>
>> This specific hw block perform various operations before, in the middle and
>> after PHY initialization. That made me make reset controlller initialize PHYs.
>>
>> I'm happy to implement a more proper design if someone can just suggest how.
>> I don't have any better idea :(
> 
> So the reset controller block has more than just resets? I'd hide all 
> this in the phy driver rather than hide the phy in the reset driver. So 
> for DT provide the phy a phandle to the reset block to poke the 
> registers directly.

Rafal was pointed to drivers/phy/broadcom/phy-brcm-usb-init.c which
should be a good starting point for driving the 4908 USB PHY. From there
the USB PHY may optionally be a consumer of a reset controller if it
needs to, not clear if that is necessary.
-- 
Florian