| 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
| ||
|
Message-ID: <20240107030247.GA3416@thinkpad>
Date: Sun, 7 Jan 2024 08:32:47 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Frank Li <Frank.li@....com>
Cc: krzysztof.kozlowski@...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, robh@...nel.org, s.hauer@...gutronix.de,
shawnguo@...nel.org
Subject: Re: [PATCH v7 01/16] PCI: imx6: Simplify clock handling by using
bulk_clk_*() function
On Sat, Jan 06, 2024 at 11:48:35AM -0500, Frank Li wrote:
> On Sat, Jan 06, 2024 at 08:57:08PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Dec 27, 2023 at 01:27:12PM -0500, Frank Li wrote:
> >
> > Subject mentions, 'bulk_clk' APIs but there is no such thing. It should be
> > 'clk_bulk()' APIs.
> >
> > > Refactors the clock handling logic. Adds clk_names[] define in drvdata.
> > > Using clk_bulk*() api simplifies the code.
> > >
> >
> > I've mentioned this many times in the past. But let me reiterate here again:
> >
> > Commit message should be in imperative mood as per Linux Kernel rules for
> > submitting patches. Please see here:
> > Documentation/process/submitting-patches.rst
> >
> > The relevant part is:
> >
> > "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> > to do frotz", as if you are giving orders to the codebase to change
> > its behaviour."
> >
> > Please use this same format for rest of the patches as well for future ones.
>
> I may have not understand *imperative mode*. Asked an English native
> speaker. Do you menas
>
> *Refector* the clock handling logic. *Add* clk_names[] define in drvdata.
> *Use* clk_bulk*() api *simplify* the code.
Yes!
>
> >
> > > Signed-off-by: Frank Li <Frank.Li@....com>
> > > ---
> > >
> > > Notes:
> > > Change from v4 to v5
> > > - update commit message
> > > - direct using clk name list, instead of macro
> > > - still keep caculate clk list count because sizeof return pre allocated
> > > array size.
> > >
> > > Change from v3 to v4
> > > - using clk_bulk_*() API
> > > Change from v1 to v3
> > > - none
> > >
> > > drivers/pci/controller/dwc/pci-imx6.c | 125 ++++++++------------------
> > > 1 file changed, 35 insertions(+), 90 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 74703362aeec7..50d9faaa17f71 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
[...]
> > >
> > > - /* Fetch clocks */
> > > - imx6_pcie->pcie_bus = devm_clk_get(dev, "pcie_bus");
> > > - if (IS_ERR(imx6_pcie->pcie_bus))
> > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_bus),
> > > - "pcie_bus clock source missing or invalid\n");
> > > + while (imx6_pcie->drvdata->clk_names[imx6_pcie->clks_cnt]) {
> > > + int i = imx6_pcie->clks_cnt;
> >
> > Why can't you initialize i to 0 directly?
>
> can't init i to 0 directly here, otherwise next loop i will not increase.
> i just reduce refer imx6_pcie->clks_cnt in
>
> imx6_pcie->clks[i].id = imx6_pcie->drvdata->clk_names[i];
>
Wait... Can't you just use ARRAY_SIZE() to calculate the clks_cnt statically?
Like,
static const char * const imx8_clk_names[] = {
"pcie_bus", "pcie", "pcie_aux",
};
[...]
.clk_names = imx8_clk_names,
.clks_cnt = ARRAY_SIZE(imx8_clk_names),
You can use the same clk_names array for multiple SoCs if the clocks are same.
I should've mentioned this in last review itself. Sorry about that.
- Mani
> Frank
>
> >
> > Rest looks good to me.
> >
> > - Mani
> >
> > > +
> > > + imx6_pcie->clks[i].id = imx6_pcie->drvdata->clk_names[i];
> > > + imx6_pcie->clks_cnt++;
> > > + }
> > >
> > > - imx6_pcie->pcie = devm_clk_get(dev, "pcie");
> > > - if (IS_ERR(imx6_pcie->pcie))
> > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie),
> > > - "pcie clock source missing or invalid\n");
> > > + /* Fetch clocks */
> > > + ret = devm_clk_bulk_get(dev, imx6_pcie->clks_cnt, imx6_pcie->clks);
> > > + if (ret)
> > > + return ret;
> > >
> > > switch (imx6_pcie->drvdata->variant) {
> > > - case IMX6SX:
> > > - imx6_pcie->pcie_inbound_axi = devm_clk_get(dev,
> > > - "pcie_inbound_axi");
> > > - if (IS_ERR(imx6_pcie->pcie_inbound_axi))
> > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_inbound_axi),
> > > - "pcie_inbound_axi clock missing or invalid\n");
> > > - break;
> > > - case IMX8MQ:
> > > - case IMX8MQ_EP:
> > > - imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> > > - if (IS_ERR(imx6_pcie->pcie_aux))
> > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> > > - "pcie_aux clock source missing or invalid\n");
> > > - fallthrough;
> > > case IMX7D:
> > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
> > > imx6_pcie->controller_id = 1;
> > > @@ -1353,10 +1300,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > > case IMX8MM_EP:
> > > case IMX8MP:
> > > case IMX8MP_EP:
> > > - imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> > > - if (IS_ERR(imx6_pcie->pcie_aux))
> > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> > > - "pcie_aux clock source missing or invalid\n");
> > > imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev,
> > > "apps");
> > > if (IS_ERR(imx6_pcie->apps_reset))
> > > @@ -1372,14 +1315,6 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > > default:
> > > break;
> > > }
> > > - /* Don't fetch the pcie_phy clock, if it has abstract PHY driver */
> > > - if (imx6_pcie->phy == NULL) {
> > > - imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
> > > - if (IS_ERR(imx6_pcie->pcie_phy))
> > > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_phy),
> > > - "pcie_phy clock source missing or invalid\n");
> > > - }
> > > -
> > >
> > > /* Grab turnoff reset */
> > > imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");
> > > @@ -1477,6 +1412,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
> > > .dbi_length = 0x200,
> > > .gpr = "fsl,imx6q-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > > },
> > > [IMX6SX] = {
> > > .variant = IMX6SX,
> > > @@ -1484,6 +1420,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE |
> > > IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > > .gpr = "fsl,imx6q-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_inbound_axi"},
> > > },
> > > [IMX6QP] = {
> > > .variant = IMX6QP,
> > > @@ -1492,40 +1429,48 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> > > IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > > .dbi_length = 0x200,
> > > .gpr = "fsl,imx6q-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > > },
> > > [IMX7D] = {
> > > .variant = IMX7D,
> > > .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > > .gpr = "fsl,imx7d-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_phy"},
> > > },
> > > [IMX8MQ] = {
> > > .variant = IMX8MQ,
> > > .gpr = "fsl,imx8mq-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> > > },
> > > [IMX8MM] = {
> > > .variant = IMX8MM,
> > > .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > },
> > > [IMX8MP] = {
> > > .variant = IMX8MP,
> > > .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND,
> > > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > },
> > > [IMX8MQ_EP] = {
> > > .variant = IMX8MQ_EP,
> > > .mode = DW_PCIE_EP_TYPE,
> > > .gpr = "fsl,imx8mq-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_phy", "pcie_aux"},
> > > },
> > > [IMX8MM_EP] = {
> > > .variant = IMX8MM_EP,
> > > .mode = DW_PCIE_EP_TYPE,
> > > .gpr = "fsl,imx8mm-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > },
> > > [IMX8MP_EP] = {
> > > .variant = IMX8MP_EP,
> > > .mode = DW_PCIE_EP_TYPE,
> > > .gpr = "fsl,imx8mp-iomuxc-gpr",
> > > + .clk_names = {"pcie_bus", "pcie", "pcie_aux"},
> > > },
> > > };
> > >
> > > --
> > > 2.34.1
> > >
> >
> > --
> > மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists