mbox series

[v7,0/4] StarFive's Pulse Width Modulation driver support

Message ID 20231110062039.103339-1-william.qiu@starfivetech.com
Headers show
Series StarFive's Pulse Width Modulation driver support | expand

Message

William Qiu Nov. 10, 2023, 6:20 a.m. UTC
Hi,

This patchset adds initial rudimentary support for the StarFive
Pulse Width Modulation controller driver. And this driver will
be used in StarFive's VisionFive 2 board.The first patch add
Documentations for the device and Patch 2 adds device probe for
the module.

Changes v6->v7:
- Rebased to v6.6.
- Added dependency architecture.
- Adopted new rounding algorithm.
- Added limitation descripton.
- Used function interfaces instead of macro definitions.
- Followed the linux coding style.

Changes v5->v6:
- Rebased to v6.6rc5.
- Changed driver into a generic OpenCores driver.
- Modified dt-bindings description into OpenCores.
- Uesd the StarFive compatible string to parameterize.

Changes v4->v5:
- Rebased to v6.6rc2.
- Updated macro definition indent.
- Replaced the clock initializes the interface.
- Fixed patch description.

Changes v3->v4:
- Rebased to v6.5rc7.
- Sorted the header files in alphabetic order.
- Changed iowrite32() to writel().
- Added a way to turn off.
- Modified polarity inversion implementation.
- Added 7100 support.
- Added dts patches.
- Used the various helpers in linux/math.h.
- Corrected formatting problems.
- Renamed dtbinding  to 'starfive,jh7100-pwm.yaml'.
- Dropped the redundant code.

Changes v2->v3:
- Fixed some formatting issues.

Changes v1->v2:
- Renamed the dt-binding 'pwm-starfive.yaml' to 'starfive,jh7110-pwm.yaml'.
- Dropped the compatible's Items.
- Dropped the unuse defines.
- Modified the code to follow the Linux coding style.
- Changed return value to dev_err_probe.
- Dropped the unnecessary local variable.

The patch series is based on v6.6.

William Qiu (4):
  dt-bindings: pwm: Add OpenCores PWM module
  pwm: opencores: Add PWM driver support
  riscv: dts: starfive: jh7110: Add PWM node and pins configuration
  riscv: dts: starfive: jh7100: Add PWM node and pins configuration

 .../bindings/pwm/opencores,pwm.yaml           |  56 +++++
 MAINTAINERS                                   |   7 +
 .../boot/dts/starfive/jh7100-common.dtsi      |  24 ++
 arch/riscv/boot/dts/starfive/jh7100.dtsi      |   9 +
 .../jh7110-starfive-visionfive-2.dtsi         |  22 ++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |   9 +
 drivers/pwm/Kconfig                           |  12 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-ocores.c                      | 236 ++++++++++++++++++
 9 files changed, 376 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm.yaml
 create mode 100644 drivers/pwm/pwm-ocores.c

--
2.34.1

Comments

William Qiu Nov. 13, 2023, 9:42 a.m. UTC | #1
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
>
Krzysztof Kozlowski Nov. 13, 2023, 8:07 p.m. UTC | #2
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
Conor Dooley Nov. 13, 2023, 8:17 p.m. UTC | #3
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.
Conor Dooley Nov. 22, 2023, 5:36 p.m. UTC | #4
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.
Conor Dooley Nov. 24, 2023, 12:44 p.m. UTC | #5
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.