[2/2] dt-binding: spi: Document Renesas R-Car RPC controller bindings

Commit Message

Mason Yang Nov. 19, 2018, 10:01 a.m. UTC

Document the bindings used by the Renesas R-Car D3 RPC controller.

Signed-off-by: Mason Yang <[e-mail]>
---
 .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt

Comments

Marek Vasut Nov. 19, 2018, 1:49 p.m. UTC | #1

On 11/19/2018 11:01 AM, Mason Yang wrote:
> Document the bindings used by the Renesas R-Car D3 RPC controller.
> 
> Signed-off-by: Mason Yang <[e-mail]>
> ---
>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> new file mode 100644
> index 0000000..8286cc8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> @@ -0,0 +1,33 @@
> +Renesas R-Car D3 RPC controller Device Tree Bindings
> +----------------------------------------------------
> +
> +Required properties:
> +- compatible: should be "renesas,rpc-r8a77995"
> +- #address-cells: should be 1
> +- #size-cells: should be 0
> +- reg: should contain 2 entries, one for the registers and one for the direct
> +       mapping area
> +- reg-names: should contain "rpc_regs" and "dirmap"
> +- interrupts: interrupt line connected to the RPC SPI controller

Do you also plan to support the RPC HF mode ? And if so, how would that
look in the bindings ? I'd like to avoid having to redesign the bindings
later to handle both the SPI and HF modes.

The HF is roughly a CFI flash with different bus interface.

btw U-Boot has drivers for both the HF and SPI mode:
http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/mtd/renesas_rpc_hf.c
http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/spi/renesas_rpc_spi.c

> +- clock-names: should contain "clk_rpc"
> +- clocks: should contain 1 entries for the CPG Module 917 clocks
> +
> +Example:
> +
> +	rpc: spi@ee200000 {
> +		compatible = "renesas,rpc-r8a77995";
> +		reg = <0 0xee200000 0 0x8100>, <0 0x08000000 0 0x4000000>;
> +		reg-names = "rpc_regs", "dirmap";
> +		clocks = <&cpg CPG_MOD 917>;
> +		clock-names = "clk_rpc";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		flash@0 {
> +			compatible = "jedec,spi-nor";
> +			reg = <0>;
> +			spi-max-frequency = <40000000>;
> +			spi-tx-bus-width = <1>;
> +			spi-rx-bus-width = <1>;
> +		};
> +	};
>

Boris Brezillon Nov. 19, 2018, 2:10 p.m. UTC | #2

On Mon, 19 Nov 2018 14:49:31 +0100
Marek Vasut <[e-mail]> wrote:

> On 11/19/2018 11:01 AM, Mason Yang wrote:
> > Document the bindings used by the Renesas R-Car D3 RPC controller.
> > 
> > Signed-off-by: Mason Yang <[e-mail]>
> > ---
> >  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> > new file mode 100644
> > index 0000000..8286cc8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> > @@ -0,0 +1,33 @@
> > +Renesas R-Car D3 RPC controller Device Tree Bindings
> > +----------------------------------------------------
> > +
> > +Required properties:
> > +- compatible: should be "renesas,rpc-r8a77995"
> > +- #address-cells: should be 1
> > +- #size-cells: should be 0
> > +- reg: should contain 2 entries, one for the registers and one for the direct
> > +       mapping area
> > +- reg-names: should contain "rpc_regs" and "dirmap"
> > +- interrupts: interrupt line connected to the RPC SPI controller  
> 
> Do you also plan to support the RPC HF mode ? And if so, how would that
> look in the bindings ?

Not sure this approach is still accepted, but that's how we solved the
problem for the flexcom block [1].

[1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt

Marek Vasut Nov. 19, 2018, 2:14 p.m. UTC | #3

On 11/19/2018 03:10 PM, Boris Brezillon wrote:
> On Mon, 19 Nov 2018 14:49:31 +0100
> Marek Vasut <[e-mail]> wrote:
> 
>> On 11/19/2018 11:01 AM, Mason Yang wrote:
>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
>>>
>>> Signed-off-by: Mason Yang <[e-mail]>
>>> ---
>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
>>>  1 file changed, 33 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>> new file mode 100644
>>> index 0000000..8286cc8
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>> @@ -0,0 +1,33 @@
>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
>>> +----------------------------------------------------
>>> +
>>> +Required properties:
>>> +- compatible: should be "renesas,rpc-r8a77995"
>>> +- #address-cells: should be 1
>>> +- #size-cells: should be 0
>>> +- reg: should contain 2 entries, one for the registers and one for the direct
>>> +       mapping area
>>> +- reg-names: should contain "rpc_regs" and "dirmap"
>>> +- interrupts: interrupt line connected to the RPC SPI controller  
>>
>> Do you also plan to support the RPC HF mode ? And if so, how would that
>> look in the bindings ?
> 
> Not sure this approach is still accepted, but that's how we solved the
> problem for the flexcom block [1].
> 
> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt

That looks pretty horrible.

In U-Boot we check whether the device hanging under the controller node
is JEDEC SPI flash or CFI flash and based on that decide what the config
of the controller should be (SPI or HF). Not sure that's much better,but
at least it doesn't need extra nodes which do not really represent any
kind of real hardware.

Boris Brezillon Nov. 19, 2018, 2:43 p.m. UTC | #4

On Mon, 19 Nov 2018 15:14:07 +0100
Marek Vasut <[e-mail]> wrote:

> On 11/19/2018 03:10 PM, Boris Brezillon wrote:
> > On Mon, 19 Nov 2018 14:49:31 +0100
> > Marek Vasut <[e-mail]> wrote:
> >   
> >> On 11/19/2018 11:01 AM, Mason Yang wrote:  
> >>> Document the bindings used by the Renesas R-Car D3 RPC controller.
> >>>
> >>> Signed-off-by: Mason Yang <[e-mail]>
> >>> ---
> >>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
> >>>  1 file changed, 33 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>> new file mode 100644
> >>> index 0000000..8286cc8
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>> @@ -0,0 +1,33 @@
> >>> +Renesas R-Car D3 RPC controller Device Tree Bindings
> >>> +----------------------------------------------------
> >>> +
> >>> +Required properties:
> >>> +- compatible: should be "renesas,rpc-r8a77995"
> >>> +- #address-cells: should be 1
> >>> +- #size-cells: should be 0
> >>> +- reg: should contain 2 entries, one for the registers and one for the direct
> >>> +       mapping area
> >>> +- reg-names: should contain "rpc_regs" and "dirmap"
> >>> +- interrupts: interrupt line connected to the RPC SPI controller    
> >>
> >> Do you also plan to support the RPC HF mode ? And if so, how would that
> >> look in the bindings ?  
> > 
> > Not sure this approach is still accepted, but that's how we solved the
> > problem for the flexcom block [1].
> > 
> > [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt  
> 
> That looks pretty horrible.
> 
> In U-Boot we check whether the device hanging under the controller node
> is JEDEC SPI flash or CFI flash and based on that decide what the config
> of the controller should be (SPI or HF). Not sure that's much better,but
> at least it doesn't need extra nodes which do not really represent any
> kind of real hardware.
> 

The subnodes are not needed, you can just have a property that tells in
which mode the controller is supposed to operate, and the MFD would
create a sub-device that points to the same device_node. Or we can have
a single driver that decides what to declare (a spi_controller or flash
controller), but you'd still have to decide where to place this
driver...

Marek Vasut Nov. 19, 2018, 3:12 p.m. UTC | #5

On 11/19/2018 03:43 PM, Boris Brezillon wrote:
> On Mon, 19 Nov 2018 15:14:07 +0100
> Marek Vasut <[e-mail]> wrote:
> 
>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:
>>> On Mon, 19 Nov 2018 14:49:31 +0100
>>> Marek Vasut <[e-mail]> wrote:
>>>   
>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:  
>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
>>>>>
>>>>> Signed-off-by: Mason Yang <[e-mail]>
>>>>> ---
>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
>>>>>  1 file changed, 33 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>> new file mode 100644
>>>>> index 0000000..8286cc8
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>> @@ -0,0 +1,33 @@
>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
>>>>> +----------------------------------------------------
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible: should be "renesas,rpc-r8a77995"
>>>>> +- #address-cells: should be 1
>>>>> +- #size-cells: should be 0
>>>>> +- reg: should contain 2 entries, one for the registers and one for the direct
>>>>> +       mapping area
>>>>> +- reg-names: should contain "rpc_regs" and "dirmap"
>>>>> +- interrupts: interrupt line connected to the RPC SPI controller    
>>>>
>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
>>>> look in the bindings ?  
>>>
>>> Not sure this approach is still accepted, but that's how we solved the
>>> problem for the flexcom block [1].
>>>
>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt  
>>
>> That looks pretty horrible.
>>
>> In U-Boot we check whether the device hanging under the controller node
>> is JEDEC SPI flash or CFI flash and based on that decide what the config
>> of the controller should be (SPI or HF). Not sure that's much better,but
>> at least it doesn't need extra nodes which do not really represent any
>> kind of real hardware.
>>
> 
> The subnodes are not needed, you can just have a property that tells in
> which mode the controller is supposed to operate, and the MFD would
> create a sub-device that points to the same device_node.

Do you even need a dedicated property ? I think you can decide purely on
what node is hanging under the controller (jedec spi nor or cfi nor).

> Or we can have
> a single driver that decides what to declare (a spi_controller or flash
> controller), but you'd still have to decide where to place this
> driver...

I'd definitely prefer a single driver.

Boris Brezillon Nov. 19, 2018, 3:21 p.m. UTC | #6

On Mon, 19 Nov 2018 16:12:41 +0100
Marek Vasut <[e-mail]> wrote:

> On 11/19/2018 03:43 PM, Boris Brezillon wrote:
> > On Mon, 19 Nov 2018 15:14:07 +0100
> > Marek Vasut <[e-mail]> wrote:
> >   
> >> On 11/19/2018 03:10 PM, Boris Brezillon wrote:  
> >>> On Mon, 19 Nov 2018 14:49:31 +0100
> >>> Marek Vasut <[e-mail]> wrote:
> >>>     
> >>>> On 11/19/2018 11:01 AM, Mason Yang wrote:    
> >>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
> >>>>>
> >>>>> Signed-off-by: Mason Yang <[e-mail]>
> >>>>> ---
> >>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
> >>>>>  1 file changed, 33 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>> new file mode 100644
> >>>>> index 0000000..8286cc8
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>> @@ -0,0 +1,33 @@
> >>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
> >>>>> +----------------------------------------------------
> >>>>> +
> >>>>> +Required properties:
> >>>>> +- compatible: should be "renesas,rpc-r8a77995"
> >>>>> +- #address-cells: should be 1
> >>>>> +- #size-cells: should be 0
> >>>>> +- reg: should contain 2 entries, one for the registers and one for the direct
> >>>>> +       mapping area
> >>>>> +- reg-names: should contain "rpc_regs" and "dirmap"
> >>>>> +- interrupts: interrupt line connected to the RPC SPI controller      
> >>>>
> >>>> Do you also plan to support the RPC HF mode ? And if so, how would that
> >>>> look in the bindings ?    
> >>>
> >>> Not sure this approach is still accepted, but that's how we solved the
> >>> problem for the flexcom block [1].
> >>>
> >>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt    
> >>
> >> That looks pretty horrible.
> >>
> >> In U-Boot we check whether the device hanging under the controller node
> >> is JEDEC SPI flash or CFI flash and based on that decide what the config
> >> of the controller should be (SPI or HF). Not sure that's much better,but
> >> at least it doesn't need extra nodes which do not really represent any
> >> kind of real hardware.
> >>  
> > 
> > The subnodes are not needed, you can just have a property that tells in
> > which mode the controller is supposed to operate, and the MFD would
> > create a sub-device that points to the same device_node.  
> 
> Do you even need a dedicated property ? I think you can decide purely on
> what node is hanging under the controller (jedec spi nor or cfi nor).

Yes, that could work if they have well-known compatibles. As soon as
people start using flash-specific compats (like some people do for
their SPI NORs) it becomes a maintenance burden.

> 
> > Or we can have
> > a single driver that decides what to declare (a spi_controller or flash
> > controller), but you'd still have to decide where to place this
> > driver...  
> 
> I'd definitely prefer a single driver.
> 

Where would you put this driver? I really don't like the idea of having
MTD drivers spread over the tree. Don't know what's Mark's opinion on
this matter.

Marek Vasut Nov. 19, 2018, 10:11 p.m. UTC | #7

On 11/19/2018 04:21 PM, Boris Brezillon wrote:
> On Mon, 19 Nov 2018 16:12:41 +0100
> Marek Vasut <[e-mail]> wrote:
> 
>> On 11/19/2018 03:43 PM, Boris Brezillon wrote:
>>> On Mon, 19 Nov 2018 15:14:07 +0100
>>> Marek Vasut <[e-mail]> wrote:
>>>   
>>>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:  
>>>>> On Mon, 19 Nov 2018 14:49:31 +0100
>>>>> Marek Vasut <[e-mail]> wrote:
>>>>>     
>>>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:    
>>>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
>>>>>>>
>>>>>>> Signed-off-by: Mason Yang <[e-mail]>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
>>>>>>>  1 file changed, 33 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..8286cc8
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>>>> @@ -0,0 +1,33 @@
>>>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
>>>>>>> +----------------------------------------------------
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible: should be "renesas,rpc-r8a77995"
>>>>>>> +- #address-cells: should be 1
>>>>>>> +- #size-cells: should be 0
>>>>>>> +- reg: should contain 2 entries, one for the registers and one for the direct
>>>>>>> +       mapping area
>>>>>>> +- reg-names: should contain "rpc_regs" and "dirmap"
>>>>>>> +- interrupts: interrupt line connected to the RPC SPI controller      
>>>>>>
>>>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
>>>>>> look in the bindings ?    
>>>>>
>>>>> Not sure this approach is still accepted, but that's how we solved the
>>>>> problem for the flexcom block [1].
>>>>>
>>>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt    
>>>>
>>>> That looks pretty horrible.
>>>>
>>>> In U-Boot we check whether the device hanging under the controller node
>>>> is JEDEC SPI flash or CFI flash and based on that decide what the config
>>>> of the controller should be (SPI or HF). Not sure that's much better,but
>>>> at least it doesn't need extra nodes which do not really represent any
>>>> kind of real hardware.
>>>>  
>>>
>>> The subnodes are not needed, you can just have a property that tells in
>>> which mode the controller is supposed to operate, and the MFD would
>>> create a sub-device that points to the same device_node.  
>>
>> Do you even need a dedicated property ? I think you can decide purely on
>> what node is hanging under the controller (jedec spi nor or cfi nor).
> 
> Yes, that could work if they have well-known compatibles. As soon as
> people start using flash-specific compats (like some people do for
> their SPI NORs) it becomes a maintenance burden.

Which, on this controller, is very likely never gonna happen. Once it
does , we can add a custom property.

>>> Or we can have
>>> a single driver that decides what to declare (a spi_controller or flash
>>> controller), but you'd still have to decide where to place this
>>> driver...  
>>
>> I'd definitely prefer a single driver.
>>
> 
> Where would you put this driver? I really don't like the idea of having
> MTD drivers spread over the tree. Don't know what's Mark's opinion on
> this matter.

Well, it's both CFI (hyperflash) and SF (well, SPI flash) controller, so
where would this go ?

Boris Brezillon Nov. 19, 2018, 10:19 p.m. UTC | #8

On Mon, 19 Nov 2018 23:11:31 +0100
Marek Vasut <[e-mail]> wrote:

> On 11/19/2018 04:21 PM, Boris Brezillon wrote:
> > On Mon, 19 Nov 2018 16:12:41 +0100
> > Marek Vasut <[e-mail]> wrote:
> >   
> >> On 11/19/2018 03:43 PM, Boris Brezillon wrote:  
> >>> On Mon, 19 Nov 2018 15:14:07 +0100
> >>> Marek Vasut <[e-mail]> wrote:
> >>>     
> >>>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:    
> >>>>> On Mon, 19 Nov 2018 14:49:31 +0100
> >>>>> Marek Vasut <[e-mail]> wrote:
> >>>>>       
> >>>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:      
> >>>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
> >>>>>>>
> >>>>>>> Signed-off-by: Mason Yang <[e-mail]>
> >>>>>>> ---
> >>>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
> >>>>>>>  1 file changed, 33 insertions(+)
> >>>>>>>  create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>>>> new file mode 100644
> >>>>>>> index 0000000..8286cc8
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>>>> @@ -0,0 +1,33 @@
> >>>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
> >>>>>>> +----------------------------------------------------
> >>>>>>> +
> >>>>>>> +Required properties:
> >>>>>>> +- compatible: should be "renesas,rpc-r8a77995"
> >>>>>>> +- #address-cells: should be 1
> >>>>>>> +- #size-cells: should be 0
> >>>>>>> +- reg: should contain 2 entries, one for the registers and one for the direct
> >>>>>>> +       mapping area
> >>>>>>> +- reg-names: should contain "rpc_regs" and "dirmap"
> >>>>>>> +- interrupts: interrupt line connected to the RPC SPI controller        
> >>>>>>
> >>>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
> >>>>>> look in the bindings ?      
> >>>>>
> >>>>> Not sure this approach is still accepted, but that's how we solved the
> >>>>> problem for the flexcom block [1].
> >>>>>
> >>>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt      
> >>>>
> >>>> That looks pretty horrible.
> >>>>
> >>>> In U-Boot we check whether the device hanging under the controller node
> >>>> is JEDEC SPI flash or CFI flash and based on that decide what the config
> >>>> of the controller should be (SPI or HF). Not sure that's much better,but
> >>>> at least it doesn't need extra nodes which do not really represent any
> >>>> kind of real hardware.
> >>>>    
> >>>
> >>> The subnodes are not needed, you can just have a property that tells in
> >>> which mode the controller is supposed to operate, and the MFD would
> >>> create a sub-device that points to the same device_node.    
> >>
> >> Do you even need a dedicated property ? I think you can decide purely on
> >> what node is hanging under the controller (jedec spi nor or cfi nor).  
> > 
> > Yes, that could work if they have well-known compatibles. As soon as
> > people start using flash-specific compats (like some people do for
> > their SPI NORs) it becomes a maintenance burden.  
> 
> Which, on this controller, is very likely never gonna happen. Once it
> does , we can add a custom property.
> 
> >>> Or we can have
> >>> a single driver that decides what to declare (a spi_controller or flash
> >>> controller), but you'd still have to decide where to place this
> >>> driver...    
> >>
> >> I'd definitely prefer a single driver.
> >>  
> > 
> > Where would you put this driver? I really don't like the idea of having
> > MTD drivers spread over the tree. Don't know what's Mark's opinion on
> > this matter.  
> 
> Well, it's both CFI (hyperflash) and SF (well, SPI flash) controller, so
> where would this go ?
> 

The spi-mem layer is in drivers/spi/ so it could go in drivers/spi/
(spi-mem controller) or drivers/mtd/ (CFI controller). Looks like you
didn't closely follow what has happened in the spi-nor subsystem
lately :P.

Marek Vasut Nov. 19, 2018, 10:22 p.m. UTC | #9

On 11/19/2018 11:19 PM, Boris Brezillon wrote:
> On Mon, 19 Nov 2018 23:11:31 +0100
> Marek Vasut <[e-mail]> wrote:
> 
>> On 11/19/2018 04:21 PM, Boris Brezillon wrote:
>>> On Mon, 19 Nov 2018 16:12:41 +0100
>>> Marek Vasut <[e-mail]> wrote:
>>>   
>>>> On 11/19/2018 03:43 PM, Boris Brezillon wrote:  
>>>>> On Mon, 19 Nov 2018 15:14:07 +0100
>>>>> Marek Vasut <[e-mail]> wrote:
>>>>>     
>>>>>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:    
>>>>>>> On Mon, 19 Nov 2018 14:49:31 +0100
>>>>>>> Marek Vasut <[e-mail]> wrote:
>>>>>>>       
>>>>>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:      
>>>>>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Mason Yang <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
>>>>>>>>>  1 file changed, 33 insertions(+)
>>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000..8286cc8
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>>>>>> @@ -0,0 +1,33 @@
>>>>>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
>>>>>>>>> +----------------------------------------------------
>>>>>>>>> +
>>>>>>>>> +Required properties:
>>>>>>>>> +- compatible: should be "renesas,rpc-r8a77995"
>>>>>>>>> +- #address-cells: should be 1
>>>>>>>>> +- #size-cells: should be 0
>>>>>>>>> +- reg: should contain 2 entries, one for the registers and one for the direct
>>>>>>>>> +       mapping area
>>>>>>>>> +- reg-names: should contain "rpc_regs" and "dirmap"
>>>>>>>>> +- interrupts: interrupt line connected to the RPC SPI controller        
>>>>>>>>
>>>>>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
>>>>>>>> look in the bindings ?      
>>>>>>>
>>>>>>> Not sure this approach is still accepted, but that's how we solved the
>>>>>>> problem for the flexcom block [1].
>>>>>>>
>>>>>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt      
>>>>>>
>>>>>> That looks pretty horrible.
>>>>>>
>>>>>> In U-Boot we check whether the device hanging under the controller node
>>>>>> is JEDEC SPI flash or CFI flash and based on that decide what the config
>>>>>> of the controller should be (SPI or HF). Not sure that's much better,but
>>>>>> at least it doesn't need extra nodes which do not really represent any
>>>>>> kind of real hardware.
>>>>>>    
>>>>>
>>>>> The subnodes are not needed, you can just have a property that tells in
>>>>> which mode the controller is supposed to operate, and the MFD would
>>>>> create a sub-device that points to the same device_node.    
>>>>
>>>> Do you even need a dedicated property ? I think you can decide purely on
>>>> what node is hanging under the controller (jedec spi nor or cfi nor).  
>>>
>>> Yes, that could work if they have well-known compatibles. As soon as
>>> people start using flash-specific compats (like some people do for
>>> their SPI NORs) it becomes a maintenance burden.  
>>
>> Which, on this controller, is very likely never gonna happen. Once it
>> does , we can add a custom property.
>>
>>>>> Or we can have
>>>>> a single driver that decides what to declare (a spi_controller or flash
>>>>> controller), but you'd still have to decide where to place this
>>>>> driver...    
>>>>
>>>> I'd definitely prefer a single driver.
>>>>  
>>>
>>> Where would you put this driver? I really don't like the idea of having
>>> MTD drivers spread over the tree. Don't know what's Mark's opinion on
>>> this matter.  
>>
>> Well, it's both CFI (hyperflash) and SF (well, SPI flash) controller, so
>> where would this go ?
>>
> 
> The spi-mem layer is in drivers/spi/ so it could go in drivers/spi/
> (spi-mem controller) or drivers/mtd/ (CFI controller).

drivers/mtd is probably a better option, since it's not a generic SPI
controller.

Boris Brezillon Nov. 19, 2018, 10:25 p.m. UTC | #10

On Mon, 19 Nov 2018 23:22:45 +0100
Marek Vasut <[e-mail]> wrote:

> On 11/19/2018 11:19 PM, Boris Brezillon wrote:
> > On Mon, 19 Nov 2018 23:11:31 +0100
> > Marek Vasut <[e-mail]> wrote:
> >   
> >> On 11/19/2018 04:21 PM, Boris Brezillon wrote:  
> >>> On Mon, 19 Nov 2018 16:12:41 +0100
> >>> Marek Vasut <[e-mail]> wrote:
> >>>     
> >>>> On 11/19/2018 03:43 PM, Boris Brezillon wrote:    
> >>>>> On Mon, 19 Nov 2018 15:14:07 +0100
> >>>>> Marek Vasut <[e-mail]> wrote:
> >>>>>       
> >>>>>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:      
> >>>>>>> On Mon, 19 Nov 2018 14:49:31 +0100
> >>>>>>> Marek Vasut <[e-mail]> wrote:
> >>>>>>>         
> >>>>>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:        
> >>>>>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Mason Yang <[e-mail]>
> >>>>>>>>> ---
> >>>>>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
> >>>>>>>>>  1 file changed, 33 insertions(+)
> >>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>>>>>>
> >>>>>>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>>>>>> new file mode 100644
> >>>>>>>>> index 0000000..8286cc8
> >>>>>>>>> --- /dev/null
> >>>>>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>>>>>> @@ -0,0 +1,33 @@
> >>>>>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
> >>>>>>>>> +----------------------------------------------------
> >>>>>>>>> +
> >>>>>>>>> +Required properties:
> >>>>>>>>> +- compatible: should be "renesas,rpc-r8a77995"
> >>>>>>>>> +- #address-cells: should be 1
> >>>>>>>>> +- #size-cells: should be 0
> >>>>>>>>> +- reg: should contain 2 entries, one for the registers and one for the direct
> >>>>>>>>> +       mapping area
> >>>>>>>>> +- reg-names: should contain "rpc_regs" and "dirmap"
> >>>>>>>>> +- interrupts: interrupt line connected to the RPC SPI controller          
> >>>>>>>>
> >>>>>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
> >>>>>>>> look in the bindings ?        
> >>>>>>>
> >>>>>>> Not sure this approach is still accepted, but that's how we solved the
> >>>>>>> problem for the flexcom block [1].
> >>>>>>>
> >>>>>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt        
> >>>>>>
> >>>>>> That looks pretty horrible.
> >>>>>>
> >>>>>> In U-Boot we check whether the device hanging under the controller node
> >>>>>> is JEDEC SPI flash or CFI flash and based on that decide what the config
> >>>>>> of the controller should be (SPI or HF). Not sure that's much better,but
> >>>>>> at least it doesn't need extra nodes which do not really represent any
> >>>>>> kind of real hardware.
> >>>>>>      
> >>>>>
> >>>>> The subnodes are not needed, you can just have a property that tells in
> >>>>> which mode the controller is supposed to operate, and the MFD would
> >>>>> create a sub-device that points to the same device_node.      
> >>>>
> >>>> Do you even need a dedicated property ? I think you can decide purely on
> >>>> what node is hanging under the controller (jedec spi nor or cfi nor).    
> >>>
> >>> Yes, that could work if they have well-known compatibles. As soon as
> >>> people start using flash-specific compats (like some people do for
> >>> their SPI NORs) it becomes a maintenance burden.    
> >>
> >> Which, on this controller, is very likely never gonna happen. Once it
> >> does , we can add a custom property.
> >>  
> >>>>> Or we can have
> >>>>> a single driver that decides what to declare (a spi_controller or flash
> >>>>> controller), but you'd still have to decide where to place this
> >>>>> driver...      
> >>>>
> >>>> I'd definitely prefer a single driver.
> >>>>    
> >>>
> >>> Where would you put this driver? I really don't like the idea of having
> >>> MTD drivers spread over the tree. Don't know what's Mark's opinion on
> >>> this matter.    
> >>
> >> Well, it's both CFI (hyperflash) and SF (well, SPI flash) controller, so
> >> where would this go ?
> >>  
> > 
> > The spi-mem layer is in drivers/spi/ so it could go in drivers/spi/
> > (spi-mem controller) or drivers/mtd/ (CFI controller).  
> 
> drivers/mtd is probably a better option, since it's not a generic SPI
> controller.
> 

No, spi-mem controller drivers should go in drivers/spi/ even if they
don't implement the generic SPI interface (it's allowed to only
implement the spi_mem interface).

>

Marek Vasut Nov. 19, 2018, 10:29 p.m. UTC | #11

On 11/19/2018 11:25 PM, Boris Brezillon wrote:
> On Mon, 19 Nov 2018 23:22:45 +0100
> Marek Vasut <[e-mail]> wrote:
> 
>> On 11/19/2018 11:19 PM, Boris Brezillon wrote:
>>> On Mon, 19 Nov 2018 23:11:31 +0100
>>> Marek Vasut <[e-mail]> wrote:
>>>   
>>>> On 11/19/2018 04:21 PM, Boris Brezillon wrote:  
>>>>> On Mon, 19 Nov 2018 16:12:41 +0100
>>>>> Marek Vasut <[e-mail]> wrote:
>>>>>     
>>>>>> On 11/19/2018 03:43 PM, Boris Brezillon wrote:    
>>>>>>> On Mon, 19 Nov 2018 15:14:07 +0100
>>>>>>> Marek Vasut <[e-mail]> wrote:
>>>>>>>       
>>>>>>>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:      
>>>>>>>>> On Mon, 19 Nov 2018 14:49:31 +0100
>>>>>>>>> Marek Vasut <[e-mail]> wrote:
>>>>>>>>>         
>>>>>>>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:        
>>>>>>>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Mason Yang <[e-mail]>
>>>>>>>>>>> ---
>>>>>>>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
>>>>>>>>>>>  1 file changed, 33 insertions(+)
>>>>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>>>>>>>> new file mode 100644
>>>>>>>>>>> index 0000000..8286cc8
>>>>>>>>>>> --- /dev/null
>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>>>>>>>>>>> @@ -0,0 +1,33 @@
>>>>>>>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
>>>>>>>>>>> +----------------------------------------------------
>>>>>>>>>>> +
>>>>>>>>>>> +Required properties:
>>>>>>>>>>> +- compatible: should be "renesas,rpc-r8a77995"
>>>>>>>>>>> +- #address-cells: should be 1
>>>>>>>>>>> +- #size-cells: should be 0
>>>>>>>>>>> +- reg: should contain 2 entries, one for the registers and one for the direct
>>>>>>>>>>> +       mapping area
>>>>>>>>>>> +- reg-names: should contain "rpc_regs" and "dirmap"
>>>>>>>>>>> +- interrupts: interrupt line connected to the RPC SPI controller          
>>>>>>>>>>
>>>>>>>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
>>>>>>>>>> look in the bindings ?        
>>>>>>>>>
>>>>>>>>> Not sure this approach is still accepted, but that's how we solved the
>>>>>>>>> problem for the flexcom block [1].
>>>>>>>>>
>>>>>>>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt        
>>>>>>>>
>>>>>>>> That looks pretty horrible.
>>>>>>>>
>>>>>>>> In U-Boot we check whether the device hanging under the controller node
>>>>>>>> is JEDEC SPI flash or CFI flash and based on that decide what the config
>>>>>>>> of the controller should be (SPI or HF). Not sure that's much better,but
>>>>>>>> at least it doesn't need extra nodes which do not really represent any
>>>>>>>> kind of real hardware.
>>>>>>>>      
>>>>>>>
>>>>>>> The subnodes are not needed, you can just have a property that tells in
>>>>>>> which mode the controller is supposed to operate, and the MFD would
>>>>>>> create a sub-device that points to the same device_node.      
>>>>>>
>>>>>> Do you even need a dedicated property ? I think you can decide purely on
>>>>>> what node is hanging under the controller (jedec spi nor or cfi nor).    
>>>>>
>>>>> Yes, that could work if they have well-known compatibles. As soon as
>>>>> people start using flash-specific compats (like some people do for
>>>>> their SPI NORs) it becomes a maintenance burden.    
>>>>
>>>> Which, on this controller, is very likely never gonna happen. Once it
>>>> does , we can add a custom property.
>>>>  
>>>>>>> Or we can have
>>>>>>> a single driver that decides what to declare (a spi_controller or flash
>>>>>>> controller), but you'd still have to decide where to place this
>>>>>>> driver...      
>>>>>>
>>>>>> I'd definitely prefer a single driver.
>>>>>>    
>>>>>
>>>>> Where would you put this driver? I really don't like the idea of having
>>>>> MTD drivers spread over the tree. Don't know what's Mark's opinion on
>>>>> this matter.    
>>>>
>>>> Well, it's both CFI (hyperflash) and SF (well, SPI flash) controller, so
>>>> where would this go ?
>>>>  
>>>
>>> The spi-mem layer is in drivers/spi/ so it could go in drivers/spi/
>>> (spi-mem controller) or drivers/mtd/ (CFI controller).  
>>
>> drivers/mtd is probably a better option, since it's not a generic SPI
>> controller.
>>
> 
> No, spi-mem controller drivers should go in drivers/spi/ even if they
> don't implement the generic SPI interface (it's allowed to only
> implement the spi_mem interface).

Except this is not only SPI MEM controller, this is also hyperflash
(that is, CFI) controller. It can drive both types of chips. Thus , I
think it fits better in drivers/mtd/ .

Boris Brezillon Nov. 19, 2018, 10:31 p.m. UTC | #12

On Mon, 19 Nov 2018 23:29:00 +0100
Marek Vasut <[e-mail]> wrote:

> On 11/19/2018 11:25 PM, Boris Brezillon wrote:
> > On Mon, 19 Nov 2018 23:22:45 +0100
> > Marek Vasut <[e-mail]> wrote:
> >   
> >> On 11/19/2018 11:19 PM, Boris Brezillon wrote:  
> >>> On Mon, 19 Nov 2018 23:11:31 +0100
> >>> Marek Vasut <[e-mail]> wrote:
> >>>     
> >>>> On 11/19/2018 04:21 PM, Boris Brezillon wrote:    
> >>>>> On Mon, 19 Nov 2018 16:12:41 +0100
> >>>>> Marek Vasut <[e-mail]> wrote:
> >>>>>       
> >>>>>> On 11/19/2018 03:43 PM, Boris Brezillon wrote:      
> >>>>>>> On Mon, 19 Nov 2018 15:14:07 +0100
> >>>>>>> Marek Vasut <[e-mail]> wrote:
> >>>>>>>         
> >>>>>>>> On 11/19/2018 03:10 PM, Boris Brezillon wrote:        
> >>>>>>>>> On Mon, 19 Nov 2018 14:49:31 +0100
> >>>>>>>>> Marek Vasut <[e-mail]> wrote:
> >>>>>>>>>           
> >>>>>>>>>> On 11/19/2018 11:01 AM, Mason Yang wrote:          
> >>>>>>>>>>> Document the bindings used by the Renesas R-Car D3 RPC controller.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Mason Yang <[e-mail]>
> >>>>>>>>>>> ---
> >>>>>>>>>>>  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 ++++++++++++++++++++++
> >>>>>>>>>>>  1 file changed, 33 insertions(+)
> >>>>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>>>>>>>> new file mode 100644
> >>>>>>>>>>> index 0000000..8286cc8
> >>>>>>>>>>> --- /dev/null
> >>>>>>>>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> >>>>>>>>>>> @@ -0,0 +1,33 @@
> >>>>>>>>>>> +Renesas R-Car D3 RPC controller Device Tree Bindings
> >>>>>>>>>>> +----------------------------------------------------
> >>>>>>>>>>> +
> >>>>>>>>>>> +Required properties:
> >>>>>>>>>>> +- compatible: should be "renesas,rpc-r8a77995"
> >>>>>>>>>>> +- #address-cells: should be 1
> >>>>>>>>>>> +- #size-cells: should be 0
> >>>>>>>>>>> +- reg: should contain 2 entries, one for the registers and one for the direct
> >>>>>>>>>>> +       mapping area
> >>>>>>>>>>> +- reg-names: should contain "rpc_regs" and "dirmap"
> >>>>>>>>>>> +- interrupts: interrupt line connected to the RPC SPI controller            
> >>>>>>>>>>
> >>>>>>>>>> Do you also plan to support the RPC HF mode ? And if so, how would that
> >>>>>>>>>> look in the bindings ?          
> >>>>>>>>>
> >>>>>>>>> Not sure this approach is still accepted, but that's how we solved the
> >>>>>>>>> problem for the flexcom block [1].
> >>>>>>>>>
> >>>>>>>>> [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt          
> >>>>>>>>
> >>>>>>>> That looks pretty horrible.
> >>>>>>>>
> >>>>>>>> In U-Boot we check whether the device hanging under the controller node
> >>>>>>>> is JEDEC SPI flash or CFI flash and based on that decide what the config
> >>>>>>>> of the controller should be (SPI or HF). Not sure that's much better,but
> >>>>>>>> at least it doesn't need extra nodes which do not really represent any
> >>>>>>>> kind of real hardware.
> >>>>>>>>        
> >>>>>>>
> >>>>>>> The subnodes are not needed, you can just have a property that tells in
> >>>>>>> which mode the controller is supposed to operate, and the MFD would
> >>>>>>> create a sub-device that points to the same device_node.        
> >>>>>>
> >>>>>> Do you even need a dedicated property ? I think you can decide purely on
> >>>>>> what node is hanging under the controller (jedec spi nor or cfi nor).      
> >>>>>
> >>>>> Yes, that could work if they have well-known compatibles. As soon as
> >>>>> people start using flash-specific compats (like some people do for
> >>>>> their SPI NORs) it becomes a maintenance burden.      
> >>>>
> >>>> Which, on this controller, is very likely never gonna happen. Once it
> >>>> does , we can add a custom property.
> >>>>    
> >>>>>>> Or we can have
> >>>>>>> a single driver that decides what to declare (a spi_controller or flash
> >>>>>>> controller), but you'd still have to decide where to place this
> >>>>>>> driver...        
> >>>>>>
> >>>>>> I'd definitely prefer a single driver.
> >>>>>>      
> >>>>>
> >>>>> Where would you put this driver? I really don't like the idea of having
> >>>>> MTD drivers spread over the tree. Don't know what's Mark's opinion on
> >>>>> this matter.      
> >>>>
> >>>> Well, it's both CFI (hyperflash) and SF (well, SPI flash) controller, so
> >>>> where would this go ?
> >>>>    
> >>>
> >>> The spi-mem layer is in drivers/spi/ so it could go in drivers/spi/
> >>> (spi-mem controller) or drivers/mtd/ (CFI controller).    
> >>
> >> drivers/mtd is probably a better option, since it's not a generic SPI
> >> controller.
> >>  
> > 
> > No, spi-mem controller drivers should go in drivers/spi/ even if they
> > don't implement the generic SPI interface (it's allowed to only
> > implement the spi_mem interface).  
> 
> Except this is not only SPI MEM controller, this is also hyperflash
> (that is, CFI) controller. It can drive both types of chips. Thus , I
> think it fits better in drivers/mtd/ .
> 

Okay, then I guess we need an ack from Mark on that.

Geert Uytterhoeven Nov. 20, 2018, 8:07 a.m. UTC | #13

Hi Mason,

On Mon, Nov 19, 2018 at 11:02 AM Mason Yang <[e-mail]> wrote:
> Document the bindings used by the Renesas R-Car D3 RPC controller.
>
> Signed-off-by: Mason Yang <[e-mail]>

Thanks for your patch

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> @@ -0,0 +1,33 @@
> +Renesas R-Car D3 RPC controller Device Tree Bindings

R-Car Gen3

> +----------------------------------------------------
> +
> +Required properties:
> +- compatible: should be "renesas,rpc-r8a77995"

Please use "renesas,r8a77995-rpc".

> +- #address-cells: should be 1
> +- #size-cells: should be 0
> +- reg: should contain 2 entries, one for the registers and one for the direct
> +       mapping area
> +- reg-names: should contain "rpc_regs" and "dirmap"
> +- interrupts: interrupt line connected to the RPC SPI controller
> +- clock-names: should contain "clk_rpc"
> +- clocks: should contain 1 entries for the CPG Module 917 clocks

... for the module's clock

> +
> +Example:
> +
> +       rpc: spi@ee200000 {
> +               compatible = "renesas,rpc-r8a77995";
> +               reg = <0 0xee200000 0 0x8100>, <0 0x08000000 0 0x4000000>;
> +               reg-names = "rpc_regs", "dirmap";
> +               clocks = <&cpg CPG_MOD 917>;
> +               clock-names = "clk_rpc";
> +               #address-cells = <1>;
> +               #size-cells = <0>;

power-domains?
resets?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Marek Vasut Nov. 20, 2018, 12:57 p.m. UTC | #14

On 11/20/2018 06:42 AM, [e-mail] wrote:
> Hi Marek,
> 
> Marek Vasut <[e-mail]> 已在 2018/11/19 下午 09:49:31 上寫入:
> 
>> Marek Vasut <[e-mail]>
>> 2018/11/19 下午 09:49
>>
>> To
>>
>> Mason Yang <[e-mail]>, [e-mail],
>> [e-mail], [e-mail], linux-
>> [e-mail], [e-mail], Simon Horman
>> <[e-mail]>,
>>
>> cc
>>
>> [e-mail], [e-mail], Geert
>> Uytterhoeven <[e-mail]>, [e-mail]
>>
>> Subject
>>
>> Re: [PATCH 2/2] dt-binding: spi: Document Renesas R-Car RPC
>> controller bindings
>>
>> On 11/19/2018 11:01 AM, Mason Yang wrote:
>> > Document the bindings used by the Renesas R-Car D3 RPC controller.
>> >
>> > Signed-off-by: Mason Yang <[e-mail]>
>> > ---
>> >  .../devicetree/bindings/spi/spi-renesas-rpc.txt    | 33 +++++++++
>> +++++++++++++
>> >  1 file changed, 33 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/spi/spi-
>> renesas-rpc.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-
>> rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>> > new file mode 100644
>> > index 0000000..8286cc8
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>> > @@ -0,0 +1,33 @@
>> > +Renesas R-Car D3 RPC controller Device Tree Bindings
>> > +----------------------------------------------------
>> > +
>> > +Required properties:
>> > +- compatible: should be "renesas,rpc-r8a77995"
>> > +- #address-cells: should be 1
>> > +- #size-cells: should be 0
>> > +- reg: should contain 2 entries, one for the registers and one
>> for the direct
>> > +       mapping area
>> > +- reg-names: should contain "rpc_regs" and "dirmap"
>> > +- interrupts: interrupt line connected to the RPC SPI controller
>>
>> Do you also plan to support the RPC HF mode ? And if so, how would that
>> look in the bindings ? I'd like to avoid having to redesign the bindings
>> later to handle both the SPI and HF modes.
>>
> 
> I patched this RPC SPI/Octa driver is for mx25uw51245g and you may also
> refer to Boris's patch. [1][2][3]

Does this mean the driver is specific to one particular kind of flash ?

> [1] https://patchwork.kernel.org/cover/10638055/
> [2] https://patchwork.kernel.org/patch/10638057/
> [3] https://patchwork.kernel.org/patch/10638085/

Marek Vasut Nov. 21, 2018, 1:51 a.m. UTC | #15

On 11/21/2018 01:53 AM, [e-mail] wrote:
> Hi Marek,

Hi,

>> Marek Vasut <[e-mail]>
>> 2018/11/20 下午 08:57
>>
>> >> > diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-
>> >> rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>> >> > new file mode 100644
>> >> > index 0000000..8286cc8
>> >> > --- /dev/null
>> >> > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>> >> > @@ -0,0 +1,33 @@
>> >> > +Renesas R-Car D3 RPC controller Device Tree Bindings
>> >> > +----------------------------------------------------
>> >> > +
>> >> > +Required properties:
>> >> > +- compatible: should be "renesas,rpc-r8a77995"
>> >> > +- #address-cells: should be 1
>> >> > +- #size-cells: should be 0
>> >> > +- reg: should contain 2 entries, one for the registers and one
>> >> for the direct
>> >> > +       mapping area
>> >> > +- reg-names: should contain "rpc_regs" and "dirmap"
>> >> > +- interrupts: interrupt line connected to the RPC SPI controller
>> >>
>> >> Do you also plan to support the RPC HF mode ? And if so, how would that
>> >> look in the bindings ? I'd like to avoid having to redesign the
> bindings
>> >> later to handle both the SPI and HF modes.
>> >>
>> >
>> > I patched this RPC SPI/Octa driver is for mx25uw51245g and you may also
>> > refer to Boris's patch. [1][2][3]
>>
>> Does this mean the driver is specific to one particular kind of flash ?
>>
> 
> No, this driver supports all SPI flash, spi, qspi and octa flash.
> 
> The target is Octa 8-8-8 DDR2 mode once spi-nor layer is merged.[1][2][3]

The HyperFlash must also be supported, cfr my previous comment that I'd
like to avoid redesigning the bindings again when the HF mode is added.

Patch

diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
new file mode 100644
index 0000000..8286cc8
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
@@ -0,0 +1,33 @@ 
+Renesas R-Car D3 RPC controller Device Tree Bindings
+----------------------------------------------------
+
+Required properties:
+- compatible: should be "renesas,rpc-r8a77995"
+- #address-cells: should be 1
+- #size-cells: should be 0
+- reg: should contain 2 entries, one for the registers and one for the direct
+       mapping area
+- reg-names: should contain "rpc_regs" and "dirmap"
+- interrupts: interrupt line connected to the RPC SPI controller
+- clock-names: should contain "clk_rpc"
+- clocks: should contain 1 entries for the CPG Module 917 clocks
+
+Example:
+
+	rpc: spi@ee200000 {
+		compatible = "renesas,rpc-r8a77995";
+		reg = <0 0xee200000 0 0x8100>, <0 0x08000000 0 0x4000000>;
+		reg-names = "rpc_regs", "dirmap";
+		clocks = <&cpg CPG_MOD 917>;
+		clock-names = "clk_rpc";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		flash@0 {
+			compatible = "jedec,spi-nor";
+			reg = <0>;
+			spi-max-frequency = <40000000>;
+			spi-tx-bus-width = <1>;
+			spi-rx-bus-width = <1>;
+		};
+	};