diff mbox series

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

Message ID 1556092536-17095-4-git-send-email-masonccyang@mxic.com.tw (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series mfd: Add Renesas R-Car Gen3 RPC-IF MFD & SPI driver | expand

Commit Message

Mason Yang April 24, 2019, 7:55 a.m. UTC
Document the bindings used by the Renesas R-Car Gen3 RPC-IF MFD.

Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
---
 .../devicetree/bindings/mfd/mfd-renesas-rpc.txt    | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt

Comments

Rob Herring (Arm) April 24, 2019, 9:23 p.m. UTC | #1
On Wed, Apr 24, 2019 at 03:55:36PM +0800, Mason Yang wrote:
> Document the bindings used by the Renesas R-Car Gen3 RPC-IF MFD.
> 
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> ---
>  .../devicetree/bindings/mfd/mfd-renesas-rpc.txt    | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> new file mode 100644
> index 0000000..668b822
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> @@ -0,0 +1,40 @@
> +Renesas R-Car Gen3 RPC-IF MFD Device Tree Bindings
> +--------------------------------------------------

Looks like a SPI flash controller from the example. What makes it an 
MFD?

> +
> +Required properties:
> +- compatible: should be an SoC-specific compatible value, followed by
> +		"renesas,rcar-gen3-rpc" as a fallback.
> +		supported SoC-specific values are:
> +		"renesas,r8a77995-rpc"	(R-Car D3)
> +- reg: should contain three register areas:
> +	first for the base address of RPC-IF registers,
> +	second for the direct mapping read mode and
> +	third for the write buffer area.
> +- reg-names: should contain "regs", "dirmap" and "wbuf"
> +- clocks: should contain 1 entries for the module's clock
> +- clock-names: should contain "rpc"
> +- #address-cells: should be 1
> +- #size-cells: should be 0
> +
> +Example:
> +
> +	rpc: rpc-if@ee200000 {
> +		compatible = "renesas,r8a77995-rpc", "renesas,rcar-gen3-rpc";
> +		reg = <0 0xee200000 0 0x200>, <0 0x08000000 0 0x4000000>,
> +		      <0 0xee208000 0 0x100>;
> +		reg-names = "regs", "dirmap", "wbuf";
> +		clocks = <&cpg CPG_MOD 917>;
> +		clock-names = "rpc";
> +		power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
> +		resets = <&cpg 917>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		flash@0 {
> +			compatible = "jedec,spi-nor";
> +			reg = <0>;
> +			spi-max-frequency = <40000000>;
> +			spi-tx-bus-width = <1>;
> +			spi-rx-bus-width = <1>;
> +		};
> +	};
> -- 
> 1.9.1
>
Marek Vasut April 24, 2019, 9:54 p.m. UTC | #2
On 4/24/19 11:23 PM, Rob Herring wrote:
> On Wed, Apr 24, 2019 at 03:55:36PM +0800, Mason Yang wrote:
>> Document the bindings used by the Renesas R-Car Gen3 RPC-IF MFD.
>>
>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
>> ---
>>  .../devicetree/bindings/mfd/mfd-renesas-rpc.txt    | 40 ++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
>> new file mode 100644
>> index 0000000..668b822
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
>> @@ -0,0 +1,40 @@
>> +Renesas R-Car Gen3 RPC-IF MFD Device Tree Bindings
>> +--------------------------------------------------
> 
> Looks like a SPI flash controller from the example. What makes it an 
> MFD?

It supports both SPI NOR and HyperFlash (CFI-compliant flash with
different bus interface).

>> +
>> +Required properties:
>> +- compatible: should be an SoC-specific compatible value, followed by
>> +		"renesas,rcar-gen3-rpc" as a fallback.
>> +		supported SoC-specific values are:
>> +		"renesas,r8a77995-rpc"	(R-Car D3)
>> +- reg: should contain three register areas:
>> +	first for the base address of RPC-IF registers,
>> +	second for the direct mapping read mode and
>> +	third for the write buffer area.
>> +- reg-names: should contain "regs", "dirmap" and "wbuf"
>> +- clocks: should contain 1 entries for the module's clock
>> +- clock-names: should contain "rpc"
>> +- #address-cells: should be 1
>> +- #size-cells: should be 0
>> +
>> +Example:
>> +
>> +	rpc: rpc-if@ee200000 {
>> +		compatible = "renesas,r8a77995-rpc", "renesas,rcar-gen3-rpc";
>> +		reg = <0 0xee200000 0 0x200>, <0 0x08000000 0 0x4000000>,
>> +		      <0 0xee208000 0 0x100>;
>> +		reg-names = "regs", "dirmap", "wbuf";
>> +		clocks = <&cpg CPG_MOD 917>;
>> +		clock-names = "rpc";
>> +		power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
>> +		resets = <&cpg 917>;
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		flash@0 {
>> +			compatible = "jedec,spi-nor";
>> +			reg = <0>;
>> +			spi-max-frequency = <40000000>;
>> +			spi-tx-bus-width = <1>;
>> +			spi-rx-bus-width = <1>;
>> +		};
>> +	};
>> -- 
>> 1.9.1
>>
Rob Herring (Arm) April 25, 2019, 11:07 p.m. UTC | #3
On Wed, Apr 24, 2019 at 4:54 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 4/24/19 11:23 PM, Rob Herring wrote:
> > On Wed, Apr 24, 2019 at 03:55:36PM +0800, Mason Yang wrote:
> >> Document the bindings used by the Renesas R-Car Gen3 RPC-IF MFD.
> >>
> >> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> >> ---
> >>  .../devicetree/bindings/mfd/mfd-renesas-rpc.txt    | 40 ++++++++++++++++++++++
> >>  1 file changed, 40 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> >> new file mode 100644
> >> index 0000000..668b822
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> >> @@ -0,0 +1,40 @@
> >> +Renesas R-Car Gen3 RPC-IF MFD Device Tree Bindings
> >> +--------------------------------------------------
> >
> > Looks like a SPI flash controller from the example. What makes it an
> > MFD?
>
> It supports both SPI NOR and HyperFlash (CFI-compliant flash with
> different bus interface).

Not sure that really makes it an MFD. Lee probably has an opinion.

In any case, can we get a complete binding that shows both.

Rob
Marek Vasut April 26, 2019, 1:44 a.m. UTC | #4
On 4/26/19 1:07 AM, Rob Herring wrote:
> On Wed, Apr 24, 2019 at 4:54 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> On 4/24/19 11:23 PM, Rob Herring wrote:
>>> On Wed, Apr 24, 2019 at 03:55:36PM +0800, Mason Yang wrote:
>>>> Document the bindings used by the Renesas R-Car Gen3 RPC-IF MFD.
>>>>
>>>> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
>>>> ---
>>>>  .../devicetree/bindings/mfd/mfd-renesas-rpc.txt    | 40 ++++++++++++++++++++++
>>>>  1 file changed, 40 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
>>>> new file mode 100644
>>>> index 0000000..668b822
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
>>>> @@ -0,0 +1,40 @@
>>>> +Renesas R-Car Gen3 RPC-IF MFD Device Tree Bindings
>>>> +--------------------------------------------------
>>>
>>> Looks like a SPI flash controller from the example. What makes it an
>>> MFD?
>>
>> It supports both SPI NOR and HyperFlash (CFI-compliant flash with
>> different bus interface).
> 
> Not sure that really makes it an MFD. Lee probably has an opinion.
> 
> In any case, can we get a complete binding that shows both.

Presumably that would only mean the flash compatible is some "cfi,nor"
or somesuch. But I agree, would be nice to have both in the example.
Lee Jones May 7, 2019, 12:57 p.m. UTC | #5
On Wed, 24 Apr 2019, Marek Vasut wrote:

> On 4/24/19 11:23 PM, Rob Herring wrote:
> > On Wed, Apr 24, 2019 at 03:55:36PM +0800, Mason Yang wrote:
> >> Document the bindings used by the Renesas R-Car Gen3 RPC-IF MFD.
> >>
> >> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> >> ---
> >>  .../devicetree/bindings/mfd/mfd-renesas-rpc.txt    | 40 ++++++++++++++++++++++
> >>  1 file changed, 40 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> >> new file mode 100644
> >> index 0000000..668b822
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> >> @@ -0,0 +1,40 @@
> >> +Renesas R-Car Gen3 RPC-IF MFD Device Tree Bindings
> >> +--------------------------------------------------
> > 
> > Looks like a SPI flash controller from the example. What makes it an 
> > MFD?
> 
> It supports both SPI NOR and HyperFlash (CFI-compliant flash with
> different bus interface).

Looks like you're registering one OR the other.

Why don't you just do this from DT?

No reason for this to be an MFD IMHO.
Marek Vasut May 8, 2019, 3:52 a.m. UTC | #6
On 5/8/19 4:48 AM, masonccyang@mxic.com.tw wrote:
> Hi Jones,
> 
>> "Lee Jones" <lee.jones@linaro.org>
>> 2019/05/07 下午 08:58
>>
>> To
>>
>> "Marek Vasut" <marek.vasut@gmail.com>,
>>
>> cc
>>
>> "Rob Herring" <robh@kernel.org>, "Mason Yang"
>> <masonccyang@mxic.com.tw>, broonie@kernel.org, linux-
>> kernel@vger.kernel.org, linux-spi@vger.kernel.org,
>> bbrezillon@kernel.org, linux-renesas-soc@vger.kernel.org, "Geert
>> Uytterhoeven" <geert+renesas@glider.be>,
>> sergei.shtylyov@cogentembedded.com, mark.rutland@arm.com,
>> devicetree@vger.kernel.org, juliensu@mxic.com.tw, "Simon Horman"
>> <horms@verge.net.au>, zhengxunli@mxic.com.tw
>>
>> Subject
>>
>> Re: [PATCH v12 3/3] dt-bindings: mfd: Document Renesas R-Car Gen3
>> RPC-IF MFD bindings
>>
>> On Wed, 24 Apr 2019, Marek Vasut wrote:
>>
>> > On 4/24/19 11:23 PM, Rob Herring wrote:
>> > > On Wed, Apr 24, 2019 at 03:55:36PM +0800, Mason Yang wrote:
>> > >> Document the bindings used by the Renesas R-Car Gen3 RPC-IF MFD.
>> > >>
>> > >> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
>> > >> ---
>> > >>  .../devicetree/bindings/mfd/mfd-renesas-rpc.txt    | 40 ++++++
>> ++++++++++++++++
>> > >>  1 file changed, 40 insertions(+)
>> > >>  create mode 100644 Documentation/devicetree/bindings/mfd/mfd-
>> renesas-rpc.txt
>> > >>
>> > >> diff --git a/Documentation/devicetree/bindings/mfd/mfd-renesas-
>> rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
>> > >> new file mode 100644
>> > >> index 0000000..668b822
>> > >> --- /dev/null
>> > >> +++ b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
>> > >> @@ -0,0 +1,40 @@
>> > >> +Renesas R-Car Gen3 RPC-IF MFD Device Tree Bindings
>> > >> +--------------------------------------------------
>> > >
>> > > Looks like a SPI flash controller from the example. What makes it an
>> > > MFD?
>> >
>> > It supports both SPI NOR and HyperFlash (CFI-compliant flash with
>> > different bus interface).
>>
>> Looks like you're registering one OR the other.
>>
>> Why don't you just do this from DT?
>>
>> No reason for this to be an MFD IMHO.
> 
> 
> okay, I will patch it back to SPI mode only.

I don't think that's what Lee meant . The controller supports _both_
modes , hence it would have the same compatible string. You just need to
extract the mode of operation from the DT.
Lee Jones May 8, 2019, 6:11 a.m. UTC | #7
On Wed, 08 May 2019, Marek Vasut wrote:

> On 5/8/19 4:48 AM, masonccyang@mxic.com.tw wrote:
> > Hi Jones,
> > 
> >> "Lee Jones" <lee.jones@linaro.org>
> >> 2019/05/07 下午 08:58
> >>
> >> To
> >>
> >> "Marek Vasut" <marek.vasut@gmail.com>,
> >>
> >> cc
> >>
> >> "Rob Herring" <robh@kernel.org>, "Mason Yang"
> >> <masonccyang@mxic.com.tw>, broonie@kernel.org, linux-
> >> kernel@vger.kernel.org, linux-spi@vger.kernel.org,
> >> bbrezillon@kernel.org, linux-renesas-soc@vger.kernel.org, "Geert
> >> Uytterhoeven" <geert+renesas@glider.be>,
> >> sergei.shtylyov@cogentembedded.com, mark.rutland@arm.com,
> >> devicetree@vger.kernel.org, juliensu@mxic.com.tw, "Simon Horman"
> >> <horms@verge.net.au>, zhengxunli@mxic.com.tw
> >>
> >> Subject
> >>
> >> Re: [PATCH v12 3/3] dt-bindings: mfd: Document Renesas R-Car Gen3
> >> RPC-IF MFD bindings
> >>
> >> On Wed, 24 Apr 2019, Marek Vasut wrote:
> >>
> >> > On 4/24/19 11:23 PM, Rob Herring wrote:
> >> > > On Wed, Apr 24, 2019 at 03:55:36PM +0800, Mason Yang wrote:
> >> > >> Document the bindings used by the Renesas R-Car Gen3 RPC-IF MFD.
> >> > >>
> >> > >> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> >> > >> ---
> >> > >>  .../devicetree/bindings/mfd/mfd-renesas-rpc.txt    | 40 ++++++
> >> ++++++++++++++++
> >> > >>  1 file changed, 40 insertions(+)
> >> > >>  create mode 100644 Documentation/devicetree/bindings/mfd/mfd-
> >> renesas-rpc.txt
> >> > >>
> >> > >> diff --git a/Documentation/devicetree/bindings/mfd/mfd-renesas-
> >> rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> >> > >> new file mode 100644
> >> > >> index 0000000..668b822
> >> > >> --- /dev/null
> >> > >> +++ b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> >> > >> @@ -0,0 +1,40 @@
> >> > >> +Renesas R-Car Gen3 RPC-IF MFD Device Tree Bindings
> >> > >> +--------------------------------------------------
> >> > >
> >> > > Looks like a SPI flash controller from the example. What makes it an
> >> > > MFD?
> >> >
> >> > It supports both SPI NOR and HyperFlash (CFI-compliant flash with
> >> > different bus interface).
> >>
> >> Looks like you're registering one OR the other.
> >>
> >> Why don't you just do this from DT?
> >>
> >> No reason for this to be an MFD IMHO.
> > 
> > 
> > okay, I will patch it back to SPI mode only.
> 
> I don't think that's what Lee meant . The controller supports _both_
> modes , hence it would have the same compatible string. You just need to
> extract the mode of operation from the DT.

HiSilicon attempted to upstream something similar, only their
controller provided NAND and NOR functionality.  They used different
compatible strings to differentiate between the varying
technologies.

They too tried to use MFD as a means to select between them (which was
also NACKed).  Not sure what they ended up doing, but the original
submission and (half of) the conversation can be found at [0].  Some
more of the thread continues at [1].

Hope that helps.

[0] https://groups.google.com/forum/#!topic/fa.linux.kernel/F6i9o8sfOIw
[1] https://marc.info/?l=devicetree&m=147669165104431&w=2
Sergei Shtylyov May 9, 2019, 7:23 p.m. UTC | #8
Hello!

On 05/09/2019 05:06 AM, masonccyang@mxic.com.tw wrote:

[...]
>> > >> > On 4/24/19 11:23 PM, Rob Herring wrote:
>> > >> > > On Wed, Apr 24, 2019 at 03:55:36PM +0800, Mason Yang wrote:
>> > >> > >> Document the bindings used by the Renesas R-Car Gen3 RPC-IF MFD.
>> > >> > >>
>> > >> > >> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
>> > >> > >> ---
>> > >> > >>  .../devicetree/bindings/mfd/mfd-renesas-rpc.txt    | 40 ++++++
>> > >> ++++++++++++++++
>> > >> > >>  1 file changed, 40 insertions(+)
>> > >> > >>  create mode 100644 Documentation/devicetree/bindings/mfd/mfd-
>> > >> renesas-rpc.txt
>> > >> > >>
>> > >> > >> diff --git a/Documentation/devicetree/bindings/mfd/mfd-renesas-
>> > >> rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
>> > >> > >> new file mode 100644
>> > >> > >> index 0000000..668b822
>> > >> > >> --- /dev/null
>> > >> > >> +++ b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
>> > >> > >> @@ -0,0 +1,40 @@
>> > >> > >> +Renesas R-Car Gen3 RPC-IF MFD Device Tree Bindings
>> > >> > >> +--------------------------------------------------
>> > >> > >
>> > >> > > Looks like a SPI flash controller from the example. What makes it an
>> > >> > > MFD?
>> > >> >
>> > >> > It supports both SPI NOR and HyperFlash (CFI-compliant flash with
>> > >> > different bus interface).
>> > >>
>> > >> Looks like you're registering one OR the other.
>> > >>
>> > >> Why don't you just do this from DT?
>> > >>
>> > >> No reason for this to be an MFD IMHO.
>> > >
>> > >
>> > > okay, I will patch it back to SPI mode only.
>> >
>> > I don't think that's what Lee meant . The controller supports _both_
>> > modes , hence it would have the same compatible string. You just need to
>> > extract the mode of operation from the DT.
>>
>> HiSilicon attempted to upstream something similar, only their
>> controller provided NAND and NOR functionality.  They used different
>> compatible strings to differentiate between the varying
>> technologies.
>>
>> They too tried to use MFD as a means to select between them (which was
>> also NACKed).  Not sure what they ended up doing, but the original
>> submission and (half of) the conversation can be found at [0].  Some
>> more of the thread continues at [1].
>>
>> Hope that helps.
>>
>> [0] https://groups.google.com/forum/#!topic/fa.linux.kernel/F6i9o8sfOIw
>> [1] https://marc.info/?l=devicetree&m=147669165104431&w=2
> 
> 
> Hi Marek,
> 
> By Jones's comments:
> --------------------------------------------------------------------------
>> From: Shunquan Lin <linshunquan1@hisilicon.com>
>>
>> This patch adds driver support for HiSilicon Flash Memory
>> Controller(FMC). HiSilicon FMC is a multi-functions device which
>> supports SPI Nor flash controller, SPI nand Flash controller and
>> parallel nand flash controller.
> 
> MFDs are for devices which span multiple subsystems.

   And we do! One of the subdrivers will live under drivers/spi/, the other
under drivers/mtd/...

> 
> Please submit this to MTD.
> 
> _https://marc.info/?l=devicetree&m=147376842210229&w=2_
> -------------------------------------------------------------------------------------------------
> 
> 
> I will patch RPC-IF back to SPI mode according to previous patches:

   I still don't see why you want to do this...

> -----------------------------------------------------------------------
> On 2/12/19 3:22 PM, Mark Brown wrote:
>> The patch
>>
>>    spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
>>
>> has been applied to the spi tree at
>>
>>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
>>
>> All being well this means that it will be integrated into the linux-next
>> tree (usually sometime in the next 24 hours) and sent to Linus during
>> the next merge window (or sooner if it is a bug fix), however if
>> problems are discovered then the patch may be dropped or reverted.  
>>
>> You may get further e-mails resulting from automated or manual testing
>> and review of the tree, please engage with people reporting problems and
>> send followup patches addressing any issues that are reported if needed.
>>
>> If any updates are required or you are submitting further changes they
>> should be sent as incremental updates against current git, existing
>> patches will not be replaced.
>>
>> Please add any relevant lists and maintainers to the CCs when replying
>> to this mail.
> ------------------------------------------------------------------------
> 
> agreed it ?

   No, I don't agree.

[...]
> thanks & best regards,
> Mason

MBR, Sergei
Geert Uytterhoeven May 13, 2019, 7:11 a.m. UTC | #9
Hi Mason,

On Fri, May 10, 2019 at 3:09 AM <masonccyang@mxic.com.tw> wrote:
> > "Sergei Shtylyov" <sergei.shtylyov@cogentembedded.com>
> > 2019/05/10 上午 03:24
> > mark.rutland@arm.com, "Rob Herring" <robh@kernel.org>, zhengxunli@mxic.com.tw
> > On 05/09/2019 05:06 AM, masonccyang@mxic.com.tw wrote:
> >
> > [...]
> > >> > >> > On 4/24/19 11:23 PM, Rob Herring wrote:
> > >> > >> > > On Wed, Apr 24, 2019 at 03:55:36PM +0800, Mason Yang wrote:
> > >> > >> > >> Document the bindings used by the Renesas R-Car Gen3 RPC-IF MFD.
> > >> > >> > >>
> > >> > >> > >> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> > >> > >> > >> ---
> > >> > >> > >>  .../devicetree/bindings/mfd/mfd-renesas-rpc.txt    | 40 ++++++
> > >> > >> ++++++++++++++++
> > >> > >> > >>  1 file changed, 40 insertions(+)
> > >> > >> > >>  create mode 100644 Documentation/devicetree/bindings/mfd/mfd-
> > >> > >> renesas-rpc.txt
> > >> > >> > >>
> > >> > >> > >> diff --git a/Documentation/devicetree/bindings/mfd/mfd-renesas-
> > >> > >> rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> > >> > >> > >> new file mode 100644
> > >> > >> > >> index 0000000..668b822
> > >> > >> > >> --- /dev/null
> > >> > >> > >> +++ b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
> > >> > >> > >> @@ -0,0 +1,40 @@
> > >> > >> > >> +Renesas R-Car Gen3 RPC-IF MFD Device Tree Bindings
> > >> > >> > >> +--------------------------------------------------
> > >> > >> > >
> > >> > >> > > Looks like a SPI flash controller from the example. What
> > makes it an
> > >> > >> > > MFD?
> > >> > >> >
> > >> > >> > It supports both SPI NOR and HyperFlash (CFI-compliant flash with
> > >> > >> > different bus interface).
> > >> > >>
> > >> > >> Looks like you're registering one OR the other.
> > >> > >>
> > >> > >> Why don't you just do this from DT?
> > >> > >>
> > >> > >> No reason for this to be an MFD IMHO.
> > >> > >
> > >> > >
> > >> > > okay, I will patch it back to SPI mode only.
> > >> >
> > >> > I don't think that's what Lee meant . The controller supports _both_
> > >> > modes , hence it would have the same compatible string. You just need to
> > >> > extract the mode of operation from the DT.
> > >>
> > >> HiSilicon attempted to upstream something similar, only their
> > >> controller provided NAND and NOR functionality.  They used different
> > >> compatible strings to differentiate between the varying
> > >> technologies.
> > >>
> > >> They too tried to use MFD as a means to select between them (which was
> > >> also NACKed).  Not sure what they ended up doing, but the original
> > >> submission and (half of) the conversation can be found at [0].  Some
> > >> more of the thread continues at [1].
> > >>
> > >> Hope that helps.
> > >>
> > >> [0] https://groups.google.com/forum/#!topic/fa.linux.kernel/F6i9o8sfOIw
> > >> [1] https://marc.info/?l=devicetree&m=147669165104431&w=2
> > >
> > >
> > > Hi Marek,
> > >
> > > By Jones's comments:
> > > --------------------------------------------------------------------------
> > >> From: Shunquan Lin <linshunquan1@hisilicon.com>
> > >>
> > >> This patch adds driver support for HiSilicon Flash Memory
> > >> Controller(FMC). HiSilicon FMC is a multi-functions device which
> > >> supports SPI Nor flash controller, SPI nand Flash controller and
> > >> parallel nand flash controller.
> > >
> > > MFDs are for devices which span multiple subsystems.
> >
> >    And we do! One of the subdrivers will live under drivers/spi/, the other
> > under drivers/mtd/...
> >
>
> From my point of view, I think Jones mean to MFD's subsystems are working simultaneously
> at the run-time, one period of time is working for sub-device-1 and later period of time
> is working for sub-device-2 and so on.
>
> But for RPC-IF, SPI or HF mode is decided at boot time by pins configure and later in kernel
> by dtb, RPC-IF can't switch SPI and HF mode at the run time.

> So far, Jones seems don't agree RPC-IF to MFD and then RPC MFD will not applied
> to mfd tree by him !

There's precedence for such constructs being an MFD: please see
drivers/mfd/at91-usart.c, which registers a single MFD cell for either
serial or SPI.

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven May 13, 2019, 2:43 p.m. UTC | #10
Hi Mason,

Note that if you send multipart/text+html emails, they will be dropped silently
by most Linux mailing lists.
Hence I'm quoting your last email fully, to give other people a chance
reading it (and commenting).

On Mon, May 13, 2019 at 11:37 AM <masonccyang@mxic.com.tw> wrote:
> > > > [...]
> > > > >> > >> > On 4/24/19 11:23 PM, Rob Herring wrote:
> > > > >> > >> > > On Wed, Apr 24, 2019 at 03:55:36PM +0800, Mason Yang wrote:
> > > > >> > >> > >> Document the bindings used by the Renesas R-Car
> > Gen3 RPC-IF MFD.
> > > > >> > >> > >>
> > > > >> > >> > >> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> > > > >> > >> > >> ---
> > > > >> > >> > >>  .../devicetree/bindings/mfd/mfd-renesas-rpc.txt
> > | 40 ++++++
> > > > >> > >> ++++++++++++++++
> > > > >> > >> > >>  1 file changed, 40 insertions(+)
> > > > >> > >> > >>  create mode 100644 Documentation/devicetree/
> > bindings/mfd/mfd-
> > > > >> > >> renesas-rpc.txt
> > > > >> > >> > >>
> > > > >> > >> > >> diff --git a/Documentation/devicetree/bindings/mfd/
> > mfd-renesas-
> > > > >> > >> rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-
> > renesas-rpc.txt
> > > > >> > >> > >> new file mode 100644
> > > > >> > >> > >> index 0000000..668b822
> > > > >> > >> > >> --- /dev/null
> > > > >> > >> > >> +++ b/Documentation/devicetree/bindings/mfd/mfd-
> > renesas-rpc.txt
> > > > >> > >> > >> @@ -0,0 +1,40 @@
> > > > >> > >> > >> +Renesas R-Car Gen3 RPC-IF MFD Device Tree Bindings
> > > > >> > >> > >> +--------------------------------------------------
> > > > >> > >> > >
> > > > >> > >> > > Looks like a SPI flash controller from the example. What
> > > > makes it an
> > > > >> > >> > > MFD?
> > > > >> > >> >
> > > > >> > >> > It supports both SPI NOR and HyperFlash (CFI-compliantflash with
> > > > >> > >> > different bus interface).
> > > > >> > >>
> > > > >> > >> Looks like you're registering one OR the other.
> > > > >> > >>
> > > > >> > >> Why don't you just do this from DT?
> > > > >> > >>
> > > > >> > >> No reason for this to be an MFD IMHO.
> > > > >> > >
> > > > >> > >
> > > > >> > > okay, I will patch it back to SPI mode only.
> > > > >> >
> > > > >> > I don't think that's what Lee meant . The controller supports _both_
> > > > >> > modes , hence it would have the same compatible string. You
> > just need to
> > > > >> > extract the mode of operation from the DT.
> > > > >>
> > > > >> HiSilicon attempted to upstream something similar, only their
> > > > >> controller provided NAND and NOR functionality.  They used different
> > > > >> compatible strings to differentiate between the varying
> > > > >> technologies.
> > > > >>
> > > > >> They too tried to use MFD as a means to select between them (which was
> > > > >> also NACKed).  Not sure what they ended up doing, but the original
> > > > >> submission and (half of) the conversation can be found at [0].  Some
> > > > >> more of the thread continues at [1].
> > > > >>
> > > > >> Hope that helps.
> > > > >>
> > > > >> [0] https://groups.google.com/forum/#!topic/fa.linux.kernel/F6i9o8sfOIw
> > > > >> [1] https://marc.info/?l=devicetree&m=147669165104431&w=2
> > > > >
> > > > >
> > > > > Hi Marek,
> > > > >
> > > > > By Jones's comments:
> > > > >
> > --------------------------------------------------------------------------
> > > > >> From: Shunquan Lin <linshunquan1@hisilicon.com>
> > > > >>
> > > > >> This patch adds driver support for HiSilicon Flash Memory
> > > > >> Controller(FMC). HiSilicon FMC is a multi-functions device which
> > > > >> supports SPI Nor flash controller, SPI nand Flash controller and
> > > > >> parallel nand flash controller.
> > > > >
> > > > > MFDs are for devices which span multiple subsystems.
> > > >
> > > >    And we do! One of the subdrivers will live under drivers/
> > spi/, the other
> > > > under drivers/mtd/...
> > > >
> > >
> > > From my point of view, I think Jones mean to MFD's subsystems are
> > working simultaneously
> > > at the run-time, one period of time is working for sub-device-1
> > and later period of time
> > > is working for sub-device-2 and so on.
> > >
> > > But for RPC-IF, SPI or HF mode is decided at boot time by pins
> > configure and later in kernel
> > > by dtb, RPC-IF can't switch SPI and HF mode at the run time.
> >
> > > So far, Jones seems don't agree RPC-IF to MFD and then RPC MFD
> > will not applied
> > > to mfd tree by him !
> >
> > There's precedence for such constructs being an MFD: please see
> > drivers/mfd/at91-usart.c, which registers a single MFD cell for either
> > serial or SPI.
>
> okay, many thanks for your information.
>
> How about to patch RPF-IF dts to:
> -------------------------------------------------------------->
>
> Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
> ---------------------------------------------------------
>
>   RPC-IF supports both SPI NOR and HyperFlash (CFI-compliant flash)
>
>   Required properties:
>   - compatible: should be an SoC-specific compatible value, followed by
>                   "renesas,rcar-gen3-rpc" as a fallback.
>                   supported SoC-specific values are:
>                   "renesas,r8a77995-rpc"  (R-Car D3)
>   - reg: should contain three register areas:
>           first for the base address of RPC-IF registers,
>           second for the direct mapping read mode and
>           third for the write buffer area.
>   - reg-names: should contain "regs", "dirmap" and "wbuf"
>   - clocks: should contain 1 entries for the module's clock
>   - clock-names: should contain "rpc"
>
>   Example:
>   - SPI mode:
>
>           rpc: rpc-if@ee200000 {
>                   compatible = "renesas,r8a77995-rpc", "renesas,rcar-gen3-rpc";
>                   reg = <0 0xee200000 0 0x200>, <0 0x08000000 0 0x4000000>,
>                         <0 0xee208000 0 0x100>;
>                   reg-names = "regs", "dirmap", "wbuf";
>                   clocks = <&cpg CPG_MOD 917>;
>                   clock-names = "rpc";
>                   power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
>                   resets = <&cpg 917>;
>                   #address-cells = <1>;
>                   #size-cells = <0>;
>
>                   flash@0 {
>                           compatible = "jedec,spi-nor";
>                           reg = <0>;
>                           spi-max-frequency = <40000000>;
>                           spi-tx-bus-width = <1>;
>                           spi-rx-bus-width = <1>;
>                   };
>           };
>
>   - HF mode:
>           rpc: rpc-if@ee200000 {
>                   compatible = "renesas,r8a77995-rpc", "renesas,rcar-gen3-rpc";
>                   reg = <0 0xee200000 0 0x200>, <0 0x08000000 0 0x4000000>,
>                         <0 0xee208000 0 0x100>;
>                   reg-names = "regs", "dirmap", "wbuf";
>                   clocks = <&cpg CPG_MOD 917>;
>                   clock-names = "rpc";
>                   power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
>                   resets = <&cpg 917>;
>                   #address-cells = <1>;
>                   #size-cells = <1>;
>
>                   flash@0 {
>                           compatible = "cfi-flash";
>                           reg = <0 0x4000000>;
>                   };
>           };
>
> --------------------------------------------------------------<
>
> Is it OK ?

I'll defer to the MTD experts.

Gr{oetje,eeting}s,

                        Geert
Sergei Shtylyov May 13, 2019, 7:08 p.m. UTC | #11
Hello!

On 05/13/2019 12:37 PM, masonccyang@mxic.com.tw wrote:

>> > > [...]
>> > > >> > >> > On 4/24/19 11:23 PM, Rob Herring wrote:
>> > > >> > >> > > On Wed, Apr 24, 2019 at 03:55:36PM +0800, Mason Yang wrote:
>> > > >> > >> > >> Document the bindings used by the Renesas R-Car
>> Gen3 RPC-IF MFD.
>> > > >> > >> > >>
>> > > >> > >> > >> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
>> > > >> > >> > >> ---
>> > > >> > >> > >>  .../devicetree/bindings/mfd/mfd-renesas-rpc.txt  
>> | 40 ++++++
>> > > >> > >> ++++++++++++++++
>> > > >> > >> > >>  1 file changed, 40 insertions(+)
>> > > >> > >> > >>  create mode 100644 Documentation/devicetree/
>> bindings/mfd/mfd-
>> > > >> > >> renesas-rpc.txt
>> > > >> > >> > >>
>> > > >> > >> > >> diff --git a/Documentation/devicetree/bindings/mfd/
>> mfd-renesas-
>> > > >> > >> rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-
>> renesas-rpc.txt
>> > > >> > >> > >> new file mode 100644
>> > > >> > >> > >> index 0000000..668b822
>> > > >> > >> > >> --- /dev/null
>> > > >> > >> > >> +++ b/Documentation/devicetree/bindings/mfd/mfd-
>> renesas-rpc.txt
>> > > >> > >> > >> @@ -0,0 +1,40 @@
>> > > >> > >> > >> +Renesas R-Car Gen3 RPC-IF MFD Device Tree Bindings
>> > > >> > >> > >> +--------------------------------------------------
>> > > >> > >> > >
>> > > >> > >> > > Looks like a SPI flash controller from the example. What
>> > > makes it an
>> > > >> > >> > > MFD?
>> > > >> > >> >
>> > > >> > >> > It supports both SPI NOR and HyperFlash (CFI-compliantflash with
>> > > >> > >> > different bus interface).
>> > > >> > >>
>> > > >> > >> Looks like you're registering one OR the other.
>> > > >> > >>
>> > > >> > >> Why don't you just do this from DT?
>> > > >> > >>
>> > > >> > >> No reason for this to be an MFD IMHO.
>> > > >> > >
>> > > >> > >
>> > > >> > > okay, I will patch it back to SPI mode only.
>> > > >> >
>> > > >> > I don't think that's what Lee meant . The controller supports _both_
>> > > >> > modes , hence it would have the same compatible string. You
>> just need to
>> > > >> > extract the mode of operation from the DT.
>> > > >>
>> > > >> HiSilicon attempted to upstream something similar, only their
>> > > >> controller provided NAND and NOR functionality.  They used different
>> > > >> compatible strings to differentiate between the varying
>> > > >> technologies.
>> > > >>
>> > > >> They too tried to use MFD as a means to select between them (which was
>> > > >> also NACKed).  Not sure what they ended up doing, but the original
>> > > >> submission and (half of) the conversation can be found at [0].  Some
>> > > >> more of the thread continues at [1].
>> > > >>
>> > > >> Hope that helps.
>> > > >>
>> > > >> [0] https://groups.google.com/forum/#!topic/fa.linux.kernel/F6i9o8sfOIw
>> > > >> [1] https://marc.info/?l=devicetree&m=147669165104431&w=2
>> > > >
>> > > >
>> > > > Hi Marek,
>> > > >
>> > > > By Jones's comments:
>> > > >
>> --------------------------------------------------------------------------
>> > > >> From: Shunquan Lin <linshunquan1@hisilicon.com>
>> > > >>
>> > > >> This patch adds driver support for HiSilicon Flash Memory
>> > > >> Controller(FMC). HiSilicon FMC is a multi-functions device which
>> > > >> supports SPI Nor flash controller, SPI nand Flash controller and
>> > > >> parallel nand flash controller.
>> > > >
>> > > > MFDs are for devices which span multiple subsystems.
>> > >
>> > >    And we do! One of the subdrivers will live under drivers/
>> spi/, the other
>> > > under drivers/mtd/...
>> > >
>> >
>> > From my point of view, I think Jones mean to MFD's subsystems are
>> working simultaneously
>> > at the run-time, one period of time is working for sub-device-1
>> and later period of time
>> > is working for sub-device-2 and so on.
>> >
>> > But for RPC-IF, SPI or HF mode is decided at boot time by pins
>> configure and later in kernel
>> > by dtb, RPC-IF can't switch SPI and HF mode at the run time.
>>
>> > So far, Jones seems don't agree RPC-IF to MFD and then RPC MFD
>> will not applied
>> > to mfd tree by him !
>>
>> There's precedence for such constructs being an MFD: please see
>> drivers/mfd/at91-usart.c, which registers a single MFD cell for either
>> serial or SPI.

   Thanks fir your example, Geert! :-)

> okay, many thanks for your information.
> 
> How about to patch RPF-IF dts to:
> -------------------------------------------------------------->
> 
> Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
> ---------------------------------------------------------
>  
>   RPC-IF supports both SPI NOR and HyperFlash (CFI-compliant flash)
>  
>   Required properties:
>   - compatible: should be an SoC-specific compatible value, followed by
>                   "renesas,rcar-gen3-rpc" as a fallback.
>                   supported SoC-specific values are:
>                   "renesas,r8a77995-rpc"  (R-Car D3)
>   - reg: should contain three register areas:
>           first for the base address of RPC-IF registers,

   I'd drop "the base address" here.

>           second for the direct mapping read mode and
>           third for the write buffer area.
>   - reg-names: should contain "regs", "dirmap" and "wbuf"
>   - clocks: should contain 1 entries for the module's clock
>   - clock-names: should contain "rpc"

   I suspect we'd need the RPC/RPCD2 clocks mentioned as well (not sure yet)...
   And how about "power-domains", "resets" (seen in the example below),
also what about #address-cells & #size-cells?

>  
>   Example:

   Could you please indent with 1 or 2 tabs where you used 8 or 16 spaces?

>   - SPI mode:
>  
>           rpc: rpc-if@ee200000 {

   The node names should be generic, based on the device class. And in this
case I'd like to use "spi@ee200000" as otherwise dtc keeps bitching like below:

arch/arm64/boot/dts/renesas/r8a77980.dtsi:1344.21-1359.5: Warning (spi_bus_bridge):
/soc/rpc@ee200000: node name for SPI buses should be 'spi'
  also defined at arch/arm64/boot/dts/renesas/r8a77980-condor.dts:283.6-343.3
arch/arm64/boot/dts/renesas/r8a77980-condor.dtb: Warning (spi_bus_reg):
Failed prerequisite 'spi_bus_bridge'


>   - HF mode:
>           rpc: rpc-if@ee200000 {

   Again, spi@<...>.

>                   compatible = "renesas,r8a77995-rpc", "renesas,rcar-gen3-rpc";
>                   reg = <0 0xee200000 0 0x200>, <0 0x08000000 0 0x4000000>,
>                         <0 0xee208000 0 0x100>;
>                   reg-names = "regs", "dirmap", "wbuf";
>                   clocks = <&cpg CPG_MOD 917>;
>                   clock-names = "rpc";
>                   power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
>                   resets = <&cpg 917>;
>                   #address-cells = <1>;
>                   #size-cells = <1>;
>  
>                   flash@0 {
>                           compatible = "cfi-flash";

   The working HF implementation has "cypress,hyperflash" before "cfi-flash".

>                           reg = <0 0x4000000>;
>                   };
>           };
> 
> --------------------------------------------------------------<
> 
> Is it OK ?

   Yeah, seems good (assuming you fix the issues above).

[...]
> thanks & best regards,
> Mason

MBR, Sergei
Mason Yang May 14, 2019, 2:33 a.m. UTC | #12
Hi Geert,

> Subject
> 
> Re: [PATCH v12 3/3] dt-bindings: mfd: Document Renesas R-Car Gen3 RPC-IF 
MFD bindings
> 
> Hi Mason,
> 
> Note that if you send multipart/text+html emails, they will be dropped 
silently
> by most Linux mailing lists.
> Hence I'm quoting your last email fully, to give other people a chance
> reading it (and commenting).

Thanks for your remind !
I have configured my Lotus Notes to plain text mode only and it should be 
OK
for Linux mailing lists now.

thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Mason Yang May 14, 2019, 9:46 a.m. UTC | #13
Hello,


> >> There's precedence for such constructs being an MFD: please see
> >> drivers/mfd/at91-usart.c, which registers a single MFD cell for 
either
> >> serial or SPI.
> 
>    Thanks fir your example, Geert! :-)
> 
> > okay, many thanks for your information.
> > 
> > How about to patch RPF-IF dts to:
> > -------------------------------------------------------------->
> > 
> > Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
> > ---------------------------------------------------------
> > 
> >   RPC-IF supports both SPI NOR and HyperFlash (CFI-compliant flash)
> > 
> >   Required properties:
> >   - compatible: should be an SoC-specific compatible value, followed 
by
> >                   "renesas,rcar-gen3-rpc" as a fallback.
> >                   supported SoC-specific values are:
> >                   "renesas,r8a77995-rpc"  (R-Car D3)
> >   - reg: should contain three register areas:
> >           first for the base address of RPC-IF registers,
> 
>    I'd drop "the base address" here.

okay.

> 
> >           second for the direct mapping read mode and
> >           third for the write buffer area.
> >   - reg-names: should contain "regs", "dirmap" and "wbuf"
> >   - clocks: should contain 1 entries for the module's clock
> >   - clock-names: should contain "rpc"
> 
>    I suspect we'd need the RPC/RPCD2 clocks mentioned as well (not sure 
yet)...

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


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

okay, patch to

rpc_if: spi@<...>

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

what about rpc_if: hf@<...>


> 
> >                   compatible = "renesas,r8a77995-rpc", 
"renesas,rcar-gen3-rpc";
> >                   reg = <0 0xee200000 0 0x200>, <0 0x08000000 0 
0x4000000>,
> >                         <0 0xee208000 0 0x100>;
> >                   reg-names = "regs", "dirmap", "wbuf";
> >                   clocks = <&cpg CPG_MOD 917>;
> >                   clock-names = "rpc";
> >                   power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
> >                   resets = <&cpg 917>;
> >                   #address-cells = <1>;
> >                   #size-cells = <1>;
> > 
> >                   flash@0 {
> >                           compatible = "cfi-flash";
> 
>    The working HF implementation has "cypress,hyperflash" before 
"cfi-flash".
> 
> >                           reg = <0 0x4000000>;
> >                   };
> >           };
> > 
> > --------------------------------------------------------------<
> > 
> > Is it OK ?
> 
>    Yeah, seems good (assuming you fix the issues above).

Patch new DTS to
===============================================================> 

+Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
+---------------------------------------------------------
+
+RPC-IF supports both SPI NOR and HyperFlash (CFI-compliant flash)
+
+Required properties:
+- compatible: should be an SoC-specific compatible value, followed by
+                                "renesas,rcar-gen3-rpc" as a fallback.
+                                supported SoC-specific values are:
+                                "renesas,r8a77995-rpc"          (R-Car 
D3)
+- reg: should contain three register areas:
+                first for RPC-IF registers,
+                second for the direct mapping read mode and
+                third for the write buffer area.
+- reg-names: should contain "regs", "dirmap" and "wbuf"
+- clocks: should contain 1 entries for the module's clock
+- clock-names: should contain "rpc"
+- #address-cells: should be 1
+- #size-cells: should be 0
+
+Example:
+- SPI mode:
+
+                rpc_if: spi@ee200000 {
+                                compatible = "renesas,r8a77995-rpc", 
"renesas,rcar-gen3-rpc";
+                                reg = <0 0xee200000 0 0x200>, <0 
0x08000000 0 0x4000000>,
+                                      <0 0xee208000 0 0x100>;
+                                reg-names = "regs", "dirmap", "wbuf";
+                                clocks = <&cpg CPG_MOD 917>;
+                                clock-names = "rpc";
+                                power-domains = <&sysc 
R8A77995_PD_ALWAYS_ON>;
+                                resets = <&cpg 917>;
+                                #address-cells = <1>;
+                                #size-cells = <0>;
+
+                                flash@0 {
+                                                compatible = 
"jedec,spi-nor";
+                                                reg = <0>;
+                                                spi-max-frequency = 
<40000000>;
+                                                spi-tx-bus-width = <1>;
+                                                spi-rx-bus-width = <1>;
+                                };
+                };
+
+- HF mode:
+                rpc_if: hf@ee200000 {
+                                compatible = "renesas,r8a77995-rpc", 
"renesas,rcar-gen3-rpc";
+                                reg = <0 0xee200000 0 0x200>, <0 
0x08000000 0 0x4000000>,
+                                      <0 0xee208000 0 0x100>;
+                                reg-names = "regs", "dirmap", "wbuf";
+                                clocks = <&cpg CPG_MOD 917>;
+                                clock-names = "rpc";
+                                power-domains = <&sysc 
R8A77995_PD_ALWAYS_ON>;
+                                resets = <&cpg 917>;
+                                #address-cells = <1>;
+                                #size-cells = <0>;
+
+                                flash@0 {
+                                                compatible = 
"cypress,hyperflash", "cfi-flash";
+                                                reg = <0>;
+                                };
+                };

=======================================================================<

OK ?


thanks & best regards,
Mason



CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Mason Yang May 14, 2019, 10 a.m. UTC | #14
Hi Sergei, 

> > 
> >    Could you please indent with 1 or 2 tabs where you used 8 or 16 
spaces?

Sorry, I just copy this new DTS from git to my Notes email system,
therefore, the "tabs space" is not alignment with original file.

> 
> Patch new DTS to
> ===============================================================> 
> 
> +Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
> +---------------------------------------------------------
> +
> +RPC-IF supports both SPI NOR and HyperFlash (CFI-compliant flash)
> +
> +Required properties:
> +- compatible: should be an SoC-specific compatible value, followed by
> +      "renesas,rcar-gen3-rpc" as a fallback.
> +      supported SoC-specific values are:
> +      "renesas,r8a77995-rpc"   (R-Car D3)
> +- reg: should contain three register areas:
> +   first for RPC-IF registers,
> +   second for the direct mapping read mode and
> +   third for the write buffer area.
> +- reg-names: should contain "regs", "dirmap" and "wbuf"
> +- clocks: should contain 1 entries for the module's clock
> +- clock-names: should contain "rpc"
> +- #address-cells: should be 1
> +- #size-cells: should be 0
> +
> +Example:
> +- SPI mode:
> +
> +   rpc_if: spi@ee200000 {
> +      compatible = "renesas,r8a77995-rpc", "renesas,rcar-gen3-rpc";
> +      reg = <0 0xee200000 0 0x200>, <0 0x08000000 0 0x4000000>,
> +            <0 0xee208000 0 0x100>;
> +      reg-names = "regs", "dirmap", "wbuf";
> +      clocks = <&cpg CPG_MOD 917>;
> +      clock-names = "rpc";
> +      power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
> +      resets = <&cpg 917>;
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      flash@0 {
> +         compatible = "jedec,spi-nor";
> +         reg = <0>;
> +         spi-max-frequency = <40000000>;
> +         spi-tx-bus-width = <1>;
> +         spi-rx-bus-width = <1>;
> +      };
> +   };
> +
> +- HF mode:
> +   rpc_if: hf@ee200000 {
> +      compatible = "renesas,r8a77995-rpc", "renesas,rcar-gen3-rpc";
> +      reg = <0 0xee200000 0 0x200>, <0 0x08000000 0 0x4000000>,
> +            <0 0xee208000 0 0x100>;
> +      reg-names = "regs", "dirmap", "wbuf";
> +      clocks = <&cpg CPG_MOD 917>;
> +      clock-names = "rpc";
> +      power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
> +      resets = <&cpg 917>;
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      flash@0 {
> +         compatible = "cypress,hyperflash", "cfi-flash";
> +         reg = <0>;
> +      };
> +   };
> 
> =======================================================================<

thanks for your review.

best regards,
Mason 



CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Geert Uytterhoeven May 14, 2019, 10:07 a.m. UTC | #15
Hi Mason,

On Tue, May 14, 2019 at 11:46 AM <masonccyang@mxic.com.tw> wrote:
> > >> There's precedence for such constructs being an MFD: please see
> > >> drivers/mfd/at91-usart.c, which registers a single MFD cell for
> > >           second for the direct mapping read mode and
> > >           third for the write buffer area.
> > >   - reg-names: should contain "regs", "dirmap" and "wbuf"
> > >   - clocks: should contain 1 entries for the module's clock
> > >   - clock-names: should contain "rpc"
> >
> >    I suspect we'd need the RPC/RPCD2 clocks mentioned as well (not sure
> yet)...
>
> Need it ?
> RPCD2 is derived from RPC and it's value is half of RPC,
> i.e., RPC = 160MHz, RPCD2 = 80 MHz

That hierarchy depends on the actual SoC, right?
AFAIU, both are derived from RPCSRC, and can be controlled individually
on some SoCs.

BTW, I still haven't seen a patch for drivers/clk/renesas/r8a77995-cpg-mssr.c
adding RPC-related clocks, so I have no idea what's the Linux view of the
hierarchy on your system.

Thanks!

Gr{oetje,eeting}s,

                        Geert
Sergei Shtylyov May 14, 2019, 8:27 p.m. UTC | #16
On 05/14/2019 12:46 PM, masonccyang@mxic.com.tw wrote:

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

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

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

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

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

   I know.

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

   That, or just keep the node label.

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

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

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

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

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

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

[...]

MBR, Sergei
Mason Yang May 20, 2019, 7:23 a.m. UTC | #17
Hi Sergei, 

> >>> -------------------------------------------------------------->
> >>>
> >>> Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
> >>> ---------------------------------------------------------
> >>>
> >>>   RPC-IF supports both SPI NOR and HyperFlash (CFI-compliant flash)
> >>>
> >>>   Required properties:
> >>>   - compatible: should be an SoC-specific compatible value, followed 

> > by
> >>>                   "renesas,rcar-gen3-rpc" as a fallback.
> >>>                   supported SoC-specific values are:
> >>>                   "renesas,r8a77995-rpc"  (R-Car D3)
> >>>   - reg: should contain three register areas:
> >>>           first for the base address of RPC-IF registers,
> >>
> >>    I'd drop "the base address" here.
> > 
> > okay.
> > 
> >>>           second for the direct mapping read mode and
> >>>           third for the write buffer area.
> >>>   - reg-names: should contain "regs", "dirmap" and "wbuf"
> >>>   - clocks: should contain 1 entries for the module's clock
> >>>   - clock-names: should contain "rpc"
> >>
> >>    I suspect we'd need the RPC/RPCD2 clocks mentioned as well (not 
sure 
> > yet)...
> > 
> > Need it ?
> 
>    You seem to call clk_get_rate() on the module clock, I doubt that's
> correct topologically...

I think it's correct but just like Geert mentioned that there is no any 
patch
in drivers/clk/renesas/r8a77995-cpg-mssr.c adding RPC-related clocks.


I patched dt-bindings/clock/r8a77995-cpg-mssr.h for some simple testing

-#define R8A77995_CLK_RPC               29
-#define R8A77995_CLK_RPCD2             30
+#define R8A77995_CLK_RPC               31
+#define R8A77995_CLK_RPCD2             32

by clk_prepare_enable() & clk_disable_unprepare() with CPG_MOD 917 
on D3 draak board, it is working fine.


> >>
> >>>   - SPI mode:git
> >>>
> >>>           rpc: rpc-if@ee200000 {
> >>
> >>    The node names should be generic, based on the device class. And 
in 
> > this
> >> case I'd like to use "spi@ee200000" as otherwise dtc keeps bitching 
like 
> > below:
> > 
> > okay, patch to
> > 
> > rpc_if: spi@<...>
> 
>    That, or just keep the node label.

okay.


> >>>   - HF mode:
> >>>           rpc: rpc-if@ee200000 {
> >>
> >>    Again, spi@<...>.
> > 
> > what about rpc_if: hf@<...>
> 
>    Can't change the node name, as it's declared in the .dtsi files, not 
*.dts
> ones. And "spi" works for the HF case as well -- no complaints from dtc. 
:-)

okay,

 
Patch DTS to
===============================================================> 
+Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
+---------------------------------------------------------
+
+RPC-IF supports both SPI NOR and HyperFlash (CFI-compliant flash)
+
+Required properties:
+- compatible: should be an SoC-specific compatible value, followed by
+                                "renesas,rcar-gen3-rpc" as a fallback.
+                                supported SoC-specific values are:
+                                "renesas,r8a77995-rpc"          (R-Car 
D3)
+- reg: should contain three register areas:
+                first for RPC-IF registers,
+                second for the direct mapping read mode and
+                third for the write buffer area.
+- reg-names: should contain "regs", "dirmap" and "wbuf"
+- clocks: should contain 1 entries for the module's clock
+- clock-names: should contain "rpc"
+- power-domains: should contain system-controller(sysc) for 
power-domain-cell
+- resets: should contain clock pulse generator(cpg) for reset-cell,
+                  power-domain-cell and clock-cell
+- #address-cells: should be 1
+- #size-cells: should be 0
+
+Example:
+- SPI mode:
+
+                rpc: spi@ee200000 {
+                                compatible = "renesas,r8a77995-rpc", 
"renesas,rcar-gen3-rpc";
+                                reg = <0 0xee200000 0 0x200>, <0 
0x08000000 0 0x4000000>,
+                                      <0 0xee208000 0 0x100>;
+                                reg-names = "regs", "dirmap", "wbuf";
+                                clocks = <&cpg CPG_MOD 917>;
+                                clock-names = "rpc";
+                                power-domains = <&sysc 
R8A77995_PD_ALWAYS_ON>;
+                                resets = <&cpg 917>;
+                                #address-cells = <1>;
+                                #size-cells = <0>;
+
+                                flash@0 {
+                                                compatible = 
"jedec,spi-nor";
+                                                reg = <0>;
+                                                spi-max-frequency = 
<40000000>;
+                                                spi-tx-bus-width = <1>;
+                                                spi-rx-bus-width = <1>;
+                                };
+                };
+
+- HF mode:
+                rpc: spi@ee200000 {
+                                compatible = "renesas,r8a77995-rpc", 
"renesas,rcar-gen3-rpc";
+                                reg = <0 0xee200000 0 0x200>, <0 
0x08000000 0 0x4000000>,
+                                      <0 0xee208000 0 0x100>;
+                                reg-names = "regs", "dirmap", "wbuf";
+                                clocks = <&cpg CPG_MOD 917>;
+                                clock-names = "rpc";
+                                power-domains = <&sysc 
R8A77995_PD_ALWAYS_ON>;
+                                resets = <&cpg 917>;
+                                #address-cells = <1>;
+                                #size-cells = <0>;
+
+                                flash@0 {
+                                                compatible = 
"cypress,hyperflash", "cfi-flash";
+                                                reg = <0>;
+                                };
+                };
===============================================================<

thanks & best regards,
Mason


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Geert Uytterhoeven May 20, 2019, 7:44 a.m. UTC | #18
Hi Mason,

On Mon, May 20, 2019 at 9:24 AM <masonccyang@mxic.com.tw> wrote:
> > >>>   - clocks: should contain 1 entries for the module's clock
> > >>>   - clock-names: should contain "rpc"
> > >>
> > >>    I suspect we'd need the RPC/RPCD2 clocks mentioned as well (not
> sure
> > > yet)...
> > >
> > > Need it ?
> >
> >    You seem to call clk_get_rate() on the module clock, I doubt that's
> > correct topologically...
>
> I think it's correct but just like Geert mentioned that there is no any
> patch
> in drivers/clk/renesas/r8a77995-cpg-mssr.c adding RPC-related clocks.
>
>
> I patched dt-bindings/clock/r8a77995-cpg-mssr.h for some simple testing
>
> -#define R8A77995_CLK_RPC               29
> -#define R8A77995_CLK_RPCD2             30
> +#define R8A77995_CLK_RPC               31
> +#define R8A77995_CLK_RPCD2             32

That change doesn't make sense to me...

> by clk_prepare_enable() & clk_disable_unprepare() with CPG_MOD 917
> on D3 draak board, it is working fine.

... and is not sufficient to allow the above two calls.

Besides, making explicit clk_prepare_enable() calls bypasses Runtime PM
and the automatic disabling of unused clocks, thus hiding bugs related to
Runtime PM.  Which is probably why your driver doesn't work for Sergei...


Gr{oetje,eeting}s,

                        Geert
Mason Yang May 21, 2019, 1:28 a.m. UTC | #19
Hi Geert,

> 
> On Mon, May 20, 2019 at 9:24 AM <masonccyang@mxic.com.tw> wrote:
> > > >>>   - clocks: should contain 1 entries for the module's clock
> > > >>>   - clock-names: should contain "rpc"
> > > >>
> > > >>    I suspect we'd need the RPC/RPCD2 clocks mentioned as well 
(not
> > sure
> > > > yet)...
> > > >
> > > > Need it ?
> > >
> > >    You seem to call clk_get_rate() on the module clock, I doubt 
that's
> > > correct topologically...
> >
> > I think it's correct but just like Geert mentioned that there is no 
any
> > patch
> > in drivers/clk/renesas/r8a77995-cpg-mssr.c adding RPC-related clocks.
> >
> >
> > I patched dt-bindings/clock/r8a77995-cpg-mssr.h for some simple 
testing
> >
> > -#define R8A77995_CLK_RPC               29
> > -#define R8A77995_CLK_RPCD2             30
> > +#define R8A77995_CLK_RPC               31
> > +#define R8A77995_CLK_RPCD2             32
> 
> That change doesn't make sense to me...
> 
> > by clk_prepare_enable() & clk_disable_unprepare() with CPG_MOD 917
> > on D3 draak board, it is working fine.
> 
> ... and is not sufficient to allow the above two calls.
> 
> Besides, making explicit clk_prepare_enable() calls bypasses Runtime PM
> and the automatic disabling of unused clocks, thus hiding bugs related 
to
> Runtime PM.  Which is probably why your driver doesn't work for 
Sergei...
> 

Got it, thanks for your explanation.

best regards,
Mason


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Sergei Shtylyov May 22, 2019, 4:32 p.m. UTC | #20
On 05/20/2019 10:23 AM, masonccyang@mxic.com.tw wrote:

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

   clk_set_rate(), sorry.

> 
> I think it's correct but just like Geert mentioned that there is no any 
> patch
> in drivers/clk/renesas/r8a77995-cpg-mssr.c adding RPC-related clocks.
> 
> 
> I patched dt-bindings/clock/r8a77995-cpg-mssr.h for some simple testing
> 
> -#define R8A77995_CLK_RPC               29
> -#define R8A77995_CLK_RPCD2             30
> +#define R8A77995_CLK_RPC               31
> +#define R8A77995_CLK_RPCD2             32

   Hm, what does this do?

> by clk_prepare_enable() & clk_disable_unprepare() with CPG_MOD 917 
> on D3 draak board, it is working fine.

>>>>>   - SPI mode:git
>>>>>
>>>>>           rpc: rpc-if@ee200000 {
>>>>
>>>>    The node names should be generic, based on the device class. And in 
>>>> this
>>>> case I'd like to use "spi@ee200000" as otherwise dtc keeps bitching like 
>>>> below:
>>>
>>> okay, patch to
>>>
>>> rpc_if: spi@<...>
>>
>>    That, or just keep the node label.
> 
> okay.
> 
>>>>>   - HF mode:
>>>>>           rpc: rpc-if@ee200000 {
>>>>
>>>>    Again, spi@<...>.
>>>
>>> what about rpc_if: hf@<...>
>>
>>    Can't change the node name, as it's declared in the .dtsi files, not *.dts
>> ones. And "spi" works for the HF case as well -- no complaints from dtc. 
> :-)

   Maybe it's possible using the "name" prop, don't know...

> okay,
>  
> Patch DTS to
> ===============================================================> 
> +Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
> +---------------------------------------------------------
> +
> +RPC-IF supports both SPI NOR and HyperFlash (CFI-compliant flash)
> +
> +Required properties:
> +- compatible: should be an SoC-specific compatible value, followed by
> +                                "renesas,rcar-gen3-rpc" as a fallback.
> +                                supported SoC-specific values are:
> +                                "renesas,r8a77995-rpc"          (R-Car 
> D3)
> +- reg: should contain three register areas:
> +                first for RPC-IF registers,
> +                second for the direct mapping read mode and
> +                third for the write buffer area.
> +- reg-names: should contain "regs", "dirmap" and "wbuf"
> +- clocks: should contain 1 entries for the module's clock

   1 entry (clock node phandle and specifier).

> +- clock-names: should contain "rpc"
> +- power-domains: should contain system-controller(sysc) for 
> power-domain-cell

   What's "power-domain-cell"? I know "#power-domain-cells". I'd like this
to be "the power domain node's phandle and specifier".

> +- resets: should contain clock pulse generator(cpg) for reset-cell,

   It's "#reset-cells" again. I'd like this to be "the reset node's phandle
and specifier".

> +                  power-domain-cell and clock-cell

   Why mntion clock-cell at all here?

> +- #address-cells: should be 1
> +- #size-cells: should be 0
> +
> +Example:
> +- SPI mode:
> +
> +                rpc: spi@ee200000 {
> +                                compatible = "renesas,r8a77995-rpc", 
> "renesas,rcar-gen3-rpc";
> +                                reg = <0 0xee200000 0 0x200>, <0 
> 0x08000000 0 0x4000000>,
> +                                      <0 0xee208000 0 0x100>;
> +                                reg-names = "regs", "dirmap", "wbuf";
> +                                clocks = <&cpg CPG_MOD 917>;
> +                                clock-names = "rpc";
> +                                power-domains = <&sysc 
> R8A77995_PD_ALWAYS_ON>;
> +                                resets = <&cpg 917>;
> +                                #address-cells = <1>;
> +                                #size-cells = <0>;
> +
> +                                flash@0 {
> +                                                compatible = 
> "jedec,spi-nor";
> +                                                reg = <0>;
> +                                                spi-max-frequency = 
> <40000000>;
> +                                                spi-tx-bus-width = <1>;
> +                                                spi-rx-bus-width = <1>;
> +                                };
> +                };
> +
> +- HF mode:

   HyperFlash, please.

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

   And I don't think duplicating the same device node is a good idea...

> +                                flash@0 {
> +                                                compatible = 
> "cypress,hyperflash", "cfi-flash";
> +                                                reg = <0>;
> +                                };
> +                };
> ===============================================================<
> 
> thanks & best regards,
> Mason
[...]

MBR, Sergei
Geert Uytterhoeven May 22, 2019, 5:05 p.m. UTC | #21
Hi Sergei,

On Wed, May 22, 2019 at 6:32 PM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 05/20/2019 10:23 AM, masonccyang@mxic.com.tw wrote:
> > +- clocks: should contain 1 entries for the module's clock
>
>    1 entry (clock node phandle and specifier).

Doesn't "specifier" mean "phandle + optional arguments"?

Gr{oetje,eeting}s,

                        Geert
Sergei Shtylyov May 22, 2019, 5:23 p.m. UTC | #22
On 05/22/2019 08:05 PM, Geert Uytterhoeven wrote:

>> On 05/20/2019 10:23 AM, masonccyang@mxic.com.tw wrote:
>>> +- clocks: should contain 1 entries for the module's clock
>>
>>    1 entry (clock node phandle and specifier).
> 
> Doesn't "specifier" mean "phandle + optional arguments"?

   No.
   E.g. when specifying the IRQs, the "interrupts" prop contains one or more
interrupt specifiers, the phandle is specified in the "interrupt-parent"
prop (see the DT spec). 

> Gr{oetje,eeting}s,
> 
>                         Geert

MBR, Sergei
Geert Uytterhoeven May 22, 2019, 5:44 p.m. UTC | #23
Hi Sergei,

On Wed, May 22, 2019 at 7:23 PM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 05/22/2019 08:05 PM, Geert Uytterhoeven wrote:
> >> On 05/20/2019 10:23 AM, masonccyang@mxic.com.tw wrote:
> >>> +- clocks: should contain 1 entries for the module's clock
> >>
> >>    1 entry (clock node phandle and specifier).
> >
> > Doesn't "specifier" mean "phandle + optional arguments"?
>
>    No.
>    E.g. when specifying the IRQs, the "interrupts" prop contains one or more
> interrupt specifiers, the phandle is specified in the "interrupt-parent"
> prop (see the DT spec).

Mea culpa, I stand corrected.

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
new file mode 100644
index 0000000..668b822
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/mfd-renesas-rpc.txt
@@ -0,0 +1,40 @@ 
+Renesas R-Car Gen3 RPC-IF MFD Device Tree Bindings
+--------------------------------------------------
+
+Required properties:
+- compatible: should be an SoC-specific compatible value, followed by
+		"renesas,rcar-gen3-rpc" as a fallback.
+		supported SoC-specific values are:
+		"renesas,r8a77995-rpc"	(R-Car D3)
+- reg: should contain three register areas:
+	first for the base address of RPC-IF registers,
+	second for the direct mapping read mode and
+	third for the write buffer area.
+- reg-names: should contain "regs", "dirmap" and "wbuf"
+- clocks: should contain 1 entries for the module's clock
+- clock-names: should contain "rpc"
+- #address-cells: should be 1
+- #size-cells: should be 0
+
+Example:
+
+	rpc: rpc-if@ee200000 {
+		compatible = "renesas,r8a77995-rpc", "renesas,rcar-gen3-rpc";
+		reg = <0 0xee200000 0 0x200>, <0 0x08000000 0 0x4000000>,
+		      <0 0xee208000 0 0x100>;
+		reg-names = "regs", "dirmap", "wbuf";
+		clocks = <&cpg CPG_MOD 917>;
+		clock-names = "rpc";
+		power-domains = <&sysc R8A77995_PD_ALWAYS_ON>;
+		resets = <&cpg 917>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		flash@0 {
+			compatible = "jedec,spi-nor";
+			reg = <0>;
+			spi-max-frequency = <40000000>;
+			spi-tx-bus-width = <1>;
+			spi-rx-bus-width = <1>;
+		};
+	};