Hi Kever,

On 2023-05-09 13:58, Kever Yang wrote:

Hi Jonas,

On 2023/4/23 02:19, Jonas Karlman wrote:

...

The vpcie3v3 regulator is typically a fixed regulator controlled using gpio. Change to use enable and disable calls on the regulator instead of trying to set a voltage value.

Signed-off-by: Jonas Karlman jonas@kwiboo.se

drivers/pci/pcie_dw_rockchip.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/pcie_dw_rockchip.c b/drivers/pci/pcie_dw_rockchip.c index 225af400ba70..a5b900f95981 100644 --- a/drivers/pci/pcie_dw_rockchip.c +++ b/drivers/pci/pcie_dw_rockchip.c @@ -288,21 +288,16 @@ static int rockchip_pcie_init_port(struct udevice *dev) struct rk_pcie *priv = dev_get_priv(dev);

/* Set power and maybe external ref clk input */

• if (priv->vpcie3v3) {

• ret = regulator_set_value(priv->vpcie3v3, 3300000);
  

• if (ret) {
  

• 	dev_err(priv->dw.dev, "failed to enable vpcie3v3 (ret=%d)\n",
  

• 		ret);
  

• 	return ret;
  

• }
  

• ret = regulator_set_enable_if_allowed(priv->vpcie3v3, true);
• if (ret && ret != -ENOSYS) {

• dev_err(dev, "failed to enable vpcie3v3 (ret=%d)\n", ret);
  

• return ret;
  

  }

• udelay(MACRO_US * 1000);

Why remove this udelay?

This may help the power stable before we can access pcie phy.

I removed this to closer match the linux driver, if a delay is needed it can be added using e.g. the startup-delay-us prop of a fixed regulator.

startup-delay-us: description: startup time in microseconds

Based on testing on Rock 3A, CM3, Quartz64, SoQuartz, Odroid-M1 and Rock 5B a delay using startup-delay-us did not seem necessary.

Regards, Jonas

Thanks,

• Kever

...

• ret = generic_phy_init(&priv->phy); if (ret) { dev_err(dev, "failed to init phy (ret=%d)\n", ret);

• return ret;
  

• goto err_disable_regulator;
  

  }

  ret = generic_phy_power_on(&priv->phy);

@@ -345,6 +340,8 @@ err_power_off_phy: generic_phy_power_off(&priv->phy); err_exit_phy: generic_phy_exit(&priv->phy); +err_disable_regulator:

• regulator_set_enable_if_allowed(priv->vpcie3v3, false);

  return ret; }

Hi Jonas,

On 2023/4/23 02:19, Jonas Karlman wrote:

Use a similar pattern and delay values as the linux mainline driver to speed up failing when nothing is connected.

Reduce fail speed from around 5+ seconds down to around one second on a Radxa ROCK 3 Model A, where pcie2x1 is probed before pcie3x2 M2 slot.

Signed-off-by: Jonas Karlman jonas@kwiboo.se

drivers/pci/pcie_dw_rockchip.c | 68 ++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 31 deletions(-)

diff --git a/drivers/pci/pcie_dw_rockchip.c b/drivers/pci/pcie_dw_rockchip.c index a5b900f95981..fd3da47272b3 100644 --- a/drivers/pci/pcie_dw_rockchip.c +++ b/drivers/pci/pcie_dw_rockchip.c @@ -61,9 +61,6 @@ struct rk_pcie { #define PCIE_CLIENT_DBG_TRANSITION_DATA 0xffff0000 #define PCIE_CLIENT_DBF_EN 0xffff0003

-/* Parameters for the waiting for #perst signal */ -#define MACRO_US 1000

• static int rk_pcie_read(void __iomem *addr, int size, u32 *val) { if ((uintptr_t)addr & (size - 1)) {

@@ -242,43 +239,46 @@ static int rk_pcie_link_up(struct rk_pcie *priv, u32 cap_speed) /* DW pre link configurations */ rk_pcie_configure(priv, cap_speed);

• /* Rest the device */
• if (dm_gpio_is_valid(&priv->rst_gpio)) {

• dm_gpio_set_value(&priv->rst_gpio, 0);
  

• /*
  

•  * Minimal is 100ms from spec but we see
  

•  * some wired devices need much more, such as 600ms.
  

•  * Add a enough delay to cover all cases.
  

•  */
  

• udelay(MACRO_US * 1000);
  

This delay has been changed.

• dm_gpio_set_value(&priv->rst_gpio, 1);
  

• }
• rk_pcie_disable_ltssm(priv); rk_pcie_link_status_clear(priv); rk_pcie_enable_debug(priv);

• /* Reset the device */
• if (dm_gpio_is_valid(&priv->rst_gpio))

• dm_gpio_set_value(&priv->rst_gpio, 0);
  

• /* Enable LTSSM */ rk_pcie_enable_ltssm(priv);

• for (retries = 0; retries < 5; retries++) {

• if (is_link_up(priv)) {
  

• 	dev_info(priv->dw.dev, "PCIe Link up, LTSSM is 0x%x\n",
  

• 		 rk_pcie_readl_apb(priv, PCIE_CLIENT_LTSSM_STATUS));
  

• 	rk_pcie_debug_dump(priv);
  

• 	return 0;
  

• }
  

• dev_info(priv->dw.dev, "PCIe Linking... LTSSM is 0x%x\n",
  

• 	 rk_pcie_readl_apb(priv, PCIE_CLIENT_LTSSM_STATUS));
  

• rk_pcie_debug_dump(priv);
  

• udelay(MACRO_US * 1000);
  

• /*

• * PCIe requires the refclk to be stable for 100ms prior to releasing
  

• * PERST. See table 2-4 in section 2.6.2 AC Specifications of the PCI
  

• * Express Card Electromechanical Specification, 1.1. However, we don't
  

• * know if the refclk is coming from RC's PHY or external OSC. If it's
  

• * from RC, so enabling LTSSM is the just right place to release #PERST.
  

• */
  

• mdelay(100);

There is a common about this delay, the spec is 100ms, but some device on the public market

may not work with this delay, it's better to keep previous delay value.

Thanks,

- Kever

• if (dm_gpio_is_valid(&priv->rst_gpio))

• dm_gpio_set_value(&priv->rst_gpio, 1);
  

• /* Check if the link is up or not */
• for (retries = 0; retries < 10; retries++) {

• if (is_link_up(priv))
  

• 	break;
  

• mdelay(100);
  

• }
• if (retries >= 10) {

• dev_err(priv->dw.dev, "PCIe-%d Link Fail\n",
  

• 	dev_seq(priv->dw.dev));
  

• return -EIO;
  

  }

• dev_err(priv->dw.dev, "PCIe-%d Link Fail\n", dev_seq(priv->dw.dev));
• /* Link maybe in Gen switch recovery but we need to wait more 1s */
• udelay(MACRO_US * 1000);
• return -EIO;

• dev_info(priv->dw.dev, "PCIe Link up, LTSSM is 0x%x\n",

•  rk_pcie_readl_apb(priv, PCIE_CLIENT_LTSSM_STATUS));
  

• rk_pcie_debug_dump(priv);

• return 0; }

  static int rockchip_pcie_init_port(struct udevice *dev)

@@ -287,6 +287,12 @@ static int rockchip_pcie_init_port(struct udevice *dev) u32 val; struct rk_pcie *priv = dev_get_priv(dev);

• ret = reset_assert_bulk(&priv->rsts);
• if (ret) {

• dev_err(dev, "failed to assert resets (ret=%d)\n", ret);
  

• return ret;
  

• }
• /* Set power and maybe external ref clk input */ ret = regulator_set_enable_if_allowed(priv->vpcie3v3, true); if (ret && ret != -ENOSYS) {

Hi Kever,

On 2023-05-09 14:19, Kever Yang wrote:

Hi Jonas,

On 2023/4/23 02:19, Jonas Karlman wrote:

...

PCI Autoconfig read the Root Complex BARs and try to claim the entire 1 GiB memory region on RK3568, leaving no space for any attached device.

Return an invalid value during config read of Root Complex BARs during autoconfig to work around such issue.

Could you share the error message without this workaround?

I think the driver can works without this patch, the 1GiB memory BAR will assigned fail

and has no affect to BAR allocate for attached device.

Both BAR0 and BAR1 of the Root Complex on RK3568 report that it wants 1 GiB allocations (value=c0000000). On RK3588 the Root Complex BARs was entirely skipped (value=0).

The 1 GiB memory BAR assignment used to fail when a 0x0-0x3fffffff pci range was used, because u-boot would skip initial 4 KiB and instead use 0x1000-0x3fffffff as the pci range.

https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pci/pci_auto_comm...

With the change to use a 0x40000000-0x7fffffff pci range, the first 1 GiB allocation instead succeed and consume the entire usable range, leaving allocation for BAR1 of the Root Complex and BARs for any attached devices to fail.

I did not find any hw reg that could be used to control the returned value for the BAR0/1 or the Root Complex, value c0000000 was always read back.

With this change to mask BAR0 and BAR1 for Root Complex on RK3568, the entire pci range is available for BARs of any attached device.

Regards, Jonas

Thanks,

• Kever

...

Signed-off-by: Jonas Karlman jonas@kwiboo.se

drivers/pci/pcie_dw_rockchip.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie_dw_rockchip.c b/drivers/pci/pcie_dw_rockchip.c index fd3da47272b3..924ecb93e963 100644 --- a/drivers/pci/pcie_dw_rockchip.c +++ b/drivers/pci/pcie_dw_rockchip.c @@ -146,6 +146,32 @@ static inline void rk_pcie_writel_apb(struct rk_pcie *rk_pcie, u32 reg, __rk_pcie_write_apb(rk_pcie, rk_pcie->apb_base, reg, 0x4, val); }

+/**

• • The BARs of bridge should be hidden during enumeration to avoid
• • allocation of the entire memory region by PCIe core on RK3568.
• */

+static bool rk_pcie_hide_rc_bar(struct pcie_dw *pcie, pci_dev_t bdf,

• 		uint offset)
  

+{

• int bus = PCI_BUS(bdf) - pcie->first_busno;
• return bus == 0 && PCI_DEV(bdf) == 0 && PCI_FUNC(bdf) == 0 &&

•       offset >= PCI_BASE_ADDRESS_0 && offset <= PCI_BASE_ADDRESS_1;
  

+}

+static int rk_pcie_read_config(const struct udevice *bus, pci_dev_t bdf,

• 	       uint offset, ulong *valuep,
  

• 	       enum pci_size_t size)
  

+{

• struct pcie_dw *pcie = dev_get_priv(bus);
• int ret = pcie_dw_read_config(bus, bdf, offset, valuep, size);
• if (!ret && rk_pcie_hide_rc_bar(pcie, bdf, offset))

• *valuep = pci_get_ff(size);
  

• return ret;

+}

• /**
  • rk_pcie_configure() - Configure link capabilities and speed

@@ -476,7 +502,7 @@ rockchip_pcie_probe_err_init_port: }

static const struct dm_pci_ops rockchip_pcie_ops = {

• .read_config = pcie_dw_read_config,

• .read_config = rk_pcie_read_config, .write_config = pcie_dw_write_config, };

On 2023/4/23 02:19, Jonas Karlman wrote:

The commit 12df2c182ccb ("regulator: dt-bindings: fixed-regulator: allow gpios property") in linux v6.3-rc1 added support for use of either a gpios or gpio prop with a fixed-regulator.

This adds support for the new gpios prop to the fixed-regulator driver.

Signed-off-by: Jonas Karlman jonas@kwiboo.se

Reviewed-by: Kever Yang kever.yang@rock-chips.com

Thanks, - Kever

---

drivers/power/regulator/fixed.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/power/regulator/fixed.c b/drivers/power/regulator/fixed.c index ad3b4b98d667..fcfb467a46e3 100644 --- a/drivers/power/regulator/fixed.c +++ b/drivers/power/regulator/fixed.c @@ -25,6 +25,7 @@ static int fixed_regulator_of_to_plat(struct udevice *dev) { struct dm_regulator_uclass_plat *uc_pdata; struct regulator_common_plat *plat;

• bool gpios;

  plat = dev_get_plat(dev); uc_pdata = dev_get_uclass_plat(dev);

@@ -33,7 +34,9 @@ static int fixed_regulator_of_to_plat(struct udevice *dev)

uc_pdata->type = REGULATOR_TYPE_FIXED;

• return regulator_common_of_to_plat(dev, plat, "gpio");

• gpios = dev_read_bool(dev, "gpios");

• return regulator_common_of_to_plat(dev, plat,

• 			   gpios ? "gpios" : "gpio");
  

  }

  static int fixed_regulator_get_value(struct udevice *dev)

On 2023/4/23 02:19, Jonas Karlman wrote:

Add dummy support for the CLK_PCIEPHY2_REF clock.

Signed-off-by: Jonas Karlman jonas@kwiboo.se

Reviewed-by: Kever Yang kever.yang@rock-chips.com

Thanks, - Kever

---

drivers/clk/rockchip/clk_rk3568.c | 1 + 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/rockchip/clk_rk3568.c b/drivers/clk/rockchip/clk_rk3568.c index cefc263971a6..c8e688789e4c 100644 --- a/drivers/clk/rockchip/clk_rk3568.c +++ b/drivers/clk/rockchip/clk_rk3568.c @@ -427,6 +427,7 @@ static ulong rk3568_pmuclk_set_rate(struct clk *clk, ulong rate) break; case CLK_PCIEPHY0_REF: case CLK_PCIEPHY1_REF:

• case CLK_PCIEPHY2_REF: return 0; default: return -ENOENT;

Hi Jonas,

On 2023/4/23 02:19, Jonas Karlman wrote:

Add missing pinctrl and defconfig options to enable PCIe and NVMe support on Radxa ROCK 3 Model A.

Signed-off-by: Jonas Karlman jonas@kwiboo.se

arch/arm/dts/rk3568-rock-3a-u-boot.dtsi | 14 ++++++++++++++ configs/rock-3a-rk3568_defconfig | 4 ++++ 2 files changed, 18 insertions(+)

diff --git a/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi b/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi index 8abee24c02c3..f3ee50949cc0 100644 --- a/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi +++ b/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi @@ -19,6 +19,12 @@

&pinctrl { bootph-pre-ram;

• pcie {

• pcie3x2_reset_h: pcie3x2-reset-h {
  

• 	rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO &pcfg_pull_none>;
  

• };
  

• }; };

  &pcfg_pull_up {

@@ -73,6 +79,14 @@ bootph-pre-ram; };

+&pcie2x1 {

• pinctrl-0 = <&pcie20m1_pins &pcie_reset_h>;

+};

+&pcie3x2 {

• pinctrl-0 = <&pcie30x2m1_pins &pcie3x2_reset_h>;

+};

Did you really met issue without this patch?

The pcie using GPIO for reset and other function instead of pcie function, and these GPIOs's default

setting is GPIO, so they should work by default.

Thanks,

- Kever

• &sfc { bootph-pre-ram; u-boot,spl-sfc-no-dma;

diff --git a/configs/rock-3a-rk3568_defconfig b/configs/rock-3a-rk3568_defconfig index 64864a300153..9d725b82a96a 100644 --- a/configs/rock-3a-rk3568_defconfig +++ b/configs/rock-3a-rk3568_defconfig @@ -46,6 +46,7 @@ CONFIG_CMD_GPIO=y CONFIG_CMD_GPT=y CONFIG_CMD_I2C=y CONFIG_CMD_MMC=y +CONFIG_CMD_PCI=y CONFIG_CMD_USB=y # CONFIG_CMD_SETEXPR is not set CONFIG_CMD_PMIC=y @@ -70,6 +71,9 @@ CONFIG_SPI_FLASH_MACRONIX=y CONFIG_SPI_FLASH_XTX=y CONFIG_ETH_DESIGNWARE=y CONFIG_GMAC_ROCKCHIP=y +CONFIG_NVME_PCI=y +CONFIG_PCI=y +CONFIG_PCIE_DW_ROCKCHIP=y CONFIG_PHY_ROCKCHIP_INNO_USB2=y CONFIG_PHY_ROCKCHIP_NANENG_COMBOPHY=y CONFIG_SPL_PINCTRL=y

On 2023/4/23 02:19, Jonas Karlman wrote:

Update config, IO and memory regions used based on [1] with pcie3x2 config reg size corrected from 16 to 1 MiB.

[1] https://lore.kernel.org/lkml/20221112114125.1637543-2-aholmes@omnom.net/

Signed-off-by: Jonas Karlman jonas@kwiboo.se

Reviewed-by: Kever Yang kever.yang@rock-chips.com

Thanks, - Kever

---

arch/arm/dts/rk3568.dtsi | 14 ++++++++------ arch/arm/dts/rk356x.dtsi | 7 ++++--- 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/arm/dts/rk3568.dtsi b/arch/arm/dts/rk3568.dtsi index ba67b58f05b7..f1be76a54ceb 100644 --- a/arch/arm/dts/rk3568.dtsi +++ b/arch/arm/dts/rk3568.dtsi @@ -94,9 +94,10 @@ power-domains = <&power RK3568_PD_PIPE>; reg = <0x3 0xc0400000 0x0 0x00400000>, <0x0 0xfe270000 0x0 0x00010000>,

•       <0x3 0x7f000000 0x0 0x01000000>;
  

• ranges = <0x01000000 0x0 0x3ef00000 0x3 0x7ef00000 0x0 0x00100000>,
  

• 	 <0x02000000 0x0 0x00000000 0x3 0x40000000 0x0 0x3ef00000>;
  

•       <0x0 0xf2000000 0x0 0x00100000>;
  

• ranges = <0x01000000 0x0 0xf2100000 0x0 0xf2100000 0x0 0x00100000>,
  

• 	 <0x02000000 0x0 0xf2200000 0x0 0xf2200000 0x0 0x01e00000>,
  

• 	 <0x03000000 0x0 0x40000000 0x3 0x40000000 0x0 0x40000000>;
  

  reg-names = "dbi", "apb", "config"; resets = <&cru SRST_PCIE30X1_POWERUP>; reset-names = "pipe";

@@ -146,9 +147,10 @@ power-domains = <&power RK3568_PD_PIPE>; reg = <0x3 0xc0800000 0x0 0x00400000>, <0x0 0xfe280000 0x0 0x00010000>,

•       <0x3 0xbf000000 0x0 0x01000000>;
  

• ranges = <0x01000000 0x0 0x3ef00000 0x3 0xbef00000 0x0 0x00100000>,
  

• 	 <0x02000000 0x0 0x00000000 0x3 0x80000000 0x0 0x3ef00000>;
  

•       <0x0 0xf0000000 0x0 0x00100000>;
  

• ranges = <0x01000000 0x0 0xf0100000 0x0 0xf0100000 0x0 0x00100000>,
  

• 	 <0x02000000 0x0 0xf0200000 0x0 0xf0200000 0x0 0x01e00000>,
  

• 	 <0x03000000 0x0 0x40000000 0x3 0x80000000 0x0 0x40000000>;
  

  reg-names = "dbi", "apb", "config"; resets = <&cru SRST_PCIE30X2_POWERUP>; reset-names = "pipe";

diff --git a/arch/arm/dts/rk356x.dtsi b/arch/arm/dts/rk356x.dtsi index 6492ace0de6b..e0591c194bec 100644 --- a/arch/arm/dts/rk356x.dtsi +++ b/arch/arm/dts/rk356x.dtsi @@ -951,7 +951,7 @@ compatible = "rockchip,rk3568-pcie"; reg = <0x3 0xc0000000 0x0 0x00400000>, <0x0 0xfe260000 0x0 0x00010000>,

•       <0x3 0x3f000000 0x0 0x01000000>;
  

•       <0x0 0xf4000000 0x0 0x00100000>;
  

  reg-names = "dbi", "apb", "config"; interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>,

@@ -980,8 +980,9 @@ phys = <&combphy2 PHY_TYPE_PCIE>; phy-names = "pcie-phy"; power-domains = <&power RK3568_PD_PIPE>;

• ranges = <0x01000000 0x0 0x3ef00000 0x3 0x3ef00000 0x0 0x00100000
  

• 	  0x02000000 0x0 0x00000000 0x3 0x00000000 0x0 0x3ef00000>;
  

• ranges = <0x01000000 0x0 0xf4100000 0x0 0xf4100000 0x0 0x00100000>,
  

• 	 <0x02000000 0x0 0xf4200000 0x0 0xf4200000 0x0 0x01e00000>,
  

• 	 <0x03000000 0x0 0x40000000 0x3 0x00000000 0x0 0x40000000>;
  

  resets = <&cru SRST_PCIE20_POWERUP>; reset-names = "pipe"; #address-cells = <3>;