[0/6] arm64: meson: Add support for USB on Amlogic A1
mbox series

Message ID 1574405757-76184-1-git-send-email-hanjie.lin@amlogic.com
Headers show
Series
  • arm64: meson: Add support for USB on Amlogic A1
Related show

Message

Hanjie Lin Nov. 22, 2019, 6:55 a.m. UTC
This patchset adds support for USB on Amlogic A1 SoCs.

This patchset is composed with :
- bindings of the PHY
- bindings of the USB Control Glue
- PHY Driver
- USB Control Glue driver
- dts of the PHY
- dts of the USB Controller

The Amlogic A1 USB Complex is composed of :
- 1 DWC3 USB controller for USB2 Host functionality
- 1 USB2 PHY for USB2 Host functionality

The USB Control Glue setups the clocks and the reset about DWC3 USB
controller, and binds to the USB2 PHY. It also configures the 8bit
UTMI interfaces for the USB2 PHY, including setting USB2 phy mode.

The USB2 PHY driver initializes the phy analog settings, phy PLL 
setup and phy tuning.

This patchset is based on A1 clock/power domain/reset series at [0].

[0]
https://patchwork.kernel.org/project/linux-amlogic/list/?series=185477
https://patchwork.kernel.org/project/linux-amlogic/list/?series=180055
https://patchwork.kernel.org/project/linux-amlogic/list/?series=189643

Hanjie Lin (6):
  dt-bindings: phy: Add Amlogic G12A USB2 PHY Bindings
  dt-bindings: usb: dwc3: Add the Amlogic A1 Family DWC3 Glue Bindings
  phy: amlogic: Add Amlogic A1 USB2 PHY Driver
  usb: dwc3: Add Amlogic A1 DWC3 glue
  arm64: dts: meson: a1: Enable USB2 PHY
  arm64: dts: meson: a1: Enable DWC3 controller

 .../bindings/phy/amlogic,meson-a1-usb2-phy.yaml    |  55 +++
 .../devicetree/bindings/usb/amlogic,dwc3.txt       |  53 +++
 arch/arm64/boot/dts/amlogic/meson-a1.dtsi          |  41 +++
 drivers/phy/amlogic/Kconfig                        |  13 +
 drivers/phy/amlogic/Makefile                       |   1 +
 drivers/phy/amlogic/phy-meson-a1-usb2.c            | 327 +++++++++++++++++
 drivers/usb/dwc3/Kconfig                           |  11 +
 drivers/usb/dwc3/Makefile                          |   1 +
 drivers/usb/dwc3/dwc3-meson-a1.c                   | 397 +++++++++++++++++++++
 9 files changed, 899 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/amlogic,meson-a1-usb2-phy.yaml
 create mode 100644 drivers/phy/amlogic/phy-meson-a1-usb2.c
 create mode 100644 drivers/usb/dwc3/dwc3-meson-a1.c

Comments

Martin Blumenstingl Nov. 22, 2019, 7:52 a.m. UTC | #1
Hello Hanjie,

On Fri, Nov 22, 2019 at 7:55 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
[...]
>   dt-bindings: phy: Add Amlogic G12A USB2 PHY Bindings
>   dt-bindings: usb: dwc3: Add the Amlogic A1 Family DWC3 Glue Bindings
>   phy: amlogic: Add Amlogic A1 USB2 PHY Driver
drivers/phy/amlogic/phy-meson-g12a-usb2.c seems very similar to the A1
USB2 PHY you are introducing here.

>   usb: dwc3: Add Amlogic A1 DWC3 glue
drivers/usb/dwc3/dwc3-meson-g12a.c is also very similar to the dwc3 glue.

I have two questions:
- how is the PHY and the dwc3 glue different from G12A (or SM1)?
- why do we need a separate set of new drivers (instead of updating
the existing drivers)?

We try to use one driver for the same IP block, even if there are
several revisions with small differences (for example the SAR ADC
driver supports all SoC generations from Meson8 to G12A/G12B/SM1,
because 80-90% of the code is shared across all revisions).


Martin
Hanjie Lin Nov. 25, 2019, 7:53 a.m. UTC | #2
On 2019/11/22 15:52, Martin Blumenstingl wrote:
> Hello Hanjie,
> 
> On Fri, Nov 22, 2019 at 7:55 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
> [...]
>>   dt-bindings: phy: Add Amlogic G12A USB2 PHY Bindings
>>   dt-bindings: usb: dwc3: Add the Amlogic A1 Family DWC3 Glue Bindings
>>   phy: amlogic: Add Amlogic A1 USB2 PHY Driver
> drivers/phy/amlogic/phy-meson-g12a-usb2.c seems very similar to the A1
> USB2 PHY you are introducing here.
> 
>>   usb: dwc3: Add Amlogic A1 DWC3 glue
> drivers/usb/dwc3/dwc3-meson-g12a.c is also very similar to the dwc3 glue.
> 
> I have two questions:
> - how is the PHY and the dwc3 glue different from G12A (or SM1)?
> - why do we need a separate set of new drivers (instead of updating
> the existing drivers)?
> 
> We try to use one driver for the same IP block, even if there are
> several revisions with small differences (for example the SAR ADC
> driver supports all SoC generations from Meson8 to G12A/G12B/SM1,
> because 80-90% of the code is shared across all revisions).
> 
> 
> Martin
> 
> .
> 

Hi Martin,

thanks for the comment.

1, G12A have usb2-phy0/usb2-phy1/usb3-phy0 three phys and an interrupt to support host/peripheral/otg modes.
   A1 has one usb2-phy0 phy and only support host mode.
   
2, G12A glue/phy drivers are for G12A SoCs, there are some diffrences to A1.
   G12A glue driver have dr_mode and interrupts two attributes to support otg mode while A1 hasn't this requirement.
   G12A glue driver has a hard coding vbus regulator code to support otg mode while A1 hasn't this requirement.
   G12A glue driver has a hard coding support phys while A1 only supports host mode.
   	enum {
		USB2_HOST_PHY = 0,
		USB2_OTG_PHY,
		USB3_HOST_PHY,
		PHY_COUNT,
		};
   G12A glue driver only supports one clock while A1 needs four clocks.
   G12A and A1 phy drivers have different register configurations since hardware differences.
   
3, We have estimated these differences and we thought it's more clear and readable to have a dedicated glue/phy
   driver for A1 SoCs, so also dedicated dt-bindings.
Martin Blumenstingl Nov. 25, 2019, 10:02 p.m. UTC | #3
Hi Hanjie,

On Mon, Nov 25, 2019 at 8:53 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
>
>
>
> On 2019/11/22 15:52, Martin Blumenstingl wrote:
> > Hello Hanjie,
> >
> > On Fri, Nov 22, 2019 at 7:55 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
> > [...]
> >>   dt-bindings: phy: Add Amlogic G12A USB2 PHY Bindings
> >>   dt-bindings: usb: dwc3: Add the Amlogic A1 Family DWC3 Glue Bindings
> >>   phy: amlogic: Add Amlogic A1 USB2 PHY Driver
> > drivers/phy/amlogic/phy-meson-g12a-usb2.c seems very similar to the A1
> > USB2 PHY you are introducing here.
> >
> >>   usb: dwc3: Add Amlogic A1 DWC3 glue
> > drivers/usb/dwc3/dwc3-meson-g12a.c is also very similar to the dwc3 glue.
> >
> > I have two questions:
> > - how is the PHY and the dwc3 glue different from G12A (or SM1)?
> > - why do we need a separate set of new drivers (instead of updating
> > the existing drivers)?
> >
> > We try to use one driver for the same IP block, even if there are
> > several revisions with small differences (for example the SAR ADC
> > driver supports all SoC generations from Meson8 to G12A/G12B/SM1,
> > because 80-90% of the code is shared across all revisions).
> >
> >
> > Martin
> >
> > .
> >
>
> Hi Martin,
>
> thanks for the comment.
>
> 1, G12A have usb2-phy0/usb2-phy1/usb3-phy0 three phys and an interrupt to support host/peripheral/otg modes.
>    A1 has one usb2-phy0 phy and only support host mode.
dwc3-meson-g12a treats PHYs as optional
so if you only pass "usb2-phy0" and skip usb2-phy1/usb3-phy0 then it
will still work fine
(I didn't check whether the binding also reflects this)

> 2, G12A glue/phy drivers are for G12A SoCs, there are some diffrences to A1.
>    G12A glue driver have dr_mode and interrupts two attributes to support otg mode while A1 hasn't this requirement.
dwc3-meson-g12a ignores the interrupt for HOST-only mode
(I didn't check whether the IRQ is optional in the dt-binding)

>    G12A glue driver has a hard coding vbus regulator code to support otg mode while A1 hasn't this requirement.
my understanding is that whether a board has a VBUS regulator depends
on the board design. it has nothing to do with the SoC itself

>    G12A glue driver has a hard coding support phys while A1 only supports host mode.
>         enum {
>                 USB2_HOST_PHY = 0,
>                 USB2_OTG_PHY,
>                 USB3_HOST_PHY,
>                 PHY_COUNT,
>                 };
this goes together with comment #1 - you can skip USB2_OTG_PHY and
USB3_HOST_PHY and the driver should still work fine

>    G12A glue driver only supports one clock while A1 needs four clocks.
indeed, the dwc3-meson-g12a needs to be updated to support this
I don't think that I have used it myself yet but there's the
clk_bulk_data framework
it seems to fit this use-case pretty well: define an arbitrary number
of clocks for G12A/B an another set of clocks for A1 - then use the
clk_bulk_data framework to enable/disable them all at once

>    G12A and A1 phy drivers have different register configurations since hardware differences.
other drivers have similar requirements: (mostly) identical register
layout but different values per SoC
here are two examples (I'm not sure if they are good examples though):
Lantiq/Intel SoC [0] and Allwinner SoCs [1]

I compared your driver with phy-meson-g12a-usb2 and only found four differences:
1) PHY_CTRL_R18_MPLL_DCO_CLK_SEL is set for A1
2) PHY_CTRL_R13_UPDATE_PMA_SIGNALS is not set for A1
3) PHY_CTRL_R21 is updated twice for A1 (once for earlier gen SoCs)
4) A1 doesn't reference the "xtal" clock

Difference 4) seems to be a general problem because there seems to be
a PLL inside the PHY registers and that PLL must be fed by some input
clock
So I believe that there is some clock input (which is currently
missing from your A1 USB2 PHY driver)

> 3, We have estimated these differences and we thought it's more clear and readable to have a dedicated glue/phy
>    driver for A1 SoCs, so also dedicated dt-bindings.
I think we should separate the driver and dt-bindings

Based on what I have seen so far my preference for the PHY is:
- use the existing dt-binding, because it seems to be the same IP
block with different register configuration
- use the existing driver because there are only three different
register values (to me it feels like a dedicated driver for these
means more overhead for little benefit)

for the glue I think:
- extend the existing dt-bindings and make some of the PHYs and the
interrupt line optional. making the PHYs optional will be needed when
adding GXL/GXM/AXG support anyways
- use the existing driver and make the clock inputs depend on the SoC
- everything else should already work as is

please let me know if I missed something:
comparing/reviewing the new and existing drivers is harder than just
copying the existing one and modifying that copy
(this is one of the reasons why I think that duplicating code makes
the drivers harder to maintain)

I also thought about the negative consequences of extending the
existing driver(s).
modifying the existing code could break the driver for existing boards.
however, I think that is not a problem because BayLibre's Kernel CI
labs have good coverage for G12A, G12B and SM1.
so if you add some A1 boards there (or host your own lab with A1
boards) any breakage will be found early (the Kernel CI bot even does
git bisect and sends emails)


Martin


[0] https://github.com/torvalds/linux/blob/d2912cb15bdda8ba4a5dd73396ad62641af2f520/drivers/phy/lantiq/phy-lantiq-rcu-usb2.c#L47
[1] https://github.com/torvalds/linux/blob/c942fddf8793b2013be8c901b47d0a8dc02bf99f/drivers/phy/allwinner/phy-sun4i-usb.c#L862
Neil Armstrong Nov. 26, 2019, 1:11 p.m. UTC | #4
On 25/11/2019 23:02, Martin Blumenstingl wrote:
> Hi Hanjie,
> 
> On Mon, Nov 25, 2019 at 8:53 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
>>
>>
>>
>> On 2019/11/22 15:52, Martin Blumenstingl wrote:
>>> Hello Hanjie,
>>>
>>> On Fri, Nov 22, 2019 at 7:55 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
>>> [...]
>>>>   dt-bindings: phy: Add Amlogic G12A USB2 PHY Bindings
>>>>   dt-bindings: usb: dwc3: Add the Amlogic A1 Family DWC3 Glue Bindings
>>>>   phy: amlogic: Add Amlogic A1 USB2 PHY Driver
>>> drivers/phy/amlogic/phy-meson-g12a-usb2.c seems very similar to the A1
>>> USB2 PHY you are introducing here.
>>>
>>>>   usb: dwc3: Add Amlogic A1 DWC3 glue
>>> drivers/usb/dwc3/dwc3-meson-g12a.c is also very similar to the dwc3 glue.
>>>
>>> I have two questions:
>>> - how is the PHY and the dwc3 glue different from G12A (or SM1)?
>>> - why do we need a separate set of new drivers (instead of updating
>>> the existing drivers)?
>>>
>>> We try to use one driver for the same IP block, even if there are
>>> several revisions with small differences (for example the SAR ADC
>>> driver supports all SoC generations from Meson8 to G12A/G12B/SM1,
>>> because 80-90% of the code is shared across all revisions).
>>>
>>>
>>> Martin
>>>
>>> .
>>>
>>
>> Hi Martin,
>>
>> thanks for the comment.
>>
>> 1, G12A have usb2-phy0/usb2-phy1/usb3-phy0 three phys and an interrupt to support host/peripheral/otg modes.
>>    A1 has one usb2-phy0 phy and only support host mode.
> dwc3-meson-g12a treats PHYs as optional
> so if you only pass "usb2-phy0" and skip usb2-phy1/usb3-phy0 then it
> will still work fine
> (I didn't check whether the binding also reflects this)

Exact, a simple match data could make max-phys to 1 for A1.

> 
>> 2, G12A glue/phy drivers are for G12A SoCs, there are some diffrences to A1.
>>    G12A glue driver have dr_mode and interrupts two attributes to support otg mode while A1 hasn't this requirement.
> dwc3-meson-g12a ignores the interrupt for HOST-only mode
> (I didn't check whether the IRQ is optional in the dt-binding)

Interrupt support and OTG manual switch is optional and can be easily bypassed.

> 
>>    G12A glue driver has a hard coding vbus regulator code to support otg mode while A1 hasn't this requirement.
> my understanding is that whether a board has a VBUS regulator depends
> on the board design. it has nothing to do with the SoC itself
> 
>>    G12A glue driver has a hard coding support phys while A1 only supports host mode.
>>         enum {
>>                 USB2_HOST_PHY = 0,
>>                 USB2_OTG_PHY,
>>                 USB3_HOST_PHY,
>>                 PHY_COUNT,
>>                 };
> this goes together with comment #1 - you can skip USB2_OTG_PHY and
> USB3_HOST_PHY and the driver should still work fine

Exact

> 
>>    G12A glue driver only supports one clock while A1 needs four clocks.
> indeed, the dwc3-meson-g12a needs to be updated to support this
> I don't think that I have used it myself yet but there's the
> clk_bulk_data framework
> it seems to fit this use-case pretty well: define an arbitrary number
> of clocks for G12A/B an another set of clocks for A1 - then use the
> clk_bulk_data framework to enable/disable them all at once

Exact, a simple conversion to clk_bulk_* would be enough

> 
>>    G12A and A1 phy drivers have different register configurations since hardware differences.
> other drivers have similar requirements: (mostly) identical register
> layout but different values per SoC
> here are two examples (I'm not sure if they are good examples though):
> Lantiq/Intel SoC [0] and Allwinner SoCs [1]
> 
> I compared your driver with phy-meson-g12a-usb2 and only found four differences:
> 1) PHY_CTRL_R18_MPLL_DCO_CLK_SEL is set for A1
> 2) PHY_CTRL_R13_UPDATE_PMA_SIGNALS is not set for A1
> 3) PHY_CTRL_R21 is updated twice for A1 (once for earlier gen SoCs)
> 4) A1 doesn't reference the "xtal" clock
> 
> Difference 4) seems to be a general problem because there seems to be
> a PLL inside the PHY registers and that PLL must be fed by some input
> clock
> So I believe that there is some clock input (which is currently
> missing from your A1 USB2 PHY driver)

These differences are trivial to add with a match data structure.

> 
>> 3, We have estimated these differences and we thought it's more clear and readable to have a dedicated glue/phy
>>    driver for A1 SoCs, so also dedicated dt-bindings.
> I think we should separate the driver and dt-bindings
> 
> Based on what I have seen so far my preference for the PHY is:
> - use the existing dt-binding, because it seems to be the same IP
> block with different register configuration
> - use the existing driver because there are only three different
> register values (to me it feels like a dedicated driver for these
> means more overhead for little benefit)
> 
> for the glue I think:
> - extend the existing dt-bindings and make some of the PHYs and the
> interrupt line optional. making the PHYs optional will be needed when
> adding GXL/GXM/AXG support anyways
> - use the existing driver and make the clock inputs depend on the SoC
> - everything else should already work as is
> 
> please let me know if I missed something:
> comparing/reviewing the new and existing drivers is harder than just
> copying the existing one and modifying that copy
> (this is one of the reasons why I think that duplicating code makes
> the drivers harder to maintain)
> 
> I also thought about the negative consequences of extending the
> existing driver(s).
> modifying the existing code could break the driver for existing boards.
> however, I think that is not a problem because BayLibre's Kernel CI
> labs have good coverage for G12A, G12B and SM1.
> so if you add some A1 boards there (or host your own lab with A1
> boards) any breakage will be found early (the Kernel CI bot even does
> git bisect and sends emails)
The overall architecture is the same since the GXL SoCs, we also plan to
move the GXL/GXM USB complex into the G12A usb-ctrl driver because the architecture
is very similar.

For A1, the changes will be quite minimal, please try and post a RFC version so we
can evaluate.

Thanks,

Neil

> 
> 
> Martin
> 
> 
> [0] https://github.com/torvalds/linux/blob/d2912cb15bdda8ba4a5dd73396ad62641af2f520/drivers/phy/lantiq/phy-lantiq-rcu-usb2.c#L47
> [1] https://github.com/torvalds/linux/blob/c942fddf8793b2013be8c901b47d0a8dc02bf99f/drivers/phy/allwinner/phy-sun4i-usb.c#L862
>
Hanjie Lin Nov. 27, 2019, 7:11 a.m. UTC | #5
On 2019/11/26 6:02, Martin Blumenstingl wrote:
> Hi Hanjie,
> 
> On Mon, Nov 25, 2019 at 8:53 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
>>
>>
>>
>> On 2019/11/22 15:52, Martin Blumenstingl wrote:
>>> Hello Hanjie,
>>>
>>> On Fri, Nov 22, 2019 at 7:55 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
>>> [...]
>>>>   dt-bindings: phy: Add Amlogic G12A USB2 PHY Bindings
>>>>   dt-bindings: usb: dwc3: Add the Amlogic A1 Family DWC3 Glue Bindings
>>>>   phy: amlogic: Add Amlogic A1 USB2 PHY Driver
>>> drivers/phy/amlogic/phy-meson-g12a-usb2.c seems very similar to the A1
>>> USB2 PHY you are introducing here.
>>>
>>>>   usb: dwc3: Add Amlogic A1 DWC3 glue
>>> drivers/usb/dwc3/dwc3-meson-g12a.c is also very similar to the dwc3 glue.
>>>
>>> I have two questions:
>>> - how is the PHY and the dwc3 glue different from G12A (or SM1)?
>>> - why do we need a separate set of new drivers (instead of updating
>>> the existing drivers)?
>>>
>>> We try to use one driver for the same IP block, even if there are
>>> several revisions with small differences (for example the SAR ADC
>>> driver supports all SoC generations from Meson8 to G12A/G12B/SM1,
>>> because 80-90% of the code is shared across all revisions).
>>>
>>>
>>> Martin
>>>
>>> .
>>>
>>
>> Hi Martin,
>>
>> thanks for the comment.
>>
>> 1, G12A have usb2-phy0/usb2-phy1/usb3-phy0 three phys and an interrupt to support host/peripheral/otg modes.
>>    A1 has one usb2-phy0 phy and only support host mode.
> dwc3-meson-g12a treats PHYs as optional
> so if you only pass "usb2-phy0" and skip usb2-phy1/usb3-phy0 then it
> will still work fine
> (I didn't check whether the binding also reflects this)
> 
>> 2, G12A glue/phy drivers are for G12A SoCs, there are some diffrences to A1.
>>    G12A glue driver have dr_mode and interrupts two attributes to support otg mode while A1 hasn't this requirement.
> dwc3-meson-g12a ignores the interrupt for HOST-only mode
> (I didn't check whether the IRQ is optional in the dt-binding)
> 
>>    G12A glue driver has a hard coding vbus regulator code to support otg mode while A1 hasn't this requirement.
> my understanding is that whether a board has a VBUS regulator depends
> on the board design. it has nothing to do with the SoC itself
> 
>>    G12A glue driver has a hard coding support phys while A1 only supports host mode.
>>         enum {
>>                 USB2_HOST_PHY = 0,
>>                 USB2_OTG_PHY,
>>                 USB3_HOST_PHY,
>>                 PHY_COUNT,
>>                 };
> this goes together with comment #1 - you can skip USB2_OTG_PHY and
> USB3_HOST_PHY and the driver should still work fine
> 
>>    G12A glue driver only supports one clock while A1 needs four clocks.
> indeed, the dwc3-meson-g12a needs to be updated to support this
> I don't think that I have used it myself yet but there's the
> clk_bulk_data framework
> it seems to fit this use-case pretty well: define an arbitrary number
> of clocks for G12A/B an another set of clocks for A1 - then use the
> clk_bulk_data framework to enable/disable them all at once
> 
>>    G12A and A1 phy drivers have different register configurations since hardware differences.
> other drivers have similar requirements: (mostly) identical register
> layout but different values per SoC
> here are two examples (I'm not sure if they are good examples though):
> Lantiq/Intel SoC [0] and Allwinner SoCs [1]
> 
> I compared your driver with phy-meson-g12a-usb2 and only found four differences:
> 1) PHY_CTRL_R18_MPLL_DCO_CLK_SEL is set for A1
> 2) PHY_CTRL_R13_UPDATE_PMA_SIGNALS is not set for A1
> 3) PHY_CTRL_R21 is updated twice for A1 (once for earlier gen SoCs)
> 4) A1 doesn't reference the "xtal" clock
> 
> Difference 4) seems to be a general problem because there seems to be
> a PLL inside the PHY registers and that PLL must be fed by some input
> clock
> So I believe that there is some clock input (which is currently
> missing from your A1 USB2 PHY driver)
> 
>> 3, We have estimated these differences and we thought it's more clear and readable to have a dedicated glue/phy
>>    driver for A1 SoCs, so also dedicated dt-bindings.
> I think we should separate the driver and dt-bindings
> 
> Based on what I have seen so far my preference for the PHY is:
> - use the existing dt-binding, because it seems to be the same IP
> block with different register configuration
> - use the existing driver because there are only three different
> register values (to me it feels like a dedicated driver for these
> means more overhead for little benefit)
> 
> for the glue I think:
> - extend the existing dt-bindings and make some of the PHYs and the
> interrupt line optional. making the PHYs optional will be needed when
> adding GXL/GXM/AXG support anyways
> - use the existing driver and make the clock inputs depend on the SoC
> - everything else should already work as is
> 
> please let me know if I missed something:
> comparing/reviewing the new and existing drivers is harder than just
> copying the existing one and modifying that copy
> (this is one of the reasons why I think that duplicating code makes
> the drivers harder to maintain)
> 
> I also thought about the negative consequences of extending the
> existing driver(s).
> modifying the existing code could break the driver for existing boards.
> however, I think that is not a problem because BayLibre's Kernel CI
> labs have good coverage for G12A, G12B and SM1.
> so if you add some A1 boards there (or host your own lab with A1
> boards) any breakage will be found early (the Kernel CI bot even does
> git bisect and sends emails)
> 
> 
> Martin
> 
> 
> [0] https://github.com/torvalds/linux/blob/d2912cb15bdda8ba4a5dd73396ad62641af2f520/drivers/phy/lantiq/phy-lantiq-rcu-usb2.c#L47
> [1] https://github.com/torvalds/linux/blob/c942fddf8793b2013be8c901b47d0a8dc02bf99f/drivers/phy/allwinner/phy-sun4i-usb.c#L862
> 
> .
> 

Hi Martin:

Okay.
We will try to move A1 usb phy/ctrl driver into G12A driver and send a V2 patch later.

Thanks.
Hanjie Lin Nov. 27, 2019, 7:11 a.m. UTC | #6
On 2019/11/26 21:11, Neil Armstrong wrote:
> On 25/11/2019 23:02, Martin Blumenstingl wrote:
>> Hi Hanjie,
>>
>> On Mon, Nov 25, 2019 at 8:53 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
>>>
>>>
>>>
>>> On 2019/11/22 15:52, Martin Blumenstingl wrote:
>>>> Hello Hanjie,
>>>>
>>>> On Fri, Nov 22, 2019 at 7:55 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
>>>> [...]
>>>>>   dt-bindings: phy: Add Amlogic G12A USB2 PHY Bindings
>>>>>   dt-bindings: usb: dwc3: Add the Amlogic A1 Family DWC3 Glue Bindings
>>>>>   phy: amlogic: Add Amlogic A1 USB2 PHY Driver
>>>> drivers/phy/amlogic/phy-meson-g12a-usb2.c seems very similar to the A1
>>>> USB2 PHY you are introducing here.
>>>>
>>>>>   usb: dwc3: Add Amlogic A1 DWC3 glue
>>>> drivers/usb/dwc3/dwc3-meson-g12a.c is also very similar to the dwc3 glue.
>>>>
>>>> I have two questions:
>>>> - how is the PHY and the dwc3 glue different from G12A (or SM1)?
>>>> - why do we need a separate set of new drivers (instead of updating
>>>> the existing drivers)?
>>>>
>>>> We try to use one driver for the same IP block, even if there are
>>>> several revisions with small differences (for example the SAR ADC
>>>> driver supports all SoC generations from Meson8 to G12A/G12B/SM1,
>>>> because 80-90% of the code is shared across all revisions).
>>>>
>>>>
>>>> Martin
>>>>
>>>> .
>>>>
>>>
>>> Hi Martin,
>>>
>>> thanks for the comment.
>>>
>>> 1, G12A have usb2-phy0/usb2-phy1/usb3-phy0 three phys and an interrupt to support host/peripheral/otg modes.
>>>    A1 has one usb2-phy0 phy and only support host mode.
>> dwc3-meson-g12a treats PHYs as optional
>> so if you only pass "usb2-phy0" and skip usb2-phy1/usb3-phy0 then it
>> will still work fine
>> (I didn't check whether the binding also reflects this)
> 
> Exact, a simple match data could make max-phys to 1 for A1.
> 
>>
>>> 2, G12A glue/phy drivers are for G12A SoCs, there are some diffrences to A1.
>>>    G12A glue driver have dr_mode and interrupts two attributes to support otg mode while A1 hasn't this requirement.
>> dwc3-meson-g12a ignores the interrupt for HOST-only mode
>> (I didn't check whether the IRQ is optional in the dt-binding)
> 
> Interrupt support and OTG manual switch is optional and can be easily bypassed.
> 
>>
>>>    G12A glue driver has a hard coding vbus regulator code to support otg mode while A1 hasn't this requirement.
>> my understanding is that whether a board has a VBUS regulator depends
>> on the board design. it has nothing to do with the SoC itself
>>
>>>    G12A glue driver has a hard coding support phys while A1 only supports host mode.
>>>         enum {
>>>                 USB2_HOST_PHY = 0,
>>>                 USB2_OTG_PHY,
>>>                 USB3_HOST_PHY,
>>>                 PHY_COUNT,
>>>                 };
>> this goes together with comment #1 - you can skip USB2_OTG_PHY and
>> USB3_HOST_PHY and the driver should still work fine
> 
> Exact
> 
>>
>>>    G12A glue driver only supports one clock while A1 needs four clocks.
>> indeed, the dwc3-meson-g12a needs to be updated to support this
>> I don't think that I have used it myself yet but there's the
>> clk_bulk_data framework
>> it seems to fit this use-case pretty well: define an arbitrary number
>> of clocks for G12A/B an another set of clocks for A1 - then use the
>> clk_bulk_data framework to enable/disable them all at once
> 
> Exact, a simple conversion to clk_bulk_* would be enough
> 
>>
>>>    G12A and A1 phy drivers have different register configurations since hardware differences.
>> other drivers have similar requirements: (mostly) identical register
>> layout but different values per SoC
>> here are two examples (I'm not sure if they are good examples though):
>> Lantiq/Intel SoC [0] and Allwinner SoCs [1]
>>
>> I compared your driver with phy-meson-g12a-usb2 and only found four differences:
>> 1) PHY_CTRL_R18_MPLL_DCO_CLK_SEL is set for A1
>> 2) PHY_CTRL_R13_UPDATE_PMA_SIGNALS is not set for A1
>> 3) PHY_CTRL_R21 is updated twice for A1 (once for earlier gen SoCs)
>> 4) A1 doesn't reference the "xtal" clock
>>
>> Difference 4) seems to be a general problem because there seems to be
>> a PLL inside the PHY registers and that PLL must be fed by some input
>> clock
>> So I believe that there is some clock input (which is currently
>> missing from your A1 USB2 PHY driver)
> 
> These differences are trivial to add with a match data structure.
> 
>>
>>> 3, We have estimated these differences and we thought it's more clear and readable to have a dedicated glue/phy
>>>    driver for A1 SoCs, so also dedicated dt-bindings.
>> I think we should separate the driver and dt-bindings
>>
>> Based on what I have seen so far my preference for the PHY is:
>> - use the existing dt-binding, because it seems to be the same IP
>> block with different register configuration
>> - use the existing driver because there are only three different
>> register values (to me it feels like a dedicated driver for these
>> means more overhead for little benefit)
>>
>> for the glue I think:
>> - extend the existing dt-bindings and make some of the PHYs and the
>> interrupt line optional. making the PHYs optional will be needed when
>> adding GXL/GXM/AXG support anyways
>> - use the existing driver and make the clock inputs depend on the SoC
>> - everything else should already work as is
>>
>> please let me know if I missed something:
>> comparing/reviewing the new and existing drivers is harder than just
>> copying the existing one and modifying that copy
>> (this is one of the reasons why I think that duplicating code makes
>> the drivers harder to maintain)
>>
>> I also thought about the negative consequences of extending the
>> existing driver(s).
>> modifying the existing code could break the driver for existing boards.
>> however, I think that is not a problem because BayLibre's Kernel CI
>> labs have good coverage for G12A, G12B and SM1.
>> so if you add some A1 boards there (or host your own lab with A1
>> boards) any breakage will be found early (the Kernel CI bot even does
>> git bisect and sends emails)
> The overall architecture is the same since the GXL SoCs, we also plan to
> move the GXL/GXM USB complex into the G12A usb-ctrl driver because the architecture
> is very similar.
> 
> For A1, the changes will be quite minimal, please try and post a RFC version so we
> can evaluate.
> 
> Thanks,
> 
> Neil
> 
>>
>>
>> Martin
>>
>>
>> [0] https://github.com/torvalds/linux/blob/d2912cb15bdda8ba4a5dd73396ad62641af2f520/drivers/phy/lantiq/phy-lantiq-rcu-usb2.c#L47
>> [1] https://github.com/torvalds/linux/blob/c942fddf8793b2013be8c901b47d0a8dc02bf99f/drivers/phy/allwinner/phy-sun4i-usb.c#L862
>>
> 
> .
> 


Hi Neil:

Okay.
We will try to move A1 usb phy/ctrl driver into G12A driver and send a V2 patch later.

Thanks.