[0/5] Exynos EHCI/OHCI: resolve conflict with the generic USB device bindings
mbox series

Message ID 20190521115849.9882-1-m.szyprowski@samsung.com
Headers show
Series
  • Exynos EHCI/OHCI: resolve conflict with the generic USB device bindings
Related show

Message

Marek Szyprowski May 21, 2019, 11:58 a.m. UTC
Dear All,

Commit 69bec7259853 ("USB: core: let USB device know device node") added
support for attaching devicetree node for USB devices. Those nodes are
children of their USB host controller. However Exynos EHCI and OHCI
driver bindings already define child-nodes for each physical root hub
port and assigns respective PHY controller and parameters to them. This
leads to the conflict. A workaround for it has been merged as commit
01d4071486fe ("usb: exynos: add workaround for the USB device bindings
conflict"), but it disabled support for USB device binding for Exynos
EHCI/OHCI controllers.

This patchset tries to resolve this binding conflict by changing Exynos
EHCI/OHCI bindings: PHYs are moved from the sub-nodes to a standard array
under the 'phys' property. Such solution has been suggested by Måns
Rullgård in the following thread: https://lkml.org/lkml/2019/5/13/228

To keep everything working during the transitional time, the changes has
been split into 2 steps. First step (patches 1-3) need to be merged before
the second one (patches 4-5). Patches from each step can be merged to
respective trees without any dependencies - the only requirement is that
second step has to be merged after merging all patches from the first one.

This patchset has been tested on various Exynos4 boards with different
USB host controller configurations (Odroids family: X2, U3, XU3).

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Marek Szyprowski (5):
  dt-bindings: switch Exynos EHCI/OHCI bindings to use array of generic
    PHYs
  ARM: dts: exynos: Add array of generic PHYs to EHCI/OHCI devices
  usb: exynos: add support for getting PHYs from the standard dt array
  ARM: dts: exynos: Remove obsolete port sub-nodes from EHCI/OHCI
    devices
  usb: exynos: Remove support for legacy PHY bindings

 .../devicetree/bindings/usb/exynos-usb.txt    | 41 ++++++----------
 arch/arm/boot/dts/exynos4.dtsi                | 28 ++---------
 .../boot/dts/exynos4210-universal_c210.dts    |  8 +---
 arch/arm/boot/dts/exynos4412-itop-elite.dts   |  9 +---
 arch/arm/boot/dts/exynos4412-odroidu3.dts     |  8 +---
 arch/arm/boot/dts/exynos4412-odroidx.dts      |  5 +-
 arch/arm/boot/dts/exynos4412-origen.dts       |  9 +---
 arch/arm/boot/dts/exynos5250.dtsi             | 16 ++-----
 arch/arm/boot/dts/exynos54xx.dtsi             | 18 ++-----
 drivers/usb/host/ehci-exynos.c                | 48 +++----------------
 drivers/usb/host/ohci-exynos.c                | 48 +++----------------
 11 files changed, 50 insertions(+), 188 deletions(-)

Comments

Måns Rullgård May 21, 2019, 1:30 p.m. UTC | #1
Marek Szyprowski <m.szyprowski@samsung.com> writes:

> Dear All,
>
> Commit 69bec7259853 ("USB: core: let USB device know device node") added
> support for attaching devicetree node for USB devices. Those nodes are
> children of their USB host controller. However Exynos EHCI and OHCI
> driver bindings already define child-nodes for each physical root hub
> port and assigns respective PHY controller and parameters to them. This
> leads to the conflict. A workaround for it has been merged as commit
> 01d4071486fe ("usb: exynos: add workaround for the USB device bindings
> conflict"), but it disabled support for USB device binding for Exynos
> EHCI/OHCI controllers.
>
> This patchset tries to resolve this binding conflict by changing Exynos
> EHCI/OHCI bindings: PHYs are moved from the sub-nodes to a standard array
> under the 'phys' property. Such solution has been suggested by Måns
> Rullgård in the following thread: https://lkml.org/lkml/2019/5/13/228
>
> To keep everything working during the transitional time, the changes has
> been split into 2 steps. First step (patches 1-3) need to be merged before
> the second one (patches 4-5). Patches from each step can be merged to
> respective trees without any dependencies - the only requirement is that
> second step has to be merged after merging all patches from the first one.
>
> This patchset has been tested on various Exynos4 boards with different
> USB host controller configurations (Odroids family: X2, U3, XU3).
>
> Best regards
> Marek Szyprowski
> Samsung R&D Institute Poland
>
> Marek Szyprowski (5):
>   dt-bindings: switch Exynos EHCI/OHCI bindings to use array of generic
>     PHYs
>   ARM: dts: exynos: Add array of generic PHYs to EHCI/OHCI devices
>   usb: exynos: add support for getting PHYs from the standard dt array
>   ARM: dts: exynos: Remove obsolete port sub-nodes from EHCI/OHCI
>     devices
>   usb: exynos: Remove support for legacy PHY bindings

You could retain compatibility with old devicetrees (which may be
useful) by using the "phys" property if it exists and falling back
on the old method if it doesn't.  Then you would get this sequence
of changes:

1. Update binding definition.
2. Support new binding in driver, with fallback to old.
3. Switch dts files to new binding.
Marek Szyprowski May 22, 2019, 6:01 a.m. UTC | #2
Hi Måns

On 2019-05-21 15:30, Måns Rullgård wrote:
> Marek Szyprowski <m.szyprowski@samsung.com> writes:
>> Dear All,
>>
>> Commit 69bec7259853 ("USB: core: let USB device know device node") added
>> support for attaching devicetree node for USB devices. Those nodes are
>> children of their USB host controller. However Exynos EHCI and OHCI
>> driver bindings already define child-nodes for each physical root hub
>> port and assigns respective PHY controller and parameters to them. This
>> leads to the conflict. A workaround for it has been merged as commit
>> 01d4071486fe ("usb: exynos: add workaround for the USB device bindings
>> conflict"), but it disabled support for USB device binding for Exynos
>> EHCI/OHCI controllers.
>>
>> This patchset tries to resolve this binding conflict by changing Exynos
>> EHCI/OHCI bindings: PHYs are moved from the sub-nodes to a standard array
>> under the 'phys' property. Such solution has been suggested by Måns
>> Rullgård in the following thread: https://lkml.org/lkml/2019/5/13/228
>>
>> To keep everything working during the transitional time, the changes has
>> been split into 2 steps. First step (patches 1-3) need to be merged before
>> the second one (patches 4-5). Patches from each step can be merged to
>> respective trees without any dependencies - the only requirement is that
>> second step has to be merged after merging all patches from the first one.
>>
>> This patchset has been tested on various Exynos4 boards with different
>> USB host controller configurations (Odroids family: X2, U3, XU3).
>>
>> Best regards
>> Marek Szyprowski
>> Samsung R&D Institute Poland
>>
>> Marek Szyprowski (5):
>>    dt-bindings: switch Exynos EHCI/OHCI bindings to use array of generic
>>      PHYs
>>    ARM: dts: exynos: Add array of generic PHYs to EHCI/OHCI devices
>>    usb: exynos: add support for getting PHYs from the standard dt array
>>    ARM: dts: exynos: Remove obsolete port sub-nodes from EHCI/OHCI
>>      devices
>>    usb: exynos: Remove support for legacy PHY bindings
> You could retain compatibility with old devicetrees (which may be
> useful) by using the "phys" property if it exists and falling back
> on the old method if it doesn't.  Then you would get this sequence
> of changes:
>
> 1. Update binding definition.
> 2. Support new binding in driver, with fallback to old.
> 3. Switch dts files to new binding.

This is exactly what I did in this patchset. Until Patch #5 is applied, 
Exynos EHCI/OHCI drivers supports both ways of getting PHYs and is fully 
compatible with existing DTBs. This last patch should be applied at 
least one release later that the first 3 patches to keep everything 
working during the -rcX time.

Compatibility with so called old DTBs is not so important, because there 
are no boards with Exynos4 and Exynos5 SoCs, which would not update DTB 
together with the kernel zImage. There have been already some 
significant compatibility breaks related to those SoCs during last years.


Best regards
Måns Rullgård May 22, 2019, 10:54 a.m. UTC | #3
Marek Szyprowski <m.szyprowski@samsung.com> writes:

> Hi Måns
>
> On 2019-05-21 15:30, Måns Rullgård wrote:
>> Marek Szyprowski <m.szyprowski@samsung.com> writes:
>>> Dear All,
>>>
>>> Commit 69bec7259853 ("USB: core: let USB device know device node") added
>>> support for attaching devicetree node for USB devices. Those nodes are
>>> children of their USB host controller. However Exynos EHCI and OHCI
>>> driver bindings already define child-nodes for each physical root hub
>>> port and assigns respective PHY controller and parameters to them. This
>>> leads to the conflict. A workaround for it has been merged as commit
>>> 01d4071486fe ("usb: exynos: add workaround for the USB device bindings
>>> conflict"), but it disabled support for USB device binding for Exynos
>>> EHCI/OHCI controllers.
>>>
>>> This patchset tries to resolve this binding conflict by changing Exynos
>>> EHCI/OHCI bindings: PHYs are moved from the sub-nodes to a standard array
>>> under the 'phys' property. Such solution has been suggested by Måns
>>> Rullgård in the following thread: https://lkml.org/lkml/2019/5/13/228
>>>
>>> To keep everything working during the transitional time, the changes has
>>> been split into 2 steps. First step (patches 1-3) need to be merged before
>>> the second one (patches 4-5). Patches from each step can be merged to
>>> respective trees without any dependencies - the only requirement is that
>>> second step has to be merged after merging all patches from the first one.
>>>
>>> This patchset has been tested on various Exynos4 boards with different
>>> USB host controller configurations (Odroids family: X2, U3, XU3).
>>>
>>> Best regards
>>> Marek Szyprowski
>>> Samsung R&D Institute Poland
>>>
>>> Marek Szyprowski (5):
>>>    dt-bindings: switch Exynos EHCI/OHCI bindings to use array of generic
>>>      PHYs
>>>    ARM: dts: exynos: Add array of generic PHYs to EHCI/OHCI devices
>>>    usb: exynos: add support for getting PHYs from the standard dt array
>>>    ARM: dts: exynos: Remove obsolete port sub-nodes from EHCI/OHCI
>>>      devices
>>>    usb: exynos: Remove support for legacy PHY bindings
>> You could retain compatibility with old devicetrees (which may be
>> useful) by using the "phys" property if it exists and falling back
>> on the old method if it doesn't.  Then you would get this sequence
>> of changes:
>>
>> 1. Update binding definition.
>> 2. Support new binding in driver, with fallback to old.
>> 3. Switch dts files to new binding.
>
> This is exactly what I did in this patchset. Until Patch #5 is applied, 
> Exynos EHCI/OHCI drivers supports both ways of getting PHYs and is fully 
> compatible with existing DTBs. This last patch should be applied at 
> least one release later that the first 3 patches to keep everything 
> working during the -rcX time.

I'm suggesting you keep the fallback in the driver.  It does no harm,
and it's contained in one place.

On the dts side, you're adding the new phys property without removing
the old-style nodes at first.  If you put the driver change first, the
dts could be switched to the new style in one patch without a confusing
hybrid ever existing.

> Compatibility with so called old DTBs is not so important, because there 
> are no boards with Exynos4 and Exynos5 SoCs, which would not update DTB 
> together with the kernel zImage. There have been already some 
> significant compatibility breaks related to those SoCs during last years.

You can't possibly know what's out there.  Besides, isn't the general
policy to not break compatibility without a very good reason?
Marek Szyprowski June 5, 2019, 8:37 a.m. UTC | #4
Hi Måns,

On 2019-05-22 12:54, Måns Rullgård wrote:
> Marek Szyprowski <m.szyprowski@samsung.com> writes:
>> On 2019-05-21 15:30, Måns Rullgård wrote:
>>> Marek Szyprowski <m.szyprowski@samsung.com> writes:
>>>> Commit 69bec7259853 ("USB: core: let USB device know device node") added
>>>> support for attaching devicetree node for USB devices. Those nodes are
>>>> children of their USB host controller. However Exynos EHCI and OHCI
>>>> driver bindings already define child-nodes for each physical root hub
>>>> port and assigns respective PHY controller and parameters to them. This
>>>> leads to the conflict. A workaround for it has been merged as commit
>>>> 01d4071486fe ("usb: exynos: add workaround for the USB device bindings
>>>> conflict"), but it disabled support for USB device binding for Exynos
>>>> EHCI/OHCI controllers.
>>>>
>>>> This patchset tries to resolve this binding conflict by changing Exynos
>>>> EHCI/OHCI bindings: PHYs are moved from the sub-nodes to a standard array
>>>> under the 'phys' property. Such solution has been suggested by Måns
>>>> Rullgård in the following thread: https://lkml.org/lkml/2019/5/13/228
>>>>
>>>> To keep everything working during the transitional time, the changes has
>>>> been split into 2 steps. First step (patches 1-3) need to be merged before
>>>> the second one (patches 4-5). Patches from each step can be merged to
>>>> respective trees without any dependencies - the only requirement is that
>>>> second step has to be merged after merging all patches from the first one.
>>>>
>>>> This patchset has been tested on various Exynos4 boards with different
>>>> USB host controller configurations (Odroids family: X2, U3, XU3).
>>>>
>>>> Best regards
>>>> Marek Szyprowski
>>>> Samsung R&D Institute Poland
>>>>
>>>> Marek Szyprowski (5):
>>>>     dt-bindings: switch Exynos EHCI/OHCI bindings to use array of generic
>>>>       PHYs
>>>>     ARM: dts: exynos: Add array of generic PHYs to EHCI/OHCI devices
>>>>     usb: exynos: add support for getting PHYs from the standard dt array
>>>>     ARM: dts: exynos: Remove obsolete port sub-nodes from EHCI/OHCI
>>>>       devices
>>>>     usb: exynos: Remove support for legacy PHY bindings
>>> You could retain compatibility with old devicetrees (which may be
>>> useful) by using the "phys" property if it exists and falling back
>>> on the old method if it doesn't.  Then you would get this sequence
>>> of changes:
>>>
>>> 1. Update binding definition.
>>> 2. Support new binding in driver, with fallback to old.
>>> 3. Switch dts files to new binding.
>> This is exactly what I did in this patchset. Until Patch #5 is applied,
>> Exynos EHCI/OHCI drivers supports both ways of getting PHYs and is fully
>> compatible with existing DTBs. This last patch should be applied at
>> least one release later that the first 3 patches to keep everything
>> working during the -rcX time.
> I'm suggesting you keep the fallback in the driver.  It does no harm,
> and it's contained in one place.
>
> On the dts side, you're adding the new phys property without removing
> the old-style nodes at first.  If you put the driver change first, the
> dts could be switched to the new style in one patch without a confusing
> hybrid ever existing.

This was just a proposed way of applying the patches. We can change the 
order and apply patch #3 first, then in the next kernel release, apply 
patch #2 and #4 together, and the last step, 2 releases later, apply the 
last one. In my proposed approach (apply #2 and #3 together to the 
respective kernel trees for the next release), the final result is 
applied a release earlier.

>> Compatibility with so called old DTBs is not so important, because there
>> are no boards with Exynos4 and Exynos5 SoCs, which would not update DTB
>> together with the kernel zImage. There have been already some
>> significant compatibility breaks related to those SoCs during last years.
> You can't possibly know what's out there.  Besides, isn't the general
> policy to not break compatibility without a very good reason?

There have been already some significant changes and compatibility 
breaks in Exynos DTB ABI and noone complained. We can also ignore 
completely this patchset and keep compatibility with old DTBs just with 
the workaround merged in commit 01d4071486fe18ec91f78725d81c7e46557c629a 
("usb: exynos: add workaround for the USB device bindings conflict")...

Best regards