Hi, On Fri, Dec 20, 2019 at 04:10:03PM +0800, Chen-Yu Tsai wrote: > On Fri, Dec 20, 2019 at 4:03 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote: > > > > Hi, > > > > On Thu, Dec 19, 2019 at 11:24:52PM +0800, Chen-Yu Tsai wrote: > > > On Thu, Dec 19, 2019 at 4:43 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote: > > > > > > > > The Allwinner A80 SoCs have a USB PHY controller that is used by Linux, > > > > with a matching Device Tree binding. > > > > > > > > Now that we have the DT validation in place, let's convert the device tree > > > > bindings for that controller over to a YAML schemas. > > > > > > > > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx> > > > > --- > > > > .../phy/allwinner,sun9i-a80-usb-phy.yaml | 135 ++++++++++++++++++ > > > > .../devicetree/bindings/phy/sun9i-usb-phy.txt | 37 ----- > > > > 2 files changed, 135 insertions(+), 37 deletions(-) > > > > create mode 100644 Documentation/devicetree/bindings/phy/allwinner,sun9i-a80-usb-phy.yaml > > > > delete mode 100644 Documentation/devicetree/bindings/phy/sun9i-usb-phy.txt > > > > > > > > diff --git a/Documentation/devicetree/bindings/phy/allwinner,sun9i-a80-usb-phy.yaml b/Documentation/devicetree/bindings/phy/allwinner,sun9i-a80-usb-phy.yaml > > > > new file mode 100644 > > > > index 000000000000..ded7d6f0a119 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/phy/allwinner,sun9i-a80-usb-phy.yaml > > > > @@ -0,0 +1,135 @@ > > > > +# SPDX-License-Identifier: GPL-2.0 > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/phy/allwinner,sun9i-a80-usb-phy.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Allwinner A80 USB PHY Device Tree Bindings > > > > + > > > > +maintainers: > > > > + - Chen-Yu Tsai <wens@xxxxxxxx> > > > > + - Maxime Ripard <mripard@xxxxxxxxxx> > > > > + > > > > +properties: > > > > + "#phy-cells": > > > > + const: 0 > > > > + > > > > + compatible: > > > > + const: allwinner,sun9i-a80-usb-phy > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + clocks: > > > > + anyOf: > > > > + - description: Main PHY Clock > > > > + > > > > + - items: > > > > + - description: Main PHY clock > > > > + - description: HSIC 12MHz clock > > > > + - description: HSIC 480MHz clock > > > > + > > > > + clock-names: > > > > + oneOf: > > > > + - const: phy > > > > + > > > > + - items: > > > > + - const: phy > > > > + - const: hsic_12M > > > > + - const: hsic_480M > > > > + > > > > + resets: > > > > + anyOf: > > > > + - description: Normal USB PHY reset > > > > + > > > > + - items: > > > > + - description: Normal USB PHY reset > > > > + - description: HSIC Reset > > > > + > > > > + reset-names: > > > > + oneOf: > > > > + - const: phy > > > > + > > > > + - items: > > > > + - const: phy > > > > + - const: hsic > > > > + > > > > + phy_type: > > > > + const: hsic > > > > + description: > > > > + When absent, the PHY type will be assumed to be normal USB. > > > > + > > > > + phy-supply: > > > > + description: > > > > + Regulator that powers VBUS > > > > + > > > > +required: > > > > + - "#phy-cells" > > > > + - compatible > > > > + - reg > > > > + - clocks > > > > + - clock-names > > > > + - resets > > > > + - reset-names > > > > + > > > > +additionalProperties: false > > > > + > > > > +if: > > > > + properties: > > > > + phy_type: > > > > + const: hsic > > > > + > > > > + required: > > > > + - phy_type > > > > + > > > > +then: > > > > + properties: > > > > + clocks: > > > > + maxItems: 3 > > > > + > > > > + clock-names: > > > > + maxItems: 3 > > > > + > > > > + resets: > > > > + maxItems: 2 > > > > + > > > > + reset-names: > > > > + maxItems: 2 > > > > > > So this is slightly incorrect. If phy_type == "hsic", then the > > > "phy" clock and reset should not be needed. I say should because > > > no boards actually came with HSIC implemented. The A80 Optimus > > > board had the HSIC lines on one of the GPIO headers, but I never > > > had any HSIC chips lol. > > > > This isn't what the previous binding was saying though :/ > > From the original binding: > > - clock-names : depending on the "phy_type" property, > * "phy" for normal USB > * "hsic_480M", "hsic_12M" for HSIC > - resets : a list of phandle + reset specifier pairs > - reset-names : depending on the "phy_type" property, > * "phy" for normal USB > * "hsic" for HSIC > > It is recommended to list all clocks and resets available. > The driver will only use those matching the phy_type. I'm not quite sure how we want to fix this then, or even what there's to fix. The previous binding is saying that we need either phy or hsic, and that we should ideally set both. The DT are following that recommendation, and we have either one item for the clocks (phy), or three (phy + 2 HSIC clocks). resets is in a similar situation. The binding allows to have either one or three, and enforce that in HSIC we have three (but leaves the option open to have either 1 or 3 in the normal phy type). As far as I can see, we cover what the binding was saying. Am I missing something? Maxime
Attachment:
signature.asc
Description: PGP signature
