Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH v7 4/8] drm/sunxi: Add DT bindings documentation of Allwinner HDMI
From: Jean-Francois Moine @ 2016-11-29 19:27 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Dave Airlie,
	Maxime Ripard, Rob Herring, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <5996605.Gh0PjPIAcn@avalon>

On Tue, 29 Nov 2016 20:46:22 +0200
Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org> wrote:
	[snip]
> > +Example:
> > +
> > +	hdmi: hdmi@01ee0000 {
> > +		compatible = "allwinner,sun8i-a83t-hdmi";
> > +		reg = <0x01ee0000 0x20000>;
> > +		clocks = <&ccu CLK_BUS_HDMI>, <&ccu CLK_HDMI>,
> > +			 <&ccu CLK_HDMI_DDC>;
> > +		clock-names = "bus", "clock", "ddc-clock";
> > +		resets = <&ccu RST_HDMI0>, <&ccu RST_HDMI1>;
> > +		reset-names = "hdmi0", "hdmi1";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&hdmi_pins_a>;
> > +		status = "disabled";
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		port@0 {			/* video */
> > +			reg = <0>;
> > +			hdmi_tcon1: endpoint {
> > +				remote-endpoint = <&tcon1_hdmi>;
> > +			};
> > +		};
> > +		port@1 {			/* audio */
> > +			reg = <1>;
> > +			hdmi_i2s2: endpoint {
> > +				remote-endpoint = <&i2s2_hdmi>;
> > +			};
> > +		};
> 
> You need a third port for the HDMI encoder output, connected to an HDMI 
> connector DT node.

I don't see what you mean. The HDMI device is both the encoder
and connector (as the TDA998x):

plane -> DE2 mixer ---> TCON -----> HDMI -----> display device
----- plane ------    - CRTC -   - encoder  \
                                   connector -- (HDMI cable)
         audio-controller -   - audio-codec /

> > +	};

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply

* Re: [PATCH 2/4] ARM64: dts: meson-gx: Add Graphic Controller nodes
From: Laurent Pinchart @ 2016-11-29 19:19 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: airlied-cv59FeDIM0c, khilman-rdvid1DuHRBWk0Htik3J/w,
	carlo-KA+7E9HrN00dnm+yROfE0A,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
	linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	victor.wan-LpR1jeaWuhtBDgjK7y7TUQ,
	jerry.cao-LpR1jeaWuhtBDgjK7y7TUQ, Xing.Xu-LpR1jeaWuhtBDgjK7y7TUQ,
	devicetree-u79uwXL29TY76Z2rM5mHXA, daniel-/w4YWyX8dFk
In-Reply-To: <1812737.pZP73QSyU1@avalon>

Hi Neil,

On Tuesday 29 Nov 2016 21:16:17 Laurent Pinchart wrote:
> On Tuesday 29 Nov 2016 11:47:47 Neil Armstrong wrote:
> > Add Video Processing Unit and CVBS Output nodes, and enable CVBS on
> > selected boards.
> > 
> > Signed-off-by: Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> > ---
> > 
> >  arch/arm64/boot/dts/amlogic/meson-gx.dtsi          | 46 +++++++++++++++++
> >  .../boot/dts/amlogic/meson-gxbb-nexbox-a95x.dts    |  4 ++
> >  arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi   |  4 ++
> >  arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |  8 ++++
> >  .../boot/dts/amlogic/meson-gxl-nexbox-a95x.dts     |  4 ++
> >  arch/arm64/boot/dts/amlogic/meson-gxl.dtsi         |  8 ++++
> >  .../arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts |  4 ++
> >  arch/arm64/boot/dts/amlogic/meson-gxm.dtsi         |  8 ++++
> >  8 files changed, 86 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> > b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi index fc033c0..644d5f6 100644
> > --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> > +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> > @@ -153,6 +153,27 @@
> >  		};
> >  	};
> > 
> > +	venc_cvbs: venc-cvbs {
> > +		compatible = "amlogic,meson-gx-cvbs";
> > +		status = "disabled";
> 
> Still no registers here ?

Or is this the internal video encoder, not the HHI VDAC ? If so, haven't we 
decided when discussing the previous version that there's no need for a DT 
node to model the video encoder as it's part of the VPU ?

> > +
> > +		ports {
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			venc_cvbs_in: port@0 {
> 
> Nitpicking, you don't need a label here as ports are never referenced by
> phandle. Same for the vpu node below.
> 
> > +				 #address-cells = <1>;
> > +				 #size-cells = <0>;
> > +				 reg = <0>;
> > +
> > +				 venc_cvbs_in_vpu: endpoint@0 {
> > +					 reg = <0>;
> 
> And there's no requirement to number the endpoint if there's a single one of
> them (but it's not forbidden either).
> 
> > +					 remote-endpoint =
> 
> <&vpu_out_venc_cvbs>;
> 
> > +				};
> > +			};
> > +		};
> > +	};
> > +
> > 
> >  	soc {

Shouldn't the above node be placed inside the soc ?

> >  		compatible = "simple-bus";
> >  		#address-cells = <2>;
> > 
> > @@ -356,5 +377,30 @@
> > 
> >  				status = "disabled";
> >  			
> >  			};
> >  		
> >  		};
> > 
> > +
> > +		vpu: vpu@d0100000 {
> > +			compatible = "amlogic,meson-gx-vpu";
> > +			reg = <0x0 0xd0100000 0x0 0x100000>,
> > +			      <0x0 0xc883c000 0x0 0x1000>,
> > +			      <0x0 0xc8838000 0x0 0x1000>;
> > +			reg-names = "base", "hhi", "dmc";
> > +			interrupts = <GIC_SPI 3 IRQ_TYPE_EDGE_RISING>;
> > +
> > +			ports {
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +
> > +				vpu_out: port@1 {
> > +					#address-cells = <1>;
> > +					#size-cells = <0>;
> > +					reg = <1>;
> > +
> > +					vpu_out_venc_cvbs: endpoint@0 {
> > +						reg = <0>;
> > +						remote-endpoint =
> 
> <&venc_cvbs_in_vpu>;
> 
> > +					};
> > +				};
> > +			};
> > +		};
> > 
> >  	};
> >  
> >  };
> > 
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-nexbox-a95x.dts
> > b/arch/arm64/boot/dts/amlogic/meson-gxbb-nexbox-a95x.dts index
> > 9696820..a55d1cf 100644
> > --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-nexbox-a95x.dts
> > +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-nexbox-a95x.dts
> > @@ -229,3 +229,7 @@
> > 
> >  	clocks = <&clkc CLKID_FCLK_DIV4>;
> >  	clock-names = "clkin0";
> >  
> >  };
> > 
> > +
> > +&venc_cvbs {
> > +	status = "okay";
> > +};
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
> > b/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi index 5e5e2de..3c09bd1
> > 100644
> > --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
> > +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
> > @@ -266,3 +266,7 @@
> > 
> >  	clocks = <&clkc CLKID_FCLK_DIV4>;
> >  	clock-names = "clkin0";
> >  
> >  };
> > 
> > +
> > +&venc_cvbs {
> > +	status = "okay";
> > +};
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> > b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi index ac5ad3b..1a321c8f
> > 100644
> > --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> > +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> > @@ -506,3 +506,11 @@
> > 
> >  		 <&clkc CLKID_FCLK_DIV2>;
> >  	
> >  	clock-names = "core", "clkin0", "clkin1";
> >  
> >  };
> > 
> > +
> > +&venc_cvbs {
> > +	compatible = "amlogic,meson-gxbb-cvbs", "amlogic,meson-gx-cvbs";
> > +};
> > +
> > +&vpu {
> > +	compatible = "amlogic,meson-gxbb-vpu", "amlogic,meson-gx-vpu";
> > +};
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-nexbox-a95x.dts
> > b/arch/arm64/boot/dts/amlogic/meson-gxl-nexbox-a95x.dts index
> > e99101a..2a9b46f 100644
> > --- a/arch/arm64/boot/dts/amlogic/meson-gxl-nexbox-a95x.dts
> > +++ b/arch/arm64/boot/dts/amlogic/meson-gxl-nexbox-a95x.dts
> > @@ -203,3 +203,7 @@
> > 
> >  	clocks = <&clkc CLKID_FCLK_DIV4>;
> >  	clock-names = "clkin0";
> >  
> >  };
> > 
> > +
> > +&venc_cvbs {
> > +	status = "okay";
> > +};
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> > b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi index 3af54dc..b60c5ce 100644
> > --- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> > +++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> > @@ -299,3 +299,11 @@
> > 
> >  		 <&clkc CLKID_FCLK_DIV2>;
> >  	
> >  	clock-names = "core", "clkin0", "clkin1";
> >  
> >  };
> > 
> > +
> > +&venc_cvbs {
> > +	compatible = "amlogic,meson-gxl-cvbs", "amlogic,meson-gx-cvbs";
> > +};
> > +
> > +&vpu {
> > +	compatible = "amlogic,meson-gxl-vpu", "amlogic,meson-gx-vpu";
> > +};
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts
> > b/arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts index
> > d320727..1ae2451 100644
> > --- a/arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts
> > +++ b/arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts
> > @@ -167,3 +167,7 @@
> > 
> >  		max-speed = <1000>;
> >  	
> >  	};
> >  
> >  };
> > 
> > +
> > +&venc_cvbs {
> > +	status = "okay";
> > +};
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-gxm.dtsi
> > b/arch/arm64/boot/dts/amlogic/meson-gxm.dtsi index c1974bb..fecd8c2 100644
> > --- a/arch/arm64/boot/dts/amlogic/meson-gxm.dtsi
> > +++ b/arch/arm64/boot/dts/amlogic/meson-gxm.dtsi
> > @@ -112,3 +112,11 @@
> > 
> >  		};
> >  	
> >  	};
> >  
> >  };
> > 
> > +
> > +&venc_cvbs {
> > +	compatible = "amlogic,meson-gxm-cvbs", "amlogic,meson-gx-cvbs";
> > +};
> > +
> > +&vpu {
> > +	compatible = "amlogic,meson-gxm-vpu", "amlogic,meson-gx-vpu";
> > +};

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/4] ARM64: dts: meson-gx: Add Graphic Controller nodes
From: Laurent Pinchart @ 2016-11-29 19:16 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: devicetree, Xing.Xu, victor.wan, khilman, linux-kernel, dri-devel,
	jerry.cao, carlo, linux-amlogic, linux-arm-kernel
In-Reply-To: <1480416469-9655-3-git-send-email-narmstrong@baylibre.com>

Hi Neil,

Thank you for the patch.

On Tuesday 29 Nov 2016 11:47:47 Neil Armstrong wrote:
> Add Video Processing Unit and CVBS Output nodes, and enable CVBS on selected
> boards.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  arch/arm64/boot/dts/amlogic/meson-gx.dtsi          | 46 +++++++++++++++++++
>  .../boot/dts/amlogic/meson-gxbb-nexbox-a95x.dts    |  4 ++
>  arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi   |  4 ++
>  arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |  8 ++++
>  .../boot/dts/amlogic/meson-gxl-nexbox-a95x.dts     |  4 ++
>  arch/arm64/boot/dts/amlogic/meson-gxl.dtsi         |  8 ++++
>  .../arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts |  4 ++
>  arch/arm64/boot/dts/amlogic/meson-gxm.dtsi         |  8 ++++
>  8 files changed, 86 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi index fc033c0..644d5f6 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> @@ -153,6 +153,27 @@
>  		};
>  	};
> 
> +	venc_cvbs: venc-cvbs {
> +		compatible = "amlogic,meson-gx-cvbs";
> +		status = "disabled";

Still no registers here ?

> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			venc_cvbs_in: port@0 {

Nitpicking, you don't need a label here as ports are never referenced by 
phandle. Same for the vpu node below.

> +				 #address-cells = <1>;
> +				 #size-cells = <0>;
> +				 reg = <0>;
> +
> +				 venc_cvbs_in_vpu: endpoint@0 {
> +					 reg = <0>;

And there's no requirement to number the endpoint if there's a single one of 
them (but it's not forbidden either).

> +					 remote-endpoint = 
<&vpu_out_venc_cvbs>;
> +				};
> +			};
> +		};
> +	};
> +
>  	soc {
>  		compatible = "simple-bus";
>  		#address-cells = <2>;
> @@ -356,5 +377,30 @@
>  				status = "disabled";
>  			};
>  		};
> +
> +		vpu: vpu@d0100000 {
> +			compatible = "amlogic,meson-gx-vpu";
> +			reg = <0x0 0xd0100000 0x0 0x100000>,
> +			      <0x0 0xc883c000 0x0 0x1000>,
> +			      <0x0 0xc8838000 0x0 0x1000>;
> +			reg-names = "base", "hhi", "dmc";
> +			interrupts = <GIC_SPI 3 IRQ_TYPE_EDGE_RISING>;
> +
> +			ports {
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				vpu_out: port@1 {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +					reg = <1>;
> +
> +					vpu_out_venc_cvbs: endpoint@0 {
> +						reg = <0>;
> +						remote-endpoint = 
<&venc_cvbs_in_vpu>;
> +					};
> +				};
> +			};
> +		};
>  	};
>  };
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-nexbox-a95x.dts
> b/arch/arm64/boot/dts/amlogic/meson-gxbb-nexbox-a95x.dts index
> 9696820..a55d1cf 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-nexbox-a95x.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-nexbox-a95x.dts
> @@ -229,3 +229,7 @@
>  	clocks = <&clkc CLKID_FCLK_DIV4>;
>  	clock-names = "clkin0";
>  };
> +
> +&venc_cvbs {
> +	status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi index 5e5e2de..3c09bd1
> 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-p20x.dtsi
> @@ -266,3 +266,7 @@
>  	clocks = <&clkc CLKID_FCLK_DIV4>;
>  	clock-names = "clkin0";
>  };
> +
> +&venc_cvbs {
> +	status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi index ac5ad3b..1a321c8f
> 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> @@ -506,3 +506,11 @@
>  		 <&clkc CLKID_FCLK_DIV2>;
>  	clock-names = "core", "clkin0", "clkin1";
>  };
> +
> +&venc_cvbs {
> +	compatible = "amlogic,meson-gxbb-cvbs", "amlogic,meson-gx-cvbs";
> +};
> +
> +&vpu {
> +	compatible = "amlogic,meson-gxbb-vpu", "amlogic,meson-gx-vpu";
> +};
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-nexbox-a95x.dts
> b/arch/arm64/boot/dts/amlogic/meson-gxl-nexbox-a95x.dts index
> e99101a..2a9b46f 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxl-nexbox-a95x.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxl-nexbox-a95x.dts
> @@ -203,3 +203,7 @@
>  	clocks = <&clkc CLKID_FCLK_DIV4>;
>  	clock-names = "clkin0";
>  };
> +
> +&venc_cvbs {
> +	status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi index 3af54dc..b60c5ce 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> @@ -299,3 +299,11 @@
>  		 <&clkc CLKID_FCLK_DIV2>;
>  	clock-names = "core", "clkin0", "clkin1";
>  };
> +
> +&venc_cvbs {
> +	compatible = "amlogic,meson-gxl-cvbs", "amlogic,meson-gx-cvbs";
> +};
> +
> +&vpu {
> +	compatible = "amlogic,meson-gxl-vpu", "amlogic,meson-gx-vpu";
> +};
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts
> b/arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts index
> d320727..1ae2451 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxm-nexbox-a1.dts
> @@ -167,3 +167,7 @@
>  		max-speed = <1000>;
>  	};
>  };
> +
> +&venc_cvbs {
> +	status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxm.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gxm.dtsi index c1974bb..fecd8c2 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxm.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxm.dtsi
> @@ -112,3 +112,11 @@
>  		};
>  	};
>  };
> +
> +&venc_cvbs {
> +	compatible = "amlogic,meson-gxm-cvbs", "amlogic,meson-gx-cvbs";
> +};
> +
> +&vpu {
> +	compatible = "amlogic,meson-gxm-vpu", "amlogic,meson-gx-vpu";
> +};

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply

* Re: [PATCHv2] mfd: cpcap: Add minimal support
From: Tony Lindgren @ 2016-11-29 19:12 UTC (permalink / raw)
  To: Lee Jones, Samuel Ortiz
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Marcel Partap, Mark Rutland,
	Michael Scott, Rob Herring
In-Reply-To: <20161129164702.5334-1-tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [161129 08:48]:
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/motorola-cpcap.txt
> @@ -0,0 +1,31 @@
> +Motorola CPCAP PMIC device tree binding
> +
> +Required properties:
> +- compatible		: One or both of "motorola,cpcap" or "ste,6556002"
> +- reg			: SPI chip select
> +- interrupt-parent	: The parent interrupt controller
> +- interrupts		: The interrupt line the device is connected to
> +- interrupt-controller	: Marks the device node as an interrupt controller
> +- #interrupt-cells	: The number of cells to describe an IRQ, should be 2
> +- #address-cells	: Child device offset number of cells, typically 1
> +- #size-cells		: Child device size number of cells, typically 1
> +- spi-max-frequency	: Typically set to 3000000
> +- spi-cs_high		: SPI chip select direction
> +
> +Example:
> +
> +&mcspi1 {
> +	cpcap: pmic@0 {
> +		compatible = "motorola,cpcap", "ste,6556002";
> +		reg = <0>;	/* cs0 */
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <7 IRQ_TYPE_EDGE_RISING>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		spi-max-frequency = <3000000>;
> +		spi-cs-high;
> +	};
> +};

The #address-cells should be 0 instead of 1 above.

Tony
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 1/1] scripts: Fixing NULL pointer dereference when pos->file is NULL
From: Frank Rowand @ 2016-11-29 19:08 UTC (permalink / raw)
  To: Maninder Singh, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	frowand.list-Re5JQEeQqe8AvxtiuMwx3w
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	v.narang-Sze3O3UU22JBDgjK7y7TUQ, pankaj.m-Sze3O3UU22JBDgjK7y7TUQ,
	ajeet.y-Sze3O3UU22JBDgjK7y7TUQ
In-Reply-To: <1480415699-35335-1-git-send-email-maninder1.s-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

On 11/29/16 02:34, Maninder Singh wrote:
> This patch fixes NULL pointer dereference when pos->file is NULL.
> 
> caught with static analysis tool.
> Signed-off-by: Maninder Singh <maninder1.s-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Vaneet Narang <v.narang-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  scripts/dtc/srcpos.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/scripts/dtc/srcpos.c b/scripts/dtc/srcpos.c
> index f534c22..360fd14 100644
> --- a/scripts/dtc/srcpos.c
> +++ b/scripts/dtc/srcpos.c
> @@ -252,12 +252,11 @@ struct srcpos *
>  srcpos_dump(struct srcpos *pos)
>  {
>  	printf("file        : \"%s\"\n",
> -	       pos->file ? (char *) pos->file : "<no file>");
> +	       pos->file ?  pos->file->name : "<no file>");
>  	printf("first_line  : %d\n", pos->first_line);
>  	printf("first_column: %d\n", pos->first_column);
>  	printf("last_line   : %d\n", pos->last_line);
>  	printf("last_column : %d\n", pos->last_column);
> -	printf("file        : %s\n", pos->file->name);
>  }
>  
>  
> 

Hi Maninder,

Please send any patches for dtc to the devicetree-compiler
mail list.  For details, see:

   http://vger.kernel.org/vger-lists.html#devicetree-compiler

-Frank
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v7 4/8] drm/sunxi: Add DT bindings documentation of Allwinner HDMI
From: Laurent Pinchart @ 2016-11-29 18:46 UTC (permalink / raw)
  To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: Jean-Francois Moine, Dave Airlie, Maxime Ripard, Rob Herring,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <cd1caff75b6069af98680f78d672546748cb43c5.1480414715.git.moinejf-GANU6spQydw@public.gmane.org>

Hi Jean-François,

Thank you for the patch.

On Tuesday 29 Nov 2016 10:08:25 Jean-Francois Moine wrote:
> Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
> ---
>  .../devicetree/bindings/display/sunxi/hdmi.txt     | 56 +++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/sunxi/hdmi.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/sunxi/hdmi.txt
> b/Documentation/devicetree/bindings/display/sunxi/hdmi.txt new file mode
> 100644
> index 0000000..1e107cb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sunxi/hdmi.txt
> @@ -0,0 +1,56 @@
> +Allwinner HDMI Transmitter
> +==========================
> +
> +The Allwinner HDMI transmitters are included in the SoCs.
> +They support audio and video.
> +
> +Required properties:
> + - compatible : should be one of
> +		"allwinner,sun8i-a83t-hdmi"
> +		"allwinner,sun8i-h3-hdmi"
> + - reg: base address and size of the I/O memory
> + - clocks : phandles to the HDMI clocks as described in
> +	Documentation/devicetree/bindings/clock/clock-bindings.txt
> + - clock-names : must be
> +		"bus" : bus gate
> +		"clock" : streaming clock
> +		"ddc-clock" : DDC clock
> + - resets : One or two phandles to the HDMI resets
> + - reset-names : when 2 phandles, must be
> +		"hdmi0" and "hdmi1"
> + - #address-cells : should be <1>
> + - #size-cells : should be <0>
> +
> +Required nodes:
> + - port: Audio and video input port nodes with endpoint definitions
> +	as defined in Documentation/devicetree/bindings/graph.txt.
> +	port@0 is video and port@1 is audio.
> +
> +Example:
> +
> +	hdmi: hdmi@01ee0000 {
> +		compatible = "allwinner,sun8i-a83t-hdmi";
> +		reg = <0x01ee0000 0x20000>;
> +		clocks = <&ccu CLK_BUS_HDMI>, <&ccu CLK_HDMI>,
> +			 <&ccu CLK_HDMI_DDC>;
> +		clock-names = "bus", "clock", "ddc-clock";
> +		resets = <&ccu RST_HDMI0>, <&ccu RST_HDMI1>;
> +		reset-names = "hdmi0", "hdmi1";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&hdmi_pins_a>;
> +		status = "disabled";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		port@0 {			/* video */
> +			reg = <0>;
> +			hdmi_tcon1: endpoint {
> +				remote-endpoint = <&tcon1_hdmi>;
> +			};
> +		};
> +		port@1 {			/* audio */
> +			reg = <1>;
> +			hdmi_i2s2: endpoint {
> +				remote-endpoint = <&i2s2_hdmi>;
> +			};
> +		};

You need a third port for the HDMI encoder output, connected to an HDMI 
connector DT node.

> +	};

-- 
Regards,

Laurent Pinchart

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply

* Re: [PATCH v7 2/8] drm/sun8i: Add DT bindings documentation of Allwinner DE2
From: Laurent Pinchart @ 2016-11-29 18:45 UTC (permalink / raw)
  To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Rob Herring, Maxime Ripard,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <36111311.d8ul9hQ2CU@avalon>

Hi Jean-François,

A brief update.

On Tuesday 29 Nov 2016 20:41:30 Laurent Pinchart wrote:
> On Monday 28 Nov 2016 19:02:39 Jean-Francois Moine wrote:
> > Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
> > ---
> > 
> >  .../bindings/display/sunxi/sun8i-de2.txt           | 121 ++++++++++++++++
> >  1 file changed, 121 insertions(+)
> >  create mode 100644
> > 
> > Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt
> > b/Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt new file
> > mode 100644
> > index 0000000..edf38b8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt
> > @@ -0,0 +1,121 @@
> > +Allwinner sun8i Display Engine 2 subsystem
> > +==========================================
> > +
> > +The Allwinner DE2 subsystem contains a display controller (DE2),
> > +one or two LCD controllers (Timing CONtrollers) and their external
> > +interfaces (encoders/connectors).
> > +
> > +          +-----------+
> > +          | DE2       |
> > +          |           |
> > +          | +-------+ |
> > +  plane --->|       | |   +------+
> > +          | | mixer |---->| TCON |---> encoder  ---> display
> > +  plane --->|       | |   +------+     connector     device
> > +          | +-------+ |
> > +          |           |
> > +          | +-------+ |
> > +  plane --->|       | |   +------+
> > +          | | mixer |---->| TCON |---> encoder  ---> display
> > +  plane --->|       | |   +------+     connector     device
> > +          | +-------+ |
> > +          +-----------+
> > +
> > +The DE2 contains a processor which mixes the input planes and creates
> > +the images which are sent to the TCONs.
> > +From the software point of vue, there are 2 independent real-time
> > +mixers, each one being statically associated to one TCON.
> > +
> > +The TCONs adjust the image format to the one of the display device.
> > +
> > +Display controller (DE2)
> > +========================
> > +
> > +Required properties:
> > +
> > +- compatible: value should be one of the following
> > +		"allwinner,sun8i-a83t-display-engine"
> > +		"allwinner,sun8i-h3-display-engine"
> > +
> > +- reg: base address and size of the I/O memory
> > +
> > +- clocks: must include clock specifiers corresponding to entries in the
> > +		clock-names property.
> > +
> > +- clock-names: must contain
> > +		"bus": bus gate
> > +		"clock": clock
> > +
> > +- resets: phandle to the reset of the device
> > +
> > +- ports: must contain a list of 2 phandles, indexed by mixer number,
> > +	and pointing to display interface ports of TCONs
> > +
> > +LCD controller (TCON)
> > +=====================
> > +
> > +Required properties:
> > +
> > +- compatible: should be
> > +		"allwinner,sun8i-a83t-tcon"
> > +
> > +- reg: base address and size of the I/O memory
> > +
> > +- clocks: must include clock specifiers corresponding to entries in the
> > +		clock-names property.
> > +
> > +- clock-names: must contain
> > +		"bus": bus gate
> > +		"clock": pixel clock
> > +
> > +- resets: phandle to the reset of the device
> > +
> > +- interrupts: interrupt number for the TCON
> > +
> > +- port: port node with endpoint definitions as defined in
> > +	Documentation/devicetree/bindings/media/video-interfaces.txt
> > +
> > +Example:
> > +
> > +	de: de-controller@01000000 {
> > +		compatible = "allwinner,sun8i-h3-display-engine";
> > +		reg = <0x01000000 0x400000>;
> > +		clocks = <&ccu CLK_BUS_DE>, <&ccu CLK_DE>;
> > +		clock-names = "bus", "clock";
> > +		resets = <&ccu RST_BUS_DE>;
> > +		ports = <&tcon0_p>, <&tcon1_p>;
> 
> This isn't how the OF graph DT bindings are used. You should instead have
> 
> 	ports {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 		port@0 {

I forgot to add reg = <0>; (and similarly for port 1).

> 			de_out_0: endpoint {
> 				remote_endpoint = <&tcon0_hdmi>;
> 			};
> 		};
> 		port@1 {
> 			/* No endpoint as the port is not connected */
> 		};
> 	};
> 
> > +	};
> > +
> > +	tcon0: lcd-controller@01c0c000 {
> > +		compatible = "allwinner,sun8i-a83t-tcon";
> > +		reg = <0x01c0c000 0x400>;
> > +		clocks = <&ccu CLK_BUS_TCON0>, <&ccu CLK_TCON0>;
> > +		clock-names = "bus", "clock";
> > +		resets = <&ccu RST_BUS_TCON0>;
> > +		interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		tcon0_p: port {
> > +			tcon0_hdmi: endpoint {
> > +				remote-endpoint = <&hdmi_tcon0>;
> > +			};
> > +		};
> 
> and here
> 
> 	port {
> 		tcon0_hdmi: endpoint {
> 			remote-endpoint = <&de_out_0>;
> 		};
> 	};

The TCON has an output, so this should instead be

	port@0 {
		reg = <0>;
 		tcon0_in: endpoint {
 			remote-endpoint = <&de_out_0>;
 		};
	};
	port@1 {
		reg = <1>;
 		tcon1_out: endpoint {
 			remote-endpoint = <&hdmi_in>;
 		};
	};

(the second port requires adding the HDMI encoder to the example)

> > +	};
> > +
> > +	/* not used */
> > +	tcon1: lcd-controller@01c0d000 {
> > +		compatible = "allwinner,sun8i-h3-tcon";
> > +		reg = <0x01c0d000 0x400>;
> > +		clocks = <&ccu CLK_BUS_TCON1>,
> > +			 <&ccu CLK_TCON0>;	/* no clock */
> > +		clock-names = "bus", "clock";
> > +		interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
> > +		status = "disabled";
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		tcon1_p: port {
> > +			endpoint {
> > +				/* empty */
> > +			};
> > +		};
> 
> and here
> 
> 	port {
> 		/* No endpoint as the port is not connected */
> 	};
> 
> (although I'm not sure why you don't connect it)
> 
> > +	};

-- 
Regards,

Laurent Pinchart

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply

* Re: [PATCH v7 2/8] drm/sun8i: Add DT bindings documentation of Allwinner DE2
From: Laurent Pinchart @ 2016-11-29 18:41 UTC (permalink / raw)
  To: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
  Cc: Jean-Francois Moine, Dave Airlie, Maxime Ripard, Rob Herring,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <92fc53084274d5ab04fe28f2dc0b16cf5d94481e.1480414715.git.moinejf-GANU6spQydw@public.gmane.org>

Hi Jean-François,

Thank you for the patch.

On Monday 28 Nov 2016 19:02:39 Jean-Francois Moine wrote:
> Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
> ---
>  .../bindings/display/sunxi/sun8i-de2.txt           | 121 ++++++++++++++++++
>  1 file changed, 121 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt
> b/Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt new file
> mode 100644
> index 0000000..edf38b8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sunxi/sun8i-de2.txt
> @@ -0,0 +1,121 @@
> +Allwinner sun8i Display Engine 2 subsystem
> +==========================================
> +
> +The Allwinner DE2 subsystem contains a display controller (DE2),
> +one or two LCD controllers (Timing CONtrollers) and their external
> +interfaces (encoders/connectors).
> +
> +          +-----------+
> +          | DE2       |
> +          |           |
> +          | +-------+ |
> +  plane --->|       | |   +------+
> +          | | mixer |---->| TCON |---> encoder  ---> display
> +  plane --->|       | |   +------+     connector     device
> +          | +-------+ |
> +          |           |
> +          | +-------+ |
> +  plane --->|       | |   +------+
> +          | | mixer |---->| TCON |---> encoder  ---> display
> +  plane --->|       | |   +------+     connector     device
> +          | +-------+ |
> +          +-----------+
> +
> +The DE2 contains a processor which mixes the input planes and creates
> +the images which are sent to the TCONs.
> +From the software point of vue, there are 2 independent real-time
> +mixers, each one being statically associated to one TCON.
> +
> +The TCONs adjust the image format to the one of the display device.
> +
> +Display controller (DE2)
> +========================
> +
> +Required properties:
> +
> +- compatible: value should be one of the following
> +		"allwinner,sun8i-a83t-display-engine"
> +		"allwinner,sun8i-h3-display-engine"
> +
> +- reg: base address and size of the I/O memory
> +
> +- clocks: must include clock specifiers corresponding to entries in the
> +		clock-names property.
> +
> +- clock-names: must contain
> +		"bus": bus gate
> +		"clock": clock
> +
> +- resets: phandle to the reset of the device
> +
> +- ports: must contain a list of 2 phandles, indexed by mixer number,
> +	and pointing to display interface ports of TCONs
> +
> +LCD controller (TCON)
> +=====================
> +
> +Required properties:
> +
> +- compatible: should be
> +		"allwinner,sun8i-a83t-tcon"
> +
> +- reg: base address and size of the I/O memory
> +
> +- clocks: must include clock specifiers corresponding to entries in the
> +		clock-names property.
> +
> +- clock-names: must contain
> +		"bus": bus gate
> +		"clock": pixel clock
> +
> +- resets: phandle to the reset of the device
> +
> +- interrupts: interrupt number for the TCON
> +
> +- port: port node with endpoint definitions as defined in
> +	Documentation/devicetree/bindings/media/video-interfaces.txt
> +
> +Example:
> +
> +	de: de-controller@01000000 {
> +		compatible = "allwinner,sun8i-h3-display-engine";
> +		reg = <0x01000000 0x400000>;
> +		clocks = <&ccu CLK_BUS_DE>, <&ccu CLK_DE>;
> +		clock-names = "bus", "clock";
> +		resets = <&ccu RST_BUS_DE>;
> +		ports = <&tcon0_p>, <&tcon1_p>;

This isn't how the OF graph DT bindings are used. You should instead have

	ports {
		#address-cells = <1>;
		#size-cells = <0>;
		port@0 {
			de_out_0: endpoint {
				remote_endpoint = <&tcon0_hdmi>;
			};
		};
		port@1 {
			/* No endpoint as the port is not connected */
		};
	};

> +	};
> +
> +	tcon0: lcd-controller@01c0c000 {
> +		compatible = "allwinner,sun8i-a83t-tcon";
> +		reg = <0x01c0c000 0x400>;
> +		clocks = <&ccu CLK_BUS_TCON0>, <&ccu CLK_TCON0>;
> +		clock-names = "bus", "clock";
> +		resets = <&ccu RST_BUS_TCON0>;
> +		interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		tcon0_p: port {
> +			tcon0_hdmi: endpoint {
> +				remote-endpoint = <&hdmi_tcon0>;
> +			};
> +		};

and here

	port {
		tcon0_hdmi: endpoint {
			remote-endpoint = <&de_out_0>;
		};
	};

> +	};
> +
> +	/* not used */
> +	tcon1: lcd-controller@01c0d000 {
> +		compatible = "allwinner,sun8i-h3-tcon";
> +		reg = <0x01c0d000 0x400>;
> +		clocks = <&ccu CLK_BUS_TCON1>,
> +			 <&ccu CLK_TCON0>;	/* no clock */
> +		clock-names = "bus", "clock";
> +		interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
> +		status = "disabled";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		tcon1_p: port {
> +			endpoint {
> +				/* empty */
> +			};
> +		};

and here

	port {
		/* No endpoint as the port is not connected */
	};

(although I'm not sure why you don't connect it)

> +	};

-- 
Regards,

Laurent Pinchart

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply

* Re: [PATCH v3 1/2] of: Fix issue where code would fall through to error case.
From: Frank Rowand @ 2016-11-29 18:41 UTC (permalink / raw)
  To: Moritz Fischer, devicetree-u79uwXL29TY76Z2rM5mHXA
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w,
	mdf-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <1480278058-19688-1-git-send-email-moritz.fischer-+aYTwkv1SeIAvxtiuMwx3w@public.gmane.org>

On 11/27/16 12:20, Moritz Fischer wrote:
> No longer fall through into the error case that prints out
> an error if no error (err = 0) occurred.
> 
> Rework error handling to print error where it occured instead
> of having a global catch-all at the end of the function.
> 
> Fixes d9181b20a83(of: Add back an error message, restructured)
> Signed-off-by: Moritz Fischer <moritz.fischer-+aYTwkv1SeIAvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Frank Rowand <frank.rowand-mEdOJwZ7QcZBDgjK7y7TUQ@public.gmane.org>
> ---
> Hi Rob, Frank
> 
> this has already Frank's Reviewed-by: tag on it, but since I changed around the
> earlier part (before tree_node gets assigned) Frank might wanna take another look
> at this.

< snip >

Hi Moritz,

I agree with Rob's suggested one line solution (that I initially misunderstood).
The one line fix is to add "if (err)" immediately following label "err_out" in
of_resolve_phandles().

As far as patch 2/2, I'm not bothered by the two instances of line over 80 chars.
But if Rob wants to take patch 2/2 I have no objection.

-Frank
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 01/13] devicetree/bindings: display: Document common panel properties
From: Laurent Pinchart @ 2016-11-29 18:23 UTC (permalink / raw)
  To: Rob Herring
  Cc: open list:MEDIA DRIVERS FOR RENESAS - FCP,
	devicetree@vger.kernel.org, Tomi Valkeinen, Laurent Pinchart,
	dri-devel
In-Reply-To: <CAL_JsqL0+4--nE1UDHWzqS-s18A+q2i76S8=woCb5huV3jUMew@mail.gmail.com>

Hi Rob,

On Tuesday 29 Nov 2016 09:14:09 Rob Herring wrote:
> On Tue, Nov 29, 2016 at 2:27 AM, Laurent Pinchart wrote:
> > On Tuesday 22 Nov 2016 11:36:55 Laurent Pinchart wrote:
> >> On Monday 21 Nov 2016 10:48:15 Rob Herring wrote:
> >>> On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote:
> >>>> Document properties common to several display panels in a central
> >>>> location that can be referenced by the panel device tree bindings.
> >>> 
> >>> Looks good. Just one comment...
> >>> 
> >>> [...]
> >>> 
> >>>> +Connectivity
> >>>> +------------
> >>>> +
> >>>> +- ports: Panels receive video data through one or multiple
> >>>> connections. While
> >>>> +  the nature of those connections is specific to the panel type, the
> >>>> +  connectivity is expressed in a standard fashion using ports as
> >>>> specified in
> >>>> +  the device graph bindings defined in
> >>>> +  Documentation/devicetree/bindings/graph.txt.
> >>> 
> >>> We allow panels to either use graph binding or be a child of the
> >>> display controller.
> >> 
> >> I knew that some display controllers use a phandle to the panel (see the
> >> fsl,panel and nvidia,panel properties), but I didn't know we had panels
> >> as children of display controller nodes. I don't think we should allow
> >> that for anything but DSI panels, as the DT hierarchy is based on control
> >> buses. Are you sure we have other panels instantiated through that
> >> mechanism ?
>
> Some panels have no control bus, so were do we place them?

I'd say under the root node, like all similar control-less devices.

> I would say the hierarchy is based on buses with a preference for the
> control bus when there are multiple buses. I'm not a fan of just sticking
> things are the top level.

OK, so much for my comment a few lines up :-)

The problem with placing non-DSI panels as children of the display controller 
and not using OF graph is that the panel bindings become dependent of the 
display controller being used. A display controller using OF graph would 
require the panel to do the same, while a display controller expecting a panel 
child node (with a specific name) would require DT properties for the panel 
node.

I'm also not sure the complexity of OF graph is really that prohibitive if you 
compare it to panels as child nodes. To get the panel driver to bind to the 
panel DT node the display controller driver would need to create a platform 
device for the panel and register it. That's not very difficult, but parsing a 
single port and endpoint isn't either (and we could even provide a helper 
function for that, a version of of_drm_find_panel() that would take as an 
argument the display controller device node instead of the panel device node).

> > Ping ?
> > 
> > Please note that this file documents properties common to multiple panel
> > DT bindings, but in no way makes it mandatory to use the OF graph bindings
> > for panels. The decision is left to individual bindings.
> 
> It is mandatory in the sense that we don't want more cases of "fsl,panel".

That I agree with :-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply

* Re: [PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support
From: Andrew Lunn @ 2016-11-29 18:20 UTC (permalink / raw)
  To: Vivien Didelot
  Cc: Uwe Kleine-König, Rob Herring, Frank Rowand,
	Andreas Färber, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Michal Hrusecki, Tomas Hlavacek, Bed??icha Ko??atu,
	Florian Fainelli, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <87oa0yb29b.fsf-BOtmAI95c/+pXNIQCVAXCG0Lkn3mC4nZ0tOlhedn3YvkypF1WZHjJXhe7Zk3YmMvjmZSf7Nhrd8@public.gmane.org>

On Tue, Nov 29, 2016 at 12:54:24PM -0500, Vivien Didelot wrote:
> Hi,
> 
> Uwe Kleine-König <uwe-rXY34ruvC2xidJT2blvkqNi2O/JbrIOy@public.gmane.org> writes:
> 
> > Also it seems wrong to write "marvell,mv88e6085" (only) if I know the
> > hardware is really a "marvell,mv88e6176".
> 
> I agree. It might be complex for a user to dig into the driver in order
> to figure out how the switch ID is read and which compatible to choose.
> 
> I've sent a patch to change this https://lkml.org/lkml/2016/6/8/1198 but
> Andrew had a stronger opinion on compatible strings, which makes sense.
> 
> >> Linus has said he does not like ARM devices because of all the busses
> >> which are not enumerable. Here we have a device which with a little
> >> bit of help we can enumerate. So we should. 
> >
> > If you write
> >
> > 	compatible = "marvell,mv88e6176", "marvell,mv88e6085";
> >
> > you can still enumerate in the same way as before.
> 
> So we don't break the existing DTS files, I like this.
> 
> The driver already prints info about the detected switch. Instead of
> failing at probe, which seems against the notion of compatible and
> breaks the existing behavior, it could report the eventual mismatch?

I'm still against it.

Say we let the driver probe and warn when the compatible string is
wrong. Is this likely to get fixed? Probably not, it is just a
warning, people ignore them.

A few years later, we have accumulated a number of broken device
trees. And suddenly we really do have a Marvell device with a broken
ID in its port register, or more likely, the revision number was not
incremented but there was incompatible register changes. We suddenly
do have to look at the compatible string. But we know many are actually
wrong. How do we know which to trust? We basically have to say, if the
compatible is "marvell,mv88e6042" we trust it, all the others we don't
trust and ignore.

Isn't that madness?

What we have today is verified correct. If this hypothetical
"marvell,mv88e6042" does happen, then no problems, we add a compatible
string for it, and it works.

We should probably add a comment to the mv88e6xxx_of_match array,
explaining how it currently works.

      Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/2] net: dsa: mv88e6xxx: Add 88E6176 device tree support
From: Vivien Didelot @ 2016-11-29 17:54 UTC (permalink / raw)
  To: Uwe Kleine-König, Andrew Lunn
  Cc: Rob Herring, Frank Rowand, Andreas Färber, netdev,
	linux-arm-kernel, Michal Hrusecki, Tomas Hlavacek,
	Bed??icha Ko??atu, Florian Fainelli, linux-kernel, devicetree
In-Reply-To: <2c59cc79-b6dc-9920-1725-a7785ff3b6bf@kleine-koenig.org>

Hi,

Uwe Kleine-König <uwe@kleine-koenig.org> writes:

> Also it seems wrong to write "marvell,mv88e6085" (only) if I know the
> hardware is really a "marvell,mv88e6176".

I agree. It might be complex for a user to dig into the driver in order
to figure out how the switch ID is read and which compatible to choose.

I've sent a patch to change this https://lkml.org/lkml/2016/6/8/1198 but
Andrew had a stronger opinion on compatible strings, which makes sense.

>> Linus has said he does not like ARM devices because of all the busses
>> which are not enumerable. Here we have a device which with a little
>> bit of help we can enumerate. So we should. 
>
> If you write
>
> 	compatible = "marvell,mv88e6176", "marvell,mv88e6085";
>
> you can still enumerate in the same way as before.

So we don't break the existing DTS files, I like this.

The driver already prints info about the detected switch. Instead of
failing at probe, which seems against the notion of compatible and
breaks the existing behavior, it could report the eventual mismatch?

We have examples for both usage, still I don't know what the best
practices are. My _preference_ would go with enumerating them all.

Thanks,

        Vivien

^ permalink raw reply

* Re: [RESEND PATCH 2/2] PCI: rockchip: Add quirk to disable RC's ASPM L0s
From: Bjorn Helgaas @ 2016-11-29 17:25 UTC (permalink / raw)
  To: Rob Herring
  Cc: Shawn Lin, Bjorn Helgaas,
	linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	open list:ARM/Rockchip SoC..., Wenrui Li, Brian Norris,
	Jeffy Chen, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAL_JsqKkK15Jb3tWh1Mw7RU6d8=6RYMjSEs-c4EQySaUAwnFUA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Tue, Nov 29, 2016 at 09:34:09AM -0600, Rob Herring wrote:
> On Sun, Nov 13, 2016 at 10:11 PM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> > Rockchip's RC outputs 100MHz reference clock but there are
> > two methods for PHY to generate it.
> >
> > (1)One of them is to use system PLL to generate 100MHz clock and
> > the PHY will relock it and filter signal noise then outputs the
> > reference clock.
> >
> > (2)Another way is to share Soc's 24MHZ crystal oscillator with
> > PHY and force PHY's DLL to generate 100MHz internally.
> >
> > When using case(2), the exit from L0s doesn't work fine occasionally
> > due to the broken design of RC receiver's logical circuit. So even if
> > we use extended-synch, it still fails for PHY to relock the bits from
> > FTS sometimes. This will hang the system.
> >
> > Maybe we could argue that why not use case(1) to avoid it? The reason
> > is that as we could see the reference clock is derived from system PLL
> > and the path from it to PHY isn't so clean which means there are some
> > noise introduced by power-domain and other buses can't be filterd out
> > by PHY and we could see noise from the frequency spectrum by oscilloscope.
> > This makes the TX compatibility test a little difficult to pass the spec.
> > So case(1) and case(2) are both used indeed now. If using case(2), we
> > should disable RC's L0s support, and that is why we need this property to
> > indicate this quirk.
> >
> > Also after checking quirk.c, I noticed there is already a quirk for
> > disabling L0s unconditionally, quirk_disable_aspm_l0s. But obviously we
> > shouldn't do that as mentioned above that case(1) could still works fine
> > with L0s.
> >
> > Reported-by: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> > Cc: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> >
> > ---
> >
> >  Documentation/devicetree/bindings/pci/rockchip-pcie.txt | 2 ++
> >  drivers/pci/host/pcie-rockchip.c                        | 9 +++++++++
> >  2 files changed, 11 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > index ba67b39..cfa44a7 100644
> > --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> > @@ -42,6 +42,8 @@ Required properties:
> >  Optional Property:
> >  - ep-gpios: contain the entry for pre-reset gpio
> >  - num-lanes: number of lanes to use
> > +- quirk,aspm-no-l0s: RC won't support ASPM L0s. This property is needed if
> 
> quirk is not a vendor. Drop it.

I dropped this patch from pci/host-rockchip for now.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip
From: Javier Martinez Canillas @ 2016-11-29 17:13 UTC (permalink / raw)
  To: Matt Ranostay
  Cc: linux-wireless@vger.kernel.org, Linux Kernel,
	devicetree@vger.kernel.org, linux-mmc@vger.kernel.org,
	Tony Lindgren, Ulf Hansson, Mark Rutland, Srinivas Kandagatla
In-Reply-To: <1479434109-8745-1-git-send-email-matt@ranostay.consulting>

Hello Matt,

On Thu, Nov 17, 2016 at 10:55 PM, Matt Ranostay
<matt@ranostay.consulting> wrote:
> Allow power sequencing for the Marvell SD8787 Wifi/BT chip.
> This can be abstracted to other chipsets if needed in the future.
>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> ---
>  .../devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt  |  14 +++
>  .../bindings/net/wireless/marvell-sd8xxx.txt       |   4 +
>  drivers/mmc/core/Kconfig                           |  10 ++
>  drivers/mmc/core/Makefile                          |   1 +
>  drivers/mmc/core/pwrseq_sd8787.c                   | 117 +++++++++++++++++++++
>  5 files changed, 146 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
>  create mode 100644 drivers/mmc/core/pwrseq_sd8787.c
>
> diff --git a/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt

According Documentation/devicetree/bindings/submitting-patches.txt,
the DT bindings patches should posted as a separate patch.

> new file mode 100644
> index 000000000000..1b658351629b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.txt
> @@ -0,0 +1,14 @@
> +* Marvell SD8787 power sequence provider
> +
> +Required properties:
> +- compatible: must be "mmc-pwrseq-sd8787".

Since this is not a generic binding, the compatible string should have
a vendor prefix.

> +- pwndn-gpio: contains a power down GPIO specifier.
> +- reset-gpio: contains a reset GPIO specifier.
> +

I wonder if we really need a custom power sequence provider for just
this SDIO WiFI chip though. AFAICT the only missing piece in
mmc-pwrseq-simple is the power down GPIO property, so maybe
mmc-pwrseq-simple could be extended instead to have an optional
powerdown-gpios property and instead in the Marvell SD8787 DT binding
can be mentioned which mmc-pwrseq-simple properties are required for
the device.

> +Example:
> +
> +       wifi_pwrseq: wifi_pwrseq {
> +               compatible = "mmc-pwrseq-sd8787";
> +               pwrdn-gpio = <&twl_gpio 0 GPIO_ACTIVE_LOW>;
> +               reset-gpio = <&twl_gpio 1 GPIO_ACTIVE_LOW>;
> +       }
> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt

Does this patch depend on a previous posted series? I don't see this
file in today's linux-next...

> index c421aba0a5bc..08fd65d35725 100644
> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
> @@ -32,6 +32,9 @@ Optional properties:
>                  so that the wifi chip can wakeup host platform under certain condition.
>                  during system resume, the irq will be disabled to make sure
>                  unnecessary interrupt is not received.
> +  - vmmc-supply: a phandle of a regulator, supplying VCC to the card
> +  - mmc-pwrseq:  phandle to the MMC power sequence node. See "mmc-pwrseq-*"
> +                for documentation of MMC power sequence bindings.
>
>  Example:
>
> @@ -44,6 +47,7 @@ so that firmware can wakeup host using this device side pin.
>  &mmc3 {
>         status = "okay";
>         vmmc-supply = <&wlan_en_reg>;
> +       mmc-pwrseq = <&wifi_pwrseq>;
>         bus-width = <4>;
>         cap-power-off-card;
>         keep-power-in-suspend;

I think this change should be split in a separate patch as well.

Best regards,
Javier

^ permalink raw reply

* Re: [PATCH] mmc: pwrseq: add support for Marvell SD8787 chip
From: Ulf Hansson @ 2016-11-29 16:53 UTC (permalink / raw)
  To: Rob Herring
  Cc: Matt Ranostay, linux-wireless@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-mmc@vger.kernel.org, Tony Lindgren, Mark Rutland,
	Srinivas Kandagatla
In-Reply-To: <CAL_Jsq+80LAkCGEdP5T0AWFV2nRtZ8nrkBoNRB47CR9JN=9dog@mail.gmail.com>

On 29 November 2016 at 15:51, Rob Herring <robh@kernel.org> wrote:
> On Mon, Nov 28, 2016 at 9:54 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> [...]
>>
>>>> +
>>>> +Example:
>>>> +
>>>> +     wifi_pwrseq: wifi_pwrseq {
>>>> +             compatible = "mmc-pwrseq-sd8787";
>>>> +             pwrdn-gpio = <&twl_gpio 0 GPIO_ACTIVE_LOW>;
>>>> +             reset-gpio = <&twl_gpio 1 GPIO_ACTIVE_LOW>;
>>>> +     }
>>>> diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>>> index c421aba0a5bc..08fd65d35725 100644
>>>> --- a/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>>> +++ b/Documentation/devicetree/bindings/net/wireless/marvell-sd8xxx.txt
>>>> @@ -32,6 +32,9 @@ Optional properties:
>>>>                so that the wifi chip can wakeup host platform under certain condition.
>>>>                during system resume, the irq will be disabled to make sure
>>>>                unnecessary interrupt is not received.
>>>> +  - vmmc-supply: a phandle of a regulator, supplying VCC to the card
>>>
>>> This is why pwrseq is wrong. You have some properties in the card node
>>> and some in pwrseq node. Everything should be in the card node.
>>
>> Put "all" in the card node, just doesn't work for MMC. Particular in
>> cases when we have removable cards, as then it would be wrong to have
>> a card node.
>
> When is there a problem with removable cards? The connector is
> standard and everything needed (CD, VMMC, VDDIO, etc.) is defined in
> the host controller node. If that isn't sufficient, then we should
> start defining a connector node.

I probably didn't get your point. Anyway, let's try again.

I don't see any problems with removable cards and neither with
non-removable cards.

Normally, we don't need a connector node, but instead we put the
related properties/phandles in the host controller node. This works
fine!

For SDIO func devices, sometimes we need a child node of the host
controller node, as to be able to describe certain characteristics of
the SDIO func device. So this case is kind of a special type of card
node (because one can have several SDIO func devices attached, even if
this isn't used).

Now, to follow the current bindings, we must not put connector related
bindings in the card node, like the vmmc-supply, perhaps that is what
causes confusion here?

>
>> The mmc pwrseq DT bindings just follows the legacy approach for MMC
>> and that's why the pwrseq handle is at the controller node. Yes, would
>> could have done it differently, but this is the case now, so we will
>> have to accept that.
>
> We're stuck with supporting the existing cases. That doesn't mean
> we're stuck with the same thing for new cases.

Agree!

But I think there is nothing to improve in this case, or am I wrong?

Kind regards
Uffe

^ permalink raw reply

* Re: [PATCH] of: Fix issue where code would fall through to error case.
From: Frank Rowand @ 2016-11-29 16:52 UTC (permalink / raw)
  To: Rob Herring
  Cc: Moritz Fischer, Moritz Fischer, linux-kernel@vger.kernel.org,
	Pantelis Antoniou, moritz, devicetree@vger.kernel.org
In-Reply-To: <CAL_Jsq+eQYGq7ZtiQaerBk1SP=K1Wgd+b_2UGKFoRS4_s_8Fvg@mail.gmail.com>

On 11/29/16 07:06, Rob Herring wrote:
> On Mon, Nov 28, 2016 at 9:30 AM, Frank Rowand <frowand.list@gmail.com> wrote:
>> On 11/26/16 13:39, Frank Rowand wrote:
>>> On 11/23/16 13:58, Rob Herring wrote:
>>>> On Thu, Nov 17, 2016 at 6:10 PM, Moritz Fischer
>>>> <moritz.fischer.private@gmail.com> wrote:
>>>>> On Thu, Nov 17, 2016 at 4:02 PM, Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>> On 11/17/16 15:40, Frank Rowand wrote:
>>>>>>> On 11/17/16 15:25, Moritz Fischer wrote:
>>>>>>>> No longer fall through into the error case that prints out
>>>>>>>> an error if no error (err = 0) occurred.
>>>>>>>>
>>>>>>>> Fixes d9181b20a83(of: Add back an error message, restructured)
>>>>>>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>>>>>>> ---
>>>>>>>>  drivers/of/resolver.c | 6 +++++-
>>>>>>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>>>>>>>> index 783bd09..785076d 100644
>>>>>>>> --- a/drivers/of/resolver.c
>>>>>>>> +++ b/drivers/of/resolver.c
>>>>>>>> @@ -358,9 +358,13 @@ int of_resolve_phandles(struct device_node *overlay)
>>>>>>>>
>>>>>>>>              err = update_usages_of_a_phandle_reference(overlay, prop, phandle);
>>>>>>>>              if (err)
>>>>>>>> -                    break;
>>>>>>>> +                    goto err_out;
>>>>>>>>      }
>>>>>>>>
>>>>>>>> +    of_node_put(tree_symbols);
>>>>>>>> +
>>>>>>>> +    return 0;
>>>>>>>> +
>>>>>>>>  err_out:
>>>>>>>>      pr_err("overlay phandle fixup failed: %d\n", err);
>>>>>>>>  out:
>>>>>>>
>>>>>>> Thanks for catching that.
>>>>>>>
>>>>>>> Rob, please apply.
>>>>>>>
>>>>>>> Reviewed-by: Frank Rowand <frank.rowand@am.sony.com>
>>>>>>>
>>>>>>> -Frank
>>>>>>
>>>>>> On second thought, isn't the common pattern when clean up is needed for
>>>>>> both the no-error path and the error path something like:
>>>>>>
>>>>>>
>>>>>>         out:
>>>>>>                 of_node_put(tree_symbols);
>>>>>>                 return err;
>>>>>>
>>>>>>         err_out:
>>>>>>                 pr_err("overlay phandle fixup failed: %d\n", err);
>>>>>>                 goto out;
>>>>>>         }
>>>>>>
>>>>>>
>>>>>> I don't have a strong opinion, whatever Rob wants to take is fine with me.
>>>>>
>>>>> Same here. I tried to avoid the jumping back part, but if that's the
>>>>> common pattern,
>>>>> I can submit a v2 doing that instead.
>>>>
>>>> Both are ugly. Just do:
>>>>
>>>> if (err)
>>>>   pr_err(...);
>>>>
>>>> Rob
>>>
>>> Agreed.  Thanks for the touch of sanity Rob.
>>>
>>> -Frank
>>
>> I succumbed to looking only at the few lines of code above and not the
>> fuller context of the file that the patch applies to.
>>
>> The proposed patch was fixing the problem that a normal completion
>> of the for loop was falling through into the err_out label.  So what
>> looks cleaner ("if (err) pr_err(...)") is actually not correct.
> 
> What!? The *only* problem was printing the error message in the err=0
> case. All that needs to be fixed is not doing that. If we do that,
> then we really only need 1 goto label.
> 
> Rob

I misread your original suggestion to mean to put the "if (err) pr_err(...)"
inside the for loop, where Moritz had made his changes.

Now I understand what you really meant, to put the "if (err) pr_err(...)"
after the for loop.

Yes, that is a good way to do it.

Sorry for the extra noise....

-Frank

^ permalink raw reply

* [PATCHv2] mfd: cpcap: Add minimal support
From: Tony Lindgren @ 2016-11-29 16:47 UTC (permalink / raw)
  To: Lee Jones, Samuel Ortiz
  Cc: linux-kernel, linux-omap, devicetree, Marcel Partap, Mark Rutland,
	Michael Scott, Rob Herring

Many Motorola phones like droid 4 are using a custom PMIC called CPCAP
or 6556002. We can support it's core features quite easily with regmap_spi
and regmap_irq.

The children of cpcap, such as regulators, ADC and USB, can be just regular
device drivers and defined in the dts file. They get probed as we call
of_platform_populate() at the end of our probe, and then the children
can just call dev_get_regmap(dev.parent, NULL) to get the regmap.

Cc: devicetree@vger.kernel.org
Cc: Marcel Partap <mpartap@gmx.net>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Michael Scott <michael.scott@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---

Changes from v1:

- Addressed comments from Lee Jones except did not start
  generalizing bulk adding of IRQ chips. That can be done
  later as needed.

- Dropped the ranges translation based on comments from
  Lee Jones and Rob Herring. We are using regmap anyways.

- Changed naming to use motorola-cpcap as this is Motorola
  custom PMIC manufactured by STE and TI.

- Moved the revision and vendor check to motorola-cpcap.h.
  This keeps the undocumented register bits limited to
  these functions and the child device drivers can use
  them for the related workarounds.

- Checked that motorola-cpcap.h is really aligned despite
  how the patch looks for some of the lines.

---
 .../devicetree/bindings/mfd/motorola-cpcap.txt     |  31 +++
 drivers/mfd/Kconfig                                |  11 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/motorola-cpcap.c                       | 244 +++++++++++++++++
 include/linux/mfd/motorola-cpcap.h                 | 289 +++++++++++++++++++++
 5 files changed, 576 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/motorola-cpcap.txt
 create mode 100644 drivers/mfd/motorola-cpcap.c
 create mode 100644 include/linux/mfd/motorola-cpcap.h

diff --git a/Documentation/devicetree/bindings/mfd/motorola-cpcap.txt b/Documentation/devicetree/bindings/mfd/motorola-cpcap.txt
new file mode 100644
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/motorola-cpcap.txt
@@ -0,0 +1,31 @@
+Motorola CPCAP PMIC device tree binding
+
+Required properties:
+- compatible		: One or both of "motorola,cpcap" or "ste,6556002"
+- reg			: SPI chip select
+- interrupt-parent	: The parent interrupt controller
+- interrupts		: The interrupt line the device is connected to
+- interrupt-controller	: Marks the device node as an interrupt controller
+- #interrupt-cells	: The number of cells to describe an IRQ, should be 2
+- #address-cells	: Child device offset number of cells, typically 1
+- #size-cells		: Child device size number of cells, typically 1
+- spi-max-frequency	: Typically set to 3000000
+- spi-cs_high		: SPI chip select direction
+
+Example:
+
+&mcspi1 {
+	cpcap: pmic@0 {
+		compatible = "motorola,cpcap", "ste,6556002";
+		reg = <0>;	/* cs0 */
+		interrupt-parent = <&gpio1>;
+		interrupts = <7 IRQ_TYPE_EDGE_RISING>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		spi-max-frequency = <3000000>;
+		spi-cs-high;
+	};
+};
+
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -713,6 +713,17 @@ config EZX_PCAP
 	  This enables the PCAP ASIC present on EZX Phones. This is
 	  needed for MMC, TouchScreen, Sound, USB, etc..
 
+config MFD_CPCAP
+	tristate "Support for Motorola CPCAP"
+	depends on SPI
+	depends on OF || COMPILE_TEST
+	select REGMAP_SPI
+	select REGMAP_IRQ
+	help
+	  Say yes here if you want to include driver for CPCAP.
+	  It is used on many Motorola phones and tablets as a PMIC.
+	  At least Motorola Droid 4 is known to use CPCAP.
+
 config MFD_VIPERBOARD
         tristate "Nano River Technologies Viperboard"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -97,6 +97,7 @@ obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
 obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
 
 obj-$(CONFIG_EZX_PCAP)		+= ezx-pcap.o
+obj-$(CONFIG_MFD_CPCAP)		+= motorola-cpcap.o
 
 obj-$(CONFIG_MCP)		+= mcp-core.o
 obj-$(CONFIG_MCP_SA11X0)	+= mcp-sa11x0.o
diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
new file mode 100644
--- /dev/null
+++ b/drivers/mfd/motorola-cpcap.c
@@ -0,0 +1,244 @@
+/*
+ * Motorola CPCAP PMIC core driver
+ *
+ * Copyright (C) 2016 Tony Lindgren <tony@atomide.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/sysfs.h>
+
+#include <linux/mfd/motorola-cpcap.h>
+#include <linux/spi/spi.h>
+
+#define CPCAP_NR_IRQ_REG_BANKS	6
+#define CPCAP_NR_IRQ_CHIPS	3
+
+struct cpcap_ddata {
+	struct spi_device *spi;
+	struct regmap_irq *irqs;
+	struct regmap_irq_chip_data *irqdata[CPCAP_NR_IRQ_CHIPS];
+	const struct regmap_config *regmap_conf;
+	struct regmap *regmap;
+};
+
+static int cpcap_check_revision(struct cpcap_ddata *cpcap)
+{
+	u16 vendor, rev;
+	int ret;
+
+	ret = cpcap_get_vendor(&cpcap->spi->dev, cpcap->regmap, &vendor);
+	if (ret)
+		return ret;
+
+	ret = cpcap_get_revision(&cpcap->spi->dev, cpcap->regmap, &rev);
+	if (ret)
+		return ret;
+
+	dev_info(&cpcap->spi->dev, "CPCAP vendor: %s rev: %i.%i (%x)\n",
+		 vendor == CPCAP_VENDOR_ST ? "ST" : "TI",
+		 CPCAP_REVISION_MAJOR(rev), CPCAP_REVISION_MINOR(rev),
+		 rev);
+
+	if (rev < CPCAP_REVISION_2_1) {
+		dev_info(&cpcap->spi->dev,
+			 "Please add old CPCAP revision support as needed\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+/*
+ * First two irq chips are the two private macro interrupt chips, the third
+ * irq chip is for register banks 1 - 4 and is available for drivers to use.
+ */
+static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
+	{
+		.name = "cpcap-m2",
+		.num_regs = 1,
+		.status_base = CPCAP_REG_MI1,
+		.ack_base = CPCAP_REG_MI1,
+		.mask_base = CPCAP_REG_MIM1,
+		.use_ack = true,
+	},
+	{
+		.name = "cpcap-m2",
+		.num_regs = 1,
+		.status_base = CPCAP_REG_MI2,
+		.ack_base = CPCAP_REG_MI2,
+		.mask_base = CPCAP_REG_MIM2,
+		.use_ack = true,
+	},
+	{
+		.name = "cpcap1-4",
+		.num_regs = 4,
+		.status_base = CPCAP_REG_INT1,
+		.ack_base = CPCAP_REG_INT1,
+		.mask_base = CPCAP_REG_INTM1,
+		.type_base = CPCAP_REG_INTS1,
+		.use_ack = true,
+	},
+};
+
+static int cpcap_init_irq_chip(struct cpcap_ddata *cpcap, int irq_chip,
+			       int irq_start, int nr_irqs)
+{
+	struct regmap_irq_chip *chip = &cpcap_irq_chip[irq_chip];
+	int i, ret;
+
+	for (i = irq_start; i < irq_start + nr_irqs; i++) {
+		struct regmap_irq *cpcap_irq = &cpcap->irqs[i];
+
+		cpcap_irq->reg_offset =
+			((i - irq_start) / cpcap->regmap_conf->val_bits) *
+			cpcap->regmap_conf->reg_stride;
+		cpcap_irq->mask = BIT(i % cpcap->regmap_conf->val_bits);
+	}
+	chip->irqs = &cpcap->irqs[irq_start];
+	chip->num_irqs = nr_irqs;
+	chip->irq_drv_data = cpcap;
+
+	ret = devm_regmap_add_irq_chip(&cpcap->spi->dev, cpcap->regmap,
+				       cpcap->spi->irq,
+				       IRQF_TRIGGER_RISING |
+				       IRQF_SHARED, -1,
+				       chip, &cpcap->irqdata[irq_chip]);
+	if (ret) {
+		dev_err(&cpcap->spi->dev, "could not add irq chip %i: %i\n",
+			irq_chip, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cpcap_init_irq(struct cpcap_ddata *cpcap)
+{
+	int ret;
+
+	cpcap->irqs = devm_kzalloc(&cpcap->spi->dev,
+				   sizeof(*cpcap->irqs) *
+				   CPCAP_NR_IRQ_REG_BANKS *
+				   cpcap->regmap_conf->val_bits,
+				   GFP_KERNEL);
+	if (!cpcap->irqs)
+		return -ENOMEM;
+
+	ret = cpcap_init_irq_chip(cpcap, 0, 0, 16);
+	if (ret)
+		return ret;
+
+	ret = cpcap_init_irq_chip(cpcap, 1, 16, 16);
+	if (ret)
+		return ret;
+
+	ret = cpcap_init_irq_chip(cpcap, 2, 32, 64);
+	if (ret)
+		return ret;
+
+	enable_irq_wake(cpcap->spi->irq);
+
+	return 0;
+}
+
+static const struct of_device_id cpcap_of_match[] = {
+	{ .compatible = "motorola,cpcap", },
+	{ .compatible = "st,6556002", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cpcap_of_match);
+
+static const struct regmap_config cpcap_regmap_config = {
+	.reg_bits = 16,
+	.reg_stride = 4,
+	.pad_bits = 0,
+	.val_bits = 16,
+	.write_flag_mask = 0x8000,
+	.max_register = CPCAP_REG_ST_TEST2,
+	.cache_type = REGCACHE_NONE,
+	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+};
+
+static int cpcap_probe(struct spi_device *spi)
+{
+	const struct of_device_id *match;
+	int ret = -EINVAL;
+	struct cpcap_ddata *cpcap;
+
+	match = of_match_device(of_match_ptr(cpcap_of_match), &spi->dev);
+	if (!match)
+		return -ENODEV;
+
+	cpcap = devm_kzalloc(&spi->dev, sizeof(*cpcap), GFP_KERNEL);
+	if (!cpcap)
+		return -ENOMEM;
+
+	cpcap->spi = spi;
+	spi_set_drvdata(spi, cpcap);
+
+	spi->bits_per_word = 16;
+	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
+
+	ret = spi_setup(spi);
+	if (ret < 0)
+		return ret;
+
+	cpcap->regmap_conf = &cpcap_regmap_config;
+	cpcap->regmap = devm_regmap_init_spi(spi, &cpcap_regmap_config);
+	if (IS_ERR(cpcap->regmap)) {
+		ret = PTR_ERR(cpcap->regmap);
+		dev_err(&cpcap->spi->dev, "Failed to initialize regmap: %d\n",
+			ret);
+
+		return ret;
+	}
+
+	ret = cpcap_check_revision(cpcap);
+	if (ret) {
+		dev_err(&cpcap->spi->dev, "Failed to detect CPCAP: %i\n", ret);
+		return ret;
+	}
+
+	ret = cpcap_init_irq(cpcap);
+	if (ret)
+		return ret;
+
+	return of_platform_populate(spi->dev.of_node, NULL, NULL,
+				    &cpcap->spi->dev);
+}
+
+static int cpcap_remove(struct spi_device *pdev)
+{
+	struct cpcap_ddata *cpcap = spi_get_drvdata(pdev);
+
+	of_platform_depopulate(&cpcap->spi->dev);
+
+	return 0;
+}
+
+static struct spi_driver cpcap_driver = {
+	.driver = {
+		.name = "cpcap-core",
+		.of_match_table = cpcap_of_match,
+	},
+	.probe = cpcap_probe,
+	.remove = cpcap_remove,
+};
+module_spi_driver(cpcap_driver);
+
+MODULE_ALIAS("platform:cpcap");
+MODULE_DESCRIPTION("CPCAP driver");
+MODULE_AUTHOR("Tony Lindgren <tony@atomide.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/motorola-cpcap.h b/include/linux/mfd/motorola-cpcap.h
new file mode 100644
--- /dev/null
+++ b/include/linux/mfd/motorola-cpcap.h
@@ -0,0 +1,289 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Note that the register defines are based on earlier cpcap.h in
+ * Motorola Linux kernel tree Copyright (C) 2007-2009 Motorola, Inc.
+ *
+ * Rewritten for the real register offsets instead of enumeration
+ * to make the defines usable with Linux kernel regmap support
+ * Copyright (C) 2016 Tony Lindgren <tony@atomide.com>.
+ */
+
+#define CPCAP_VENDOR_ST		0
+#define CPCAP_VENDOR_TI		1
+
+#define CPCAP_REVISION_MAJOR(r)	(((r) >> 4) + 1)
+#define CPCAP_REVISION_MINOR(r)	((r) & 0xf)
+
+#define CPCAP_REVISION_1_0	0x08
+#define CPCAP_REVISION_1_1	0x09
+#define CPCAP_REVISION_2_0	0x10
+#define CPCAP_REVISION_2_1	0x11
+
+/* CPCAP registers */
+#define CPCAP_REG_INT1		0x0000	/* Interrupt 1 */
+#define CPCAP_REG_INT2		0x0004	/* Interrupt 2 */
+#define CPCAP_REG_INT3		0x0008	/* Interrupt 3 */
+#define CPCAP_REG_INT4		0x000c	/* Interrupt 4 */
+#define CPCAP_REG_INTM1		0x0010	/* Interrupt Mask 1 */
+#define CPCAP_REG_INTM2		0x0014	/* Interrupt Mask 2 */
+#define CPCAP_REG_INTM3		0x0018	/* Interrupt Mask 3 */
+#define CPCAP_REG_INTM4		0x001c	/* Interrupt Mask 4 */
+#define CPCAP_REG_INTS1		0x0020	/* Interrupt Sense 1 */
+#define CPCAP_REG_INTS2		0x0024	/* Interrupt Sense 2 */
+#define CPCAP_REG_INTS3		0x0028	/* Interrupt Sense 3 */
+#define CPCAP_REG_INTS4		0x002c	/* Interrupt Sense 4 */
+#define CPCAP_REG_ASSIGN1	0x0030	/* Resource Assignment 1 */
+#define CPCAP_REG_ASSIGN2	0x0034	/* Resource Assignment 2 */
+#define CPCAP_REG_ASSIGN3	0x0038	/* Resource Assignment 3 */
+#define CPCAP_REG_ASSIGN4	0x003c	/* Resource Assignment 4 */
+#define CPCAP_REG_ASSIGN5	0x0040	/* Resource Assignment 5 */
+#define CPCAP_REG_ASSIGN6	0x0044	/* Resource Assignment 6 */
+#define CPCAP_REG_VERSC1	0x0048	/* Version Control 1 */
+#define CPCAP_REG_VERSC2	0x004c	/* Version Control 2 */
+
+#define CPCAP_REG_MI1		0x0200	/* Macro Interrupt 1 */
+#define CPCAP_REG_MIM1		0x0204	/* Macro Interrupt Mask 1 */
+#define CPCAP_REG_MI2		0x0208	/* Macro Interrupt 2 */
+#define CPCAP_REG_MIM2		0x020c	/* Macro Interrupt Mask 2 */
+#define CPCAP_REG_UCC1		0x0210	/* UC Control 1 */
+#define CPCAP_REG_UCC2		0x0214	/* UC Control 2 */
+
+#define CPCAP_REG_PC1		0x021c	/* Power Cut 1 */
+#define CPCAP_REG_PC2		0x0220	/* Power Cut 2 */
+#define CPCAP_REG_BPEOL		0x0224	/* BP and EOL */
+#define CPCAP_REG_PGC		0x0228	/* Power Gate and Control */
+#define CPCAP_REG_MT1		0x022c	/* Memory Transfer 1 */
+#define CPCAP_REG_MT2		0x0230	/* Memory Transfer 2 */
+#define CPCAP_REG_MT3		0x0234	/* Memory Transfer 3 */
+#define CPCAP_REG_PF		0x0238	/* Print Format */
+
+#define CPCAP_REG_SCC		0x0400	/* System Clock Control */
+#define CPCAP_REG_SW1		0x0404	/* Stop Watch 1 */
+#define CPCAP_REG_SW2		0x0408	/* Stop Watch 2 */
+#define CPCAP_REG_UCTM		0x040c	/* UC Turbo Mode */
+#define CPCAP_REG_TOD1		0x0410	/* Time of Day 1 */
+#define CPCAP_REG_TOD2		0x0414	/* Time of Day 2 */
+#define CPCAP_REG_TODA1		0x0418	/* Time of Day Alarm 1 */
+#define CPCAP_REG_TODA2		0x041c	/* Time of Day Alarm 2 */
+#define CPCAP_REG_DAY		0x0420	/* Day */
+#define CPCAP_REG_DAYA		0x0424	/* Day Alarm */
+#define CPCAP_REG_VAL1		0x0428	/* Validity 1 */
+#define CPCAP_REG_VAL2		0x042c	/* Validity 2 */
+
+#define CPCAP_REG_SDVSPLL	0x0600	/* Switcher DVS and PLL */
+#define CPCAP_REG_SI2CC1	0x0604	/* Switcher I2C Control 1 */
+#define CPCAP_REG_Si2CC2	0x0608	/* Switcher I2C Control 2 */
+#define CPCAP_REG_S1C1		0x060c	/* Switcher 1 Control 1 */
+#define CPCAP_REG_S1C2		0x0610	/* Switcher 1 Control 2 */
+#define CPCAP_REG_S2C1		0x0614	/* Switcher 2 Control 1 */
+#define CPCAP_REG_S2C2		0x0618	/* Switcher 2 Control 2 */
+#define CPCAP_REG_S3C		0x061c	/* Switcher 3 Control */
+#define CPCAP_REG_S4C1		0x0620	/* Switcher 4 Control 1 */
+#define CPCAP_REG_S4C2		0x0624	/* Switcher 4 Control 2 */
+#define CPCAP_REG_S5C		0x0628	/* Switcher 5 Control */
+#define CPCAP_REG_S6C		0x062c	/* Switcher 6 Control */
+#define CPCAP_REG_VCAMC		0x0630	/* VCAM Control */
+#define CPCAP_REG_VCSIC		0x0634	/* VCSI Control */
+#define CPCAP_REG_VDACC		0x0638	/* VDAC Control */
+#define CPCAP_REG_VDIGC		0x063c	/* VDIG Control */
+#define CPCAP_REG_VFUSEC	0x0640	/* VFUSE Control */
+#define CPCAP_REG_VHVIOC	0x0644	/* VHVIO Control */
+#define CPCAP_REG_VSDIOC	0x0648	/* VSDIO Control */
+#define CPCAP_REG_VPLLC		0x064c	/* VPLL Control */
+#define CPCAP_REG_VRF1C		0x0650	/* VRF1 Control */
+#define CPCAP_REG_VRF2C		0x0654	/* VRF2 Control */
+#define CPCAP_REG_VRFREFC	0x0658	/* VRFREF Control */
+#define CPCAP_REG_VWLAN1C	0x065c	/* VWLAN1 Control */
+#define CPCAP_REG_VWLAN2C	0x0660	/* VWLAN2 Control */
+#define CPCAP_REG_VSIMC		0x0664	/* VSIM Control */
+#define CPCAP_REG_VVIBC		0x0668	/* VVIB Control */
+#define CPCAP_REG_VUSBC		0x066c	/* VUSB Control */
+#define CPCAP_REG_VUSBINT1C	0x0670	/* VUSBINT1 Control */
+#define CPCAP_REG_VUSBINT2C	0x0674	/* VUSBINT2 Control */
+#define CPCAP_REG_URT		0x0678	/* Useroff Regulator Trigger */
+#define CPCAP_REG_URM1		0x067c	/* Useroff Regulator Mask 1 */
+#define CPCAP_REG_URM2		0x0680	/* Useroff Regulator Mask 2 */
+
+#define CPCAP_REG_VAUDIOC	0x0800	/* VAUDIO Control */
+#define CPCAP_REG_CC		0x0804	/* Codec Control */
+#define CPCAP_REG_CDI		0x0808	/* Codec Digital Interface */
+#define CPCAP_REG_SDAC		0x080c	/* Stereo DAC */
+#define CPCAP_REG_SDACDI	0x0810	/* Stereo DAC Digital Interface */
+#define CPCAP_REG_TXI		0x0814	/* TX Inputs */
+#define CPCAP_REG_TXMP		0x0818	/* TX MIC PGA's */
+#define CPCAP_REG_RXOA		0x081c	/* RX Output Amplifiers */
+#define CPCAP_REG_RXVC		0x0820	/* RX Volume Control */
+#define CPCAP_REG_RXCOA		0x0824	/* RX Codec to Output Amps */
+#define CPCAP_REG_RXSDOA	0x0828	/* RX Stereo DAC to Output Amps */
+#define CPCAP_REG_RXEPOA	0x082c	/* RX External PGA to Output Amps */
+#define CPCAP_REG_RXLL		0x0830	/* RX Low Latency */
+#define CPCAP_REG_A2LA		0x0834	/* A2 Loudspeaker Amplifier */
+#define CPCAP_REG_MIPIS1	0x0838	/* MIPI Slimbus 1 */
+#define CPCAP_REG_MIPIS2	0x083c	/* MIPI Slimbus 2 */
+#define CPCAP_REG_MIPIS3	0x0840	/* MIPI Slimbus 3. */
+#define CPCAP_REG_LVAB		0x0844	/* LMR Volume and A4 Balanced. */
+
+#define CPCAP_REG_CCC1		0x0a00	/* Coulomb Counter Control 1 */
+#define CPCAP_REG_CRM		0x0a04	/* Charger and Reverse Mode */
+#define CPCAP_REG_CCCC2		0x0a08	/* Coincell and Coulomb Ctr Ctrl 2 */
+#define CPCAP_REG_CCS1		0x0a0c	/* Coulomb Counter Sample 1 */
+#define CPCAP_REG_CCS2		0x0a10	/* Coulomb Counter Sample 2 */
+#define CPCAP_REG_CCA1		0x0a14	/* Coulomb Counter Accumulator 1 */
+#define CPCAP_REG_CCA2		0x0a18	/* Coulomb Counter Accumulator 2 */
+#define CPCAP_REG_CCM		0x0a1c	/* Coulomb Counter Mode */
+#define CPCAP_REG_CCO		0x0a20	/* Coulomb Counter Offset */
+#define CPCAP_REG_CCI		0x0a24	/* Coulomb Counter Integrator */
+
+#define CPCAP_REG_ADCC1		0x0c00	/* A/D Converter Configuration 1 */
+#define CPCAP_REG_ADCC2		0x0c04	/* A/D Converter Configuration 2 */
+#define CPCAP_REG_ADCD0		0x0c08	/* A/D Converter Data 0 */
+#define CPCAP_REG_ADCD1		0x0c0c	/* A/D Converter Data 1 */
+#define CPCAP_REG_ADCD2		0x0c10	/* A/D Converter Data 2 */
+#define CPCAP_REG_ADCD3		0x0c14	/* A/D Converter Data 3 */
+#define CPCAP_REG_ADCD4		0x0c18	/* A/D Converter Data 4 */
+#define CPCAP_REG_ADCD5		0x0c1c	/* A/D Converter Data 5 */
+#define CPCAP_REG_ADCD6		0x0c20	/* A/D Converter Data 6 */
+#define CPCAP_REG_ADCD7		0x0c24	/* A/D Converter Data 7 */
+#define CPCAP_REG_ADCAL1	0x0c28	/* A/D Converter Calibration 1 */
+#define CPCAP_REG_ADCAL2	0x0c2c	/* A/D Converter Calibration 2 */
+
+#define CPCAP_REG_USBC1		0x0e00	/* USB Control 1 */
+#define CPCAP_REG_USBC2		0x0e04	/* USB Control 2 */
+#define CPCAP_REG_USBC3		0x0e08	/* USB Control 3 */
+#define CPCAP_REG_UVIDL		0x0e0c	/* ULPI Vendor ID Low */
+#define CPCAP_REG_UVIDH		0x0e10	/* ULPI Vendor ID High */
+#define CPCAP_REG_UPIDL		0x0e14	/* ULPI Product ID Low */
+#define CPCAP_REG_UPIDH		0x0e18	/* ULPI Product ID High */
+#define CPCAP_REG_UFC1		0x0e1c	/* ULPI Function Control 1 */
+#define CPCAP_REG_UFC2		0x0e20	/* ULPI Function Control 2 */
+#define CPCAP_REG_UFC3		0x0e24	/* ULPI Function Control 3 */
+#define CPCAP_REG_UIC1		0x0e28	/* ULPI Interface Control 1 */
+#define CPCAP_REG_UIC2		0x0e2c	/* ULPI Interface Control 2 */
+#define CPCAP_REG_UIC3		0x0e30	/* ULPI Interface Control 3 */
+#define CPCAP_REG_USBOTG1	0x0e34	/* USB OTG Control 1 */
+#define CPCAP_REG_USBOTG2	0x0e38	/* USB OTG Control 2 */
+#define CPCAP_REG_USBOTG3	0x0e3c	/* USB OTG Control 3 */
+#define CPCAP_REG_UIER1		0x0e40	/* USB Interrupt Enable Rising 1 */
+#define CPCAP_REG_UIER2		0x0e44	/* USB Interrupt Enable Rising 2 */
+#define CPCAP_REG_UIER3		0x0e48	/* USB Interrupt Enable Rising 3 */
+#define CPCAP_REG_UIEF1		0x0e4c	/* USB Interrupt Enable Falling 1 */
+#define CPCAP_REG_UIEF2		0x0e50	/* USB Interrupt Enable Falling 1 */
+#define CPCAP_REG_UIEF3		0x0e54	/* USB Interrupt Enable Falling 1 */
+#define CPCAP_REG_UIS		0x0e58	/* USB Interrupt Status */
+#define CPCAP_REG_UIL		0x0e5c	/* USB Interrupt Latch */
+#define CPCAP_REG_USBD		0x0e60	/* USB Debug */
+#define CPCAP_REG_SCR1		0x0e64	/* Scratch 1 */
+#define CPCAP_REG_SCR2		0x0e68	/* Scratch 2 */
+#define CPCAP_REG_SCR3		0x0e6c	/* Scratch 3 */
+
+#define CPCAP_REG_VMC		0x0eac	/* Video Mux Control */
+#define CPCAP_REG_OWDC		0x0eb0	/* One Wire Device Control */
+#define CPCAP_REG_GPIO0		0x0eb4	/* GPIO 0 Control */
+
+#define CPCAP_REG_GPIO1		0x0ebc	/* GPIO 1 Control */
+
+#define CPCAP_REG_GPIO2		0x0ec4	/* GPIO 2 Control */
+
+#define CPCAP_REG_GPIO3		0x0ecc	/* GPIO 3 Control */
+
+#define CPCAP_REG_GPIO4		0x0ed4	/* GPIO 4 Control */
+
+#define CPCAP_REG_GPIO5		0x0edc	/* GPIO 5 Control */
+
+#define CPCAP_REG_GPIO6		0x0ee4	/* GPIO 6 Control */
+
+#define CPCAP_REG_MDLC		0x1000	/* Main Display Lighting Control */
+#define CPCAP_REG_KLC		0x1004	/* Keypad Lighting Control */
+#define CPCAP_REG_ADLC		0x1008	/* Aux Display Lighting Control */
+#define CPCAP_REG_REDC		0x100c	/* Red Triode Control */
+#define CPCAP_REG_GREENC	0x1010	/* Green Triode Control */
+#define CPCAP_REG_BLUEC		0x1014	/* Blue Triode Control */
+#define CPCAP_REG_CFC		0x1018	/* Camera Flash Control */
+#define CPCAP_REG_ABC		0x101c	/* Adaptive Boost Control */
+#define CPCAP_REG_BLEDC		0x1020	/* Bluetooth LED Control */
+#define CPCAP_REG_CLEDC		0x1024	/* Camera Privacy LED Control */
+
+#define CPCAP_REG_OW1C		0x1200	/* One Wire 1 Command */
+#define CPCAP_REG_OW1D		0x1204	/* One Wire 1 Data */
+#define CPCAP_REG_OW1I		0x1208	/* One Wire 1 Interrupt */
+#define CPCAP_REG_OW1IE		0x120c	/* One Wire 1 Interrupt Enable */
+
+#define CPCAP_REG_OW1		0x1214	/* One Wire 1 Control */
+
+#define CPCAP_REG_OW2C		0x1220	/* One Wire 2 Command */
+#define CPCAP_REG_OW2D		0x1224	/* One Wire 2 Data */
+#define CPCAP_REG_OW2I		0x1228	/* One Wire 2 Interrupt */
+#define CPCAP_REG_OW2IE		0x122c	/* One Wire 2 Interrupt Enable */
+
+#define CPCAP_REG_OW2		0x1234	/* One Wire 2 Control */
+
+#define CPCAP_REG_OW3C		0x1240	/* One Wire 3 Command */
+#define CPCAP_REG_OW3D		0x1244	/* One Wire 3 Data */
+#define CPCAP_REG_OW3I		0x1248	/* One Wire 3 Interrupt */
+#define CPCAP_REG_OW3IE		0x124c	/* One Wire 3 Interrupt Enable */
+
+#define CPCAP_REG_OW3		0x1254	/* One Wire 3 Control */
+#define CPCAP_REG_GCAIC		0x1258	/* GCAI Clock Control */
+#define CPCAP_REG_GCAIM		0x125c	/* GCAI GPIO Mode */
+#define CPCAP_REG_LGDIR		0x1260	/* LMR GCAI GPIO Direction */
+#define CPCAP_REG_LGPU		0x1264	/* LMR GCAI GPIO Pull-up */
+#define CPCAP_REG_LGPIN		0x1268	/* LMR GCAI GPIO Pin */
+#define CPCAP_REG_LGMASK	0x126c	/* LMR GCAI GPIO Mask */
+#define CPCAP_REG_LDEB		0x1270	/* LMR Debounce Settings */
+#define CPCAP_REG_LGDET		0x1274	/* LMR GCAI Detach Detect */
+#define CPCAP_REG_LMISC		0x1278	/* LMR Misc Bits */
+#define CPCAP_REG_LMACE		0x127c	/* LMR Mace IC Support */
+
+#define CPCAP_REG_TEST		0x7c00	/* Test */
+
+#define CPCAP_REG_ST_TEST1	0x7d08	/* ST Test1 */
+
+#define CPCAP_REG_ST_TEST2	0x7d18	/* ST Test2 */
+
+/*
+ * Helpers for child devices to check the revision and vendor.
+ *
+ * REVISIT: No documentation for the bits below, please update
+ * to use proper names for defines when available.
+ */
+
+static inline int cpcap_get_revision(struct device *dev,
+				     struct regmap *regmap,
+				     u16 *revision)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(regmap, CPCAP_REG_VERSC1, &val);
+	if (ret) {
+		dev_err(dev, "Could not read revision\n");
+
+		return ret;
+	}
+
+	*revision = ((val >> 3) & 0x7) | ((val << 3) & 0x38);
+
+	return 0;
+}
+
+static inline int cpcap_get_vendor(struct device *dev,
+				   struct regmap *regmap,
+				   u16 *vendor)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(regmap, CPCAP_REG_VERSC1, &val);
+	if (ret) {
+		dev_err(dev, "Could not read vendor\n");
+
+		return ret;
+	}
+
+	*vendor = (val >> 6) & 0x7;
+
+	return 0;
+}
-- 
2.10.2

^ permalink raw reply

* Re: [PATCH v11 5/7] overlay: Documentation for the overlay sugar syntax
From: Frank Rowand @ 2016-11-29 16:45 UTC (permalink / raw)
  To: David Gibson
  Cc: Pantelis Antoniou, Jon Loeliger, Grant Likely, Rob Herring,
	Jan Luebbe, Sascha Hauer, Phil Elwell, Simon Glass, Maxime Ripard,
	Thomas Petazzoni, Boris Brezillon, Antoine Tenart, Stephen Boyd,
	Devicetree Compiler, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161129051042.GO13307-K0bRW+63XPQe6aEkudXLsA@public.gmane.org>

On 11/28/16 21:10, David Gibson wrote:
> On Mon, Nov 28, 2016 at 08:36:07PM -0800, Frank Rowand wrote:
>> On 11/28/16 19:10, David Gibson wrote:
>>> On Mon, Nov 28, 2016 at 06:05:39PM +0200, Pantelis Antoniou wrote:
>>>> There exists a syntactic sugar version of overlays which
>>>> make them simpler to write for the trivial case of a single target.
>>
>> It also works for multiple targets.  (See the example I provided in
>> my comment to v10.)
>>
>>
>>>>
>>>> Document it in the device tree object internals.
>>>>
>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>>
>>> I'm with Frank that I think this, rather than being regarded mere
>>> syntactic sugar, should be considered the primary way of describing
>>> overlays.
>>>
>>> Obviously we need to support the fully written out version as well.
>>
>> If we need to support the fully written out version, can we make that
>> a discouraged, non-preferred method?  Maybe require an option to
>> enable compiling this style of dts?
> 
> Yeah.  To avoid further proliferation of options, I'm thinking a
> single "backwards compat" option which would:
> 	- Use the dtb magic instead of dtb magic
> 	- Disable checks which would reject explicit creation of
> 	  __overlay__ / __symbols__ / __fixups__ nodes
> 	- Anything other special behaviour we need
> 	
>> I can imagine some reasons to support the fully written out version,
>> but can we document what those reasons are?
> 
> I believe the main one is the dts files in this format out in the
> field.  Mind you, I guess we're already requiring them to tweak how
> they declare the /plugin/ option.

It might be easy to write a program that transforms the expanded
format to the simple format.  I'll try to make some time to see
how difficult it is.  The transformation is relatively easy to
do manually, but I don't know how many dts files would need to
be converted.

-Frank

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 12/13] net: ethernet: ti: cpts: calc mult and shift from refclk freq
From: Grygorii Strashko @ 2016-11-29 16:22 UTC (permalink / raw)
  To: Richard Cochran
  Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori, linux-kernel,
	linux-omap, Rob Herring, devicetree, Murali Karicheri,
	Wingman Kwok, John Stultz, Thomas Gleixner
In-Reply-To: <20161129103453.GJ3110@localhost.localdomain>



On 11/29/2016 04:34 AM, Richard Cochran wrote:
> On Mon, Nov 28, 2016 at 05:03:36PM -0600, Grygorii Strashko wrote:
>> +static void cpts_calc_mult_shift(struct cpts *cpts)
>> +{
>> +	u64 frac, maxsec, ns;
>> +	u32 freq, mult, shift;
>> +
>> +	freq = clk_get_rate(cpts->refclk);
>> +
>> +	/* Calc the maximum number of seconds which we can run before
>> +	 * wrapping around.
>> +	 */
>> +	maxsec = cpts->cc.mask;
>> +	do_div(maxsec, freq);
>> +	if (maxsec > 600 && cpts->cc.mask > UINT_MAX)
>> +		maxsec = 600;
> 
> The reason for this test is not obvious.  Why check cc.mask against
> UINT_MAX?  Please use the comment to explain it.
> 

Yeah. This is copy paste from __clocksource_update_freq_scale(), but
I'm going to limit it to 10 sec for now, because otherwise it will result in too small
mult in case of 64bit counter.

if (maxsec > 10)
	maxsec = 10;

-- 
regards,
-grygorii

^ permalink raw reply

* [PATCH v4 9/9] clocksource/drivers/rockchip_timer: implement clocksource timer
From: Alexander Kochetkov @ 2016-11-29 16:14 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-arm-kernel, linux-rockchip
  Cc: Thomas Gleixner, Heiko Stuebner, Mark Rutland, Rob Herring,
	Russell King, Caesar Wang, Huang Tao, Daniel Lezcano,
	Alexander Kochetkov
In-Reply-To: <1480436092-10728-1-git-send-email-al.kochet@gmail.com>

The clock supplying the arm-global-timer on the rk3188 is coming from the
the cpu clock itself and thus changes its rate everytime cpufreq adjusts
the cpu frequency making this timer unsuitable as a stable clocksource
and sched clock.

The rk3188, rk3288 and following socs share a separate timer block already
handled by the rockchip-timer driver. Therefore adapt this driver to also
be able to act as clocksource and sched clock on rk3188.

In order to test clocksource you can run following commands and check
how much time it take in real. On rk3188 it take about ~45 seconds.

    cpufreq-set -f 1.6GHZ
    date; sleep 60; date

In order to use the patch you need to declare two timers in the dts
file. The first timer will be initialized as clockevent provider
and the second one as clocksource. The clockevent must be from
alive subsystem as it used as backup for the local timers at sleep
time.

The patch does not break compatibility with older device tree files.
The older device tree files contain only one timer. The timer
will be initialized as clockevent, as expected.

rk3288 (and probably anything newer) is irrelevant to this patch,
as it has the arch timer interface. This patch may be usefull
for Cortex-A9/A5 based parts.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clocksource/rockchip_timer.c |  137 +++++++++++++++++++++++++++++-----
 1 file changed, 117 insertions(+), 20 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 61c3bb1..a127822 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -11,6 +11,7 @@
 #include <linux/clockchips.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
+#include <linux/sched_clock.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
@@ -19,6 +20,8 @@
 
 #define TIMER_LOAD_COUNT0	0x00
 #define TIMER_LOAD_COUNT1	0x04
+#define TIMER_CURRENT_VALUE0	0x08
+#define TIMER_CURRENT_VALUE1	0x0C
 #define TIMER_CONTROL_REG3288	0x10
 #define TIMER_CONTROL_REG3399	0x1c
 #define TIMER_INT_STATUS	0x18
@@ -40,7 +43,19 @@ struct rk_clock_event_device {
 	struct rk_timer timer;
 };
 
+struct rk_clocksource {
+	struct clocksource cs;
+	struct rk_timer timer;
+};
+
+enum {
+	ROCKCHIP_CLKSRC_CLOCKEVENT = 0,
+	ROCKCHIP_CLKSRC_CLOCKSOURCE = 1,
+};
+
 static struct rk_clock_event_device bc_timer;
+static struct rk_clocksource cs_timer;
+static int rk_next_clksrc = ROCKCHIP_CLKSRC_CLOCKEVENT;
 
 static inline struct rk_clock_event_device*
 rk_clock_event_device(struct clock_event_device *ce)
@@ -63,11 +78,37 @@ static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
 	writel_relaxed(TIMER_ENABLE | flags, timer->ctrl);
 }
 
-static void rk_timer_update_counter(unsigned long cycles,
-				    struct rk_timer *timer)
+static void rk_timer_update_counter(u64 cycles, struct rk_timer *timer)
+{
+	u32 lower = cycles & 0xFFFFFFFF;
+	u32 upper = (cycles >> 32) & 0xFFFFFFFF;
+
+	writel_relaxed(lower, timer->base + TIMER_LOAD_COUNT0);
+	writel_relaxed(upper, timer->base + TIMER_LOAD_COUNT1);
+}
+
+static u64 notrace _rk_timer_counter_read(struct rk_timer *timer)
 {
-	writel_relaxed(cycles, timer->base + TIMER_LOAD_COUNT0);
-	writel_relaxed(0, timer->base + TIMER_LOAD_COUNT1);
+	u64 counter;
+	u32 lower;
+	u32 upper, old_upper;
+
+	upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
+	do {
+		old_upper = upper;
+		lower = readl_relaxed(timer->base + TIMER_CURRENT_VALUE0);
+		upper = readl_relaxed(timer->base + TIMER_CURRENT_VALUE1);
+	} while (upper != old_upper);
+
+	counter = upper;
+	counter <<= 32;
+	counter |= lower;
+	return counter;
+}
+
+static u64 rk_timer_counter_read(struct rk_timer *timer)
+{
+	return _rk_timer_counter_read(timer);
 }
 
 static void rk_timer_interrupt_clear(struct rk_timer *timer)
@@ -120,13 +161,46 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static cycle_t rk_timer_clocksource_read(struct clocksource *cs)
+{
+	struct rk_clocksource *_cs =
+		container_of(cs, struct rk_clocksource, cs);
+
+	return ~rk_timer_counter_read(&_cs->timer);
+}
+
+static u64 notrace rk_timer_sched_clock_read(void)
+{
+	struct rk_clocksource *_cs = &cs_timer;
+
+	return ~_rk_timer_counter_read(&_cs->timer);
+}
+
 static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 {
-	struct clock_event_device *ce = &bc_timer.ce;
-	struct rk_timer *timer = &bc_timer.timer;
+	struct clock_event_device *ce = NULL;
+	struct clocksource *cs = NULL;
+	struct rk_timer *timer = NULL;
 	struct clk *timer_clk;
 	struct clk *pclk;
 	int ret = -EINVAL, irq;
+	int clksrc;
+
+	clksrc = rk_next_clksrc;
+	rk_next_clksrc++;
+
+	switch (clksrc) {
+	case ROCKCHIP_CLKSRC_CLOCKEVENT:
+		ce = &bc_timer.ce;
+		timer = &bc_timer.timer;
+		break;
+	case ROCKCHIP_CLKSRC_CLOCKSOURCE:
+		cs = &cs_timer.cs;
+		timer = &cs_timer.timer;
+		break;
+	default:
+		return -ENODEV;
+	}
 
 	timer->base = of_iomap(np, 0);
 	if (!timer->base) {
@@ -170,26 +244,49 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_irq;
 	}
 
-	ce->name = TIMER_NAME;
-	ce->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
-		       CLOCK_EVT_FEAT_DYNIRQ;
-	ce->set_next_event = rk_timer_set_next_event;
-	ce->set_state_shutdown = rk_timer_shutdown;
-	ce->set_state_periodic = rk_timer_set_periodic;
-	ce->irq = irq;
-	ce->cpumask = cpu_possible_mask;
-	ce->rating = 250;
+	if (ce) {
+		ce->name = TIMER_NAME;
+		ce->features = CLOCK_EVT_FEAT_PERIODIC |
+			       CLOCK_EVT_FEAT_ONESHOT |
+			       CLOCK_EVT_FEAT_DYNIRQ;
+		ce->set_next_event = rk_timer_set_next_event;
+		ce->set_state_shutdown = rk_timer_shutdown;
+		ce->set_state_periodic = rk_timer_set_periodic;
+		ce->irq = irq;
+		ce->cpumask = cpu_possible_mask;
+		ce->rating = 250;
+	}
+
+	if (cs) {
+		cs->name = TIMER_NAME;
+		cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+		cs->mask = CLOCKSOURCE_MASK(64);
+		cs->read = rk_timer_clocksource_read;
+		cs->rating = 250;
+	}
 
 	rk_timer_interrupt_clear(timer);
 	rk_timer_disable(timer);
 
-	ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
-	if (ret) {
-		pr_err("Failed to initialize '%s': %d\n", TIMER_NAME, ret);
-		goto out_irq;
+	if (ce) {
+		ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER,
+				  TIMER_NAME, ce);
+		if (ret) {
+			pr_err("Failed to initialize '%s': %d\n",
+				TIMER_NAME, ret);
+			goto out_irq;
+		}
+
+		clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
 	}
 
-	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
+	if (cs) {
+		rk_timer_update_counter(U64_MAX, timer);
+		rk_timer_enable(timer, 0);
+		clocksource_register_hz(cs, timer->freq);
+		sched_clock_register(rk_timer_sched_clock_read, 64,
+				     timer->freq);
+	}
 
 	return 0;
 
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH v4 8/9] clocksource/drivers/rockchip_timer: move TIMER_INT_UNMASK out of rk_timer_enable()
From: Alexander Kochetkov @ 2016-11-29 16:14 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-arm-kernel, linux-rockchip
  Cc: Thomas Gleixner, Heiko Stuebner, Mark Rutland, Rob Herring,
	Russell King, Caesar Wang, Huang Tao, Daniel Lezcano,
	Alexander Kochetkov
In-Reply-To: <1480436092-10728-1-git-send-email-al.kochet@gmail.com>

This allow to enable timer without enabling interrupts from it.
As that mode will be used in clocksource implementation.

This is refactoring step without functional changes.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clocksource/rockchip_timer.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index a17dc61..61c3bb1 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -60,8 +60,7 @@ static inline void rk_timer_disable(struct rk_timer *timer)
 
 static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
 {
-	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       timer->ctrl);
+	writel_relaxed(TIMER_ENABLE | flags, timer->ctrl);
 }
 
 static void rk_timer_update_counter(unsigned long cycles,
@@ -83,7 +82,8 @@ static inline int rk_timer_set_next_event(unsigned long cycles,
 
 	rk_timer_disable(timer);
 	rk_timer_update_counter(cycles, timer);
-	rk_timer_enable(timer, TIMER_MODE_USER_DEFINED_COUNT);
+	rk_timer_enable(timer, TIMER_MODE_USER_DEFINED_COUNT |
+			       TIMER_INT_UNMASK);
 	return 0;
 }
 
@@ -101,7 +101,7 @@ static int rk_timer_set_periodic(struct clock_event_device *ce)
 
 	rk_timer_disable(timer);
 	rk_timer_update_counter(timer->freq / HZ - 1, timer);
-	rk_timer_enable(timer, TIMER_MODE_FREE_RUNNING);
+	rk_timer_enable(timer, TIMER_MODE_FREE_RUNNING | TIMER_INT_UNMASK);
 	return 0;
 }
 
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH v4 7/9] clocksource/drivers/rockchip_timer: low level routines take rk_timer as parameter
From: Alexander Kochetkov @ 2016-11-29 16:14 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-arm-kernel, linux-rockchip
  Cc: Thomas Gleixner, Heiko Stuebner, Mark Rutland, Rob Herring,
	Russell King, Caesar Wang, Huang Tao, Daniel Lezcano,
	Alexander Kochetkov
In-Reply-To: <1480436092-10728-1-git-send-email-al.kochet@gmail.com>

Pass rk_timer instead of clock_event_device to low lever timer routines.
So that code could be reused by clocksource implementation.

Drop rk_base() and rk_ctrl().

This is refactoring step without functional changes.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clocksource/rockchip_timer.c |   57 ++++++++++++++++------------------
 1 file changed, 27 insertions(+), 30 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 6d68d4c..a17dc61 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -53,70 +53,67 @@ static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
 	return &rk_clock_event_device(ce)->timer;
 }
 
-static inline void __iomem *rk_base(struct clock_event_device *ce)
+static inline void rk_timer_disable(struct rk_timer *timer)
 {
-	return rk_timer(ce)->base;
+	writel_relaxed(TIMER_DISABLE, timer->ctrl);
 }
 
-static inline void __iomem *rk_ctrl(struct clock_event_device *ce)
-{
-	return rk_timer(ce)->ctrl;
-}
-
-static inline void rk_timer_disable(struct clock_event_device *ce)
-{
-	writel_relaxed(TIMER_DISABLE, rk_ctrl(ce));
-}
-
-static inline void rk_timer_enable(struct clock_event_device *ce, u32 flags)
+static inline void rk_timer_enable(struct rk_timer *timer, u32 flags)
 {
 	writel_relaxed(TIMER_ENABLE | TIMER_INT_UNMASK | flags,
-		       rk_ctrl(ce));
+		       timer->ctrl);
 }
 
 static void rk_timer_update_counter(unsigned long cycles,
-				    struct clock_event_device *ce)
+				    struct rk_timer *timer)
 {
-	writel_relaxed(cycles, rk_base(ce) + TIMER_LOAD_COUNT0);
-	writel_relaxed(0, rk_base(ce) + TIMER_LOAD_COUNT1);
+	writel_relaxed(cycles, timer->base + TIMER_LOAD_COUNT0);
+	writel_relaxed(0, timer->base + TIMER_LOAD_COUNT1);
 }
 
-static void rk_timer_interrupt_clear(struct clock_event_device *ce)
+static void rk_timer_interrupt_clear(struct rk_timer *timer)
 {
-	writel_relaxed(1, rk_base(ce) + TIMER_INT_STATUS);
+	writel_relaxed(1, timer->base + TIMER_INT_STATUS);
 }
 
 static inline int rk_timer_set_next_event(unsigned long cycles,
 					  struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
-	rk_timer_update_counter(cycles, ce);
-	rk_timer_enable(ce, TIMER_MODE_USER_DEFINED_COUNT);
+	struct rk_timer *timer = rk_timer(ce);
+
+	rk_timer_disable(timer);
+	rk_timer_update_counter(cycles, timer);
+	rk_timer_enable(timer, TIMER_MODE_USER_DEFINED_COUNT);
 	return 0;
 }
 
 static int rk_timer_shutdown(struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
+	struct rk_timer *timer = rk_timer(ce);
+
+	rk_timer_disable(timer);
 	return 0;
 }
 
 static int rk_timer_set_periodic(struct clock_event_device *ce)
 {
-	rk_timer_disable(ce);
-	rk_timer_update_counter(rk_timer(ce)->freq / HZ - 1, ce);
-	rk_timer_enable(ce, TIMER_MODE_FREE_RUNNING);
+	struct rk_timer *timer = rk_timer(ce);
+
+	rk_timer_disable(timer);
+	rk_timer_update_counter(timer->freq / HZ - 1, timer);
+	rk_timer_enable(timer, TIMER_MODE_FREE_RUNNING);
 	return 0;
 }
 
 static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 {
 	struct clock_event_device *ce = dev_id;
+	struct rk_timer *timer = rk_timer(ce);
 
-	rk_timer_interrupt_clear(ce);
+	rk_timer_interrupt_clear(timer);
 
 	if (clockevent_state_oneshot(ce))
-		rk_timer_disable(ce);
+		rk_timer_disable(timer);
 
 	ce->event_handler(ce);
 
@@ -183,8 +180,8 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 	ce->cpumask = cpu_possible_mask;
 	ce->rating = 250;
 
-	rk_timer_interrupt_clear(ce);
-	rk_timer_disable(ce);
+	rk_timer_interrupt_clear(timer);
+	rk_timer_disable(timer);
 
 	ret = request_irq(irq, rk_timer_interrupt, IRQF_TIMER, TIMER_NAME, ce);
 	if (ret) {
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH v4 6/9] clocksource/drivers/rockchip_timer: split bc_timer into rk_timer and rk_clock_event_device
From: Alexander Kochetkov @ 2016-11-29 16:14 UTC (permalink / raw)
  To: linux-kernel, devicetree, linux-arm-kernel, linux-rockchip
  Cc: Thomas Gleixner, Heiko Stuebner, Mark Rutland, Rob Herring,
	Russell King, Caesar Wang, Huang Tao, Daniel Lezcano,
	Alexander Kochetkov
In-Reply-To: <1480436092-10728-1-git-send-email-al.kochet@gmail.com>

The patch move ce field out of struct bc_timer into struct
rk_clock_event_device and rename struct bc_timer to struct rk_timer.

This is refactoring step without functional changes.

Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com>
---
 drivers/clocksource/rockchip_timer.c |   33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/clocksource/rockchip_timer.c b/drivers/clocksource/rockchip_timer.c
index 23e267a..6d68d4c 100644
--- a/drivers/clocksource/rockchip_timer.c
+++ b/drivers/clocksource/rockchip_timer.c
@@ -29,18 +29,28 @@
 #define TIMER_MODE_USER_DEFINED_COUNT		(1 << 1)
 #define TIMER_INT_UNMASK			(1 << 2)
 
-struct bc_timer {
-	struct clock_event_device ce;
+struct rk_timer {
 	void __iomem *base;
 	void __iomem *ctrl;
 	u32 freq;
 };
 
-static struct bc_timer bc_timer;
+struct rk_clock_event_device {
+	struct clock_event_device ce;
+	struct rk_timer timer;
+};
+
+static struct rk_clock_event_device bc_timer;
+
+static inline struct rk_clock_event_device*
+rk_clock_event_device(struct clock_event_device *ce)
+{
+	return container_of(ce, struct rk_clock_event_device, ce);
+}
 
-static inline struct bc_timer *rk_timer(struct clock_event_device *ce)
+static inline struct rk_timer *rk_timer(struct clock_event_device *ce)
 {
-	return container_of(ce, struct bc_timer, ce);
+	return &rk_clock_event_device(ce)->timer;
 }
 
 static inline void __iomem *rk_base(struct clock_event_device *ce)
@@ -116,16 +126,17 @@ static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
 static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 {
 	struct clock_event_device *ce = &bc_timer.ce;
+	struct rk_timer *timer = &bc_timer.timer;
 	struct clk *timer_clk;
 	struct clk *pclk;
 	int ret = -EINVAL, irq;
 
-	bc_timer.base = of_iomap(np, 0);
-	if (!bc_timer.base) {
+	timer->base = of_iomap(np, 0);
+	if (!timer->base) {
 		pr_err("Failed to get base address for '%s'\n", TIMER_NAME);
 		return -ENXIO;
 	}
-	bc_timer.ctrl = bc_timer.base + ctrl_reg;
+	timer->ctrl = timer->base + ctrl_reg;
 
 	pclk = of_clk_get_by_name(np, "pclk");
 	if (IS_ERR(pclk)) {
@@ -153,7 +164,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_timer_clk;
 	}
 
-	bc_timer.freq = clk_get_rate(timer_clk);
+	timer->freq = clk_get_rate(timer_clk);
 
 	irq = irq_of_parse_and_map(np, 0);
 	if (!irq) {
@@ -181,7 +192,7 @@ static int __init rk_timer_init(struct device_node *np, u32 ctrl_reg)
 		goto out_irq;
 	}
 
-	clockevents_config_and_register(ce, bc_timer.freq, 1, UINT_MAX);
+	clockevents_config_and_register(ce, timer->freq, 1, UINT_MAX);
 
 	return 0;
 
@@ -190,7 +201,7 @@ out_irq:
 out_timer_clk:
 	clk_disable_unprepare(pclk);
 out_unmap:
-	iounmap(bc_timer.base);
+	iounmap(timer->base);
 
 	return ret;
 }
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH v4 5/9] ARM: dts: rockchip: disable arm-global-timer for rk3188
From: Alexander Kochetkov @ 2016-11-29 16:14 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: Mark Rutland, Huang Tao, Heiko Stuebner, Alexander Kochetkov,
	Daniel Lezcano, Russell King, Rob Herring, Thomas Gleixner,
	Caesar Wang
In-Reply-To: <1480436092-10728-1-git-send-email-al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

arm-global-timer can provide clockevents, clocksource and shed_clock. But
on rk3188 platform it provide only clocksource and shed_clock. clockevents
from  arm-global-timer is not used by kernel because there is another
clockevent provider with higher rating (smp-twd).

My commit from the series implement clocksource and shed_clock using
rockchip_timer. But sched clock from rk_timer is not selected by kernel
due to lower frequency than arm-global-timer, and clocksource from
rk_timer is not selected by kernel due to lower rating than
arm-global-timer. And I don't want to increase clocksource rating
because ratings greater 300 used for high frequency clocksources.

clocksource and shed_clock is quite unstable, because their rate depends
on cpu frequency. So disable arm-global-timer and use clocksource and
sched_clock from rockchip_timer.

Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/rk3188.dtsi |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index 0dc52fe..44da3d42 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -546,6 +546,7 @@
 
 &global_timer {
 	interrupts = <GIC_PPI 11 0xf04>;
+	status = "disabled";
 };
 
 &local_timer {
-- 
1.7.9.5

^ permalink raw reply related

* [PATCH v4 4/9] ARM: dts: rockchip: add timer entries to rk3188 SoC
From: Alexander Kochetkov @ 2016-11-29 16:14 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: Mark Rutland, Huang Tao, Heiko Stuebner, Alexander Kochetkov,
	Daniel Lezcano, Russell King, Rob Herring, Thomas Gleixner,
	Caesar Wang
In-Reply-To: <1480436092-10728-1-git-send-email-al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

The patch add two timers to all rk3188 based boards.

The first timer is from alive subsystem and it act as a backup
for the local timers at sleep time. It act the same as other
SoC rockchip timers already present in kernel.

The second timer is from CPU subsystem and act as replacement
for the arm-global-timer clocksource and sched clock. It run
at stable frequency 24MHz.

Signed-off-by: Alexander Kochetkov <al.kochet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/rk3188.dtsi |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/rk3188.dtsi b/arch/arm/boot/dts/rk3188.dtsi
index 31f81b2..0dc52fe 100644
--- a/arch/arm/boot/dts/rk3188.dtsi
+++ b/arch/arm/boot/dts/rk3188.dtsi
@@ -106,6 +106,22 @@
 		};
 	};
 
+	timer3: timer@2000e000 {
+		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer";
+		reg = <0x2000e000 0x20>;
+		interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru SCLK_TIMER3>, <&cru PCLK_TIMER3>;
+		clock-names = "timer", "pclk";
+	};
+
+	timer6: timer@200380a0 {
+		compatible = "rockchip,rk3188-timer", "rockchip,rk3288-timer";
+		reg = <0x200380a0 0x20>;
+		interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru SCLK_TIMER6>, <&cru PCLK_TIMER0>;
+		clock-names = "timer", "pclk";
+	};
+
 	i2s0: i2s@1011a000 {
 		compatible = "rockchip,rk3188-i2s", "rockchip,rk3066-i2s";
 		reg = <0x1011a000 0x2000>;
-- 
1.7.9.5

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox