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
| ||
|
Date: Tue, 25 Aug 2020 11:05:14 +0800 From: "Ramuthevar, Vadivel MuruganX" <vadivel.muruganx.ramuthevar@...ux.intel.com> To: Vinod Koul <vkoul@...nel.org> Cc: linux-kernel@...r.kernel.org, kishon@...com, devicetree@...r.kernel.org, robh+dt@...nel.org, gregkh@...uxfoundation.org, p.zabel@...gutronix.de, balbi@...nel.org, andriy.shevchenko@...el.com, cheol.yong.kim@...el.com, qi-ming.wu@...el.com, yin1.li@...el.com Subject: Re: [RESEND PATCH v8 2/2] phy: Add USB3 PHY support for Intel LGM SoC Hi Vinod, Thank you for the review comments... On 24/8/2020 12:06 am, Vinod Koul wrote: > On 17-08-20, 15:05, Ramuthevar,Vadivel MuruganX wrote: >> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@...ux.intel.com> >> >> Add support for USB PHY on Intel LGM SoC. >> >> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@...ux.intel.com> >> Reviewed-by: Philipp Zabel <p.zabel@...gutronix.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? Good catch, it's not required. > >> +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? Noted. > >> +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? Yes, it's required during the reboot and already device is connected status update to controller, well tested. > >> +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 Sure, we can handle it separately. > >> + 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 Noted. > >> + } else { >> + val = TCPC_DISCONN; >> + dev_info(ta->phy.dev, "disconnected\n"); > > here too Noted. > >> +static int vbus_notifier(struct notifier_block *nb, unsigned long evnt, void *ptr) >> +{ >> + return NOTIFY_DONE; >> +} > > empty notifier, why bother? Let me check and double confirm if it's required or not. > >> +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? yes you're right but not required for Intel LGM SoC, it's working fine. Regards Vadivel > >> + /* >> + * 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..? >
Powered by blists - more mailing lists