2023-11-22 07:03:56

by William Qiu

[permalink] [raw]
Subject: Re: [PATCH v7 1/4] dt-bindings: pwm: Add OpenCores PWM module



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


2023-11-22 17:37:10

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH v7 1/4] dt-bindings: pwm: Add OpenCores PWM module

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.


Attachments:
(No filename) (1.97 kB)
signature.asc (235.00 B)
Download all attachments

2023-11-24 07:39:17

by William Qiu

[permalink] [raw]
Subject: Re: [PATCH v7 1/4] dt-bindings: pwm: Add OpenCores PWM module



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

2023-11-24 12:44:42

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH v7 1/4] dt-bindings: pwm: Add OpenCores PWM module

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.


Attachments:
(No filename) (2.69 kB)
signature.asc (235.00 B)
Download all attachments