Patchwork [5/8] phy: amlogic: add Amlogic G12A USB2 PHY Driver

login
register
mail settings
Submitter Neil Armstrong
Date Feb. 12, 2019, 3:14 p.m.
Message ID <20190212151413.24632-6-narmstrong@baylibre.com>
Download mbox | patch
Permalink /patch/724009/
State New
Headers show

Comments

Neil Armstrong - Feb. 12, 2019, 3:14 p.m.
This adds support for the USB2 PHY found in the Amlogic G12A SoC Family.

It supports Host and/or Peripheral mode, depending on it's position.
The first PHY is only used as Host, but the second supports Dual modes
defined by the USB Control Glue HW in front of the USB Controllers.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/phy/amlogic/Kconfig               |  12 ++
 drivers/phy/amlogic/Makefile              |   1 +
 drivers/phy/amlogic/phy-meson-g12a-usb2.c | 191 ++++++++++++++++++++++
 3 files changed, 204 insertions(+)
 create mode 100644 drivers/phy/amlogic/phy-meson-g12a-usb2.c
Martin Blumenstingl - Feb. 17, 2019, 10:24 p.m.
Hi Neil,

On Tue, Feb 12, 2019 at 4:15 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> This adds support for the USB2 PHY found in the Amlogic G12A SoC Family.
>
> It supports Host and/or Peripheral mode, depending on it's position.
> The first PHY is only used as Host, but the second supports Dual modes
> defined by the USB Control Glue HW in front of the USB Controllers.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/phy/amlogic/Kconfig               |  12 ++
>  drivers/phy/amlogic/Makefile              |   1 +
>  drivers/phy/amlogic/phy-meson-g12a-usb2.c | 191 ++++++++++++++++++++++
>  3 files changed, 204 insertions(+)
>  create mode 100644 drivers/phy/amlogic/phy-meson-g12a-usb2.c
>
> diff --git a/drivers/phy/amlogic/Kconfig b/drivers/phy/amlogic/Kconfig
> index 23fe1cda2f70..78d6e194dce9 100644
> --- a/drivers/phy/amlogic/Kconfig
> +++ b/drivers/phy/amlogic/Kconfig
> @@ -36,3 +36,15 @@ config PHY_MESON_GXL_USB3
>           Enable this to support the Meson USB3 PHY and OTG detection
>           IP block found in Meson GXL and GXM SoCs.
>           If unsure, say N.
> +
> +config PHY_MESON_G12A_USB2
> +       tristate "Meson G12A USB2 PHY drivers"
aw, there's a typo in the GXL driver names (from which this new
Kconfig entry may have been copied): it should be "driver" instead of
"drivers"

> +       default ARCH_MESON
> +       depends on OF && (ARCH_MESON || COMPILE_TEST)
> +       depends on USB_SUPPORT
I don't think you need USB_SUPPORT (neither does PHY_MESON_GXL_USB2,
but it seems that I accidentally left it in from an early development
iteration), see below

> +       select GENERIC_PHY
> +       select REGMAP_MMIO
> +       help
> +         Enable this to support the Meson USB2 PHYs found in Meson
> +         G12A SoCs.
> +         If unsure, say N.
> diff --git a/drivers/phy/amlogic/Makefile b/drivers/phy/amlogic/Makefile
> index 4fd8848c194d..7d4d10f5a6b3 100644
> --- a/drivers/phy/amlogic/Makefile
> +++ b/drivers/phy/amlogic/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_PHY_MESON8B_USB2)         += phy-meson8b-usb2.o
>  obj-$(CONFIG_PHY_MESON_GXL_USB2)       += phy-meson-gxl-usb2.o
> +obj-$(CONFIG_PHY_MESON_G12A_USB2)      += phy-meson-g12a-usb2.o
>  obj-$(CONFIG_PHY_MESON_GXL_USB3)       += phy-meson-gxl-usb3.o
> diff --git a/drivers/phy/amlogic/phy-meson-g12a-usb2.c b/drivers/phy/amlogic/phy-meson-g12a-usb2.c
> new file mode 100644
> index 000000000000..3b6271a8be02
> --- /dev/null
> +++ b/drivers/phy/amlogic/phy-meson-g12a-usb2.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Meson G12A USB2 PHY driver
> + *
> + * Copyright (C) 2017 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> + * Copyright (C) 2017 Amlogic, Inc. All rights reserved
> + * Copyright (C) 2019 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
I don't see any syscon usage in the driver so you can drop this

> +#include <linux/reset.h>
> +#include <linux/phy/phy.h>
> +#include <linux/usb/otg.h>
do you need this? if you drop "depends on USB_SUPPORT" from Kconfig
then you want to get rid of this as well

> +#include <linux/platform_device.h>
> +
> +#define PHY_CTRL_R0                                            0x0
> +#define PHY_CTRL_R1                                            0x4
> +#define PHY_CTRL_R2                                            0x8
> +#define PHY_CTRL_R3                                            0xc
> +#define PHY_CTRL_R4                                            0x10
> +#define PHY_CTRL_R5                                            0x14
> +#define PHY_CTRL_R6                                            0x18
> +#define PHY_CTRL_R7                                            0x1c
> +#define PHY_CTRL_R8                                            0x20
> +#define PHY_CTRL_R9                                            0x24
> +#define PHY_CTRL_R10                                           0x28
> +#define PHY_CTRL_R11                                           0x2c
> +#define PHY_CTRL_R12                                           0x30
> +#define PHY_CTRL_R13                                           0x34
> +#define PHY_CTRL_R14                                           0x38
> +#define PHY_CTRL_R15                                           0x3c
> +#define PHY_CTRL_R16                                           0x40
> +#define PHY_CTRL_R17                                           0x44
> +#define PHY_CTRL_R18                                           0x48
> +#define PHY_CTRL_R19                                           0x4c
> +#define PHY_CTRL_R20                                           0x50
> +#define PHY_CTRL_R21                                           0x54
> +#define PHY_CTRL_R22                                           0x58
> +#define PHY_CTRL_R23                                           0x5c
> +
> +#define RESET_COMPLETE_TIME                                    1000
> +#define PLL_RESET_COMPLETE_TIME                                        100
> +
> +struct phy_meson_g12a_usb2_priv {
> +       struct device           *dev;
> +       struct regmap           *regmap;
> +       struct clk              *clk;
> +       struct reset_control    *reset;
> +};
> +
> +static const struct regmap_config phy_meson_g12a_usb2_regmap_conf = {
> +       .reg_bits = 8,
> +       .val_bits = 32,
> +       .reg_stride = 4,
> +       .max_register = PHY_CTRL_R23,
> +};
> +
> +static int phy_meson_g12a_usb2_init(struct phy *phy)
> +{
> +       struct phy_meson_g12a_usb2_priv *priv = phy_get_drvdata(phy);
> +       int ret;
> +
> +       ret = reset_control_reset(priv->reset);
> +       if (ret)
> +               return ret;
> +
> +       udelay(RESET_COMPLETE_TIME);
> +
> +       /* usb2_otg_aca_en == 0 */
> +       regmap_update_bits(priv->regmap, PHY_CTRL_R21, BIT(2), 0);
do you have the name of the other register bits as well? having
#defines would make the code easier to read

> +
> +       /* PLL Setup : 24MHz * 20 / 1 = 480MHz */
> +       regmap_write(priv->regmap, PHY_CTRL_R16, 0x39400414);
> +       regmap_write(priv->regmap, PHY_CTRL_R17, 0x927e0000);
> +       regmap_write(priv->regmap, PHY_CTRL_R18, 0xac5f49e5);
> +
> +       udelay(PLL_RESET_COMPLETE_TIME);
> +
> +       /* UnReset PLL */
> +       regmap_write(priv->regmap, PHY_CTRL_R16, 0x19400414);
> +
> +       /* PHY Tuning */
> +       regmap_write(priv->regmap, PHY_CTRL_R20, 0xfe18);
> +       regmap_write(priv->regmap, PHY_CTRL_R4, 0x8000fff);
> +
> +       /* Tuning Disconnect Threshold */
> +       regmap_write(priv->regmap, PHY_CTRL_R3, 0x34);
> +
> +       /* Analog Settings */
> +       regmap_write(priv->regmap, PHY_CTRL_R14, 0);
> +       regmap_write(priv->regmap, PHY_CTRL_R13, 0x78000);
> +
> +       return 0;
> +}
> +
> +static int phy_meson_g12a_usb2_exit(struct phy *phy)
> +{
> +       struct phy_meson_g12a_usb2_priv *priv = phy_get_drvdata(phy);
> +
> +       return reset_control_reset(priv->reset);
do you have information on whether we should reset_control_assert here
instead of reset_control_reset?

> +}
> +
> +/* set_mode is not needed, mode setting is handled via the UTMI bus */
> +static const struct phy_ops phy_meson_g12a_usb2_ops = {
> +       .init           = phy_meson_g12a_usb2_init,
> +       .exit           = phy_meson_g12a_usb2_exit,
> +       .owner          = THIS_MODULE,
> +};
> +
> +static int phy_meson_g12a_usb2_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct phy_provider *phy_provider;
> +       struct resource *res;
> +       struct phy_meson_g12a_usb2_priv *priv;
> +       struct phy *phy;
> +       void __iomem *base;
> +       int ret;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->dev = dev;
> +       platform_set_drvdata(pdev, priv);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       priv->regmap = devm_regmap_init_mmio(dev, base,
> +                                            &phy_meson_g12a_usb2_regmap_conf);
> +       if (IS_ERR(priv->regmap))
> +               return PTR_ERR(priv->regmap);
> +
> +       priv->clk = devm_clk_get(dev, "xtal");
> +       if (IS_ERR(priv->clk))
> +               return PTR_ERR(priv->clk);
> +
> +       priv->reset = devm_reset_control_get(dev, "phy");
> +       if (IS_ERR(priv->reset))
> +               return PTR_ERR(priv->reset);
> +
> +       ret = reset_control_deassert(priv->reset);
should we move this to phy_meson_g12a_usb2_init (which currently uses
reset_control_reset)?
then we could also use reset_control_assert in phy_meson_g12a_usb2_exit

> +       if (ret)
> +               return ret;
> +
> +       phy = devm_phy_create(dev, NULL, &phy_meson_g12a_usb2_ops);
> +       if (IS_ERR(phy)) {
> +               ret = PTR_ERR(phy);
> +               if (ret != -EPROBE_DEFER)
> +                       dev_err(dev, "failed to create PHY\n");
phy-core only returns EPROBE_DEFER in case the phy-supply is not ready yet.
we won't need a phy-supply (at least as far as I know, if we do then
please document this in the dt-bindings patch). for USB VBUS dwc2
support the "vbus-supply" property which we should use instead.
I leave it up to you to decide whether the check makes sense

> +               return ret;
> +       }
> +
> +       phy_set_bus_width(phy, 8);
> +       phy_set_drvdata(phy, priv);
> +
> +       phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +
> +       return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static const struct of_device_id phy_meson_g12a_usb2_of_match[] = {
> +       { .compatible = "amlogic,g12a-usb2-phy", },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, phy_meson_g12a_usb2_of_match);
> +
> +static struct platform_driver phy_meson_g12a_usb2_driver = {
> +       .probe  = phy_meson_g12a_usb2_probe,
> +       .driver = {
> +               .name           = "phy-meson-g12a-usb2",
> +               .of_match_table = phy_meson_g12a_usb2_of_match,
> +       },
> +};
> +module_platform_driver(phy_meson_g12a_usb2_driver);
> +
> +MODULE_AUTHOR("Martin Blumenstingl <martin.blumenstingl@googlemail.com>");
I haven't contributed any of the relevant code to this driver (I
assume you took one of our existing Amlogic PHY drivers as skeleton?).
it's up to you whether you want to keep me in here


Regards
Martin
Neil Armstrong - Feb. 18, 2019, 10:38 a.m.
Hi Martin,

On 17/02/2019 23:24, Martin Blumenstingl wrote:
> Hi Neil,
> 
> On Tue, Feb 12, 2019 at 4:15 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>>
>> This adds support for the USB2 PHY found in the Amlogic G12A SoC Family.
>>
>> It supports Host and/or Peripheral mode, depending on it's position.
>> The first PHY is only used as Host, but the second supports Dual modes
>> defined by the USB Control Glue HW in front of the USB Controllers.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/phy/amlogic/Kconfig               |  12 ++
>>  drivers/phy/amlogic/Makefile              |   1 +
>>  drivers/phy/amlogic/phy-meson-g12a-usb2.c | 191 ++++++++++++++++++++++
>>  3 files changed, 204 insertions(+)
>>  create mode 100644 drivers/phy/amlogic/phy-meson-g12a-usb2.c
>>
>> diff --git a/drivers/phy/amlogic/Kconfig b/drivers/phy/amlogic/Kconfig
>> index 23fe1cda2f70..78d6e194dce9 100644
>> --- a/drivers/phy/amlogic/Kconfig
>> +++ b/drivers/phy/amlogic/Kconfig
>> @@ -36,3 +36,15 @@ config PHY_MESON_GXL_USB3
>>           Enable this to support the Meson USB3 PHY and OTG detection
>>           IP block found in Meson GXL and GXM SoCs.
>>           If unsure, say N.
>> +
>> +config PHY_MESON_G12A_USB2
>> +       tristate "Meson G12A USB2 PHY drivers"
> aw, there's a typo in the GXL driver names (from which this new
> Kconfig entry may have been copied): it should be "driver" instead of
> "drivers"

Will fix...

> 
>> +       default ARCH_MESON
>> +       depends on OF && (ARCH_MESON || COMPILE_TEST)
>> +       depends on USB_SUPPORT
> I don't think you need USB_SUPPORT (neither does PHY_MESON_GXL_USB2,
> but it seems that I accidentally left it in from an early development
> iteration), see below

Indeed, thanks for pointing this

> 
>> +       select GENERIC_PHY
>> +       select REGMAP_MMIO
>> +       help
>> +         Enable this to support the Meson USB2 PHYs found in Meson
>> +         G12A SoCs.
>> +         If unsure, say N.
>> diff --git a/drivers/phy/amlogic/Makefile b/drivers/phy/amlogic/Makefile
>> index 4fd8848c194d..7d4d10f5a6b3 100644
>> --- a/drivers/phy/amlogic/Makefile
>> +++ b/drivers/phy/amlogic/Makefile
>> @@ -1,3 +1,4 @@
>>  obj-$(CONFIG_PHY_MESON8B_USB2)         += phy-meson8b-usb2.o
>>  obj-$(CONFIG_PHY_MESON_GXL_USB2)       += phy-meson-gxl-usb2.o
>> +obj-$(CONFIG_PHY_MESON_G12A_USB2)      += phy-meson-g12a-usb2.o
>>  obj-$(CONFIG_PHY_MESON_GXL_USB3)       += phy-meson-gxl-usb3.o
>> diff --git a/drivers/phy/amlogic/phy-meson-g12a-usb2.c b/drivers/phy/amlogic/phy-meson-g12a-usb2.c
>> new file mode 100644
>> index 000000000000..3b6271a8be02
>> --- /dev/null
>> +++ b/drivers/phy/amlogic/phy-meson-g12a-usb2.c
>> @@ -0,0 +1,191 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Meson G12A USB2 PHY driver
>> + *
>> + * Copyright (C) 2017 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> + * Copyright (C) 2017 Amlogic, Inc. All rights reserved
>> + * Copyright (C) 2019 BayLibre, SAS
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
> I don't see any syscon usage in the driver so you can drop this

Forgot to remove it

> 
>> +#include <linux/reset.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/usb/otg.h>
> do you need this? if you drop "depends on USB_SUPPORT" from Kconfig
> then you want to get rid of this as well

Indeed, no need to this

> 
>> +#include <linux/platform_device.h>
>> +
>> +#define PHY_CTRL_R0                                            0x0
>> +#define PHY_CTRL_R1                                            0x4
>> +#define PHY_CTRL_R2                                            0x8
>> +#define PHY_CTRL_R3                                            0xc
>> +#define PHY_CTRL_R4                                            0x10
>> +#define PHY_CTRL_R5                                            0x14
>> +#define PHY_CTRL_R6                                            0x18
>> +#define PHY_CTRL_R7                                            0x1c
>> +#define PHY_CTRL_R8                                            0x20
>> +#define PHY_CTRL_R9                                            0x24
>> +#define PHY_CTRL_R10                                           0x28
>> +#define PHY_CTRL_R11                                           0x2c
>> +#define PHY_CTRL_R12                                           0x30
>> +#define PHY_CTRL_R13                                           0x34
>> +#define PHY_CTRL_R14                                           0x38
>> +#define PHY_CTRL_R15                                           0x3c
>> +#define PHY_CTRL_R16                                           0x40
>> +#define PHY_CTRL_R17                                           0x44
>> +#define PHY_CTRL_R18                                           0x48
>> +#define PHY_CTRL_R19                                           0x4c
>> +#define PHY_CTRL_R20                                           0x50
>> +#define PHY_CTRL_R21                                           0x54
>> +#define PHY_CTRL_R22                                           0x58
>> +#define PHY_CTRL_R23                                           0x5c
>> +
>> +#define RESET_COMPLETE_TIME                                    1000
>> +#define PLL_RESET_COMPLETE_TIME                                        100
>> +
>> +struct phy_meson_g12a_usb2_priv {
>> +       struct device           *dev;
>> +       struct regmap           *regmap;
>> +       struct clk              *clk;
>> +       struct reset_control    *reset;
>> +};
>> +
>> +static const struct regmap_config phy_meson_g12a_usb2_regmap_conf = {
>> +       .reg_bits = 8,
>> +       .val_bits = 32,
>> +       .reg_stride = 4,
>> +       .max_register = PHY_CTRL_R23,
>> +};
>> +
>> +static int phy_meson_g12a_usb2_init(struct phy *phy)
>> +{
>> +       struct phy_meson_g12a_usb2_priv *priv = phy_get_drvdata(phy);
>> +       int ret;
>> +
>> +       ret = reset_control_reset(priv->reset);
>> +       if (ret)
>> +               return ret;
>> +
>> +       udelay(RESET_COMPLETE_TIME);
>> +
>> +       /* usb2_otg_aca_en == 0 */
>> +       regmap_update_bits(priv->regmap, PHY_CTRL_R21, BIT(2), 0);
> do you have the name of the other register bits as well? having
> #defines would make the code easier to read

It's on my TODO list...

> 
>> +
>> +       /* PLL Setup : 24MHz * 20 / 1 = 480MHz */
>> +       regmap_write(priv->regmap, PHY_CTRL_R16, 0x39400414);
>> +       regmap_write(priv->regmap, PHY_CTRL_R17, 0x927e0000);
>> +       regmap_write(priv->regmap, PHY_CTRL_R18, 0xac5f49e5);
>> +
>> +       udelay(PLL_RESET_COMPLETE_TIME);
>> +
>> +       /* UnReset PLL */
>> +       regmap_write(priv->regmap, PHY_CTRL_R16, 0x19400414);
>> +
>> +       /* PHY Tuning */
>> +       regmap_write(priv->regmap, PHY_CTRL_R20, 0xfe18);
>> +       regmap_write(priv->regmap, PHY_CTRL_R4, 0x8000fff);
>> +
>> +       /* Tuning Disconnect Threshold */
>> +       regmap_write(priv->regmap, PHY_CTRL_R3, 0x34);
>> +
>> +       /* Analog Settings */
>> +       regmap_write(priv->regmap, PHY_CTRL_R14, 0);
>> +       regmap_write(priv->regmap, PHY_CTRL_R13, 0x78000);
>> +
>> +       return 0;
>> +}
>> +
>> +static int phy_meson_g12a_usb2_exit(struct phy *phy)
>> +{
>> +       struct phy_meson_g12a_usb2_priv *priv = phy_get_drvdata(phy);
>> +
>> +       return reset_control_reset(priv->reset);
> do you have information on whether we should reset_control_assert here
> instead of reset_control_reset?

No, I don't have it, I haven't fully validated the remove/suspend path yet,
but this should at least put the PHY in an unconfigured mode.

> 
>> +}
>> +
>> +/* set_mode is not needed, mode setting is handled via the UTMI bus */
>> +static const struct phy_ops phy_meson_g12a_usb2_ops = {
>> +       .init           = phy_meson_g12a_usb2_init,
>> +       .exit           = phy_meson_g12a_usb2_exit,
>> +       .owner          = THIS_MODULE,
>> +};
>> +
>> +static int phy_meson_g12a_usb2_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct phy_provider *phy_provider;
>> +       struct resource *res;
>> +       struct phy_meson_g12a_usb2_priv *priv;
>> +       struct phy *phy;
>> +       void __iomem *base;
>> +       int ret;
>> +
>> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +       if (!priv)
>> +               return -ENOMEM;
>> +
>> +       priv->dev = dev;
>> +       platform_set_drvdata(pdev, priv);
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       base = devm_ioremap_resource(dev, res);
>> +       if (IS_ERR(base))
>> +               return PTR_ERR(base);
>> +
>> +       priv->regmap = devm_regmap_init_mmio(dev, base,
>> +                                            &phy_meson_g12a_usb2_regmap_conf);
>> +       if (IS_ERR(priv->regmap))
>> +               return PTR_ERR(priv->regmap);
>> +
>> +       priv->clk = devm_clk_get(dev, "xtal");
>> +       if (IS_ERR(priv->clk))
>> +               return PTR_ERR(priv->clk);
>> +
>> +       priv->reset = devm_reset_control_get(dev, "phy");
>> +       if (IS_ERR(priv->reset))
>> +               return PTR_ERR(priv->reset);
>> +
>> +       ret = reset_control_deassert(priv->reset);
> should we move this to phy_meson_g12a_usb2_init (which currently uses
> reset_control_reset)?
> then we could also use reset_control_assert in phy_meson_g12a_usb2_exit

Maybe it would be better, indeed.

> 
>> +       if (ret)
>> +               return ret;
>> +
>> +       phy = devm_phy_create(dev, NULL, &phy_meson_g12a_usb2_ops);
>> +       if (IS_ERR(phy)) {
>> +               ret = PTR_ERR(phy);
>> +               if (ret != -EPROBE_DEFER)
>> +                       dev_err(dev, "failed to create PHY\n");
> phy-core only returns EPROBE_DEFER in case the phy-supply is not ready yet.
> we won't need a phy-supply (at least as far as I know, if we do then
> please document this in the dt-bindings patch). for USB VBUS dwc2
> support the "vbus-supply" property which we should use instead.
> I leave it up to you to decide whether the check makes sense

It's always better to handle these return cases, better to leave it.

> 
>> +               return ret;
>> +       }
>> +
>> +       phy_set_bus_width(phy, 8);
>> +       phy_set_drvdata(phy, priv);
>> +
>> +       phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>> +
>> +       return PTR_ERR_OR_ZERO(phy_provider);
>> +}
>> +
>> +static const struct of_device_id phy_meson_g12a_usb2_of_match[] = {
>> +       { .compatible = "amlogic,g12a-usb2-phy", },
>> +       { },
>> +};
>> +MODULE_DEVICE_TABLE(of, phy_meson_g12a_usb2_of_match);
>> +
>> +static struct platform_driver phy_meson_g12a_usb2_driver = {
>> +       .probe  = phy_meson_g12a_usb2_probe,
>> +       .driver = {
>> +               .name           = "phy-meson-g12a-usb2",
>> +               .of_match_table = phy_meson_g12a_usb2_of_match,
>> +       },
>> +};
>> +module_platform_driver(phy_meson_g12a_usb2_driver);
>> +
>> +MODULE_AUTHOR("Martin Blumenstingl <martin.blumenstingl@googlemail.com>");
> I haven't contributed any of the relevant code to this driver (I
> assume you took one of our existing Amlogic PHY drivers as skeleton?).
> it's up to you whether you want to keep me in here

Yep it's for the skeleton ! No harm to keep you in !

> 
> 
> Regards
> Martin
> 

Thanks,
Neil

Patch

diff --git a/drivers/phy/amlogic/Kconfig b/drivers/phy/amlogic/Kconfig
index 23fe1cda2f70..78d6e194dce9 100644
--- a/drivers/phy/amlogic/Kconfig
+++ b/drivers/phy/amlogic/Kconfig
@@ -36,3 +36,15 @@  config PHY_MESON_GXL_USB3
 	  Enable this to support the Meson USB3 PHY and OTG detection
 	  IP block found in Meson GXL and GXM SoCs.
 	  If unsure, say N.
+
+config PHY_MESON_G12A_USB2
+	tristate "Meson G12A USB2 PHY drivers"
+	default ARCH_MESON
+	depends on OF && (ARCH_MESON || COMPILE_TEST)
+	depends on USB_SUPPORT
+	select GENERIC_PHY
+	select REGMAP_MMIO
+	help
+	  Enable this to support the Meson USB2 PHYs found in Meson
+	  G12A SoCs.
+	  If unsure, say N.
diff --git a/drivers/phy/amlogic/Makefile b/drivers/phy/amlogic/Makefile
index 4fd8848c194d..7d4d10f5a6b3 100644
--- a/drivers/phy/amlogic/Makefile
+++ b/drivers/phy/amlogic/Makefile
@@ -1,3 +1,4 @@ 
 obj-$(CONFIG_PHY_MESON8B_USB2)		+= phy-meson8b-usb2.o
 obj-$(CONFIG_PHY_MESON_GXL_USB2)	+= phy-meson-gxl-usb2.o
+obj-$(CONFIG_PHY_MESON_G12A_USB2)	+= phy-meson-g12a-usb2.o
 obj-$(CONFIG_PHY_MESON_GXL_USB3)	+= phy-meson-gxl-usb3.o
diff --git a/drivers/phy/amlogic/phy-meson-g12a-usb2.c b/drivers/phy/amlogic/phy-meson-g12a-usb2.c
new file mode 100644
index 000000000000..3b6271a8be02
--- /dev/null
+++ b/drivers/phy/amlogic/phy-meson-g12a-usb2.c
@@ -0,0 +1,191 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Meson G12A USB2 PHY driver
+ *
+ * Copyright (C) 2017 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
+ * Copyright (C) 2017 Amlogic, Inc. All rights reserved
+ * Copyright (C) 2019 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/reset.h>
+#include <linux/phy/phy.h>
+#include <linux/usb/otg.h>
+#include <linux/platform_device.h>
+
+#define PHY_CTRL_R0						0x0
+#define PHY_CTRL_R1						0x4
+#define PHY_CTRL_R2						0x8
+#define PHY_CTRL_R3						0xc
+#define PHY_CTRL_R4						0x10
+#define PHY_CTRL_R5						0x14
+#define PHY_CTRL_R6						0x18
+#define PHY_CTRL_R7						0x1c
+#define PHY_CTRL_R8						0x20
+#define PHY_CTRL_R9						0x24
+#define PHY_CTRL_R10						0x28
+#define PHY_CTRL_R11						0x2c
+#define PHY_CTRL_R12						0x30
+#define PHY_CTRL_R13						0x34
+#define PHY_CTRL_R14						0x38
+#define PHY_CTRL_R15						0x3c
+#define PHY_CTRL_R16						0x40
+#define PHY_CTRL_R17						0x44
+#define PHY_CTRL_R18						0x48
+#define PHY_CTRL_R19						0x4c
+#define PHY_CTRL_R20						0x50
+#define PHY_CTRL_R21						0x54
+#define PHY_CTRL_R22						0x58
+#define PHY_CTRL_R23						0x5c
+
+#define RESET_COMPLETE_TIME					1000
+#define PLL_RESET_COMPLETE_TIME					100
+
+struct phy_meson_g12a_usb2_priv {
+	struct device		*dev;
+	struct regmap		*regmap;
+	struct clk		*clk;
+	struct reset_control	*reset;
+};
+
+static const struct regmap_config phy_meson_g12a_usb2_regmap_conf = {
+	.reg_bits = 8,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = PHY_CTRL_R23,
+};
+
+static int phy_meson_g12a_usb2_init(struct phy *phy)
+{
+	struct phy_meson_g12a_usb2_priv *priv = phy_get_drvdata(phy);
+	int ret;
+
+	ret = reset_control_reset(priv->reset);
+	if (ret)
+		return ret;
+
+	udelay(RESET_COMPLETE_TIME);
+
+	/* usb2_otg_aca_en == 0 */
+	regmap_update_bits(priv->regmap, PHY_CTRL_R21, BIT(2), 0);
+
+	/* PLL Setup : 24MHz * 20 / 1 = 480MHz */
+	regmap_write(priv->regmap, PHY_CTRL_R16, 0x39400414);
+	regmap_write(priv->regmap, PHY_CTRL_R17, 0x927e0000);
+	regmap_write(priv->regmap, PHY_CTRL_R18, 0xac5f49e5);
+
+	udelay(PLL_RESET_COMPLETE_TIME);
+
+	/* UnReset PLL */
+	regmap_write(priv->regmap, PHY_CTRL_R16, 0x19400414);
+
+	/* PHY Tuning */
+	regmap_write(priv->regmap, PHY_CTRL_R20, 0xfe18);
+	regmap_write(priv->regmap, PHY_CTRL_R4, 0x8000fff);
+
+	/* Tuning Disconnect Threshold */
+	regmap_write(priv->regmap, PHY_CTRL_R3, 0x34);
+
+	/* Analog Settings */
+	regmap_write(priv->regmap, PHY_CTRL_R14, 0);
+	regmap_write(priv->regmap, PHY_CTRL_R13, 0x78000);
+
+	return 0;
+}
+
+static int phy_meson_g12a_usb2_exit(struct phy *phy)
+{
+	struct phy_meson_g12a_usb2_priv *priv = phy_get_drvdata(phy);
+
+	return reset_control_reset(priv->reset);
+}
+
+/* set_mode is not needed, mode setting is handled via the UTMI bus */
+static const struct phy_ops phy_meson_g12a_usb2_ops = {
+	.init		= phy_meson_g12a_usb2_init,
+	.exit		= phy_meson_g12a_usb2_exit,
+	.owner		= THIS_MODULE,
+};
+
+static int phy_meson_g12a_usb2_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy_provider *phy_provider;
+	struct resource *res;
+	struct phy_meson_g12a_usb2_priv *priv;
+	struct phy *phy;
+	void __iomem *base;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	platform_set_drvdata(pdev, priv);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	priv->regmap = devm_regmap_init_mmio(dev, base,
+					     &phy_meson_g12a_usb2_regmap_conf);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
+
+	priv->clk = devm_clk_get(dev, "xtal");
+	if (IS_ERR(priv->clk))
+		return PTR_ERR(priv->clk);
+
+	priv->reset = devm_reset_control_get(dev, "phy");
+	if (IS_ERR(priv->reset))
+		return PTR_ERR(priv->reset);
+
+	ret = reset_control_deassert(priv->reset);
+	if (ret)
+		return ret;
+
+	phy = devm_phy_create(dev, NULL, &phy_meson_g12a_usb2_ops);
+	if (IS_ERR(phy)) {
+		ret = PTR_ERR(phy);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to create PHY\n");
+
+		return ret;
+	}
+
+	phy_set_bus_width(phy, 8);
+	phy_set_drvdata(phy, priv);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id phy_meson_g12a_usb2_of_match[] = {
+	{ .compatible = "amlogic,g12a-usb2-phy", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, phy_meson_g12a_usb2_of_match);
+
+static struct platform_driver phy_meson_g12a_usb2_driver = {
+	.probe	= phy_meson_g12a_usb2_probe,
+	.driver	= {
+		.name		= "phy-meson-g12a-usb2",
+		.of_match_table	= phy_meson_g12a_usb2_of_match,
+	},
+};
+module_platform_driver(phy_meson_g12a_usb2_driver);
+
+MODULE_AUTHOR("Martin Blumenstingl <martin.blumenstingl@googlemail.com>");
+MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
+MODULE_DESCRIPTION("Meson G12A USB2 PHY driver");
+MODULE_LICENSE("GPL v2");