lkml.org 
[lkml]   [2020]   [Aug]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RESEND PATCH v8 2/2] phy: Add USB3 PHY support for Intel LGM SoC
    On 17-08-20, 15:05, Ramuthevar,Vadivel MuruganX wrote:
    > From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
    >
    > Add support for USB PHY on Intel LGM SoC.
    >
    > Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
    > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
    > ---
    > drivers/phy/Kconfig | 11 ++
    > drivers/phy/Makefile | 1 +
    > drivers/phy/phy-lgm-usb.c | 278 ++++++++++++++++++++++++++++++++++++++++++++++
    > 3 files changed, 290 insertions(+)
    > create mode 100644 drivers/phy/phy-lgm-usb.c
    >
    > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
    > index de9362c25c07..01b53f86004c 100644
    > --- a/drivers/phy/Kconfig
    > +++ b/drivers/phy/Kconfig
    > @@ -49,6 +49,17 @@ config PHY_XGENE
    > help
    > This option enables support for APM X-Gene SoC multi-purpose PHY.
    >
    > +config USB_LGM_PHY
    > + tristate "INTEL Lightning Mountain USB PHY Driver"
    > + depends on USB_SUPPORT

    Why is the dependent on USB..? Should that not be other way around?

    > +static int get_flipped(struct tca_apb *ta, bool *flipped)
    > +{
    > + union extcon_property_value property;
    > + int ret;
    > +
    > + ret = extcon_get_property(ta->phy.edev, EXTCON_USB_HOST,
    > + EXTCON_PROP_USB_TYPEC_POLARITY, &property);
    > + if (ret) {
    > + dev_err(ta->phy.dev, "no polarity property from extcon\n");
    > + return ret;
    > + }
    > +
    > + *flipped = property.intval;
    > +
    > + return ret;

    return 0 here?

    > +static int phy_init(struct usb_phy *phy)
    > +{
    > + struct tca_apb *ta = container_of(phy, struct tca_apb, phy);
    > + void __iomem *ctrl1 = phy->io_priv + CTRL1_OFFSET;
    > + int val, ret, i;
    > +
    > + if (ta->phy_initialized)
    > + return 0;
    > +
    > + for (i = 0; i < ARRAY_SIZE(PHY_RESETS); i++)
    > + reset_control_deassert(ta->resets[i]);
    > +
    > + ret = readl_poll_timeout(ctrl1, val, val & SRAM_INIT_DONE, 10, 10 * 1000);
    > + if (ret) {
    > + dev_err(ta->phy.dev, "SRAM init failed, 0x%x\n", val);
    > + return ret;
    > + }
    > +
    > + writel(readl(ctrl1) | SRAM_EXT_LD_DONE, ctrl1);
    > +
    > + ta->phy_initialized = true;
    > + if (!ta->phy.edev) {
    > + writel(TCPC_CONN, ta->phy.io_priv + TCPC_OFFSET);
    > + return phy->set_vbus(phy, true);
    > + }
    > +
    > + schedule_work(&ta->wk);

    why work for init?

    > +static void tca_work(struct work_struct *work)
    > +{
    > + struct tca_apb *ta = container_of(work, struct tca_apb, wk);
    > + bool connected;
    > + bool flipped = false;
    > + u32 val;
    > + int ret;
    > +
    > + ret = get_flipped(ta, &flipped);

    so every time this work is scheduled you are reading from firmware, why..
    Typically we should read in probe and store it..

    > + connected = extcon_get_state(ta->phy.edev, EXTCON_USB_HOST) && !ret;

    lets handle ret and extcon_get_state separately please

    > + if (connected == ta->connected)
    > + return;
    > +
    > + ta->connected = connected;
    > + if (connected) {
    > + val = TCPC_CONN;
    > + if (flipped)
    > + val |= TCPC_FLIPPED;
    > + dev_info(ta->phy.dev, "connected%s\n", flipped ? " flipped" : "");

    debug perhaps

    > + } else {
    > + val = TCPC_DISCONN;
    > + dev_info(ta->phy.dev, "disconnected\n");

    here too

    > +static int vbus_notifier(struct notifier_block *nb, unsigned long evnt, void *ptr)
    > +{
    > + return NOTIFY_DONE;
    > +}

    empty notifier, why bother?

    > +static int phy_probe(struct platform_device *pdev)
    > +{
    > + struct reset_control *resets[ARRAY_SIZE(CTL_RESETS)];
    > + struct device *dev = &pdev->dev;
    > + struct usb_phy *phy;
    > + struct tca_apb *ta;
    > + int i;
    > +
    > + ta = devm_kzalloc(dev, sizeof(*ta), GFP_KERNEL);
    > + if (!ta)
    > + return -ENOMEM;
    > +
    > + platform_set_drvdata(pdev, ta);
    > + INIT_WORK(&ta->wk, tca_work);
    > +
    > + phy = &ta->phy;
    > + phy->dev = dev;
    > + phy->label = dev_name(dev);
    > + phy->type = USB_PHY_TYPE_USB3;
    > + phy->init = phy_init;
    > + phy->shutdown = phy_shutdown;
    > + phy->set_vbus = phy_set_vbus;
    > + phy->id_nb.notifier_call = id_notifier;
    > + phy->vbus_nb.notifier_call = vbus_notifier;
    > +
    > + phy->io_priv = devm_platform_ioremap_resource(pdev, 0);
    > + if (IS_ERR(phy->io_priv))
    > + return PTR_ERR(phy->io_priv);
    > +
    > + ta->vbus = devm_regulator_get(dev, "vbus");
    > + if (IS_ERR(ta->vbus))
    > + return PTR_ERR(ta->vbus);
    > +
    > + for (i = 0; i < ARRAY_SIZE(CTL_RESETS); i++) {
    > + resets[i] = devm_reset_control_get_exclusive(dev, CTL_RESETS[i]);
    > + if (IS_ERR(resets[i])) {
    > + dev_err(dev, "%s reset not found\n", CTL_RESETS[i]);
    > + return PTR_ERR(resets[i]);
    > + }
    > + }
    > +
    > + for (i = 0; i < ARRAY_SIZE(PHY_RESETS); i++) {
    > + ta->resets[i] = devm_reset_control_get_exclusive(dev, PHY_RESETS[i]);
    > + if (IS_ERR(ta->resets[i])) {
    > + dev_err(dev, "%s reset not found\n", PHY_RESETS[i]);
    > + return PTR_ERR(ta->resets[i]);
    > + }
    > + }
    > +
    > + for (i = 0; i < ARRAY_SIZE(CTL_RESETS); i++)
    > + reset_control_assert(resets[i]);
    > +
    > + for (i = 0; i < ARRAY_SIZE(PHY_RESETS); i++)
    > + reset_control_assert(ta->resets[i]);

    no time lag b/w assert and dessert below?

    > + /*
    > + * Out-of-band reset of the controller after PHY reset will cause
    > + * controller malfunctioning, so we should use in-band controller
    > + * reset only and leave the controller de-asserted here.
    > + */
    > + for (i = 0; i < ARRAY_SIZE(CTL_RESETS); i++)
    > + reset_control_deassert(resets[i]);
    > +
    > + /* Need to wait at least 20us after de-assert the controller */
    > + usleep_range(20, 100);
    > +
    > + return usb_add_phy_dev(phy);

    aha, this is usb_phy stuff.

    Kishon this is not really a generic phy driver, should it be in
    drivers/phy..?

    --
    ~Vinod

    \
     
     \ /
      Last update: 2020-08-23 18:07    [W:4.610 / U:0.144 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site