Re: [PATCH 1/4] phy-sun4i-usb: Fix sun8i_r40_cfg

Re: [PATCH 1/4] phy-sun4i-usb: Fix sun8i_r40_cfg

[Andre Przywara]

On Wed, 16 Jun 2021 10:33:23 +0800
qianfanguijin@163.com (qianfanguijin@163.com) wrote:

Hi,

first many thanks for sending this! Indeed OTG support was
broken/missing on the R40, also in Linux.
So it seems you have the found the problem: the missing PHY multiplex.
Many thanks for that! This indeed enables OTG functionality for me
(although with some changes). That means that actually a similar patch
needs to go through Linux.
Do you plan on enabling support in Linux as well?

> From: qianfan Zhao <qianfanguijin@163.com>
> 
> The address of sun8i_r40's phyctrl is 0x01c13404,

But this isn't quite right, is it? See below.

> enable_pmu and dual_route.

Ah, of course! The R40 is closer to the A33/A23 here, with not having
separate EHCI0/OHCI0 controllers, instead relying on the MUSB host IP.
So indeed we don't have the PHY multiplex for PHY0.
I think this is the root cause of the non-working OTG support so far!

> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
> ---
>  drivers/phy/allwinner/phy-sun4i-usb.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> index 5723c98032..608ba46242 100644
> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> @@ -587,10 +587,10 @@ static const struct sun4i_usb_phy_cfg sun8i_r40_cfg = {
>  	.num_phys = 3,
>  	.type = sun8i_r40_phy,
>  	.disc_thresh = 3,
> -	.phyctl_offset = REG_PHYCTL_A33,
> +	.phyctl_offset = REG_PHYCTL_A10,

So this doesn't work for me, no device appearing on the host. Also
writing anything to this register (+0x04) reads back as 0, so it's not
implemented, as in the H3. The register at +0x10 however works, and if I
keep the A33 line, OTG indeed works for me. Same in Linux, btw.

>  	.dedicated_clocks = true,
> -	.enable_pmu_unk1 = true,
> -	.phy0_dual_route = true,
> +	.enable_pmu_unk1 = false,
> +	.phy0_dual_route = false,

If they are false, you don't need to list them, the default of 0 will
take care of this.

Cheers,
Andre

>  };
>  
>  static const struct sun4i_usb_phy_cfg sun8i_v3s_cfg = {

Re: [PATCH 1/4] phy-sun4i-usb: Fix sun8i_r40_cfg

[qianfan]

在 2021/6/21 8:33, Andre Przywara 写道:
> On Wed, 16 Jun 2021 10:33:23 +0800
> qianfanguijin@163.com (qianfanguijin@163.com) wrote:
>
> Hi,
>
> first many thanks for sending this! Indeed OTG support was
> broken/missing on the R40, also in Linux.
> So it seems you have the found the problem: the missing PHY multiplex.
> Many thanks for that! This indeed enables OTG functionality for me
> (although with some changes). That means that actually a similar patch
> needs to go through Linux.
> Do you plan on enabling support in Linux as well?
sure.
>> From: qianfan Zhao <qianfanguijin@163.com>
>>
>> The address of sun8i_r40's phyctrl is 0x01c13404,
> But this isn't quite right, is it? See below.

Yes, the right address of R40 is 0x01c13410. I had checked the bsp code 
of allwinner: #if defined (CONFIG_ARCH_SUN50I) || defined 
(CONFIG_ARCH_SUN8IW10) || defined (CONFIG_ARCH_SUN8IW11)#define  
USBPHYC_REG_o_PHYCTL            0x0410#else#define  
USBPHYC_REG_o_PHYCTL            0x0404#endif

But I had no idea why the addres 0x01c13404 can work fine, maybe I load u-boot by using
sunxi-tools, that the usb otg is already init by IBR.

>> enable_pmu and dual_route.
> Ah, of course! The R40 is closer to the A33/A23 here, with not having
> separate EHCI0/OHCI0 controllers, instead relying on the MUSB host IP.
> So indeed we don't have the PHY multiplex for PHY0.
> I think this is the root cause of the non-working OTG support so far!
>
>> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
>> ---
>>   drivers/phy/allwinner/phy-sun4i-usb.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
>> index 5723c98032..608ba46242 100644
>> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
>> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
>> @@ -587,10 +587,10 @@ static const struct sun4i_usb_phy_cfg sun8i_r40_cfg = {
>>   	.num_phys = 3,
>>   	.type = sun8i_r40_phy,
>>   	.disc_thresh = 3,
>> -	.phyctl_offset = REG_PHYCTL_A33,
>> +	.phyctl_offset = REG_PHYCTL_A10,
> So this doesn't work for me, no device appearing on the host. Also
> writing anything to this register (+0x04) reads back as 0, so it's not
> implemented, as in the H3. The register at +0x10 however works, and if I
> keep the A33 line, OTG indeed works for me. Same in Linux, btw.
>
>>   	.dedicated_clocks = true,
>> -	.enable_pmu_unk1 = true,
>> -	.phy0_dual_route = true,
>> +	.enable_pmu_unk1 = false,
>> +	.phy0_dual_route = false,
> If they are false, you don't need to list them, the default of 0 will
> take care of this.

Thanks for yours guide, I will make a change later.

>
> Cheers,
> Andre
>
>>   };
>>   
>>   static const struct sun4i_usb_phy_cfg sun8i_v3s_cfg = {

Re: [PATCH 2/4] sunxi_musb: Add musb configurations of sun8i-r40

[Andre Przywara]

On Wed, 16 Jun 2021 10:33:24 +0800
qianfanguijin@163.com (qianfanguijin@163.com) wrote:

Hi,

> From: qianfan Zhao <qianfanguijin@163.com>
> 
> R40 has 8 user-configurable endpoints and 8KB FIFO for EPs.

This means the MUSB controller is fully compatible to the H3, so this
whole patch is not needed:

> 
> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
> ---
>  drivers/usb/musb-new/sunxi.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/musb-new/sunxi.c
> b/drivers/usb/musb-new/sunxi.c index fea4105f3d..e03299ea5b 100644
> --- a/drivers/usb/musb-new/sunxi.c
> +++ b/drivers/usb/musb-new/sunxi.c
> @@ -263,7 +263,6 @@ static int sunxi_musb_enable(struct musb *musb)
>  	}
>  
>  	USBC_ForceVbusValidToHigh(musb->mregs);
> -
>  	enabled = true;
>  	return 0;
>  }
> @@ -438,6 +437,30 @@ static struct musb_hdrc_config musb_config_h3 = {
>  	.ram_bits	= SUNXI_MUSB_RAM_BITS,
>  };
>  
> +/* R40/A40 OTG supports only 4 endpoints */
> +#define SUNXI_MUSB_MAX_EP_NUM_R40	5
> +
> +static struct musb_fifo_cfg sunxi_musb_mode_cfg_r40[] = {
> +	MUSB_EP_FIFO_SINGLE(1, FIFO_TX, 512),
> +	MUSB_EP_FIFO_SINGLE(1, FIFO_RX, 512),
> +	MUSB_EP_FIFO_SINGLE(2, FIFO_TX, 512),
> +	MUSB_EP_FIFO_SINGLE(2, FIFO_RX, 512),
> +	MUSB_EP_FIFO_SINGLE(3, FIFO_TX, 512),
> +	MUSB_EP_FIFO_SINGLE(3, FIFO_RX, 512),
> +	MUSB_EP_FIFO_SINGLE(4, FIFO_TX, 512),
> +	MUSB_EP_FIFO_SINGLE(4, FIFO_RX, 512),
> +};
> +
> +static struct musb_hdrc_config musb_config_r40 = {
> +	.fifo_cfg       = sunxi_musb_mode_cfg_r40,
> +	.fifo_cfg_size  = ARRAY_SIZE(sunxi_musb_mode_cfg_r40),
> +	.multipoint	= true,
> +	.dyn_fifo	= true,
> +	.soft_con       = true,
> +	.num_eps	= SUNXI_MUSB_MAX_EP_NUM_R40,
> +	.ram_bits	= SUNXI_MUSB_RAM_BITS,
> +};

Apart from changing the name, this is identical to the H3, so you won't
need a separate structure for that.

> +
>  static int musb_usb_probe(struct udevice *dev)
>  {
>  	struct sunxi_glue *glue = dev_get_priv(dev);
> @@ -527,6 +550,10 @@ static const struct sunxi_musb_config
> sun8i_h3_cfg = { .config = &musb_config_h3,
>  };
>  
> +static const struct sunxi_musb_config sun8i_r40_cfg = {
> +	.config = &musb_config_r40,
> +};
> +
>  static const struct udevice_id sunxi_musb_ids[] = {
>  	{ .compatible = "allwinner,sun4i-a10-musb",
>  			.data = (ulong)&sun4i_a10_cfg },
> @@ -536,6 +563,8 @@ static const struct udevice_id sunxi_musb_ids[] =
> { .data = (ulong)&sun6i_a31_cfg },
>  	{ .compatible = "allwinner,sun8i-h3-musb",
>  			.data = (ulong)&sun8i_h3_cfg },
> +	{ .compatible = "allwinner,sun8i-r40-musb",
> +			.data = (ulong)&sun8i_r40_cfg },

And since there is no difference to the H3, you don't need a separate
compatible string either. Just use "allwinner,sun8i-r40-musb",
"allwinner,sun8i-h3-musb" in the DT, and drop this whole patch here.

Cheers,
Andre

>  	{ }
>  };
>  

Re: [PATCH 3/4] dts: bpi-m2u: Enable USB_OTG by default

[Andre Przywara]

On Wed, 16 Jun 2021 10:33:25 +0800
qianfanguijin@163.com (qianfanguijin@163.com) wrote:

Hi,

in general this patch looks mostly alright (see below), but this needs
to go through Linux first, since we only sync DTs this way.

Some comments below, for when you send this to Linux:

> From: qianfan Zhao <qianfanguijin@163.com>
> 
> bpi-m2u has a hardware usb_otg, let's enable it in dts.
> 
> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
> ---
>  arch/arm/dts/sun8i-r40-bananapi-m2-ultra.dts |  4 ++++
>  arch/arm/dts/sun8i-r40.dtsi                  | 13 +++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/arch/arm/dts/sun8i-r40-bananapi-m2-ultra.dts b/arch/arm/dts/sun8i-r40-bananapi-m2-ultra.dts
> index a6a1087a0c..828ddc63ae 100644
> --- a/arch/arm/dts/sun8i-r40-bananapi-m2-ultra.dts
> +++ b/arch/arm/dts/sun8i-r40-bananapi-m2-ultra.dts
> @@ -117,6 +117,10 @@
>  	status = "okay";
>  };
>  
> +&usb_otg {

Please keep this sorted alphabetically, so it belongs just above the
usbphy node below.
And please put the dr_mode property here (even though U-Boot ignores
this, AFAIK).
Also we would need the USB-ID and power supply in the PHY node
below, compare other DTs with a micro-USB socket and proper VBUS
support.

> +	status = "okay";
> +};
> +
>  &ehci1 {
>  	status = "okay";
>  };
> diff --git a/arch/arm/dts/sun8i-r40.dtsi b/arch/arm/dts/sun8i-r40.dtsi
> index d5ad3b9efd..7c7a9cd9f8 100644
> --- a/arch/arm/dts/sun8i-r40.dtsi
> +++ b/arch/arm/dts/sun8i-r40.dtsi
> @@ -363,6 +363,19 @@
>  			#size-cells = <0>;
>  		};
>  
> +		usb_otg: usb@1c13000 {
> +			compatible = "allwinner,sun8i-r40-musb";

Since this is fully compatible with the H3, please use:
			compatible = "allwinner,sun8i-r40-musb",
				     "allwinner,sun8i-h3-musb";

This way we won't need code changes to the MUSB driver.

The rest (address, interrupt) look alright.

Cheers,
Andre


> +			reg = <0x01c13000 0x0400>;
> +			clocks = <&ccu CLK_BUS_OTG>;
> +			resets = <&ccu RST_BUS_OTG>;
> +			interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "mc";
> +			phys = <&usbphy 0>;
> +			phy-names = "usb";
> +			extcon = <&usbphy 0>;
> +			status = "disabled";
> +		};
> +
>  		usbphy: phy@1c13400 {
>  			compatible = "allwinner,sun8i-r40-usb-phy";
>  			reg = <0x01c13400 0x14>,

Re: [PATCH 4/4] sunxi: defconfig: bpi-m2u: Enable usb gadget and ums by default

[Andre Przywara]

On Wed, 16 Jun 2021 10:33:26 +0800
qianfanguijin@163.com (qianfanguijin@163.com) wrote:

Hi,

> From: qianfan Zhao <qianfanguijin@163.com>
> 
> Since the usb otg driver support R40 device, we enable usb gadget
> functions and ums.
> 
> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
> ---
>  configs/Bananapi_M2_Ultra_defconfig | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/configs/Bananapi_M2_Ultra_defconfig b/configs/Bananapi_M2_Ultra_defconfig
> index 37bcb3d7bf..a0869d56ae 100644
> --- a/configs/Bananapi_M2_Ultra_defconfig
> +++ b/configs/Bananapi_M2_Ultra_defconfig
> @@ -7,12 +7,13 @@ CONFIG_MACPWR="PA17"
>  CONFIG_MMC0_CD_PIN="PH13"
>  CONFIG_MMC_SUNXI_SLOT_EXTRA=2
>  CONFIG_USB1_VBUS_PIN="PH23"
> -CONFIG_USB2_VBUS_PIN="PH23"
>  CONFIG_DEFAULT_DEVICE_TREE="sun8i-r40-bananapi-m2-ultra"
>  CONFIG_AHCI=y
>  # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set
>  CONFIG_SPL_I2C_SUPPORT=y
> +CONFIG_CMD_USB_MASS_STORAGE=y

So in general we don't have those kind of command enables in the
defconfig. It might be worth discussing whether we should enable the
really useful "ums" command by default, for all Allwinner devices
enabling USB gadget mode, but this would be done differently.

>  CONFIG_SCSI_AHCI=y
> +# CONFIG_USB_FUNCTION_FASTBOOT is not set

Why is this explicitly disabled? From what I can see, it doesn't hurt
to leave it enabled (through Kconfig's default), it does not interfere
with other gadgets like UMS or Ethernet.


>  CONFIG_RGMII=y
>  CONFIG_SUN8I_EMAC=y
>  CONFIG_AXP_DLDO4_VOLT=2500
> @@ -20,3 +21,5 @@ CONFIG_AXP_ELDO3_VOLT=1200
>  CONFIG_SCSI=y
>  CONFIG_USB_EHCI_HCD=y
>  CONFIG_USB_OHCI_HCD=y
> +CONFIG_USB_MUSB_GADGET=y
> +CONFIG_USB_GADGET_DOWNLOAD=y

This last symbol is enabled automatically by the FASTBOOT option.

Cheers,
Andre