RE: [PATCH 2/3] phy: renesas: rcar-gen3-usb2: Add support for R7S9210
Hi Shimodaさん From: Yoshihiro Shimoda Sent: Monday, November 19, 2018 3:58 AM > And I read Figure 32.1 of the RZ/A2 documentation and I wonder if we need > to release > USBCTR.PLL_RST even if we use USB peripheral mode. I will ask the RZ/A2 design team to confirm. If this setting is required, I think we should use your suggestion: From: Yoshihiro Shimoda Sent: Thursday, November 15, 2018 4:20 AM > So, I have an idea to set the default mode by using "dr_mode" property, > instead of hardcoded. Since the driver already has such a function, > we can reuse rcar_gen3_device_recognition() to set the default value. > To achieve that, we need to modify the following though. > - Don't enable "is_otg_channel". > - Don't call rcar_gen3_enable_vbus_ctrl() to avoid ADPCTRL register >because RZ/A2 doesn't have it. > - Don't need to call rcar_gen3_set_linectrl to avoid LINECTRL1 register >because RZ/A2 doesn't seem to need the setting on host mode. Chris
RE: [PATCH 2/3] phy: renesas: rcar-gen3-usb2: Add support for R7S9210
Hi Chris-san, > From: Chris Brandt, Sent: Thursday, November 15, 2018 9:34 PM > > Hi Shimodaさん > > > From: Yoshihiro Shimoda > > Sent: Thursday, November 15, 2018 4:20 AM > > > > Host does NOT work: > > > //else > > > // /* No otg, so default to host mode */ > > > // writel(0x, usb2_base + USB2_COMMCTRL); > > > > I got it. However, I have a concern how to set the mode to peripheral on > > RZ/A2 > > if we applied this code. If someone would like to use the USB as > > peripheral > > on his board, this code is not suitable. > > But USB peripheral is a different driver. So, this code will not run. So > USB2_COMMCTRL will keep the default value 0x8000. > > Correct? Ah, this is you're correct. R-Car Gen3 code (drivers/usb/renesas_usbhs/rcar3.c) doesn't have phy control for now. > > So, I have an idea to set the default mode by using "dr_mode" property, > > instead of hardcoded. Since the driver already has such a function, > > we can reuse rcar_gen3_device_recognition() to set the default value. > > To achieve that, we need to modify the following though. > > - Don't enable "is_otg_channel". > > - Don't call rcar_gen3_enable_vbus_ctrl() to avoid ADPCTRL register > >because RZ/A2 doesn't have it. > > - Don't need to call rcar_gen3_set_linectrl to avoid LINECTRL1 register > >because RZ/A2 doesn't seem to need the setting on host mode. > > > > What do you think? > > If a board is designed for USB peripheral, why would they enable a EHCI > host driver for the same USB channel? > I am confused. I meant that USB peripheral of RZ/A2 need the phy driver, not a EHCI host driver. And I read Figure 32.1 of the RZ/A2 documentation and I wonder if we need to release USBCTR.PLL_RST even if we use USB peripheral mode. # Since the documentations doesn't mention the PHY area exactly, it's confusable for us though. # But, the phy driver assumed the driver handled "AHB Bridge", "Core" and "UCOM" registers. > > > > (In other words, if we use the port as peripheral with the reset value > > > > 0x8000, does it work?) > > > > > > We have not been able to get USB peripheral working on RZ/A2 yet. > > > For peripheral, RZ/A2 has HS-USB. > > > After plugging into the PC, HS-USB goes to Suspended state (DVSQ = > > "0110"). > > > It should go to Configured state (DVSQ = "0011") > > > > I guess we need to modify ./drivers/usb/renesas_usbhs/rza.c for RZ/A2 > > because RZ/A2's HS-USB has SYSCFG.CNEN, but doesn't have SYSCFG.UPLLE? > > Today for RZ/A1, we tell people to use USB0 first in their board design, > then use USB1 if they need a second USB channel. USB pins are dedicated > (no other function) so there should be no design conflicts. > > If only USB1 is used, USB0 must also be enabled in DT as a dummy driver > (so SYSCFG.UPLLE can get set for USB1). Oh, I heard RZ/G1C also has such a hardware register [1]. [1] https://patchwork.kernel.org/patch/10655855/ RZ/G1C channel 0 only has UGCTRL register, but need to set for channel 1. Best regards, Yoshihiro Shimoda > > > According to the RZ/A2 Hardware Manual, COMMCTRL should be 0x8000 > > when > > > using HS-USB. > > > > > > There are 2 channels of USB on RZ/A2 (host x 2, HS_USB x 2) > > > > I got it. So, I guess someone wants to use 1 host and 1 peripheral :) > > Yes. > > Chris
RE: [PATCH 2/3] phy: renesas: rcar-gen3-usb2: Add support for R7S9210
Hi Shimodaさん > From: Yoshihiro Shimoda > Sent: Thursday, November 15, 2018 4:20 AM > > Host does NOT work: > > //else > > // /* No otg, so default to host mode */ > > // writel(0x, usb2_base + USB2_COMMCTRL); > > I got it. However, I have a concern how to set the mode to peripheral on > RZ/A2 > if we applied this code. If someone would like to use the USB as > peripheral > on his board, this code is not suitable. But USB peripheral is a different driver. So, this code will not run. So USB2_COMMCTRL will keep the default value 0x8000. Correct? > So, I have an idea to set the default mode by using "dr_mode" property, > instead of hardcoded. Since the driver already has such a function, > we can reuse rcar_gen3_device_recognition() to set the default value. > To achieve that, we need to modify the following though. > - Don't enable "is_otg_channel". > - Don't call rcar_gen3_enable_vbus_ctrl() to avoid ADPCTRL register >because RZ/A2 doesn't have it. > - Don't need to call rcar_gen3_set_linectrl to avoid LINECTRL1 register >because RZ/A2 doesn't seem to need the setting on host mode. > > What do you think? If a board is designed for USB peripheral, why would they enable a EHCI host driver for the same USB channel? I am confused. > > > (In other words, if we use the port as peripheral with the reset value > > > 0x8000, does it work?) > > > > We have not been able to get USB peripheral working on RZ/A2 yet. > > For peripheral, RZ/A2 has HS-USB. > > After plugging into the PC, HS-USB goes to Suspended state (DVSQ = > "0110"). > > It should go to Configured state (DVSQ = "0011") > > I guess we need to modify ./drivers/usb/renesas_usbhs/rza.c for RZ/A2 > because RZ/A2's HS-USB has SYSCFG.CNEN, but doesn't have SYSCFG.UPLLE? Today for RZ/A1, we tell people to use USB0 first in their board design, then use USB1 if they need a second USB channel. USB pins are dedicated (no other function) so there should be no design conflicts. If only USB1 is used, USB0 must also be enabled in DT as a dummy driver (so SYSCFG.UPLLE can get set for USB1). > > According to the RZ/A2 Hardware Manual, COMMCTRL should be 0x8000 > when > > using HS-USB. > > > > There are 2 channels of USB on RZ/A2 (host x 2, HS_USB x 2) > > I got it. So, I guess someone wants to use 1 host and 1 peripheral :) Yes. Chris
RE: [PATCH 2/3] phy: renesas: rcar-gen3-usb2: Add support for R7S9210
Hi Chris-san, > From: Chris Brandt, Sent: Wednesday, November 14, 2018 10:03 PM > > Hi Shimoda-san, > > > From: Yoshihiro Shimoda > > Sent: Wednesday, November 14, 2018 7:24 AM > > > > > config PHY_RCAR_GEN3_USB2 > > > > > tristate "Renesas R-Car generation 3 USB 2.0 PHY driver" > > > > > depends on ARCH_RENESAS > > > > > - depends on EXTCON > > > > > + depends on EXTCON || ARCH_R7S9210 > > > > > > > > Does this mean that you don't want to use EXTCON if ARCH_R7S9210=y? > > > > > > EXTCON is not required for RZ/A2. So, I want to be able to leave EXTCON > > > un-selected (save flash space). > > > > I got it. > > I added the depend on EXTCON, but R-Car Gen3 environment can build > > EXTCON=n. > > However, I realized that build error happens if EXTCON=m. > > So, I think we have to revise this line as following at first: > > > > "depends on EXTCON || !EXTCON # if EXTCON=m, this cannot be built-in" > > like drivers/phy/qualcomm/Kconfig. > > OK. I will change it. > >(should this be a separate patch?) Yes, this should be a separate patch. > > > Without this code, RZ/A2 will not work. > > > > I'd like to clarify this. Does this mean RZ/A2 will not work as host mode? > > Correct. If I remove 'else' code, RZ/A2 host mode does not work. > I just tested again now. > > Host works: > else > /* No otg, so default to host mode */ > writel(0x, usb2_base + USB2_COMMCTRL); > > Host does NOT work: > //else > // /* No otg, so default to host mode */ > // writel(0x, usb2_base + USB2_COMMCTRL); I got it. However, I have a concern how to set the mode to peripheral on RZ/A2 if we applied this code. If someone would like to use the USB as peripheral on his board, this code is not suitable. So, I have an idea to set the default mode by using "dr_mode" property, instead of hardcoded. Since the driver already has such a function, we can reuse rcar_gen3_device_recognition() to set the default value. To achieve that, we need to modify the following though. - Don't enable "is_otg_channel". - Don't call rcar_gen3_enable_vbus_ctrl() to avoid ADPCTRL register because RZ/A2 doesn't have it. - Don't need to call rcar_gen3_set_linectrl to avoid LINECTRL1 register because RZ/A2 doesn't seem to need the setting on host mode. What do you think? > > (In other words, if we use the port as peripheral with the reset value > > 0x8000, does it work?) > > We have not been able to get USB peripheral working on RZ/A2 yet. > For peripheral, RZ/A2 has HS-USB. > After plugging into the PC, HS-USB goes to Suspended state (DVSQ = "0110"). > It should go to Configured state (DVSQ = "0011") I guess we need to modify ./drivers/usb/renesas_usbhs/rza.c for RZ/A2 because RZ/A2's HS-USB has SYSCFG.CNEN, but doesn't have SYSCFG.UPLLE? > According to the RZ/A2 Hardware Manual, COMMCTRL should be 0x8000 when > using HS-USB. > > There are 2 channels of USB on RZ/A2 (host x 2, HS_USB x 2) I got it. So, I guess someone wants to use 1 host and 1 peripheral :) Best regards, Yoshihiro Shimoda > Chris
RE: [PATCH 2/3] phy: renesas: rcar-gen3-usb2: Add support for R7S9210
Hi Shimoda-san, > From: Yoshihiro Shimoda > Sent: Wednesday, November 14, 2018 7:24 AM > > > > config PHY_RCAR_GEN3_USB2 > > > > tristate "Renesas R-Car generation 3 USB 2.0 PHY driver" > > > > depends on ARCH_RENESAS > > > > - depends on EXTCON > > > > + depends on EXTCON || ARCH_R7S9210 > > > > > > Does this mean that you don't want to use EXTCON if ARCH_R7S9210=y? > > > > EXTCON is not required for RZ/A2. So, I want to be able to leave EXTCON > > un-selected (save flash space). > > I got it. > I added the depend on EXTCON, but R-Car Gen3 environment can build > EXTCON=n. > However, I realized that build error happens if EXTCON=m. > So, I think we have to revise this line as following at first: > > "depends on EXTCON || !EXTCON # if EXTCON=m, this cannot be built-in" > like drivers/phy/qualcomm/Kconfig. OK. I will change it. (should this be a separate patch?) > > Without this code, RZ/A2 will not work. > > I'd like to clarify this. Does this mean RZ/A2 will not work as host mode? Correct. If I remove 'else' code, RZ/A2 host mode does not work. I just tested again now. Host works: else /* No otg, so default to host mode */ writel(0x, usb2_base + USB2_COMMCTRL); Host does NOT work: //else // /* No otg, so default to host mode */ // writel(0x, usb2_base + USB2_COMMCTRL); > (In other words, if we use the port as peripheral with the reset value > 0x8000, does it work?) We have not been able to get USB peripheral working on RZ/A2 yet. For peripheral, RZ/A2 has HS-USB. After plugging into the PC, HS-USB goes to Suspended state (DVSQ = "0110"). It should go to Configured state (DVSQ = "0011") According to the RZ/A2 Hardware Manual, COMMCTRL should be 0x8000 when using HS-USB. There are 2 channels of USB on RZ/A2 (host x 2, HS_USB x 2) Chris
RE: [PATCH 2/3] phy: renesas: rcar-gen3-usb2: Add support for R7S9210
Hi Chris-san, > From: Chris Brandt, Sent: Wednesday, November 14, 2018 8:18 PM > > Hi Shimoda-san, > > > From: Yoshihiro Shimoda > > Sent: Wednesday, November 14, 2018 5:50 AM > > > config PHY_RCAR_GEN3_USB2 > > > tristate "Renesas R-Car generation 3 USB 2.0 PHY driver" > > > depends on ARCH_RENESAS > > > - depends on EXTCON > > > + depends on EXTCON || ARCH_R7S9210 > > > > Does this mean that you don't want to use EXTCON if ARCH_R7S9210=y? > > EXTCON is not required for RZ/A2. So, I want to be able to leave EXTCON > un-selected (save flash space). I got it. I added the depend on EXTCON, but R-Car Gen3 environment can build EXTCON=n. However, I realized that build error happens if EXTCON=m. So, I think we have to revise this line as following at first: "depends on EXTCON || !EXTCON # if EXTCON=m, this cannot be built-in" like drivers/phy/qualcomm/Kconfig. > > > /* Initialize otg part */ > > > if (channel->is_otg_channel) > > > rcar_gen3_init_otg(channel); > > > + else > > > + /* No otg, so default to host mode */ > > > + writel(0x, usb2_base + USB2_COMMCTRL); > > > > This "else" code will run on R-Car H3 USB port1(host only) for instance. > > I think adding this code is possible, but I'd like to separate patch from > > this adding R7S9210 support. What do you think? > > This code is required for RZ/A2. > The reason is the reset value for COMMCTRL in RZ/A2 is 0x8000 (Peripheral > mode). This value is the same as all R-Car Gen3 hardware. However, host only ports don't seem to use this value... > The reset value in R-Car H3 is 0x (Host mode). No. R-Car H3's reset value is also 0x8000 (Peripheral mode). # Note that R-Car H3 can select the mode by HS-USB / UGCTRL2 register though... > Without this code, RZ/A2 will not work. I'd like to clarify this. Does this mean RZ/A2 will not work as host mode? (In other words, if we use the port as peripheral with the reset value 0x8000, does it work?) Best regards, Yoshihiro Shimoda > > Chris
RE: [PATCH 2/3] phy: renesas: rcar-gen3-usb2: Add support for R7S9210
Hi Shimoda-san, > From: Yoshihiro Shimoda > Sent: Wednesday, November 14, 2018 5:50 AM > > config PHY_RCAR_GEN3_USB2 > > tristate "Renesas R-Car generation 3 USB 2.0 PHY driver" > > depends on ARCH_RENESAS > > - depends on EXTCON > > + depends on EXTCON || ARCH_R7S9210 > > Does this mean that you don't want to use EXTCON if ARCH_R7S9210=y? EXTCON is not required for RZ/A2. So, I want to be able to leave EXTCON un-selected (save flash space). > > /* Initialize otg part */ > > if (channel->is_otg_channel) > > rcar_gen3_init_otg(channel); > > + else > > + /* No otg, so default to host mode */ > > + writel(0x, usb2_base + USB2_COMMCTRL); > > This "else" code will run on R-Car H3 USB port1(host only) for instance. > I think adding this code is possible, but I'd like to separate patch from > this adding R7S9210 support. What do you think? This code is required for RZ/A2. The reason is the reset value for COMMCTRL in RZ/A2 is 0x8000 (Peripheral mode). The reset value in R-Car H3 is 0x (Host mode). Without this code, RZ/A2 will not work. Chris
RE: [PATCH 2/3] phy: renesas: rcar-gen3-usb2: Add support for R7S9210
Hi Chris-san,
Thank you for the patch!
> From: Chris Brandt, Sent: Thursday, November 8, 2018 2:36 AM
>
> The RZ/A2 has the same USB2 host controller as R-Car Gen3 with only some
> minor differences.
>
> Signed-off-by: Chris Brandt
> ---
> drivers/phy/renesas/Kconfig | 2 +-
> drivers/phy/renesas/phy-rcar-gen3-usb2.c | 12
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/phy/renesas/Kconfig b/drivers/phy/renesas/Kconfig
> index e340a925bbb1..beebeba31e84 100644
> --- a/drivers/phy/renesas/Kconfig
> +++ b/drivers/phy/renesas/Kconfig
> @@ -19,7 +19,7 @@ config PHY_RCAR_GEN3_PCIE
> config PHY_RCAR_GEN3_USB2
> tristate "Renesas R-Car generation 3 USB 2.0 PHY driver"
> depends on ARCH_RENESAS
> - depends on EXTCON
> + depends on EXTCON || ARCH_R7S9210
Does this mean that you don't want to use EXTCON if ARCH_R7S9210=y?
> depends on USB_SUPPORT
> select GENERIC_PHY
> select USB_COMMON
> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index d0f412c25981..96ac75ba40ea 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -34,6 +34,7 @@
> #define USB2_VBCTRL 0x60c
> #define USB2_LINECTRL1 0x610
> #define USB2_ADPCTRL 0x630
> +#define USB2_PHYCLK_CTRL 0x644
>
> /* INT_ENABLE */
> #define USB2_INT_ENABLE_UCOM_INTEN BIT(3)
> @@ -88,6 +89,7 @@ struct rcar_gen3_chan {
> bool extcon_host;
> bool is_otg_channel;
> bool uses_otg_pins;
> + bool uses_usb_x1;
> };
>
> /*
> @@ -326,6 +328,9 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
> struct rcar_gen3_chan *channel = phy_get_drvdata(p);
> void __iomem *usb2_base = channel->base;
>
> + if (channel->uses_usb_x1)
> + writel(0x0001, usb2_base + USB2_PHYCLK_CTRL);
> +
> /* Initialize USB2 part */
> writel(USB2_INT_ENABLE_INIT, usb2_base + USB2_INT_ENABLE);
> writel(USB2_SPD_RSM_TIMSET_INIT, usb2_base + USB2_SPD_RSM_TIMSET);
> @@ -334,6 +339,9 @@ static int rcar_gen3_phy_usb2_init(struct phy *p)
> /* Initialize otg part */
> if (channel->is_otg_channel)
> rcar_gen3_init_otg(channel);
> + else
> + /* No otg, so default to host mode */
> + writel(0x, usb2_base + USB2_COMMCTRL);
This "else" code will run on R-Car H3 USB port1(host only) for instance.
I think adding this code is possible, but I'd like to separate patch from
this adding R7S9210 support. What do you think?
> return 0;
> }
> @@ -406,6 +414,7 @@ static irqreturn_t rcar_gen3_phy_usb2_irq(int irq, void
> *_ch)
> }
>
> static const struct of_device_id rcar_gen3_phy_usb2_match_table[] = {
> + { .compatible = "renesas,usb2-phy-r7s9210" },
According to the PATCH 3/3, R7S9210 will use "renesas,rcar-gen3-usb2-phy".
So, you can remove this line.
> { .compatible = "renesas,usb2-phy-r8a7795" },
> { .compatible = "renesas,usb2-phy-r8a7796" },
> { .compatible = "renesas,usb2-phy-r8a77965" },
> @@ -471,6 +480,9 @@ static int rcar_gen3_phy_usb2_probe(struct
> platform_device *pdev)
> }
> }
>
> + if (of_property_read_bool(dev->of_node, "renesas,uses_usb_x1"))
> + channel->uses_usb_x1 = true;
> +
I'll reply the PATCH 3/3 though, we have to describe the new property into the
dt-bindings doc.
Best regards,
Yoshihiro Shimoda
> /*
>* devm_phy_create() will call pm_runtime_enable(>dev);
>* And then, phy-core will manage runtime pm for this device.
> --
> 2.16.1
