Re: [PATCH v2] dt-bindings: riscv: deprecate riscv,isa
On 08/06/2023 17:54, Conor Dooley wrote: From: Conor Dooley intro = When the RISC-V dt-bindings were accepted upstream in Linux, the base ISA etc had yet to be ratified. By the ratification of the base ISA, incompatible changes had snuck into the specifications - for example the Zicsr and Zifencei extensions were spun out of the base ISA. Fast forward to today, and the reason for this patch. Currently the riscv,isa dt property permits only a specific subset of the ISA string - in particular it excludes version numbering. With the current constraints, it is not possible to discern whether "rv64i" means that the hart supports the fence.i instruction, for example. Future systems may choose to implement their own instruction fencing, perhaps using a vendor extension, or they may not implement the optional counter extensions. Software needs a way to determine this. versioning schemes == "Use the extension versions that are described in the ISA manual" you may say, and it's not like this has not been considered. Firstly, software that parses the riscv,isa property at runtime will need to contain a lookup table of some sort that maps arbitrary versions to versions it understands. There is not a consistent application of version number applied to extensions, with a higgledy-piggledy collection of tags, "bare" and version documents awaiting the reader on the "recently ratified extensions" page: https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions As an aside, this is reflected in the patch too, since many extensions have yet to appear in a release of the ISA specs, and are defined by commits in their respective "working draft" repositories. Secondly, there is an issue of backwards compatibility, whereby allowing numbers in the ISA string, some parsers may be broken. This would require an additional property to be created to even use the versions in this manner. boolean properties == If a new property is needed, the whole approach may as well be looked at from the bottom up. A string with limited character choices etc is hardly the best approach for communicating extension information to software. Switching to using boolean properties, one per extension, allows us to define explicit meanings for the DT representation of each extension - rather than the current situation where different operating systems or other bits of software may impart different meanings to characters in the string. Clearly the best source of meanings is the specifications themselves, this just provides us the ability to choose at what point in time the meaning is set. If an extension changes incompatibility in the future, a new property will be required. Off-list, some of the RVI folks have committed to shoring up the wording in either the ISA specifications, the riscv-isa-manual or so that in the future, modifications to and additions or removals of features will require a new extension. Codifying that assertion somewhere would make it quite unlikely that compatibility would be broken, but we have the tools required to deal with it, if & when it crops up. It is in our collective interest, as consumers of extension meanings, to define a scheme that enforces compatibility. The use of boolean properties, rather than elements in a string, will also permit validation that the strings have a meaning, as well as potentially reject mutually exclusive combinations, or enforce dependencies between instructions. That would not be possible with the current dt-schema infrastructure for arbitrary strings, as we would need to add a riscv,isa parser to dt-validate! That's not implemented in this patch, but rather left as future work (for the brave, or the foolish). acpi The current ACPI ECR is based on having a string unfortunately, but ideally ACPI will move to another method, perhaps GUIDs, that give explicit meaning to extensions. parser simplicity = Many systems that parse DT at runtime already implement an function that can check for the presence of boolean properties, rather than having to implement - although unfortunately for backwards compatibility with old dtbs, existing parsers may not be removable - which may greatly simplify dt parsing code. For example, in Linux, checking for an extension becomes as simple as: of_property_present(extension_node, "zicbom") vendor extensions = Compared to riscv,isa, this proposed scheme promotes vendor extensions, oft touted as the strength of RISC-V, to first-class citizens. At present, extensions are defined as meaning what the RISC-V ISA specifications say they do. There is no realistic way of using that interface to provide cross-platform definitions for what vendor extensions mean. Vendor extensions may also have even less consistency than RVI do in terms of versioning, or no care about backwards compatibility. A boolean property allows us to assign explicit
Re: [PATCH v2] dt-bindings: riscv: deprecate riscv,isa
On Thu, 22 Jun 2023 11:59:32 PDT (-0700), Conor Dooley wrote: On Thu, Jun 22, 2023 at 11:25:35AM -0700, Palmer Dabbelt wrote: Reviewed-by: Palmer Dabbelt Acked-by: Palmer Dabbelt I'm not wed to any particular encoding for the properties, IMO that's more of a decision for the DT folks. IMO the important bit is to just get away from ISA strings and move towards some tightly-specified properties that indicate how the HW actually behaves. I'm going to resubmit with Rob's list of strings. I'll keep your tags, since the spirit of the patch will be the same, with enforced meanings for each extension. Works for me, thanks. Cheers, Conor.
Re: [PATCH v2] dt-bindings: riscv: deprecate riscv,isa
On Thu, Jun 22, 2023 at 11:25:35AM -0700, Palmer Dabbelt wrote: > Reviewed-by: Palmer Dabbelt > Acked-by: Palmer Dabbelt > > I'm not wed to any particular encoding for the properties, IMO that's more > of a decision for the DT folks. IMO the important bit is to just get away > from ISA strings and move towards some tightly-specified properties that > indicate how the HW actually behaves. I'm going to resubmit with Rob's list of strings. I'll keep your tags, since the spirit of the patch will be the same, with enforced meanings for each extension. Cheers, Conor. signature.asc Description: PGP signature
Re: [PATCH v2] dt-bindings: riscv: deprecate riscv,isa
On Thu, 08 Jun 2023 09:54:05 PDT (-0700), Conor Dooley wrote: From: Conor Dooley intro = When the RISC-V dt-bindings were accepted upstream in Linux, the base ISA etc had yet to be ratified. By the ratification of the base ISA, incompatible changes had snuck into the specifications - for example the Zicsr and Zifencei extensions were spun out of the base ISA. Fast forward to today, and the reason for this patch. Currently the riscv,isa dt property permits only a specific subset of the ISA string - in particular it excludes version numbering. With the current constraints, it is not possible to discern whether "rv64i" means that the hart supports the fence.i instruction, for example. Future systems may choose to implement their own instruction fencing, perhaps using a vendor extension, or they may not implement the optional counter extensions. Software needs a way to determine this. versioning schemes == "Use the extension versions that are described in the ISA manual" you may say, and it's not like this has not been considered. Firstly, software that parses the riscv,isa property at runtime will need to contain a lookup table of some sort that maps arbitrary versions to versions it understands. There is not a consistent application of version number applied to extensions, with a higgledy-piggledy collection of tags, "bare" and version documents awaiting the reader on the "recently ratified extensions" page: https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions As an aside, this is reflected in the patch too, since many extensions have yet to appear in a release of the ISA specs, and are defined by commits in their respective "working draft" repositories. Secondly, there is an issue of backwards compatibility, whereby allowing numbers in the ISA string, some parsers may be broken. This would require an additional property to be created to even use the versions in this manner. boolean properties == If a new property is needed, the whole approach may as well be looked at from the bottom up. A string with limited character choices etc is hardly the best approach for communicating extension information to software. Switching to using boolean properties, one per extension, allows us to define explicit meanings for the DT representation of each extension - rather than the current situation where different operating systems or other bits of software may impart different meanings to characters in the string. Clearly the best source of meanings is the specifications themselves, this just provides us the ability to choose at what point in time the meaning is set. If an extension changes incompatibility in the future, a new property will be required. Off-list, some of the RVI folks have committed to shoring up the wording in either the ISA specifications, the riscv-isa-manual or so that in the future, modifications to and additions or removals of features will require a new extension. Codifying that assertion somewhere would make it quite unlikely that compatibility would be broken, but we have the tools required to deal with it, if & when it crops up. It is in our collective interest, as consumers of extension meanings, to define a scheme that enforces compatibility. The use of boolean properties, rather than elements in a string, will also permit validation that the strings have a meaning, as well as potentially reject mutually exclusive combinations, or enforce dependencies between instructions. That would not be possible with the current dt-schema infrastructure for arbitrary strings, as we would need to add a riscv,isa parser to dt-validate! That's not implemented in this patch, but rather left as future work (for the brave, or the foolish). acpi The current ACPI ECR is based on having a string unfortunately, but ideally ACPI will move to another method, perhaps GUIDs, that give explicit meaning to extensions. parser simplicity = Many systems that parse DT at runtime already implement an function that can check for the presence of boolean properties, rather than having to implement - although unfortunately for backwards compatibility with old dtbs, existing parsers may not be removable - which may greatly simplify dt parsing code. For example, in Linux, checking for an extension becomes as simple as: of_property_present(extension_node, "zicbom") vendor extensions = Compared to riscv,isa, this proposed scheme promotes vendor extensions, oft touted as the strength of RISC-V, to first-class citizens. At present, extensions are defined as meaning what the RISC-V ISA specifications say they do. There is no realistic way of using that interface to provide cross-platform definitions for what vendor extensions mean. Vendor extensions may also have even less consistency than RVI do in terms of versioning, or no care about backwards compatibility. A boolean property allows us to
Re: [PATCH v2] dt-bindings: riscv: deprecate riscv,isa
On Fri, Jun 09, 2023 at 08:03:44AM -0600, Rob Herring wrote: > Nope, vendor prefixes don't go in node names. That's not explicit > anywhere, but goes against using generic node names. Yeah, that makes sense. > Also, note that looking at the DT spec, there's already prior art here > with PPC. See "power-isa-*". Though that appears in .dts files, but no > kernel code. Maybe you already saw that as your v1 is similar. There's > not really much advantage to align with it, but also not much reason > to deviate. I went back and forth on riscv-isa- versus riscv,isa-, picking the , for consistency with other properties in the file. I thought that doing it like power would be frowned upon & that it might've been done like that for historic reasons, especially given existing properties we have for riscv ISA related things are vendor properties w/ the comma. I don't have strong feelings either way though, so if you say s/,/-/ then that's good by me! Cheers, Conor. signature.asc Description: PGP signature
Re: [PATCH v2] dt-bindings: riscv: deprecate riscv,isa
On Thu, Jun 8, 2023 at 12:05 PM Conor Dooley wrote:
>
> On Thu, Jun 08, 2023 at 11:49:08AM -0600, Rob Herring wrote:
> > On Thu, 08 Jun 2023 17:54:05 +0100, Conor Dooley wrote:
>
> > > As a result of implementing Sean's suggestion, I believe I need to add
> > > riscv,isa-extensions as an exception to the rules preventing vendor
> > > properties being of object type, otherwise dt_binding_check is less than
> > > happy with me.
>
> > dtschema/dtc warnings/errors:
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/extensions.yaml:
> > properties:riscv,isa-extensions: 'oneOf' conditional failed, one must be
> > fixed:
> > Additional properties are not allowed ('additionalProperties',
> > 'properties' were unexpected)
> > hint: A vendor boolean property can use "type: boolean"
> >
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/extensions.yaml:
> > properties:riscv,isa-extensions: 'oneOf' conditional failed, one must be
> > fixed:
> > 'enum' is a required property
> > 'const' is a required property
> > hint: A vendor string property with exact values has an
> > implicit type
> > from schema $id:
> > http://devicetree.org/meta-schemas/vendor-props.yaml#
> > Additional properties are not allowed ('additionalProperties',
> > 'properties', 'type' were unexpected)
> > hint: A vendor string property with exact values has an
> > implicit type
> >
> > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/extensions.yaml:
> > properties:riscv,isa-extensions: 'oneOf' conditional failed, one must be
> > fixed:
> > '$ref' is a required property
> > 'allOf' is a required property
> > hint: A vendor property needs a $ref to types.yaml
> > from schema $id:
> > http://devicetree.org/meta-schemas/vendor-props.yaml#
> > 'boolean' was expected
> > hint: A vendor boolean property can use "type: boolean"
> > hint: Vendor specific properties must have a type and description
> > unless they have a defined, common suffix.
> > from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
>
> Yeah, expected. I think I need an exception in vendor-props for this to
> pass the checks.
Nope, vendor prefixes don't go in node names. That's not explicit
anywhere, but goes against using generic node names.
Also, note that looking at the DT spec, there's already prior art here
with PPC. See "power-isa-*". Though that appears in .dts files, but no
kernel code. Maybe you already saw that as your v1 is similar. There's
not really much advantage to align with it, but also not much reason
to deviate.
Rob
Re: [PATCH v2] dt-bindings: riscv: deprecate riscv,isa
On Thu, Jun 08, 2023 at 11:49:08AM -0600, Rob Herring wrote:
> On Thu, 08 Jun 2023 17:54:05 +0100, Conor Dooley wrote:
> > As a result of implementing Sean's suggestion, I believe I need to add
> > riscv,isa-extensions as an exception to the rules preventing vendor
> > properties being of object type, otherwise dt_binding_check is less than
> > happy with me.
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/extensions.yaml:
> properties:riscv,isa-extensions: 'oneOf' conditional failed, one must be
> fixed:
> Additional properties are not allowed ('additionalProperties',
> 'properties' were unexpected)
> hint: A vendor boolean property can use "type: boolean"
>
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/extensions.yaml:
> properties:riscv,isa-extensions: 'oneOf' conditional failed, one must be
> fixed:
> 'enum' is a required property
> 'const' is a required property
> hint: A vendor string property with exact values has an
> implicit type
> from schema $id:
> http://devicetree.org/meta-schemas/vendor-props.yaml#
> Additional properties are not allowed ('additionalProperties',
> 'properties', 'type' were unexpected)
> hint: A vendor string property with exact values has an
> implicit type
>
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/riscv/extensions.yaml:
> properties:riscv,isa-extensions: 'oneOf' conditional failed, one must be
> fixed:
> '$ref' is a required property
> 'allOf' is a required property
> hint: A vendor property needs a $ref to types.yaml
> from schema $id:
> http://devicetree.org/meta-schemas/vendor-props.yaml#
> 'boolean' was expected
> hint: A vendor boolean property can use "type: boolean"
> hint: Vendor specific properties must have a type and description
> unless they have a defined, common suffix.
> from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
Yeah, expected. I think I need an exception in vendor-props for this to
pass the checks.
signature.asc
Description: PGP signature
Re: [PATCH v2] dt-bindings: riscv: deprecate riscv,isa
On Thu, 08 Jun 2023 17:54:05 +0100, Conor Dooley wrote: > From: Conor Dooley > > intro > = > > When the RISC-V dt-bindings were accepted upstream in Linux, the base > ISA etc had yet to be ratified. By the ratification of the base ISA, > incompatible changes had snuck into the specifications - for example the > Zicsr and Zifencei extensions were spun out of the base ISA. > > Fast forward to today, and the reason for this patch. > Currently the riscv,isa dt property permits only a specific subset of > the ISA string - in particular it excludes version numbering. > With the current constraints, it is not possible to discern whether > "rv64i" means that the hart supports the fence.i instruction, for > example. > Future systems may choose to implement their own instruction fencing, > perhaps using a vendor extension, or they may not implement the optional > counter extensions. Software needs a way to determine this. > > versioning schemes > == > > "Use the extension versions that are described in the ISA manual" you > may say, and it's not like this has not been considered. > Firstly, software that parses the riscv,isa property at runtime will > need to contain a lookup table of some sort that maps arbitrary versions > to versions it understands. There is not a consistent application of > version number applied to extensions, with a higgledy-piggledy > collection of tags, "bare" and version documents awaiting the reader on > the "recently ratified extensions" page: > https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions > > As an aside, this is reflected in the patch too, since many > extensions have yet to appear in a release of the ISA specs, > and are defined by commits in their respective "working draft" > repositories. > > Secondly, there is an issue of backwards compatibility, whereby allowing > numbers in the ISA string, some parsers may be broken. This would > require an additional property to be created to even use the versions in > this manner. > > boolean properties > == > > If a new property is needed, the whole approach may as well be looked at > from the bottom up. A string with limited character choices etc is > hardly the best approach for communicating extension information to > software. > > Switching to using boolean properties, one per extension, allows us to > define explicit meanings for the DT representation of each extension - > rather than the current situation where different operating systems or > other bits of software may impart different meanings to characters in > the string. Clearly the best source of meanings is the specifications > themselves, this just provides us the ability to choose at what point > in time the meaning is set. If an extension changes incompatibility in > the future, a new property will be required. > > Off-list, some of the RVI folks have committed to shoring up the wording > in either the ISA specifications, the riscv-isa-manual or > so that in the future, modifications to and additions or removals of > features will require a new extension. Codifying that assertion > somewhere would make it quite unlikely that compatibility would be > broken, but we have the tools required to deal with it, if & when it > crops up. > It is in our collective interest, as consumers of extension meanings, to > define a scheme that enforces compatibility. > > The use of boolean properties, rather than elements in a string, will > also permit validation that the strings have a meaning, as well as > potentially reject mutually exclusive combinations, or enforce > dependencies between instructions. That would not be possible with the > current dt-schema infrastructure for arbitrary strings, as we would need > to add a riscv,isa parser to dt-validate! That's not implemented in this > patch, but rather left as future work (for the brave, or the foolish). > > acpi > > > The current ACPI ECR is based on having a string unfortunately, but > ideally ACPI will move to another method, perhaps GUIDs, that give > explicit meaning to extensions. > > parser simplicity > = > > Many systems that parse DT at runtime already implement an function that > can check for the presence of boolean properties, rather than having to > implement - although unfortunately for backwards compatibility with old > dtbs, existing parsers may not be removable - which may greatly simplify > dt parsing code. For example, in Linux, checking for an extension > becomes as simple as: > of_property_present(extension_node, "zicbom") > > vendor extensions > = > > Compared to riscv,isa, this proposed scheme promotes vendor extensions, > oft touted as the strength of RISC-V, to first-class citizens. > At present, extensions are defined as meaning what the RISC-V ISA > specifications say they do. There is no realistic way of using that > interface to provide cross-platform definitions for
