Re: [PATCH v5 1/9] dt-bindings: usb: Add Type-C switch binding

[Krzysztof Kozlowski]

On 24/06/2022 23:41, Prashant Malani wrote:
> On Fri, Jun 24, 2022 at 12:50 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>>
>> Quoting Prashant Malani (2022-06-23 19:48:04)
>>> On Thu, Jun 23, 2022 at 7:13 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>>>>
>>>> Quoting Prashant Malani (2022-06-23 17:35:38)
>>>>> On Thu, Jun 23, 2022 at 4:14 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>>>>>>
>>>>>> I'm not aware of any documentation for the dos and don'ts here. Are
>>>>>> there any examples in the bindings directory that split up a device into
>>>>>> subnodes that isn't in bindings/mfd?
>>>>>
>>>>> usb-c-connector [3] and its users is an example.
>>>>
>>>> What are the subnodes? The graph ports? That is not what I meant.
>>>
>>> cros-ec-typec [4] uses subnodes of usb-c-connector. Chrome OS DTs
>>> use the ports from the included usb-c-connector to switching hardware.
>>
>> Ok, got it. usb-c-connector nodes are children of the typec controller
>> (in this case cros-ec-typec) because otherwise we would need to make a
>> phandle link from the usb-c-connector node(s) under the root node / to
>> the typec controller. The phandle link may need to be done in both
>> directions, so it makes more sense to put the usb-c-connector nodes
>> underneath the typec controller to express the direct relationship
>> between the typec controller and the usb-c-connectors.
>>
>> Furthermore, the usb-c-connector is not integrated as part of the EC in
>> the same package. There is a discrete part placed on the board that
>> corresponds to the usb-c-connector and that is separate from the EC. The
>> connectors are in essence only controllable through the EC because
>> that's the typec controller.
> 
> From the perspective of the AP, the `usb-c-connector` *is* an integrated part of
> the Chrome EC; there is no alternative way to control it except
> through the Chrome EC.
> So the above example reinforces the usage model for typec-switch (which is
> also an "integrated" component).
> 
>> It's similar to how we place i2c devices as
>> child nodes of the i2c controller.
>>
>>>
>>>> I meant splitting up a device functionality, like type-c and display
>>>> bridge, into subnodes. Composition of devices through DT bindings isn't
>>>> how it's done. Instead, we dump all the different functionality into the
>>>> same node. For example, look at the number of bindings that have both
>>>> #clock-cells and #reset-cells, when those are distinct frameworks in the
>>>> kernel and also different properties. We don't make subnodes to contain
>>>> the different functionality of a device.
>>>>
>>>> And in this case I still don't see the point to making a subnode.
>>>
>>> I've already provided my best effort at explaining the rationale.
>>>
>>>> The
>>>> API can simply setup a type-c switch based on a graph binding for the
>>>> toplevel node, e.g. the drm-bridge, and the driver can tell the API
>>>> which port+endpoint to use to search the graph for the usb-c-connector
>>>> to associate with the switch.
>>>
>>> OK, drm-bridge uses that approach. This is another approach. I didn't fully
>>> understand why we *have* to follow what drm-bridge is doing.
>>>
>>>> We don't need to connect the graph within
>>>> the drm-bridge node to the graph within the typec-switch node to do
>>>> that. That's an internal detail of the drm-bridge that we don't expose
>>>> to DT, because the driver knows the detail.
>>>
>>> I still don't understand why we can't do that. These devices have actual
>>> hardware blocks that represent the Type-C switch functionality.
>>>
>>
>> We don't break up device functionality for an IC into different subnodes
>> with different compatibles. Similarly, we don't describe internal
>> connection details of device nodes. The device driver that binds to the
>> compatible should know the details of the internal block diagram of the
>> part.
> 
> I don't completely agree with the above. There
> is scope for middle-ground where some details can be codified into
> DT bindings, and the driver can have the flexibility to be able to handle them.
> But this now devolves into an ideological debate which I don't want
> to get involved in, so I will restrict my responses on this subject.
> 
>> The DT binding should describe the external connections of the
>> part and have properties that inform the driver about how the part was
>> integrated into the system (e.g. mode-switch). The unwritten DT mantra
>> is "less is more".
>>
>> We could definitely make many subnodes and add properties for everything
>> inside an IC so that the DT describes the complete block diagram of the
>> part, but at that point the driver is a shell of its former self.
> 
> That is a pathological/extreme argument which is not the case here,
> we're just adding 1 sub-node because it's a sub-component that interfaces
> with a kernel framework (Type-C class etc). The driver should be able to deal
> with varying hardware configurations for the device and I don't believe that
> makes it a "shell of its former self" any more than hard-coding port
> details in the driver.
> 
>> The driver will spend time parsing properties to learn details that are
> 
> This parsing only occurs 1 once at probe, so I don't consider it much
> of an overhead. The alternative suggested leads to the driver using time
> looking up OF ports (with the port number). I fail to see how either is
> noticeably more efficient than the other, especially on modern systems.
> 
>> entirely unchanging for the lifetime of the device (e.g. that the device
>> has typec switch capabilities); things that should be hard-coded in the
>> driver.
>>
>> Of course, if the device is integrated into the system and doesn't need
>> to perform typec switching, then we want a property to tell the driver
>> that this device is integrated in a way that the typec switch is not
>> needed/used. Basically the driver should key that functionality off of
>> the presence of the 'mode-switch' or 'orientation-switch' property
>> instead of off the presence of a typec-switch subnode.
>>
>>>>>
>>>>>>
>>>>>> How would I even know which two differential pairs correspond to port0
>>>>>> or port1 in this binding in the ITE case?
>>>>>
>>>>> Why do we need to know that? It doesn't affect this or the other
>>>>> driver or hardware's
>>>>> functioning in a perceivable way.
>>>>
>>>> If the device registers allow control of the DP lane to physical pin
>>>> mapping, so that DP lane0 and DP lane1 can be swapped logically, then
>>>> we'll want to know which DP lanes we need to swap by writing some lane
>>>> remapping register in the device. Sometimes for routing purposes devices
>>>> support this lane remapping feature so the PCB can route the lines
>>>> directly to the connector instead of going in circles and destroying the
>>>> signal integrity.
>>>
>>> Then add more end-points to port@1 (for each differential pair
>>> you want to describe) of the usb-c-connector and route them
>>> to the typec-switch accordingly.
>>> FWIW I'm not aware of h/w *that supports DP alt mode* that uses the
>>> functionality
>>> you're referring to.
>>>
>>
>> The Qualcomm QMP usb+dp phy supports lane remapping.
> 
> Ok great. So we can follow the method described above for specifying these
> differential pairs if required. That is not related to this patch
> series (although it is compatible
> with it).
> 
>>
>>>>
>>>>>
>>>>>> Is that why you're proposing this binding? To
>>>>>> avoid describing a graph binding in the usb-c-connector and effectively
>>>>>> "pushing" the port count up to the mux?
>>>>>
>>>>> No, that is not the intention behind this series. The
>>>>> 'usb-c-connector' still needs the
>>>>> graph binding to the `typec-switch`. SBU, HS and SS lanes might have different
>>>>> muxes altogether (usb-c-connect has separate ports for SBU, HS and SS lanes)
>>>>
>>>> If the usb-c-connector still needs a graph binding to the typec-switch
>>>> then why isn't that part of this series?
>>>
>>> That's not what I meant (what I meant earlier is the intention is not
>>> what you stated).
>>> I simply meant that the usb-c-connectors ports should be connected to
>>> the typec-switch
>>> ports. There isn't any binding update required for this.
>>>
>>
>> Ok. Got it.
> 
> This really is a limited binding change that helps describe connections
> between Type-C components, helps these components integrate with
> the kernel Type-C framework, and consolidates the associated properties.
> I believe it works for most current use cases in the upstream kernel.
> 
> I'm happy to discuss more theoretical use cases further, but
> respectfully, I prefer to do
> so off-list.
> 
> If the maintainer is OK with it (Krzysztof has reviewed it, but I
> don't want to presume
> what the protocol is for patches in this subsystem), and we've
> provided 2 users as asked for

Although I reviewed it, but Stephen has legitimate concerns and they
should be addressed.

I guess Rob's feedback would be valuable here as well.

I think it would help if you articulated the exact problem, because
there is a quite a discussion. Do I understand correctly that the
bindings mimic USB connector and this is not appropriate for this type
of a device?

Or maybe this should not be represented in DT at all?

> in v4 [5], then I request its consideration for submission.
> If the maintainers have further concerns, we'd be happy to address them.
> 
> Best regards,
> 
> -Prashant
> 
> [5] https://lore.kernel.org/linux-usb/20220616193424.GA3844759-robh@xxxxxxxxxx/


Best regards,
Krzysztof