| 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: <a2a87ae8-932a-4bc0-8c9c-f08bf109f0f2@kernel.org>
Date: Wed, 1 Nov 2023 11:51:00 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Pu Li <pu.li@...soc.com>, Vinod Koul <vkoul@...nel.org>,
Kishon Vijay Abraham I <kishon@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>
Cc: Zhiyong Liu <zhiyong.liu@...soc.com>,
Chunyan Zhang <zhang.lyra@...il.com>,
Orson Zhai <orsonzhai@...il.com>,
linux-phy@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] phy: sprd: Add Spreadtrum usb20 hsphy driver
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@...soc.com>
> ---
> +
> +#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@...il.com>");
> +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
Powered by blists - more mailing lists