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

[Andrew Jeffery]

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 at 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 at 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
>