Re: [PATCH 2/2] phy: sprd: Add Spreadtrum usb20 hsphy driver

From: Krzysztof Kozlowski
Date: Wed Nov 01 2023 - 06:51:19 EST

Next message: Fuad Tabba: "Re: [PATCH v13 16/35] KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory"
Previous message: Vlastimil Babka: "Re: kernel/fork.c:1647:9: sparse: sparse: incorrect type in argument 1 (different address spaces)"
In reply to: Pu Li: "[PATCH 2/2] phy: sprd: Add Spreadtrum usb20 hsphy driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 01/11/2023 06:44, Pu Li wrote:
> Add Spreadtrum platform USB20 HSPHY driver support. This driver
> takes care of all the PHY functionality, normally paired with
> DesignWare USB20 (DRD) Controller or Spreadtrum musb phy (DRD )controller.
>
> Signed-off-by: Pu Li <pu.li@xxxxxxxxxx>
> ---

> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/timer.h>
> +#include <linux/usb/otg.h>
> +#include <uapi/linux/usb/charger.h>
> +
> +#include "phy-sprd-usb20-hs.h"
> +
> +static const struct sprd_hsphy_cfg *phy_cfg;

File-scope variables do not look good.

> +

...

> +
> +static int sprd_hsphy_cali_mode(void)
> +{
> + struct device_node *cmdline_node;
> + const char *cmdline, *mode;
> + int ret;
> +
> + cmdline_node = of_find_node_by_path("/chosen");
> + ret = of_property_read_string(cmdline_node, "bootargs", &cmdline);
> +
> + if (ret) {
> + pr_err("Can't not parse bootargs\n");
> + return 0;
> + }
> +
> + mode = strstr(cmdline, "androidboot.mode=cali");

NAK, drop this nonsense.

> + if (mode)
> + return 1;
> +
> + mode = strstr(cmdline, "sprdboot.mode=cali");

NAK, drop this nonsense.

> + if (mode)
> + return 1;
> +
> + return 0;
> +}
> +
> +static int sprd_hsphy_probe(struct platform_device *pdev)
> +{
> + struct sprd_hsphy *phy;
> + struct resource *res;
> + struct device *dev = &pdev->dev;
> + int ret = 0, calimode = 0;
> + struct usb_otg *otg;
> +
> + phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
> + if (!phy)
> + return -ENOMEM;
> +
> + otg = devm_kzalloc(dev, sizeof(*otg), GFP_KERNEL);
> + if (!otg)
> + return -ENOMEM;
> +
> + /* phy cfg data */
> + phy_cfg = of_device_get_match_data(dev);
> + if (!phy_cfg) {
> + dev_err(dev, "no matching driver data found\n");
> + return -EINVAL;
> + }
> +
> + /* set vdd */
> + ret = of_property_read_u32(dev->of_node, "sprd,vdd-voltage",
> + &phy->vdd_vol);
> + if (ret < 0) {
> + dev_err(dev, "unable to read hsphy vdd voltage\n");
> + return ret;
> + }
> +
> + calimode = sprd_hsphy_cali_mode();
> + if (calimode) {
> + phy->vdd_vol = phy_cfg->parameters[FULLSPEED_USB33_TUNE];
> + dev_info(dev, "calimode vdd_vol:%d\n", phy->vdd_vol);
> + }
> +
> + phy->vdd = devm_regulator_get(dev, "vdd");
> + if (IS_ERR(phy->vdd)) {
> + dev_err(dev, "unable to get hsphy vdd supply\n");

You do not have regulators. You clearly did not test the code, DTS or
the bindings. Maybe nothing here was tested.

> + return PTR_ERR(phy->vdd);

Syntax is anyway return dev_err_probe().

> + }
> +
> + ret = regulator_set_voltage(phy->vdd, phy->vdd_vol, phy->vdd_vol);
> + if (ret < 0) {
> + dev_err(dev, "fail to set hsphy vdd voltage at %dmV\n",
> + phy->vdd_vol);
> + return ret;
> + }
> +
> + /* phy tune */
> + if (phy_cfg->phy_version == VERSION1) {
> + ret = of_property_read_u32(dev->of_node, "sprd,tune-value",

Nope, it is not allowed in your bindings.

> + &phy->phy_tune);
> + if (ret < 0) {
> + dev_err(dev, "unable to read hsphy usb phy tune\n");
> + return ret;
> + }
> + }
> +
> + /* phy base */
> + if (phy_cfg->phy_version == VERSION1 ||
> + phy_cfg->phy_version == VERSION2) {
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy_glb_regs");

This was not expressed in the bindings.

> + if (!res) {
> + dev_err(dev, "missing USB PHY registers resource\n");
> + return -ENODEV;
> + }
> +
> + phy->base = devm_ioremap(dev, res->start, resource_size(res));
> + if (IS_ERR(phy->base)) {
> + dev_err(dev, "unable to get phy base!\n");
> + return PTR_ERR(phy->base);
> + }
> + }
> +
> + /* analog & aoapb & apahb regmap */
> + phy->aon_apb = syscon_regmap_lookup_by_phandle(dev->of_node,
> + "sprd,syscon-enable");
> + if (IS_ERR(phy->aon_apb)) {
> + dev_err(dev, "USB aon apb syscon failed!\n");

return dev_err_probe, if it stays

> + return PTR_ERR(phy->aon_apb);
> + }
> +
> + if (phy_cfg->phy_version == VERSION2) {

> + phy->ap_ahb = syscon_regmap_lookup_by_phandle(dev->of_node,
> + "sprd,syscon-apahb");

NAK, there is no such property!

> + if (IS_ERR(phy->ap_ahb)) {
> + dev_err(dev, "USB apahb syscon failed!\n");

> + return PTR_ERR(phy->ap_ahb);
> + }
> + }
> +
> + if (phy_cfg->phy_version != VERSION1) {

This was not expressed in your bindings

> + phy->analog = syscon_regmap_lookup_by_phandle(dev->of_node,
> + "sprd,syscon-ana");

> + if (IS_ERR(phy->analog)) {
> + dev_err(dev, "USB analog syscon failed!\n");
> + return PTR_ERR(phy->analog);

return dev_err_probe, if it stays, but I insist to remove it.

> + }
> + }
> +
> + /* prepare eye pattern */
> + ret = sprd_eye_pattern_prepared(phy, dev);
> + if (ret < 0)
> + dev_warn(dev, "sprd_eye_pattern_prepared failed, ret = %d\n", ret);
> +
> + /* enable usb module */
> + if (phy_cfg->phy_version == VERSION2 ||
> + phy_cfg->phy_version == VERSION3) {
> + phy_cfg->cfg_ops->usb_enable_ctrl(phy, CTRL2);
> + }
> +
> + /* usb phy power down */
> + if (phy_cfg->phy_version != VERSION4)
> + phy_cfg->cfg_ops->usb_phy_power_ctrl(phy, CTRL2);
> +
> + phy->dev = dev;
> + phy->phy.dev = dev;
> + phy->phy.label = "sprd-hsphy";
> + phy->phy.otg = otg;
> + phy->phy.init = sprd_hsphy_init;
> + phy->phy.shutdown = sprd_hsphy_shutdown;
> + phy->phy.set_vbus = sprd_hostphy_set;
> + phy->phy.type = USB_PHY_TYPE_USB2;
> + phy->phy.vbus_nb.notifier_call = sprd_hsphy_vbus_notify;
> + otg->usb_phy = &phy->phy;
> +
> + device_init_wakeup(phy->dev, true);
> + phy->wake_lock = wakeup_source_register(phy->dev, "sprd-hsphy");
> + if (!phy->wake_lock) {
> + dev_err(dev, "fail to register wakeup lock.\n");
> + return -ENOMEM;
> + }
> + INIT_WORK(&phy->work, sprd_hsphy_charger_detect_work);
> + platform_set_drvdata(pdev, phy);
> +
> + ret = usb_add_phy_dev(&phy->phy);
> + if (ret) {
> + dev_err(dev, "fail to add phy\n");
> + return ret;
> + }
> +
> + ret = sysfs_create_groups(&dev->kobj, usb_hsphy_groups);
> + if (ret)
> + dev_warn(dev, "failed to create usb hsphy attributes\n");
> +
> + if (extcon_get_state(phy->phy.edev, EXTCON_USB) > 0)
> + usb_phy_set_charger_state(&phy->phy, USB_CHARGER_PRESENT);
> +
> + dev_info(dev, "sprd usb phy probe ok !\n");

Drop. This code looks very, very poor :(. Lack of testing is even worse.

> +
> + return ret;
> +}

...

> +static int __init sprd_hsphy_driver_init(void)
> +{
> + return platform_driver_register(&sprd_hsphy_driver);
> +}
> +
> +static void __exit sprd_hsphy_driver_exit(void)
> +{
> + platform_driver_unregister(&sprd_hsphy_driver);
> +}
> +
> +late_initcall(sprd_hsphy_driver_init);
> +module_exit(sprd_hsphy_driver_exit);
> +
> +MODULE_ALIAS("platform:spreadtrum-usb20-hsphy");

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.

> +MODULE_AUTHOR("Pu Li <lip308226@xxxxxxxxx>");
> +MODULE_DESCRIPTION("Spreadtrum USB20 HSPHY driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/phy/sprd/phy-sprd-usb20-hs.h b/drivers/phy/sprd/phy-sprd-usb20-hs.h
> new file mode 100644
> index 000000000000..897ee5e64482
> --- /dev/null
> +++ b/drivers/phy/sprd/phy-sprd-usb20-hs.h
> @@ -0,0 +1,525 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * phy-sprd-usb20-hs.h - Spreadtrum usb20 phy Glue layer h file
> + *
> + * Copyright 2020-2023 Unisoc Inc.
> + */
> +
> +#ifndef __SPRD_USB20_HS_H
> +#define __SPRD_USB20_HS_H
> +
> +#include <linux/regmap.h>
> +#include <linux/usb/phy.h>
> +
> +struct sprd_hsphy {
> + struct device *dev;
> + struct usb_phy phy;
> + void __iomem *base;
> + struct regulator *vdd;
> + struct regmap *aon_apb;
> + struct regmap *ap_ahb;
> + struct regmap *analog;
> + struct wakeup_source *wake_lock;
> + struct work_struct work;
> + unsigned long event;
> + u32 vdd_vol;
> + u32 phy_tune;
> + u32 host_eye_pattern;
> + u32 device_eye_pattern;
> + u32 host_otg_ctrl0;
> + u32 device_otg_ctrl0;
> + u32 host_otg_ctrl1;
> + u32 device_otg_ctrl1;
> + atomic_t reset;
> + atomic_t inited;
> + bool is_host;
> +};
> +
> +enum hsphy_parameters {
> + TUNEHSAMP_SHIFT,
> + TUNEEQ_SHIFT,
> + TFREGRES_SHIFT,
> + FULLSPEED_USB33_TUNE,
> +};
> +
> +enum sprd_hsphy_reg_layout {
> + REG_AON_APB_APB_RST1,
> + REG_AON_APB_APB_RST2,
> + REG_AON_APB_APB_EB1,
> + REG_AON_APB_APB_EB2,
> + REG_AON_APB_CGM_REG1,
> + REG_AON_APB_OTG_PHY_TEST,
> + REG_AON_APB_OTG_PHY_CTRL,
> + REG_AON_APB_PWR_CTRL,
> + REG_AON_APB_AON_SOC_USB_CTRL,
> + REG_AON_APB_MIPI_CSI_POWER_CTRL,
> + REG_AP_AHB_AHB_EB,
> + REG_AP_AHB_AHB_RST,
> + REG_AP_AHB_OTG_CTRL0,
> + REG_AP_AHB_OTG_CTRL1,
> + REG_AP_AHB_OTG_PHY_CTRL,
> + REG_AP_AHB_OTG_PHY_TUNE,
> + REG_AP_AHB_OTG_PHY_TEST,
> + REG_ANALOG_USB20_USB20_ISO_SW,
> + REG_ANALOG_USB20_USB20_BATTER_PLL,
> + REG_ANALOG_USB20_USB20_UTMI_CTL1,
> + REG_ANALOG_USB20_USB20_TRIMMING,
> + REG_ANALOG_USB20_USB20_UTMI_CTL2,
> + REG_ANALOG_USB20_REG_SEL_CFG_0,
> + REG_ANALOG_USB20_IDDG,
> + REG_ANALOG_USB20_USB20_PHY,
> +};
> +
> +enum sprd_hsphy_mask_layout {
> + MASK_AON_APB_USB_PHY_PD_S,
> + MASK_AON_APB_USB_PHY_PD_L,
> + MASK_AON_APB_ANLG_APB_EB,
> + MASK_AON_APB_ANLG_EB,
> + MASK_AON_APB_OTG_REF_EB,
> + MASK_AON_APB_ANA_EB,
> + MASK_AON_APB_OTG_UTMI_EB,
> + MASK_AON_APB_AON_USB2_TOP_EB,
> + MASK_AON_APB_OTG_PHY_EB,
> + MASK_AON_APB_CGM_OTG_REF_EN,
> + MASK_AON_APB_CGM_DPHY_REF_EN,
> + MASK_AON_APB_USB_ISO_SW_EN,
> + MASK_AON_APB_OTG_PHY_SOFT_RST,
> + MASK_AON_APB_OTG_UTMI_SOFT_RST,
> + MASK_AON_APB_OTG_VBUS_VALID_PHYREG,
> + MASK_AON_APB_USB2_PHY_IDDIG,
> + MASK_AON_APB_UTMI_WIDTH_SEL,
> + MASK_AON_APB_USB20_CTRL_MUX_REG,
> + MASK_AON_APB_USB20_ISO_SW_EN,
> + MASK_AON_APB_C2G_ANALOG_USB20_USB20_PS_PD_S,
> + MASK_AON_APB_C2G_ANALOG_USB20_USB20_PS_PD_L,
> + MASK_AP_AHB_OTG_EB,
> + MASK_AP_AHB_OTG_PHY_SOFT_RST,
> + MASK_AP_AHB_OTG_UTMI_SOFT_RST,
> + MASK_AP_AHB_OTG_SOFT_RST,
> + MASK_AP_AHB_USB2_PHY_IDDIG,
> + MASK_AP_AHB_OTG_DPPULLDOWN,
> + MASK_AP_AHB_OTG_DMPULLDOWN,
> + MASK_AP_AHB_OTG_VBUS_VALID_EXT,
> + MASK_AP_AHB_OTG_VBUS_VALID_PHYREG,
> + MASK_AP_AHB_UTMI_WIDTH_SEL,
> + MASK_AP_AHB_USB2_DATABUS16_8,
> + MASK_AP_AHB_USB20_SAMPLER_SEL,
> + MASK_AP_AHB_USB20_TUNEHSAMP,
> + MASK_AP_AHB_USB20_TUNEEQ,
> + MASK_AP_AHB_USB20_TFREGRES,
> + MASK_ANALOG_USB20_USB20_VBUSVLDEXT,
> + MASK_ANALOG_USB20_USB20_DATABUS16_8,
> + MASK_DBG_SEL_ANALOG_USB20_USB20_DMPULLDOWN,
> + MASK_DBG_SEL_ANALOG_USB20_USB20_DPPULLDOWN,
> + MASK_ANALOG_USB20_USB20_DMPULLDOWN,
> + MASK_ANALOG_USB20_USB20_DPPULLDOWN,
> + MASK_ANALOG_USB20_UTMIOTG_IDDG,
> + MASK_ANALOG_USB20_USB20_PS_PD_S,
> + MASK_ANALOG_USB20_USB20_PS_PD_L,
> + MASK_ANALOG_USB20_USB20_RESERVED,
> + MASK_ANALOG_USB20_USB20_ISO_SW_EN,
> + MASK_ANALOG_USB20_USB20_TUNEHSAMP,
> + MASK_ANALOG_USB20_USB20_TUNEEQ,
> + MASK_ANALOG_USB20_USB20_TFREGRES,
> +};
> +
> +enum {
> + CTRL0 = 0,
> + CTRL1,
> + CTRL2,
> +};
> +
> +struct sprd_hsphy_cfg_ops {
> + void (*usb_enable_ctrl)(struct sprd_hsphy *phy, int on);
> + void (*usb_phy_power_ctrl)(struct sprd_hsphy *phy, int on);
> + void (*usb_vbus_ctrl)(struct sprd_hsphy *phy, int on);
> + void (*utmi_width_sel)(struct sprd_hsphy *phy);
> + void (*reset_core)(struct sprd_hsphy *phy);
> + int (*set_mode)(struct sprd_hsphy *phy, int on);
> +};
> +
> +enum hsphy_ip_version {
> + VERSION1,
> + VERSION2,
> + VERSION3,
> + VERSION4,
> +};
> +
> +enum hsphy_owner {
> + PIKE2,
> + SHARKLE,
> + SHARKL3,
> + SHARKL5,
> + SHARKL5PRO,
> + QOGIRL6,
> + QOGIRN6LITE,
> + UIS8520,
> +};
> +
> +struct sprd_hsphy_cfg {
> + /* array of registers with different offsets */
> + const unsigned int *regs;
> +
> + const unsigned int *masks;
> +
> + /* private ops for each SOC */
> + const struct sprd_hsphy_cfg_ops *cfg_ops;
> +
> + const unsigned int *parameters;
> +
> + enum hsphy_ip_version phy_version;
> +
> + enum hsphy_owner owner;
> +};
> +
> +static const unsigned int pike2_regs_layout[] = {

Static data allocated in every unit including this header? No, this does
not look like correct code (yeah, it compiles but it is just wrong).

Best regards,
Krzysztof

Next message: Fuad Tabba: "Re: [PATCH v13 16/35] KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory"
Previous message: Vlastimil Babka: "Re: kernel/fork.c:1647:9: sparse: sparse: incorrect type in argument 1 (different address spaces)"
In reply to: Pu Li: "[PATCH 2/2] phy: sprd: Add Spreadtrum usb20 hsphy driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]