Re: [PATCH v12 3/3] dt-bindings: mfd: Document Renesas R-Car Gen3 RPC-IF MFD bindings

[Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>]

On 05/14/2019 12:46 PM, masonccyang@xxxxxxxxxxx wrote:

>>>> There's precedence for such constructs being an MFD: please see
>>>> drivers/mfd/at91-usart.c, which registers a single MFD cell for 
> either
>>>> serial or SPI.
>>
>>    Thanks fir your example, Geert! :-)

   s/fir/for/, not the firtree season anymore. :-)

>>> okay, many thanks for your information.
>>>
>>> How about to patch RPF-IF dts to:
>>> -------------------------------------------------------------->
>>>
>>> Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
>>> ---------------------------------------------------------
>>>
>>>   RPC-IF supports both SPI NOR and HyperFlash (CFI-compliant flash)
>>>
>>>   Required properties:
>>>   - compatible: should be an SoC-specific compatible value, followed 
> by
>>>                   "renesas,rcar-gen3-rpc" as a fallback.
>>>                   supported SoC-specific values are:
>>>                   "renesas,r8a77995-rpc"  (R-Car D3)
>>>   - reg: should contain three register areas:
>>>           first for the base address of RPC-IF registers,
>>
>>    I'd drop "the base address" here.
> 
> okay.
> 
>>>           second for the direct mapping read mode and
>>>           third for the write buffer area.
>>>   - reg-names: should contain "regs", "dirmap" and "wbuf"
>>>   - clocks: should contain 1 entries for the module's clock
>>>   - clock-names: should contain "rpc"
>>
>>    I suspect we'd need the RPC/RPCD2 clocks mentioned as well (not sure 
> yet)...
> 
> Need it ?

   You seem to call clk_get_rate() on the module clock, I doubt that's
correct topologically...

> RPCD2 is derived from RPC and it's value is half of RPC,
> i.e., RPC = 160MHz, RPCD2 = 80 MHz

   I know.

>>    And how about "power-domains", "resets" (seen in the example below),
>> also what about #address-cells & #size-cells?
>>
>>>
>>>   Example:
>>
>>    Could you please indent with 1 or 2 tabs where you used 8 or 16 
> spaces?
>>
>>>   - SPI mode:
>>>
>>>           rpc: rpc-if@ee200000 {
>>
>>    The node names should be generic, based on the device class. And in 
> this
>> case I'd like to use "spi@ee200000" as otherwise dtc keeps bitching like 
> below:
> 
> okay, patch to
> 
> rpc_if: spi@<...>

   That, or just keep the node label.

>> arch/arm64/boot/dts/renesas/r8a77980.dtsi:1344.21-1359.5: Warning 
> (spi_bus_bridge):
>> /soc/rpc@ee200000: node name for SPI buses should be 'spi'
>>   also defined at 
> arch/arm64/boot/dts/renesas/r8a77980-condor.dts:283.6-343.3
>> arch/arm64/boot/dts/renesas/r8a77980-condor.dtb: Warning (spi_bus_reg):
>> Failed prerequisite 'spi_bus_bridge'
>>
>>
>>>   - HF mode:
>>>           rpc: rpc-if@ee200000 {
>>
>>    Again, spi@<...>.
> 
> what about rpc_if: hf@<...>

   Can't change the node name, as it's declared in the .dtsi files, not *.dts
ones. And "spi" works for the HF case as well -- no complaints from dtc. :-)

>>>                   compatible = "renesas,r8a77995-rpc", 
> "renesas,rcar-gen3-rpc";
>>>                   reg = <0 0xee200000 0 0x200>, <0 0x08000000 0 
> 0x4000000>,
>>>                         <0 0xee208000 0 0x100>;
>>>                   reg-names = "regs", "dirmap", "wbuf";
>>>                   clocks = <&cpg CPG_MOD 917>;
>>>                   clock-names = "rpc";
>>>                   power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
>>>                   resets = <&cpg 917>;
>>>                   #address-cells = <1>;
>>>                   #size-cells = <1>;
>>>
>>>                   flash@0 {
>>>                           compatible = "cfi-flash";
>>
>>    The working HF implementation has "cypress,hyperflash" before 
> "cfi-flash".
>>
>>>                           reg = <0 0x4000000>;
>>>                   };
>>>           };
>>>
>>> --------------------------------------------------------------<
>>>
>>> Is it OK ?
>>
>>    Yeah, seems good (assuming you fix the issues above).
> 
> Patch new DTS to
> ===============================================================> 
> 
> +Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
> +---------------------------------------------------------
> +
> +RPC-IF supports both SPI NOR and HyperFlash (CFI-compliant flash)
> +
> +Required properties:
> +- compatible: should be an SoC-specific compatible value, followed by
> +                                "renesas,rcar-gen3-rpc" as a fallback.
> +                                supported SoC-specific values are:
> +                                "renesas,r8a77995-rpc"          (R-Car 
> D3)
> +- reg: should contain three register areas:
> +                first for RPC-IF registers,
> +                second for the direct mapping read mode and
> +                third for the write buffer area.
> +- reg-names: should contain "regs", "dirmap" and "wbuf"
> +- clocks: should contain 1 entries for the module's clock
> +- clock-names: should contain "rpc"
> +- #address-cells: should be 1
> +- #size-cells: should be 0

   Still nothing about the "oower-domains" and "resets" props... :-(

> +
> +Example:
> +- SPI mode:
> +
> +                rpc_if: spi@ee200000 {
> +                                compatible = "renesas,r8a77995-rpc", 
> "renesas,rcar-gen3-rpc";
> +                                reg = <0 0xee200000 0 0x200>, <0 
> 0x08000000 0 0x4000000>,
> +                                      <0 0xee208000 0 0x100>;
> +                                reg-names = "regs", "dirmap", "wbuf";
> +                                clocks = <&cpg CPG_MOD 917>;
> +                                clock-names = "rpc";
> +                                power-domains = <&sysc 
> R8A77995_PD_ALWAYS_ON>;
> +                                resets = <&cpg 917>;
> +                                #address-cells = <1>;
> +                                #size-cells = <0>;
[...]
> =======================================================================<
> 
> OK ?

   Yes, with the remaining issue fixed.
 
> thanks & best regards,
> Mason

[...]

MBR, Sergei