dts: Add initial support of USB PHYs

[ydamigos]

Add initial support of USB PHYs.

[ydamigos]

@jli157 FYI

[codecov-io]

Codecov Report

Merging #10359 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #10359   +/-   ##
=======================================
  Coverage   48.37%   48.37%           
=======================================
  Files         265      265           
  Lines       42188    42188           
  Branches    10137    10137           
=======================================
  Hits        20408    20408           
  Misses      17703    17703           
  Partials     4077     4077

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a933297...918015b. Read the comment docs.

[galak]

When phys has multiple handles (like on stm32f723.dtsi) what does that mean?

[ydamigos]

When phys has multiple handles (like on stm32f723.dtsi) what does that mean?

The available PHYs for the USB controller. In case of stm32f723 the available PHYs are:
– An on-chip full-speed PHY
– A UTMI interface for internal HS PHY

[erwango]

@ydamigos , looks nice, but it is not clear to me what issue this addition is going to solve, and no application is provided to demonstrate.
Any pointer on a discussion that could help to understand the context?

[ydamigos]

Any pointer on a discussion that could help to understand the context?

@erwango It needs PR #9062 to get merged. Then, we could use DT to get hw information for USBPHYC and not stm32cube in stm32 usb driver.

[galak]

@erwango It needs PR #9062 to get merged. Then, we could use DT to get hw information for USBPHYC and not stm32cube in stm32 usb driver.

merged #9062 :)

[aurel32]

Thanks @ydamigos for working on that. I have to say I don't necessary understand all the logic parsing the device tree to produce an header file so I still find a bit difficult to understand how the new DT entries are going to be used in practice. Anyway I already have a few comments/questions:

• The STM32F723 OTG_HS interface only has a single HS PHY (that can work in FS mode). The documentation about HS PHY is quite confusing, and even wrong in some aspects. Therefore I believe the usbotg_hs phys entry has to be changed to:
  phys = <&usbphyc>;
• The usbotg_fs_phy basically has no address and no property, so the same is used for both the usbotg_hs and usbotg_fs instances. Is it a problem? In other part of the device tree we tend to have one instance per device, post-fixed with a number.
• I guess in the future we can add support for an external PHY in the device tree, which in that case will be outside of the soc hierarchy. Is that correct?
• When compiling with your patches applied (after fixing the conflicts), I get the following warnings:

stm32f723e_disco.dts_compiled: Warning (phys_property): /soc/usb@50000000: Missing property '#phy-cells' in node /soc/usbphy or bad phandle (referred from phys[0])
stm32f723e_disco.dts_compiled: Warning (phys_property): /soc/usb@40040000: Missing property '#phy-cells' in node /soc/usbphy or bad phandle (referred from phys[0])

[ydamigos]

@aurel32 Thanks for the review

The STM32F723 OTG_HS interface only has a single HS PHY (that can work in FS mode). The documentation about HS PHY is quite confusing, and even wrong in some aspects. Therefore I believe the usbotg_hs phys entry has to be changed to:
phys = <&usbphyc>;

You are right. I updated it.

The usbotg_fs_phy basically has no address and no property, so the same is used for both the usbotg_hs and usbotg_fs instances. Is it a problem? In other part of the device tree we tend to have one instance per device, post-fixed with a number.

Yes, I should use a different one. It is a different embedded phy in a different controller.

I guess in the future we can add support for an external PHY in the device tree, which in that case will be outside of the soc hierarchy. Is that correct?

Yes, on board level.

When compiling with your patches applied (after fixing the conflicts)

I have seen the warnings and I'll fix them. I thought #phy-cells was used in linux only but it is needed by DT compiler.

[ydamigos]

When phys has multiple handles (like on stm32f723.dtsi) what does that mean?

I had a better look at linux phy bindings and a controller has multiple phys if it can use simultaneously.

[erwango]

Great work.
Only adaptation would be to use CONFIG_DT_COMPAT_XX flags instead of CONFIG_XXX_BASE_ADDRESS

[erwango]

We're now generating CONFIG_DT_COMPAT_XXXX flags (more details here: #9406).
These should be used for driver specific implementations/variations. Since you've set the ground with phys nodes it would fit perfectly here.
Please use this instead of 'CONFIG_USBPHYC_BASE_ADDRESS'.
Apply everywhere.

[ydamigos]

I changed it to DT_COMPAT_ST_STM32_USBPHYC.

[galak]

Curious, why the use of "zephyr,usb-nop-xceiv" as a compatible (specifically the 'zephyr' bit?)

[ydamigos]

Curious, why the use of "zephyr,usb-nop-xceiv" as a compatible (specifically the 'zephyr' bit?)

@galak I borrowed the idea from linux, where usb-nop-xceiv is used by all the usb transceivers which are built-in with USB IP. I added the 'zephyr' part to highlight that it is a zephyr "implementation".

[galak]

@galak I borrowed the idea from linux, where usb-nop-xceiv is used by all the usb transceivers which are built-in with USB IP. I added the 'zephyr' part to highlight that it is a zephyr "implementation".

what compat does linux use?

[ydamigos]

what compat does linux use?

usb-nop-xceiv

[galak]

usb-nop-xceiv

Any reason not to use the same, is there some zephyr specific semantic we have?

[ydamigos]

Any reason not to use the same, is there some zephyr specific semantic we have?

No, there is nothing zephyr specific. I changed it to usb-nop-xceiv.