- 
                Notifications
    
You must be signed in to change notification settings  - Fork 8.2k
 
drivers: udc_dwc2: Add nRF54L20 UDC DWC2 quirks #80384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| #endif /*DT_HAS_COMPAT_STATUS_OKAY(st_stm32f4_fsotg) */ | ||
| 
               | 
          ||
| #if DT_HAS_COMPAT_STATUS_OKAY(nordic_nrf_usbhs) | ||
| #if DT_HAS_COMPAT_STATUS_OKAY(nordic_nrf_usbhs_nrfs) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no point in renaming it or postfixing it with -nrfs. And there is no reason to break someone else's work with a compatible rename. You will need to communicate with us internally to find a suitable compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a direct need for solving the compatible naming issue. Both nRF54H20 and nRF54L20 do have USBHS peripheral. Yet, the nRF54H20 requires nrfs to get information about VBUS and nRF54L20 has USBREG peripheral for that. Therefore I think the nordic,nrf-usbhs-nrfs and nordic,nrf-usbhs-usbreg is probably the cleanest approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no strong opinion, although I think the -nrfs suffix makes the difference clear. I'll leave it up to your preference, just please confirm that this is what you want after reading Tomasz's comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced to approve changes that break other people's work for no reason. "nordic, nrf-usbhs" should not be renamed. Also, new compatilbe needs a better name, something like "nordic, nrf-usbhs-fancy-soc-family". (nrfs is some software, usbreg is ambiguous)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the nordic,nrf-usbhs change and the nRF54L20 now has nordic,nrf-usbhs-nrf54l compatible instead.
        
          
                drivers/usb/udc/Kconfig.dwc2
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can not see any justification why 8-bit should be preferred. And Kconfig is just the wrong place to configure it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is UTMI+ PHY capable of both 8-bit and 16-bit operation connected to DWC2 controller then there is very little need to use 8-bit (clocking is different between the two, so there hardware behavior changes slightly).
However, it is also possible that there is only 8-bit capable UTMI+ PHY connected to DWC2 controller that supports both 8-bit and 16-bit. In such case this is a killer feature (because 16-bit simply won't work).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing can be move within #if DT_HAS_COMPAT_STATUS_OKAY(x) || DT_HAS_COMPAT_STATUS_OKAY(y) ... #endif and reused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant || instead of &&.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have moved the common parts to a common #if
f00d54a    to
    09fe93b      
    Compare
  
    | 
           @tmon-nordic @jfischer-no could you take a look?  | 
    
        
          
                drivers/usb/udc/udc_dwc2.c
              
                Outdated
          
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think LOG_DBG("Using 8-bit UTMI+"); would be more appropriate here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
901e981    to
    9a34724      
    Compare
  
    | 
           It looks like the tests are consistently failing in due to which is weird, because I haven't done anything with UARTE. Has adding a new node to DTS enabled some new tests in twister?  | 
    
| 
           CI is green. @tmon-nordic @jfischer-no @masz-nordic could you review?  | 
    
          
 Ping  | 
    
Added USBHS device tree node for nRF54L20. Signed-off-by: Rafał Kuźnia <[email protected]>
The nRF54L20 does not have the NRFS layer and the set of quirks will be different than in nRF54H20. Added new set of quirks for the nRF54L20 SoC. Signed-off-by: Rafał Kuźnia <[email protected]>
| 
           I have removed the commit that introduced   | 
    
| 
           This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.  | 
    
| 
           Initial quirks implementation with the changes necessary to work on actual silicon is downstream at nrfconnect/sdk-zephyr#2770 Support will be upstreamed together with rest of nRF54L20 support.  | 
    
PR consists of 3 changes: