Re: [PATCH v3 05/10] ARM: dts: qcom: msm8960: add RPM clock controller and fix USB clocks

[Konrad Dybcio]

On 6/16/26 3:04 PM, Antony Kurniawan Soemardi wrote:
> On 6/9/2026 7:21 PM, Konrad Dybcio wrote:
>> On 6/1/26 10:51 AM, Antony Kurniawan Soemardi via B4 Relay wrote:
>>> From: Antony Kurniawan Soemardi <linux@xxxxxxxxxxxxxx>
>>> @@ -507,8 +519,12 @@ usb1: usb@12500000 {
>>>               reg = <0x12500000 0x200>,
>>>                     <0x12500200 0x200>;
>>>               interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>;
>>> -            clocks = <&gcc USB_HS1_XCVR_CLK>, <&gcc USB_HS1_H_CLK>;
>>> -            clock-names = "core", "iface";
>>> +            clocks = <&gcc USB_HS1_H_CLK>,
>>> +                 <&rpmcc RPM_DAYTONA_FABRIC_CLK>,
>>> +                 <&gcc USB_HS1_XCVR_CLK>;
>>> +            clock-names = "iface",
>>> +                      "core",
>>> +                      "fs";
>>
>> The bindings change you sent changes the expectations - "core" used
>> to be the first clock. And I would guesstimate that the
>> DAYTONA_FABRIC clock is not really "core" - does downstream do any
>> ratesetting on the other two?
> 
> Looking at the downstream, I can only find HS1_XCVR being set to 60MHz, DAYTONA_FABRIC being set to the max rate (just for voting purposes?). I don't see any clk_set_rate for HS1_P though.
> 
> Would you rather the other way around? Like "core", "iface", and "fs"? My concern is that such a change would result in a large number of warnings for newer SoC device trees.

I didn't notice you're actually aligning the order with bindings. I was
under the impression this was a random change.

For the clock assignments themselves, I think the schema reflects a
full-speed (i.e. usb 1.x) core.. I dug out some ancient doc that says
that we should have:

- ahb (bus clock - perhaps daytona in this case?)
- system (core clock for the thing, >55 Mhz for compliant HS operation
  or at least 35 MHz for any sort of operation)
- ulpi_clk (60 MHz, coming from the USB PHY) (we can probably ignore this
  in our description)
- inactivity_timer since there's a BAM instance attached to this host 
  (possibly handled implicitly)

for the record, there's 4 hosts:

USB1_HS @ 0x12500000 (this one)
USB2_HSIC @ 0x12520000
USB1_FS @ 0x18000000
USB2_FS @ 0x18100000

I don't know if they are all exposed and functional though

Konrad