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
