| Message ID | 20231110062039.103339-2-william.qiu@starfivetech.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | StarFive's Pulse Width Modulation driver support | expand |
On 2023/11/10 20:24, Krzysztof Kozlowski wrote: > On 10/11/2023 07:20, William Qiu wrote: >> Add documentation to describe OpenCores Pulse Width Modulation >> controller driver. >> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com> >> Reviewed-by: Hal Feng <hal.feng@starfivetech.com> >> --- >> .../bindings/pwm/opencores,pwm.yaml | 56 +++++++++++++++++++ >> 1 file changed, 56 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm.yaml >> >> diff --git a/Documentation/devicetree/bindings/pwm/opencores,pwm.yaml b/Documentation/devicetree/bindings/pwm/opencores,pwm.yaml >> new file mode 100644 >> index 000000000000..8f776bbc1112 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pwm/opencores,pwm.yaml >> @@ -0,0 +1,56 @@ >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pwm/opencores,pwm.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: OpenCores PWM controller >> + >> +maintainers: >> + - William Qiu <william.qiu@starfivetech.com> >> + >> +description: >> + OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC core >> + generates binary signal with user-programmable low and high periods. All PTC counters and >> + registers are 32-bit. > > Wrap at 80 (as Coding Style asks) > Will update. >> + >> +allOf: >> + - $ref: pwm.yaml# >> + >> +properties: >> + compatible: >> + oneOf: >> + - items: >> + - enum: >> + - starfive,jh7100-pwm >> + - starfive,jh7110-pwm >> + - const: opencores,pwm > > That's a very, very generic compatible. Are you sure, 100% sure, that > all designs from OpenCores from now till next 100 years will be 100% > compatible? > My description is not accurate enough, this is OpenCores PTC IP, and PWM is one of those modes, so it might be better to replace compatible with "opencores, ptc-pwm" What do you think? Best Regrads, William > > Best regards, > Krzysztof >
On 2023/11/10 20:24, Krzysztof Kozlowski wrote: > On 10/11/2023 07:20, William Qiu wrote: >> Add documentation to describe OpenCores Pulse Width Modulation >> controller driver. > > Please describe the hardware, not the driver. > Will modify the description. Thanks. Best Regards, William > Best regards, > Krzysztof >
On 13/11/2023 10:42, William Qiu wrote: > Will update. >>> + >>> +allOf: >>> + - $ref: pwm.yaml# >>> + >>> +properties: >>> + compatible: >>> + oneOf: >>> + - items: >>> + - enum: >>> + - starfive,jh7100-pwm >>> + - starfive,jh7110-pwm >>> + - const: opencores,pwm >> >> That's a very, very generic compatible. Are you sure, 100% sure, that >> all designs from OpenCores from now till next 100 years will be 100% >> compatible? >> > My description is not accurate enough, this is OpenCores PTC IP, and PWM > is one of those modes, so it might be better to replace compatible with > "opencores, ptc-pwm" > > What do you think? Sorry, maybe this answers maybe doesn't. What is "PTC"? Best regards, Krzysztof
On Mon, Nov 13, 2023 at 09:07:15PM +0100, Krzysztof Kozlowski wrote: > On 13/11/2023 10:42, William Qiu wrote: > > Will update. > >>> + > >>> +allOf: > >>> + - $ref: pwm.yaml# > >>> + > >>> +properties: > >>> + compatible: > >>> + oneOf: > >>> + - items: > >>> + - enum: > >>> + - starfive,jh7100-pwm > >>> + - starfive,jh7110-pwm > >>> + - const: opencores,pwm > >> > >> That's a very, very generic compatible. Are you sure, 100% sure, that > >> all designs from OpenCores from now till next 100 years will be 100% > >> compatible? > >> > > My description is not accurate enough, this is OpenCores PTC IP, and PWM > > is one of those modes, so it might be better to replace compatible with > > "opencores, ptc-pwm" > > > > What do you think? > > Sorry, maybe this answers maybe doesn't. What is "PTC"? "pwm timer counter". AFAIU, the IP can be configured to provide all 3. I think that William pointed out on an earlier revision that they have only implemented the pwm on their hardware. I don't think putting in "ptc" is a sufficient differentiator though, as clearly there could be several different versions of "ptc-pwm" that have the same concern about "all designs from OpenCores for now till the next 100 years" being compatible. Cheers. Conor.
On 2023/11/14 4:17, Conor Dooley wrote: > On Mon, Nov 13, 2023 at 09:07:15PM +0100, Krzysztof Kozlowski wrote: >> On 13/11/2023 10:42, William Qiu wrote: >> > Will update. >> >>> + >> >>> +allOf: >> >>> + - $ref: pwm.yaml# >> >>> + >> >>> +properties: >> >>> + compatible: >> >>> + oneOf: >> >>> + - items: >> >>> + - enum: >> >>> + - starfive,jh7100-pwm >> >>> + - starfive,jh7110-pwm >> >>> + - const: opencores,pwm >> >> >> >> That's a very, very generic compatible. Are you sure, 100% sure, that >> >> all designs from OpenCores from now till next 100 years will be 100% >> >> compatible? >> >> >> > My description is not accurate enough, this is OpenCores PTC IP, and PWM >> > is one of those modes, so it might be better to replace compatible with >> > "opencores, ptc-pwm" >> > >> > What do you think? >> >> Sorry, maybe this answers maybe doesn't. What is "PTC"? > > "pwm timer counter". AFAIU, the IP can be configured to provide all 3. > I think that William pointed out on an earlier revision that they have > only implemented the pwm on their hardware. > I don't think putting in "ptc" is a sufficient differentiator though, as > clearly there could be several different versions of "ptc-pwm" that have > the same concern about "all designs from OpenCores for now till the next > 100 years" being compatible. > > Cheers. > Conor. Hiļ¼Conor and Krzysztof, After discussion and review of materials, we plan to use "opencores,ptc-pwm-v1" as this version of compatible, so that it can also be compatible in the future. What do you think? Best regards, William
On Wed, Nov 22, 2023 at 03:03:36PM +0800, William Qiu wrote: > > > On 2023/11/14 4:17, Conor Dooley wrote: > > On Mon, Nov 13, 2023 at 09:07:15PM +0100, Krzysztof Kozlowski wrote: > >> On 13/11/2023 10:42, William Qiu wrote: > >> > Will update. > >> >>> + > >> >>> +allOf: > >> >>> + - $ref: pwm.yaml# > >> >>> + > >> >>> +properties: > >> >>> + compatible: > >> >>> + oneOf: > >> >>> + - items: > >> >>> + - enum: > >> >>> + - starfive,jh7100-pwm > >> >>> + - starfive,jh7110-pwm > >> >>> + - const: opencores,pwm > >> >> > >> >> That's a very, very generic compatible. Are you sure, 100% sure, that > >> >> all designs from OpenCores from now till next 100 years will be 100% > >> >> compatible? > >> >> > >> > My description is not accurate enough, this is OpenCores PTC IP, and PWM > >> > is one of those modes, so it might be better to replace compatible with > >> > "opencores, ptc-pwm" > >> > > >> > What do you think? > >> > >> Sorry, maybe this answers maybe doesn't. What is "PTC"? > > > > "pwm timer counter". AFAIU, the IP can be configured to provide all 3. > > I think that William pointed out on an earlier revision that they have > > only implemented the pwm on their hardware. > > I don't think putting in "ptc" is a sufficient differentiator though, as > > clearly there could be several different versions of "ptc-pwm" that have > > the same concern about "all designs from OpenCores for now till the next > > 100 years" being compatible. Perhaps noting what "ptc" stands for in the description field would be a good idea. > After discussion and review of materials, we plan to use "opencores,ptc-pwm-v1" > as this version of compatible, so that it can also be compatible in the future. > > What do you think? Do we know that it is actually "v1" of the IP? I would suggest using the version that actually matches the version of the IP that you are using in your SoC. Thanks, Conor.
On 2023/11/23 1:36, Conor Dooley wrote: > On Wed, Nov 22, 2023 at 03:03:36PM +0800, William Qiu wrote: >> >> >> On 2023/11/14 4:17, Conor Dooley wrote: >> > On Mon, Nov 13, 2023 at 09:07:15PM +0100, Krzysztof Kozlowski wrote: >> >> On 13/11/2023 10:42, William Qiu wrote: >> >> > Will update. >> >> >>> + >> >> >>> +allOf: >> >> >>> + - $ref: pwm.yaml# >> >> >>> + >> >> >>> +properties: >> >> >>> + compatible: >> >> >>> + oneOf: >> >> >>> + - items: >> >> >>> + - enum: >> >> >>> + - starfive,jh7100-pwm >> >> >>> + - starfive,jh7110-pwm >> >> >>> + - const: opencores,pwm >> >> >> >> >> >> That's a very, very generic compatible. Are you sure, 100% sure, that >> >> >> all designs from OpenCores from now till next 100 years will be 100% >> >> >> compatible? >> >> >> >> >> > My description is not accurate enough, this is OpenCores PTC IP, and PWM >> >> > is one of those modes, so it might be better to replace compatible with >> >> > "opencores, ptc-pwm" >> >> > >> >> > What do you think? >> >> >> >> Sorry, maybe this answers maybe doesn't. What is "PTC"? >> > >> > "pwm timer counter". AFAIU, the IP can be configured to provide all 3. >> > I think that William pointed out on an earlier revision that they have >> > only implemented the pwm on their hardware. >> > I don't think putting in "ptc" is a sufficient differentiator though, as >> > clearly there could be several different versions of "ptc-pwm" that have >> > the same concern about "all designs from OpenCores for now till the next >> > 100 years" being compatible. > > Perhaps noting what "ptc" stands for in the description field would be a > good idea. > I will add. >> After discussion and review of materials, we plan to use "opencores,ptc-pwm-v1" >> as this version of compatible, so that it can also be compatible in the future. >> >> What do you think? > > Do we know that it is actually "v1" of the IP? I would suggest using the > version that actually matches the version of the IP that you are using > in your SoC. > > Thanks, > Conor. There is no version list on their official website, so it is not certain whether it is v1, but at least the driver is the first version. What do you think is the best way? Thanks, William
On Fri, Nov 24, 2023 at 03:38:41PM +0800, William Qiu wrote: > > > On 2023/11/23 1:36, Conor Dooley wrote: > > On Wed, Nov 22, 2023 at 03:03:36PM +0800, William Qiu wrote: > >> > >> > >> On 2023/11/14 4:17, Conor Dooley wrote: > >> > On Mon, Nov 13, 2023 at 09:07:15PM +0100, Krzysztof Kozlowski wrote: > >> >> On 13/11/2023 10:42, William Qiu wrote: > >> >> > Will update. > >> >> >>> + > >> >> >>> +allOf: > >> >> >>> + - $ref: pwm.yaml# > >> >> >>> + > >> >> >>> +properties: > >> >> >>> + compatible: > >> >> >>> + oneOf: > >> >> >>> + - items: > >> >> >>> + - enum: > >> >> >>> + - starfive,jh7100-pwm > >> >> >>> + - starfive,jh7110-pwm > >> >> >>> + - const: opencores,pwm > >> >> >> > >> >> >> That's a very, very generic compatible. Are you sure, 100% sure, that > >> >> >> all designs from OpenCores from now till next 100 years will be 100% > >> >> >> compatible? > >> >> >> > >> >> > My description is not accurate enough, this is OpenCores PTC IP, and PWM > >> >> > is one of those modes, so it might be better to replace compatible with > >> >> > "opencores, ptc-pwm" > >> >> > > >> >> > What do you think? > >> >> > >> >> Sorry, maybe this answers maybe doesn't. What is "PTC"? > >> > > >> > "pwm timer counter". AFAIU, the IP can be configured to provide all 3. > >> > I think that William pointed out on an earlier revision that they have > >> > only implemented the pwm on their hardware. > >> > I don't think putting in "ptc" is a sufficient differentiator though, as > >> > clearly there could be several different versions of "ptc-pwm" that have > >> > the same concern about "all designs from OpenCores for now till the next > >> > 100 years" being compatible. > > > > Perhaps noting what "ptc" stands for in the description field would be a > > good idea. > > > I will add. > >> After discussion and review of materials, we plan to use "opencores,ptc-pwm-v1" > >> as this version of compatible, so that it can also be compatible in the future. > >> > >> What do you think? > > > > Do we know that it is actually "v1" of the IP? I would suggest using the > > version that actually matches the version of the IP that you are using > > in your SoC. > > > > Thanks, > > Conor. > > There is no version list on their official website, so it is not certain whether > it is v1, but at least the driver is the first version. > > What do you think is the best way? I don't have an account, so I cannot open the "ptc_spec.pdf at this link: https://opencores.org/projects/ptc/downloads but I would take whatever documentation you have for the spec and see what it says as the revision on the front cover.
diff --git a/Documentation/devicetree/bindings/pwm/opencores,pwm.yaml b/Documentation/devicetree/bindings/pwm/opencores,pwm.yaml new file mode 100644 index 000000000000..8f776bbc1112 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/opencores,pwm.yaml @@ -0,0 +1,56 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/opencores,pwm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: OpenCores PWM controller + +maintainers: + - William Qiu <william.qiu@starfivetech.com> + +description: + OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC core + generates binary signal with user-programmable low and high periods. All PTC counters and + registers are 32-bit. + +allOf: + - $ref: pwm.yaml# + +properties: + compatible: + oneOf: + - items: + - enum: + - starfive,jh7100-pwm + - starfive,jh7110-pwm + - const: opencores,pwm + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + + "#pwm-cells": + const: 3 + +required: + - compatible + - reg + - clocks + +additionalProperties: false + +examples: + - | + pwm@12490000 { + compatible = "starfive,jh7110-pwm", "opencores,pwm"; + reg = <0x12490000 0x10000>; + clocks = <&clkgen 181>; + resets = <&rstgen 109>; + #pwm-cells = <3>; + };