Re: [PATCH v2 2/4] dt-bindings: phy: nuvoton,ma35d1-usb2-phy: extend for dual-port OTG support

[Joey Lu <a0987203069@gmail.com>]

On 6/30/2026 2:16 PM, Krzysztof Kozlowski wrote:
> On 29/06/2026 12:40, Joey Lu wrote:
>> On 6/25/2026 3:58 PM, Krzysztof Kozlowski wrote:
>>> On Thu, Jun 25, 2026 at 10:39:56AM +0800, Joey Lu wrote:
>>>>    properties:
>>>>      compatible:
>>>>        enum:
>>>>          - nuvoton,ma35d1-usb2-phy
>>>>    
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>>      "#phy-cells":
>>>> -    const: 0
>>>> +    const: 1
>>>> +    description:
>>>> +      The single cell selects the PHY port. 0 selects the OTG port (USB0,
>>>> +      shared with DWC2 gadget controller) and 1 selects the host-only port
>>>> +      (USB1).
>>>>    
>>>> -  clocks:
>>>> -    maxItems: 1
>>> This is odd, considering that parent does not have clocks. So explain me
>>> this:
>>> 1. USB PHY needed clocks.
>>> 2. You extend USB PHY to cover second part.
>>> 3. That extension for second part means that clocks are not needed.
>>> Really, how? How is it possible in hardware?
>> The hardware has two independent clock domains:
>>
>>     - The PHY analog block takes the 24 MHz HXT as its reference, wired
>>       directly to the PHY's internal PLL, which derives the required
>> operating
>>       frequencies internally. This reference path is entirely outside the SoC
>>       software clock tree; no software-gatable clock gate needs to be enabled
>>       for the PHY to power up and lock its PLL. The only software control the
>>       PHY driver exercises is toggling each PHY's Power-On Reset (POR) bit,
>>       which resides in the SYS register block. The driver accesses this via
>>       the parent regmap
>>
>>     - `HUSBH0_GATE` / `HUSBH1_GATE` / `USBD_GATE` are AHB/APB bus interface
>>       clocks for the host and gadget (EHCI, OHCI, DWC2). They gate
>>       the register-access path between the CPU and each controller, not
>> the PHY
>>       analog circuitry itself.
>>
>> The original single-port driver enabled `HUSBH0_GATE` as if it belonged
>> to the
>> PHY, but that gate is actually owned by EHCI0/OHCI0 and is already
>> managed by
>> those controller drivers through their own `clocks` DTS bindings. The PHY
>> driver was redundantly enabling the same gate.
>>
>> When extending the driver to cover PHY1, the same pattern held: EHCI1/OHCI1
>> manage `HUSBH1_GATE` themselves. There is no clock that belongs
>> exclusively to
>> the PHY, so `clocks` will be dropped from the PHY binding entirely.
> What driver has to do with it?
>
> You did not answer the question. How adding missing OTG to existing
> device causes that hardware to lose a clock? How is it possible?
>
To answer the hardware question directly: adding OTG support does not
cause the hardware to lose a clock. The hardware clock topology is
identical in both the single-port and dual-port cases.

The PHY analog block derives its reference from the 24 MHz HXT crystal
through an always-on path via the PHY's internal PLL. No software-gated
clock in the kernel clock tree sits in this path. The PHY's power-on and
reset state is managed through USBPMISCR in the SYS register block, which
the driver accesses via the nuvoton,sys regmap outside the clock tree.

`HUSBH0_GATE` gates the AHB bus interface to the EHCI0/OHCI0 host
controllers that share PHY0. This relationship is the same in both the
single-port and dual-port configurations.
>>>> +  nuvoton,rcalcode:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>>> +    minItems: 1
>>>> +    maxItems: 2
>>> You should require two values. I understand that any PHY is optional,
>>> thus you skip the entry, so how would you provide value for PHY1 only?
>> `nuvoton,rcalcode` will be changed to require exactly two values
>> (`minItems: 2, maxItems: 2`), one for PHY0 and one for PHY1 respectively.
>> The property will remain optional overall; when absent, each port
>> retains its
>> power-on default value loaded at hardware initialisation. When present, both
>> entries must be supplied.
> So are you going to implement it or not?
>
Yes, I will implement this change in the next version of the patchset.
>>>> +    items:
>>>> +      minimum: 0
>>>> +      maximum: 15
>>>> +    description:
>>>> +      Resistor calibration trim codes for PHY0 and PHY1 respectively.
>>>> +      Each 4-bit value is written to the RCALCODE field in USBPMISCR and
>>>> +      adjusts the PHY's internal termination resistance. Both entries are
>>>> +      optional; when absent the hardware reset default is used.
>>>>    
>>>> -  nuvoton,sys:
>>>> -    $ref: /schemas/types.yaml#/definitions/phandle
>>>> +  nuvoton,oc-active-high:
>>>> +    type: boolean
>>>>        description:
>>>> -      phandle to syscon for checking the PHY clock status.
>>>> +      When present, the over-current detect input from the VBUS power switch
>>>> +      is treated as active-high. The default (property absent) is active-low.
>>>> +      This setting is shared by both USB host ports.
>>>>    
>>>>    required:
>>>>      - compatible
>>>> +  - reg
>>> That's ABI break which was not explained in the commit msg - neither
>>> specifying impact nor actually providing reasons why you break ABI.
>>>
>>> And honestly, you have no resources here except the address, so now it
>>> is clear that this should be folded into parent. See DTS101 talk slides.
>> The commit message will be updated to explicitly acknowledge the ABI break:
>> existing DTS files that contain a standalone `usb-phy` node without a `reg`
>> property will fail dt-schema validation after this change. The impact is
>> limited to the MA35D1 SoC; no upstream DTS for this SoC existed before this
>> patch series, so no in-tree board files are broken. The break is intentional
> But all of out of tree users are broken.
On reflection, the approach is being revised to eliminate the ABI break 
entirely.
The next revision will keep the existing `nuvoton,ma35d1-usb2-phy` binding
intact with all its current required properties (`compatible`, `clocks`,
`nuvoton,sys`, `#phy-cells`). New OTG and dual-port capability will be
expressed as strictly optional backward-compatible additions:

- `#phy-cells` updated from `const: 0` to `enum: [0, 1]`. Existing DTS
   with `#phy-cells = <0>` continues to validate and behave as before.
   New DTS opting into dual-port uses `#phy-cells = <1>`.
- `nuvoton,rcalcode`: optional, exactly two values when present.
- `nuvoton,oc-active-high`: optional boolean.
>
> Best regards,
> Krzysztof