On Tue, 31 Oct 2023 01:39:53 -0500
Samuel Holland <sam...@sholland.org> wrote:
Hi Samuel,
> The device tree binding for the PHY provides VBUS supplies as regulator
> references. Now that all boards have the appropriate regulator uclass
> drivers enabled, the PHY driver can switch to using them. This replaces
> direct GPIO usage, which in some cases needed a special DM-incompatible
> "virtual" GPIO from the PMIC.
I like that one, avoids one more Kconfig duplicate.
Testing this revealed one problem, though:
> The following boards provided a value for CONFIG_USB0_VBUS_PIN, but are
> missing the "usb0_vbus-supply" property in their device tree. None of
> them have the MUSB controller enabled in host or OTG mode, so they
> should see no impact:
> - Ainol_AW1_defconfig / sun7i-a20-ainol-aw1
> - Ampe_A76_defconfig / sun5i-a13-ampe-a76
> - CHIP_pro_defconfig / sun5i-gr8-chip-pro
> - Cubieboard4_defconfig / sun9i-a80-cubieboard4
> - Merrii_A80_Optimus_defconfig / sun9i-a80-optimus
> - Sunchip_CX-A99_defconfig / sun9i-a80-cx-a99
> - Yones_Toptech_BD1078_defconfig / sun7i-a20-yones-toptech-bd1078
> - Yones_Toptech_BS1078_V2_defconfig /
> sun6i-a31s-yones-toptech-bs1078-v2
> - iNet_3F_defconfig / sun4i-a10-inet-3f
> - iNet_3W_defconfig / sun4i-a10-inet-3w
> - iNet_86VS_defconfig / sun5i-a13-inet-86vs
> - iNet_D978_rev2_defconfig / sun8i-a33-inet-d978-rev2
> - icnova-a20-swac_defconfig / sun7i-a20-icnova-swac
> - sun8i_a23_evb_defconfig / sun8i-a23-evb
>
> Similarly, the following boards set CONFIG_USB1_VBUS_PIN, but do not
> have "usb1_vbus-supply" in their device tree. Neither of them have USB
> enabled at all, so again there should be no impact:
> - Cubieboard4_defconfig / sun9i-a80-cubieboard4 (also for USB3)
> - sun8i_a23_evb_defconfig / sun8i-a23-evb
>
> The following boards use a different pin for USB1 VBUS between their
> defconfig and their device tree. Depending on which is correct, they
> may be broken:
> - Linksprite_pcDuino3_Nano_defconfig (PH11) /
> sun7i-a20-pcduino3-nano (PD2)
> - icnova-a20-swac_defconfig (PG10) / sun7i-a20-icnova-swac (PH6)
>
> Finally, this board has conflicting pins given for its USB2 VBUS:
> - Lamobo_R1_defconfig (PH3) / sun7i-a20-lamobo-r1 (PH12)
>
> Signed-off-by: Samuel Holland <sam...@sholland.org>
> ---
>
> drivers/phy/allwinner/phy-sun4i-usb.c | 41 +++++++++++++--------------
> 1 file changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c
> b/drivers/phy/allwinner/phy-sun4i-usb.c
> index 6624e9134f4..dc34b828a3a 100644
> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> @@ -87,27 +87,22 @@ struct sun4i_usb_phy_cfg {
> };
>
> struct sun4i_usb_phy_info {
> - const char *gpio_vbus;
> const char *gpio_vbus_det;
> const char *gpio_id_det;
> } phy_info[] = {
> {
> - .gpio_vbus = CONFIG_USB0_VBUS_PIN,
> .gpio_vbus_det = CONFIG_USB0_VBUS_DET,
> .gpio_id_det = CONFIG_USB0_ID_DET,
> },
> {
> - .gpio_vbus = CONFIG_USB1_VBUS_PIN,
> .gpio_vbus_det = NULL,
> .gpio_id_det = NULL,
> },
> {
> - .gpio_vbus = CONFIG_USB2_VBUS_PIN,
> .gpio_vbus_det = NULL,
> .gpio_id_det = NULL,
> },
> {
> - .gpio_vbus = CONFIG_USB3_VBUS_PIN,
> .gpio_vbus_det = NULL,
> .gpio_id_det = NULL,
> },
> @@ -115,12 +110,12 @@ struct sun4i_usb_phy_info {
>
> struct sun4i_usb_phy_plat {
> void __iomem *pmu;
> - struct gpio_desc gpio_vbus;
> struct gpio_desc gpio_vbus_det;
> struct gpio_desc gpio_id_det;
> struct clk clocks;
> struct clk clk2;
> struct reset_ctl resets;
> + struct udevice *vbus;
> int id;
> };
>
> @@ -209,6 +204,7 @@ static int sun4i_usb_phy_power_on(struct phy *phy)
> {
> struct sun4i_usb_phy_data *data = dev_get_priv(phy->dev);
> struct sun4i_usb_phy_plat *usb_phy = &data->usb_phy[phy->id];
> + int ret;
>
> if (initial_usb_scan_delay) {
> mdelay(initial_usb_scan_delay);
> @@ -221,8 +217,11 @@ static int sun4i_usb_phy_power_on(struct phy *phy)
> return 0;
> }
>
> - if (dm_gpio_is_valid(&usb_phy->gpio_vbus))
> - dm_gpio_set_value(&usb_phy->gpio_vbus, 1);
> + if (usb_phy->vbus) {
> + ret = regulator_set_enable(usb_phy->vbus, true);
> + if (ret && ret != -ENOSYS)
> + return ret;
> + }
>
> return 0;
> }
> @@ -231,9 +230,13 @@ static int sun4i_usb_phy_power_off(struct phy *phy)
> {
> struct sun4i_usb_phy_data *data = dev_get_priv(phy->dev);
> struct sun4i_usb_phy_plat *usb_phy = &data->usb_phy[phy->id];
> + int ret;
>
> - if (dm_gpio_is_valid(&usb_phy->gpio_vbus))
> - dm_gpio_set_value(&usb_phy->gpio_vbus, 0);
> + if (usb_phy->vbus) {
> + ret = regulator_set_enable(usb_phy->vbus, false);
> + if (ret && ret != -ENOSYS)
> + return ret;
> + }
>
> return 0;
> }
> @@ -483,22 +486,16 @@ static int sun4i_usb_phy_probe(struct udevice *dev)
> for (i = 0; i < data->cfg->num_phys; i++) {
> struct sun4i_usb_phy_plat *phy = &plat[i];
> struct sun4i_usb_phy_info *info = &phy_info[i];
> - char name[16];
> + char name[32];
>
> if (data->cfg->missing_phys & BIT(i))
> continue;
>
> - ret = dm_gpio_lookup_name(info->gpio_vbus, &phy->gpio_vbus);
> - if (ret == 0) {
> - ret = dm_gpio_request(&phy->gpio_vbus, "usb_vbus");
> - if (ret)
> - return ret;
> - ret = dm_gpio_set_dir_flags(&phy->gpio_vbus,
> - GPIOD_IS_OUT);
> - if (ret)
> - return ret;
> - ret = dm_gpio_set_value(&phy->gpio_vbus, 0);
> - if (ret)
> + snprintf(name, sizeof(name), "usb%d_vbus-supply", i);
> + ret = device_get_supply_regulator(dev, name, &phy->vbus);
> + if (phy->vbus) {
> + ret = regulator_set_enable(phy->vbus, false);
It's quite likely that this regulator is already disabled, in this case
regulator_set_enable() returns -EALREADY, and probe() exists prematurely.
There is regulator_set_enable_if_allowed() to cover that case, and also
if we want to disable an always-on regulator. When replacing it (and
the call above, in sun4i_usb_phy_power_off()), it works for me, for
instance for the GPIO controlled Orange Pi Zero3, where is was failing
before.
I have fixed that in my tree, if nothing else pops up, there is no need
to re-send.
Cheers,
Andre
> + if (ret && ret != -ENOSYS)
> return ret;
> }
>
