diff mbox series

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

Message ID 20241118-pcie-en7581-fixes-v4-3-24bb61703ad7@kernel.org (mailing list archive)
State New
Headers show
Series PCI: mediatek-gen3: mtk_pcie_en7581_power_up() code refactoring | expand

Commit Message

Lorenzo Bianconi Nov. 18, 2024, 8:04 a.m. UTC 
In order to make the code more readable, the reset_control_bulk_assert()
for PHY reset lines is moved to make it pair with
reset_control_bulk_deassert() in mtk_pcie_power_up() and
mtk_pcie_en7581_power_up(). The same change is done for
reset_control_assert() used to assert MAC reset line.

Introduce PCIE_MTK_RESET_TIME_US macro for the time needed to
complete PCIe reset on MediaTek controller.

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/pci/controller/pcie-mediatek-gen3.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

Comments

Philipp Zabel Nov. 18, 2024, 8:49 a.m. UTC | #1 
On Mo, 2024-11-18 at 09:04 +0100, Lorenzo Bianconi wrote:
> In order to make the code more readable, the reset_control_bulk_assert()
> for PHY reset lines is moved to make it pair with
> reset_control_bulk_deassert() in mtk_pcie_power_up() and
> mtk_pcie_en7581_power_up(). The same change is done for
> reset_control_assert() used to assert MAC reset line.
> 
> Introduce PCIE_MTK_RESET_TIME_US macro for the time needed to
> complete PCIe reset on MediaTek controller.
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 3cfcb45d31508142d28d338ff213f70de9b4e608..2b80edd4462ad4e9f2a5d192db7f99307113eb8a 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -125,6 +125,8 @@
>  
>  #define MAX_NUM_PHY_RESETS		3
>  
> +#define PCIE_MTK_RESET_TIME_US		10
> +
>  /* Time in ms needed to complete PCIe reset on EN7581 SoC */
>  #define PCIE_EN7581_RESET_TIME_MS	100
>  
> @@ -912,6 +914,14 @@ 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.

This comment is not correct anymore.


regards
Philipp
Lorenzo Bianconi Nov. 18, 2024, 9:03 a.m. UTC | #2 
On Nov 18, Philipp Zabel wrote:
> On Mo, 2024-11-18 at 09:04 +0100, Lorenzo Bianconi wrote:
> > In order to make the code more readable, the reset_control_bulk_assert()
> > for PHY reset lines is moved to make it pair with
> > reset_control_bulk_deassert() in mtk_pcie_power_up() and
> > mtk_pcie_en7581_power_up(). The same change is done for
> > reset_control_assert() used to assert MAC reset line.
> > 
> > Introduce PCIE_MTK_RESET_TIME_US macro for the time needed to
> > complete PCIe reset on MediaTek controller.
> > 
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  drivers/pci/controller/pcie-mediatek-gen3.c | 27 +++++++++++++++++++--------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index 3cfcb45d31508142d28d338ff213f70de9b4e608..2b80edd4462ad4e9f2a5d192db7f99307113eb8a 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -125,6 +125,8 @@
> >  
> >  #define MAX_NUM_PHY_RESETS		3
> >  
> > +#define PCIE_MTK_RESET_TIME_US		10
> > +
> >  /* Time in ms needed to complete PCIe reset on EN7581 SoC */
> >  #define PCIE_EN7581_RESET_TIME_MS	100
> >  
> > @@ -912,6 +914,14 @@ 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.
> 
> This comment is not correct anymore.

I agree naming is hard, but I guess we can assume with 'bulk' we refer to both
phy and mac reset (similar to what we have in mtk_pcie_power_up()),
what do you think?

Regards,
Lorenzo

> 
> 
> regards
> Philipp
Philipp Zabel Nov. 18, 2024, 11:01 a.m. UTC | #3 
On Mo, 2024-11-18 at 10:03 +0100, Lorenzo Bianconi wrote:
> On Nov 18, Philipp Zabel wrote:
> > On Mo, 2024-11-18 at 09:04 +0100, Lorenzo Bianconi wrote:
> > > In order to make the code more readable, the reset_control_bulk_assert()
> > > for PHY reset lines is moved to make it pair with
> > > reset_control_bulk_deassert() in mtk_pcie_power_up() and
> > > mtk_pcie_en7581_power_up(). The same change is done for
> > > reset_control_assert() used to assert MAC reset line.
> > > 
> > > Introduce PCIE_MTK_RESET_TIME_US macro for the time needed to
> > > complete PCIe reset on MediaTek controller.
> > > 
> > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  drivers/pci/controller/pcie-mediatek-gen3.c | 27 +++++++++++++++++++--------
> > >  1 file changed, 19 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > index 3cfcb45d31508142d28d338ff213f70de9b4e608..2b80edd4462ad4e9f2a5d192db7f99307113eb8a 100644
> > > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > @@ -125,6 +125,8 @@
> > >  
> > >  #define MAX_NUM_PHY_RESETS		3
> > >  
> > > +#define PCIE_MTK_RESET_TIME_US		10
> > > +
> > >  /* Time in ms needed to complete PCIe reset on EN7581 SoC */
> > >  #define PCIE_EN7581_RESET_TIME_MS	100
> > >  
> > > @@ -912,6 +914,14 @@ 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.
> > 
> > This comment is not correct anymore.
> 
> I agree naming is hard, but I guess we can assume with 'bulk' we refer to both
> phy and mac reset (similar to what we have in mtk_pcie_power_up()),
> what do you think?

My point is that the referenced (bulk) assert isn't in mtk_pcie_setup()
anymore - it was just moved right above this comment.

I wonder why that delay is required at all, though. Does the reset
controller driver return from reset_control_ops::assert before the
reset line to the PCI controller is asserted?

regards
Philipp
Lorenzo Bianconi Nov. 18, 2024, 11:48 a.m. UTC | #4 
> On Mo, 2024-11-18 at 10:03 +0100, Lorenzo Bianconi wrote:
> > On Nov 18, Philipp Zabel wrote:
> > > On Mo, 2024-11-18 at 09:04 +0100, Lorenzo Bianconi wrote:
> > > > In order to make the code more readable, the reset_control_bulk_assert()
> > > > for PHY reset lines is moved to make it pair with
> > > > reset_control_bulk_deassert() in mtk_pcie_power_up() and
> > > > mtk_pcie_en7581_power_up(). The same change is done for
> > > > reset_control_assert() used to assert MAC reset line.
> > > > 
> > > > Introduce PCIE_MTK_RESET_TIME_US macro for the time needed to
> > > > complete PCIe reset on MediaTek controller.
> > > > 
> > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > >  drivers/pci/controller/pcie-mediatek-gen3.c | 27 +++++++++++++++++++--------
> > > >  1 file changed, 19 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > index 3cfcb45d31508142d28d338ff213f70de9b4e608..2b80edd4462ad4e9f2a5d192db7f99307113eb8a 100644
> > > > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > @@ -125,6 +125,8 @@
> > > >  
> > > >  #define MAX_NUM_PHY_RESETS		3
> > > >  
> > > > +#define PCIE_MTK_RESET_TIME_US		10
> > > > +
> > > >  /* Time in ms needed to complete PCIe reset on EN7581 SoC */
> > > >  #define PCIE_EN7581_RESET_TIME_MS	100
> > > >  
> > > > @@ -912,6 +914,14 @@ 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.
> > > 
> > > This comment is not correct anymore.
> > 
> > I agree naming is hard, but I guess we can assume with 'bulk' we refer to both
> > phy and mac reset (similar to what we have in mtk_pcie_power_up()),
> > what do you think?
> 
> My point is that the referenced (bulk) assert isn't in mtk_pcie_setup()
> anymore - it was just moved right above this comment.

ok, thx. I got what you mean now, I will fix it.

> 
> I wonder why that delay is required at all, though. Does the reset
> controller driver return from reset_control_ops::assert before the
> reset line to the PCI controller is asserted?

We discussed it in a previous revision and Bjorn requested to have the required delaies 
in the PCIe driver.

https://patchwork.kernel.org/project/linux-pci/patch/aca00bd672ee576ad96d279414fc0835ff31f637.1720022580.git.lorenzo@kernel.org/#26110282

Btw this the same approach we had in mtk_pcie_setup() before this series (usleep_range(10, 20)).

Regards,
Lorenzo

> 
> regards
> Philipp
Philipp Zabel Nov. 18, 2024, 12:07 p.m. UTC | #5 
On Mo, 2024-11-18 at 12:48 +0100, Lorenzo Bianconi wrote:
> > On Mo, 2024-11-18 at 10:03 +0100, Lorenzo Bianconi wrote:
> > > On Nov 18, Philipp Zabel wrote:
> > > > On Mo, 2024-11-18 at 09:04 +0100, Lorenzo Bianconi wrote:
> > > > > In order to make the code more readable, the
> > > > > reset_control_bulk_assert()
> > > > > for PHY reset lines is moved to make it pair with
> > > > > reset_control_bulk_deassert() in mtk_pcie_power_up() and
> > > > > mtk_pcie_en7581_power_up(). The same change is done for
> > > > > reset_control_assert() used to assert MAC reset line.
> > > > > 
> > > > > Introduce PCIE_MTK_RESET_TIME_US macro for the time needed to
> > > > > complete PCIe reset on MediaTek controller.
> > > > > 
> > > > > Reviewed-by: AngeloGioacchino Del Regno
> > > > > <angelogioacchino.delregno@collabora.com>
> > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > ---
> > > > >  drivers/pci/controller/pcie-mediatek-gen3.c | 27
> > > > > +++++++++++++++++++--------
> > > > >  1 file changed, 19 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > > b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > > index
> > > > > 3cfcb45d31508142d28d338ff213f70de9b4e608..2b80edd4462ad4e9f2a
> > > > > 5d192db7f99307113eb8a 100644
> > > > > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > > @@ -125,6 +125,8 @@
> > > > >  
> > > > >  #define MAX_NUM_PHY_RESETS		3
> > > > >  
> > > > > +#define PCIE_MTK_RESET_TIME_US		10
> > > > > +
> > > > >  /* Time in ms needed to complete PCIe reset on EN7581 SoC */
> > > > >  #define PCIE_EN7581_RESET_TIME_MS	100
> > > > >  
> > > > > @@ -912,6 +914,14 @@ 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.
> > > > 
> > > > This comment is not correct anymore.
> > > 
> > > I agree naming is hard, but I guess we can assume with 'bulk' we
> > > refer to both
> > > phy and mac reset (similar to what we have in
> > > mtk_pcie_power_up()),
> > > what do you think?
> > 
> > My point is that the referenced (bulk) assert isn't in
> > mtk_pcie_setup()
> > anymore - it was just moved right above this comment.
> 
> ok, thx. I got what you mean now, I will fix it.
> 
> > 
> > I wonder why that delay is required at all, though. Does the reset
> > controller driver return from reset_control_ops::assert before the
> > reset line to the PCI controller is asserted?
> 
> We discussed it in a previous revision and Bjorn requested to have
> the required delaies 
> in the PCIe driver.
> 
> https://patchwork.kernel.org/project/linux-pci/patch/aca00bd672ee576ad96d279414fc0835ff31f637.1720022580.git.lorenzo@kernel.org/#26110282

Ok, thank you.

regards
Philipp
Manivannan Sadhasivam Nov. 23, 2024, 9:06 a.m. UTC | #6 
On Mon, Nov 18, 2024 at 09:04:55AM +0100, Lorenzo Bianconi wrote:
> In order to make the code more readable, the reset_control_bulk_assert()
> for PHY reset lines is moved to make it pair with
> reset_control_bulk_deassert() in mtk_pcie_power_up() and
> mtk_pcie_en7581_power_up(). The same change is done for
> reset_control_assert() used to assert MAC reset line.
> 
> Introduce PCIE_MTK_RESET_TIME_US macro for the time needed to
> complete PCIe reset on MediaTek controller.
> 
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 3cfcb45d31508142d28d338ff213f70de9b4e608..2b80edd4462ad4e9f2a5d192db7f99307113eb8a 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -125,6 +125,8 @@
>  
>  #define MAX_NUM_PHY_RESETS		3
>  
> +#define PCIE_MTK_RESET_TIME_US		10
> +
>  /* Time in ms needed to complete PCIe reset on EN7581 SoC */
>  #define PCIE_EN7581_RESET_TIME_MS	100
>  
> @@ -912,6 +914,14 @@ 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.
> @@ -986,6 +996,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(PCIE_MTK_RESET_TIME_US, 2 * PCIE_MTK_RESET_TIME_US);
> +
>  	/* PHY power on and enable pipe clock */
>  	err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
>  	if (err) {
> @@ -1070,14 +1089,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
>
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
index 3cfcb45d31508142d28d338ff213f70de9b4e608..2b80edd4462ad4e9f2a5d192db7f99307113eb8a 100644
--- a/drivers/pci/controller/pcie-mediatek-gen3.c
+++ b/drivers/pci/controller/pcie-mediatek-gen3.c
@@ -125,6 +125,8 @@ 
 
 #define MAX_NUM_PHY_RESETS		3
 
+#define PCIE_MTK_RESET_TIME_US		10
+
 /* Time in ms needed to complete PCIe reset on EN7581 SoC */
 #define PCIE_EN7581_RESET_TIME_MS	100
 
@@ -912,6 +914,14 @@  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.
@@ -986,6 +996,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(PCIE_MTK_RESET_TIME_US, 2 * PCIE_MTK_RESET_TIME_US);
+
 	/* PHY power on and enable pipe clock */
 	err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
 	if (err) {
@@ -1070,14 +1089,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);