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

kernel test robot Nov. 20, 2018, 1:56 p.m. UTC | #15

Hi Mason,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on spi/for-next]
[also build test ERROR on v4.20-rc3 next-20181120]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Mason-Yang/spi-Add-Renesas-R-Car-RPC-SPI-controller-driver/20181120-020310
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

>> drivers/spi/spi-renesas-rpc.c:283:54: warning: incorrect type in argument 2 (different address spaces)
   drivers/spi/spi-renesas-rpc.c:283:54:    expected void const volatile [noderef] <asn:2>*addr
   drivers/spi/spi-renesas-rpc.c:283:54:    got void *<noident>
>> drivers/spi/spi-renesas-rpc.c:369:31: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
>> drivers/spi/spi-renesas-rpc.c:372:13: error: using member 'info' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:375:41: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:379:41: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:392:50: error: using member 'info' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:400:31: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:403:13: error: using member 'info' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:409:41: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:413:41: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:445:31: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:447:17: error: using member 'info' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:447:37: error: using member 'info' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:450:42: error: using member 'mem' in incomplete struct spi_mem_dirmap_desc
   drivers/spi/spi-renesas-rpc.c:454:17: error: using member 'info' in incomplete struct spi_mem_dirmap_desc
>> drivers/spi/spi-renesas-rpc.c:484:10: error: unknown field name in initializer
   drivers/spi/spi-renesas-rpc.c:485:10: error: unknown field name in initializer
   drivers/spi/spi-renesas-rpc.c:486:10: error: unknown field name in initializer
>> drivers/spi/spi-renesas-rpc.c:372:13: warning: unknown expression (8 46)
   drivers/spi/spi-renesas-rpc.c:403:13: warning: unknown expression (8 46)
   drivers/spi/spi-renesas-rpc.c:447:23: warning: unknown expression (8 46)
   drivers/spi/spi-renesas-rpc.c:447:43: warning: unknown expression (8 46)
   drivers/spi/spi-renesas-rpc.c:454:36: warning: unknown expression (8 46)
   drivers/spi/spi-renesas-rpc.c:366:47: warning: 'struct spi_mem_dirmap_desc' declared inside parameter list will not be visible outside of this definition or declaration
    static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
                                                  ^~~~~~~~~~~~~~~~~~~
   In file included from drivers/spi/spi-renesas-rpc.c:18:0:
   drivers/spi/spi-renesas-rpc.c: In function 'rpc_spi_mem_dirmap_read':
   drivers/spi/spi-renesas-rpc.c:369:51: error: dereferencing pointer to incomplete type 'struct spi_mem_dirmap_desc'
     struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
                                                      ^
   include/linux/spi/spi.h:1333:66: note: in definition of macro 'spi_master_get_devdata'
    #define spi_master_get_devdata(_ctlr) spi_controller_get_devdata(_ctlr)
                                                                     ^~~~~
   drivers/spi/spi-renesas-rpc.c: At top level:
   drivers/spi/spi-renesas-rpc.c:397:48: warning: 'struct spi_mem_dirmap_desc' declared inside parameter list will not be visible outside of this definition or declaration
    static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
                                                   ^~~~~~~~~~~~~~~~~~~
   In file included from drivers/spi/spi-renesas-rpc.c:18:0:
   drivers/spi/spi-renesas-rpc.c: In function 'rpc_spi_mem_dirmap_write':
   drivers/spi/spi-renesas-rpc.c:400:51: error: dereferencing pointer to incomplete type 'struct spi_mem_dirmap_desc'
     struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
                                                      ^
   include/linux/spi/spi.h:1333:66: note: in definition of macro 'spi_master_get_devdata'
    #define spi_master_get_devdata(_ctlr) spi_controller_get_devdata(_ctlr)
                                                                     ^~~~~
   drivers/spi/spi-renesas-rpc.c: At top level:
   drivers/spi/spi-renesas-rpc.c:443:45: warning: 'struct spi_mem_dirmap_desc' declared inside parameter list will not be visible outside of this definition or declaration
    static int rpc_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
                                                ^~~~~~~~~~~~~~~~~~~
   In file included from drivers/spi/spi-renesas-rpc.c:18:0:
   drivers/spi/spi-renesas-rpc.c: In function 'rpc_spi_mem_dirmap_create':
   drivers/spi/spi-renesas-rpc.c:445:51: error: dereferencing pointer to incomplete type 'struct spi_mem_dirmap_desc'
     struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
                                                      ^
   include/linux/spi/spi.h:1333:66: note: in definition of macro 'spi_master_get_devdata'
    #define spi_master_get_devdata(_ctlr) spi_controller_get_devdata(_ctlr)
                                                                     ^~~~~
   drivers/spi/spi-renesas-rpc.c: At top level:
   drivers/spi/spi-renesas-rpc.c:484:3: error: 'const struct spi_controller_mem_ops' has no member named 'dirmap_create'
     .dirmap_create = rpc_spi_mem_dirmap_create,
      ^~~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:484:19: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
     .dirmap_create = rpc_spi_mem_dirmap_create,
                      ^~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:484:19: note: (near initialization for 'rpc_spi_mem_ops')
   drivers/spi/spi-renesas-rpc.c:484:19: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
   drivers/spi/spi-renesas-rpc.c:484:19: note: (near initialization for 'rpc_spi_mem_ops.supports_op')
   drivers/spi/spi-renesas-rpc.c:485:3: error: 'const struct spi_controller_mem_ops' has no member named 'dirmap_read'
     .dirmap_read = rpc_spi_mem_dirmap_read,
      ^~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:485:17: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
     .dirmap_read = rpc_spi_mem_dirmap_read,
                    ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:485:17: note: (near initialization for 'rpc_spi_mem_ops')
   drivers/spi/spi-renesas-rpc.c:485:17: warning: excess elements in struct initializer
   drivers/spi/spi-renesas-rpc.c:485:17: note: (near initialization for 'rpc_spi_mem_ops')
   drivers/spi/spi-renesas-rpc.c:486:3: error: 'const struct spi_controller_mem_ops' has no member named 'dirmap_write'
     .dirmap_write = rpc_spi_mem_dirmap_write,
      ^~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:486:18: error: positional initialization of field in 'struct' declared with 'designated_init' attribute [-Werror=designated-init]
     .dirmap_write = rpc_spi_mem_dirmap_write,
                     ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/spi/spi-renesas-rpc.c:486:18: note: (near initialization for 'rpc_spi_mem_ops')
   drivers/spi/spi-renesas-rpc.c:486:18: warning: excess elements in struct initializer
   drivers/spi/spi-renesas-rpc.c:486:18: note: (near initialization for 'rpc_spi_mem_ops')
   cc1: some warnings being treated as errors

vim +/mem +369 drivers/spi/spi-renesas-rpc.c

c4789a83 Mason Yang 2018-11-19  226  
c4789a83 Mason Yang 2018-11-19  227  static int rpc_spi_io_xfer(struct rpc_spi *rpc,
c4789a83 Mason Yang 2018-11-19  228  			   const void *tx_buf, void *rx_buf)
c4789a83 Mason Yang 2018-11-19  229  {
c4789a83 Mason Yang 2018-11-19  230  	u32 smenr, smcr, data, pos = 0;
c4789a83 Mason Yang 2018-11-19  231  	int ret = 0;
c4789a83 Mason Yang 2018-11-19  232  
c4789a83 Mason Yang 2018-11-19  233  	writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
c4789a83 Mason Yang 2018-11-19  234  	       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
c4789a83 Mason Yang 2018-11-19  235  	writel(0x0, rpc->regs + RPC_SMDRENR);
c4789a83 Mason Yang 2018-11-19  236  
c4789a83 Mason Yang 2018-11-19  237  	if (tx_buf) {
c4789a83 Mason Yang 2018-11-19  238  		writel(rpc->cmd, rpc->regs + RPC_SMCMR);
c4789a83 Mason Yang 2018-11-19  239  		writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
c4789a83 Mason Yang 2018-11-19  240  		writel(rpc->addr, rpc->regs + RPC_SMADR);
c4789a83 Mason Yang 2018-11-19  241  		smenr = rpc->smenr;
c4789a83 Mason Yang 2018-11-19  242  
c4789a83 Mason Yang 2018-11-19  243  		while (pos < rpc->xferlen) {
c4789a83 Mason Yang 2018-11-19  244  			u32 nbytes = rpc->xferlen  - pos;
c4789a83 Mason Yang 2018-11-19  245  
c4789a83 Mason Yang 2018-11-19  246  			writel(*(u32 *)(tx_buf + pos), rpc->regs + RPC_SMWDR0);
c4789a83 Mason Yang 2018-11-19  247  
c4789a83 Mason Yang 2018-11-19  248  			if (nbytes > 4) {
c4789a83 Mason Yang 2018-11-19  249  				nbytes = 4;
c4789a83 Mason Yang 2018-11-19  250  				smcr = rpc->smcr |
c4789a83 Mason Yang 2018-11-19  251  				       RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
c4789a83 Mason Yang 2018-11-19  252  			} else {
c4789a83 Mason Yang 2018-11-19  253  				smcr = rpc->smcr | RPC_SMCR_SPIE;
c4789a83 Mason Yang 2018-11-19  254  			}
c4789a83 Mason Yang 2018-11-19  255  
c4789a83 Mason Yang 2018-11-19  256  			writel(smenr, rpc->regs + RPC_SMENR);
c4789a83 Mason Yang 2018-11-19  257  			writel(smcr, rpc->regs + RPC_SMCR);
c4789a83 Mason Yang 2018-11-19  258  			ret = wait_msg_xfer_end(rpc);
c4789a83 Mason Yang 2018-11-19  259  			if (ret)
c4789a83 Mason Yang 2018-11-19  260  				goto out;
c4789a83 Mason Yang 2018-11-19  261  
c4789a83 Mason Yang 2018-11-19  262  			pos += nbytes;
c4789a83 Mason Yang 2018-11-19  263  			smenr = rpc->smenr & ~RPC_SMENR_CDE &
c4789a83 Mason Yang 2018-11-19  264  					     ~RPC_SMENR_ADE(0xf);
c4789a83 Mason Yang 2018-11-19  265  		}
c4789a83 Mason Yang 2018-11-19  266  	} else if (rx_buf) {
c4789a83 Mason Yang 2018-11-19  267  		while (pos < rpc->xferlen) {
c4789a83 Mason Yang 2018-11-19  268  			u32 nbytes = rpc->xferlen  - pos;
c4789a83 Mason Yang 2018-11-19  269  
c4789a83 Mason Yang 2018-11-19  270  			if (nbytes > 4)
c4789a83 Mason Yang 2018-11-19  271  				nbytes = 4;
c4789a83 Mason Yang 2018-11-19  272  
c4789a83 Mason Yang 2018-11-19  273  			writel(rpc->cmd, rpc->regs + RPC_SMCMR);
c4789a83 Mason Yang 2018-11-19  274  			writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
c4789a83 Mason Yang 2018-11-19  275  			writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
c4789a83 Mason Yang 2018-11-19  276  			writel(rpc->smenr, rpc->regs + RPC_SMENR);
c4789a83 Mason Yang 2018-11-19  277  			writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
c4789a83 Mason Yang 2018-11-19  278  			ret = wait_msg_xfer_end(rpc);
c4789a83 Mason Yang 2018-11-19  279  			if (ret)
c4789a83 Mason Yang 2018-11-19  280  				goto out;
c4789a83 Mason Yang 2018-11-19  281  
c4789a83 Mason Yang 2018-11-19  282  			data = readl(rpc->regs + RPC_SMRDR0);
c4789a83 Mason Yang 2018-11-19 @283  			memcpy_fromio(rx_buf + pos, (void *)&data, nbytes);
c4789a83 Mason Yang 2018-11-19  284  			pos += nbytes;
c4789a83 Mason Yang 2018-11-19  285  		}
c4789a83 Mason Yang 2018-11-19  286  	} else {
c4789a83 Mason Yang 2018-11-19  287  		writel(rpc->cmd, rpc->regs + RPC_SMCMR);
c4789a83 Mason Yang 2018-11-19  288  		writel(rpc->dummy, rpc->regs + RPC_SMDMCR);
c4789a83 Mason Yang 2018-11-19  289  		writel(rpc->addr + pos, rpc->regs + RPC_SMADR);
c4789a83 Mason Yang 2018-11-19  290  		writel(rpc->smenr, rpc->regs + RPC_SMENR);
c4789a83 Mason Yang 2018-11-19  291  		writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
c4789a83 Mason Yang 2018-11-19  292  		ret = wait_msg_xfer_end(rpc);
c4789a83 Mason Yang 2018-11-19  293  	}
c4789a83 Mason Yang 2018-11-19  294  out:
c4789a83 Mason Yang 2018-11-19  295  	return ret;
c4789a83 Mason Yang 2018-11-19  296  }
c4789a83 Mason Yang 2018-11-19  297  
c4789a83 Mason Yang 2018-11-19  298  static void rpc_spi_mem_set_prep_op_cfg(struct spi_device *spi,
c4789a83 Mason Yang 2018-11-19  299  					const struct spi_mem_op *op,
c4789a83 Mason Yang 2018-11-19  300  					u64 *offs, size_t *len)
c4789a83 Mason Yang 2018-11-19  301  {
c4789a83 Mason Yang 2018-11-19  302  	struct rpc_spi *rpc = spi_master_get_devdata(spi->master);
c4789a83 Mason Yang 2018-11-19  303  
c4789a83 Mason Yang 2018-11-19  304  	rpc->cmd = RPC_SMCMR_CMD(op->cmd.opcode);
c4789a83 Mason Yang 2018-11-19  305  	rpc->smenr = RPC_SMENR_CDE |
c4789a83 Mason Yang 2018-11-19  306  		     RPC_SMENR_CDB(fls(op->cmd.buswidth >> 1));
c4789a83 Mason Yang 2018-11-19  307  	rpc->totalxferlen = 1;
c4789a83 Mason Yang 2018-11-19  308  	rpc->xferlen = 0;
c4789a83 Mason Yang 2018-11-19  309  	rpc->addr = 0;
c4789a83 Mason Yang 2018-11-19  310  
c4789a83 Mason Yang 2018-11-19  311  	if (op->addr.nbytes) {
c4789a83 Mason Yang 2018-11-19  312  		rpc->smenr |= RPC_SMENR_ADB(fls(op->addr.buswidth >> 1));
c4789a83 Mason Yang 2018-11-19  313  		if (op->addr.nbytes == 4)
c4789a83 Mason Yang 2018-11-19  314  			rpc->smenr |= RPC_SMENR_ADE(0xf);
c4789a83 Mason Yang 2018-11-19  315  		else
c4789a83 Mason Yang 2018-11-19  316  			rpc->smenr |= RPC_SMENR_ADE(0x7);
c4789a83 Mason Yang 2018-11-19  317  
c4789a83 Mason Yang 2018-11-19  318  		if (!offs && !len)
c4789a83 Mason Yang 2018-11-19  319  			rpc->addr = *(u32 *)offs;
c4789a83 Mason Yang 2018-11-19  320  		else
c4789a83 Mason Yang 2018-11-19  321  			rpc->addr = op->addr.val;
c4789a83 Mason Yang 2018-11-19  322  		rpc->totalxferlen += op->addr.nbytes;
c4789a83 Mason Yang 2018-11-19  323  	}
c4789a83 Mason Yang 2018-11-19  324  
c4789a83 Mason Yang 2018-11-19  325  	if (op->dummy.nbytes) {
c4789a83 Mason Yang 2018-11-19  326  		rpc->smenr |= RPC_SMENR_DME;
c4789a83 Mason Yang 2018-11-19  327  		rpc->dummy = RPC_SMDMCR_DMCYC(op->dummy.nbytes);
c4789a83 Mason Yang 2018-11-19  328  		rpc->totalxferlen += op->dummy.nbytes;
c4789a83 Mason Yang 2018-11-19  329  	}
c4789a83 Mason Yang 2018-11-19  330  
c4789a83 Mason Yang 2018-11-19  331  	if (op->data.nbytes || (offs && len)) {
c4789a83 Mason Yang 2018-11-19  332  		rpc->smenr |= RPC_SMENR_SPIDE(rpc_bits_xfer(op->data.nbytes)) |
c4789a83 Mason Yang 2018-11-19  333  			      RPC_SMENR_SPIDB(fls(op->data.buswidth >> 1));
c4789a83 Mason Yang 2018-11-19  334  
c4789a83 Mason Yang 2018-11-19  335  		if (op->data.dir == SPI_MEM_DATA_IN) {
c4789a83 Mason Yang 2018-11-19  336  			rpc->smcr = RPC_SMCR_SPIRE;
c4789a83 Mason Yang 2018-11-19  337  			rpc->xfer_dir = SPI_MEM_DATA_IN;
c4789a83 Mason Yang 2018-11-19  338  		} else if (op->data.dir == SPI_MEM_DATA_OUT) {
c4789a83 Mason Yang 2018-11-19  339  			rpc->smcr = RPC_SMCR_SPIWE;
c4789a83 Mason Yang 2018-11-19  340  			rpc->xfer_dir = SPI_MEM_DATA_OUT;
c4789a83 Mason Yang 2018-11-19  341  		}
c4789a83 Mason Yang 2018-11-19  342  
c4789a83 Mason Yang 2018-11-19  343  		if (offs && len) {
c4789a83 Mason Yang 2018-11-19  344  			rpc->xferlen = *(u32 *)len;
c4789a83 Mason Yang 2018-11-19  345  			rpc->totalxferlen += *(u32 *)len;
c4789a83 Mason Yang 2018-11-19  346  		} else {
c4789a83 Mason Yang 2018-11-19  347  			rpc->xferlen = op->data.nbytes;
c4789a83 Mason Yang 2018-11-19  348  			rpc->totalxferlen += op->data.nbytes;
c4789a83 Mason Yang 2018-11-19  349  		}
c4789a83 Mason Yang 2018-11-19  350  	}
c4789a83 Mason Yang 2018-11-19  351  }
c4789a83 Mason Yang 2018-11-19  352  
c4789a83 Mason Yang 2018-11-19  353  static bool rpc_spi_mem_supports_op(struct spi_mem *mem,
c4789a83 Mason Yang 2018-11-19  354  				    const struct spi_mem_op *op)
c4789a83 Mason Yang 2018-11-19  355  {
c4789a83 Mason Yang 2018-11-19  356  	if (op->data.buswidth > 4 || op->addr.buswidth > 4 ||
c4789a83 Mason Yang 2018-11-19  357  	    op->dummy.buswidth > 4 || op->cmd.buswidth > 4)
c4789a83 Mason Yang 2018-11-19  358  		return false;
c4789a83 Mason Yang 2018-11-19  359  
c4789a83 Mason Yang 2018-11-19  360  	if (op->addr.nbytes > 4)
c4789a83 Mason Yang 2018-11-19  361  		return false;
c4789a83 Mason Yang 2018-11-19  362  
c4789a83 Mason Yang 2018-11-19  363  	return true;
c4789a83 Mason Yang 2018-11-19  364  }
c4789a83 Mason Yang 2018-11-19  365  
c4789a83 Mason Yang 2018-11-19  366  static ssize_t rpc_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
c4789a83 Mason Yang 2018-11-19  367  				       u64 offs, size_t len, void *buf)
c4789a83 Mason Yang 2018-11-19  368  {
c4789a83 Mason Yang 2018-11-19 @369  	struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
c4789a83 Mason Yang 2018-11-19  370  	int ret;
c4789a83 Mason Yang 2018-11-19  371  
c4789a83 Mason Yang 2018-11-19 @372  	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
c4789a83 Mason Yang 2018-11-19  373  		return -EINVAL;
c4789a83 Mason Yang 2018-11-19  374  
c4789a83 Mason Yang 2018-11-19  375  	ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
c4789a83 Mason Yang 2018-11-19  376  	if (ret)
c4789a83 Mason Yang 2018-11-19  377  		return ret;
c4789a83 Mason Yang 2018-11-19  378  
c4789a83 Mason Yang 2018-11-19  379  	rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
c4789a83 Mason Yang 2018-11-19  380  				    &desc->info.op_tmpl, &offs, &len);
c4789a83 Mason Yang 2018-11-19  381  
c4789a83 Mason Yang 2018-11-19  382  	writel(RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
c4789a83 Mason Yang 2018-11-19  383  	       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
c4789a83 Mason Yang 2018-11-19  384  
c4789a83 Mason Yang 2018-11-19  385  	writel(RPC_DRCR_RBURST(0x1f) | RPC_DRCR_RBE, rpc->regs + RPC_DRCR);
c4789a83 Mason Yang 2018-11-19  386  	writel(rpc->cmd, rpc->regs + RPC_DRCMR);
c4789a83 Mason Yang 2018-11-19  387  	writel(RPC_DREAR_EAC, rpc->regs + RPC_DREAR);
c4789a83 Mason Yang 2018-11-19  388  	writel(0, rpc->regs + RPC_DROPR);
c4789a83 Mason Yang 2018-11-19  389  	writel(rpc->smenr, rpc->regs + RPC_DRENR);
c4789a83 Mason Yang 2018-11-19  390  	writel(rpc->dummy, rpc->regs + RPC_DRDMCR);
c4789a83 Mason Yang 2018-11-19  391  	writel(0x0, rpc->regs + RPC_DRDRENR);
c4789a83 Mason Yang 2018-11-19  392  	memcpy_fromio(buf, rpc->linear.map + desc->info.offset + offs, len);
c4789a83 Mason Yang 2018-11-19  393  
c4789a83 Mason Yang 2018-11-19  394  	return len;
c4789a83 Mason Yang 2018-11-19  395  }
c4789a83 Mason Yang 2018-11-19  396  
c4789a83 Mason Yang 2018-11-19  397  static ssize_t rpc_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
c4789a83 Mason Yang 2018-11-19  398  					u64 offs, size_t len, const void *buf)
c4789a83 Mason Yang 2018-11-19  399  {
c4789a83 Mason Yang 2018-11-19 @400  	struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
c4789a83 Mason Yang 2018-11-19  401  	int tx_offs, ret;
c4789a83 Mason Yang 2018-11-19  402  
c4789a83 Mason Yang 2018-11-19  403  	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
c4789a83 Mason Yang 2018-11-19  404  		return -EINVAL;
c4789a83 Mason Yang 2018-11-19  405  
c4789a83 Mason Yang 2018-11-19  406  	if (WARN_ON(len > RPC_WBUF_SIZE))
c4789a83 Mason Yang 2018-11-19  407  		return -EIO;
c4789a83 Mason Yang 2018-11-19  408  
c4789a83 Mason Yang 2018-11-19  409  	ret = rpc_spi_set_freq(rpc, desc->mem->spi->max_speed_hz);
c4789a83 Mason Yang 2018-11-19  410  	if (ret)
c4789a83 Mason Yang 2018-11-19  411  		return ret;
c4789a83 Mason Yang 2018-11-19  412  
c4789a83 Mason Yang 2018-11-19 @413  	rpc_spi_mem_set_prep_op_cfg(desc->mem->spi,
c4789a83 Mason Yang 2018-11-19  414  				    &desc->info.op_tmpl, &offs, &len);
c4789a83 Mason Yang 2018-11-19  415  
c4789a83 Mason Yang 2018-11-19  416  	writel(RPC_CMNCR_MD | RPC_CMNCR_SFDE | RPC_CMNCR_MOIIO_HIZ |
c4789a83 Mason Yang 2018-11-19  417  	       RPC_CMNCR_IOFV_HIZ | RPC_CMNCR_BSZ(0), rpc->regs + RPC_CMNCR);
c4789a83 Mason Yang 2018-11-19  418  	writel(0x0, rpc->regs + RPC_SMDRENR);
c4789a83 Mason Yang 2018-11-19  419  
c4789a83 Mason Yang 2018-11-19  420  	writel(RPC_PHYCNT_CAL | 0x260 | RPC_PHYCNT_WBUF2 | RPC_PHYCNT_WBUF,
c4789a83 Mason Yang 2018-11-19  421  	       rpc->regs + RPC_PHYCNT);
c4789a83 Mason Yang 2018-11-19  422  
c4789a83 Mason Yang 2018-11-19  423  	for (tx_offs = 0; tx_offs < RPC_WBUF_SIZE; tx_offs += 4)
c4789a83 Mason Yang 2018-11-19  424  		writel(*(u32 *)(buf + tx_offs), rpc->regs + RPC_WBUF + tx_offs);
c4789a83 Mason Yang 2018-11-19  425  
c4789a83 Mason Yang 2018-11-19  426  	writel(rpc->cmd, rpc->regs + RPC_SMCMR);
c4789a83 Mason Yang 2018-11-19  427  	writel(offs, rpc->regs + RPC_SMADR);
c4789a83 Mason Yang 2018-11-19  428  	writel(rpc->smenr, rpc->regs + RPC_SMENR);
c4789a83 Mason Yang 2018-11-19  429  	writel(rpc->smcr | RPC_SMCR_SPIE, rpc->regs + RPC_SMCR);
c4789a83 Mason Yang 2018-11-19  430  	ret = wait_msg_xfer_end(rpc);
c4789a83 Mason Yang 2018-11-19  431  	if (ret)
c4789a83 Mason Yang 2018-11-19  432  		goto out;
c4789a83 Mason Yang 2018-11-19  433  
c4789a83 Mason Yang 2018-11-19  434  	writel(RPC_DRCR_RCF, rpc->regs + RPC_DRCR);
c4789a83 Mason Yang 2018-11-19  435  	writel(RPC_PHYCNT_CAL | RPC_PHYCNT_STRTIM(0) | 0x260,
c4789a83 Mason Yang 2018-11-19  436  	       rpc->regs + RPC_PHYCNT);
c4789a83 Mason Yang 2018-11-19  437  
c4789a83 Mason Yang 2018-11-19  438  	return len;
c4789a83 Mason Yang 2018-11-19  439  out:
c4789a83 Mason Yang 2018-11-19  440  	return ret;
c4789a83 Mason Yang 2018-11-19  441  }
c4789a83 Mason Yang 2018-11-19  442  
c4789a83 Mason Yang 2018-11-19  443  static int rpc_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
c4789a83 Mason Yang 2018-11-19  444  {
c4789a83 Mason Yang 2018-11-19  445  	struct rpc_spi *rpc = spi_master_get_devdata(desc->mem->spi->master);
c4789a83 Mason Yang 2018-11-19  446  
c4789a83 Mason Yang 2018-11-19  447  	if (desc->info.offset + desc->info.length > U32_MAX)
c4789a83 Mason Yang 2018-11-19  448  		return -ENOTSUPP;
c4789a83 Mason Yang 2018-11-19  449  
c4789a83 Mason Yang 2018-11-19  450  	if (!rpc_spi_mem_supports_op(desc->mem, &desc->info.op_tmpl))
c4789a83 Mason Yang 2018-11-19  451  		return -ENOTSUPP;
c4789a83 Mason Yang 2018-11-19  452  
c4789a83 Mason Yang 2018-11-19  453  	if (!rpc->linear.map &&
c4789a83 Mason Yang 2018-11-19  454  	    desc->info.op_tmpl.data.dir == SPI_MEM_DATA_IN)
c4789a83 Mason Yang 2018-11-19  455  		return -ENOTSUPP;
c4789a83 Mason Yang 2018-11-19  456  
c4789a83 Mason Yang 2018-11-19  457  	return 0;
c4789a83 Mason Yang 2018-11-19  458  }
c4789a83 Mason Yang 2018-11-19  459  
c4789a83 Mason Yang 2018-11-19  460  static int rpc_spi_mem_exec_op(struct spi_mem *mem,
c4789a83 Mason Yang 2018-11-19  461  			       const struct spi_mem_op *op)
c4789a83 Mason Yang 2018-11-19  462  {
c4789a83 Mason Yang 2018-11-19  463  	struct rpc_spi *rpc = spi_master_get_devdata(mem->spi->master);
c4789a83 Mason Yang 2018-11-19  464  	int ret;
c4789a83 Mason Yang 2018-11-19  465  
c4789a83 Mason Yang 2018-11-19  466  	ret = rpc_spi_set_freq(rpc, mem->spi->max_speed_hz);
c4789a83 Mason Yang 2018-11-19  467  	if (ret)
c4789a83 Mason Yang 2018-11-19  468  		return ret;
c4789a83 Mason Yang 2018-11-19  469  
c4789a83 Mason Yang 2018-11-19  470  	rpc_spi_mem_set_prep_op_cfg(mem->spi, op, NULL, NULL);
c4789a83 Mason Yang 2018-11-19  471  
c4789a83 Mason Yang 2018-11-19  472  	ret = rpc_spi_io_xfer(rpc,
c4789a83 Mason Yang 2018-11-19  473  			      op->data.dir == SPI_MEM_DATA_OUT ?
c4789a83 Mason Yang 2018-11-19  474  			      op->data.buf.out : NULL,
c4789a83 Mason Yang 2018-11-19  475  			      op->data.dir == SPI_MEM_DATA_IN ?
c4789a83 Mason Yang 2018-11-19  476  			      op->data.buf.in : NULL);
c4789a83 Mason Yang 2018-11-19  477  
c4789a83 Mason Yang 2018-11-19  478  	return ret;
c4789a83 Mason Yang 2018-11-19  479  }
c4789a83 Mason Yang 2018-11-19  480  
c4789a83 Mason Yang 2018-11-19  481  static const struct spi_controller_mem_ops rpc_spi_mem_ops = {
c4789a83 Mason Yang 2018-11-19  482  	.supports_op = rpc_spi_mem_supports_op,
c4789a83 Mason Yang 2018-11-19  483  	.exec_op = rpc_spi_mem_exec_op,
c4789a83 Mason Yang 2018-11-19 @484  	.dirmap_create = rpc_spi_mem_dirmap_create,
c4789a83 Mason Yang 2018-11-19  485  	.dirmap_read = rpc_spi_mem_dirmap_read,
c4789a83 Mason Yang 2018-11-19  486  	.dirmap_write = rpc_spi_mem_dirmap_write,
c4789a83 Mason Yang 2018-11-19  487  };
c4789a83 Mason Yang 2018-11-19  488  

:::::: The code at line 369 was first introduced by commit
:::::: c4789a83a8be18f144419fba933a3f5ca1b78837 spi: Add Renesas R-Car RPC SPI controller driver

:::::: TO: Mason Yang <[e-mail]>
:::::: CC: 0day robot <[e-mail]>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

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>;
+		};
+	};