From: Bjorn Helgaas <helgaas@kernel.org> To: Minda Chen <minda.chen@starfivetech.com> Cc: "Daire McNamara" <daire.mcnamara@microchip.com>, "Conor Dooley" <conor@kernel.org>, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Bjorn Helgaas" <bhelgaas@google.com>, "Lorenzo Pieralisi" <lpieralisi@kernel.org>, "Krzysztof Wilczyński" <kw@linux.com>, "Emil Renner Berthing" <emil.renner.berthing@canonical.com>, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, linux-pci@vger.kernel.org, "Paul Walmsley" <paul.walmsley@sifive.com>, "Palmer Dabbelt" <palmer@dabbelt.com>, "Albert Ou" <aou@eecs.berkeley.edu>, "Philipp Zabel" <p.zabel@pengutronix.de>, "Mason Huo" <mason.huo@starfivetech.com>, "Leyfoon Tan" <leyfoon.tan@starfivetech.com>, "Kevin Xie" <kevin.xie@starfivetech.com> Subject: Re: [PATCH v1 8/9] PCI: PLDA: starfive: Add JH7110 PCIe controller Date: Wed, 19 Jul 2023 11:48:51 -0500 [thread overview] Message-ID: <20230719164851.GA505840@bhelgaas> (raw) In-Reply-To: <20230719102057.22329-9-minda.chen@starfivetech.com> On Wed, Jul 19, 2023 at 06:20:56PM +0800, Minda Chen wrote: > Add StarFive JH7110 SoC PCIe controller platform > driver codes. Rewrap all the commit logs to fill 75 columns or so. > #define PCIE_PCI_IDS_DW1 0x9c > - > +#define IDS_CLASS_CODE_SHIFT 16 > +#define PCI_MISC 0xB4 Surrounding code uses lower-case hex. Make it all match. > +#define STG_SYSCON_AXI4_SLVL_ARFUNC_MASK GENMASK(22, 8) > +#define STG_SYSCON_AXI4_SLVL_ARFUNC_SHIFT 8 When practical, use FIELD_GET() and FIELD_PREP() to avoid the need for *_SHIFT macros. > +struct starfive_jh7110_pcie { > + struct plda_pcie plda; > + struct reset_control *resets; > + struct clk_bulk_data *clks; > + struct regmap *reg_syscon; > + struct gpio_desc *power_gpio; > + struct gpio_desc *reset_gpio; > + > + u32 stg_arfun; > + u32 stg_awfun; > + u32 stg_rp_nep; > + u32 stg_lnksta; > + > + int num_clks; If you indent one member with tabs, e.g., "struct plda_pcie plda", they should all be indented to match. > + * The BAR0/1 of bridge should be hidden during enumeration to > + * avoid the sizing and resource allocation by PCIe core. > + */ > +static bool starfive_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int devfn, > + int offset) > +{ > + if (pci_is_root_bus(bus) && !devfn && > + (offset == PCI_BASE_ADDRESS_0 || offset == PCI_BASE_ADDRESS_1)) > + return true; > + > + return false; > +} > + > +int starfive_pcie_config_write(struct pci_bus *bus, unsigned int devfn, > + int where, int size, u32 value) > +{ > + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) > + return PCIBIOS_BAD_REGISTER_NUMBER; I think you are trying present BARs 0 & 1 as unimplemented. Such BARs are hardwired to zero, so you should make them behave that way (both read and write). Many callers of config accessors don't check the return value, so I don't think it's reliable to just return PCIBIOS_BAD_REGISTER_NUMBER. > +static int starfive_pcie_is_link_up(struct starfive_jh7110_pcie *pcie) > +{ > + struct device *dev = pcie->plda.dev; > + int ret; > + u32 stg_reg_val; > + > + /* 100ms timeout value should be enough for Gen1/2 training */ > + ret = regmap_read_poll_timeout(pcie->reg_syscon, > + pcie->stg_lnksta, > + stg_reg_val, > + stg_reg_val & DATA_LINK_ACTIVE, > + 10 * 1000, 100 * 1000); > + > + /* If the link is down (no device in slot), then exit. */ > + if (ret == -ETIMEDOUT) { > + dev_info(dev, "Port link down, exit.\n"); > + return 0; > + } else if (ret == 0) { > + dev_info(dev, "Port link up.\n"); > + return 1; > + } Please copy the naming and style of the "*_pcie_link_up()" functions in other drivers. These are boolean functions with no side effects, including no timeouts. Some drivers have "*wait_for_link()" functions if polling is needed. > + return dev_err_probe(dev, ret, > + "failed to initialize pcie phy\n"); Driver messages should match (all capitalized or none capitalized). > + /* Enable root port */ Superfluous comment, since the function name says the same. > + plda_pcie_enable_root_port(plda); > + /* Ensure that PERST has been asserted for at least 100 ms */ > + msleep(300); > + gpiod_set_value_cansleep(pcie->reset_gpio, 0); At least 100 ms, but you sleep *300* ms? This is probably related to https://lore.kernel.org/r/20230718155515.GA483233@bhelgaas Please include a comment with the source of the delay value. I assume it's T_PVPERL and T_PERST-CLK from the PCIe CEM spec. This way we can someday share those #defines across drivers. > +#ifdef CONFIG_PM_SLEEP > +static int __maybe_unused starfive_pcie_suspend_noirq(struct device *dev) I think you can dispense with some of these #ifdefs and the __maybe_unused as in https://lore.kernel.org/all/20220720224829.GA1667002@bhelgaas/ > +{ > + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); > + > + if (!pcie) > + return 0; How can this happen? If we're only detecting memory corruption, it's not worth it. Bjorn
WARNING: multiple messages have this Message-ID (diff)
From: Bjorn Helgaas <helgaas@kernel.org> To: Minda Chen <minda.chen@starfivetech.com> Cc: "Daire McNamara" <daire.mcnamara@microchip.com>, "Conor Dooley" <conor@kernel.org>, "Rob Herring" <robh+dt@kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Bjorn Helgaas" <bhelgaas@google.com>, "Lorenzo Pieralisi" <lpieralisi@kernel.org>, "Krzysztof Wilczyński" <kw@linux.com>, "Emil Renner Berthing" <emil.renner.berthing@canonical.com>, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, linux-pci@vger.kernel.org, "Paul Walmsley" <paul.walmsley@sifive.com>, "Palmer Dabbelt" <palmer@dabbelt.com>, "Albert Ou" <aou@eecs.berkeley.edu>, "Philipp Zabel" <p.zabel@pengutronix.de>, "Mason Huo" <mason.huo@starfivetech.com>, "Leyfoon Tan" <leyfoon.tan@starfivetech.com>, "Kevin Xie" <kevin.xie@starfivetech.com> Subject: Re: [PATCH v1 8/9] PCI: PLDA: starfive: Add JH7110 PCIe controller Date: Wed, 19 Jul 2023 11:48:51 -0500 [thread overview] Message-ID: <20230719164851.GA505840@bhelgaas> (raw) In-Reply-To: <20230719102057.22329-9-minda.chen@starfivetech.com> On Wed, Jul 19, 2023 at 06:20:56PM +0800, Minda Chen wrote: > Add StarFive JH7110 SoC PCIe controller platform > driver codes. Rewrap all the commit logs to fill 75 columns or so. > #define PCIE_PCI_IDS_DW1 0x9c > - > +#define IDS_CLASS_CODE_SHIFT 16 > +#define PCI_MISC 0xB4 Surrounding code uses lower-case hex. Make it all match. > +#define STG_SYSCON_AXI4_SLVL_ARFUNC_MASK GENMASK(22, 8) > +#define STG_SYSCON_AXI4_SLVL_ARFUNC_SHIFT 8 When practical, use FIELD_GET() and FIELD_PREP() to avoid the need for *_SHIFT macros. > +struct starfive_jh7110_pcie { > + struct plda_pcie plda; > + struct reset_control *resets; > + struct clk_bulk_data *clks; > + struct regmap *reg_syscon; > + struct gpio_desc *power_gpio; > + struct gpio_desc *reset_gpio; > + > + u32 stg_arfun; > + u32 stg_awfun; > + u32 stg_rp_nep; > + u32 stg_lnksta; > + > + int num_clks; If you indent one member with tabs, e.g., "struct plda_pcie plda", they should all be indented to match. > + * The BAR0/1 of bridge should be hidden during enumeration to > + * avoid the sizing and resource allocation by PCIe core. > + */ > +static bool starfive_pcie_hide_rc_bar(struct pci_bus *bus, unsigned int devfn, > + int offset) > +{ > + if (pci_is_root_bus(bus) && !devfn && > + (offset == PCI_BASE_ADDRESS_0 || offset == PCI_BASE_ADDRESS_1)) > + return true; > + > + return false; > +} > + > +int starfive_pcie_config_write(struct pci_bus *bus, unsigned int devfn, > + int where, int size, u32 value) > +{ > + if (starfive_pcie_hide_rc_bar(bus, devfn, where)) > + return PCIBIOS_BAD_REGISTER_NUMBER; I think you are trying present BARs 0 & 1 as unimplemented. Such BARs are hardwired to zero, so you should make them behave that way (both read and write). Many callers of config accessors don't check the return value, so I don't think it's reliable to just return PCIBIOS_BAD_REGISTER_NUMBER. > +static int starfive_pcie_is_link_up(struct starfive_jh7110_pcie *pcie) > +{ > + struct device *dev = pcie->plda.dev; > + int ret; > + u32 stg_reg_val; > + > + /* 100ms timeout value should be enough for Gen1/2 training */ > + ret = regmap_read_poll_timeout(pcie->reg_syscon, > + pcie->stg_lnksta, > + stg_reg_val, > + stg_reg_val & DATA_LINK_ACTIVE, > + 10 * 1000, 100 * 1000); > + > + /* If the link is down (no device in slot), then exit. */ > + if (ret == -ETIMEDOUT) { > + dev_info(dev, "Port link down, exit.\n"); > + return 0; > + } else if (ret == 0) { > + dev_info(dev, "Port link up.\n"); > + return 1; > + } Please copy the naming and style of the "*_pcie_link_up()" functions in other drivers. These are boolean functions with no side effects, including no timeouts. Some drivers have "*wait_for_link()" functions if polling is needed. > + return dev_err_probe(dev, ret, > + "failed to initialize pcie phy\n"); Driver messages should match (all capitalized or none capitalized). > + /* Enable root port */ Superfluous comment, since the function name says the same. > + plda_pcie_enable_root_port(plda); > + /* Ensure that PERST has been asserted for at least 100 ms */ > + msleep(300); > + gpiod_set_value_cansleep(pcie->reset_gpio, 0); At least 100 ms, but you sleep *300* ms? This is probably related to https://lore.kernel.org/r/20230718155515.GA483233@bhelgaas Please include a comment with the source of the delay value. I assume it's T_PVPERL and T_PERST-CLK from the PCIe CEM spec. This way we can someday share those #defines across drivers. > +#ifdef CONFIG_PM_SLEEP > +static int __maybe_unused starfive_pcie_suspend_noirq(struct device *dev) I think you can dispense with some of these #ifdefs and the __maybe_unused as in https://lore.kernel.org/all/20220720224829.GA1667002@bhelgaas/ > +{ > + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev); > + > + if (!pcie) > + return 0; How can this happen? If we're only detecting memory corruption, it's not worth it. Bjorn _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2023-07-19 16:49 UTC|newest] Thread overview: 92+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-07-19 10:20 [PATCH v1 0/9] Refactoring Microchip PolarFire PCIe driver Minda Chen 2023-07-19 10:20 ` Minda Chen 2023-07-19 10:20 ` [PATCH v1 1/9] dt-bindings: PCI: Add PLDA XpressRICH PCIe host common properties Minda Chen 2023-07-19 10:20 ` Minda Chen 2023-07-19 10:52 ` Krzysztof Kozlowski 2023-07-19 10:52 ` Krzysztof Kozlowski 2023-07-20 6:59 ` Minda Chen 2023-07-20 6:59 ` Minda Chen 2023-07-19 22:31 ` Rob Herring 2023-07-19 22:31 ` Rob Herring 2023-07-20 6:47 ` Minda Chen 2023-07-20 6:47 ` Minda Chen 2023-07-19 10:20 ` [PATCH v1 2/9] dt-bindings: PCI: microchip: Remove the PLDA " Minda Chen 2023-07-19 10:20 ` Minda Chen 2023-07-19 10:53 ` Krzysztof Kozlowski 2023-07-19 10:53 ` Krzysztof Kozlowski 2023-07-19 10:20 ` [PATCH v1 3/9] PCI: PLDA: Get PLDA common codes from Microchip PolarFire host Minda Chen 2023-07-19 10:20 ` Minda Chen 2023-07-19 10:20 ` [PATCH v1 4/9] PCI: microchip: Move PCIe driver to PLDA directory Minda Chen 2023-07-19 10:20 ` Minda Chen 2023-07-20 11:07 ` Conor Dooley 2023-07-20 11:07 ` Conor Dooley 2023-07-20 12:26 ` Conor Dooley 2023-07-20 12:26 ` Conor Dooley 2023-07-21 1:12 ` Minda Chen 2023-07-21 1:12 ` Minda Chen 2023-07-19 10:20 ` [PATCH v1 5/9] dt-bindings: PLDA: Add PLDA XpressRICH PCIe host controller Minda Chen 2023-07-19 10:20 ` Minda Chen 2023-07-19 10:55 ` Krzysztof Kozlowski 2023-07-19 10:55 ` Krzysztof Kozlowski 2023-07-19 22:29 ` Rob Herring 2023-07-19 22:29 ` Rob Herring 2023-07-20 7:02 ` Minda Chen 2023-07-20 7:02 ` Minda Chen 2023-07-19 10:20 ` [PATCH v1 6/9] PCI: PLDA: Add host conroller platform driver Minda Chen 2023-07-19 10:20 ` Minda Chen 2023-07-19 10:20 ` [PATCH v1 7/9] dt-bindings: PCI: Add StarFive JH7110 PCIe controller Minda Chen 2023-07-19 10:20 ` Minda Chen 2023-07-19 10:56 ` Krzysztof Kozlowski 2023-07-19 10:56 ` Krzysztof Kozlowski 2023-07-19 10:20 ` [PATCH v1 8/9] PCI: PLDA: starfive: Add " Minda Chen 2023-07-19 10:20 ` Minda Chen 2023-07-19 16:48 ` Bjorn Helgaas [this message] 2023-07-19 16:48 ` Bjorn Helgaas 2023-07-20 10:11 ` Kevin Xie 2023-07-20 10:11 ` Kevin Xie 2023-07-20 16:15 ` Bjorn Helgaas 2023-07-20 16:15 ` Bjorn Helgaas 2023-07-24 10:48 ` Kevin Xie 2023-07-24 10:48 ` Kevin Xie 2023-07-25 20:46 ` Bjorn Helgaas 2023-07-25 20:46 ` Bjorn Helgaas 2023-07-27 21:40 ` Bjorn Helgaas 2023-07-27 21:40 ` Bjorn Helgaas 2023-07-31 5:52 ` Kevin Xie 2023-07-31 5:52 ` Kevin Xie 2023-07-31 23:12 ` Bjorn Helgaas 2023-07-31 23:12 ` Bjorn Helgaas 2023-08-01 7:05 ` Pali Rohár 2023-08-01 7:05 ` Pali Rohár 2023-08-01 7:05 ` Kevin Xie 2023-08-01 7:05 ` Kevin Xie 2023-08-01 7:14 ` Pali Rohár 2023-08-01 7:14 ` Pali Rohár 2023-08-02 17:14 ` Bjorn Helgaas 2023-08-02 17:14 ` Bjorn Helgaas 2023-08-02 17:18 ` Bjorn Helgaas 2023-08-02 17:18 ` Bjorn Helgaas 2023-08-03 2:23 ` Kevin Xie 2023-08-03 2:23 ` Kevin Xie 2023-08-03 6:58 ` Pali Rohár 2023-08-03 6:58 ` Pali Rohár 2023-08-03 7:43 ` Kevin Xie 2023-08-03 7:43 ` Kevin Xie 2023-07-20 11:14 ` Conor Dooley 2023-07-20 11:14 ` Conor Dooley 2023-07-21 1:03 ` Minda Chen 2023-07-21 1:03 ` Minda Chen 2023-07-19 10:20 ` [PATCH v1 9/9] riscv: dts: starfive: add PCIe dts configuration for JH7110 Minda Chen 2023-07-19 10:20 ` Minda Chen 2023-07-19 15:26 ` [PATCH v1 0/9] Refactoring Microchip PolarFire PCIe driver Bjorn Helgaas 2023-07-19 15:26 ` Bjorn Helgaas 2023-07-20 2:15 ` Minda Chen 2023-07-20 2:15 ` Minda Chen 2023-07-20 12:12 ` Conor Dooley 2023-07-20 12:12 ` Conor Dooley 2023-07-21 9:34 ` Minda Chen 2023-07-21 9:34 ` Minda Chen 2023-07-21 9:55 ` Minda Chen 2023-07-21 9:55 ` Minda Chen 2023-07-19 16:58 ` Conor Dooley 2023-07-19 16:58 ` Conor Dooley
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20230719164851.GA505840@bhelgaas \ --to=helgaas@kernel.org \ --cc=aou@eecs.berkeley.edu \ --cc=bhelgaas@google.com \ --cc=conor@kernel.org \ --cc=daire.mcnamara@microchip.com \ --cc=devicetree@vger.kernel.org \ --cc=emil.renner.berthing@canonical.com \ --cc=kevin.xie@starfivetech.com \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=kw@linux.com \ --cc=leyfoon.tan@starfivetech.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=lpieralisi@kernel.org \ --cc=mason.huo@starfivetech.com \ --cc=minda.chen@starfivetech.com \ --cc=p.zabel@pengutronix.de \ --cc=palmer@dabbelt.com \ --cc=paul.walmsley@sifive.com \ --cc=robh+dt@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.