[PATCH 0/3] PCI: mediatek-gen3: mtk_pcie_en7581_power_up code refactoring

[PATCH 0/3] PCI: mediatek-gen3: mtk_pcie_en7581_power_up code refactoring

[Lorenzo Bianconi]

Minor fixes and code refactoring in mtk_pcie_en7581_power_up routine

---
Lorenzo Bianconi (3):
      PCI: mediatek-gen3: Add missing reset_control_deassert for mac_rst in mtk_pcie_en7581_power_up
      PCI: mediatek-gen3: rely on clk_bulk_prepare_enable in mtk_pcie_en7581_power_up
      PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

 drivers/pci/controller/pcie-mediatek-gen3.c | 42 ++++++++++++++++-------------
 1 file changed, 23 insertions(+), 19 deletions(-)
---
base-commit: 3102ce10f3111e4c3b8fb233dc93f29e220adaf7
change-id: 20241107-pcie-en7581-fixes-296505fd2353

Best regards,
-- 
Lorenzo Bianconi <lorenzo@kernel.org>

[PATCH 1/3] PCI: mediatek-gen3: Add missing reset_control_deassert for mac_rst in mtk_pcie_en7581_power_up

[Lorenzo Bianconi]

Even if this is not a real issue since Airoha EN7581 SoC does not require
the mac reset line, add missing reset_control_deassert() for mac reset
line in mtk_pcie_en7581_power_up callback.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 66ce4b5d309bb6d64618c70ac5e0a529e0910511..0fac0b9fd785e463d26d29d695b923db41eef9cc 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -897,6 +897,9 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
 	 */
 	mdelay(PCIE_EN7581_RESET_TIME_MS);
 
+	/* MAC power on and enable transaction layer clocks */
+	reset_control_deassert(pcie->mac_reset);
+
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 
@@ -931,6 +934,7 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
 err_clk_prepare:
 	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
+	reset_control_assert(pcie->mac_reset);
 	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
 err_phy_deassert:
 	phy_power_off(pcie->phy);

-- 
2.47.0

[PATCH 2/3] PCI: mediatek-gen3: rely on clk_bulk_prepare_enable in mtk_pcie_en7581_power_up

[Lorenzo Bianconi]

Squash clk_bulk_prepare and clk_bulk_enable in
clk_bulk_prepare_enable in mtk_pcie_en7581_power_up routine

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 0fac0b9fd785e463d26d29d695b923db41eef9cc..8c8c733a145634cdbfefd339f4a692f25a6e24de 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -903,12 +903,6 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 
-	err = clk_bulk_prepare(pcie->num_clks, pcie->clks);
-	if (err) {
-		dev_err(dev, "failed to prepare clock\n");
-		goto err_clk_prepare;
-	}
-
 	val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
 	      FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
 	      FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
@@ -921,17 +915,15 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
 	      FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf);
 	writel_relaxed(val, pcie->base + PCIE_PIPE4_PIE8_REG);
 
-	err = clk_bulk_enable(pcie->num_clks, pcie->clks);
+	err = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);
 	if (err) {
 		dev_err(dev, "failed to prepare clock\n");
-		goto err_clk_enable;
+		goto err_clk_init;
 	}
 
 	return 0;
 
-err_clk_enable:
-	clk_bulk_unprepare(pcie->num_clks, pcie->clks);
-err_clk_prepare:
+err_clk_init:
 	pm_runtime_put_sync(dev);
 	pm_runtime_disable(dev);
 	reset_control_assert(pcie->mac_reset);

-- 
2.47.0

Re: [PATCH 2/3] PCI: mediatek-gen3: rely on clk_bulk_prepare_enable in mtk_pcie_en7581_power_up

[Bjorn Helgaas]

On Thu, Nov 07, 2024 at 02:50:54PM +0100, Lorenzo Bianconi wrote:
> Squash clk_bulk_prepare and clk_bulk_enable in
> clk_bulk_prepare_enable in mtk_pcie_en7581_power_up routine

Thank you, this is much better.

Can you add "()" after function names in subject and commit logs here
and in other patches?

> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 0fac0b9fd785e463d26d29d695b923db41eef9cc..8c8c733a145634cdbfefd339f4a692f25a6e24de 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -903,12 +903,6 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
>  	pm_runtime_enable(dev);
>  	pm_runtime_get_sync(dev);
>  
> -	err = clk_bulk_prepare(pcie->num_clks, pcie->clks);
> -	if (err) {
> -		dev_err(dev, "failed to prepare clock\n");
> -		goto err_clk_prepare;
> -	}
> -
>  	val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
>  	      FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
>  	      FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
> @@ -921,17 +915,15 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
>  	      FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf);
>  	writel_relaxed(val, pcie->base + PCIE_PIPE4_PIE8_REG);
>  
> -	err = clk_bulk_enable(pcie->num_clks, pcie->clks);
> +	err = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);
>  	if (err) {
>  		dev_err(dev, "failed to prepare clock\n");
> -		goto err_clk_enable;
> +		goto err_clk_init;
>  	}
>  
>  	return 0;
>  
> -err_clk_enable:
> -	clk_bulk_unprepare(pcie->num_clks, pcie->clks);
> -err_clk_prepare:
> +err_clk_init:
>  	pm_runtime_put_sync(dev);
>  	pm_runtime_disable(dev);
>  	reset_control_assert(pcie->mac_reset);
> 
> -- 
> 2.47.0
> 

Re: [PATCH 2/3] PCI: mediatek-gen3: rely on clk_bulk_prepare_enable in mtk_pcie_en7581_power_up

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2165 bytes --]

> On Thu, Nov 07, 2024 at 02:50:54PM +0100, Lorenzo Bianconi wrote:
> > Squash clk_bulk_prepare and clk_bulk_enable in
> > clk_bulk_prepare_enable in mtk_pcie_en7581_power_up routine
> 
> Thank you, this is much better.
> 
> Can you add "()" after function names in subject and commit logs here
> and in other patches?

sure, I will do in v2.

Regards,
Lorenzo

> 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 14 +++-----------
> >  1 file changed, 3 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index 0fac0b9fd785e463d26d29d695b923db41eef9cc..8c8c733a145634cdbfefd339f4a692f25a6e24de 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -903,12 +903,6 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> >  	pm_runtime_enable(dev);
> >  	pm_runtime_get_sync(dev);
> >  
> > -	err = clk_bulk_prepare(pcie->num_clks, pcie->clks);
> > -	if (err) {
> > -		dev_err(dev, "failed to prepare clock\n");
> > -		goto err_clk_prepare;
> > -	}
> > -
> >  	val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
> >  	      FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
> >  	      FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
> > @@ -921,17 +915,15 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> >  	      FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf);
> >  	writel_relaxed(val, pcie->base + PCIE_PIPE4_PIE8_REG);
> >  
> > -	err = clk_bulk_enable(pcie->num_clks, pcie->clks);
> > +	err = clk_bulk_prepare_enable(pcie->num_clks, pcie->clks);
> >  	if (err) {
> >  		dev_err(dev, "failed to prepare clock\n");
> > -		goto err_clk_enable;
> > +		goto err_clk_init;
> >  	}
> >  
> >  	return 0;
> >  
> > -err_clk_enable:
> > -	clk_bulk_unprepare(pcie->num_clks, pcie->clks);
> > -err_clk_prepare:
> > +err_clk_init:
> >  	pm_runtime_put_sync(dev);
> >  	pm_runtime_disable(dev);
> >  	reset_control_assert(pcie->mac_reset);
> > 
> > -- 
> > 2.47.0
> > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[Lorenzo Bianconi]

In order to make the code more readable, move phy and mac reset lines
assert/de-assert configuration in .power_up callback
(mtk_pcie_en7581_power_up/mtk_pcie_power_up).

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 8c8c733a145634cdbfefd339f4a692f25a6e24de..c0127d0fb4f059b9f9e816360130e183e8f0e990 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -867,6 +867,13 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
 	int err;
 	u32 val;
 
+	/*
+	 * The controller may have been left out of reset by the bootloader
+	 * so make sure that we get a clean start by asserting resets here.
+	 */
+	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
+				  pcie->phy_resets);
+	reset_control_assert(pcie->mac_reset);
 	/*
 	 * Wait for the time needed to complete the bulk assert in
 	 * mtk_pcie_setup for EN7581 SoC.
@@ -941,6 +948,15 @@ static int mtk_pcie_power_up(struct mtk_gen3_pcie *pcie)
 	struct device *dev = pcie->dev;
 	int err;
 
+	/*
+	 * The controller may have been left out of reset by the bootloader
+	 * so make sure that we get a clean start by asserting resets here.
+	 */
+	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
+				  pcie->phy_resets);
+	reset_control_assert(pcie->mac_reset);
+	usleep_range(10, 20);
+
 	/* PHY power on and enable pipe clock */
 	err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
 	if (err) {
@@ -1013,14 +1029,6 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
 	 * counter since the bulk is shared.
 	 */
 	reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
-	/*
-	 * The controller may have been left out of reset by the bootloader
-	 * so make sure that we get a clean start by asserting resets here.
-	 */
-	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
-
-	reset_control_assert(pcie->mac_reset);
-	usleep_range(10, 20);
 
 	/* Don't touch the hardware registers before power up */
 	err = pcie->soc->power_up(pcie);

-- 
2.47.0

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[Bjorn Helgaas]

On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> In order to make the code more readable, move phy and mac reset lines
> assert/de-assert configuration in .power_up callback
> (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 8c8c733a145634cdbfefd339f4a692f25a6e24de..c0127d0fb4f059b9f9e816360130e183e8f0e990 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -867,6 +867,13 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
>  	int err;
>  	u32 val;
>  
> +	/*
> +	 * The controller may have been left out of reset by the bootloader
> +	 * so make sure that we get a clean start by asserting resets here.
> +	 */
> +	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> +				  pcie->phy_resets);
> +	reset_control_assert(pcie->mac_reset);

Add blank line here.

>  	/*
>  	 * Wait for the time needed to complete the bulk assert in
>  	 * mtk_pcie_setup for EN7581 SoC.
> @@ -941,6 +948,15 @@ static int mtk_pcie_power_up(struct mtk_gen3_pcie *pcie)
>  	struct device *dev = pcie->dev;
>  	int err;
>  
> +	/*
> +	 * The controller may have been left out of reset by the bootloader
> +	 * so make sure that we get a clean start by asserting resets here.
> +	 */
> +	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> +				  pcie->phy_resets);
> +	reset_control_assert(pcie->mac_reset);
> +	usleep_range(10, 20);

Unrelated to this patch, but since you're moving it, do you know what
this delay is for?  Can we add a #define and a spec citation for it?

Is there a requirement that the PHY and MAC reset ordering be
different for EN7581 vs other chips?

EN7581:

  assert PHY reset
  assert MAC reset
  power on PHY
  deassert PHY reset
  deassert MAC reset

others:

  assert PHY reset
  assert MAC reset
  deassert PHY reset
  power on PHY
  deassert MAC reset

Is there one order that would work for both?

>  	/* PHY power on and enable pipe clock */
>  	err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
>  	if (err) {
> @@ -1013,14 +1029,6 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
>  	 * counter since the bulk is shared.
>  	 */
>  	reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
> -	/*
> -	 * The controller may have been left out of reset by the bootloader
> -	 * so make sure that we get a clean start by asserting resets here.
> -	 */
> -	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
> -
> -	reset_control_assert(pcie->mac_reset);
> -	usleep_range(10, 20);
>  
>  	/* Don't touch the hardware registers before power up */
>  	err = pcie->soc->power_up(pcie);
> 
> -- 
> 2.47.0
> 

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3528 bytes --]

> On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> > In order to make the code more readable, move phy and mac reset lines
> > assert/de-assert configuration in .power_up callback
> > (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index 8c8c733a145634cdbfefd339f4a692f25a6e24de..c0127d0fb4f059b9f9e816360130e183e8f0e990 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -867,6 +867,13 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> >  	int err;
> >  	u32 val;
> >  
> > +	/*
> > +	 * The controller may have been left out of reset by the bootloader
> > +	 * so make sure that we get a clean start by asserting resets here.
> > +	 */
> > +	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> > +				  pcie->phy_resets);
> > +	reset_control_assert(pcie->mac_reset);
> 
> Add blank line here.

ack, I will fix it.

> 
> >  	/*
> >  	 * Wait for the time needed to complete the bulk assert in
> >  	 * mtk_pcie_setup for EN7581 SoC.
> > @@ -941,6 +948,15 @@ static int mtk_pcie_power_up(struct mtk_gen3_pcie *pcie)
> >  	struct device *dev = pcie->dev;
> >  	int err;
> >  
> > +	/*
> > +	 * The controller may have been left out of reset by the bootloader
> > +	 * so make sure that we get a clean start by asserting resets here.
> > +	 */
> > +	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> > +				  pcie->phy_resets);
> > +	reset_control_assert(pcie->mac_reset);
> > +	usleep_range(10, 20);
> 
> Unrelated to this patch, but since you're moving it, do you know what
> this delay is for?  Can we add a #define and a spec citation for it?

I am not sure about it, this was already there.
@Jianjun Wang: any input on it?

> 
> Is there a requirement that the PHY and MAC reset ordering be
> different for EN7581 vs other chips?
> 
> EN7581:
> 
>   assert PHY reset
>   assert MAC reset
>   power on PHY
>   deassert PHY reset
>   deassert MAC reset
> 
> others:
> 
>   assert PHY reset
>   assert MAC reset
>   deassert PHY reset
>   power on PHY
>   deassert MAC reset
> 
> Is there one order that would work for both?

EN7581 requires to run phy_init()/phy_power_on() before deassert PHY reset
lines.

Regards,
Lorenzo

> 
> >  	/* PHY power on and enable pipe clock */
> >  	err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
> >  	if (err) {
> > @@ -1013,14 +1029,6 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
> >  	 * counter since the bulk is shared.
> >  	 */
> >  	reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
> > -	/*
> > -	 * The controller may have been left out of reset by the bootloader
> > -	 * so make sure that we get a clean start by asserting resets here.
> > -	 */
> > -	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
> > -
> > -	reset_control_assert(pcie->mac_reset);
> > -	usleep_range(10, 20);
> >  
> >  	/* Don't touch the hardware registers before power up */
> >  	err = pcie->soc->power_up(pcie);
> > 
> > -- 
> > 2.47.0
> > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[Bjorn Helgaas]

On Thu, Nov 07, 2024 at 05:08:55PM +0100, Lorenzo Bianconi wrote:
> > On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> > > In order to make the code more readable, move phy and mac reset lines
> > > assert/de-assert configuration in .power_up callback
> > > (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> ...

> > Is there a requirement that the PHY and MAC reset ordering be
> > different for EN7581 vs other chips?
> > 
> > EN7581:
> > 
> >   assert PHY reset
> >   assert MAC reset
> >   power on PHY
> >   deassert PHY reset
> >   deassert MAC reset
> > 
> > others:
> > 
> >   assert PHY reset
> >   assert MAC reset
> >   deassert PHY reset
> >   power on PHY
> >   deassert MAC reset
> > 
> > Is there one order that would work for both?
> 
> EN7581 requires to run phy_init()/phy_power_on() before deassert PHY reset
> lines.

And the other chips require the PHY power-on to be *after* deasserting
PHY reset?

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1156 bytes --]

On Nov 07, Bjorn Helgaas wrote:
> On Thu, Nov 07, 2024 at 05:08:55PM +0100, Lorenzo Bianconi wrote:
> > > On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> > > > In order to make the code more readable, move phy and mac reset lines
> > > > assert/de-assert configuration in .power_up callback
> > > > (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> > ...
> 
> > > Is there a requirement that the PHY and MAC reset ordering be
> > > different for EN7581 vs other chips?
> > > 
> > > EN7581:
> > > 
> > >   assert PHY reset
> > >   assert MAC reset
> > >   power on PHY
> > >   deassert PHY reset
> > >   deassert MAC reset
> > > 
> > > others:
> > > 
> > >   assert PHY reset
> > >   assert MAC reset
> > >   deassert PHY reset
> > >   power on PHY
> > >   deassert MAC reset
> > > 
> > > Is there one order that would work for both?
> > 
> > EN7581 requires to run phy_init()/phy_power_on() before deassert PHY reset
> > lines.
> 
> And the other chips require the PHY power-on to be *after* deasserting
> PHY reset?

I am not sure about it, this is the only Airoha device I have.

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[Jianjun Wang (王建军)]

On Thu, 2024-11-07 at 10:21 -0600, Bjorn Helgaas wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Thu, Nov 07, 2024 at 05:08:55PM +0100, Lorenzo Bianconi wrote:
> > > On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> > > > In order to make the code more readable, move phy and mac reset
> > > > lines
> > > > assert/de-assert configuration in .power_up callback
> > > > (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> > 
> > ...
> > > Is there a requirement that the PHY and MAC reset ordering be
> > > different for EN7581 vs other chips?
> > > 
> > > EN7581:
> > > 
> > >   assert PHY reset
> > >   assert MAC reset
> > >   power on PHY
> > >   deassert PHY reset
> > >   deassert MAC reset
> > > 
> > > others:
> > > 
> > >   assert PHY reset
> > >   assert MAC reset
> > >   deassert PHY reset
> > >   power on PHY
> > >   deassert MAC reset
> > > 
> > > Is there one order that would work for both?
> > 
> > EN7581 requires to run phy_init()/phy_power_on() before deassert
> > PHY reset
> > lines.
> 
> And the other chips require the PHY power-on to be *after*
> deasserting
> PHY reset?

For MediaTek's chips, the reset will clear all register values and
reset the hardware state. Therefore, we can only initialize and power-
on the MAC and PHY after deasserting their resets.

Thanks.

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[Bjorn Helgaas]

On Fri, Nov 08, 2024 at 02:51:15AM +0000, Jianjun Wang (王建军) wrote:
> On Thu, 2024-11-07 at 10:21 -0600, Bjorn Helgaas wrote:
> > On Thu, Nov 07, 2024 at 05:08:55PM +0100, Lorenzo Bianconi wrote:
> > > > On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> > > > > In order to make the code more readable, move phy and mac
> > > > > reset lines assert/de-assert configuration in .power_up
> > > > > callback (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> > > 
> > > ...
> > > > Is there a requirement that the PHY and MAC reset ordering be
> > > > different for EN7581 vs other chips?
> > > > 
> > > > EN7581:
> > > > 
> > > >   assert PHY reset
> > > >   assert MAC reset
> > > >   power on PHY
> > > >   deassert PHY reset
> > > >   deassert MAC reset
> > > > 
> > > > others:
> > > > 
> > > >   assert PHY reset
> > > >   assert MAC reset
> > > >   deassert PHY reset
> > > >   power on PHY
> > > >   deassert MAC reset
> > > > 
> > > > Is there one order that would work for both?
> > > 
> > > EN7581 requires to run phy_init()/phy_power_on() before deassert
> > > PHY reset lines.
> > 
> > And the other chips require the PHY power-on to be *after*
> > deasserting PHY reset?
> 
> For MediaTek's chips, the reset will clear all register values and
> reset the hardware state. Therefore, we can only initialize and
> power-on the MAC and PHY after deasserting their resets.

OK, it sounds like you're saying the Airoha EN7581 is not a MediaTek
chip and does require a different ordering of PHY reset deassert and
PHY power-on:

  - EN7581 requires PHY power-on before PHY reset deassert,

  - other chips require PHY reset deassert before PHY power-on.

That's fine and probably worth a short comment in
mtk_pcie_en7581_power_up(), e.g., "Unlike the MediaTek controllers,
the Airoha EN7581 requires PHY power-on before PHY reset deassert".

Bjorn

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[lorenzo]

[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]

On Nov 08, Bjorn Helgaas wrote:
> On Fri, Nov 08, 2024 at 02:51:15AM +0000, Jianjun Wang (王建军) wrote:
> > On Thu, 2024-11-07 at 10:21 -0600, Bjorn Helgaas wrote:
> > > On Thu, Nov 07, 2024 at 05:08:55PM +0100, Lorenzo Bianconi wrote:
> > > > > On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> > > > > > In order to make the code more readable, move phy and mac
> > > > > > reset lines assert/de-assert configuration in .power_up
> > > > > > callback (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> > > > 
> > > > ...
> > > > > Is there a requirement that the PHY and MAC reset ordering be
> > > > > different for EN7581 vs other chips?
> > > > > 
> > > > > EN7581:
> > > > > 
> > > > >   assert PHY reset
> > > > >   assert MAC reset
> > > > >   power on PHY
> > > > >   deassert PHY reset
> > > > >   deassert MAC reset
> > > > > 
> > > > > others:
> > > > > 
> > > > >   assert PHY reset
> > > > >   assert MAC reset
> > > > >   deassert PHY reset
> > > > >   power on PHY
> > > > >   deassert MAC reset
> > > > > 
> > > > > Is there one order that would work for both?
> > > > 
> > > > EN7581 requires to run phy_init()/phy_power_on() before deassert
> > > > PHY reset lines.
> > > 
> > > And the other chips require the PHY power-on to be *after*
> > > deasserting PHY reset?
> > 
> > For MediaTek's chips, the reset will clear all register values and
> > reset the hardware state. Therefore, we can only initialize and
> > power-on the MAC and PHY after deasserting their resets.
> 
> OK, it sounds like you're saying the Airoha EN7581 is not a MediaTek
> chip and does require a different ordering of PHY reset deassert and
> PHY power-on:
> 
>   - EN7581 requires PHY power-on before PHY reset deassert,
> 
>   - other chips require PHY reset deassert before PHY power-on.
> 
> That's fine and probably worth a short comment in
> mtk_pcie_en7581_power_up(), e.g., "Unlike the MediaTek controllers,
> the Airoha EN7581 requires PHY power-on before PHY reset deassert".
> 
> Bjorn

ack, I will add it.

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[Jianjun Wang (王建军)]

On Thu, 2024-11-07 at 17:08 +0100, Lorenzo Bianconi wrote:
> > On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> > > In order to make the code more readable, move phy and mac reset
> > > lines
> > > assert/de-assert configuration in .power_up callback
> > > (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> > > 
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  drivers/pci/controller/pcie-mediatek-gen3.c | 24
> > > ++++++++++++++++--------
> > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > index
> > > 8c8c733a145634cdbfefd339f4a692f25a6e24de..c0127d0fb4f059b9f9e8163
> > > 60130e183e8f0e990 100644
> > > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > @@ -867,6 +867,13 @@ static int mtk_pcie_en7581_power_up(struct
> > > mtk_gen3_pcie *pcie)
> > >  	int err;
> > >  	u32 val;
> > >  
> > > +	/*
> > > +	 * The controller may have been left out of reset by the
> > > bootloader
> > > +	 * so make sure that we get a clean start by asserting resets
> > > here.
> > > +	 */
> > > +	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> > > +				  pcie->phy_resets);
> > > +	reset_control_assert(pcie->mac_reset);
> > 
> > Add blank line here.
> 
> ack, I will fix it.
> 
> > 
> > >  	/*
> > >  	 * Wait for the time needed to complete the bulk assert in
> > >  	 * mtk_pcie_setup for EN7581 SoC.
> > > @@ -941,6 +948,15 @@ static int mtk_pcie_power_up(struct
> > > mtk_gen3_pcie *pcie)
> > >  	struct device *dev = pcie->dev;
> > >  	int err;
> > >  
> > > +	/*
> > > +	 * The controller may have been left out of reset by the
> > > bootloader
> > > +	 * so make sure that we get a clean start by asserting resets
> > > here.
> > > +	 */
> > > +	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> > > +				  pcie->phy_resets);
> > > +	reset_control_assert(pcie->mac_reset);
> > > +	usleep_range(10, 20);
> > 
> > Unrelated to this patch, but since you're moving it, do you know
> > what
> > this delay is for?  Can we add a #define and a spec citation for
> > it?
> 
> I am not sure about it, this was already there.
> @Jianjun Wang: any input on it?

This delay is used to ensure the reset is effective. A delay of 10us
should be sufficient in this scenario.

> 
> > 
> > Is there a requirement that the PHY and MAC reset ordering be
> > different for EN7581 vs other chips?
> > 
> > EN7581:
> > 
> >   assert PHY reset
> >   assert MAC reset
> >   power on PHY
> >   deassert PHY reset
> >   deassert MAC reset
> > 
> > others:
> > 
> >   assert PHY reset
> >   assert MAC reset
> >   deassert PHY reset
> >   power on PHY
> >   deassert MAC reset
> > 
> > Is there one order that would work for both?
> 
> EN7581 requires to run phy_init()/phy_power_on() before deassert PHY
> reset
> lines.
> 
> Regards,
> Lorenzo
> 
> > 
> > >  	/* PHY power on and enable pipe clock */
> > >  	err = reset_control_bulk_deassert(pcie->soc-
> > > >phy_resets.num_resets, pcie->phy_resets);
> > >  	if (err) {
> > > @@ -1013,14 +1029,6 @@ static int mtk_pcie_setup(struct
> > > mtk_gen3_pcie *pcie)
> > >  	 * counter since the bulk is shared.
> > >  	 */
> > >  	reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets,
> > > pcie->phy_resets);
> > > -	/*
> > > -	 * The controller may have been left out of reset by the
> > > bootloader
> > > -	 * so make sure that we get a clean start by asserting resets
> > > here.
> > > -	 */
> > > -	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> > > pcie->phy_resets);
> > > -
> > > -	reset_control_assert(pcie->mac_reset);
> > > -	usleep_range(10, 20);
> > >  
> > >  	/* Don't touch the hardware registers before power up */
> > >  	err = pcie->soc->power_up(pcie);
> > > 
> > > -- 
> > > 2.47.0
> > > 

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[lorenzo]

[-- Attachment #1: Type: text/plain, Size: 4488 bytes --]

> On Thu, 2024-11-07 at 17:08 +0100, Lorenzo Bianconi wrote:
> > > On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> > > > In order to make the code more readable, move phy and mac reset
> > > > lines
> > > > assert/de-assert configuration in .power_up callback
> > > > (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> > > > 
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > >  drivers/pci/controller/pcie-mediatek-gen3.c | 24
> > > > ++++++++++++++++--------
> > > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > index
> > > > 8c8c733a145634cdbfefd339f4a692f25a6e24de..c0127d0fb4f059b9f9e8163
> > > > 60130e183e8f0e990 100644
> > > > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > @@ -867,6 +867,13 @@ static int mtk_pcie_en7581_power_up(struct
> > > > mtk_gen3_pcie *pcie)
> > > >  	int err;
> > > >  	u32 val;
> > > >  
> > > > +	/*
> > > > +	 * The controller may have been left out of reset by the
> > > > bootloader
> > > > +	 * so make sure that we get a clean start by asserting resets
> > > > here.
> > > > +	 */
> > > > +	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> > > > +				  pcie->phy_resets);
> > > > +	reset_control_assert(pcie->mac_reset);
> > > 
> > > Add blank line here.
> > 
> > ack, I will fix it.
> > 
> > > 
> > > >  	/*
> > > >  	 * Wait for the time needed to complete the bulk assert in
> > > >  	 * mtk_pcie_setup for EN7581 SoC.
> > > > @@ -941,6 +948,15 @@ static int mtk_pcie_power_up(struct
> > > > mtk_gen3_pcie *pcie)
> > > >  	struct device *dev = pcie->dev;
> > > >  	int err;
> > > >  
> > > > +	/*
> > > > +	 * The controller may have been left out of reset by the
> > > > bootloader
> > > > +	 * so make sure that we get a clean start by asserting resets
> > > > here.
> > > > +	 */
> > > > +	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> > > > +				  pcie->phy_resets);
> > > > +	reset_control_assert(pcie->mac_reset);
> > > > +	usleep_range(10, 20);
> > > 
> > > Unrelated to this patch, but since you're moving it, do you know
> > > what
> > > this delay is for?  Can we add a #define and a spec citation for
> > > it?
> > 
> > I am not sure about it, this was already there.
> > @Jianjun Wang: any input on it?
> 
> This delay is used to ensure the reset is effective. A delay of 10us
> should be sufficient in this scenario.

ack, so we can introduce a marco like:

#define PCIE_RESET_TIME_US	10
...

usleep_range(PCIE_RESET_TIME_US, 2 * PCIE_RESET_TIME_US);

what do you think?

Regards,
Lorenzo

> 
> > 
> > > 
> > > Is there a requirement that the PHY and MAC reset ordering be
> > > different for EN7581 vs other chips?
> > > 
> > > EN7581:
> > > 
> > >   assert PHY reset
> > >   assert MAC reset
> > >   power on PHY
> > >   deassert PHY reset
> > >   deassert MAC reset
> > > 
> > > others:
> > > 
> > >   assert PHY reset
> > >   assert MAC reset
> > >   deassert PHY reset
> > >   power on PHY
> > >   deassert MAC reset
> > > 
> > > Is there one order that would work for both?
> > 
> > EN7581 requires to run phy_init()/phy_power_on() before deassert PHY
> > reset
> > lines.
> > 
> > Regards,
> > Lorenzo
> > 
> > > 
> > > >  	/* PHY power on and enable pipe clock */
> > > >  	err = reset_control_bulk_deassert(pcie->soc-
> > > > >phy_resets.num_resets, pcie->phy_resets);
> > > >  	if (err) {
> > > > @@ -1013,14 +1029,6 @@ static int mtk_pcie_setup(struct
> > > > mtk_gen3_pcie *pcie)
> > > >  	 * counter since the bulk is shared.
> > > >  	 */
> > > >  	reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets,
> > > > pcie->phy_resets);
> > > > -	/*
> > > > -	 * The controller may have been left out of reset by the
> > > > bootloader
> > > > -	 * so make sure that we get a clean start by asserting resets
> > > > here.
> > > > -	 */
> > > > -	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> > > > pcie->phy_resets);
> > > > -
> > > > -	reset_control_assert(pcie->mac_reset);
> > > > -	usleep_range(10, 20);
> > > >  
> > > >  	/* Don't touch the hardware registers before power up */
> > > >  	err = pcie->soc->power_up(pcie);
> > > > 
> > > > -- 
> > > > 2.47.0
> > > > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[Bjorn Helgaas]

On Fri, Nov 08, 2024 at 09:39:41AM +0100, lorenzo@kernel.org wrote:
> > On Thu, 2024-11-07 at 17:08 +0100, Lorenzo Bianconi wrote:
> > > > On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> > > > > In order to make the code more readable, move phy and mac
> > > > > reset lines assert/de-assert configuration in .power_up
> > > > > callback (mtk_pcie_en7581_power_up/mtk_pcie_power_up).

> > > > > +	/*
> > > > > +	 * The controller may have been left out of reset by the
> > > > > bootloader
> > > > > +	 * so make sure that we get a clean start by asserting resets
> > > > > here.
> > > > > +	 */
> > > > > +	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> > > > > +				  pcie->phy_resets);
> > > > > +	reset_control_assert(pcie->mac_reset);
> > > > > +	usleep_range(10, 20);
> > > > 
> > > > Unrelated to this patch, but since you're moving it, do you
> > > > know what this delay is for?  Can we add a #define and a spec
> > > > citation for it?
> > > 
> > > I am not sure about it, this was already there.  @Jianjun Wang:
> > > any input on it?
> > 
> > This delay is used to ensure the reset is effective. A delay of
> > 10us should be sufficient in this scenario.
> 
> ack, so we can introduce a marco like:
> 
> #define PCIE_RESET_TIME_US	10
> ...
> 
> usleep_range(PCIE_RESET_TIME_US, 2 * PCIE_RESET_TIME_US);

Unless this corresponds to a value specified by the PCIe base spec or
CEM spec, this macro should be internal to pcie-mediatek-gen3.c.

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[lorenzo]

[-- Attachment #1: Type: text/plain, Size: 1716 bytes --]

> On Fri, Nov 08, 2024 at 09:39:41AM +0100, lorenzo@kernel.org wrote:
> > > On Thu, 2024-11-07 at 17:08 +0100, Lorenzo Bianconi wrote:
> > > > > On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> > > > > > In order to make the code more readable, move phy and mac
> > > > > > reset lines assert/de-assert configuration in .power_up
> > > > > > callback (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> 
> > > > > > +	/*
> > > > > > +	 * The controller may have been left out of reset by the
> > > > > > bootloader
> > > > > > +	 * so make sure that we get a clean start by asserting resets
> > > > > > here.
> > > > > > +	 */
> > > > > > +	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> > > > > > +				  pcie->phy_resets);
> > > > > > +	reset_control_assert(pcie->mac_reset);
> > > > > > +	usleep_range(10, 20);
> > > > > 
> > > > > Unrelated to this patch, but since you're moving it, do you
> > > > > know what this delay is for?  Can we add a #define and a spec
> > > > > citation for it?
> > > > 
> > > > I am not sure about it, this was already there.  @Jianjun Wang:
> > > > any input on it?
> > > 
> > > This delay is used to ensure the reset is effective. A delay of
> > > 10us should be sufficient in this scenario.
> > 
> > ack, so we can introduce a marco like:
> > 
> > #define PCIE_RESET_TIME_US	10
> > ...
> > 
> > usleep_range(PCIE_RESET_TIME_US, 2 * PCIE_RESET_TIME_US);
> 
> Unless this corresponds to a value specified by the PCIe base spec or
> CEM spec, this macro should be internal to pcie-mediatek-gen3.c.

My plan is to add it in pcie-mediatek-gen3.c. Do you think PCIE_RESET_TIME_US
is too generic?

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[Bjorn Helgaas]

On Fri, Nov 08, 2024 at 06:04:13PM +0100, lorenzo@kernel.org wrote:
> > On Fri, Nov 08, 2024 at 09:39:41AM +0100, lorenzo@kernel.org wrote:
> > > > On Thu, 2024-11-07 at 17:08 +0100, Lorenzo Bianconi wrote:
> > > > > > On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> > > > > > > In order to make the code more readable, move phy and mac
> > > > > > > reset lines assert/de-assert configuration in .power_up
> > > > > > > callback (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> > 
> > > > > > > +	/*
> > > > > > > +	 * The controller may have been left out of reset by the
> > > > > > > bootloader
> > > > > > > +	 * so make sure that we get a clean start by asserting resets
> > > > > > > here.
> > > > > > > +	 */
> > > > > > > +	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> > > > > > > +				  pcie->phy_resets);
> > > > > > > +	reset_control_assert(pcie->mac_reset);
> > > > > > > +	usleep_range(10, 20);
> > > > > > 
> > > > > > Unrelated to this patch, but since you're moving it, do you
> > > > > > know what this delay is for?  Can we add a #define and a spec
> > > > > > citation for it?
> > > > > 
> > > > > I am not sure about it, this was already there.  @Jianjun Wang:
> > > > > any input on it?
> > > > 
> > > > This delay is used to ensure the reset is effective. A delay of
> > > > 10us should be sufficient in this scenario.
> > > 
> > > ack, so we can introduce a marco like:
> > > 
> > > #define PCIE_RESET_TIME_US	10
> > > ...
> > > 
> > > usleep_range(PCIE_RESET_TIME_US, 2 * PCIE_RESET_TIME_US);
> > 
> > Unless this corresponds to a value specified by the PCIe base spec
> > or CEM spec, this macro should be internal to
> > pcie-mediatek-gen3.c.
> 
> My plan is to add it in pcie-mediatek-gen3.c. Do you think
> PCIE_RESET_TIME_US is too generic?

It's generic, but so are most of the other #defines in
pcie-mediatek-gen3.c, so I'd follow suit.

Connect it to language in the MediaTek spec if possible, i.e., if the
spec names this parameter, try to use the same name.

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[lorenzo]

[-- Attachment #1: Type: text/plain, Size: 2400 bytes --]

On Nov 08, Bjorn Helgaas wrote:
> On Fri, Nov 08, 2024 at 06:04:13PM +0100, lorenzo@kernel.org wrote:
> > > On Fri, Nov 08, 2024 at 09:39:41AM +0100, lorenzo@kernel.org wrote:
> > > > > On Thu, 2024-11-07 at 17:08 +0100, Lorenzo Bianconi wrote:
> > > > > > > On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> > > > > > > > In order to make the code more readable, move phy and mac
> > > > > > > > reset lines assert/de-assert configuration in .power_up
> > > > > > > > callback (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> > > 
> > > > > > > > +	/*
> > > > > > > > +	 * The controller may have been left out of reset by the
> > > > > > > > bootloader
> > > > > > > > +	 * so make sure that we get a clean start by asserting resets
> > > > > > > > here.
> > > > > > > > +	 */
> > > > > > > > +	reset_control_bulk_assert(pcie->soc->phy_resets.num_resets,
> > > > > > > > +				  pcie->phy_resets);
> > > > > > > > +	reset_control_assert(pcie->mac_reset);
> > > > > > > > +	usleep_range(10, 20);
> > > > > > > 
> > > > > > > Unrelated to this patch, but since you're moving it, do you
> > > > > > > know what this delay is for?  Can we add a #define and a spec
> > > > > > > citation for it?
> > > > > > 
> > > > > > I am not sure about it, this was already there.  @Jianjun Wang:
> > > > > > any input on it?
> > > > > 
> > > > > This delay is used to ensure the reset is effective. A delay of
> > > > > 10us should be sufficient in this scenario.
> > > > 
> > > > ack, so we can introduce a marco like:
> > > > 
> > > > #define PCIE_RESET_TIME_US	10
> > > > ...
> > > > 
> > > > usleep_range(PCIE_RESET_TIME_US, 2 * PCIE_RESET_TIME_US);
> > > 
> > > Unless this corresponds to a value specified by the PCIe base spec
> > > or CEM spec, this macro should be internal to
> > > pcie-mediatek-gen3.c.
> > 
> > My plan is to add it in pcie-mediatek-gen3.c. Do you think
> > PCIE_RESET_TIME_US is too generic?
> 
> It's generic, but so are most of the other #defines in
> pcie-mediatek-gen3.c, so I'd follow suit.
> 
> Connect it to language in the MediaTek spec if possible, i.e., if the
> spec names this parameter, try to use the same name.

unfortunately I do not have any mediatek spec documentation available.

@Jianjun Wang: is PCIE_RESET_TIME_US fine for you or according to the available
documentation?

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

[Jianjun Wang (王建军)]

On Fri, 2024-11-08 at 18:23 +0100, lorenzo@kernel.org wrote:
> On Nov 08, Bjorn Helgaas wrote:
> > On Fri, Nov 08, 2024 at 06:04:13PM +0100, lorenzo@kernel.org wrote:
> > > > On Fri, Nov 08, 2024 at 09:39:41AM +0100, lorenzo@kernel.org
> > > > wrote:
> > > > > > On Thu, 2024-11-07 at 17:08 +0100, Lorenzo Bianconi wrote:
> > > > > > > > On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo
> > > > > > > > Bianconi wrote:
> > > > > > > > > In order to make the code more readable, move phy and
> > > > > > > > > mac
> > > > > > > > > reset lines assert/de-assert configuration in
> > > > > > > > > .power_up
> > > > > > > > > callback
> > > > > > > > > (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> > > > > > > > > +	/*
> > > > > > > > > +	 * The controller may have been left out of
> > > > > > > > > reset by the
> > > > > > > > > bootloader
> > > > > > > > > +	 * so make sure that we get a clean start by
> > > > > > > > > asserting resets
> > > > > > > > > here.
> > > > > > > > > +	 */
> > > > > > > > > +	reset_control_bulk_assert(pcie->soc-
> > > > > > > > > >phy_resets.num_resets,
> > > > > > > > > +				  pcie->phy_resets);
> > > > > > > > > +	reset_control_assert(pcie->mac_reset);
> > > > > > > > > +	usleep_range(10, 20);
> > > > > > > > 
> > > > > > > > Unrelated to this patch, but since you're moving it, do
> > > > > > > > you
> > > > > > > > know what this delay is for?  Can we add a #define and
> > > > > > > > a spec
> > > > > > > > citation for it?
> > > > > > > 
> > > > > > > I am not sure about it, this was already there.  @Jianjun
> > > > > > > Wang:
> > > > > > > any input on it?
> > > > > > 
> > > > > > This delay is used to ensure the reset is effective. A
> > > > > > delay of
> > > > > > 10us should be sufficient in this scenario.
> > > > > 
> > > > > ack, so we can introduce a marco like:
> > > > > 
> > > > > #define PCIE_RESET_TIME_US	10
> > > > > ...
> > > > > 
> > > > > usleep_range(PCIE_RESET_TIME_US, 2 * PCIE_RESET_TIME_US);
> > > > 
> > > > Unless this corresponds to a value specified by the PCIe base
> > > > spec
> > > > or CEM spec, this macro should be internal to
> > > > pcie-mediatek-gen3.c.
> > > 
> > > My plan is to add it in pcie-mediatek-gen3.c. Do you think
> > > PCIE_RESET_TIME_US is too generic?
> > 
> > It's generic, but so are most of the other #defines in
> > pcie-mediatek-gen3.c, so I'd follow suit.
> > 
> > Connect it to language in the MediaTek spec if possible, i.e., if
> > the
> > spec names this parameter, try to use the same name.
> 
> unfortunately I do not have any mediatek spec documentation
> available.
> 
> @Jianjun Wang: is PCIE_RESET_TIME_US fine for you or according to the
> available
> documentation?

It's fine for me.

Thanks.

> 
> Regards,
> Lorenzo