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

Re: [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.

Re: [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);
>

Re: [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);
>>
>

Re: [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

Re: [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

Re: [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

Re: [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.