| Message ID | 20241118-pcie-en7581-fixes-v4-3-24bb61703ad7@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: mediatek-gen3: mtk_pcie_en7581_power_up() code refactoring | expand |
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
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
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
> 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
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
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);