usb-c: Add sink VIF policies to device tree, update the GenVIF script #52759
Conversation
madhurimaparuchuri
requested review from
sambhurst,
galak,
tejlmand and
nashif
as code owners
zephyrbot
added
area: USB-C
area: Build System
area: Devicetree Binding
madhurimaparuchuri
force-pushed
the
gvif
branch
2 times, most recently
from
bea1aaf to
ccff834
Compare
madhurimaparuchuri
force-pushed
the
gvif
branch
from
ccff834 to
29e68cc
Compare
There was a problem hiding this comment.
to me this seems like a misuse of devicetree, but I will let @mbolivar-nordic and @galak decide on this.
Waiting for their review comments.
dts/bindings/usb-c/vif.yaml
Outdated
| A VIF node is a root element that describes the Qualifying Product in VIF | ||
| specification. It contains introductory material followed by an optional | ||
| product-level field element, zero or more Components, and zero or more | ||
| Optional Content elements. | ||
| This is based on VIF XML file schema in: | ||
| https://compliance.usb.org/cv/VendorInfoFile/Schemas/Current/VendorInfoFile.xsd | ||
|
|
||
| compatible: "vif" |
There was a problem hiding this comment.
not sure I understand why vif should be a node.
Afaiu VIF is a Vendor Information file which describes the usb device, and creation of a vif file based on dts was introduced here: #51660
Starting to add a vif node in devicetree seems to be a misuse of the devicetree to me. but I would like @mbolivar-nordic or @galak to review and decide on this.
There was a problem hiding this comment.
There are some high level specifications required for VIF, specific to product but applicable to all ports. I see USB-C DT nodes are per port, hence added this node, which is referring to the ports as child(i.e, usb-c connector nodes). And the presence of VIF node in a project is optional with more than 95% of properties being optional within.
There was a problem hiding this comment.
Without commenting on if this is appropriate to include in devicetree (I am no longer maintaining the devicetree bindings so that I can focus on other tasks), I will say that choosing "vif" for the compatible is pretty clear namespace pollution. The namespace of compatible property values is shared across all hardware in the world, so a three-letter acronym had better be for something incredibly general purpose, which this definitely is not.
Please choose a better, more descriptive name, and make sure to expand the acronym (I guess it's "vendor info file") in the description field in the binding.
In general, please remember that you are the VIF domain expert here, but this binding lives in a larger world where there's lots of other hardware and people who will read your documentation without your domain knowledge.
There was a problem hiding this comment.
@galak to review and decide on this
This is the decision maker on whether to put this in DT, since he is overall maintainer for the bindings.
I now only co-maintain the DT infrastructure and API.
There was a problem hiding this comment.
I now only co-maintain the DT infrastructure and API.
I know, but given your experience then your feedback serves as very valid input (at least better input than my feedback) on whether this approach looks appropriate for bindings.
There was a problem hiding this comment.
I'm inclined to agree that the devicetree should not directly represent the VIF fields for a few reasons:
- The USB Implementers' Forum revs the VIF format somewhat regularly to keep up with new USB features. Sometimes, existing fields change name or format without changing the underlying meaning. Keeping up with these changes in the devicetree would involve changing a public-facing (sort of) API in the devicetree bindings.
- Many of the fields will be unknowable to the Zephyr build system, and which ones are knowable depends on the hardware. To produce a valid VIF for, say, compliance testing, a developer would need to add in additional fields from some other source. If I were adding those fields, I'd probably want to use a format other than devicetree.
- The bindings need to describe the devicetree properties, so they need to be verbose like the spec or refer to the spec or summarize the spec at the risk of vagueness or inaccuracy. I'd rather avoid the issue.
- The VIF contains some parallel structure to the existing devicetree, e.g. different pieces for different ports. I'd prefer not to replicate that and risk them being inconsistent.
I'd prefer that the nodes for the port and individual port components contain properties for VIF-relevant information that is determined by the hardware, and the VIF generator would parse those properties (probably among other data sources) while constructing the VIF.
There was a problem hiding this comment.
I avoided adding elements to device tree. I am reading properties that are already there in device tree and generating the VIF, not adding any properties or nodes in this revision. The information that is there in VIF node in earlier revisions is being read form an XML file (manually added) in project folder if it's present (overriding the output VIF file).
There was a problem hiding this comment.
@tejlmand
Could you review the new revision and let me know if any changes are required in this PR
| # EXAMPLE: | ||
| # | ||
| # USB-C connector attached to a STM32 UCPD typec port controller, which has | ||
| # power delivery support and enables SINK. | ||
| # | ||
| # #include <dt-bindings/usb-c/pd.h> | ||
| # | ||
| # vbus1: vbus { | ||
| # compatible = "zephyr,usb-c-vbus-adc"; | ||
| # io-channels = <&adc2 8>; | ||
| # output-ohms = <49900>; | ||
| # full-ohms = <(330000 + 49900)>; | ||
| # }; | ||
| # | ||
| # ports { | ||
| # #address-cells = <1>; | ||
| # #size-cells = <0>; | ||
| # port1: usb-c-port@1 { | ||
| # compatible = "usb-c-connector"; | ||
| # reg = <1>; | ||
| # tcpc = <&ucpd1>; | ||
| # vbus = <&vbus1>; | ||
| # power-role = "SINK"; | ||
| # num-snk-pdos= <2>; | ||
| # sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM) | ||
| # PDO_VAR(5000, 12000, 2000)>; | ||
| # op-sink-microwatt = <10000000>; | ||
| # fr-swap-reqd-type-c-current-as-initial-source= <0>; | ||
| # vconn-swap-to-off-supported; | ||
| # }; | ||
| # }; |
There was a problem hiding this comment.
plase move example to bindings docs at the top
There was a problem hiding this comment.
Included the changes in new revision of this PR
There was a problem hiding this comment.
@gmarull
Could you review the new revision and let me know if any changes are required in this PR
dts/bindings/usb-c/vif.yaml
Outdated
|
|
||
| tid: | ||
| type: string | ||
| required: false |
There was a problem hiding this comment.
all required false are redundant
There was a problem hiding this comment.
Included the changes in new revision of this PR
dts/bindings/usb-c/vif.yaml
Outdated
|
|
||
| vif-product-type: | ||
| type: int | ||
| enum: [0, 1, 2] |
There was a problem hiding this comment.
in order to make DT files more readable, I'd suggest providing definitions for such enums. See include/zephyr/dt-bindings for examples.
There was a problem hiding this comment.
Included the changes in new revision of this PR
dts/bindings/usb-c/vif.yaml
Outdated
| # EXAMPLE: | ||
| # | ||
| # | ||
| # vif { | ||
| # address-cells = <1>; | ||
| # size-cells = <0>; | ||
| # usbcvif1: usbcvif@1 { | ||
| # compatible = "vif"; | ||
| # reg = <1>; | ||
| # vendor-name = "Zephyr"; | ||
| # model-part-number = "b_g474e_dpow1"; | ||
| # product-revision= "1"; | ||
| # tid= "0"; | ||
| # vif-product-type= <0>; | ||
| # certification-type= <0>; | ||
| # component =<&port1>; | ||
| # }; | ||
| # }; |
There was a problem hiding this comment.
Included the changes in new revision of this PR
| num-snk-pdos= <1>; | ||
| sink-pdos = <PDO_FIXED(5000, 100, 0)>; | ||
| fr-swap-reqd-type-c-current-as-initial-source= <0>; |
There was a problem hiding this comment.
missing space before =, please revisit formatting on all files.
There was a problem hiding this comment.
Included the changes in new revision of this PR
| compatible = "vif"; | ||
| reg = <1>; | ||
| vendor-name = "Zephyr"; | ||
| model-part-number = "b_g474e_dpow1"; |
There was a problem hiding this comment.
there's standard model property. Why is this needed?
There was a problem hiding this comment.
model-part-number seemed to be different from model and more specific to USB-C component. Following is it's definition : "To be provided by the Vendor. This field should match what appears in the USB-IF Product Registration form. If the Qualifying Product has a Model Name entry in USB4 DROM, then this field should be identical to that entry. The USB-IF uses this field and /VIF/Product_Revision to generate the product name in the USB Integrator’s List"
madhurimaparuchuri
force-pushed
the
gvif
branch
from
29e68cc to
0b318a1
Compare
madhurimaparuchuri
requested review from
carlescufi and
MaureenHelm
as code owners
madhurimaparuchuri
force-pushed
the
gvif
branch
from
0b318a1 to
be7c28b
Compare
madhurimaparuchuri
force-pushed
the
gvif
branch
from
432d513 to
331bec0
Compare
cmake/vif.cmake
Outdated
| ) | ||
|
|
||
| if(CONFIG_GENVIF_INPUT_VIF_XML_PATH) | ||
| if (EXISTS ${APPLICATION_SOURCE_DIR}/${CONFIG_GENVIF_INPUT_VIF_XML_PATH}) |
There was a problem hiding this comment.
still uses APPLICATION_SOURCE_DIR
cmake/vif.cmake
Outdated
| ) | ||
|
|
||
| if(CONFIG_GENVIF_INPUT_VIF_XML_PATH) | ||
| if (EXISTS ${APPLICATION_SOURCE_DIR}/${CONFIG_GENVIF_INPUT_VIF_XML_PATH}) | ||
| set(vif_source_xml ${APPLICATION_SOURCE_DIR}/${CONFIG_GENVIF_INPUT_VIF_XML_PATH}) |
There was a problem hiding this comment.
Seems you missed this part:
The CMake code can then be simplified.
Code stilkl suffers from the fact that if user makes a typo, like oof.xml instead of the correct foo.xml (which is the file existing on disk), then vif_source_xml is unset and no file argument is passed to python script.
Hence no build failure and user may not notice a mistake was made.
|
|
||
| xml_root = get_root() | ||
| source_xml_root = None | ||
| if args.vif_source_xml: |
There was a problem hiding this comment.
If this arg is required, do you need to check this here?
There was a problem hiding this comment.
Yes it's not required, removing in new revision
madhurimaparuchuri
force-pushed
the
gvif
branch
from
331bec0 to
6b12ad9
Compare
|
I was trying to push upstream rebase first and commit(my changes) next. As part of it I made soft reset of last commit, when I pushed it, this pull request is closed. I am trying to reopen it. Meanwhile this commit has latest changes which I was trying to push here to resolve comments : main...madhurimaparuchuri:zephyr:gvif |
madhurimaparuchuri
force-pushed
the
gvif
branch
2 times, most recently
from
74933c9 to
791db9c
Compare
|
The related commit (Updated version) is added back to this PR, but the commit looks like a whole new change now. Sorry for messing the PR, it's unintentional. |
cmake/vif.cmake
Outdated
| if(CONFIG_GENVIF_INPUT_VIF_XML_PATH) | ||
| if(IS_ABSOLUTE ${CONFIG_GENVIF_INPUT_VIF_XML_PATH}) | ||
| if (EXISTS ${CONFIG_GENVIF_INPUT_VIF_XML_PATH}) | ||
| set(vif_source_xml ${CONFIG_GENVIF_INPUT_VIF_XML_PATH}) |
There was a problem hiding this comment.
I made this comment a month ago which is still not addressed: #52759 (comment)
The CMake code can then be simplified.
Code stilkl suffers from the fact that if user makes a typo, like oof.xml instead of the correct foo.xml (which is the file existing on disk), then vif_source_xml is unset and no file argument is passed to python script.
Hence no build failure and user may not notice a mistake was made.
Please fix.
There was a problem hiding this comment.
The build breaks if there is a typo in the file path configured at present
As discussed offline, I added the error messages in CMAKE to say the build failed due to file not found in new revision
There was a problem hiding this comment.
I see you added an extra check below, accepted.
madhurimaparuchuri
force-pushed
the
gvif
branch
from
8874bce to
c215642
Compare
cmake/vif.cmake
Outdated
| if (EXISTS ${APPLICATION_SOURCE_DIR}/boards/${BOARD}_${vif_xml}) | ||
| set(vif_source_xml ${APPLICATION_SOURCE_DIR}/boards/${BOARD}_${vif_xml}) | ||
| elseif (EXISTS ${APPLICATION_SOURCE_DIR}/${vif_xml}) | ||
| set(vif_source_xml ${APPLICATION_SOURCE_DIR}/${vif_xml}) |
There was a problem hiding this comment.
those should also be updated to use APPLICATION_CONFIG_DIR instead of APPLICATION_CONFIG_DIR to be consistent with other configuration files.
There was a problem hiding this comment.
Included the changes in new revision
cmake/vif.cmake
Outdated
| if(CONFIG_GENVIF_INPUT_VIF_XML_PATH) | ||
| message(FATAL_ERROR "Incorrect VIF source XML file path") | ||
| else() | ||
| message(FATAL_ERROR "No VIF source XML file found") |
There was a problem hiding this comment.
Please some extra information on how to fix this situation.
Could be something along:
To fix, create 'vif.xml' in the application directory or specify a vif.xml using 'CONFIG_GENVIF_INPUT_VIF_XML_PATH'
There was a problem hiding this comment.
Included the changes in new revision, Thank you
cmake/vif.cmake
Outdated
| if(CONFIG_GENVIF_INPUT_VIF_XML_PATH) | ||
| if(IS_ABSOLUTE ${CONFIG_GENVIF_INPUT_VIF_XML_PATH}) | ||
| if (EXISTS ${CONFIG_GENVIF_INPUT_VIF_XML_PATH}) | ||
| set(vif_source_xml ${CONFIG_GENVIF_INPUT_VIF_XML_PATH}) |
There was a problem hiding this comment.
I see you added an extra check below, accepted.
madhurimaparuchuri
force-pushed
the
gvif
branch
3 times, most recently
from
bcefad9 to
5c7165d
Compare
cmake/vif.cmake
Outdated
| if (CONFIG_GENVIF_INPUT_VIF_XML_PATH) | ||
| message(FATAL_ERROR "Incorrect VIF source XML file path. To fix specify correct XML file path at 'CONFIG_GENVIF_INPUT_VIF_XML_PATH'.") | ||
| else () | ||
| message(FATAL_ERROR "No VIF source XML file found. To fix, create '${board_vif_xml}' in boards directory of application directory, or create '${vif_xml}' file in application directory or board directory, or supply a custom XML VIF path using 'CONFIG_GENVIF_INPUT_VIF_XML_PATH'.") |
There was a problem hiding this comment.
Please line wrap.
CMake allows multiple strings with message() and will concatenate those when printing.
| message(FATAL_ERROR "No VIF source XML file found. To fix, create '${board_vif_xml}' in boards directory of application directory, or create '${vif_xml}' file in application directory or board directory, or supply a custom XML VIF path using 'CONFIG_GENVIF_INPUT_VIF_XML_PATH'.") | |
| message(FATAL_ERROR "No VIF source XML file found. " | |
| "To fix, create '${board_vif_xml}' in boards directory of application directory, " | |
| "or create '${vif_xml}' file in application directory or board directory, " | |
| "or supply a custom XML VIF path using 'CONFIG_GENVIF_INPUT_VIF_XML_PATH'." | |
| ) |
cmake/vif.cmake
Outdated
| list(APPEND cmd_gen_vif --vif-source-xml ${vif_source_xml}) | ||
| else () | ||
| if (CONFIG_GENVIF_INPUT_VIF_XML_PATH) | ||
| message(FATAL_ERROR "Incorrect VIF source XML file path. To fix specify correct XML file path at 'CONFIG_GENVIF_INPUT_VIF_XML_PATH'.") |
There was a problem hiding this comment.
please also line wrap here.
Removed few VIF properties which are being hardcoded Updated the script to parse source VIF XML and add information to the output Added optional Kconfig option to configure custom source VIF XML path Cleaned up the code Signed-off-by: Madhurima Paruchuri <[email protected]>
|
@gmarull could you revisit this? |
A VIF node is added to device tree to hold VIF specifications and USB sink VIF policies are added to usb-c-connector node as properties.
The GenVIF script is updated to start parsing from VIF node.
The exisiting USB-C Sink sample project is updated to include examples of new changes in device tree.
The XML output is validated against schema
https://compliance.usb.org/cv/VendorInfoFile/Schemas/Current/VendorInfoFile.xsd Changes to XML namespaces is done according to validation
Signed-off-by: Madhurima Paruchuri [email protected]