Re: [PATCH v9 19/20] PCI: starfive: Add JH7110 PCIe controller

[Kevin Xie <kevin.xie@starfivetech.com>]

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