Received-SPF: pass (google.com: domain of linux-kernel+bounces-68957-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240122-ipq5332-nsscc-v4-0-19fa30019770@quicinc.com>
 <20240122-ipq5332-nsscc-v4-2-19fa30019770@quicinc.com> <7a69a68d-44c2-4589-b286-466d2f2a0809@lunn.ch>
 <11fda059-3d8d-4030-922a-8fef16349a65@quicinc.com> <17e2400e-6881-4e9e-90c2-9c4f77a0d41d@lunn.ch>
 <8c9ee34c-a97b-4acf-a093-9ac2afc28d0e@quicinc.com>
In-Reply-To: <8c9ee34c-a97b-4acf-a093-9ac2afc28d0e@quicinc.com>
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Date: Fri, 16 Feb 2024 18:26:31 +0200
Message-ID: <CAA8EJppe6aNf2WJ5BvaX8SPTbuaEwzRm74F8QKyFtbmnGQt=1w@mail.gmail.com>
Subject: Re: [PATCH v4 2/8] clk: qcom: ipq5332: enable few nssnoc clocks in
 driver probe
To: Kathiravan Thirumoorthy <quic_kathirav@quicinc.com>
Cc: Andrew Lunn <andrew@lunn.ch>, Bjorn Andersson <andersson@kernel.org>, 
	Konrad Dybcio <konrad.dybcio@linaro.org>, Michael Turquette <mturquette@baylibre.com>, 
	Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Richard Cochran <richardcochran@gmail.com>, Catalin Marinas <catalin.marinas@arm.com>, 
	Will Deacon <will@kernel.org>, linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org, 
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, 
	netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"

On Fri, 16 Feb 2024 at 17:33, Kathiravan Thirumoorthy
<quic_kathirav@quicinc.com> wrote:
>
>
>
> On 2/14/2024 8:14 PM, Andrew Lunn wrote:
> > On Wed, Feb 14, 2024 at 02:49:41PM +0530, Kathiravan Thirumoorthy wrote:
> >>
> >>
> >> On 1/26/2024 1:35 AM, Andrew Lunn wrote:
> >>> On Mon, Jan 22, 2024 at 11:26:58AM +0530, Kathiravan Thirumoorthy wrote:
> >>>> gcc_snoc_nssnoc_clk, gcc_snoc_nssnoc_1_clk, gcc_nssnoc_nsscc_clk are
> >>>> enabled by default and it's RCG is properly configured by bootloader.
> >>>
> >>> Which bootloader? Mainline barebox?
> >>
> >>
> >> Thanks for taking time to review the patches. I couldn't get time to respond
> >> back, sorry for the delay.
> >>
> >> I was referring to the U-boot which is delivered as part of the QSDK. I will
> >> call it out explicitly in the next patch.
> >
> > I've never used QSDK u-boot, so i can only make comments based on my
> > experience with other vendors build of u-boot. That experience is, its
> > broken for my use cases, and i try to replace it as soon as possible
> > with upstream.
> >
> > I generally want to TFTP boot the kernel and the DT blob. Sometimes
> > vendor u-boot has networking disabled. Or the TFTP client is
> > missing. If it is there, the IP addresses are fixed, and i don't want
> > to modify my network to make it compatible with the vendor
> > requirements. If the IP addresses can be configured, sometimes there
> > is no FLASH support so its not possible to actually write the
> > configuration to FLASH so that it does the right thing on reboot
> > etc...
> >
> > Often the vendor u-boot is a black box, no sources. Can you give me a
> > git URL for the u-boot in QSDK? If the sources are open, i could at
> > least rebuild it with everything turned on.
>
>
> You can get the source at
> https://git.codelinaro.org/clo/qsdk/oss/boot/u-boot-2016/-/tree/NHSS.QSDK.12.2?ref_type=heads
>
> You should be able to TFTP the images, write into the flash and
> configure the IP and so on...
>
>
> >
> > But still, it is better that Linux makes no assumptions about what the
> > boot loader has done. That makes it much easier to change the
> > bootloader.
> >
> >>>> Some of the NSS clocks needs these clocks to be enabled. To avoid
> >>>> these clocks being disabled by clock framework, drop these entries
> >>>> from the clock table and enable it in the driver probe itself.
> >>>
> >>> If they are critical clocks, i would expect a device to reference
> >>> them. The CCF only disabled unused clocks in late_initcall_sync(),
> >>> which means all drivers should of probed and taken a reference on any
> >>> clocks they require.
> >>
> >>
> >> Some of the NSSCC clocks are enabled by bootloaders and CCF disables the
> >> same (because currently there are no consumers for these clocks available in
> >> the tree. These clocks are consumed by the Networking drivers which are
> >> being upstreamed).
> >
> > If there is no network drivers, you don't need clocks to the
> > networking hardware. So CCF turning them off seems correct.
>
>
> Yeah agree with your comments.
>
> QSDK's u-boot enables the network support, so the required NSSCC clocks
> are turned ON and left it in ON state. CCF tries to disables the unused
> NSSCC clocks but system goes for reboot.
>
>
> Reason being, to access the NSSCC clocks, these GCC clocks
> (gcc_snoc_nssnoc_clk, gcc_snoc_nssnoc_1_clk, gcc_nssnoc_nsscc_clk)
> should be turned ON. But CCF disables these clocks as well due to the
> lack of consumer.

This means that NSSCC is also a consumer of those clocks. Please fix
both DT and nsscc driver to handle NSSNOC clocks.

> > Once you have actual drivers, this should solve itself, the drivers
> > will consume the clocks.
>
>
> Given that, NSSCC is being built as module, there is no issue in booting
> the kernel. But if you do insmod of the nsscc-ipq5332.ko, system will
> reset.
>
> Without the networking drivers, there is no need to install this module.
> And as you stated, once the drivers are available, there will be no issues.
>
> So can I explain the shortcomings of installing this module without the
> networking drivers in cover letter and drop this patch all together?

No. Using allyesconfig or allmodconfig and installing the full modules
set should work.

> >> However looking back, gcc_snoc_nssnoc_clk, gcc_snoc_nssnoc_1_clk,
> >> gcc_nssnoc_nsscc_clk are consumed by the networking drivers only. So is it
> >> okay to drop these clocks from the GCC driver and add it back once the
> >> actual consumer needs it?
> >
> > But why should you remove them. If nothing is using them, they should
> > be turned off.
> >
> >     Andrew
>


-- 
With best wishes
Dmitry