Re: [PATCH V2 5/7] clk: qcom: Add NSS clock Controller driver for IPQ9574

[Dmitry Baryshkov]

On Thu, 5 Oct 2023 at 12:56, Devi Priya <quic_devipriy@xxxxxxxxxxx> wrote:
>
>
>
> On 10/5/2023 12:49 PM, Dmitry Baryshkov wrote:
> > On Thu, 5 Oct 2023 at 09:26, Devi Priya <quic_devipriy@xxxxxxxxxxx> wrote:
> >>
> >>
> >>
> >> On 9/22/2023 5:31 PM, Devi Priya wrote:
> >>>
> >>>
> >>> On 9/20/2023 1:50 PM, Dmitry Baryshkov wrote:
> >>>> On Wed, 20 Sept 2023 at 09:39, Devi Priya <quic_devipriy@xxxxxxxxxxx>
> >>>> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 9/12/2023 7:38 PM, Devi Priya wrote:
> >>>>>>
> >>>>>>
> >>>>>> On 8/25/2023 5:14 PM, Dmitry Baryshkov wrote:
> >>>>>>> On Fri, 25 Aug 2023 at 12:15, Devi Priya <quic_devipriy@xxxxxxxxxxx>
> >>>>>>> wrote:
> >>>>>>>>
> >>>>>>>> Add Networking Sub System Clock Controller(NSSCC) driver for ipq9574
> >>>>>>>> based
> >>>>>>>> devices.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Devi Priya <quic_devipriy@xxxxxxxxxxx>
> >>>>>>>> ---
> >>>>>>>>     Changes in V2:
> >>>>>>>>            - Added depends on ARM64 || COMPILE_TEST in Kconfig
> >>>>>>>>            - Added module_platform_driver
> >>>>>>>>            - Dropped patch [2/6] - clk: qcom: gcc-ipq9574: Mark nssnoc
> >>>>>>>> clocks as critical
> >>>>>>>>               & added pm_clk for nssnoc clocks
> >>>>>>>>            - Updated the uniphy clock names
> >>>>>>>>
> >>>>>>>>     drivers/clk/qcom/Kconfig         |    7 +
> >>>>>>>>     drivers/clk/qcom/Makefile        |    1 +
> >>>>>>>>     drivers/clk/qcom/nsscc-ipq9574.c | 3109
> >>>>>>>> ++++++++++++++++++++++++++++++
> >>>>>>>>     3 files changed, 3117 insertions(+)
> >>>>>>>>     create mode 100644 drivers/clk/qcom/nsscc-ipq9574.c
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> >>>>>>>> index bd9bfb11b328..3ecc11e2c8e3 100644
> >>>>>>>> --- a/drivers/clk/qcom/Kconfig
> >>>>>>>> +++ b/drivers/clk/qcom/Kconfig
> >>>>>>>> @@ -203,6 +203,13 @@ config IPQ_GCC_9574
> >>>>>>>>              i2c, USB, SD/eMMC, etc. Select this for the root clock
> >>>>>>>>              of ipq9574.
> >>>>>>>>
> >>>>>>>> +config IPQ_NSSCC_9574
> >>>>>>>> +       tristate "IPQ9574 NSS Clock Controller"
> >>>>>>>> +       depends on ARM64 || COMPILE_TEST
> >>>>>>>> +       depends on IPQ_GCC_9574
> >>>>>>>> +       help
> >>>>>>>> +         Support for NSS clock controller on ipq9574 devices.
> >>>>>>>> +
> >>>>>>>>     config MSM_GCC_8660
> >>>>>>>>            tristate "MSM8660 Global Clock Controller"
> >>>>>>>>            depends on ARM || COMPILE_TEST
> >>>>>>>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> >>>>>>>> index 4790c8cca426..3f084928962e 100644
> >>>>>>>> --- a/drivers/clk/qcom/Makefile
> >>>>>>>> +++ b/drivers/clk/qcom/Makefile
> >>>>>>>> @@ -30,6 +30,7 @@ obj-$(CONFIG_IPQ_GCC_6018) += gcc-ipq6018.o
> >>>>>>>>     obj-$(CONFIG_IPQ_GCC_806X) += gcc-ipq806x.o
> >>>>>>>>     obj-$(CONFIG_IPQ_GCC_8074) += gcc-ipq8074.o
> >>>>>>>>     obj-$(CONFIG_IPQ_GCC_9574) += gcc-ipq9574.o
> >>>>>>>> +obj-$(CONFIG_IPQ_NSSCC_9574)   += nsscc-ipq9574.o
> >>>>>>>>     obj-$(CONFIG_IPQ_LCC_806X) += lcc-ipq806x.o
> >>>>>>>>     obj-$(CONFIG_MDM_GCC_9607) += gcc-mdm9607.o
> >>>>>>>>     obj-$(CONFIG_MDM_GCC_9615) += gcc-mdm9615.o
> >>>>>>>> diff --git a/drivers/clk/qcom/nsscc-ipq9574.c
> >>>>>>>> b/drivers/clk/qcom/nsscc-ipq9574.c
> >>>>>>>> new file mode 100644
> >>>>>>>> index 000000000000..65bdb449ae5f
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/drivers/clk/qcom/nsscc-ipq9574.c
> >>>>>>>> @@ -0,0 +1,3109 @@
> >>>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
> >>>>>>>> +/*
> >>>>>>>> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
> >>>>>>>> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights
> >>>>>>>> reserved.
> >>>>>>>> + */
> >>>>>>>> +
> >>>>>>>> +#include <linux/clk-provider.h>
> >>>>>>>> +#include <linux/err.h>
> >>>>>>>> +#include <linux/kernel.h>
> >>>>>>>> +#include <linux/module.h>
> >>>>>>>> +#include <linux/of.h>
> >>>>>>>> +#include <linux/of_device.h>
> >>>>>>>> +#include <linux/regmap.h>
> >>>>>>>> +#include <linux/pm_clock.h>
> >>>>>>>> +#include <linux/pm_runtime.h>
> >>>>>>>> +
> >>>>>>>> +#include <dt-bindings/clock/qcom,ipq9574-nsscc.h>
> >>>>>>>> +#include <dt-bindings/reset/qcom,ipq9574-nsscc.h>
> >>>>>>>> +
> >>>>>>>> +#include "clk-alpha-pll.h"
> >>>>>>>> +#include "clk-branch.h"
> >>>>>>>> +#include "clk-pll.h"
> >>>>>>>> +#include "clk-rcg.h"
> >>>>>>>> +#include "clk-regmap.h"
> >>>>>>>> +#include "clk-regmap-divider.h"
> >>>>>>>> +#include "clk-regmap-mux.h"
> >>>>>>>> +#include "common.h"
> >>>>>>>> +#include "reset.h"
> >>>>>>>> +
> >>>>>>>> +/* Need to match the order of clocks in DT binding */
> >>>>>>>> +enum {
> >>>>>>>> +       DT_NSSNOC_NSSCC_CLK,
> >>>>>>>> +       DT_NSSNOC_SNOC_CLK,
> >>>>>>>> +       DT_NSSNOC_SNOC_1_CLK,
> >>>>>>>
> >>>>>>> Not using the index makes it seem that these clocks are not used,
> >>>>>>> until one scrolls down to pm_clks.
> >>>>>> Okay, got it
> >>>>>>>
> >>>>>>> BTW: The NSSNOC_SNOC clocks make it look like there is an interconnect
> >>>>>>> here (not a simple NIU).
> >>>>>>
> >>>>>> Hi Dmitry, We are exploring on the ICC driver. In the meantime to
> >>>>>> unblock PCIe/NSS changes getting merged, shall we use
> >>>>>> regmap_update_bits
> >>>>>> and turn on the critical NSSNOC clocks, ANOC & SNOC pcie clocks in the
> >>>>>> probe function of the gcc driver itself as like sm8550 driver to get
> >>>>>> the
> >>>>>> changes merged?
> >>>>>>
> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/qcom/gcc-sm8550.c#n3347
> >>>>>
> >>>>> Hi Dmitry,
> >>>>> Just curious to know if we could send out the next series with the
> >>>>> proposed approach if that holds good.
> >>>>
> >>>> The answer really depends on the structure of your hardware. The issue
> >>>> is that once you commit the device bindings,you have to support them
> >>>> forever. So, if you commit the NSS clock support without interconnects
> >>>> in place, you have to keep this ANOC/SNOC/etc code forever, even after
> >>>> you land the interconnect. So I'd suggest landing the icc driver first
> >>>> (or at least implementing and sending to the mailing list), so that we
> >>>> can see how all these pieces fit together.
> >>>
> >>> Hi Dmitry,
> >>> Unlike MSM chipsets, IPQ chipsets does not have any use case wherein the
> >>> NOC clocks have to be scaled. So if these clocks can be enabled in the
> >>> probe, there is no need for an interconnect driver at all. The same
> >>> applies to both ipq9574 and ipq5332 SoCs.
> >>>
> >>
> >> Hi Dmitry,
> >> Just curious to know if we can go ahead with the proposed solution of
> >> enabling the NOC clocks in the probe as these clocks need not be scaled
> >> in IPQ chipsets & hence there would be no need for an ICC driver in
> >> ipq9574 & ipq5332 targets.
> >
> > In the probe of which driver?
> GCC driver of ipq9574 & ipq5332 targets.

Would you need to handle these clocks additionally in the
suspend/resume path? Will this increase the power consumption of the
board?

Generally, I'd say this looks like a bad idea. Consider all the
troubles we are undergoing now while sorting out the NIU clocks on RPM
and RPMH platforms.
So, unless you are 100% sure that this is a permanent solution (like
AHB clocks being always on), I'd kindly suggest implementing the NoC
attachment properly. In the end, other Qualcomm platforms use ICC
drivers, so by following this pattern we will have more common code
paths.

-- 
With best wishes
Dmitry