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

[Neil Armstrong]

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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxxx>
>> + * Copyright (C) 2017 Amlogic, Inc. All rights reserved
>> + * Copyright (C) 2019 BayLibre, SAS
>> + * Author: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>> + */
>> +
>> +#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@xxxxxxxxxxxxxx>");
> 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