On Mon, Mar 30, 2020 at 6:14 PM Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> wrote: > > On Mon, 2020-03-30 at 13:23 -0600, Rob Herring wrote: > > On Sun, Mar 15, 2020 at 12:16:32PM -0700, rentao.bupt@xxxxxxxxx wrote: > > > From: Tao Ren <rentao.bupt@xxxxxxxxx> > > > > > > Update device tree binding document for aspeed vhub's device IDs and > > > string properties. > > > > > > Signed-off-by: Tao Ren <rentao.bupt@xxxxxxxxx> > > > --- > > > No change in v2: > > > - the patch is added into the series since v2. > > > > > > .../bindings/usb/aspeed,usb-vhub.yaml | 68 +++++++++++++++++++ > > > 1 file changed, 68 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml b/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml > > > index 06399ba0d9e4..5b2e8d867219 100644 > > > --- a/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml > > > +++ b/Documentation/devicetree/bindings/usb/aspeed,usb-vhub.yaml > > > @@ -52,6 +52,59 @@ properties: > > > minimum: 1 > > > maximum: 21 > > > > > > + vhub-vendor-id: > > > + description: vhub Vendor ID > > > + allOf: > > > + - $ref: /schemas/types.yaml#/definitions/uint32 > > > + - maximum: 65535 > > > + > > > + vhub-product-id: > > > + description: vhub Product ID > > > + allOf: > > > + - $ref: /schemas/types.yaml#/definitions/uint32 > > > + - maximum: 65535 > > > > There's already standard 'vendor-id' and 'device-id' properties. Use > > those. > > So yes and no... I don't fundamentally object but keep in mind that > traditionally, the properties are about matching with a physical > hardware. > > In this case however, we are describing a virtual piece of HW and so > those IDs are going to be picked up to be exposed as the USB > vendor/device of the vhub on the USB bus. > > Not necessarily an issue but it's more "configuration" than "matching" > and as such, it might make sense to expose that with a prefix, though I > would prefer something like usb-vendor-id or usb,vendor-id... For FDT uses, it's pretty much been configuration. It's mostly been for PCI that I've seen these properties used. > > > + > > > + vhub-device-revision: > > > > Specific to USB, not vhub. > > Same as the above. > > > > + description: vhub Device Revision in binary-coded decimal > > > + allOf: > > > + - $ref: /schemas/types.yaml#/definitions/uint32 > > > + - maximum: 65535 > > > + > > > + vhub-strings: > > > + type: object > > > + > > > + properties: > > > + '#address-cells': > > > + const: 1 > > > + > > > + '#size-cells': > > > + const: 0 > > > + > > > + patternProperties: > > > + '^string@[0-9a-f]+$': > > > + type: object > > > + description: string descriptors of the specific language > > > + > > > + properties: > > > + reg: > > > + maxItems: 1 > > > + description: 16-bit Language Identifier defined by USB-IF > > > + > > > + manufacturer: > > > + description: vhub manufacturer > > > + allOf: > > > + - $ref: /schemas/types.yaml#/definitions/string > > > + > > > + product: > > > + description: vhub product name > > > + allOf: > > > + - $ref: /schemas/types.yaml#/definitions/string > > > + > > > + serial-number: > > > + description: vhub device serial number > > > + allOf: > > > + - $ref: /schemas/types.yaml#/definitions/string > > > > For all of this, it's USB specific, not vhub specific. I'm not sure this > > is the right approach. It might be better to just define properties > > which are just raw USB descriptors rather than inventing some DT format > > that then has to be converted into USB descriptors. > > Raw blob in the DT is rather annoying and leads to hard to parse stuff > for both humans and scripts. The main strenght of the DT is it's easy > to read and manipulate. True, and I'd certainly agree when we're talking about some vendor specific blob. but there's already code/tools to parse USB descriptors. > Also not the entire descriptor is configurable this way. > > That said, it could be that using the DT for the above is overkill and > instead, we should consider a configfs like the rest of USB gadget. > Though it isn't obvious how to do that, the current gadget stuff > doesn't really "fit" what we need here. Surely the descriptor building code can be shared at a minimum. Regardless of whether gadget configfs fits, usually it is pretty clear whether something belongs in DT or userspace. That should be decided first. > Maybe we could expose the port as UDCs but not actually expose them on > the bus until the hub is "activated" via a special configfs entry... If control of the hub is done by userspace, I'd think configuration should be there too. Rob
