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