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
