| Message ID | 20210617194154.2397-7-linux.amoon@gmail.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | Meson-8b and Meson-gxbb USB phy code re-structure | expand |
Hi Anand, On Thu, Jun 17, 2021 at 9:44 PM Anand Moon <linux.amoon@gmail.com> wrote: [...] > +static int phy_meson8b_usb2_reset(struct phy *phy) > +{ > + struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy); > + int ret; > + > + if (priv->is_enabled) { > + ret = reset_control_reset(priv->reset); > + if (ret) { > + dev_err(&phy->dev, "Failed to trigger USB reset\n"); > + return ret; > + } > + } > + > + return 0; > +} will this reset change the register values back to some kind of default? If it does then we're not re-applying our desired register values afterwards - which is probably asking for trouble. For shared resets (like the one we have here) reset_control_reset will only trigger the reset line once until all drivers using that reset line are unloaded. So effectively this new phy_ops.reset callback will be a no-op. [...] > - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); > + priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy"); I think this breaks compatibility with existing .dtbs and our dt-bindings (as we're not documenting a "reset-names" property). What is the goal of this one? Best regards, Martin
Hi Martin On Fri, 18 Jun 2021 at 03:54, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hi Anand, > > On Thu, Jun 17, 2021 at 9:44 PM Anand Moon <linux.amoon@gmail.com> wrote: > [...] > > +static int phy_meson8b_usb2_reset(struct phy *phy) > > +{ > > + struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy); > > + int ret; > > + > > + if (priv->is_enabled) { > > + ret = reset_control_reset(priv->reset); > > + if (ret) { > > + dev_err(&phy->dev, "Failed to trigger USB reset\n"); > > + return ret; > > + } > > + } > > + > > + return 0; > > +} > will this reset change the register values back to some kind of default? > If it does then we're not re-applying our desired register values > afterwards - which is probably asking for trouble. > > For shared resets (like the one we have here) reset_control_reset will > only trigger the reset line once until all drivers using that reset > line are unloaded. > So effectively this new phy_ops.reset callback will be a no-op. I know his register is shared between two USB IPs, but I have not observed any issues. > > [...] > > - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); > > + priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy"); > I think this breaks compatibility with existing .dtbs and our > dt-bindings (as we're not documenting a "reset-names" property). > What is the goal of this one? > OK, If we pass NULL over here there is the possibility USB phy will not get registered. > > Best regards, > Martin Thanks -Anand
Hi Anand, On Fri, Jun 18, 2021 at 5:33 PM Anand Moon <linux.amoon@gmail.com> wrote: [...] > > For shared resets (like the one we have here) reset_control_reset will > > only trigger the reset line once until all drivers using that reset > > line are unloaded. > > So effectively this new phy_ops.reset callback will be a no-op. > > I know his register is shared between two USB IPs, > but I have not observed any issues. have you checked at which point we're then actually triggering the reset? I assume that you will find that the reset is only triggered for the very first power_on/init call - which makes this patch effectively a no-op (yes, we're calling reset_control_reset then, but that doesn't mean that a reset is triggered on hardware level - see drivers/reset/core.c at around line 346). [...] > > > - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); > > > + priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy"); > > I think this breaks compatibility with existing .dtbs and our > > dt-bindings (as we're not documenting a "reset-names" property). > > What is the goal of this one? > > > > OK, If we pass NULL over here there is the possibility > USB phy will not get registered. I don't understand why - with NULL everything is working fine for me. Also no matter which name you give to the reset line (in reset-names), it will be the same reset line in all cases. If it's the same reset line before and after: why is this needed? Best regards, Martin
Hi Martin Thanks for your review comments. On Sat, 19 Jun 2021 at 01:36, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hi Anand, > > On Fri, Jun 18, 2021 at 5:33 PM Anand Moon <linux.amoon@gmail.com> wrote: > [...] > > > For shared resets (like the one we have here) reset_control_reset will > > > only trigger the reset line once until all drivers using that reset > > > line are unloaded. > > > So effectively this new phy_ops.reset callback will be a no-op. > > > > I know his register is shared between two USB IPs, > > but I have not observed any issues. > have you checked at which point we're then actually triggering the reset? > I assume that you will find that the reset is only triggered for the > very first power_on/init call - which makes this patch effectively a > no-op (yes, we're calling reset_control_reset then, but that doesn't > mean that a reset is triggered on hardware level - see > drivers/reset/core.c at around line 346). > Ok Thanks for the inputs. got your point. I was also looking into Amlogic source code for reset. (aml_cbus_update_bits) [0] https://github.com/khadas/linux/blob/khadas-vims-4.9.y/drivers/amlogic/usb/phy/phy-aml-new-usb.c is there some feature to iomap the USB with cbus? > [...] > > > > - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); > > > > + priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy"); > > > I think this breaks compatibility with existing .dtbs and our > > > dt-bindings (as we're not documenting a "reset-names" property). > > > What is the goal of this one? > > > > > > > OK, If we pass NULL over here there is the possibility > > USB phy will not get registered. > I don't understand why - with NULL everything is working fine for me. > Also no matter which name you give to the reset line (in reset-names), > it will be the same reset line in all cases. If it's the same reset > line before and after: why is this needed? > I need to investigate this reset feature. With my setup with current changes after I update the below. - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy"); + priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); if (PTR_ERR(priv->reset) == -EPROBE_DEFER) return PTR_ERR(priv->reset); Reset will break the USB initialization, see below output. [ 1.265403] dwc2 c9040000.usb: mapped PA c9040000 to VA (ptrval) [ 1.265540] dwc2 c9040000.usb: Looking up vusb_d-supply from device tree [ 1.265554] dwc2 c9040000.usb: Looking up vusb_d-supply property in node /soc/usb@c9040000 failed [ 1.265585] dwc2 c9040000.usb: supply vusb_d not found, using dummy regulator [ 1.265717] dwc2 c9040000.usb: Looking up vusb_a-supply from device tree [ 1.265730] dwc2 c9040000.usb: Looking up vusb_a-supply property in node /soc/usb@c9040000 failed [ 1.265752] dwc2 c9040000.usb: supply vusb_a not found, using dummy regulator [ 1.265812] dwc2 c9040000.usb: registering common handler for irq35 [ 1.265867] dwc2 c9040000.usb: Looking up vbus-supply from device tree [ 1.265880] dwc2 c9040000.usb: Looking up vbus-supply property in node /soc/usb@c9040000 failed [ 1.267066] dwc2 c9040000.usb: Core Release: 3.10a (snpsid=4f54310a) [ 1.270983] dwc2 c9040000.usb: dwc2_core_reset: HANG! Soft Reset timeout GRSTCTL_CSFTRST [ 1.271319] dwc2: probe of c9040000.usb failed with error -16 [ 1.273296] dwc2 c90c0000.usb: mapped PA c90c0000 to VA (ptrval) [ 1.273426] dwc2 c90c0000.usb: Looking up vusb_d-supply from device tree [ 1.273440] dwc2 c90c0000.usb: Looking up vusb_d-supply property in node /soc/usb@c90c0000 failed [ 1.273471] dwc2 c90c0000.usb: supply vusb_d not found, using dummy regulator [ 1.273607] dwc2 c90c0000.usb: Looking up vusb_a-supply from device tree [ 1.273621] dwc2 c90c0000.usb: Looking up vusb_a-supply property in node /soc/usb@c90c0000 failed [ 1.273641] dwc2 c90c0000.usb: supply vusb_a not found, using dummy regulator [ 1.273700] dwc2 c90c0000.usb: registering common handler for irq36 [ 1.273750] dwc2 c90c0000.usb: Looking up vbus-supply from device tree [ 1.273762] dwc2 c90c0000.usb: Looking up vbus-supply property in node /soc/usb@c90c0000 failed [ 1.274966] dwc2 c90c0000.usb: Core Release: 3.10a (snpsid=4f54310a) [ 1.278888] dwc2 c90c0000.usb: dwc2_core_reset: HANG! Soft Reset timeout GRSTCTL_CSFTRST [ 1.279224] dwc2: probe of c90c0000.usb failed with error -16 > > Best regards, > Martin Thanks -Anand
Hi Anand, On Mon, Jun 21, 2021 at 9:16 AM Anand Moon <linux.amoon@gmail.com> wrote: [...] > Ok Thanks for the inputs. got your point. > > I was also looking into Amlogic source code for reset. (aml_cbus_update_bits) > [0] https://github.com/khadas/linux/blob/khadas-vims-4.9.y/drivers/amlogic/usb/phy/phy-aml-new-usb.c > is there some feature to iomap the USB with cbus? for that specific code: that's what we do inside drivers/reset/reset-meson.c Amlogic's vendor kernel uses an increment of 4 bytes per value, so 0x1102 translates to 0x4408 then in mainline's meson8b.dtsi we have: compatible = "amlogic,meson8b-reset"; reg = <0x4404 0x9c>; as you can see 0x4408 is part of the reset controller node. next in include/dt-bindings/reset/amlogic,meson8b-reset.h we have: #define RESET_USB_OTG 34 the register used for reset line 34 is translated using: 0x4404 (first register) + 4 (4 * reset line / 32 = 1) = 0x4408 then the bit inside this register is translated using: reset line % 32 = 2 that's how we express aml_cbus_update_bits(0x1102, 0x1<<2, 0x1<<2); in the mainline kernel (by going through the reset subsystem) [...] > > > > > - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); > > > > > + priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy"); > > > > I think this breaks compatibility with existing .dtbs and our > > > > dt-bindings (as we're not documenting a "reset-names" property). > > > > What is the goal of this one? > > > > > > > > > > OK, If we pass NULL over here there is the possibility > > > USB phy will not get registered. > > I don't understand why - with NULL everything is working fine for me. > > Also no matter which name you give to the reset line (in reset-names), > > it will be the same reset line in all cases. If it's the same reset > > line before and after: why is this needed? > > > I need to investigate this reset feature. With my setup with current changes > after I update the below. > - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy"); > + priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); > if (PTR_ERR(priv->reset) == -EPROBE_DEFER) > return PTR_ERR(priv->reset); > > Reset will break the USB initialization, see below output. interesting, I have not seen that USB problem before and neither is Kernel CI seeing it: [0] Is it only happening with this patch or did you also see it before? Best regards, Martin [0] https://storage.staging.kernelci.org/next/master/next-20210617/arm/multi_v7_defconfig+ltp-ima/gcc-8/lab-baylibre/baseline-meson8b-odroidc1.html
Hi Martin, On Wed, 23 Jun 2021 at 01:42, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hi Anand, > > On Mon, Jun 21, 2021 at 9:16 AM Anand Moon <linux.amoon@gmail.com> wrote: > [...] > > Ok Thanks for the inputs. got your point. > > > > I was also looking into Amlogic source code for reset. (aml_cbus_update_bits) > > [0] https://github.com/khadas/linux/blob/khadas-vims-4.9.y/drivers/amlogic/usb/phy/phy-aml-new-usb.c > > is there some feature to iomap the USB with cbus? > for that specific code: that's what we do inside drivers/reset/reset-meson.c > Amlogic's vendor kernel uses an increment of 4 bytes per value, so > 0x1102 translates to 0x4408 > > then in mainline's meson8b.dtsi we have: > compatible = "amlogic,meson8b-reset"; > reg = <0x4404 0x9c>; > as you can see 0x4408 is part of the reset controller node. > > next in include/dt-bindings/reset/amlogic,meson8b-reset.h we have: > #define RESET_USB_OTG 34 > > the register used for reset line 34 is translated using: > 0x4404 (first register) + 4 (4 * reset line / 32 = 1) = 0x4408 > then the bit inside this register is translated using: > reset line % 32 = 2 > > that's how we express aml_cbus_update_bits(0x1102, 0x1<<2, 0x1<<2); in > the mainline kernel (by going through the reset subsystem) > Thank you very much for clearing my long-standing doubt on *reset logic* on Amlogic SoC. > [...] > > > > > > - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); > > > > > > + priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy"); > > > > > I think this breaks compatibility with existing .dtbs and our > > > > > dt-bindings (as we're not documenting a "reset-names" property). > > > > > What is the goal of this one? > > > > > > > > > > > > > OK, If we pass NULL over here there is the possibility > > > > USB phy will not get registered. > > > I don't understand why - with NULL everything is working fine for me. > > > Also no matter which name you give to the reset line (in reset-names), > > > it will be the same reset line in all cases. If it's the same reset > > > line before and after: why is this needed? > > > > > I need to investigate this reset feature. With my setup with current changes > > after I update the below. > > - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy"); > > + priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); > > if (PTR_ERR(priv->reset) == -EPROBE_DEFER) > > return PTR_ERR(priv->reset); > > > > Reset will break the USB initialization, see below output. > interesting, I have not seen that USB problem before and neither is > Kernel CI seeing it: [0] > Is it only happening with this patch or did you also see it before? > Yes, it could happen with this patch but It could be also linked to reorder the phy configuration. See below logs, when core reset fails on USB PHY no USB is getting registered. [ 1.267620] dwc2 c9040000.usb: mapped PA c9040000 to VA (ptrval) [ 1.267768] dwc2 c9040000.usb: Looking up vusb_d-supply from device tree [ 1.267783] dwc2 c9040000.usb: Looking up vusb_d-supply property in node /soc/usb@c9040000 failed [ 1.267814] dwc2 c9040000.usb: supply vusb_d not found, using dummy regulator [ 1.267940] dwc2 c9040000.usb: Looking up vusb_a-supply from device tree [ 1.267954] dwc2 c9040000.usb: Looking up vusb_a-supply property in node /soc/usb@c9040000 failed [ 1.267975] dwc2 c9040000.usb: supply vusb_a not found, using dummy regulator [ 1.268037] dwc2 c9040000.usb: registering common handler for irq35 [ 1.268090] dwc2 c9040000.usb: Looking up vbus-supply from device tree [ 1.268102] dwc2 c9040000.usb: Looking up vbus-supply property in node /soc/usb@c9040000 failed [ 1.269267] dwc2 c9040000.usb: Core Release: 3.10a (snpsid=4f54310a) [ 1.273185] dwc2 c9040000.usb: dwc2_core_reset: HANG! Soft Reset timeout GRSTCTL_CSFTRST [ 1.273510] dwc2: probe of c9040000.usb failed with error -16 [ 1.275474] dwc2 c90c0000.usb: mapped PA c90c0000 to VA (ptrval) [ 1.275603] dwc2 c90c0000.usb: Looking up vusb_d-supply from device tree [ 1.275617] dwc2 c90c0000.usb: Looking up vusb_d-supply property in node /soc/usb@c90c0000 failed [ 1.275646] dwc2 c90c0000.usb: supply vusb_d not found, using dummy regulator [ 1.275784] dwc2 c90c0000.usb: Looking up vusb_a-supply from device tree [ 1.275798] dwc2 c90c0000.usb: Looking up vusb_a-supply property in node /soc/usb@c90c0000 failed [ 1.275819] dwc2 c90c0000.usb: supply vusb_a not found, using dummy regulator [ 1.275877] dwc2 c90c0000.usb: registering common handler for irq36 [ 1.275930] dwc2 c90c0000.usb: Looking up vbus-supply from device tree [ 1.275942] dwc2 c90c0000.usb: Looking up vbus-supply property in node /soc/usb@c90c0000 failed [ 1.277125] dwc2 c90c0000.usb: Core Release: 3.10a (snpsid=4f54310a) [ 1.281042] dwc2 c90c0000.usb: dwc2_core_reset: HANG! Soft Reset timeout GRSTCTL_CSFTRST [ 1.281353] dwc2: probe of c90c0000.usb failed with error -16 > > Best regards, > Martin > > > [0] https://storage.staging.kernelci.org/next/master/next-20210617/arm/multi_v7_defconfig+ltp-ima/gcc-8/lab-baylibre/baseline-meson8b-odroidc1.html
Hi Martin, On Thu, 24 Jun 2021 at 20:24, Anand Moon <linux.amoon@gmail.com> wrote: > > Hi Martin, > > On Wed, 23 Jun 2021 at 01:42, Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: > > > > Hi Anand, > > > > On Mon, Jun 21, 2021 at 9:16 AM Anand Moon <linux.amoon@gmail.com> wrote: > > [...] > > > Ok Thanks for the inputs. got your point. > > > > > > I was also looking into Amlogic source code for reset. (aml_cbus_update_bits) > > > [0] https://github.com/khadas/linux/blob/khadas-vims-4.9.y/drivers/amlogic/usb/phy/phy-aml-new-usb.c > > > is there some feature to iomap the USB with cbus? > > for that specific code: that's what we do inside drivers/reset/reset-meson.c > > Amlogic's vendor kernel uses an increment of 4 bytes per value, so > > 0x1102 translates to 0x4408 > > > > then in mainline's meson8b.dtsi we have: > > compatible = "amlogic,meson8b-reset"; > > reg = <0x4404 0x9c>; > > as you can see 0x4408 is part of the reset controller node. > > > > next in include/dt-bindings/reset/amlogic,meson8b-reset.h we have: > > #define RESET_USB_OTG 34 > > > > the register used for reset line 34 is translated using: > > 0x4404 (first register) + 4 (4 * reset line / 32 = 1) = 0x4408 > > then the bit inside this register is translated using: > > reset line % 32 = 2 > > > > that's how we express aml_cbus_update_bits(0x1102, 0x1<<2, 0x1<<2); in > > the mainline kernel (by going through the reset subsystem) > > > > Thank you very much for clearing my long-standing doubt on *reset > logic* on Amlogic SoC. > > > [...] > > > > > > > - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); > > > > > > > + priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy"); > > > > > > I think this breaks compatibility with existing .dtbs and our > > > > > > dt-bindings (as we're not documenting a "reset-names" property). > > > > > > What is the goal of this one? > > > > > > > > > > > > > > > > OK, If we pass NULL over here there is the possibility > > > > > USB phy will not get registered. > > > > I don't understand why - with NULL everything is working fine for me. > > > > Also no matter which name you give to the reset line (in reset-names), > > > > it will be the same reset line in all cases. If it's the same reset > > > > line before and after: why is this needed? > > > > > > > I need to investigate this reset feature. With my setup with current changes > > > after I update the below. > > > - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy"); > > > + priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); > > > if (PTR_ERR(priv->reset) == -EPROBE_DEFER) > > > return PTR_ERR(priv->reset); > > > > > > Reset will break the USB initialization, see below output. > > interesting, I have not seen that USB problem before and neither is > > Kernel CI seeing it: [0] > > Is it only happening with this patch or did you also see it before? > > > Yes, it could happen with this patch but It could be also linked to > reorder the phy configuration. > See below logs, when core reset fails on USB PHY no USB is getting registered. > > [ 1.267620] dwc2 c9040000.usb: mapped PA c9040000 to VA (ptrval) > [ 1.267768] dwc2 c9040000.usb: Looking up vusb_d-supply from device tree > [ 1.267783] dwc2 c9040000.usb: Looking up vusb_d-supply property in > node /soc/usb@c9040000 failed > [ 1.267814] dwc2 c9040000.usb: supply vusb_d not found, using dummy regulator > [ 1.267940] dwc2 c9040000.usb: Looking up vusb_a-supply from device tree > [ 1.267954] dwc2 c9040000.usb: Looking up vusb_a-supply property in > node /soc/usb@c9040000 failed > [ 1.267975] dwc2 c9040000.usb: supply vusb_a not found, using dummy regulator > [ 1.268037] dwc2 c9040000.usb: registering common handler for irq35 > [ 1.268090] dwc2 c9040000.usb: Looking up vbus-supply from device tree > [ 1.268102] dwc2 c9040000.usb: Looking up vbus-supply property in > node /soc/usb@c9040000 failed > [ 1.269267] dwc2 c9040000.usb: Core Release: 3.10a (snpsid=4f54310a) > [ 1.273185] dwc2 c9040000.usb: dwc2_core_reset: HANG! Soft Reset > timeout GRSTCTL_CSFTRST > [ 1.273510] dwc2: probe of c9040000.usb failed with error -16 > [ 1.275474] dwc2 c90c0000.usb: mapped PA c90c0000 to VA (ptrval) > [ 1.275603] dwc2 c90c0000.usb: Looking up vusb_d-supply from device tree > [ 1.275617] dwc2 c90c0000.usb: Looking up vusb_d-supply property in > node /soc/usb@c90c0000 failed > [ 1.275646] dwc2 c90c0000.usb: supply vusb_d not found, using dummy regulator > [ 1.275784] dwc2 c90c0000.usb: Looking up vusb_a-supply from device tree > [ 1.275798] dwc2 c90c0000.usb: Looking up vusb_a-supply property in > node /soc/usb@c90c0000 failed > [ 1.275819] dwc2 c90c0000.usb: supply vusb_a not found, using dummy regulator > [ 1.275877] dwc2 c90c0000.usb: registering common handler for irq36 > [ 1.275930] dwc2 c90c0000.usb: Looking up vbus-supply from device tree > [ 1.275942] dwc2 c90c0000.usb: Looking up vbus-supply property in > node /soc/usb@c90c0000 failed > [ 1.277125] dwc2 c90c0000.usb: Core Release: 3.10a (snpsid=4f54310a) > [ 1.281042] dwc2 c90c0000.usb: dwc2_core_reset: HANG! Soft Reset > timeout GRSTCTL_CSFTRST > [ 1.281353] dwc2: probe of c90c0000.usb failed with error -16 > Sorry for the delay. We could switch the reset logic to *devm_reset_control_get_optional_exclusive* as below to fix the reset line, since both the dwc2 c90c0000.usb and c9040000.usb will have their own context to reset control register, it means the reset line is not share between two USB PHY nodes. - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); + priv->reset = devm_reset_control_get_optional_exclusive(&pdev->dev, + "reset"); > > > > Best regards, > > Martin > > > > > > [0] https://storage.staging.kernelci.org/next/master/next-20210617/arm/multi_v7_defconfig+ltp-ima/gcc-8/lab-baylibre/baseline-meson8b-odroidc1.html Thanks -Anand
Hi Anand, On Sun, Jun 27, 2021 at 10:07 PM Anand Moon <linux.amoon@gmail.com> wrote: [...] > Sorry for the delay. > We could switch the reset logic to > *devm_reset_control_get_optional_exclusive* as below > to fix the reset line, since both the dwc2 c90c0000.usb and c9040000.usb > will have their own context to reset control register, it means the > reset line is not share > between two USB PHY nodes. This is something I don't understand. As discussed in our previous mails reset_control_reset in case of the USB PHY driver (which uses the RESET_USB_OTG reset line for *both* PHYs) is equivalent to the following code in the vendor kernel: aml_cbus_update_bits(0x1102, 0x1<<2, 0x1<<2) We have two PHYs but only one reset line. So in my own words I describe the reset line as being shared. > > - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); > + priv->reset = devm_reset_control_get_optional_exclusive(&pdev->dev, > + "reset"); Have you boot-tested this? Without any .dts changes this will return NULL because there's no reset-names = "reset"; in the .dts(i). If you replace "reset" with NULL then I assume that the second PHY will fail to obtain the reset line because it's shared between two devices but we're trying to obtain it exclusively for both (PHYs). Best regards, Martin
Hi Martin, On Mon, 28 Jun 2021 at 01:55, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hi Anand, > > On Sun, Jun 27, 2021 at 10:07 PM Anand Moon <linux.amoon@gmail.com> wrote: > [...] > > Sorry for the delay. > > We could switch the reset logic to > > *devm_reset_control_get_optional_exclusive* as below > > to fix the reset line, since both the dwc2 c90c0000.usb and c9040000.usb > > will have their own context to reset control register, it means the > > reset line is not share > > between two USB PHY nodes. > This is something I don't understand. > As discussed in our previous mails reset_control_reset in case of the > USB PHY driver (which uses the RESET_USB_OTG reset line for *both* > PHYs) is equivalent to the following code in the vendor kernel: > aml_cbus_update_bits(0x1102, 0x1<<2, 0x1<<2) > > We have two PHYs but only one reset line. So in my own words I > describe the reset line as being shared. > > > > > - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); > > + priv->reset = devm_reset_control_get_optional_exclusive(&pdev->dev, > > + "reset"); > Have you boot-tested this? > Without any .dts changes this will return NULL because there's no > reset-names = "reset"; in the .dts(i). > If you replace "reset" with NULL then I assume that the second PHY > will fail to obtain the reset line because it's shared between two > devices but we're trying to obtain it exclusively for both (PHYs). > Thanks for your review comments. I have always tested with both the phy enable and with proper DTS changes. Yes, it gives false-positive results while initialization of the USB PHY. Odroid C2 it will pass but on Odroid C1 it will fail kid off. But it seems to me that the order of the PHY reset is kind of a problem. Thanks for looking into my changes. > > Best regards, > Martin Thanks -Anand
diff --git a/drivers/phy/amlogic/phy-meson8b-usb2.c b/drivers/phy/amlogic/phy-meson8b-usb2.c index ab23a584d7b7..c1ed2e5c80d8 100644 --- a/drivers/phy/amlogic/phy-meson8b-usb2.c +++ b/drivers/phy/amlogic/phy-meson8b-usb2.c @@ -133,6 +133,7 @@ struct phy_meson8b_usb2_priv { struct clk_bulk_data *clks; struct reset_control *reset; const struct phy_meson8b_usb2_match_data *match; + int is_enabled; }; static const struct regmap_config phy_meson8b_usb2_regmap_conf = { @@ -147,14 +148,6 @@ static int phy_meson8b_usb2_init(struct phy *phy) struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy); int ret; - if (!IS_ERR_OR_NULL(priv->reset)) { - ret = reset_control_reset(priv->reset); - if (ret) { - dev_err(&phy->dev, "Failed to trigger USB reset\n"); - return ret; - } - } - ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks); if (ret) { dev_err(&phy->dev, "Failed to enable USB clock\n"); @@ -173,6 +166,22 @@ static int phy_meson8b_usb2_exit(struct phy *phy) return 0; } +static int phy_meson8b_usb2_reset(struct phy *phy) +{ + struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy); + int ret; + + if (priv->is_enabled) { + ret = reset_control_reset(priv->reset); + if (ret) { + dev_err(&phy->dev, "Failed to trigger USB reset\n"); + return ret; + } + } + + return 0; +} + static int phy_meson8b_usb2_setmode(struct phy *phy, enum phy_mode mode, int submode) { @@ -200,6 +209,8 @@ static int phy_meson8b_usb2_setmode(struct phy *phy, enum phy_mode mode, return -EINVAL; } + phy_meson8b_usb2_reset(phy); + priv->dr_mode = mode; return 0; @@ -209,6 +220,8 @@ static int phy_meson8b_usb2_power_off(struct phy *phy) { struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy); + priv->is_enabled = 0; + if (priv->dr_mode == USB_DR_MODE_HOST) regmap_update_bits(priv->regmap, REG_DBG_UART, REG_DBG_UART_SET_IDDQ, @@ -221,6 +234,8 @@ static int phy_meson8b_usb2_power_on(struct phy *phy) struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy); int ret; + priv->is_enabled = 1; + regmap_update_bits(priv->regmap, REG_CONFIG, REG_CONFIG_CLK_32k_ALTSEL, REG_CONFIG_CLK_32k_ALTSEL); @@ -229,7 +244,6 @@ static int phy_meson8b_usb2_power_on(struct phy *phy) regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_FSEL_MASK, 0x5 << REG_CTRL_FSEL_SHIFT); - /* reset the PHY */ regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET, REG_CTRL_POWER_ON_RESET); @@ -257,6 +271,7 @@ static const struct phy_ops phy_meson8b_usb2_ops = { .power_on = phy_meson8b_usb2_power_on, .power_off = phy_meson8b_usb2_power_off, .set_mode = phy_meson8b_usb2_setmode, + .reset = phy_meson8b_usb2_reset, .owner = THIS_MODULE, }; @@ -301,7 +316,7 @@ static int phy_meson8b_usb2_probe(struct platform_device *pdev) if (ret) return ret; - priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); + priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy"); if (PTR_ERR(priv->reset) == -EPROBE_DEFER) return PTR_ERR(priv->reset);
Reoder the code for phy reset mode in .reset callback function. Reset control is shared between two phy so use the phy name as shared id. Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Signed-off-by: Anand Moon <linux.amoon@gmail.com> --- drivers/phy/amlogic/phy-meson8b-usb2.c | 35 ++++++++++++++++++-------- 1 file changed, 25 insertions(+), 10 deletions(-)