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@xxxxxxxx/ > 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
