On 2023/10/25 1:52, Bjorn Helgaas wrote:
> 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.
>
OK.
>> +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.
>
OK.
>> + 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.
>
OK.
>> +#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.
>
OK, I will rename it.
>> +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.
>
Agree, PCIBIOS_BAD_REGISTER_NUMBER is imprecise.
I will return PCIBIOS_SUCCESSFUL here, ignore writes and always return 0 for reads.
>> + 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)
>
Sorry, I will check and make them are wrapped to fit in 80 columns.
>> + 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).
>
Yes, domain is better.
>> + 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?
>
Yes, I will make a supplement.
>> + /* 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 ...
> */
>
OK.
>> + 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.
>
I would like to add a new define in drivers/pci/pci.h:
/*
* PCIe r6.0, sec 6.6.1, <Conventional Reset>
* Requires a minimum waiting of 100ms before sending a configuration
* request to the device.
*/
#define PCIE_BEFORE_CONFIG_REQUEST_WAIT_MS 100
When drivers use it, they can put them into the right place depends on the link speed:
1) With a Downstream Port(<=5.0 GT/s), system waits after conventional reset done.
2) With a Downstream Port(> 5.0 GT/s), system waits after link training completes.
Thus, our code will be:
+ if (pcie->reset_gpio)
+ gpiod_set_value_cansleep(pcie->reset_gpio, 0);
+ /*
+ * With a Downstream Port (<=5GT/s), software must wait a minimum
+ * of 100ms following exit from a conventional reset before
+ * sending a configuration request to the device.
*/
- msleep(100);
+ msleep(PCIE_BEFORE_CONFIG_REQUEST_WAIT_MS);
Is that what you expected?
>> + 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;
>
OK
>> +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.
>
I will remove it.
>> +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;
>
OK.
Thanks!
> Bjorn
