Documentation: dt-bindings: Add Aspeed PECI
Subsystems (2)
PATCH 1/6 Documentation: dt-bindings: Add Aspeed PECI
This commit adds a dt-bindings document for Aspeed PECI.
--- .../devicetree/bindings/misc/aspeed-peci.txt | 52 ++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/misc/aspeed-peci.txt
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +52 -0
@@ -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>; + }; +
Signature · 1 line
2.15.1
Hi Jae Hun Yoo,
Nice work! Some comments below
Quoted 39 lines · Jae Hyun Yoo
This commit adds a dt-bindings document for Aspeed PECI.
Signed-off-by Jae Hyun Yoo <[email protected]>--- .../devicetree/bindings/misc/aspeed-peci.txt | 52 ++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644
Documentation/devicetree/bindings/misc/aspeed-peci.txtDocumentation/devicetree/bindings/misc/aspeed-peci.txt new file +4
-0@@ -0,0 +1,52 @@ +* ASPEED PECI (Platform Environment Control Interface) misc driver. + +Hardware Interfaces: +- This driver implements support for the ASPEED AST2400/2500 PECI whichhas the
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +6
-0+ 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 busdirectly
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +5
-0+ - 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 controlinterface
+ - aspeed,ast2500-peci: Aspeed AST2500 family PECI control
interface+- reg: Should contain PECI registers location and length +- interrupts: Should contain PECI interrupt
Documentation/devicetree/bindings/misc/aspeed-peci.txt line 21
Quoted 3 lines
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +3
-0+ +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:
Quoted 5 lines
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +5
-0+ 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)
Documentation/devicetree/bindings/misc/aspeed-peci.txt line 35
Quoted 10 lines
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +9
-0+- 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
Quoted 2 lines
+- 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.
Quoted 5 lines
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +4
-0+ +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
Quoted 14 lines
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +11
-0+ 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>; + }; +Signature · 1 line
2.15.1
Quoted 42 lines · Andrew Jeffery
Hi Jae Hun Yoo,
Nice work! Some comments below
Quoted 37 lines · Jae Hyun Yoo
This commit adds a dt-bindings document for Aspeed PECI.
Signed-off-by Jae Hyun Yoo <[email protected]>--- .../devicetree/bindings/misc/aspeed-peci.txt | 52 ++++++++++++++++++++++ 1 file changed, 52 insertions(+)create mode 100644
Documentation/devicetree/bindings/misc/aspeed-peci.txtDocumentation/devicetree/bindings/misc/aspeed-peci.txt new file +4
-0@@ -0,0 +1,52 @@ +* ASPEED PECI (Platform Environment Control Interface) misc driver. + +Hardware Interfaces: +- This driver implements support for the ASPEED AST2400/2500 PECI whichhas the
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +6
-0+ 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 busdirectly
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +5
-0+ - 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 controlinterface
+ - aspeed,ast2500-peci: Aspeed AST2500 family PECI control
interface
Quoted 9 lines
Documentation/devicetree/bindings/misc/aspeed-peci.txt line 21
Quoted 5 lines
+- reg: Should contain PECI registers location and length +- interrupts: Should contain PECI interruptDocumentation/devicetree/bindings/misc/aspeed-peci.txt new file +3
-0+ +Optional properties: +- msg_timing_nego: Message timing negotiation periodTypically 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.
Quoted 5 lines
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.
Quoted 16 lines
Quoted 5 lines
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +5
-0+ 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)Documentation/devicetree/bindings/misc/aspeed-peci.txt line 35
Quoted 10 lines
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +9
-0+- 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
Quoted 8 lines
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!
Quoted 15 lines
Quoted 2 lines
+- 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.
Quoted 5 lines
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +4
-0+ +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:
Quoted 8 lines
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.
Quoted 4 lines
Cheers,
Andrew
Thanks a lot for sharing your time on reviewing this. Will address all you pointed out.
BR,
Jae
Quoted 14 lines
Quoted 14 lines
Documentation/devicetree/bindings/misc/aspeed-peci.txt new file +11
-0+ 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>; + }; +Signature · 1 line
2.15.1