mbox series

[v3,0/5] Add interconnect driver for IPQ5332 SoC

Message ID 20240722055539.2594434-1-quic_varada@quicinc.com
Headers show
Series Add interconnect driver for IPQ5332 SoC | expand

Message

Varadarajan Narayanan July 22, 2024, 5:55 a.m. UTC
Enable icc-clk based interconnect driver for IPQ5332. This is
similar to IPQ9574's icc-clk based driver.

dt_bindings_check and dtbs_check passed.

Ensured that icc_sync_state is called and relevant clocks are
disabled.

v3: Not taking Reviewed-By: Krzysztof, due to minor change in file

    Add 'clk: qcom: ipq5332: Register gcc_qdss_tsctr_clk_src' to fix
    gpll4_main's CLK_IGNORE_UNUSED issue.

v2: Removed dependency as it is merged
    dt-bindings update to accommodate USB clock names change
    Use icc-clk for USB also

v1:
Dependency:
[1] https://lore.kernel.org/linux-arm-msm/20240430064214.2030013-1-quic_varada@quicinc.com/


Varadarajan Narayanan (5):
  dt-bindings: interconnect: Add Qualcomm IPQ5332 support
  dt-bindings: usb: qcom,dwc3: Update ipq5332 clock details
  clk: qcom: ipq5332: Register gcc_qdss_tsctr_clk_src
  clk: qcom: ipq5332: Use icc-clk for enabling NoC related clocks
  arm64: dts: qcom: ipq5332: Add icc provider ability to gcc

 .../bindings/clock/qcom,ipq5332-gcc.yaml      |  2 +
 .../devicetree/bindings/usb/qcom,dwc3.yaml    | 17 ++++++-
 arch/arm64/boot/dts/qcom/ipq5332.dtsi         |  7 ++-
 drivers/clk/qcom/gcc-ipq5332.c                | 36 ++++++++++-----
 .../dt-bindings/interconnect/qcom,ipq5332.h   | 46 +++++++++++++++++++
 5 files changed, 94 insertions(+), 14 deletions(-)
 create mode 100644 include/dt-bindings/interconnect/qcom,ipq5332.h

Comments

Krzysztof Kozlowski July 22, 2024, 6:07 a.m. UTC | #1
On 22/07/2024 07:55, Varadarajan Narayanan wrote:
> Add interconnect-cells to clock provider so that it can be
> used as icc provider.
> 
> Add master/slave ids for Qualcomm IPQ5332 Network-On-Chip
> interfaces. This will be used by the gcc-ipq5332 driver
> for providing interconnect services using the icc-clk
> framework.
> 
> Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
> ---
> v3: Not taking Reviewed-By: Krzysztof, fixed copy paste error
>     INTERCONNECT_QCOM_IPQ9574_H -> INTERCONNECT_QCOM_IPQ5332_H

Really? You expect re-doing review because of guard typo? The guard name
does not matter. Read submitting patches before posting.

<form letter>
This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

If a tag was not added on purpose, please state why and what changed.
</form letter>

Best regards,
Krzysztof
Varadarajan Narayanan July 22, 2024, 9:11 a.m. UTC | #2
On Mon, Jul 22, 2024 at 11:24:33AM +0300, Dmitry Baryshkov wrote:
> On Mon, Jul 22, 2024 at 11:25:37AM GMT, Varadarajan Narayanan wrote:
> > gcc_qdss_tsctr_clk_src (enabled in the boot loaders and dependent
> > on gpll4_main) was not registered as one of the ipq5332 clocks.
> > Hence clk_disable_unused() disabled 'gpll4_main' assuming there
> > were no consumers for 'gpll4_main' resulting in system freeze or
> > reboots.
> >
> > After registering gcc_qdss_tsctr_clk_src, CLK_IGNORE_UNUSED can
> > be removed from gpll4_main.
>
> Commented below.
>
> >
> > Signed-off-by: Varadarajan Narayanan <quic_varada@quicinc.com>
>
> Fixes?

Will add that. Was hesitant to add as it would point to the approx
3800 line commit (3d89d52970fd) that introduced the gcc-ipq5332.c file.

> > ---
> >  drivers/clk/qcom/gcc-ipq5332.c | 12 +-----------
> >  1 file changed, 1 insertion(+), 11 deletions(-)
> >
> > diff --git a/drivers/clk/qcom/gcc-ipq5332.c b/drivers/clk/qcom/gcc-ipq5332.c
> > index f98591148a97..237b6a766179 100644
> > --- a/drivers/clk/qcom/gcc-ipq5332.c
> > +++ b/drivers/clk/qcom/gcc-ipq5332.c
> > @@ -126,17 +126,6 @@ static struct clk_alpha_pll gpll4_main = {
> >  			.parent_data = &gcc_parent_data_xo,
> >  			.num_parents = 1,
> >  			.ops = &clk_alpha_pll_stromer_ops,
> > -			/*
> > -			 * There are no consumers for this GPLL in kernel yet,
> > -			 * (will be added soon), so the clock framework
> > -			 * disables this source. But some of the clocks
> > -			 * initialized by boot loaders uses this source. So we
> > -			 * need to keep this clock ON. Add the
> > -			 * CLK_IGNORE_UNUSED flag so the clock will not be
> > -			 * disabled. Once the consumer in kernel is added, we
> > -			 * can get rid of this flag.
> > -			 */
> > -			.flags = CLK_IGNORE_UNUSED,
>
> You can't drop it in this patch, since GPLL4 still can get disabled if
> GCC_QDSS_TSCTR_CLK_SRC gets disabled. This chunk should go to the next
> patch (or you should reorder the patches).

Ok, will move the above chunk to next patch.

Thanks
Varada

> >  		},
> >  	},
> >  };
> > @@ -3388,6 +3377,7 @@ static struct clk_regmap *gcc_ipq5332_clocks[] = {
> >  	[GCC_QDSS_DAP_DIV_CLK_SRC] = &gcc_qdss_dap_div_clk_src.clkr,
> >  	[GCC_QDSS_ETR_USB_CLK] = &gcc_qdss_etr_usb_clk.clkr,
> >  	[GCC_QDSS_EUD_AT_CLK] = &gcc_qdss_eud_at_clk.clkr,
> > +	[GCC_QDSS_TSCTR_CLK_SRC] = &gcc_qdss_tsctr_clk_src.clkr,
> >  	[GCC_QPIC_AHB_CLK] = &gcc_qpic_ahb_clk.clkr,
> >  	[GCC_QPIC_CLK] = &gcc_qpic_clk.clkr,
> >  	[GCC_QPIC_IO_MACRO_CLK] = &gcc_qpic_io_macro_clk.clkr,
> > --
> > 2.34.1
> >
>
> --
> With best wishes
> Dmitry