On Wednesday, March 15, 2017 11:33:35 AM Sakari Ailus wrote:
> On 03/15/17 10:23, Sakari Ailus wrote:
> > On Wed, Mar 15, 2017 at 12:13:45AM +0100, Rafael J. Wysocki wrote:
> >> On Tue, Mar 14, 2017 at 11:53 PM, Sakari Ailus
> >> <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> >>> Rafael J. Wysocki wrote:
> >>>>
> >>>> On Tue, Mar 14, 2017 at 10:16 PM, Sakari Ailus
> >>>> <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> >>>>>
> >>>>> Rafael J. Wysocki wrote:
> >>>>>>
> >>>>>>
> >>>>>> On Tue, Mar 14, 2017 at 6:54 PM, Sakari Ailus
> >>>>>> <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> Hi Rafael,
> >>>>>>>
> >>>>>>> Rafael J. Wysocki wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On Tue, Mar 14, 2017 at 9:09 AM, Sakari Ailus
> >>>>>>>> <sakari.ailus@xxxxxxxxxxxxxxx> wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On 03/14/17 10:08, Sakari Ailus wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> How about this instead:
> >>>>>>>>>>
> >>>>>>>>>> All port nodes are located under the device's "_DSD" node in the
> >>>>>>>>>> hierarchical data extension tree. The property extension related to
> >>>>>>>>>> each port node must contain the key "port" and an integer value
> >>>>>>>>>> which
> >>>>>>>>>> is the number of the port.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> So with matching strings instead of indices, this will change, too...
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> It doesn't have to AFAICS, but the number is just redundant IMO. You
> >>>>>>>> only need a boolean property saying "this is a port", so you know that
> >>>>>>>> you should expect a list of endpoints in that object.
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> No, it's not redundant. It's the number of the physical port in the
> >>>>>>> device
> >>>>>>> --- this is how the driver gets to know where the connection has been
> >>>>>>> made.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> OK, but what exactly do you mean by "physical port"?
> >>>>>
> >>>>>
> >>>>>
> >>>>> The device (or an IP block) has physical interfaces to the world outside.
> >>>>> There could be just one, but there may be more. For an ISP, there could
> >>>>> be
> >>>>> e.g. four CSI-2 receivers to each of which you could connect a camera
> >>>>> sensor. So for an ISP device, that number tells which of the receivers a
> >>>>> given sensor is connected to.
> >>>>>
> >>>>> The mapping between this number and what the hardware datasheet refers to
> >>>>> needs to be documented per device.
> >>>>
> >>>>
> >>>> OK, so the number actually is an arbitrary piece of data associated
> >>>> with the key "port" and the interpretation of that piece of data
> >>>> depends on whoever asks for that value.
> >>>>
> >>>> IOW, the core doesn't care.
> >>>>
> >>>> With all due respect to whoever invented this on the DT side, this is
> >>>> just bad design to me, because it causes the "port" property to serve
> >>>> two different purposes at the same time. First, it tells the core
> >>>> that this object is a port. Second, it is expected to provide a piece
> >>>> of data of unspecified interpretation to somebody. Which means that
> >>>> the "port" property is both general and device-specific at the same
> >>>> time and the sanity of that is quite questionable IMO.
> >>>
> >>>
> >>> DT uses a node called either "port" or "ports" to store the port nodes. The
> >>> reg property tells the number of the port (see
> >>> Documentation/devicetree/bindings/graph.txt). I'm no DT expert, but my
> >>> understanding is that the node namespace is different from the property
> >>> namespace.
> >>
> >> So on the DT side it actually looks OK to me.
> >>
> >> And the <reg> value is referred to as the port-endpoint identifier, so
> >> I guess this is used for referring to the port/endpoint instead of an
> >> index or the key value somewhere?
> >
> > The remote-endpoint uses phandles; they're a mechanism in DT to refer to
> > different nodes in the tree (DT does not differentiate between devices and
> > non-device nodes). There's a relatively good example here:
> >
> > arch/arm/boot/dts/omap3-n9.dts
Yes, I figured that out. :-)
Still, the <rev> property value is used for something somewhere in the code I gather.
> >>
> >>> If you're concerned of possible double meanings, it's entirely possible to
> >>> put the port nodes under hierarchical data extension named e.g. "ports", and
> >>> document that this is what the node must be called (single port node could
> >>> be just called "port"). This way, it should be much more difficult to
> >>> interpret a non-port node as a port node --- roughly equivalent of the DT
> >>> ports node.
> >>>
> >>> The drawback with this change is that the size of the data structure in ASL
> >>> (and AML) will grow.
> >>
> >> The "ports" thing would only be useful if we had the other properties
> >> to put in there.
> >>
> >> So I guess we can specify that the "port" property value is the
> >> identifier of the port and then we will use this in the
> >> "remote-endpoint" property on the other end instead of an index.
> >
> > Makes sense.
> >
> >>
> >> And analogously for the "enpoint" property value.
> >
> > The endpoint hierarchical data extension node name? There is no endpoint
> > property being used by this version of the set anymore; I removed it as it
> > was redundant. However a human readable endpoint node name can be chosen
> > which that'd be quite practical to identify the endpoint.
> >
> > Then the remote-endpoint properties would be:
> >
> > Package () { device, port (integer), endpoint-node-name (string) }
> >
>
> Oh, well... the device is present there already, so the endpoint
> reference would well use an index pretty much equally well. Most of the
> time it'll be zero anyway. I.e.
>
> Package() { device, port number (integer), endpoint id (integer }
Yes, I would do that, and actually not using the index for the endpoint too.
Let ports and endpoints be symmetrical in that respect, that is a port is
required to have a { "port", id } property and an endpoind is required to
have a { "endpoint", id } property and let the ids be used in
"remote-endpoint" properties as per the above.
Then, in each case, the id would be whatever the value of the <rev> property
on the DT side would be.
> But switching from the port index to the port number is a tangible
> improvement.
Well, to me, using indices should not even be taken into consideration as
a viable approach in similar cases.
I know that using indices is a common practice in the ACPI land, but IMO that's
not a good one.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
