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:

quoted

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?

quoted

quoted

+/* 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/ (local)
My opinion is the same.

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

quoted

quoted

+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.
 

quoted

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()).

quoted

quoted

+        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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy