linux-kernel - Re: [PATCH v2 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver

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:   Fri, 10 Jun 2022 10:30:20 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Wangseok Lee <wangseok.lee@...sung.com>
Cc:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
        "kishon@...com" <kishon@...com>,
        "vkoul@...nel.org" <vkoul@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jesper.nilsson@...s.com" <jesper.nilsson@...s.com>,
        "lars.persson@...s.com" <lars.persson@...s.com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
        "kw@...ux.com" <kw@...ux.com>,
        "linux-arm-kernel@...s.com" <linux-arm-kernel@...s.com>,
        "kernel@...s.com" <kernel@...s.com>,
        Moon-Ki Jun <moonki.jun@...sung.com>,
        Sang Min Kim <hypmean.kim@...sung.com>,
        Dongjin Yang <dj76.yang@...sung.com>,
        Yeeun Kim <yeeun119.kim@...sung.com>
Subject: Re: [PATCH v2 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver

On Fri, Jun 10, 2022 at 09:03:03AM +0900, Wangseok Lee wrote:
> On 06/04/2022 01:03, Bjorn Helgaas wrote:
> > In the subject, why do you tag this "axis"?  There's an existing
> > pcie-artpec6.c that uses the driver name ""artpec6-pcie" and the
> > subject line tag "artpec6".
> > 
> > This adds pcie-artpec8.c with driver name "artpec8-pcie", so the
> > obvious choice would be "artpec8".
> > 
> > I assume you evaluated the possibility of extending artpec6 to support
> > artpec8 in addition to the artpec6 and artpec7 it already supports?
>  
> "pcie-artpec6. c" supports artpec6 and artpec7 H/W.
> artpec8 can not be expanded because H/W configuration is
> completely different from artpec6/7.
> phy and sub controller are different.

Thanks for this detail.  Can you include this in the commit log next
time around in case anybody else has a similar question?

> >> +/* FSYS SYSREG Offsets */
> > 
> > The list below seems to inclue more than just register offsets.
> > 
>  
> Is it clear to change to "FSYS blue logic system registers" 
> like Jasper Nilsson`s comment?
> https://lore.kernel.org/all/20220607070332.GY18902@axis.com/
> My opinion is the same.

Yep, that's fine.  But spell it "glue logic", not "blue logic" :)

> >> +static int artpec8_pcie_get_clk_resources(struct platform_device *pdev,
> >> +                                       struct artpec8_pcie *artpec8_ctrl)
> >> +{
> >> +        struct device *dev = &pdev->dev;
> >> +
> >> +        artpec8_ctrl->pipe_clk = devm_clk_get(dev, "pipe_clk");
> >> +        if (IS_ERR(artpec8_ctrl->pipe_clk)) {
> >> +                dev_err(dev, "couldn't get pipe clock\n");
> >> +                return -EINVAL;
> >> +        }
> >> +
> >> +        artpec8_ctrl->dbi_clk = devm_clk_get(dev, "dbi_clk");
> >> +        if (IS_ERR(artpec8_ctrl->dbi_clk)) {
> >> +                dev_info(dev, "couldn't get dbi clk\n");
> >> +                return -EINVAL;
> >> +        }
> >> +
> >> +        artpec8_ctrl->slv_clk = devm_clk_get(dev, "slv_clk");
> >> +        if (IS_ERR(artpec8_ctrl->slv_clk)) {
> >> +                dev_err(dev, "couldn't get slave clock\n");
> >> +                return -EINVAL;
> >> +        }
> >> +
> >> +        artpec8_ctrl->mstr_clk = devm_clk_get(dev, "mstr_clk");
> >> +        if (IS_ERR(artpec8_ctrl->mstr_clk)) {
> >> +                dev_info(dev, "couldn't get master clk\n");
> > 
> > It'd be nice if the err/info messages matched the exact DT name:
> > "pipe_clk", "dbi_clk", slv_clk", etc.
> > 
>  
> I will fix it.
>  
> > Why are some of the above dev_err() and others dev_info() when you
> > return -EINVAL in all cases?
>  
> When property is not found, it just to return error.
> I will modify to return PTR_ERR.

Using PTR_ERR() looks like a good idea, since then you return the
actual error from devm_clk_get() instead of always returning -EINVAL.

But that wasn't my comment.  My comment was that it looks like these
should be all dev_err() (or all dev_info()).

> >> +        switch (mode) {
> >> +        case DW_PCIE_RC_TYPE:
> >> +                artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_RC,
> >> +                                PCIE_ARTPEC8_DEVICE_TYPE);
> >> +                ret = artpec8_add_pcie_port(artpec8_ctrl, pdev);
> >> +                if (ret < 0)
> > 
> > Are there positive return values that indicate success?  Most places
> > above you assume "ret != 0" means failure, so just curious why you
> > test "ret < 0" instead of just "ret".
>  
> There is no special reason, but it seems that the format used 
> in the existing dw driver is applied.

Fair enough.  "git grep -A2 add_pcie_port drivers/pci/controller/"
says all *_add_pcie_port() calls use the same pattern, so thanks for
following that.

Bjorn