[1/6] Documentation: dt-bindings: Add Aspeed PECI

Message ID 20171206210240.6996-1-jae.hyun.yoo@linux.intel.com
State Changes Requested, archived
Headers show
Series
  • Add Aspeed PECI support
Related show

Commit Message

Jae Hyun Yoo Dec. 6, 2017, 9:02 p.m.
This commit adds a dt-bindings document for Aspeed PECI.

Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
---
 .../devicetree/bindings/misc/aspeed-peci.txt       | 52 ++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/aspeed-peci.txt

Comments

Andrew Jeffery Dec. 6, 2017, 9:42 p.m. | #1
Hi Jae Hun Yoo,

Nice work! Some comments below

On Thu, 7 Dec 2017, at 07:32, Jae Hyun Yoo wrote:
> This commit adds a dt-bindings document for Aspeed PECI.
> 
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
>  .../devicetree/bindings/misc/aspeed-peci.txt       | 52
>  ++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644
>  Documentation/devicetree/bindings/misc/aspeed-peci.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/aspeed-peci.txt
> b/Documentation/devicetree/bindings/misc/aspeed-peci.txt
> new file mode 100644
> index 000000000000..af950d72a48c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/aspeed-peci.txt
> @@ -0,0 +1,52 @@
> +* ASPEED PECI (Platform Environment Control Interface) misc driver.
> +
> +Hardware Interfaces:
> +- This driver implements support for the ASPEED AST2400/2500 PECI which
> has the
> +  following features:
> +       - Directly connected to APB bus
> +       - Intel PECI 3.1 compliant (PECI 3.0 for AST2400)
> +       - Maximum packet length is 256 bytes (Baseline transmission unit)
> +       - Support up to 8 CPUs and 2 domains per CPU
> +       - Integrate PECI compliant I/O buffers, can connect to PECI bus
> directly
> +       - Transmit buffer 32 bytes and receive buffer 32 bytes
> +
> +Required properties:
> +- compatible: "aspeed,ast2400-peci" or "aspeed,ast2500-peci"
> +       - aspeed,ast2400-peci: Aspeed AST2400 family PECI control
> interface
> +       - aspeed,ast2500-peci: Aspeed AST2500 family PECI control
> interface
> +- reg: Should contain PECI registers location and length
> +- interrupts: Should contain PECI interrupt
> +
> +Optional properties:
> +- msg_timing_nego: Message timing negotiation period

Typically we use '-' rather than '_' as a separator in property names. I
notice your example uses '-'; please change the documentation.

Separately, what unit is this property working with? Clock cycles? It's
not clear what picking any particular value means.

The two problems apply to the next few properties as well:

> +       0 ~ 255 (default: 1)
> +- addr_timing_nego: Address timing negotiation period
> +       0 ~ 255 (default: 1)
> +- rd_sampling_point: Read sampling point selection
> +       0 ~ 15 (default: 8)

> +- clk_div: Clock divider for 24MHz clock source
> +       0: Divide by 1   <- default
> +       1: Divide by 2
> +       2: Divide by 4
> +       3: Divide by 8
> +       4: Divide by 16
> +       5: Divide by 32
> +       6: Divide by 64
> +       7: Divide by 128

What is the purpose of the clock divider? To derive the peci bus
frequency? If so, you should instead use the generic 'bus-frequency'
property, and implement support in the driver to derive the described
frequency from the upstream clock rate and the value of bus-frequency
specified in the devicetree node. That is, derive the divisor value and
program it into the hardware. You should also document the 'clocks' and
'clock-names' properties here in order to describe the upstream clock

> +- cmd_timeout_ms: Command timeout in units of ms
> +       1 ~ 60000 (default: 1000)

This is a contrast to your properties above in the the unit is clear,
which is good.

> +
> +Example:
> +       peci: peci@1e78b000 {
> +               compatible = "aspeed,ast2500-peci", "syscon",
> "simple-mfd";

If you need it to be a syscon you should say so in the documentation as
well as the example. But:  

Do you actually need a syscon here? why? Do we need to share these
registers across multiple drivers? I'd be surprised if this controller
fits the syscon description.

If you want a regmap you can create one inside the driver without
affecting the devicetree. But I'd also question why you want one - they
introduce overhead where you probably don't need it.

Cheers,

Andrew

> +               reg = <0x1e78b000 0x60>;
> +               reg-io-width = <4>;
> +               interrupt-controller;
> +               interrupts = <15>;
> +               msg-timing-nego = <1>;
> +               addr-timing-nego = <1>;
> +               rd-sampling-point = <8>;
> +               clk-div = <0>;
> +               cmd-timeout-ms = <1000>;
> +       };
> +
> -- 
> 2.15.1
>
Jae Hyun Yoo Dec. 6, 2017, 10:24 p.m. | #2
On 12/6/2017 1:42 PM, Andrew Jeffery wrote:
> Hi Jae Hun Yoo,
> 
> Nice work! Some comments below
> 
> On Thu, 7 Dec 2017, at 07:32, Jae Hyun Yoo wrote:
>> This commit adds a dt-bindings document for Aspeed PECI.
>>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>>   .../devicetree/bindings/misc/aspeed-peci.txt       | 52
>>   ++++++++++++++++++++++
>>   1 file changed, 52 insertions(+)
>>   create mode 100644
>>   Documentation/devicetree/bindings/misc/aspeed-peci.txt
>>
>> diff --git a/Documentation/devicetree/bindings/misc/aspeed-peci.txt
>> b/Documentation/devicetree/bindings/misc/aspeed-peci.txt
>> new file mode 100644
>> index 000000000000..af950d72a48c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/aspeed-peci.txt
>> @@ -0,0 +1,52 @@
>> +* ASPEED PECI (Platform Environment Control Interface) misc driver.
>> +
>> +Hardware Interfaces:
>> +- This driver implements support for the ASPEED AST2400/2500 PECI which
>> has the
>> +  following features:
>> +       - Directly connected to APB bus
>> +       - Intel PECI 3.1 compliant (PECI 3.0 for AST2400)
>> +       - Maximum packet length is 256 bytes (Baseline transmission unit)
>> +       - Support up to 8 CPUs and 2 domains per CPU
>> +       - Integrate PECI compliant I/O buffers, can connect to PECI bus
>> directly
>> +       - Transmit buffer 32 bytes and receive buffer 32 bytes
>> +
>> +Required properties:
>> +- compatible: "aspeed,ast2400-peci" or "aspeed,ast2500-peci"
>> +       - aspeed,ast2400-peci: Aspeed AST2400 family PECI control
>> interface
>> +       - aspeed,ast2500-peci: Aspeed AST2500 family PECI control
>> interface
>> +- reg: Should contain PECI registers location and length
>> +- interrupts: Should contain PECI interrupt
>> +
>> +Optional properties:
>> +- msg_timing_nego: Message timing negotiation period
> 
> Typically we use '-' rather than '_' as a separator in property names. I
> notice your example uses '-'; please change the documentation.
> 

Ah, I missed this part. Thanks for your pointing it out.

> Separately, what unit is this property working with? Clock cycles? It's
> not clear what picking any particular value means.
> 
> The two problems apply to the next few properties as well:
> 

Sure, I'll fix all these and will add more details.

>> +       0 ~ 255 (default: 1)
>> +- addr_timing_nego: Address timing negotiation period
>> +       0 ~ 255 (default: 1)
>> +- rd_sampling_point: Read sampling point selection
>> +       0 ~ 15 (default: 8)
> 
>> +- clk_div: Clock divider for 24MHz clock source
>> +       0: Divide by 1   <- default
>> +       1: Divide by 2
>> +       2: Divide by 4
>> +       3: Divide by 8
>> +       4: Divide by 16
>> +       5: Divide by 32
>> +       6: Divide by 64
>> +       7: Divide by 128
> 
> What is the purpose of the clock divider? To derive the peci bus
> frequency? If so, you should instead use the generic 'bus-frequency'
> property, and implement support in the driver to derive the described
> frequency from the upstream clock rate and the value of bus-frequency
> specified in the devicetree node. That is, derive the divisor value and
> program it into the hardware. You should also document the 'clocks' and
> 'clock-names' properties here in order to describe the upstream clock
> 

Yes, right. I totally agree with you. I'll add calculation logic for bus 
clock into driver. Thanks!

>> +- cmd_timeout_ms: Command timeout in units of ms
>> +       1 ~ 60000 (default: 1000)
> 
> This is a contrast to your properties above in the the unit is clear,
> which is good.
> 
>> +
>> +Example:
>> +       peci: peci@1e78b000 {
>> +               compatible = "aspeed,ast2500-peci", "syscon",
>> "simple-mfd";
> 
> If you need it to be a syscon you should say so in the documentation as
> well as the example. But:
> 
> Do you actually need a syscon here? why? Do we need to share these
> registers across multiple drivers? I'd be surprised if this controller
> fits the syscon description.
> 
> If you want a regmap you can create one inside the driver without
> affecting the devicetree. But I'd also question why you want one - they
> introduce overhead where you probably don't need it.
> 

I simply added a syscon to use syscon_node_to_regmap api but I didn't 
consider the issue you pointed out. I'll remove the syscon and will make 
driver to use devm_regmap_init_mmio instead.

> Cheers,
> 
> Andrew
> 

Thanks a lot for sharing your time on reviewing this. Will address all 
you pointed out.

BR,

Jae

>> +               reg = <0x1e78b000 0x60>;
>> +               reg-io-width = <4>;
>> +               interrupt-controller;
>> +               interrupts = <15>;
>> +               msg-timing-nego = <1>;
>> +               addr-timing-nego = <1>;
>> +               rd-sampling-point = <8>;
>> +               clk-div = <0>;
>> +               cmd-timeout-ms = <1000>;
>> +       };
>> +
>> -- 
>> 2.15.1
>>

Patch

diff --git a/Documentation/devicetree/bindings/misc/aspeed-peci.txt b/Documentation/devicetree/bindings/misc/aspeed-peci.txt
new file mode 100644
index 000000000000..af950d72a48c
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/aspeed-peci.txt
@@ -0,0 +1,52 @@ 
+* ASPEED PECI (Platform Environment Control Interface) misc driver.
+
+Hardware Interfaces:
+- This driver implements support for the ASPEED AST2400/2500 PECI which has the
+  following features:
+	- Directly connected to APB bus
+	- Intel PECI 3.1 compliant (PECI 3.0 for AST2400)
+	- Maximum packet length is 256 bytes (Baseline transmission unit)
+	- Support up to 8 CPUs and 2 domains per CPU
+	- Integrate PECI compliant I/O buffers, can connect to PECI bus directly
+	- Transmit buffer 32 bytes and receive buffer 32 bytes
+
+Required properties:
+- compatible: "aspeed,ast2400-peci" or "aspeed,ast2500-peci"
+	- aspeed,ast2400-peci: Aspeed AST2400 family PECI control interface
+	- aspeed,ast2500-peci: Aspeed AST2500 family PECI control interface
+- reg: Should contain PECI registers location and length
+- interrupts: Should contain PECI interrupt
+
+Optional properties:
+- msg_timing_nego: Message timing negotiation period
+	0 ~ 255 (default: 1)
+- addr_timing_nego: Address timing negotiation period
+	0 ~ 255 (default: 1)
+- rd_sampling_point: Read sampling point selection
+	0 ~ 15 (default: 8)
+- clk_div: Clock divider for 24MHz clock source
+	0: Divide by 1   <- default
+	1: Divide by 2
+	2: Divide by 4
+	3: Divide by 8
+	4: Divide by 16
+	5: Divide by 32
+	6: Divide by 64
+	7: Divide by 128
+- cmd_timeout_ms: Command timeout in units of ms
+	1 ~ 60000 (default: 1000)
+
+Example:
+	peci: peci@1e78b000 {
+		compatible = "aspeed,ast2500-peci", "syscon", "simple-mfd";
+		reg = <0x1e78b000 0x60>;
+		reg-io-width = <4>;
+		interrupt-controller;
+		interrupts = <15>;
+		msg-timing-nego = <1>;
+		addr-timing-nego = <1>;
+		rd-sampling-point = <8>;
+		clk-div = <0>;
+		cmd-timeout-ms = <1000>;
+	};
+