On Fri, Oct 20, 2023 at 06:43:40PM +0800, Minda Chen wrote:
> Add StarFive JH7110 SoC PCIe controller platform
> driver codes, JH7110 with PLDA host PCIe core.
Wrap all your commit logs to fill about 75 columns (as suggested
before). "git log" adds a few spaces, so if you fill to 75 columns,
the result will still fit in a default 80 column window.
> +config PCIE_STARFIVE_HOST
> + tristate "StarFive PCIe host controller"
> + depends on OF && PCI_MSI
> + select PCIE_PLDA_HOST
> + help
> + Say Y here if you want to support the StarFive PCIe controller
> + in host mode. StarFive PCIe controller uses PLDA PCIe
> + core.
Add blank line between paragraphs. Wrap to fill 75-78 columns.
> + If you choose to build this driver as module it will
> + be dynamically linked and module will be called
> + pcie-starfive.ko
> +++ b/drivers/pci/controller/plda/pcie-plda.h
> @@ -6,14 +6,26 @@
> #ifndef _PCIE_PLDA_H
> #define _PCIE_PLDA_H
>
> +#include <linux/phy/phy.h>
I don't think you need to #include this. In this file you only use a
pointer to struct phy, so declaring the struct should be enough, e.g.,
struct phy;
You will have to #include it in pcie-starfive.c where you actually
*use* phy, of course.
> +#define CONFIG_SPACE_ADDR 0x1000u
This looks like an *offset* that you add to ->bridge_addr. Adding two
addresses together doesn't really make sense.
> +static 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 this should probably return PCIBIOS_SUCCESSFUL. There's
nothing wrong with the register number; you just want to pretend that
it's hardwired to zero. That means ignore writes and always return 0
for reads.
> + return pci_generic_config_write(bus, devfn, where, size, value);
> +}
> +
> +static int starfive_pcie_config_read(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;
Set *value to zero and return PCIBIOS_SUCCESSFUL.
> + return pci_generic_config_read(bus, devfn, where, size, value);
> +}
> +
> +static int starfive_pcie_parse_dt(struct starfive_jh7110_pcie *pcie, struct device *dev)
95% of this driver (and the rest of drivers/pci) is wrapped to fit in
80 columns, e.g.,
static int starfive_pcie_parse_dt(struct starfive_jh7110_pcie *pcie,
struct device *dev)
> + domain_nr = of_get_pci_domain_nr(dev->of_node);
> +
> + if (domain_nr < 0 || domain_nr > 1)
> + return dev_err_probe(dev, -ENODEV,
> + "failed to get valid pcie id\n");
"id" is too generic and doesn't hint about where the problem is.
Update the message ("pcie id") to mention "domain" so it corresponds
with the source ("linux,pci-domain" from DT).
> + ret = reset_control_deassert(pcie->resets);
> + if (ret) {
> + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
> + dev_err_probe(dev, ret, "failed to resets\n");
"failed to ... resets" is missing a word. "Failed to deassert
resets", I guess?
> + /* Ensure that PERST has been asserted for at least 100 ms,
> + * the sleep value is T_PVPERL from PCIe CEM spec r2.0 (Table 2-4)
> + */
Use multiline comment formatting (also below):
/*
* Ensure ...
*/
> + msleep(100);
> + if (pcie->reset_gpio)
> + gpiod_set_value_cansleep(pcie->reset_gpio, 0);
> +
> + /* As the requirement in PCIe base spec r6.0, system (<=5GT/s) must
> + * wait a minimum of 100 ms following exit from a conventional reset
> + * before sending a configuration request to the device.
Mention sec 6.6.1, where (I think) this value comes from. Eventually
we should make a #define for this because it's not specific to any one
PCIe controller.
> + msleep(100);
> +
> + if (starfive_pcie_host_wait_for_link(pcie))
> + dev_info(dev, "port link down\n");
> +
> + return ret;
We know the value here, so return it explicitly:
return 0;
> +static int starfive_pcie_suspend_noirq(struct device *dev)
> +{
> + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev);
> +
> + if (!pcie)
> + return 0;
How could it happen that "pcie" is zero? I think it could only happen
if there were a driver bug or a memory corruption. Either way, we
should remove the check so we take a NULL pointer fault and find out
about the problem.
> +static int starfive_pcie_resume_noirq(struct device *dev)
> +{
> + struct starfive_jh7110_pcie *pcie = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = starfive_pcie_enable_phy(dev, &pcie->plda);
> + if (ret)
> + return ret;
> +
> + ret = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);
> + if (ret) {
> + dev_err(dev, "failed to enable clocks\n");
> + starfive_pcie_disable_phy(&pcie->plda);
> + return ret;
> + }
> +
> + return ret;
return 0;
Bjorn
