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

[Jae Hyun Yoo]

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

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

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