Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200315191632.12536-1-rentao.bupt@gmail.com> <20200315191632.12536-7-rentao.bupt@gmail.com>
 <20200330192347.GA6388@bogus> <4dc3ac910c79dcca398eb5161dde44e1cc50baca.camel@kernel.crashing.org>
In-Reply-To: <4dc3ac910c79dcca398eb5161dde44e1cc50baca.camel@kernel.crashing.org>
From:   Rob Herring <robh@kernel.org>
Date:   Tue, 31 Mar 2020 10:21:10 -0600
Message-ID: <CAL_JsqKZeCC352TKFGDNRRogSefF9vq+J=WqCEeg59PBsSOW1w@mail.gmail.com>
Subject: Re: [PATCH v2 6/6] dt-bindings: usb: document aspeed vhub device
 ID/string properties
To:     Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc:     rentao.bupt@gmail.com, Felipe Balbi <balbi@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Joel Stanley <joel@jms.id.au>,
        Andrew Jeffery <andrew@aj.id.au>,
        Chunfeng Yun <chunfeng.yun@mediatek.com>,
        Colin Ian King <colin.king@canonical.com>,
        Stephen Boyd <swboyd@chromium.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Linux USB List <linux-usb@vger.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@lists.infradead.org>,
        linux-aspeed@lists.ozlabs.org, devicetree@vger.kernel.org,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        OpenBMC Maillist <openbmc@lists.ozlabs.org>,
        Tao Ren <taoren@fb.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Mar 30, 2020 at 6:14 PM Benjamin Herrenschmidt
<benh@kernel.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@gmail.com wrote:
> > > From: Tao Ren <rentao.bupt@gmail.com>
> > >
> > > Update device tree binding document for aspeed vhub's device IDs and
> > > string properties.
> > >
> > > Signed-off-by: Tao Ren <rentao.bupt@gmail.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.

> > > +
> > > +  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