[v3,04/10] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs

Commit Message

Jae Hyun Yoo April 10, 2018, 6:32 p.m. UTC

This commit adds a dt-bindings document of PECI adapter driver for Aspeed
AST24xx/25xx SoCs.

Signed-off-by: Jae Hyun Yoo <[e-mail]>
Reviewed-by: Haiyue Wang <[e-mail]>
Reviewed-by: James Feist <[e-mail]>
Reviewed-by: Vernon Mauery <[e-mail]>
Cc: Alan Cox <[e-mail]>
Cc: Andrew Jeffery <[e-mail]>
Cc: Andrew Lunn <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Arnd Bergmann <[e-mail]>
Cc: Benjamin Herrenschmidt <[e-mail]>
Cc: Fengguang Wu <[e-mail]>
Cc: Greg KH <[e-mail]>
Cc: Guenter Roeck <[e-mail]>
Cc: Jason M Biils <[e-mail]>
Cc: Jean Delvare <[e-mail]>
Cc: Joel Stanley <[e-mail]>
Cc: Julia Cartwright <[e-mail]>
Cc: Miguel Ojeda <[e-mail]>
Cc: Milton Miller II <[e-mail]>
Cc: Pavel Machek <[e-mail]>
Cc: Randy Dunlap <[e-mail]>
Cc: Stef van Os <[e-mail]>
Cc: Sumeet R Pawnikar <[e-mail]>
---
 .../devicetree/bindings/peci/peci-aspeed.txt       | 60 ++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt

Comments

Joel Stanley April 11, 2018, 11:52 a.m. UTC | #1

On 11 April 2018 at 04:02, Jae Hyun Yoo <[e-mail]> wrote:
> This commit adds a dt-bindings document of PECI adapter driver for Aspeed

We try to capitalise ASPEED.

> AST24xx/25xx SoCs.
> ---
>  .../devicetree/bindings/peci/peci-aspeed.txt       | 60 ++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
>
> diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> new file mode 100644
> index 000000000000..4598bb8c20fa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> @@ -0,0 +1,60 @@
> +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
> +
> +Required properties:
> +- compatible        : Should be "aspeed,ast2400-peci" or "aspeed,ast2500-peci"
> +                     - aspeed,ast2400-peci: Aspeed AST2400 family PECI
> +                                            controller
> +                     - aspeed,ast2500-peci: Aspeed AST2500 family PECI
> +                                            controller
> +- reg               : Should contain PECI controller registers location and
> +                     length.
> +- #address-cells    : Should be <1>.
> +- #size-cells       : Should be <0>.
> +- interrupts        : Should contain PECI controller interrupt.
> +- clocks            : Should contain clock source for PECI controller.
> +                     Should reference clkin.

Are you sure that this is driven by clkin? Most peripherals on the
Aspeed are attached to the apb, so should reference that clock.

> +- clock_frequency   : Should contain the operation frequency of PECI controller
> +                     in units of Hz.
> +                     187500 ~ 24000000

Can you explain why you need both the parent clock and this frequency
to be specified?

> +
> +Optional properties:
> +- msg-timing-nego   : Message timing negotiation period. This value will

Perhaps msg-timing-period? Or just msg-timing?

> +                     determine the period of message timing negotiation to be
> +                     issued by PECI controller. The unit of the programmed
> +                     value is four times of PECI clock period.
> +                     0 ~ 255 (default: 1)
> +- addr-timing-nego  : Address timing negotiation period. This value will
> +                     determine the period of address timing negotiation to be
> +                     issued by PECI controller. The unit of the programmed
> +                     value is four times of PECI clock period.
> +                     0 ~ 255 (default: 1)
> +- rd-sampling-point : Read sampling point selection. The whole period of a bit
> +                     time will be divided into 16 time frames. This value will
> +                     determine the time frame in which the controller will
> +                     sample PECI signal for data read back. Usually in the
> +                     middle of a bit time is the best.
> +                     0 ~ 15 (default: 8)
> +- cmd_timeout_ms    : Command timeout in units of ms.
> +                     1 ~ 60000 (default: 1000)
> +
> +Example:
> +       peci: peci@1e78b000 {
> +               compatible = "simple-bus";
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges = <0x0 0x1e78b000 0x60>;
> +
> +               peci0: peci-bus@0 {
> +                       compatible = "aspeed,ast2500-peci";
> +                       reg = <0x0 0x60>;
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       interrupts = <15>;
> +                       clocks = <&clk_clkin>;
> +                       clock-frequency = <24000000>;
> +                       msg-timing-nego = <1>;
> +                       addr-timing-nego = <1>;
> +                       rd-sampling-point = <8>;
> +                       cmd-timeout-ms = <1000>;
> +               };
> +       };
> --
> 2.16.2
>

Jae Hyun Yoo April 12, 2018, 2:11 a.m. UTC | #2

Hi Joel,

On 4/11/2018 4:52 AM, Joel Stanley wrote:
> On 11 April 2018 at 04:02, Jae Hyun Yoo <[e-mail]> wrote:
>> This commit adds a dt-bindings document of PECI adapter driver for Aspeed
> 
> We try to capitalise ASPEED.
> 

Got it. Will capitalize all Aspeed words.

>> AST24xx/25xx SoCs.
>> ---
>>   .../devicetree/bindings/peci/peci-aspeed.txt       | 60 ++++++++++++++++++++++
>>   1 file changed, 60 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
>>
>> diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> new file mode 100644
>> index 000000000000..4598bb8c20fa
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> @@ -0,0 +1,60 @@
>> +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
>> +
>> +Required properties:
>> +- compatible        : Should be "aspeed,ast2400-peci" or "aspeed,ast2500-peci"
>> +                     - aspeed,ast2400-peci: Aspeed AST2400 family PECI
>> +                                            controller
>> +                     - aspeed,ast2500-peci: Aspeed AST2500 family PECI
>> +                                            controller
>> +- reg               : Should contain PECI controller registers location and
>> +                     length.
>> +- #address-cells    : Should be <1>.
>> +- #size-cells       : Should be <0>.
>> +- interrupts        : Should contain PECI controller interrupt.
>> +- clocks            : Should contain clock source for PECI controller.
>> +                     Should reference clkin.
> 
> Are you sure that this is driven by clkin? Most peripherals on the
> Aspeed are attached to the apb, so should reference that clock.
> 

According to the datasheet, PECI controller module is attached to apb 
but its clock source is the 24MHz external clock.

>> +- clock_frequency   : Should contain the operation frequency of PECI controller
>> +                     in units of Hz.
>> +                     187500 ~ 24000000
> 
> Can you explain why you need both the parent clock and this frequency
> to be specified?
> 

Based on this setting, driver code makes clock divisor value to set 
operation clock of PECI controller which is adjustable.

>> +
>> +Optional properties:
>> +- msg-timing-nego   : Message timing negotiation period. This value will
> 
> Perhaps msg-timing-period? Or just msg-timing?
> 

Will use msg-timing instead.

>> +                     determine the period of message timing negotiation to be
>> +                     issued by PECI controller. The unit of the programmed
>> +                     value is four times of PECI clock period.
>> +                     0 ~ 255 (default: 1)
>> +- addr-timing-nego  : Address timing negotiation period. This value will
>> +                     determine the period of address timing negotiation to be
>> +                     issued by PECI controller. The unit of the programmed
>> +                     value is four times of PECI clock period.
>> +                     0 ~ 255 (default: 1)
>> +- rd-sampling-point : Read sampling point selection. The whole period of a bit
>> +                     time will be divided into 16 time frames. This value will
>> +                     determine the time frame in which the controller will
>> +                     sample PECI signal for data read back. Usually in the
>> +                     middle of a bit time is the best.
>> +                     0 ~ 15 (default: 8)
>> +- cmd_timeout_ms    : Command timeout in units of ms.
>> +                     1 ~ 60000 (default: 1000)
>> +
>> +Example:
>> +       peci: peci@1e78b000 {
>> +               compatible = "simple-bus";
>> +               #address-cells = <1>;
>> +               #size-cells = <1>;
>> +               ranges = <0x0 0x1e78b000 0x60>;
>> +
>> +               peci0: peci-bus@0 {
>> +                       compatible = "aspeed,ast2500-peci";
>> +                       reg = <0x0 0x60>;
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       interrupts = <15>;
>> +                       clocks = <&clk_clkin>;
>> +                       clock-frequency = <24000000>;
>> +                       msg-timing-nego = <1>;
>> +                       addr-timing-nego = <1>;
>> +                       rd-sampling-point = <8>;
>> +                       cmd-timeout-ms = <1000>;
>> +               };
>> +       };
>> --
>> 2.16.2
>>

Rob Herring April 16, 2018, 6:10 p.m. UTC | #3

On Tue, Apr 10, 2018 at 11:32:06AM -0700, Jae Hyun Yoo wrote:
> This commit adds a dt-bindings document of PECI adapter driver for Aspeed
> AST24xx/25xx SoCs.
> 
> Signed-off-by: Jae Hyun Yoo <[e-mail]>
> Reviewed-by: Haiyue Wang <[e-mail]>
> Reviewed-by: James Feist <[e-mail]>
> Reviewed-by: Vernon Mauery <[e-mail]>
> Cc: Alan Cox <[e-mail]>
> Cc: Andrew Jeffery <[e-mail]>
> Cc: Andrew Lunn <[e-mail]>
> Cc: Andy Shevchenko <[e-mail]>
> Cc: Arnd Bergmann <[e-mail]>
> Cc: Benjamin Herrenschmidt <[e-mail]>
> Cc: Fengguang Wu <[e-mail]>
> Cc: Greg KH <[e-mail]>
> Cc: Guenter Roeck <[e-mail]>
> Cc: Jason M Biils <[e-mail]>
> Cc: Jean Delvare <[e-mail]>
> Cc: Joel Stanley <[e-mail]>
> Cc: Julia Cartwright <[e-mail]>
> Cc: Miguel Ojeda <[e-mail]>
> Cc: Milton Miller II <[e-mail]>
> Cc: Pavel Machek <[e-mail]>
> Cc: Randy Dunlap <[e-mail]>
> Cc: Stef van Os <[e-mail]>
> Cc: Sumeet R Pawnikar <[e-mail]>
> ---
>  .../devicetree/bindings/peci/peci-aspeed.txt       | 60 ++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
> 
> diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> new file mode 100644
> index 000000000000..4598bb8c20fa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> @@ -0,0 +1,60 @@
> +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
> +
> +Required properties:
> +- compatible        : Should be "aspeed,ast2400-peci" or "aspeed,ast2500-peci"
> +		      - aspeed,ast2400-peci: Aspeed AST2400 family PECI
> +					     controller
> +		      - aspeed,ast2500-peci: Aspeed AST2500 family PECI
> +					     controller
> +- reg               : Should contain PECI controller registers location and
> +		      length.
> +- #address-cells    : Should be <1>.
> +- #size-cells       : Should be <0>.
> +- interrupts        : Should contain PECI controller interrupt.
> +- clocks            : Should contain clock source for PECI controller.
> +		      Should reference clkin.
> +- clock_frequency   : Should contain the operation frequency of PECI controller
> +		      in units of Hz.
> +		      187500 ~ 24000000

This is the frequency of the bus or used to derive it? It would be 
better to specify the bus frequency instead and have the driver 
calculate its internal freq. And then use "bus-frequency" instead.

> +
> +Optional properties:
> +- msg-timing-nego   : Message timing negotiation period. This value will
> +		      determine the period of message timing negotiation to be
> +		      issued by PECI controller. The unit of the programmed
> +		      value is four times of PECI clock period.
> +		      0 ~ 255 (default: 1)
> +- addr-timing-nego  : Address timing negotiation period. This value will
> +		      determine the period of address timing negotiation to be
> +		      issued by PECI controller. The unit of the programmed
> +		      value is four times of PECI clock period.
> +		      0 ~ 255 (default: 1)
> +- rd-sampling-point : Read sampling point selection. The whole period of a bit
> +		      time will be divided into 16 time frames. This value will
> +		      determine the time frame in which the controller will
> +		      sample PECI signal for data read back. Usually in the
> +		      middle of a bit time is the best.
> +		      0 ~ 15 (default: 8)
> +- cmd_timeout_ms    : Command timeout in units of ms.
> +		      1 ~ 60000 (default: 1000)

s/_/-/


All these either need vendor prefixes or should be standard properties 
for PECI adapters. I think probably the latter case. If so, the first 
2 should probably be in units of clocks (not 4 clocks). And they should 
then be documented in the common PECI binding doc.

> +
> +Example:
> +	peci: peci@1e78b000 {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x0 0x1e78b000 0x60>;

No need to show this part in examples.

> +
> +		peci0: peci-bus@0 {
> +			compatible = "aspeed,ast2500-peci";
> +			reg = <0x0 0x60>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			interrupts = <15>;
> +			clocks = <&clk_clkin>;
> +			clock-frequency = <24000000>;
> +			msg-timing-nego = <1>;
> +			addr-timing-nego = <1>;
> +			rd-sampling-point = <8>;
> +			cmd-timeout-ms = <1000>;
> +		};
> +	};
> -- 
> 2.16.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jae Hyun Yoo April 16, 2018, 11:12 p.m. UTC | #4

On 4/16/2018 11:10 AM, Rob Herring wrote:
> On Tue, Apr 10, 2018 at 11:32:06AM -0700, Jae Hyun Yoo wrote:
>> This commit adds a dt-bindings document of PECI adapter driver for Aspeed
>> AST24xx/25xx SoCs.
>>
>> Signed-off-by: Jae Hyun Yoo <[e-mail]>
>> Reviewed-by: Haiyue Wang <[e-mail]>
>> Reviewed-by: James Feist <[e-mail]>
>> Reviewed-by: Vernon Mauery <[e-mail]>
>> Cc: Alan Cox <[e-mail]>
>> Cc: Andrew Jeffery <[e-mail]>
>> Cc: Andrew Lunn <[e-mail]>
>> Cc: Andy Shevchenko <[e-mail]>
>> Cc: Arnd Bergmann <[e-mail]>
>> Cc: Benjamin Herrenschmidt <[e-mail]>
>> Cc: Fengguang Wu <[e-mail]>
>> Cc: Greg KH <[e-mail]>
>> Cc: Guenter Roeck <[e-mail]>
>> Cc: Jason M Biils <[e-mail]>
>> Cc: Jean Delvare <[e-mail]>
>> Cc: Joel Stanley <[e-mail]>
>> Cc: Julia Cartwright <[e-mail]>
>> Cc: Miguel Ojeda <[e-mail]>
>> Cc: Milton Miller II <[e-mail]>
>> Cc: Pavel Machek <[e-mail]>
>> Cc: Randy Dunlap <[e-mail]>
>> Cc: Stef van Os <[e-mail]>
>> Cc: Sumeet R Pawnikar <[e-mail]>
>> ---
>>   .../devicetree/bindings/peci/peci-aspeed.txt       | 60 ++++++++++++++++++++++
>>   1 file changed, 60 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
>>
>> diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> new file mode 100644
>> index 000000000000..4598bb8c20fa
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> @@ -0,0 +1,60 @@
>> +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
>> +
>> +Required properties:
>> +- compatible        : Should be "aspeed,ast2400-peci" or "aspeed,ast2500-peci"
>> +		      - aspeed,ast2400-peci: Aspeed AST2400 family PECI
>> +					     controller
>> +		      - aspeed,ast2500-peci: Aspeed AST2500 family PECI
>> +					     controller
>> +- reg               : Should contain PECI controller registers location and
>> +		      length.
>> +- #address-cells    : Should be <1>.
>> +- #size-cells       : Should be <0>.
>> +- interrupts        : Should contain PECI controller interrupt.
>> +- clocks            : Should contain clock source for PECI controller.
>> +		      Should reference clkin.
>> +- clock_frequency   : Should contain the operation frequency of PECI controller
>> +		      in units of Hz.
>> +		      187500 ~ 24000000
> 
> This is the frequency of the bus or used to derive it? It would be
> better to specify the bus frequency instead and have the driver
> calculate its internal freq. And then use "bus-frequency" instead.
> 

I agree with you. Actually, it is being used for operation frequency 
setting of PECI controller module in SoC so it's different from the 
meaning of "bus-frequency". I'll change it to "operation-frequency".

>> +
>> +Optional properties:
>> +- msg-timing-nego   : Message timing negotiation period. This value will
>> +		      determine the period of message timing negotiation to be
>> +		      issued by PECI controller. The unit of the programmed
>> +		      value is four times of PECI clock period.
>> +		      0 ~ 255 (default: 1)
>> +- addr-timing-nego  : Address timing negotiation period. This value will
>> +		      determine the period of address timing negotiation to be
>> +		      issued by PECI controller. The unit of the programmed
>> +		      value is four times of PECI clock period.
>> +		      0 ~ 255 (default: 1)
>> +- rd-sampling-point : Read sampling point selection. The whole period of a bit
>> +		      time will be divided into 16 time frames. This value will
>> +		      determine the time frame in which the controller will
>> +		      sample PECI signal for data read back. Usually in the
>> +		      middle of a bit time is the best.
>> +		      0 ~ 15 (default: 8)
>> +- cmd_timeout_ms    : Command timeout in units of ms.
>> +		      1 ~ 60000 (default: 1000)
> 
> s/_/-/
> 

Will fix it.

> 
> All these either need vendor prefixes or should be standard properties
> for PECI adapters. I think probably the latter case. If so, the first
> 2 should probably be in units of clocks (not 4 clocks). And they should
> then be documented in the common PECI binding doc.
> 

So far I've checked that these are ASPEED PECI controller specific 
properties so it should be listed in here.

>> +
>> +Example:
>> +	peci: peci@1e78b000 {
>> +		compatible = "simple-bus";
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		ranges = <0x0 0x1e78b000 0x60>;
> 
> No need to show this part in examples.
> 

Got it. Will drop the part.

>> +
>> +		peci0: peci-bus@0 {
>> +			compatible = "aspeed,ast2500-peci";
>> +			reg = <0x0 0x60>;
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			interrupts = <15>;
>> +			clocks = <&clk_clkin>;
>> +			clock-frequency = <24000000>;
>> +			msg-timing-nego = <1>;
>> +			addr-timing-nego = <1>;
>> +			rd-sampling-point = <8>;
>> +			cmd-timeout-ms = <1000>;
>> +		};
>> +	};
>> -- 
>> 2.16.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rob Herring April 17, 2018, 1:16 p.m. UTC | #5

On Mon, Apr 16, 2018 at 6:12 PM, Jae Hyun Yoo
<[e-mail]> wrote:
> On 4/16/2018 11:10 AM, Rob Herring wrote:
>>
>> On Tue, Apr 10, 2018 at 11:32:06AM -0700, Jae Hyun Yoo wrote:
>>>
>>> This commit adds a dt-bindings document of PECI adapter driver for Aspeed
>>> AST24xx/25xx SoCs.

[...]

>>> +- clocks            : Should contain clock source for PECI controller.
>>> +                     Should reference clkin.
>>> +- clock_frequency   : Should contain the operation frequency of PECI
>>> controller
>>> +                     in units of Hz.
>>> +                     187500 ~ 24000000
>>
>>
>> This is the frequency of the bus or used to derive it? It would be
>> better to specify the bus frequency instead and have the driver
>> calculate its internal freq. And then use "bus-frequency" instead.
>>
>
> I agree with you. Actually, it is being used for operation frequency setting
> of PECI controller module in SoC so it's different from the meaning of
> "bus-frequency". I'll change it to "operation-frequency".

No, now you've gone from a standard property name to something custom.
Why do you need to set the frequency in DT if it is not related to the
interface frequency?

Rob

Jae Hyun Yoo April 17, 2018, 6:16 p.m. UTC | #6

On 4/17/2018 6:16 AM, Rob Herring wrote:
> On Mon, Apr 16, 2018 at 6:12 PM, Jae Hyun Yoo
> <[e-mail]> wrote:
>> On 4/16/2018 11:10 AM, Rob Herring wrote:
>>>
>>> On Tue, Apr 10, 2018 at 11:32:06AM -0700, Jae Hyun Yoo wrote:
>>>>
>>>> This commit adds a dt-bindings document of PECI adapter driver for Aspeed
>>>> AST24xx/25xx SoCs.
> 
> [...]
> 
>>>> +- clocks            : Should contain clock source for PECI controller.
>>>> +                     Should reference clkin.
>>>> +- clock_frequency   : Should contain the operation frequency of PECI
>>>> controller
>>>> +                     in units of Hz.
>>>> +                     187500 ~ 24000000
>>>
>>>
>>> This is the frequency of the bus or used to derive it? It would be
>>> better to specify the bus frequency instead and have the driver
>>> calculate its internal freq. And then use "bus-frequency" instead.
>>>
>>
>> I agree with you. Actually, it is being used for operation frequency setting
>> of PECI controller module in SoC so it's different from the meaning of
>> "bus-frequency". I'll change it to "operation-frequency".
> 
> No, now you've gone from a standard property name to something custom.
> Why do you need to set the frequency in DT if it is not related to the
> interface frequency?
> 
> Rob
> 

Actually, the interface frequency is affected by the operation frequency
but there is no description of its relationship in datasheet. I'll check
again about the detail to ASPEED chip vendor and will use
'bus-frequency' if available.

Thanks,

Jae

Jae Hyun Yoo April 17, 2018, 10:06 p.m. UTC | #7

On 4/17/2018 11:16 AM, Jae Hyun Yoo wrote:
> On 4/17/2018 6:16 AM, Rob Herring wrote:
>> On Mon, Apr 16, 2018 at 6:12 PM, Jae Hyun Yoo
>> <[e-mail]> wrote:
>>> On 4/16/2018 11:10 AM, Rob Herring wrote:
>>>>
>>>> On Tue, Apr 10, 2018 at 11:32:06AM -0700, Jae Hyun Yoo wrote:
>>>>>
>>>>> This commit adds a dt-bindings document of PECI adapter driver for 
>>>>> Aspeed
>>>>> AST24xx/25xx SoCs.
>>
>> [...]
>>
>>>>> +- clocks            : Should contain clock source for PECI 
>>>>> controller.
>>>>> +                     Should reference clkin.
>>>>> +- clock_frequency   : Should contain the operation frequency of PECI
>>>>> controller
>>>>> +                     in units of Hz.
>>>>> +                     187500 ~ 24000000
>>>>
>>>>
>>>> This is the frequency of the bus or used to derive it? It would be
>>>> better to specify the bus frequency instead and have the driver
>>>> calculate its internal freq. And then use "bus-frequency" instead.
>>>>
>>>
>>> I agree with you. Actually, it is being used for operation frequency 
>>> setting
>>> of PECI controller module in SoC so it's different from the meaning of
>>> "bus-frequency". I'll change it to "operation-frequency".
>>
>> No, now you've gone from a standard property name to something custom.
>> Why do you need to set the frequency in DT if it is not related to the
>> interface frequency?
>>
>> Rob
>>
> 
> Actually, the interface frequency is affected by the operation frequency
> but there is no description of its relationship in datasheet. I'll check
> again about the detail to ASPEED chip vendor and will use
> 'bus-frequency' if available.
> 

I investigated it more deeply. Basically, by the spec, PECI bus speed
cannot be set as a fixed speed. A PECI bus can have a wide speed range
from 2Kbps to 2Mbps which is dynamically set by a handshaking sequence
between an originator and clients called 'timing negotiation' in spec.
This timing negotiation behavior happens on every single transaction so 
the bus speed also can vary on every transactions. So I'm thinking a 
custom property name for it, 'peci-clk-frequency' if it is acceptable.

Thanks,

Jae

Rob Herring April 18, 2018, 1:59 p.m. UTC | #8

On Tue, Apr 17, 2018 at 5:06 PM, Jae Hyun Yoo
<[e-mail]> wrote:
> On 4/17/2018 11:16 AM, Jae Hyun Yoo wrote:
>>
>> On 4/17/2018 6:16 AM, Rob Herring wrote:
>>>
>>> On Mon, Apr 16, 2018 at 6:12 PM, Jae Hyun Yoo
>>> <[e-mail]> wrote:
>>>>
>>>> On 4/16/2018 11:10 AM, Rob Herring wrote:
>>>>>
>>>>>
>>>>> On Tue, Apr 10, 2018 at 11:32:06AM -0700, Jae Hyun Yoo wrote:
>>>>>>
>>>>>>
>>>>>> This commit adds a dt-bindings document of PECI adapter driver for
>>>>>> Aspeed
>>>>>> AST24xx/25xx SoCs.
>>>
>>>
>>> [...]
>>>
>>>>>> +- clocks            : Should contain clock source for PECI
>>>>>> controller.
>>>>>> +                     Should reference clkin.
>>>>>> +- clock_frequency   : Should contain the operation frequency of PECI
>>>>>> controller
>>>>>> +                     in units of Hz.
>>>>>> +                     187500 ~ 24000000
>>>>>
>>>>>
>>>>>
>>>>> This is the frequency of the bus or used to derive it? It would be
>>>>> better to specify the bus frequency instead and have the driver
>>>>> calculate its internal freq. And then use "bus-frequency" instead.
>>>>>
>>>>
>>>> I agree with you. Actually, it is being used for operation frequency
>>>> setting
>>>> of PECI controller module in SoC so it's different from the meaning of
>>>> "bus-frequency". I'll change it to "operation-frequency".
>>>
>>>
>>> No, now you've gone from a standard property name to something custom.
>>> Why do you need to set the frequency in DT if it is not related to the
>>> interface frequency?
>>>
>>> Rob
>>>
>>
>> Actually, the interface frequency is affected by the operation frequency
>> but there is no description of its relationship in datasheet. I'll check
>> again about the detail to ASPEED chip vendor and will use
>> 'bus-frequency' if available.
>>
>
> I investigated it more deeply. Basically, by the spec, PECI bus speed
> cannot be set as a fixed speed. A PECI bus can have a wide speed range
> from 2Kbps to 2Mbps which is dynamically set by a handshaking sequence
> between an originator and clients called 'timing negotiation' in spec.
> This timing negotiation behavior happens on every single transaction so the
> bus speed also can vary on every transactions. So I'm thinking a custom
> property name for it, 'peci-clk-frequency' if it is acceptable.

Okay, seems bus-frequency is not appropriate here. So use
'clock-frequency' (note the '-' not '_' as that is the standard
property).

Rob

Jae Hyun Yoo April 18, 2018, 4:45 p.m. UTC | #9

On 4/18/2018 6:59 AM, Rob Herring wrote:
> On Tue, Apr 17, 2018 at 5:06 PM, Jae Hyun Yoo
> <[e-mail]> wrote:
>> On 4/17/2018 11:16 AM, Jae Hyun Yoo wrote:
>>>
>>> On 4/17/2018 6:16 AM, Rob Herring wrote:
>>>>
>>>> On Mon, Apr 16, 2018 at 6:12 PM, Jae Hyun Yoo
>>>> <[e-mail]> wrote:
>>>>>
>>>>> On 4/16/2018 11:10 AM, Rob Herring wrote:
>>>>>>
>>>>>>
>>>>>> On Tue, Apr 10, 2018 at 11:32:06AM -0700, Jae Hyun Yoo wrote:
>>>>>>>
>>>>>>>
>>>>>>> This commit adds a dt-bindings document of PECI adapter driver for
>>>>>>> Aspeed
>>>>>>> AST24xx/25xx SoCs.
>>>>
>>>>
>>>> [...]
>>>>
>>>>>>> +- clocks            : Should contain clock source for PECI
>>>>>>> controller.
>>>>>>> +                     Should reference clkin.
>>>>>>> +- clock_frequency   : Should contain the operation frequency of PECI
>>>>>>> controller
>>>>>>> +                     in units of Hz.
>>>>>>> +                     187500 ~ 24000000
>>>>>>
>>>>>>
>>>>>>
>>>>>> This is the frequency of the bus or used to derive it? It would be
>>>>>> better to specify the bus frequency instead and have the driver
>>>>>> calculate its internal freq. And then use "bus-frequency" instead.
>>>>>>
>>>>>
>>>>> I agree with you. Actually, it is being used for operation frequency
>>>>> setting
>>>>> of PECI controller module in SoC so it's different from the meaning of
>>>>> "bus-frequency". I'll change it to "operation-frequency".
>>>>
>>>>
>>>> No, now you've gone from a standard property name to something custom.
>>>> Why do you need to set the frequency in DT if it is not related to the
>>>> interface frequency?
>>>>
>>>> Rob
>>>>
>>>
>>> Actually, the interface frequency is affected by the operation frequency
>>> but there is no description of its relationship in datasheet. I'll check
>>> again about the detail to ASPEED chip vendor and will use
>>> 'bus-frequency' if available.
>>>
>>
>> I investigated it more deeply. Basically, by the spec, PECI bus speed
>> cannot be set as a fixed speed. A PECI bus can have a wide speed range
>> from 2Kbps to 2Mbps which is dynamically set by a handshaking sequence
>> between an originator and clients called 'timing negotiation' in spec.
>> This timing negotiation behavior happens on every single transaction so the
>> bus speed also can vary on every transactions. So I'm thinking a custom
>> property name for it, 'peci-clk-frequency' if it is acceptable.
> 
> Okay, seems bus-frequency is not appropriate here. So use
> 'clock-frequency' (note the '-' not '_' as that is the standard
> property).
> 
> Rob
> 

Thanks! I'll use 'clock-frequency' for it.

Jae

Patch

diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
new file mode 100644
index 000000000000..4598bb8c20fa
--- /dev/null
+++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
@@ -0,0 +1,60 @@ 
+Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
+
+Required properties:
+- compatible        : Should be "aspeed,ast2400-peci" or "aspeed,ast2500-peci"
+		      - aspeed,ast2400-peci: Aspeed AST2400 family PECI
+					     controller
+		      - aspeed,ast2500-peci: Aspeed AST2500 family PECI
+					     controller
+- reg               : Should contain PECI controller registers location and
+		      length.
+- #address-cells    : Should be <1>.
+- #size-cells       : Should be <0>.
+- interrupts        : Should contain PECI controller interrupt.
+- clocks            : Should contain clock source for PECI controller.
+		      Should reference clkin.
+- clock_frequency   : Should contain the operation frequency of PECI controller
+		      in units of Hz.
+		      187500 ~ 24000000
+
+Optional properties:
+- msg-timing-nego   : Message timing negotiation period. This value will
+		      determine the period of message timing negotiation to be
+		      issued by PECI controller. The unit of the programmed
+		      value is four times of PECI clock period.
+		      0 ~ 255 (default: 1)
+- addr-timing-nego  : Address timing negotiation period. This value will
+		      determine the period of address timing negotiation to be
+		      issued by PECI controller. The unit of the programmed
+		      value is four times of PECI clock period.
+		      0 ~ 255 (default: 1)
+- rd-sampling-point : Read sampling point selection. The whole period of a bit
+		      time will be divided into 16 time frames. This value will
+		      determine the time frame in which the controller will
+		      sample PECI signal for data read back. Usually in the
+		      middle of a bit time is the best.
+		      0 ~ 15 (default: 8)
+- cmd_timeout_ms    : Command timeout in units of ms.
+		      1 ~ 60000 (default: 1000)
+
+Example:
+	peci: peci@1e78b000 {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x1e78b000 0x60>;
+
+		peci0: peci-bus@0 {
+			compatible = "aspeed,ast2500-peci";
+			reg = <0x0 0x60>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			interrupts = <15>;
+			clocks = <&clk_clkin>;
+			clock-frequency = <24000000>;
+			msg-timing-nego = <1>;
+			addr-timing-nego = <1>;
+			rd-sampling-point = <8>;
+			cmd-timeout-ms = <1000>;
+		};
+	};