2019-09-09 16:55:28

by Neil Armstrong

Subject: [PATCH 4/6] phy: meson-g12a-usb3-pcie: Add support for PCIe mode

This adds extended PCIe PHY functions for the Amlogic G12A
USB3+PCIE Combo PHY to support reset, power_on and power_off for
PCIe exclusively.

With these callbacks, we can handle all the needed operations of the
Amlogic PCIe controller driver.

Signed-off-by: Neil Armstrong <[email protected]>
---
.../phy/amlogic/phy-meson-g12a-usb3-pcie.c | 70 ++++++++++++++++---
1 file changed, 61 insertions(+), 9 deletions(-)

diff --git a/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c b/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
index ac322d643c7a..08e322789e59 100644
--- a/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
+++ b/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
@@ -50,6 +50,8 @@
#define PHY_R5_PHY_CR_ACK BIT(16)
#define PHY_R5_PHY_BS_OUT BIT(17)

+#define PCIE_RESET_DELAY 500
+
struct phy_g12a_usb3_pcie_priv {
struct regmap *regmap;
struct regmap *regmap_cr;
@@ -196,6 +198,10 @@ static int phy_g12a_usb3_init(struct phy *phy)
struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
int data, ret;

+ ret = reset_control_reset(priv->reset);
+ if (ret)
+ return ret;
+
/* Switch PHY to USB3 */
/* TODO figure out how to handle when PCIe was set in the bootloader */
regmap_update_bits(priv->regmap, PHY_R0,
@@ -272,24 +278,64 @@ static int phy_g12a_usb3_init(struct phy *phy)
return 0;
}

-static int phy_g12a_usb3_pcie_init(struct phy *phy)
+static int phy_g12a_usb3_pcie_power_on(struct phy *phy)
+{
+ struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
+
+ if (priv->mode == PHY_TYPE_USB3)
+ return 0;
+
+ regmap_update_bits(priv->regmap, PHY_R0,
+ PHY_R0_PCIE_POWER_STATE,
+ FIELD_PREP(PHY_R0_PCIE_POWER_STATE, 0x1c));
+
+ return 0;
+}
+
+static int phy_g12a_usb3_pcie_power_off(struct phy *phy)
+{
+ struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
+
+ if (priv->mode == PHY_TYPE_USB3)
+ return 0;
+
+ regmap_update_bits(priv->regmap, PHY_R0,
+ PHY_R0_PCIE_POWER_STATE,
+ FIELD_PREP(PHY_R0_PCIE_POWER_STATE, 0x1d));
+
+ return 0;
+}
+
+static int phy_g12a_usb3_pcie_reset(struct phy *phy)
{
struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
int ret;

- ret = reset_control_reset(priv->reset);
+ if (priv->mode == PHY_TYPE_USB3)
+ return 0;
+
+ ret = reset_control_assert(priv->reset);
if (ret)
return ret;

+ udelay(PCIE_RESET_DELAY);
+
+ ret = reset_control_deassert(priv->reset);
+ if (ret)
+ return ret;
+
+ udelay(PCIE_RESET_DELAY);
+
+ return 0;
+}
+
+static int phy_g12a_usb3_pcie_init(struct phy *phy)
+{
+ struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
+
if (priv->mode == PHY_TYPE_USB3)
return phy_g12a_usb3_init(phy);

- /* Power UP PCIE */
- /* TODO figure out when the bootloader has set USB3 mode before */
- regmap_update_bits(priv->regmap, PHY_R0,
- PHY_R0_PCIE_POWER_STATE,
- FIELD_PREP(PHY_R0_PCIE_POWER_STATE, 0x1c));
-
return 0;
}

@@ -297,7 +343,10 @@ static int phy_g12a_usb3_pcie_exit(struct phy *phy)
{
struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);

- return reset_control_reset(priv->reset);
+ if (priv->mode == PHY_TYPE_USB3)
+ return reset_control_reset(priv->reset);
+
+ return 0;
}

static struct phy *phy_g12a_usb3_pcie_xlate(struct device *dev,
@@ -326,6 +375,9 @@ static struct phy *phy_g12a_usb3_pcie_xlate(struct device *dev,
static const struct phy_ops phy_g12a_usb3_pcie_ops = {
.init = phy_g12a_usb3_pcie_init,
.exit = phy_g12a_usb3_pcie_exit,
+ .power_on = phy_g12a_usb3_pcie_power_on,
+ .power_off = phy_g12a_usb3_pcie_power_off,
+ .reset = phy_g12a_usb3_pcie_reset,
.owner = THIS_MODULE,
};

--
2.17.1

2019-09-11 12:23:05

by Andrew Murray

Subject: Re: [PATCH 4/6] phy: meson-g12a-usb3-pcie: Add support for PCIe mode

On Sun, Sep 08, 2019 at 01:42:56PM +0000, Neil Armstrong wrote:
> This adds extended PCIe PHY functions for the Amlogic G12A
> USB3+PCIE Combo PHY to support reset, power_on and power_off for
> PCIe exclusively.
>
> With these callbacks, we can handle all the needed operations of the
> Amlogic PCIe controller driver.
>
> Signed-off-by: Neil Armstrong <[email protected]>
> ---
> .../phy/amlogic/phy-meson-g12a-usb3-pcie.c | 70 ++++++++++++++++---
> 1 file changed, 61 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c b/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
> index ac322d643c7a..08e322789e59 100644
> --- a/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
> +++ b/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
> @@ -50,6 +50,8 @@
> #define PHY_R5_PHY_CR_ACK BIT(16)
> #define PHY_R5_PHY_BS_OUT BIT(17)
>
> +#define PCIE_RESET_DELAY 500
> +
> struct phy_g12a_usb3_pcie_priv {
> struct regmap *regmap;
> struct regmap *regmap_cr;
> @@ -196,6 +198,10 @@ static int phy_g12a_usb3_init(struct phy *phy)
> struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
> int data, ret;
>
> + ret = reset_control_reset(priv->reset);
> + if (ret)
> + return ret;
> +

Right, so we've moved this to apply to USB only, thus assuming PCI will
call .reset for its reset (why the asymmetry?).

Thanks,

Andrew Murray

> /* Switch PHY to USB3 */
> /* TODO figure out how to handle when PCIe was set in the bootloader */
> regmap_update_bits(priv->regmap, PHY_R0,
> @@ -272,24 +278,64 @@ static int phy_g12a_usb3_init(struct phy *phy)
> return 0;
> }
>
> -static int phy_g12a_usb3_pcie_init(struct phy *phy)
> +static int phy_g12a_usb3_pcie_power_on(struct phy *phy)
> +{
> + struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
> +
> + if (priv->mode == PHY_TYPE_USB3)
> + return 0;
> +
> + regmap_update_bits(priv->regmap, PHY_R0,
> + PHY_R0_PCIE_POWER_STATE,
> + FIELD_PREP(PHY_R0_PCIE_POWER_STATE, 0x1c));
> +
> + return 0;
> +}
> +
> +static int phy_g12a_usb3_pcie_power_off(struct phy *phy)
> +{
> + struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
> +
> + if (priv->mode == PHY_TYPE_USB3)
> + return 0;
> +
> + regmap_update_bits(priv->regmap, PHY_R0,
> + PHY_R0_PCIE_POWER_STATE,
> + FIELD_PREP(PHY_R0_PCIE_POWER_STATE, 0x1d));
> +
> + return 0;
> +}
> +
> +static int phy_g12a_usb3_pcie_reset(struct phy *phy)
> {
> struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
> int ret;
>
> - ret = reset_control_reset(priv->reset);
> + if (priv->mode == PHY_TYPE_USB3)
> + return 0;
> +
> + ret = reset_control_assert(priv->reset);
> if (ret)
> return ret;
>
> + udelay(PCIE_RESET_DELAY);
> +
> + ret = reset_control_deassert(priv->reset);
> + if (ret)
> + return ret;
> +
> + udelay(PCIE_RESET_DELAY);
> +
> + return 0;
> +}
> +
> +static int phy_g12a_usb3_pcie_init(struct phy *phy)
> +{
> + struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
> +
> if (priv->mode == PHY_TYPE_USB3)
> return phy_g12a_usb3_init(phy);
>
> - /* Power UP PCIE */
> - /* TODO figure out when the bootloader has set USB3 mode before */
> - regmap_update_bits(priv->regmap, PHY_R0,
> - PHY_R0_PCIE_POWER_STATE,
> - FIELD_PREP(PHY_R0_PCIE_POWER_STATE, 0x1c));
> -
> return 0;
> }
>
> @@ -297,7 +343,10 @@ static int phy_g12a_usb3_pcie_exit(struct phy *phy)
> {
> struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
>
> - return reset_control_reset(priv->reset);
> + if (priv->mode == PHY_TYPE_USB3)
> + return reset_control_reset(priv->reset);
> +
> + return 0;
> }
>
> static struct phy *phy_g12a_usb3_pcie_xlate(struct device *dev,
> @@ -326,6 +375,9 @@ static struct phy *phy_g12a_usb3_pcie_xlate(struct device *dev,
> static const struct phy_ops phy_g12a_usb3_pcie_ops = {
> .init = phy_g12a_usb3_pcie_init,
> .exit = phy_g12a_usb3_pcie_exit,
> + .power_on = phy_g12a_usb3_pcie_power_on,
> + .power_off = phy_g12a_usb3_pcie_power_off,
> + .reset = phy_g12a_usb3_pcie_reset,
> .owner = THIS_MODULE,
> };
>
> --
> 2.17.1
>

2019-09-11 12:48:12

by Neil Armstrong

Subject: Re: [PATCH 4/6] phy: meson-g12a-usb3-pcie: Add support for PCIe mode

On 11/09/2019 14:19, Andrew Murray wrote:
> On Sun, Sep 08, 2019 at 01:42:56PM +0000, Neil Armstrong wrote:
>> This adds extended PCIe PHY functions for the Amlogic G12A
>> USB3+PCIE Combo PHY to support reset, power_on and power_off for
>> PCIe exclusively.
>>
>> With these callbacks, we can handle all the needed operations of the
>> Amlogic PCIe controller driver.
>>
>> Signed-off-by: Neil Armstrong <[email protected]>
>> ---
>> .../phy/amlogic/phy-meson-g12a-usb3-pcie.c | 70 ++++++++++++++++---
>> 1 file changed, 61 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c b/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
>> index ac322d643c7a..08e322789e59 100644
>> --- a/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
>> +++ b/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
>> @@ -50,6 +50,8 @@
>> #define PHY_R5_PHY_CR_ACK BIT(16)
>> #define PHY_R5_PHY_BS_OUT BIT(17)
>>
>> +#define PCIE_RESET_DELAY 500
>> +
>> struct phy_g12a_usb3_pcie_priv {
>> struct regmap *regmap;
>> struct regmap *regmap_cr;
>> @@ -196,6 +198,10 @@ static int phy_g12a_usb3_init(struct phy *phy)
>> struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
>> int data, ret;
>>
>> + ret = reset_control_reset(priv->reset);
>> + if (ret)
>> + return ret;
>> +
>
> Right, so we've moved this to apply to USB only, thus assuming PCI will
> call .reset for its reset (why the asymmetry?).

Exact, there is no power_on/power_off when USB3 mode is used, and vendor
always reset the PHY before switching to USB3, but for PCIe, it seems the
reset and the power_on must be done separately with the PCIe controller init
and reset in the middle.

I would prefer symmetry aswell :-/

Neil

>
> Thanks,
>
> Andrew Murray
>
>> /* Switch PHY to USB3 */
>> /* TODO figure out how to handle when PCIe was set in the bootloader */
>> regmap_update_bits(priv->regmap, PHY_R0,
>> @@ -272,24 +278,64 @@ static int phy_g12a_usb3_init(struct phy *phy)
>> return 0;
>> }
>>
>> -static int phy_g12a_usb3_pcie_init(struct phy *phy)
>> +static int phy_g12a_usb3_pcie_power_on(struct phy *phy)
>> +{
>> + struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
>> +
>> + if (priv->mode == PHY_TYPE_USB3)
>> + return 0;
>> +
>> + regmap_update_bits(priv->regmap, PHY_R0,
>> + PHY_R0_PCIE_POWER_STATE,
>> + FIELD_PREP(PHY_R0_PCIE_POWER_STATE, 0x1c));
>> +
>> + return 0;
>> +}
>> +
>> +static int phy_g12a_usb3_pcie_power_off(struct phy *phy)
>> +{
>> + struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
>> +
>> + if (priv->mode == PHY_TYPE_USB3)
>> + return 0;
>> +
>> + regmap_update_bits(priv->regmap, PHY_R0,
>> + PHY_R0_PCIE_POWER_STATE,
>> + FIELD_PREP(PHY_R0_PCIE_POWER_STATE, 0x1d));
>> +
>> + return 0;
>> +}
>> +
>> +static int phy_g12a_usb3_pcie_reset(struct phy *phy)
>> {
>> struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
>> int ret;
>>
>> - ret = reset_control_reset(priv->reset);
>> + if (priv->mode == PHY_TYPE_USB3)
>> + return 0;
>> +
>> + ret = reset_control_assert(priv->reset);
>> if (ret)
>> return ret;
>>
>> + udelay(PCIE_RESET_DELAY);
>> +
>> + ret = reset_control_deassert(priv->reset);
>> + if (ret)
>> + return ret;
>> +
>> + udelay(PCIE_RESET_DELAY);
>> +
>> + return 0;
>> +}
>> +
>> +static int phy_g12a_usb3_pcie_init(struct phy *phy)
>> +{
>> + struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
>> +
>> if (priv->mode == PHY_TYPE_USB3)
>> return phy_g12a_usb3_init(phy);
>>
>> - /* Power UP PCIE */
>> - /* TODO figure out when the bootloader has set USB3 mode before */
>> - regmap_update_bits(priv->regmap, PHY_R0,
>> - PHY_R0_PCIE_POWER_STATE,
>> - FIELD_PREP(PHY_R0_PCIE_POWER_STATE, 0x1c));
>> -
>> return 0;
>> }
>>
>> @@ -297,7 +343,10 @@ static int phy_g12a_usb3_pcie_exit(struct phy *phy)
>> {
>> struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
>>
>> - return reset_control_reset(priv->reset);
>> + if (priv->mode == PHY_TYPE_USB3)
>> + return reset_control_reset(priv->reset);
>> +
>> + return 0;
>> }
>>
>> static struct phy *phy_g12a_usb3_pcie_xlate(struct device *dev,
>> @@ -326,6 +375,9 @@ static struct phy *phy_g12a_usb3_pcie_xlate(struct device *dev,
>> static const struct phy_ops phy_g12a_usb3_pcie_ops = {
>> .init = phy_g12a_usb3_pcie_init,
>> .exit = phy_g12a_usb3_pcie_exit,
>> + .power_on = phy_g12a_usb3_pcie_power_on,
>> + .power_off = phy_g12a_usb3_pcie_power_off,
>> + .reset = phy_g12a_usb3_pcie_reset,
>> .owner = THIS_MODULE,
>> };
>>
>> --
>> 2.17.1
>>

2019-09-11 13:02:29

by Andrew Murray

Subject: Re: [PATCH 4/6] phy: meson-g12a-usb3-pcie: Add support for PCIe mode

On Wed, Sep 11, 2019 at 02:45:23PM +0200, Neil Armstrong wrote:
> On 11/09/2019 14:19, Andrew Murray wrote:
> > On Sun, Sep 08, 2019 at 01:42:56PM +0000, Neil Armstrong wrote:
> >> This adds extended PCIe PHY functions for the Amlogic G12A
> >> USB3+PCIE Combo PHY to support reset, power_on and power_off for
> >> PCIe exclusively.
> >>
> >> With these callbacks, we can handle all the needed operations of the
> >> Amlogic PCIe controller driver.
> >>
> >> Signed-off-by: Neil Armstrong <[email protected]>
> >> ---
> >> .../phy/amlogic/phy-meson-g12a-usb3-pcie.c | 70 ++++++++++++++++---
> >> 1 file changed, 61 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c b/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
> >> index ac322d643c7a..08e322789e59 100644
> >> --- a/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
> >> +++ b/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
> >> @@ -50,6 +50,8 @@
> >> #define PHY_R5_PHY_CR_ACK BIT(16)
> >> #define PHY_R5_PHY_BS_OUT BIT(17)
> >>
> >> +#define PCIE_RESET_DELAY 500
> >> +
> >> struct phy_g12a_usb3_pcie_priv {
> >> struct regmap *regmap;
> >> struct regmap *regmap_cr;
> >> @@ -196,6 +198,10 @@ static int phy_g12a_usb3_init(struct phy *phy)
> >> struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
> >> int data, ret;
> >>
> >> + ret = reset_control_reset(priv->reset);
> >> + if (ret)
> >> + return ret;
> >> +
> >
> > Right, so we've moved this to apply to USB only, thus assuming PCI will
> > call .reset for its reset (why the asymmetry?).
>
> Exact, there is no power_on/power_off when USB3 mode is used, and vendor
> always reset the PHY before switching to USB3, but for PCIe, it seems the
> reset and the power_on must be done separately with the PCIe controller init
> and reset in the middle.
>
> I would prefer symmetry aswell :-/

OK.

Thanks,

Andrew Murray

>
> Neil
>
> >
> > Thanks,
> >
> > Andrew Murray
> >
> >> /* Switch PHY to USB3 */
> >> /* TODO figure out how to handle when PCIe was set in the bootloader */
> >> regmap_update_bits(priv->regmap, PHY_R0,
> >> @@ -272,24 +278,64 @@ static int phy_g12a_usb3_init(struct phy *phy)
> >> return 0;
> >> }
> >>
> >> -static int phy_g12a_usb3_pcie_init(struct phy *phy)
> >> +static int phy_g12a_usb3_pcie_power_on(struct phy *phy)
> >> +{
> >> + struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
> >> +
> >> + if (priv->mode == PHY_TYPE_USB3)
> >> + return 0;
> >> +
> >> + regmap_update_bits(priv->regmap, PHY_R0,
> >> + PHY_R0_PCIE_POWER_STATE,
> >> + FIELD_PREP(PHY_R0_PCIE_POWER_STATE, 0x1c));
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int phy_g12a_usb3_pcie_power_off(struct phy *phy)
> >> +{
> >> + struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
> >> +
> >> + if (priv->mode == PHY_TYPE_USB3)
> >> + return 0;
> >> +
> >> + regmap_update_bits(priv->regmap, PHY_R0,
> >> + PHY_R0_PCIE_POWER_STATE,
> >> + FIELD_PREP(PHY_R0_PCIE_POWER_STATE, 0x1d));
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int phy_g12a_usb3_pcie_reset(struct phy *phy)
> >> {
> >> struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
> >> int ret;
> >>
> >> - ret = reset_control_reset(priv->reset);
> >> + if (priv->mode == PHY_TYPE_USB3)
> >> + return 0;
> >> +
> >> + ret = reset_control_assert(priv->reset);
> >> if (ret)
> >> return ret;
> >>
> >> + udelay(PCIE_RESET_DELAY);
> >> +
> >> + ret = reset_control_deassert(priv->reset);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + udelay(PCIE_RESET_DELAY);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int phy_g12a_usb3_pcie_init(struct phy *phy)
> >> +{
> >> + struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
> >> +
> >> if (priv->mode == PHY_TYPE_USB3)
> >> return phy_g12a_usb3_init(phy);
> >>
> >> - /* Power UP PCIE */
> >> - /* TODO figure out when the bootloader has set USB3 mode before */
> >> - regmap_update_bits(priv->regmap, PHY_R0,
> >> - PHY_R0_PCIE_POWER_STATE,
> >> - FIELD_PREP(PHY_R0_PCIE_POWER_STATE, 0x1c));
> >> -
> >> return 0;
> >> }
> >>
> >> @@ -297,7 +343,10 @@ static int phy_g12a_usb3_pcie_exit(struct phy *phy)
> >> {
> >> struct phy_g12a_usb3_pcie_priv *priv = phy_get_drvdata(phy);
> >>
> >> - return reset_control_reset(priv->reset);
> >> + if (priv->mode == PHY_TYPE_USB3)
> >> + return reset_control_reset(priv->reset);
> >> +
> >> + return 0;
> >> }
> >>
> >> static struct phy *phy_g12a_usb3_pcie_xlate(struct device *dev,
> >> @@ -326,6 +375,9 @@ static struct phy *phy_g12a_usb3_pcie_xlate(struct device *dev,
> >> static const struct phy_ops phy_g12a_usb3_pcie_ops = {
> >> .init = phy_g12a_usb3_pcie_init,
> >> .exit = phy_g12a_usb3_pcie_exit,
> >> + .power_on = phy_g12a_usb3_pcie_power_on,
> >> + .power_off = phy_g12a_usb3_pcie_power_off,
> >> + .reset = phy_g12a_usb3_pcie_reset,
> >> .owner = THIS_MODULE,
> >> };
> >>
> >> --
> >> 2.17.1
> >>
>