On 25.04.2024 12:26 PM, Varadarajan Narayanan wrote: > On Tue, Apr 23, 2024 at 02:58:41PM +0200, Konrad Dybcio wrote: >> >> >> On 4/18/24 11:23, Varadarajan Narayanan wrote: >>> IPQ SoCs dont involve RPM in managing NoC related clocks and >>> there is no NoC scaling. Linux itself handles these clocks. >>> However, these should not be exposed as just clocks and align >>> with other Qualcomm SoCs that handle these clocks from a >>> interconnect provider. >>> >>> Hence include icc provider capability to the gcc node so that >>> peripherals can use the interconnect facility to enable these >>> clocks. >>> >>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> >>> Signed-off-by: Varadarajan Narayanan <quic_varada@xxxxxxxxxxx> >>> --- >> >> If this is all you do to enable interconnect (which is not the case, >> as this patch only satisfies the bindings checker, the meaningful >> change happens in the previous patch) and nothing explodes, this is >> an apparent sign of your driver doing nothing. > > It appears to do nothing because, we are just enabling the clock > provider to also act as interconnect provider. Only when the > consumers are enabled with interconnect usage, this will create > paths and turn on the relevant NOC clocks. No, with sync_state it actually does "something" (sets the interconnect path bandwidths to zero). And *this* patch does nothing functionally, it only makes the dt checker happy. > > This interconnect will be used by the PCIe and NSS blocks. When > those patches were posted earlier, they were put on hold until > interconnect driver is available. > > Once this patch gets in, PCIe for example will make use of icc. > Please refer to https://lore.kernel.org/linux-arm-msm/20230519090219.15925-5-quic_devipriy@xxxxxxxxxxx/. > > The 'pcieX' nodes will include the following entries. > > interconnects = <&gcc MASTER_ANOC_PCIE0 &gcc SLAVE_ANOC_PCIE0>, > <&gcc MASTER_SNOC_PCIE0 &gcc SLAVE_SNOC_PCIE0>; > interconnect-names = "pcie-mem", "cpu-pcie"; Okay. What about USB that's already enabled? And BIMC/MEMNOC? > >> The expected reaction to "enabling interconnect" without defining the >> required paths for your hardware would be a crash-on-sync_state, as all >> unused (from Linux's POV) resources ought to be shut down. >> >> Because you lack sync_state, the interconnects silently retain the state >> that they were left in (which is not deterministic), and that's precisely >> what we want to avoid. > > I tried to set 'sync_state' to icc_sync_state to be invoked and > didn't see any crash. Have you confirmed that the registers are actually written to, and with correct values? Konrad
