| Message ID | 20220628220409.26545-1-m.szyprowski@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] phy: samsung: phy-exynos-pcie: sanitize init/power_on callbacks | expand |
> The exynos-pcie driver called phy_power_on() and then phy_init() for some > historical reasons. However the generic PHY framework assumes that the > proper sequence is to call phy_init() first, then phy_power_on(). The > operations done by both functions should be considered as one action and > as such they are called by the exynos-pcie driver (without doing anything > between them). The initialization is just a sequence of register writes, > which cannot be altered, without breaking the hardware operation. > > To match the generic PHY framework requirement, simply move all register > writes to the phy_init()/phy_exit() and drop power_on()/power_off() > callbacks. This way the driver will also work with the old (incorrect) PHY > initialization call sequence. > > Reported-by: Bjorn Helgaas <helgaas@kernel.org> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Reviewed-by: Chanho Park <chanho61.park@samsung.com> Best Regards, Chanho Park
On 29/06/2022 00:04, Marek Szyprowski wrote: > The exynos-pcie driver called phy_power_on() and then phy_init() for some > historical reasons. However the generic PHY framework assumes that the > proper sequence is to call phy_init() first, then phy_power_on(). The > operations done by both functions should be considered as one action and > as such they are called by the exynos-pcie driver (without doing anything > between them). The initialization is just a sequence of register writes, > which cannot be altered, without breaking the hardware operation. > > To match the generic PHY framework requirement, simply move all register > writes to the phy_init()/phy_exit() and drop power_on()/power_off() > callbacks. This way the driver will also work with the old (incorrect) > PHY initialization call sequence. > > Reported-by: Bjorn Helgaas <helgaas@kernel.org> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 29-06-22, 00:04, Marek Szyprowski wrote: > The exynos-pcie driver called phy_power_on() and then phy_init() for some > historical reasons. However the generic PHY framework assumes that the > proper sequence is to call phy_init() first, then phy_power_on(). The > operations done by both functions should be considered as one action and > as such they are called by the exynos-pcie driver (without doing anything > between them). The initialization is just a sequence of register writes, > which cannot be altered, without breaking the hardware operation. > > To match the generic PHY framework requirement, simply move all register > writes to the phy_init()/phy_exit() and drop power_on()/power_off() > callbacks. This way the driver will also work with the old (incorrect) > PHY initialization call sequence. Is the plan to merge thru pcie tree? > > Reported-by: Bjorn Helgaas <helgaas@kernel.org> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/phy/samsung/phy-exynos-pcie.c | 25 +++++++++---------------- > 1 file changed, 9 insertions(+), 16 deletions(-) > > diff --git a/drivers/phy/samsung/phy-exynos-pcie.c b/drivers/phy/samsung/phy-exynos-pcie.c > index 578cfe07d07a..53c9230c2907 100644 > --- a/drivers/phy/samsung/phy-exynos-pcie.c > +++ b/drivers/phy/samsung/phy-exynos-pcie.c > @@ -51,6 +51,13 @@ static int exynos5433_pcie_phy_init(struct phy *phy) > { > struct exynos_pcie_phy *ep = phy_get_drvdata(phy); > > + regmap_update_bits(ep->pmureg, EXYNOS5433_PMU_PCIE_PHY_OFFSET, > + BIT(0), 1); > + regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_GLOBAL_RESET, > + PCIE_APP_REQ_EXIT_L1_MODE, 0); > + regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_L1SUB_CM_CON, > + PCIE_REFCLK_GATING_EN, 0); > + why not retain exynos5433_pcie_phy_power_on() and call it from here and drop in ops. It would be clear to reader that these are for turning on the phy... > regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_COMMON_RESET, > PCIE_PHY_RESET, 1); > regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_MAC_RESET, > @@ -109,20 +116,7 @@ static int exynos5433_pcie_phy_init(struct phy *phy) > return 0; > } > > -static int exynos5433_pcie_phy_power_on(struct phy *phy) > -{ > - struct exynos_pcie_phy *ep = phy_get_drvdata(phy); > - > - regmap_update_bits(ep->pmureg, EXYNOS5433_PMU_PCIE_PHY_OFFSET, > - BIT(0), 1); > - regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_GLOBAL_RESET, > - PCIE_APP_REQ_EXIT_L1_MODE, 0); > - regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_L1SUB_CM_CON, > - PCIE_REFCLK_GATING_EN, 0); > - return 0; > -} > - > -static int exynos5433_pcie_phy_power_off(struct phy *phy) > +static int exynos5433_pcie_phy_exit(struct phy *phy) > { > struct exynos_pcie_phy *ep = phy_get_drvdata(phy); > > @@ -135,8 +129,7 @@ static int exynos5433_pcie_phy_power_off(struct phy *phy) > > static const struct phy_ops exynos5433_phy_ops = { > .init = exynos5433_pcie_phy_init, > - .power_on = exynos5433_pcie_phy_power_on, > - .power_off = exynos5433_pcie_phy_power_off, > + .exit = exynos5433_pcie_phy_exit, > .owner = THIS_MODULE, > }; > > -- > 2.17.1
On Tue, Jul 05, 2022 at 11:55:23AM +0530, Vinod Koul wrote: > On 29-06-22, 00:04, Marek Szyprowski wrote: > > The exynos-pcie driver called phy_power_on() and then phy_init() for some > > historical reasons. However the generic PHY framework assumes that the > > proper sequence is to call phy_init() first, then phy_power_on(). The > > operations done by both functions should be considered as one action and > > as such they are called by the exynos-pcie driver (without doing anything > > between them). The initialization is just a sequence of register writes, > > which cannot be altered, without breaking the hardware operation. > > > > To match the generic PHY framework requirement, simply move all register > > writes to the phy_init()/phy_exit() and drop power_on()/power_off() > > callbacks. This way the driver will also work with the old (incorrect) > > PHY initialization call sequence. > > Is the plan to merge thru pcie tree? I guess these patches should go together. I don't see any major exynos series pending, but I do have two minor pci-exynos.c patches in the queue. If you ack it (after resolution of your question below) I'd be happy to take both if it doesn't cause trouble for you. > > Reported-by: Bjorn Helgaas <helgaas@kernel.org> > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > --- > > drivers/phy/samsung/phy-exynos-pcie.c | 25 +++++++++---------------- > > 1 file changed, 9 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/phy/samsung/phy-exynos-pcie.c b/drivers/phy/samsung/phy-exynos-pcie.c > > index 578cfe07d07a..53c9230c2907 100644 > > --- a/drivers/phy/samsung/phy-exynos-pcie.c > > +++ b/drivers/phy/samsung/phy-exynos-pcie.c > > @@ -51,6 +51,13 @@ static int exynos5433_pcie_phy_init(struct phy *phy) > > { > > struct exynos_pcie_phy *ep = phy_get_drvdata(phy); > > > > + regmap_update_bits(ep->pmureg, EXYNOS5433_PMU_PCIE_PHY_OFFSET, > > + BIT(0), 1); > > + regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_GLOBAL_RESET, > > + PCIE_APP_REQ_EXIT_L1_MODE, 0); > > + regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_L1SUB_CM_CON, > > + PCIE_REFCLK_GATING_EN, 0); > > + > > why not retain exynos5433_pcie_phy_power_on() and call it from here and > drop in ops. It would be clear to reader that these are for turning on > the phy... > > > regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_COMMON_RESET, > > PCIE_PHY_RESET, 1); > > regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_MAC_RESET, > > @@ -109,20 +116,7 @@ static int exynos5433_pcie_phy_init(struct phy *phy) > > return 0; > > } > > > > -static int exynos5433_pcie_phy_power_on(struct phy *phy) > > -{ > > - struct exynos_pcie_phy *ep = phy_get_drvdata(phy); > > - > > - regmap_update_bits(ep->pmureg, EXYNOS5433_PMU_PCIE_PHY_OFFSET, > > - BIT(0), 1); > > - regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_GLOBAL_RESET, > > - PCIE_APP_REQ_EXIT_L1_MODE, 0); > > - regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_L1SUB_CM_CON, > > - PCIE_REFCLK_GATING_EN, 0); > > - return 0; > > -} > > - > > -static int exynos5433_pcie_phy_power_off(struct phy *phy) > > +static int exynos5433_pcie_phy_exit(struct phy *phy) > > { > > struct exynos_pcie_phy *ep = phy_get_drvdata(phy); > > > > @@ -135,8 +129,7 @@ static int exynos5433_pcie_phy_power_off(struct phy *phy) > > > > static const struct phy_ops exynos5433_phy_ops = { > > .init = exynos5433_pcie_phy_init, > > - .power_on = exynos5433_pcie_phy_power_on, > > - .power_off = exynos5433_pcie_phy_power_off, > > + .exit = exynos5433_pcie_phy_exit, > > .owner = THIS_MODULE, > > }; > > > > -- > > 2.17.1 > > -- > ~Vinod > > -- > linux-phy mailing list > linux-phy@lists.infradead.org > https://lists.infradead.org/mailman/listinfo/linux-phy
On 29-06-22, 00:04, Marek Szyprowski wrote: > The exynos-pcie driver called phy_power_on() and then phy_init() for some > historical reasons. However the generic PHY framework assumes that the > proper sequence is to call phy_init() first, then phy_power_on(). The > operations done by both functions should be considered as one action and > as such they are called by the exynos-pcie driver (without doing anything > between them). The initialization is just a sequence of register writes, > which cannot be altered, without breaking the hardware operation. > > To match the generic PHY framework requirement, simply move all register > writes to the phy_init()/phy_exit() and drop power_on()/power_off() > callbacks. This way the driver will also work with the old (incorrect) > PHY initialization call sequence. Acked-By: Vinod Koul <vkoul@kernel.org>
On 12-07-22, 15:12, Bjorn Helgaas wrote: > On Tue, Jul 05, 2022 at 11:55:23AM +0530, Vinod Koul wrote: > > On 29-06-22, 00:04, Marek Szyprowski wrote: > > > The exynos-pcie driver called phy_power_on() and then phy_init() for some > > > historical reasons. However the generic PHY framework assumes that the > > > proper sequence is to call phy_init() first, then phy_power_on(). The > > > operations done by both functions should be considered as one action and > > > as such they are called by the exynos-pcie driver (without doing anything > > > between them). The initialization is just a sequence of register writes, > > > which cannot be altered, without breaking the hardware operation. > > > > > > To match the generic PHY framework requirement, simply move all register > > > writes to the phy_init()/phy_exit() and drop power_on()/power_off() > > > callbacks. This way the driver will also work with the old (incorrect) > > > PHY initialization call sequence. > > > > Is the plan to merge thru pcie tree? > > I guess these patches should go together. I don't see any major > exynos series pending, but I do have two minor pci-exynos.c patches in > the queue. > > If you ack it (after resolution of your question below) I'd be happy > to take both if it doesn't cause trouble for you. Done now.
On Fri, Jul 15, 2022 at 05:05:30PM +0530, Vinod Koul wrote: > On 12-07-22, 15:12, Bjorn Helgaas wrote: > > On Tue, Jul 05, 2022 at 11:55:23AM +0530, Vinod Koul wrote: > > > On 29-06-22, 00:04, Marek Szyprowski wrote: > > > > The exynos-pcie driver called phy_power_on() and then phy_init() for some > > > > historical reasons. However the generic PHY framework assumes that the > > > > proper sequence is to call phy_init() first, then phy_power_on(). The > > > > operations done by both functions should be considered as one action and > > > > as such they are called by the exynos-pcie driver (without doing anything > > > > between them). The initialization is just a sequence of register writes, > > > > which cannot be altered, without breaking the hardware operation. > > > > > > > > To match the generic PHY framework requirement, simply move all register > > > > writes to the phy_init()/phy_exit() and drop power_on()/power_off() > > > > callbacks. This way the driver will also work with the old (incorrect) > > > > PHY initialization call sequence. > > > > > > Is the plan to merge thru pcie tree? > > > > I guess these patches should go together. I don't see any major > > exynos series pending, but I do have two minor pci-exynos.c patches in > > the queue. > > > > If you ack it (after resolution of your question below) I'd be happy > > to take both if it doesn't cause trouble for you. > > Done now. Is this an ack? I didn't see any response to your question (added back below). Are you happy with the patch as-is? > > > > @@ -51,6 +51,13 @@ static int exynos5433_pcie_phy_init(struct phy *phy) > > > > { > > > > struct exynos_pcie_phy *ep = phy_get_drvdata(phy); > > > > > > > > + regmap_update_bits(ep->pmureg, EXYNOS5433_PMU_PCIE_PHY_OFFSET, > > > > + BIT(0), 1); > > > > + regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_GLOBAL_RESET, > > > > + PCIE_APP_REQ_EXIT_L1_MODE, 0); > > > > + regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_L1SUB_CM_CON, > > > > + PCIE_REFCLK_GATING_EN, 0); > > > > + > > > > > > why not retain exynos5433_pcie_phy_power_on() and call it from here and > > > drop in ops. It would be clear to reader that these are for turning on > > > the phy...
On Fri, Jul 15, 2022 at 05:43:03PM -0500, Bjorn Helgaas wrote: > On Fri, Jul 15, 2022 at 05:05:30PM +0530, Vinod Koul wrote: > > On 12-07-22, 15:12, Bjorn Helgaas wrote: > > > On Tue, Jul 05, 2022 at 11:55:23AM +0530, Vinod Koul wrote: > > > > On 29-06-22, 00:04, Marek Szyprowski wrote: > > > > > The exynos-pcie driver called phy_power_on() and then phy_init() for some > > > > > historical reasons. However the generic PHY framework assumes that the > > > > > proper sequence is to call phy_init() first, then phy_power_on(). The > > > > > operations done by both functions should be considered as one action and > > > > > as such they are called by the exynos-pcie driver (without doing anything > > > > > between them). The initialization is just a sequence of register writes, > > > > > which cannot be altered, without breaking the hardware operation. > > > > > > > > > > To match the generic PHY framework requirement, simply move all register > > > > > writes to the phy_init()/phy_exit() and drop power_on()/power_off() > > > > > callbacks. This way the driver will also work with the old (incorrect) > > > > > PHY initialization call sequence. > > > > > > > > Is the plan to merge thru pcie tree? > > > > > > I guess these patches should go together. I don't see any major > > > exynos series pending, but I do have two minor pci-exynos.c patches in > > > the queue. > > > > > > If you ack it (after resolution of your question below) I'd be happy > > > to take both if it doesn't cause trouble for you. > > > > Done now. > > Is this an ack? > > I didn't see any response to your question (added back below). Are > you happy with the patch as-is? Oops, sorry, I missed your ack [1]. That was more recent than your question, so I assume you're ok with the patch as-is. I *would* like an ack from the maintainer, but I'm not sure whether Jingoo is still paying attention to pci-exynos.c. [1] https://lore.kernel.org/r/YtFQ67MmloipjNzj@matsya > > > > > @@ -51,6 +51,13 @@ static int exynos5433_pcie_phy_init(struct phy *phy) > > > > > { > > > > > struct exynos_pcie_phy *ep = phy_get_drvdata(phy); > > > > > > > > > > + regmap_update_bits(ep->pmureg, EXYNOS5433_PMU_PCIE_PHY_OFFSET, > > > > > + BIT(0), 1); > > > > > + regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_GLOBAL_RESET, > > > > > + PCIE_APP_REQ_EXIT_L1_MODE, 0); > > > > > + regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_L1SUB_CM_CON, > > > > > + PCIE_REFCLK_GATING_EN, 0); > > > > > + > > > > > > > > why not retain exynos5433_pcie_phy_power_on() and call it from here and > > > > drop in ops. It would be clear to reader that these are for turning on > > > > the phy...
On Wed, Jun 29, 2022 at 12:04:08AM +0200, Marek Szyprowski wrote: > The exynos-pcie driver called phy_power_on() and then phy_init() for some > historical reasons. However the generic PHY framework assumes that the > proper sequence is to call phy_init() first, then phy_power_on(). The > operations done by both functions should be considered as one action and > as such they are called by the exynos-pcie driver (without doing anything > between them). The initialization is just a sequence of register writes, > which cannot be altered, without breaking the hardware operation. > > To match the generic PHY framework requirement, simply move all register > writes to the phy_init()/phy_exit() and drop power_on()/power_off() > callbacks. This way the driver will also work with the old (incorrect) > PHY initialization call sequence. > > Reported-by: Bjorn Helgaas <helgaas@kernel.org> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> Both applied to pci/ctrl/exynos for v5.20, thanks! > --- > drivers/phy/samsung/phy-exynos-pcie.c | 25 +++++++++---------------- > 1 file changed, 9 insertions(+), 16 deletions(-) > > diff --git a/drivers/phy/samsung/phy-exynos-pcie.c b/drivers/phy/samsung/phy-exynos-pcie.c > index 578cfe07d07a..53c9230c2907 100644 > --- a/drivers/phy/samsung/phy-exynos-pcie.c > +++ b/drivers/phy/samsung/phy-exynos-pcie.c > @@ -51,6 +51,13 @@ static int exynos5433_pcie_phy_init(struct phy *phy) > { > struct exynos_pcie_phy *ep = phy_get_drvdata(phy); > > + regmap_update_bits(ep->pmureg, EXYNOS5433_PMU_PCIE_PHY_OFFSET, > + BIT(0), 1); > + regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_GLOBAL_RESET, > + PCIE_APP_REQ_EXIT_L1_MODE, 0); > + regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_L1SUB_CM_CON, > + PCIE_REFCLK_GATING_EN, 0); > + > regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_COMMON_RESET, > PCIE_PHY_RESET, 1); > regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_MAC_RESET, > @@ -109,20 +116,7 @@ static int exynos5433_pcie_phy_init(struct phy *phy) > return 0; > } > > -static int exynos5433_pcie_phy_power_on(struct phy *phy) > -{ > - struct exynos_pcie_phy *ep = phy_get_drvdata(phy); > - > - regmap_update_bits(ep->pmureg, EXYNOS5433_PMU_PCIE_PHY_OFFSET, > - BIT(0), 1); > - regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_GLOBAL_RESET, > - PCIE_APP_REQ_EXIT_L1_MODE, 0); > - regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_L1SUB_CM_CON, > - PCIE_REFCLK_GATING_EN, 0); > - return 0; > -} > - > -static int exynos5433_pcie_phy_power_off(struct phy *phy) > +static int exynos5433_pcie_phy_exit(struct phy *phy) > { > struct exynos_pcie_phy *ep = phy_get_drvdata(phy); > > @@ -135,8 +129,7 @@ static int exynos5433_pcie_phy_power_off(struct phy *phy) > > static const struct phy_ops exynos5433_phy_ops = { > .init = exynos5433_pcie_phy_init, > - .power_on = exynos5433_pcie_phy_power_on, > - .power_off = exynos5433_pcie_phy_power_off, > + .exit = exynos5433_pcie_phy_exit, > .owner = THIS_MODULE, > }; > > -- > 2.17.1 > > > -- > linux-phy mailing list > linux-phy@lists.infradead.org > https://lists.infradead.org/mailman/listinfo/linux-phy
diff --git a/drivers/phy/samsung/phy-exynos-pcie.c b/drivers/phy/samsung/phy-exynos-pcie.c index 578cfe07d07a..53c9230c2907 100644 --- a/drivers/phy/samsung/phy-exynos-pcie.c +++ b/drivers/phy/samsung/phy-exynos-pcie.c @@ -51,6 +51,13 @@ static int exynos5433_pcie_phy_init(struct phy *phy) { struct exynos_pcie_phy *ep = phy_get_drvdata(phy); + regmap_update_bits(ep->pmureg, EXYNOS5433_PMU_PCIE_PHY_OFFSET, + BIT(0), 1); + regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_GLOBAL_RESET, + PCIE_APP_REQ_EXIT_L1_MODE, 0); + regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_L1SUB_CM_CON, + PCIE_REFCLK_GATING_EN, 0); + regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_COMMON_RESET, PCIE_PHY_RESET, 1); regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_MAC_RESET, @@ -109,20 +116,7 @@ static int exynos5433_pcie_phy_init(struct phy *phy) return 0; } -static int exynos5433_pcie_phy_power_on(struct phy *phy) -{ - struct exynos_pcie_phy *ep = phy_get_drvdata(phy); - - regmap_update_bits(ep->pmureg, EXYNOS5433_PMU_PCIE_PHY_OFFSET, - BIT(0), 1); - regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_GLOBAL_RESET, - PCIE_APP_REQ_EXIT_L1_MODE, 0); - regmap_update_bits(ep->fsysreg, PCIE_EXYNOS5433_PHY_L1SUB_CM_CON, - PCIE_REFCLK_GATING_EN, 0); - return 0; -} - -static int exynos5433_pcie_phy_power_off(struct phy *phy) +static int exynos5433_pcie_phy_exit(struct phy *phy) { struct exynos_pcie_phy *ep = phy_get_drvdata(phy); @@ -135,8 +129,7 @@ static int exynos5433_pcie_phy_power_off(struct phy *phy) static const struct phy_ops exynos5433_phy_ops = { .init = exynos5433_pcie_phy_init, - .power_on = exynos5433_pcie_phy_power_on, - .power_off = exynos5433_pcie_phy_power_off, + .exit = exynos5433_pcie_phy_exit, .owner = THIS_MODULE, };
The exynos-pcie driver called phy_power_on() and then phy_init() for some historical reasons. However the generic PHY framework assumes that the proper sequence is to call phy_init() first, then phy_power_on(). The operations done by both functions should be considered as one action and as such they are called by the exynos-pcie driver (without doing anything between them). The initialization is just a sequence of register writes, which cannot be altered, without breaking the hardware operation. To match the generic PHY framework requirement, simply move all register writes to the phy_init()/phy_exit() and drop power_on()/power_off() callbacks. This way the driver will also work with the old (incorrect) PHY initialization call sequence. Reported-by: Bjorn Helgaas <helgaas@kernel.org> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/phy/samsung/phy-exynos-pcie.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-)