[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200401004741.GA6923@taoren-ubuntu-R90MNF91>
Date: Tue, 31 Mar 2020 17:47:43 -0700
From: Tao Ren <rentao.bupt@...il.com>
To: Rob Herring <robh@...nel.org>
Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Felipe Balbi <balbi@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Joel Stanley <joel@....id.au>,
Andrew Jeffery <andrew@...id.au>,
Chunfeng Yun <chunfeng.yun@...iatek.com>,
Colin Ian King <colin.king@...onical.com>,
Stephen Boyd <swboyd@...omium.org>,
Mark Rutland <mark.rutland@....com>,
Linux USB List <linux-usb@...r.kernel.org>,
"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE"
<linux-arm-kernel@...ts.infradead.org>,
linux-aspeed@...ts.ozlabs.org, devicetree@...r.kernel.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
Tao Ren <taoren@...com>
Subject: Re: [PATCH v2 6/6] dt-bindings: usb: document aspeed vhub device
ID/string properties
On Tue, Mar 31, 2020 at 10:21:10AM -0600, Rob Herring wrote:
> On Mon, Mar 30, 2020 at 6:14 PM Benjamin Herrenschmidt
> <benh@...nel.crashing.org> 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@...il.com wrote:
> > > > From: Tao Ren <rentao.bupt@...il.com>
> > > >
> > > > Update device tree binding document for aspeed vhub's device IDs and
> > > > string properties.
> > > >
> > > > Signed-off-by: Tao Ren <rentao.bupt@...il.com>
> > > > ---
> > > > 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.
Thank you Rob and Ben for the comments. I thought about using "vendor-id"
or "idVendor" prefixed with "usb", "hub" or "vhub", and I chose "vhub"
just to distinguish from per-port usb devices' properties in the future.
Anyways I'm very happy to update the names per your suggestions.
>
> > > > +
> > > > + 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
Perhaps it's my lack of greater knowledge in gadget/dt areas, and I'm not
quite clear what would be the recommended/accepted approach for this
case. I'm looking forward for more suggestions.
Cheers,
Tao
Powered by blists - more mailing lists