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