| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFBinCCAi_NYTvGPK+MNoqn3YhOHrSLrH4ybFYJvBSNkSCZ8BQ@mail.gmail.com>
Date: Sun, 17 Feb 2019 23:24:01 +0100
From: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To: Neil Armstrong <narmstrong@...libre.com>
Cc: gregkh@...uxfoundation.org, hminas@...opsys.com, balbi@...nel.org,
kishon@...com, linux-amlogic@...ts.infradead.org,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 5/8] phy: amlogic: add Amlogic G12A USB2 PHY Driver
Hi Neil,
On Tue, Feb 12, 2019 at 4:15 PM Neil Armstrong <narmstrong@...libre.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@...libre.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@...glemail.com>
> + * Copyright (C) 2017 Amlogic, Inc. All rights reserved
> + * Copyright (C) 2019 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@...libre.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@...glemail.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
Powered by blists - more mailing lists