linux-kernel - Re: [PATCH v3 01/13] PCI: imx6: Simplify clock handling by using HAS_CLK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 12 Dec 2023 18:18:55 -0500
From:   Frank Li <Frank.li@....com>
To:     Rob Herring <robh@...nel.org>
Cc:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        bhelgaas@...gle.com, conor+dt@...nel.org,
        devicetree@...r.kernel.org, festevam@...il.com, helgaas@...nel.org,
        hongxing.zhu@....com, imx@...ts.linux.dev, kernel@...gutronix.de,
        krzysztof.kozlowski+dt@...aro.org, kw@...ux.com,
        l.stach@...gutronix.de, linux-arm-kernel@...ts.infradead.org,
        linux-imx@....com, linux-kernel@...r.kernel.org,
        linux-pci@...r.kernel.org, lpieralisi@...nel.org,
        s.hauer@...gutronix.de, shawnguo@...nel.org
Subject: Re: [PATCH v3 01/13] PCI: imx6: Simplify clock handling by using
 HAS_CLK_* bitmask

On Tue, Dec 12, 2023 at 04:54:26PM -0600, Rob Herring wrote:
> On Tue, Dec 12, 2023 at 10:19:13PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Dec 11, 2023 at 04:58:30PM -0500, Frank Li wrote:
> > > Refactors the clock handling logic in the imx6 PCI driver by adding
> > > HAS_CLK_* bitmask define for drvdata::flags . Simplifies the code and makes
> > > it more maintainable, as future additions of SOC support will only require
> > > straightforward changes. The drvdata::flags and a bitmask ensures a cleaner
> > > and more scalable switch-case structure for handling clocks.
> > > 
> > 
> > Is there any necessity to validate each clock in the driver? I mean, can't you
> > just rely on devicetree to provide enough clocks for the functioning of the PCIe
> > controller?
> > 
> > If you can rely on devicetree (everyone should in an ideal world), then you can
> > just use devm_clk_bulk_get_all() to get all available clocks for the SoC and
> > just enable/disable whatever is available.
> 
> Or just use the *_get_optional() variants of functions. They return NULL 
> such that subsequent calls are just NOPs if the resource is not present. 
> Of course, they aren't really optional on any given platform in this 
> case, but does that really matter.

I think it'd better use devm_clk_bulk_get() by passed down a clk name list
.clk_names = {"pcie_aux", ...},

So it will not silient when dts are not matched requirement. It is not
complex because only check once at probe.

Frank

> 
> There isn't an optional variant for phys, but it can be added.
> 
> > 
> > This will greatly simplify the code.
> > 
> > Only downside of this approach is, if the devicetree is not supplying enough
> > clocks, then it will be difficult to find why PCIe is not working. But this also
> > means that the devicetree is screwed.
> 
> A sufficient schema should prevent that... That's what they're for, not 
> just torturing people to learn json-schema. :)
> 
> Rob