[PATCH 1/3] dt: bindings: add bindings for Allwinner A64 usb phy

[PATCH 1/3] dt: bindings: add bindings for Allwinner A64 usb phy

[Icenowy Zheng]

Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
---
 Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
index 95736d7..287150d 100644
--- a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
+++ b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt
@@ -10,6 +10,7 @@ Required properties:
   * allwinner,sun8i-a23-usb-phy
   * allwinner,sun8i-a33-usb-phy
   * allwinner,sun8i-h3-usb-phy
+  * allwinner,sun50i-a64-usb-phy
 - reg : a list of offset + length pairs
 - reg-names :
   * "phy_ctrl"
-- 
2.9.0

[PATCH 2/3] phy: sun4i: add support for A64 usb phy

[Icenowy Zheng]

There's something unknown in the pmu part.

Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
---
 drivers/phy/phy-sun4i-usb.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index 0a45bc6..6f94369 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -97,6 +97,7 @@ enum sun4i_usb_phy_type {
 	sun6i_a31_phy,
 	sun8i_a33_phy,
 	sun8i_h3_phy,
+	sun50i_a64_phy,
 };
 
 struct sun4i_usb_phy_cfg {
@@ -180,8 +181,9 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data,
 
 	mutex_lock(&phy_data->mutex);
 
-	if (phy_data->cfg->type == sun8i_a33_phy) {
-		/* A33 needs us to set phyctl to 0 explicitly */
+	if (phy_data->cfg->type == sun8i_a33_phy ||
+	    phy_data->cfg->type == sun50i_a64_phy) {
+		/* A33 or A64 needs us to set phyctl to 0 explicitly */
 		writel(0, phyctl);
 	}
 
@@ -264,6 +266,12 @@ static int sun4i_usb_phy_init(struct phy *_phy)
 		val = readl(phy->pmu + REG_PMU_UNK_H3);
 		writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
 	} else {
+		/* A64 needs also this unknown bit */
+		if (data->cfg->type == sun50i_a64_phy) {
+			val = readl(phy->pmu + REG_PMU_UNK_H3);
+			writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
+		}
+
 		/* Enable USB 45 Ohm resistor calibration */
 		if (phy->index == 0)
 			sun4i_usb_phy_write(phy, PHY_RES45_CAL_EN, 0x01, 1);
@@ -762,6 +770,14 @@ static const struct sun4i_usb_phy_cfg sun8i_h3_cfg = {
 	.dedicated_clocks = true,
 };
 
+static const struct sun4i_usb_phy_cfg sun50i_a64_cfg = {
+	.num_phys = 2,
+	.type = sun50i_a64_phy,
+	.disc_thresh = 3,
+	.phyctl_offset = REG_PHYCTL_A33,
+	.dedicated_clocks = true,
+};
+
 static const struct of_device_id sun4i_usb_phy_of_match[] = {
 	{ .compatible = "allwinner,sun4i-a10-usb-phy", .data = &sun4i_a10_cfg },
 	{ .compatible = "allwinner,sun5i-a13-usb-phy", .data = &sun5i_a13_cfg },
@@ -770,6 +786,7 @@ static const struct of_device_id sun4i_usb_phy_of_match[] = {
 	{ .compatible = "allwinner,sun8i-a23-usb-phy", .data = &sun8i_a23_cfg },
 	{ .compatible = "allwinner,sun8i-a33-usb-phy", .data = &sun8i_a33_cfg },
 	{ .compatible = "allwinner,sun8i-h3-usb-phy", .data = &sun8i_h3_cfg },
+	{ .compatible = "allwinner,sun50i-a64-usb-phy", .data = &sun50i_a64_cfg},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match);
-- 
2.9.0

[PATCH 3/3] ehci-platform: add the max clock number to 4

[Icenowy Zheng]

Allwinner A64 EHCI requires 4 clocks to be enabled.

Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
---
 drivers/usb/host/ehci-platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index 6816b8c..876dca4 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -38,7 +38,7 @@
 #include "ehci.h"
 
 #define DRIVER_DESC "EHCI generic platform driver"
-#define EHCI_MAX_CLKS 3
+#define EHCI_MAX_CLKS 4
 #define EHCI_MAX_RSTS 3
 #define hcd_to_ehci_priv(h) ((struct ehci_platform_priv *)hcd_to_ehci(h)->priv)
 
-- 
2.9.0

[PATCH 2/3] phy: sun4i: add support for A64 usb phy

[Amit Tomer]

Hello ,

> @@ -264,6 +266,12 @@ static int sun4i_usb_phy_init(struct phy *_phy)
>                 val = readl(phy->pmu + REG_PMU_UNK_H3);
>                 writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
>         } else {
> +               /* A64 needs also this unknown bit */
> +               if (data->cfg->type == sun50i_a64_phy) {
> +                       val = readl(phy->pmu + REG_PMU_UNK_H3);
> +                       writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
> +               }
> +

This bit is also set for H3, shall we reuse it or we should enclose it
in else-if part ?

Thanks
Amit.

[PATCH 2/3] phy: sun4i: add support for A64 usb phy

[Icenowy Zheng]

31.07.2016, 20:39, "Amit Tomer" <amittomer25@gmail.com>:
> Hello ,
>
>> ?@@ -264,6 +266,12 @@ static int sun4i_usb_phy_init(struct phy *_phy)
>> ?????????????????val = readl(phy->pmu + REG_PMU_UNK_H3);
>> ?????????????????writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
>> ?????????} else {
>> ?+ /* A64 needs also this unknown bit */
>> ?+ if (data->cfg->type == sun50i_a64_phy) {
>> ?+ val = readl(phy->pmu + REG_PMU_UNK_H3);
>> ?+ writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
>> ?+ }
>> ?+
>
> This bit is also set for H3, shall we reuse it or we should enclose it
> in else-if part ?
To be honest, I don't know which format is better...
I'm not so familiar about the tradition of Linux kernel coding style...
>
> Thanks
> Amit.

[PATCH 2/3] phy: sun4i: add support for A64 usb phy

[Icenowy Zheng]

31.07.2016, 20:39, "Amit Tomer" <amittomer25@gmail.com>:
> Hello ,
>
>> ?@@ -264,6 +266,12 @@ static int sun4i_usb_phy_init(struct phy *_phy)
>> ?????????????????val = readl(phy->pmu + REG_PMU_UNK_H3);
>> ?????????????????writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
>> ?????????} else {
>> ?+ /* A64 needs also this unknown bit */
>> ?+ if (data->cfg->type == sun50i_a64_phy) {
>> ?+ val = readl(phy->pmu + REG_PMU_UNK_H3);
>> ?+ writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
>> ?+ }
>> ?+
>
> This bit is also set for H3, shall we reuse it or we should enclose it
> in else-if part ?
To be honest, I don't know which format is better...
I'm not so familiar about the tradition of Linux kernel coding style...
>
> Thanks
> Amit.

[PATCH 2/3] phy: sun4i: add support for A64 usb phy

[Hans de Goede]

Hi,

On 31-07-16 13:25, Icenowy Zheng wrote:
> There's something unknown in the pmu part.
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>

Cool, I really like the work you're doing on A64 support,
keep up the good work!

> ---
>  drivers/phy/phy-sun4i-usb.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
> index 0a45bc6..6f94369 100644
> --- a/drivers/phy/phy-sun4i-usb.c
> +++ b/drivers/phy/phy-sun4i-usb.c
> @@ -97,6 +97,7 @@ enum sun4i_usb_phy_type {
>  	sun6i_a31_phy,
>  	sun8i_a33_phy,
>  	sun8i_h3_phy,
> +	sun50i_a64_phy,
>  };
>
>  struct sun4i_usb_phy_cfg {
> @@ -180,8 +181,9 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data,
>
>  	mutex_lock(&phy_data->mutex);
>
> -	if (phy_data->cfg->type == sun8i_a33_phy) {
> -		/* A33 needs us to set phyctl to 0 explicitly */
> +	if (phy_data->cfg->type == sun8i_a33_phy ||
> +	    phy_data->cfg->type == sun50i_a64_phy) {
> +		/* A33 or A64 needs us to set phyctl to 0 explicitly */
>  		writel(0, phyctl);
>  	}
>

Maybe add a "bool explicitly_0_phyctl;" to sun4i_usb_phy_cfg ?

Note I'm not sure of this myself, feel free to keep this as is,
we can always introduce such a bool when we get a 3th SoC which
needs it.

> @@ -264,6 +266,12 @@ static int sun4i_usb_phy_init(struct phy *_phy)
>  		val = readl(phy->pmu + REG_PMU_UNK_H3);
>  		writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
>  	} else {
> +		/* A64 needs also this unknown bit */
> +		if (data->cfg->type == sun50i_a64_phy) {
> +			val = readl(phy->pmu + REG_PMU_UNK_H3);
> +			writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
> +		}
> +
>  		/* Enable USB 45 Ohm resistor calibration */
>  		if (phy->index == 0)
>  			sun4i_usb_phy_write(phy, PHY_RES45_CAL_EN, 0x01, 1);

Erm, as pointed out thus duplicates code from the H3 code path, I believe
that you should add a "bool needs_h3_pmu_unk_poke;" to sun4i_usb_phy_cfg
and then change this bit of the code to:

         if (data->cfg->needs_h3_pmu_unk_poke) {
                 val = readl(phy->pmu + REG_PMU_UNK_H3);
                 writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
         }

         if (data->cfg->type == sun8i_h3_phy) {
                 if (phy->index == 0) {
                         val = readl(data->base + REG_PHY_UNK_H3);
                         writel(val & ~1, data->base + REG_PHY_UNK_H3);
                 }
         } else {
		... (original code)
	}

That seems like a cleaner solution to me.

And do not forget to set the needs_h3_pmu_unk_poke for the h3!

I would not add it to the sun4i_usb_phy_cfg structs for the
other type SoCs, if part of the struct is initialized the
rest will get set to 0 by the compiler and I believe that
it things will be more readable without an explicit:

	.needs_h3_pmu_unk_poke = false

Everywhere.


Thanks & Regards,

Hans




> @@ -762,6 +770,14 @@ static const struct sun4i_usb_phy_cfg sun8i_h3_cfg = {
>  	.dedicated_clocks = true,
>  };
>
> +static const struct sun4i_usb_phy_cfg sun50i_a64_cfg = {
> +	.num_phys = 2,
> +	.type = sun50i_a64_phy,
> +	.disc_thresh = 3,
> +	.phyctl_offset = REG_PHYCTL_A33,
> +	.dedicated_clocks = true,
> +};
> +
>  static const struct of_device_id sun4i_usb_phy_of_match[] = {
>  	{ .compatible = "allwinner,sun4i-a10-usb-phy", .data = &sun4i_a10_cfg },
>  	{ .compatible = "allwinner,sun5i-a13-usb-phy", .data = &sun5i_a13_cfg },
> @@ -770,6 +786,7 @@ static const struct of_device_id sun4i_usb_phy_of_match[] = {
>  	{ .compatible = "allwinner,sun8i-a23-usb-phy", .data = &sun8i_a23_cfg },
>  	{ .compatible = "allwinner,sun8i-a33-usb-phy", .data = &sun8i_a33_cfg },
>  	{ .compatible = "allwinner,sun8i-h3-usb-phy", .data = &sun8i_h3_cfg },
> +	{ .compatible = "allwinner,sun50i-a64-usb-phy", .data = &sun50i_a64_cfg},
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match);
>

[PATCH 2/3] phy: sun4i: add support for A64 usb phy

[Chen-Yu Tsai]

Hi,

On Sun, Jul 31, 2016 at 10:39 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 31-07-16 13:25, Icenowy Zheng wrote:
>>
>> There's something unknown in the pmu part.
>>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>
>
> Cool, I really like the work you're doing on A64 support,
> keep up the good work!
>
>> ---
>>  drivers/phy/phy-sun4i-usb.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
>> index 0a45bc6..6f94369 100644
>> --- a/drivers/phy/phy-sun4i-usb.c
>> +++ b/drivers/phy/phy-sun4i-usb.c
>> @@ -97,6 +97,7 @@ enum sun4i_usb_phy_type {
>>         sun6i_a31_phy,
>>         sun8i_a33_phy,
>>         sun8i_h3_phy,
>> +       sun50i_a64_phy,
>>  };
>>
>>  struct sun4i_usb_phy_cfg {
>> @@ -180,8 +181,9 @@ static void sun4i_usb_phy_write(struct sun4i_usb_phy
>> *phy, u32 addr, u32 data,
>>
>>         mutex_lock(&phy_data->mutex);
>>
>> -       if (phy_data->cfg->type == sun8i_a33_phy) {
>> -               /* A33 needs us to set phyctl to 0 explicitly */
>> +       if (phy_data->cfg->type == sun8i_a33_phy ||
>> +           phy_data->cfg->type == sun50i_a64_phy) {
>> +               /* A33 or A64 needs us to set phyctl to 0 explicitly */
>>                 writel(0, phyctl);
>>         }
>>
>
> Maybe add a "bool explicitly_0_phyctl;" to sun4i_usb_phy_cfg ?
>
> Note I'm not sure of this myself, feel free to keep this as is,
> we can always introduce such a bool when we get a 3th SoC which
> needs it.
>
>> @@ -264,6 +266,12 @@ static int sun4i_usb_phy_init(struct phy *_phy)
>>                 val = readl(phy->pmu + REG_PMU_UNK_H3);
>>                 writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
>>         } else {
>> +               /* A64 needs also this unknown bit */
>> +               if (data->cfg->type == sun50i_a64_phy) {
>> +                       val = readl(phy->pmu + REG_PMU_UNK_H3);
>> +                       writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
>> +               }
>> +
>>                 /* Enable USB 45 Ohm resistor calibration */
>>                 if (phy->index == 0)
>>                         sun4i_usb_phy_write(phy, PHY_RES45_CAL_EN, 0x01,
>> 1);
>
>
> Erm, as pointed out thus duplicates code from the H3 code path, I believe
> that you should add a "bool needs_h3_pmu_unk_poke;" to sun4i_usb_phy_cfg
> and then change this bit of the code to:
>
>         if (data->cfg->needs_h3_pmu_unk_poke) {
>                 val = readl(phy->pmu + REG_PMU_UNK_H3);
>                 writel(val & ~2, phy->pmu + REG_PMU_UNK_H3);
>         }
>
>         if (data->cfg->type == sun8i_h3_phy) {
>                 if (phy->index == 0) {
>                         val = readl(data->base + REG_PHY_UNK_H3);
>                         writel(val & ~1, data->base + REG_PHY_UNK_H3);
>                 }
>         } else {
>                 ... (original code)
>         }
>
> That seems like a cleaner solution to me.
>
> And do not forget to set the needs_h3_pmu_unk_poke for the h3!
>
> I would not add it to the sun4i_usb_phy_cfg structs for the
> other type SoCs, if part of the struct is initialized the
> rest will get set to 0 by the compiler and I believe that
> it things will be more readable without an explicit:
>
>         .needs_h3_pmu_unk_poke = false
>
> Everywhere.
>

FYI: H3 USB PHY support is not complete. USB0 PHY is not supported, and
it does not work. I did a preliminary comparison of this PHY driver and
the code in Allwinner's SDK. There are some bits missing.

ChenYu

>
> Thanks & Regards,
>
> Hans
>
>
>
>
>
>> @@ -762,6 +770,14 @@ static const struct sun4i_usb_phy_cfg sun8i_h3_cfg =
>> {
>>         .dedicated_clocks = true,
>>  };
>>
>> +static const struct sun4i_usb_phy_cfg sun50i_a64_cfg = {
>> +       .num_phys = 2,
>> +       .type = sun50i_a64_phy,
>> +       .disc_thresh = 3,
>> +       .phyctl_offset = REG_PHYCTL_A33,
>> +       .dedicated_clocks = true,
>> +};
>> +
>>  static const struct of_device_id sun4i_usb_phy_of_match[] = {
>>         { .compatible = "allwinner,sun4i-a10-usb-phy", .data =
>> &sun4i_a10_cfg },
>>         { .compatible = "allwinner,sun5i-a13-usb-phy", .data =
>> &sun5i_a13_cfg },
>> @@ -770,6 +786,7 @@ static const struct of_device_id
>> sun4i_usb_phy_of_match[] = {
>>         { .compatible = "allwinner,sun8i-a23-usb-phy", .data =
>> &sun8i_a23_cfg },
>>         { .compatible = "allwinner,sun8i-a33-usb-phy", .data =
>> &sun8i_a33_cfg },
>>         { .compatible = "allwinner,sun8i-h3-usb-phy", .data =
>> &sun8i_h3_cfg },
>> +       { .compatible = "allwinner,sun50i-a64-usb-phy", .data =
>> &sun50i_a64_cfg},
>>         { },
>>  };
>>  MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match);
>>
>

[PATCH 2/3] phy: sun4i: add support for A64 usb phy

[Hans de Goede]

Hi,

On 31-07-16 16:50, Chen-Yu Tsai wrote:

> FYI: H3 USB PHY support is not complete. USB0 PHY is not supported, and
> it does not work. I did a preliminary comparison of this PHY driver and
> the code in Allwinner's SDK. There are some bits missing.

Right that is a known issue, I believe someone was working on an
otg support patch series for the H3 though ?

Regards,

Hans

[PATCH 3/3] ehci-platform: add the max clock number to 4

[Arnd Bergmann]

On Sunday, July 31, 2016 7:25:36 PM CEST Icenowy Zheng wrote:
> Allwinner A64 EHCI requires 4 clocks to be enabled.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> 

Can you say what those four clocks are?

Are you sure that it's not just a case of a clock being
incorrectly described in the clk driver, i.e. you reference
one clock along with its parent here?

	Arnd

[PATCH 3/3] ehci-platform: add the max clock number to 4

[Icenowy Zheng]

			clocks = <&ccu CLK_A64_BUS_OHCI1>,
				 <&ccu CLK_A64_BUS_EHCI1>,
				 <&ccu CLK_A64_USB_OHCI0>,
				 <&ccu CLK_A64_USB_OHCI1>;

On A64, EHCI requires the matched OHCI to work.
And OHCI1 clock requires OHCI0 clock to work.

(But from the SoC's user manual we cannot get any infomation
about the relationship between OHCI1 clock and OHCI0 clock,
and in the manual OHCI0 clock is called OTG-OHCI)

01.08.2016, 15:01, "Arnd Bergmann" <arnd@arndb.de>:
> On Sunday, July 31, 2016 7:25:36 PM CEST Icenowy Zheng wrote:
>> ?Allwinner A64 EHCI requires 4 clocks to be enabled.
>>
>> ?Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>
> Can you say what those four clocks are?
>
> Are you sure that it's not just a case of a clock being
> incorrectly described in the clk driver, i.e. you reference
> one clock along with its parent here?
>
> ????????Arnd

[PATCH 3/3] ehci-platform: add the max clock number to 4

[Hans de Goede]

Hi,

On 01-08-16 09:05, Icenowy Zheng wrote:
> 			clocks = <&ccu CLK_A64_BUS_OHCI1>,
> 				 <&ccu CLK_A64_BUS_EHCI1>,
> 				 <&ccu CLK_A64_USB_OHCI0>,
> 				 <&ccu CLK_A64_USB_OHCI1>;
>
> On A64, EHCI requires the matched OHCI to work.

Ah, so just like on the H3 (where this also is needed
and not documented).

> And OHCI1 clock requires OHCI0 clock to work.

Hmm, that one is new, can you double check this ?

Regards,

Hans


>
> (But from the SoC's user manual we cannot get any infomation
> about the relationship between OHCI1 clock and OHCI0 clock,
> and in the manual OHCI0 clock is called OTG-OHCI)
>
> 01.08.2016, 15:01, "Arnd Bergmann" <arnd@arndb.de>:
>> On Sunday, July 31, 2016 7:25:36 PM CEST Icenowy Zheng wrote:
>>>  Allwinner A64 EHCI requires 4 clocks to be enabled.
>>>
>>>  Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>>
>> Can you say what those four clocks are?
>>
>> Are you sure that it's not just a case of a clock being
>> incorrectly described in the clk driver, i.e. you reference
>> one clock along with its parent here?
>>
>>         Arnd

[PATCH 3/3] ehci-platform: add the max clock number to 4

[Icenowy Zheng]

01.08.2016, 15:27, "Hans de Goede" <hdegoede@redhat.com>:
> Hi,
>
> On 01-08-16 09:05, Icenowy Zheng wrote:
>> ?????????????????????????clocks = <&ccu CLK_A64_BUS_OHCI1>,
>> ??????????????????????????????????<&ccu CLK_A64_BUS_EHCI1>,
>> ??????????????????????????????????<&ccu CLK_A64_USB_OHCI0>,
>> ??????????????????????????????????<&ccu CLK_A64_USB_OHCI1>;
>>
>> ?On A64, EHCI requires the matched OHCI to work.
>
> Ah, so just like on the H3 (where this also is needed
> and not documented).
Yes, I feeled that A64 is like a hybrid of H3 and A33.
>
>> ?And OHCI1 clock requires OHCI0 clock to work.
>
> Hmm, that one is new, can you double check this ?
SCLK_GATING_OHCI.
Gating Special Clock For OHCI(48M and 12M)
00: Clock is OFF
01: OTG-OHCI Clock is ON
10: Clock is OFF
11:OTG-OHCI and OHCI0 Clock is ON

P.113 of A64 user manual 1.0
>
> Regards,
>
> Hans
>
>> ?(But from the SoC's user manual we cannot get any infomation
>> ?about the relationship between OHCI1 clock and OHCI0 clock,
>> ?and in the manual OHCI0 clock is called OTG-OHCI)
>>
>> ?01.08.2016, 15:01, "Arnd Bergmann" <arnd@arndb.de>:
>>> ?On Sunday, July 31, 2016 7:25:36 PM CEST Icenowy Zheng wrote:
>>>> ??Allwinner A64 EHCI requires 4 clocks to be enabled.
>>>>
>>>> ??Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>>>
>>> ?Can you say what those four clocks are?
>>>
>>> ?Are you sure that it's not just a case of a clock being
>>> ?incorrectly described in the clk driver, i.e. you reference
>>> ?one clock along with its parent here?
>>>
>>> ?????????Arnd

[PATCH 3/3] ehci-platform: add the max clock number to 4

[Hans de Goede]

Hi,

On 01-08-16 10:18, Icenowy Zheng wrote:
>
>
> 01.08.2016, 15:27, "Hans de Goede" <hdegoede@redhat.com>:
>> Hi,
>>
>> On 01-08-16 09:05, Icenowy Zheng wrote:
>>>                          clocks = <&ccu CLK_A64_BUS_OHCI1>,
>>>                                   <&ccu CLK_A64_BUS_EHCI1>,
>>>                                   <&ccu CLK_A64_USB_OHCI0>,
>>>                                   <&ccu CLK_A64_USB_OHCI1>;
>>>
>>>  On A64, EHCI requires the matched OHCI to work.
>>
>> Ah, so just like on the H3 (where this also is needed
>> and not documented).
> Yes, I feeled that A64 is like a hybrid of H3 and A33.
>>
>>>  And OHCI1 clock requires OHCI0 clock to work.
>>
>> Hmm, that one is new, can you double check this ?
> SCLK_GATING_OHCI.
> Gating Special Clock For OHCI(48M and 12M)
> 00: Clock is OFF
> 01: OTG-OHCI Clock is ON
> 10: Clock is OFF
> 11:OTG-OHCI and OHCI0 Clock is ON
>
> P.113 of A64 user manual 1.0

Ah I see that looks weird, I assume that you're working
on getting the regular usb host on the A64 to work, iow
the "HCI0" block in the usb block diagram at p. 580, right ?

It could be that the ohci clock for that somehow is
tapped from the ohci clock for the "USB-OTG-HCI" block,
but:

P.113, other bits of the USBPHY_CFG_REG register:

23:22 R/W 0x0 OHCI1_12M_SRC_SEL. OHCI1 12M Source Select
00: 12M divided from 48M
01: 12M divided from 24M
10: LOSC
11: /

21:20 R/W 0x0 OHCI0_12M_SRC_SEL. OHCI0 12M Source Select
00: 12M divided from 48M
01: 12M divided from 24M
10: LOSC
11: /

Suggests that they are independent...

Have you tried to simply drop

                              <&ccu CLK_A64_USB_OHCI0>,

 From the clocks list and check that usb-1 devices
(e.g. a mouse / keyboard) plugged directly into the
board still work ?

If it does we can simply drop it, of it does not work
then indeed we need 4 clocks because allwinner has
done something weird again.

###

Also it seems that the CLK_A64_USB_OHCI0 /
CLK_A64_USB_OHCI1 names are wrong, the datasheet
consistently (*) refers to "usb-otg-ohci" and an
"usb-ohci0" rather then ohci0 and ohci1 (**)

Except for the USBPHY_CFG_REG documentation for bits 20:23,
which I believe is an error in the datasheet.

So we should do the same in the dt-bindings IMHO.

Regards,

Hans



*) In the system address map (p. 73), "Interrupt Source" list (p.210)
in the "Bus Clock Gating Register0" doc (p. 100) and
in the usb block diagram (p. 580).

**) Unlike the H3 where usb-otg-ohci is called usb-ohci0 and
the first non otg host controller is called usb-ohci1.