RE: [PATCH 2/2] dt-bindings: usb: cdns,usb3: Add support for USBSSP

[Pawel Laszczak]

>
>On 26-03-02 10:27:11, Krzysztof Kozlowski wrote:
>> >>>  maintainers:
>> >>>    - Pawel Laszczak <pawell@xxxxxxxxxxx>
>> >>>
>> >>> +description:
>> >>> +  Cadence USB dual-role controllers. USBSS (cdns,usb3) supports
>> >>> +up to
>> >>> +  SuperSpeed (USB 3.0). USBSSP (cdns,usbssp) is the next
>> >>> +generation with
>> >>> +  SuperSpeed Plus (USB 3.1 gen2x1) and XHCI-based device
>> >>> +controller. Both
>> >>> +  share the same register layout and resource model.
>> >>
>> >> So are compatible or not?
>> >>
>> >
>> > Sorry for the misleading description. They are NOT fully compatible.
>> > The register layout (OTG/XHCI/Device) and interrupts
>> > (OTG/XHCI/Device/Wakeup) are the same, but register contents are
>>
>> Layout cannot be the same if contents is different. Same layout means
>> same register is at the same place. If you have different register
>> with different contents at given place, how is it "same layout"?
>
>Sorry. I mean the USBSS and USBSSP share the same resource model (three
>memory regions for OTG, XHCI and Device, plus three to four interrupts). But in
>each region, eg in OTG region, the layout for each register are different for both
>controllers.
>
>Pawel, I think we could try Krzysztof's suggestion and differentiating USBSS IP and
>USBSSP IP at runtime, we could use DID register (cdns->version) to do that and
>avoid adding new IP general binding doc. What do you think so?
>
>Peter
>
>>
>> > different, esp, the device (gadget) controllers are architecturally different:
>> >
>> > - USBSS uses a custom gadget controller (cdns3_gadget_init)
>> > - USBSSP uses an XHCI-based gadget controller (cdnsp_gadget_init)
>>
>> You just described drivers, so this does not convince me at all.
>>
>> >
>> > I will fix the description in v2 to clearly state this difference.
>> >
>> >>> +
>> >>>  properties:
>> >>>    compatible:
>> >>> -    const: cdns,usb3
>> >>> +    enum:
>> >>> +      - cdns,usb3
>> >>> +      - cdns,usbssp
>> >>
>> >> Why do we need another generic compatible?
>> >>
>> >> And why do you add it now to each of device schemas using this one?
>>
>> You did not respond to this one. Look how this schema is used.
>>
>> >
>> > Like explain above, the USBSSP has a different device/gadget
>> > controller architecture from USBSS. The platform driver uses the
>> > compatible string to select the correct gadget init function:
>>
>> Again, driver stuff.
>>
>> >
>> >   if (device_get_match_data(dev) == &cdnsp_plat)
>> >       cdns->gadget_init = cdnsp_gadget_init;
>> >   else
>> >       cdns->gadget_init = cdns3_gadget_init;
>> >
>> > Without a distinct compatible, the driver cannot know which gadget
>> > controller is present. This is a Cadence IP-level distinction (not
>> > SoC-specific), so a generic compatible seems appropriate here. But
>> > please let me know if you'd prefer a different approach.
>>
>> Generic compatibles are almost never appropriate and driver code
>> rarely convinces.
>>
>> Best regards,
>> Krzysztof

The Device (Peripheral) controller in USBSSP is not just an evolution
of USBSS; it is a completely different IP core with a distinct register
map.
While the Host (XHCI) and OTG/DRD portions overlap significantly, the
Device register space in USBSSP is architecturally incompatible with USBSS. 
They are different IP cores integrated into the same subsystem:

The register map for the Device controller in USBSSP is entirely
different. Offsets that are valid for one controller point to different
functions or reserved space in the other. Using a single compatible string
would imply register-level compatibility that does not exist.
USBSS uses a custom DMA-based peripheral controller, whereas USBSSP uses
an entirely different XHCI-based architecture for the device side.

These two controllers do not provide a reliable way (e.g., via registers)
to distinguish between them in every case.
We should treat them as two separate controllers that happen to share
some common code, rather than viewing USBSSP simply as a newer
version of USBSS.

I believe Peter's intention was to use a common DT binding and a single
platform driver, as the resource model and parts of the code are nearly
identical.
However, given the architectural differences on the device side,
I believe having distinct compatible strings is the most accurate way to
describe the hardware. It prevents the driver from making "blind" accesses
to registers that have different meanings depending on the underlying IP.

Maybe to resolve this, we should have separate DT binding documentation
and potentially separate platform drivers to reflect this architectural
split?

Krzysztof, what do you think about this approach?

>
>--
>
>Best regards,
>Peter