Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH 4/6] net: ethernet: ti: cpts: add ptp pps support
From: Grygorii Strashko @ 2016-12-06 20:43 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
In-Reply-To: <20161206180857.GA20680@localhost.localdomain>



On 12/06/2016 12:08 PM, Richard Cochran wrote:
> On Wed, Nov 30, 2016 at 11:05:19AM +0100, Richard Cochran wrote:
>> Can you adjust the frequency of the keystone devices in hardware?  If
>> so, then please implement it, and just disable PPS for the CPSW.
>>
>> The only reason I used the timecounter for frequency adjustment was
>> because the am335x HW is broken.  But this shouldn't hold back other
>> newer HW without the same silicon flaws.
>
> I am talking here about the ADPLLLJ units.  Are they usable on the
> keystone?
>
> If so, please implement the frequency adjustment with them.
>

No, I think it's not impossible (at least as I know now).
i'll drop this patch for now.

By the way, I've tested am335 (BBB)+ HW_TS_PUSH + PWM.
Seems works.

-- 
regards,
-grygorii

^ permalink raw reply

* Re: [PATCH 2/6] net: ethernet: ti: cpts: add support for ext rftclk selection
From: Grygorii Strashko @ 2016-12-06 20:40 UTC (permalink / raw)
  To: Richard Cochran
  Cc: Murali Karicheri, David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA,
	Mugunthan V N, Sekhar Nori, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Wingman Kwok,
	linux-clk-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161206202558.GB23605-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>



On 12/06/2016 02:25 PM, Richard Cochran wrote:
> On Tue, Dec 06, 2016 at 01:39:40PM -0600, Grygorii Strashko wrote:
>> I come with below RFC patch. if no objection I'll move forward with it.
> 
> Thanks for following through with this!
> 
> The am335x will also need the MUX in its clock tree, won't it?
> 

Not exactly. I do not see CPTS_RFTCLK_SEL register in trm, but looks like
it's already implemented in am335 clock tree:
	cpsw_cpts_rft_clk: cpsw_cpts_rft_clk@520 {
		#clock-cells = <0>;
		compatible = "ti,mux-clock";
		clocks = <&dpll_core_m5_ck>, <&dpll_core_m4_ck>;
		reg = <0x0520>;
	};

and ssigned-clock-xx can be used to change parent in board file:

&cpsw_cpts_rft_clk {
assigned-clocks = <&cpsw_cpts_rft_clk>;
assigned-clock-parents = <&dpll_core_m4_ck>;
};


-- 
regards,
-grygorii
--
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/6] net: ethernet: ti: cpts: add support for ext rftclk selection
From: Richard Cochran @ 2016-12-06 20:25 UTC (permalink / raw)
  To: Grygorii Strashko
  Cc: Murali Karicheri, David S. Miller, netdev, Mugunthan V N,
	Sekhar Nori, linux-kernel, linux-omap, Rob Herring, devicetree,
	Wingman Kwok, linux-clk
In-Reply-To: <11994fbc-3713-6ef7-8a44-8a2442106dfc@ti.com>

On Tue, Dec 06, 2016 at 01:39:40PM -0600, Grygorii Strashko wrote:
> I come with below RFC patch. if no objection I'll move forward with it.

Thanks for following through with this!

The am335x will also need the MUX in its clock tree, won't it?


Thanks,
Richard

^ permalink raw reply

* Re: [PATCH 4/5] ARM: BCM5301X: Specify all RAM by including extra block
From: Jon Mason @ 2016-12-06 20:06 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: Florian Fainelli, Arnd Bergmann, Rob Herring, Mark Rutland,
	Russell King, Hauke Mehrtens,
	bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rafał Miłecki
In-Reply-To: <20161206171714.22738-4-zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

On Tue, Dec 06, 2016 at 06:17:13PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> 
> So far we were specifying only the first block which is always limited
> up to 128 MiB. There are many devices with 256 MiB and few with 512 MiB.

Assuming that NS is like NSP (and I'm pretty sure it is), there are 2
ways to access the first 128M of RAM, a proxy starting at address 0
and the real address.  I think you are splitting RAM by accessing it
both ways, when you really should just be accessing it at the real
address.

Thanks,
Jon

> 
> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> ---
>  arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts        | 3 ++-
>  arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts        | 3 ++-
>  arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts  | 3 ++-
>  arch/arm/boot/dts/bcm4708-netgear-r6250.dts        | 3 ++-
>  arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts     | 3 ++-
>  arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts      | 3 ++-
>  arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts        | 3 ++-
>  arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts | 3 ++-
>  arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts  | 3 ++-
>  arch/arm/boot/dts/bcm4709-asus-rt-ac87u.dts        | 3 ++-
>  arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts  | 3 ++-
>  arch/arm/boot/dts/bcm4709-netgear-r7000.dts        | 3 ++-
>  arch/arm/boot/dts/bcm4709-netgear-r8000.dts        | 3 ++-
>  arch/arm/boot/dts/bcm47094-dlink-dir-885l.dts      | 3 ++-
>  arch/arm/boot/dts/bcm47094-netgear-r8500.dts       | 3 ++-
>  15 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts b/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts
> index 112a5a8..d241cee 100644
> --- a/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts
> +++ b/arch/arm/boot/dts/bcm4708-asus-rt-ac56u.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x08000000>;
>  	};
>  
>  	leds {
> diff --git a/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts b/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts
> index 3600f56..b0e6204 100644
> --- a/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts
> +++ b/arch/arm/boot/dts/bcm4708-asus-rt-ac68u.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x08000000>;
>  	};
>  
>  	leds {
> diff --git a/arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts b/arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts
> index d49afec0..c9ba6b9 100644
> --- a/arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts
> +++ b/arch/arm/boot/dts/bcm4708-buffalo-wzr-1750dhp.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x18000000>;
>  	};
>  
>  	spi {
> diff --git a/arch/arm/boot/dts/bcm4708-netgear-r6250.dts b/arch/arm/boot/dts/bcm4708-netgear-r6250.dts
> index 8519548..b9f66c0 100644
> --- a/arch/arm/boot/dts/bcm4708-netgear-r6250.dts
> +++ b/arch/arm/boot/dts/bcm4708-netgear-r6250.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x08000000>;
>  	};
>  
>  	leds {
> diff --git a/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts b/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts
> index 6229ef2..ae0199f 100644
> --- a/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts
> +++ b/arch/arm/boot/dts/bcm4708-netgear-r6300-v2.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x08000000>;
>  	};
>  
>  	leds {
> diff --git a/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts b/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts
> index 74cfcd3..36b628b1 100644
> --- a/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts
> +++ b/arch/arm/boot/dts/bcm4708-smartrg-sr400ac.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x08000000>;
>  	};
>  
>  	leds {
> diff --git a/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts b/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts
> index 71b98cf..db8608b 100644
> --- a/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts
> +++ b/arch/arm/boot/dts/bcm47081-asus-rt-n18u.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x08000000>;
>  	};
>  
>  	leds {
> diff --git a/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts b/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts
> index 2922536..d51586d 100644
> --- a/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts
> +++ b/arch/arm/boot/dts/bcm47081-buffalo-wzr-600dhp2.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x08000000>;
>  	};
>  
>  	spi {
> diff --git a/arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts b/arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts
> index 184fd92..de041b8 100644
> --- a/arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts
> +++ b/arch/arm/boot/dts/bcm47081-buffalo-wzr-900dhp.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x08000000>;
>  	};
>  
>  	gpio-keys {
> diff --git a/arch/arm/boot/dts/bcm4709-asus-rt-ac87u.dts b/arch/arm/boot/dts/bcm4709-asus-rt-ac87u.dts
> index eac0f52..eaca687 100644
> --- a/arch/arm/boot/dts/bcm4709-asus-rt-ac87u.dts
> +++ b/arch/arm/boot/dts/bcm4709-asus-rt-ac87u.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x08000000>;
>  	};
>  
>  	leds {
> diff --git a/arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts b/arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts
> index aab39c9..b32957c 100644
> --- a/arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts
> +++ b/arch/arm/boot/dts/bcm4709-buffalo-wxr-1900dhp.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x18000000>;
>  	};
>  
>  	leds {
> diff --git a/arch/arm/boot/dts/bcm4709-netgear-r7000.dts b/arch/arm/boot/dts/bcm4709-netgear-r7000.dts
> index 7ab1176..f459a98 100644
> --- a/arch/arm/boot/dts/bcm4709-netgear-r7000.dts
> +++ b/arch/arm/boot/dts/bcm4709-netgear-r7000.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x08000000>;
>  	};
>  
>  	leds {
> diff --git a/arch/arm/boot/dts/bcm4709-netgear-r8000.dts b/arch/arm/boot/dts/bcm4709-netgear-r8000.dts
> index 56d38a3..cd13534 100644
> --- a/arch/arm/boot/dts/bcm4709-netgear-r8000.dts
> +++ b/arch/arm/boot/dts/bcm4709-netgear-r8000.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x08000000>;
>  	};
>  
>  	leds {
> diff --git a/arch/arm/boot/dts/bcm47094-dlink-dir-885l.dts b/arch/arm/boot/dts/bcm47094-dlink-dir-885l.dts
> index 7fb9270..64ded76 100644
> --- a/arch/arm/boot/dts/bcm47094-dlink-dir-885l.dts
> +++ b/arch/arm/boot/dts/bcm47094-dlink-dir-885l.dts
> @@ -21,7 +21,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x08000000>;
>  	};
>  
>  	nand: nand@18028000 {
> diff --git a/arch/arm/boot/dts/bcm47094-netgear-r8500.dts b/arch/arm/boot/dts/bcm47094-netgear-r8500.dts
> index 7ecd57c..600795e 100644
> --- a/arch/arm/boot/dts/bcm47094-netgear-r8500.dts
> +++ b/arch/arm/boot/dts/bcm47094-netgear-r8500.dts
> @@ -18,7 +18,8 @@
>  	};
>  
>  	memory {
> -		reg = <0x00000000 0x08000000>;
> +		reg = <0x00000000 0x08000000
> +		       0x88000000 0x18000000>;
>  	};
>  
>  	leds {
> -- 
> 2.10.1
> 
--
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 3/5] ARM: BCM5301X: Set GPIO enabling USB power on Netgear R7000
From: Jon Mason @ 2016-12-06 19:59 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: Florian Fainelli, Arnd Bergmann, Rob Herring, Mark Rutland,
	Russell King, Hauke Mehrtens,
	bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rafał Miłecki
In-Reply-To: <20161206171714.22738-3-zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

On Tue, Dec 06, 2016 at 06:17:12PM +0100, Rafał Miłecki wrote:
> There is one GPIO controlling power for both USB ports.
> 
> Signed-off-by: Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

Was the double Signed-off-by intentional?

> ---
>  arch/arm/boot/dts/bcm4709-netgear-r7000.dts | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/bcm4709-netgear-r7000.dts b/arch/arm/boot/dts/bcm4709-netgear-r7000.dts
> index 0225d82..7ab1176 100644
> --- a/arch/arm/boot/dts/bcm4709-netgear-r7000.dts
> +++ b/arch/arm/boot/dts/bcm4709-netgear-r7000.dts
> @@ -100,3 +100,11 @@
>  		};
>  	};
>  };
> +
> +&usb2 {
> +	vcc-gpio = <&chipcommon 0 GPIO_ACTIVE_HIGH>;
> +};
> +
> +&usb3 {
> +	vcc-gpio = <&chipcommon 0 GPIO_ACTIVE_HIGH>;
> +};
> -- 
> 2.10.1
> 
--
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 v3 3/4] [media] davinci: vpif_capture: get subdevs from DT
From: Kevin Hilman @ 2016-12-06 19:50 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Laurent Pinchart, Sakari Ailus,
	linux-media-u79uwXL29TY76Z2rM5mHXA, devicetree, Sekhar Nori,
	Axel Haslam, Bartosz Gołaszewski, Alexandre Bailon,
	David Lechner
In-Reply-To: <m2mvg9ez2h.fsf-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

On Tue, Dec 6, 2016 at 9:40 AM, Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> wrote:
> Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> writes:
>
>> On 12/01/2016 10:16 AM, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:
>>>> On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
>>>>> Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org> writes:
>>>>>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>>>>>>> Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org> writes:
>>>>>>>> On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
>>>>>>>>> Allow getting of subdevs from DT ports and endpoints.
>>>>>>>>>
>>>>>>>>> The _get_pdata() function was larely inspired by (i.e. stolen from)
>>>>>>>>> am437x-vpfe.c
>>>>>>>>>
>>>>>>>>> Signed-off-by: Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>  drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
>>>>>>>>>  include/media/davinci/vpif_types.h
>>>>>>>>>        |   9 +-
>>>>>>>>>  2 files changed, 133 insertions(+), 6 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>>> b/drivers/media/platform/davinci/vpif_capture.c index
>>>>>>>>> 94ee6cf03f02..47a4699157e7 100644
>>>>>>>>> --- a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>>> @@ -26,6 +26,8 @@
>>>>>>>>>  #include <linux/slab.h>
>>>>>>>>>
>>>>>>>>>  #include <media/v4l2-ioctl.h>
>>>>>>>>> +#include <media/v4l2-of.h>
>>>>>>>>> +#include <media/i2c/tvp514x.h>
>>>>>>>>
>>>>>>>> Do you need this header?
>>>>>>>
>>>>>>> Yes, based on discussion with Hans, since there is no DT binding for
>>>>>>> selecting the input pins of the TVP514x, I have to select it in the
>>>>>>> driver, so I need the defines from this header.  More on this below...
>>>
>>> That's really ugly :-( The problem should be fixed properly instead of adding
>>> one more offender.
>>
>> Do you have time for that, Laurent? I don't. Until that time we just need to
>> make do with this workaround.
>>
>>>
>>>>>>>>>  #include "vpif.h"
>>>>>>>>>  #include "vpif_capture.h"
>>>>>>>>> @@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
>>>>>>>>>
>>>>>>>>>        vpif_dbg(2, debug, "vpif_input_to_subdev\n");
>>>>>>>>>
>>>>>>>>> +      if (!chan_cfg)
>>>>>>>>> +              return -1;
>>>>>>>>> +      if (input_index >= chan_cfg->input_count)
>>>>>>>>> +              return -1;
>>>>>>>>>        subdev_name = chan_cfg->inputs[input_index].subdev_name;
>>>>>>>>>        if (subdev_name == NULL)
>>>>>>>>>                return -1;
>>>>>>>>> @@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
>>>>>>>>>        /* loop through the sub device list to get the sub device info
>>>>>>>>>        */
>>>>>>>>>        for (i = 0; i < vpif_cfg->subdev_count; i++) {
>>>>>>>>>                subdev_info = &vpif_cfg->subdev_info[i];
>>>>>>>>> -              if (!strcmp(subdev_info->name, subdev_name))
>>>>>>>>> +              if (subdev_info && !strcmp(subdev_info->name,
>>>>>>>>> subdev_name))
>>>>>>>>>                        return i;
>>>>>>>>>        }
>>>>>>>>>        return -1;
>>>>>>>>> @@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct
>>>>>>>>> v4l2_async_notifier *notifier,> >> >>
>>>>>>>>>  {
>>>>>>>>>        int i;
>>>>>>>>>
>>>>>>>>> +      for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
>>>>>>>>> +              struct v4l2_async_subdev *_asd = vpif_obj.config
>>>>>>>>> ->asd[i];
>>>>>>>>> +              const struct device_node *node = _asd->match.of.node;
>>>>>>>>> +
>>>>>>>>> +              if (node == subdev->of_node) {
>>>>>>>>> +                      vpif_obj.sd[i] = subdev;
>>>>>>>>> +                      vpif_obj.config->chan_config
>>>>>>>>> ->inputs[i].subdev_name =
>>>>>>>>> +                              (char *)subdev->of_node->full_name;
>>>
>>> Can subdev_name be made const instead of blindly casting the full_name pointer
>>> ? If not this is probably unsafe, and if yes it should be done :-)
>>>
>>>>>>>>> +                      vpif_dbg(2, debug,
>>>>>>>>> +                               "%s: setting input %d subdev_name =
>>>>>>>>> %s\n",
>>>>>>>>> +                               __func__, i, subdev->of_node
>>>>>>>>> ->full_name);
>>>>>>>>> +                      return 0;
>>>>>>>>> +              }
>>>>>>>>> +      }
>>>>>>>>> +
>>>>>>>>>        for (i = 0; i < vpif_obj.config->subdev_count; i++)
>>>>>>>>>                if (!strcmp(vpif_obj.config->subdev_info[i].name,
>>>>>>>>>                            subdev->name)) {
>>>>>>>>> @@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct
>>>>>>>>> v4l2_async_notifier *notifier)
>>>>>>>>>        return vpif_probe_complete();
>>>>>>>>>  }
>>>>>>>>>
>>>>>>>>> +static struct vpif_capture_config *
>>>>>>>>> +vpif_capture_get_pdata(struct platform_device *pdev)
>>>>>>>>> +{
>>>>>>>>> +      struct device_node *endpoint = NULL;
>>>>>>>>> +      struct v4l2_of_endpoint bus_cfg;
>>>>>>>>> +      struct vpif_capture_config *pdata;
>>>>>>>>> +      struct vpif_subdev_info *sdinfo;
>>>>>>>>> +      struct vpif_capture_chan_config *chan;
>>>>>>>>> +      unsigned int i;
>>>>>>>>> +
>>>>>>>>> +      dev_dbg(&pdev->dev, "vpif_get_pdata\n");
>>>>>>>>> +
>>>>>>>>> +      if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
>>>>>>>>> +              return pdev->dev.platform_data;
>>>>>>>>> +
>>>>>>>>> +      pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>>>>>>>>> +      if (!pdata)
>>>>>>>>> +              return NULL;
>>>>>>>>> +      pdata->subdev_info =
>>>>>>>>> +              devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
>>>>>>>>> +                           VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
>>>>>>>>> +
>>>>>>>>> +      if (!pdata->subdev_info)
>>>>>>>>> +              return NULL;
>>>>>>>>> +      dev_dbg(&pdev->dev, "%s\n", __func__);
>>>>>>>>> +
>>>>>>>>> +      for (i = 0; ; i++) {
>>>>>>>>> +              struct device_node *rem;
>>>>>>>>> +              unsigned int flags;
>>>>>>>>> +              int err;
>>>>>>>>> +
>>>>>>>>> +              endpoint = of_graph_get_next_endpoint(pdev
>>>>>>>>> ->dev.of_node,
>>>>>>>>> +                                                    endpoint);
>>>>>>>>> +              if (!endpoint)
>>>>>>>>> +                      break;
>>>>>>>>> +
>>>>>>>>> +              sdinfo = &pdata->subdev_info[i];
>>>>>>>>
>>>>>>>> subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.
>>>>>>>
>>>>>>> Right, I need to make the loop only go for a max of
>>>>>>> VPIF_CAPTURE_MAX_CHANNELS iterations.
>>>>>>>
>>>>>>>>> +              chan = &pdata->chan_config[i];
>>>>>>>>> +              chan->inputs = devm_kzalloc(&pdev->dev,
>>>>>>>>> +                                          sizeof(*chan->inputs) *
>>>>>>>>> +                                          VPIF_DISPLAY_MAX_CHANNELS,
>>>>>>>>> +                                          GFP_KERNEL);
>>>>>>>>> +
>>>>>>>>> +              chan->input_count++;
>>>>>>>>> +              chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;
>>>>>>>>
>>>>>>>> I wonder what's the purpose of using index i on this array as well.
>>>>>>>
>>>>>>> The number of endpoints in DT is the number of input channels
>>>>>>> configured (up to a max of VPIF_CAPTURE_MAX_CHANNELS.)
>>>>>>>
>>>>>>>> If you use that to access a corresponding entry in a different array,
>>>>>>>> I'd just create a struct that contains the port configuration and the
>>>>>>>> async sub-device. The omap3isp driver does that, for instance; see
>>>>>>>> isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if
>>>>>>>> you're interested. Up to you.
>>>>>>>
>>>>>>> OK, I'll have a look at that driver. The goal here with this series is
>>>>>>> just to get this working with DT, but also not break the existing
>>>>>>> legacy platform_device support, so I'm trying not to mess with the
>>>>>>> driver-interal data structures too much.
>>>>>>
>>>>>> Ack.
>>>>>>
>>>>>>>>> +              chan->inputs[i].input.std = V4L2_STD_ALL;
>>>>>>>>> +              chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
>>>>>>>>> +
>>>>>>>>> +              /* FIXME: need a new property? ch0:composite ch1:
>>>>>>>>> s-video */
>>>>>>>>> +              if (i == 0)
>>>>>>>>
>>>>>>>> Can you assume that the first endopoint has got a particular kind of
>>>>>>>> input? What if it's not connected?
>>>>>>>
>>>>>>> On all the boards I know of (there aren't many using this SoC), it's a
>>>>>>> safe assumption.
>>>>>>>
>>>>>>>> If this is a different physical port (not in the meaning another) in
>>>>>>>> the device, I'd use the reg property for this. Please see
>>>>>>>> Documentation/devicetree/bindings/media/video-interfaces.txt .
>>>>>>>
>>>>>>> My understanding (which is admittedly somewhat fuzzy) of the TVP514x is
>>>>>>> that it's not physically a different port.  Instead, it's just telling
>>>>>>> the TVP514x which pin(s) will be active inputs (and what kind of signal
>>>>>>> will be present.)
>>>>>>>
>>>>>>> I'm open to a better way to describe this input select from DT, but
>>>>>>> based on what I heard from Hans, there isn't currently a good way to do
>>>>>>> that except for in the driver:
>>>>>>> (c.f. https://marc.info/?l=linux-arm-kernel&m=147887871615788)
>>>>>>>
>>>>>>> Based on further discussion in that thread, it sounds like there may be
>>>>>>> a way forward coming soon, and I'll be glad to switch to that when it
>>>>>>> arrives.
>>>
>>> I'm afraid I have to disappoint Hans here, I don't have code for that yet.
>>>
>>>>>> I'm not sure that properly supporting connectors will provide any help
>>>>>> here.
>>>>>>
>>>>>> Looking at the s_routing() API, it's the calling driver that has to be
>>>>>> aware of sub-device specific function parameters. As such it's not a
>>>>>> very good idea to require that a driver is aware of the value range of
>>>>>> another driver's parameter. I wonder if a simple enumeration interface
>>>>>> would help here --- if I understand correctly, the purpose is just to
>>>>>> provide a way to choose the input using VIDIOC_S_INPUT.
>>>>>>
>>>>>> I guess that's somehow ok as long as you have no other combinations of
>>>>>> these devices but this is hardly future-proof. (And certainly not a
>>>>>> problem created by this patch.)
>>>>>
>>>>> Yeah, this is far from future proof.
>>>>>
>>>>>> It'd be still nice to fix that as presumably we don't have the option of
>>>>>> reworking how we expect the device tree to look like.
>>>>>
>>>>> Agreed.
>>>>>
>>>>> I'm just hoping someone can shed som light on "how we expect the device
>>>>> tree to look".  ;)
>>>>
>>>> :-)
>>>>
>>>> For the tvp514x, do you need more than a single endpoint on the receiver
>>>> side? Does the input that's selected affect the bus parameters?
>>>>
>>>> If it doesn't, you could create a custom endpoint property for the possible
>>>> input values. The s_routing() really should be fixed though, but that could
>>>> be postponed I guess. There are quite a few drivers using it.
>>>
>>> There's two ways to look at s_routing() in my opinion, as the calling driver
>>> should really not hardcode any knowledge specific to a particular subdev. We
>>> can either have the calling driver discover the possible routing options at
>>> runtime through the subdev API, or modify the s_routing() API.
>>>
>>
>> Some historical perspective: s_routing was added well before the device tree
>> was ever used for ARM. And at that time the vast majority of drivers were PCI
>> or USB drivers, very few platform drivers existed (and those typically used
>> sensors, not video receivers).
>>
>> Before s_routing existed the situation was even worse.
>>
>> Basically what s_routing does is a poor-man's device tree entry, telling the
>> subdev how to route video or audio from connector to the output of the chip.
>> Typically the card tables in PCI or USB drivers contain the correct arguments
>> for s_routing. Of course, today we'd do that with the DT, but that was not an
>> option years ago.
>
> So I'm still confused on the path forward here.
>
> I do not have the time (or the V4L2 knowledge/experience) to rework the
> V4L2 internals to make this work, but I'm happy to test if someone else
> is working on it.
>
> In the meantime, what do we do with this series?  I have a couple minor
> things to fixup based on review comments, but other than that, the
> s_routing decision is blocking this from getting an update for use on DT
> platforms.
>
> The alternative is to go the OMAP route for legacy drivers like this and
> just use pdata quirks for passing the legacy pdata (which has the input
> and output routes hard-coded in platform_data).

Also, FYI, I have the same issue with the output/display side of this
controller.  It's using an I2C-connected adv7343, where the input and
output routes are configured by the driver using s_routing, and the
current code passes the routes in using platform_data.

Kevin
--
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 v9 1/2] usb: dwc2: assert phy reset when waking up in rk3288 platform
From: John Youn @ 2016-12-06 19:43 UTC (permalink / raw)
  To: Ayaka, John Youn
  Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	kishon-l0cyMroinI0@public.gmane.org,
	felipe.balbi-VuQAYsv1563Yd54FQh9/CA@public.gmane.org,
	mark.rutland-5wv7dgnIgG8@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	randy.li-TNX95d0MmH7DzftRWevZcw@public.gmane.org
In-Reply-To: <AAC04FF8-1049-4153-AC77-90A423B69181-xPW3/0Ywev/iB9QmIjCX8w@public.gmane.org>

On 12/6/2016 4:00 AM, Ayaka wrote:
> Hello John
>   I still waiting them be merged, but I still can't find it at next-20161206.
> 

Can you resubmit this fixing the checkpatch issues?

You can add my ack:

Acked-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>

Regards,
John


> 從我的 iPad 傳送
> 
>> John Youn <John.Youn-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> 於 2016年10月25日 上午9:30 寫道:
>>
>>> On 10/23/2016 2:33 AM, ayaka wrote:
>>>
>>>
>>>> On 10/22/2016 03:27 AM, John Youn wrote:
>>>>> On 10/20/2016 11:38 AM, Randy Li wrote:
>>>>> On the rk3288 USB host-only port (the one that's not the OTG-enabled
>>>>> port) the PHY can get into a bad state when a wakeup is asserted (not
>>>>> just a wakeup from full system suspend but also a wakeup from
>>>>> autosuspend).
>>>>>
>>>>> We can get the PHY out of its bad state by asserting its "port reset",
>>>>> but unfortunately that seems to assert a reset onto the USB bus so it
>>>>> could confuse things if we don't actually deenumerate / reenumerate the
>>>>> device.
>>>>>
>>>>> We can also get the PHY out of its bad state by fully resetting it using
>>>>> the reset from the CRU (clock reset unit) in chip, which does a more full
>>>>> reset.  The CRU-based reset appears to actually cause devices on the bus
>>>>> to be removed and reinserted, which fixes the problem (albeit in a hacky
>>>>> way).
>>>>>
>>>>> It's unfortunate that we need to do a full re-enumeration of devices at
>>>>> wakeup time, but this is better than alternative of letting the bus get
>>>>> wedged.
>>>>>
>>>>> Signed-off-by: Randy Li <ayaka-xPW3/0Ywev/iB9QmIjCX8w@public.gmane.org>
>>>>> ---
>>>>>  drivers/usb/dwc2/core.h      |  1 +
>>>>>  drivers/usb/dwc2/core_intr.c | 11 +++++++++++
>>>>>  drivers/usb/dwc2/platform.c  |  9 +++++++++
>>>>>  3 files changed, 21 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>>>>> index 2a21a04..e91ddbc 100644
>>>>> --- a/drivers/usb/dwc2/core.h
>>>>> +++ b/drivers/usb/dwc2/core.h
>>>>> @@ -859,6 +859,7 @@ struct dwc2_hsotg {
>>>>>      unsigned int ll_hw_enabled:1;
>>>>>
>>>>>      struct phy *phy;
>>>>> +    struct work_struct phy_rst_work;
>>>>>      struct usb_phy *uphy;
>>>>>      struct dwc2_hsotg_plat *plat;
>>>>>      struct regulator_bulk_data supplies[ARRAY_SIZE(dwc2_hsotg_supply_names)];
>>>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>>>> index d85c5c9..c3d2168 100644
>>>>> --- a/drivers/usb/dwc2/core_intr.c
>>>>> +++ b/drivers/usb/dwc2/core_intr.c
>>>>> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>>>>>  static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>>>>  {
>>>>>      int ret;
>>>>> +    struct device_node *np = hsotg->dev->of_node;
>>>>>
>>>>>      /* Clear interrupt */
>>>>>      dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
>>>>> @@ -379,6 +380,16 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>>>>              /* Restart the Phy Clock */
>>>>>              pcgcctl &= ~PCGCTL_STOPPCLK;
>>>>>              dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
>>>>> +
>>>>> +            /*
>>>>> +             * It is a quirk in Rockchip RK3288, causing by
>>>>> +             * a hardware bug. This will propagate out and
>>>>> +             * eventually we'll re-enumerate the device.
>>>>> +             * Not great but the best we can do
>>>>> +             */
>>>>> +            if (of_device_is_compatible(np, "rockchip,rk3288-usb"))
>>>>> +                schedule_work(&hsotg->phy_rst_work);
>>>>> +
>>>>>              mod_timer(&hsotg->wkp_timer,
>>>>>                    jiffies + msecs_to_jiffies(71));
>>>>>          } else {
>>>>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>>>>> index 8e1728b..65953cf 100644
>>>>> --- a/drivers/usb/dwc2/platform.c
>>>>> +++ b/drivers/usb/dwc2/platform.c
>>>>> @@ -366,6 +366,14 @@ int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
>>>>>      return ret;
>>>>>  }
>>>>>
>>>>> +/* Only used to reset usb phy at interrupter runtime */
>>>>> +static void dwc2_reset_phy_work(struct work_struct *data)
>>>>> +{
>>>>> +    struct dwc2_hsotg *hsotg = container_of(data, struct dwc2_hsotg,
>>>>> +            phy_rst_work);
>>>>> +    phy_reset(hsotg->phy);
>>>>> +}
>>>>> +
>>>>>  static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
>>>>>  {
>>>>>      int i, ret;
>>>>> @@ -410,6 +418,7 @@ static int dwc2_lowlevel_hw_init(struct dwc2_hsotg *hsotg)
>>>>>              return ret;
>>>>>          }
>>>>>      }
>>>>> +    INIT_WORK(&hsotg->phy_rst_work, dwc2_reset_phy_work);
>>>>>
>>>>>      if (!hsotg->phy) {
>>>>>          hsotg->uphy = devm_usb_get_phy(hsotg->dev, USB_PHY_TYPE_USB2);
>>>>>
>>>> Hi Randy,
>>>>
>>>> This fails compile if CONFIG_GENERIC_PHY is disabled. I think you need
>>>> to make a fix to your phy_reset patch first.
>>> In the last time, cac18ecb6f44b11bc303d7afbae3887b27938fa4 have not been 
>>> merged, I though the
>>> [PATCH v8 1/3] phy: Add reset callback for not generic phy have been 
>>> merged before that. when the rebase abandon it.
>>> Should re-send that patch? As the mainline have not been affected, could 
>>> you arrange a squash for it?
>>
>> Hi Randy,
>>
>> Can you resend the fix to Kishon?
>>
>> The phy_reset patch landed in 4.9-rc so I think he should take the fix
>> for the next -rc. After that we can send the dwc2 change through
>> Felipe.
>>
>> Regards,
>> John
> 
> 



--
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/6] net: ethernet: ti: cpts: add support for ext rftclk selection
From: Grygorii Strashko @ 2016-12-06 19:39 UTC (permalink / raw)
  To: Richard Cochran, Murali Karicheri
  Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori, linux-kernel,
	linux-omap, Rob Herring, devicetree, Wingman Kwok, linux-clk
In-Reply-To: <bf207ac4-0f9f-d4a7-14ac-704d93fb7764@ti.com>



On 11/30/2016 11:35 AM, Grygorii Strashko wrote:
>
>
> On 11/30/2016 03:56 AM, Richard Cochran wrote:
>> On Mon, Nov 28, 2016 at 05:04:24PM -0600, Grygorii Strashko wrote:
>>> Some CPTS instances, which can be found on KeyStone 2 1/10G Ethernet
>>> Switch Subsystems, can control an external multiplexer that selects
>>> one of up to 32 clocks for time sync reference (RFTCLK). This feature
>>> can be configured through CPTS_RFTCLK_SEL register (offset: x08).
>>>
>>> Hence, introduce optional DT cpts_rftclk_sel poperty wich, if present,
>>> will specify CPTS reference clock. The cpts_rftclk_sel should be
>>> omitted in DT if HW doesn't support this feature. The external fixed
>>> rate clocks can be defined in board files as "fixed-clock".
>>
>> Can't you implement this using the clock tree, rather than an ad-hoc
>> DT property?
>>
>
> I've thought about this, but decided to move forward with this impl
>  which is pretty simple. I will try.
>
>

I come with below RFC patch. if no objection I'll move forward with it.

According to Keystone 2 66AK2e DM there are 7 possible ref clocks:
0000 = SYSCLK2
0001 = SYSCLK3
0010 = TIMI0
0011 = TIMI1
0100 = TSIPCLKA
1000 = TSREFCLK
1100 = TSIPCLKB
Others = Reserved

2 from above clocks are internal SYSCLK2 and SYSCLK3 - other external
(board specific). So default definition of cpts_mux will have only two parents.
If ext clock is going to be use as cpts rftclk then it should be
defined in board file and  cpts_refclk_mux definition updated to support 
this ext clock:

timi1clk: timi1clk {
	#clock-cells = <0>;
	compatible = "fixed-clock";
	clock-frequency = <xxxxxxxxxx>;
	clock-output-names = "timi1";
};

&cpts_mux {
	clocks = <&chipclk12>, <&chipclk13>, <timi1clk>;
	cpts-mux-tbl = <0>, <1>, <3>;
	assigned-clocks = <&cpts_mux>;
	assigned-clock-parents = <&timi1clk>;
};



>From ec5c7bed0e021c2ca7e9392173bf67bb9a45d0f4 Mon Sep 17 00:00:00 2001
From: Grygorii Strashko <grygorii.strashko@ti.com>
Date: Mon, 5 Dec 2016 12:34:45 -0600
Subject: [PATCH] cpts refclk sel

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 arch/arm/boot/dts/keystone-k2e-netcp.dtsi | 10 +++++-
 drivers/net/ethernet/ti/cpts.c            | 52 ++++++++++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/keystone-k2e-netcp.dtsi b/arch/arm/boot/dts/keystone-k2e-netcp.dtsi
index 919e655..b27aa22 100644
--- a/arch/arm/boot/dts/keystone-k2e-netcp.dtsi
+++ b/arch/arm/boot/dts/keystone-k2e-netcp.dtsi
@@ -138,7 +138,7 @@ netcp: netcp@24000000 {
 	/* NetCP address range */
 	ranges = <0 0x24000000 0x1000000>;

-	clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
+	clocks = <&clkpa>, <&clkcpgmac>, <&cpts_mux>;
 	clock-names = "pa_clk", "ethss_clk", "cpts";
 	dma-coherent;

@@ -162,6 +162,14 @@ netcp: netcp@24000000 {
 			cpts-ext-ts-inputs = <6>;
 			cpts-ts-comp-length;

+			cpts_mux: cpts_refclk_mux {
+				#clock-cells = <0>;
+				clocks = <&chipclk12>, <&chipclk13>;
+				cpts-mux-tbl = <0>, <1>;
+				assigned-clocks = <&cpts_mux>;
+				assigned-clock-parents = <&chipclk12>;
+			};
+
 			interfaces {
 				gbe0: interface-0 {
 					slave-port = <0>;
diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
index 938de22..ef94316 100644
--- a/drivers/net/ethernet/ti/cpts.c
+++ b/drivers/net/ethernet/ti/cpts.c
@@ -17,6 +17,7 @@
  * along with this program; if not, write to the Free Software
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301  USA
  */
+#include <linux/clk-provider.h>
 #include <linux/err.h>
 #include <linux/if.h>
 #include <linux/hrtimer.h>
@@ -672,6 +673,7 @@ int cpts_register(struct cpts *cpts)
 	cpts->phc_index = ptp_clock_index(cpts->clock);

 	schedule_delayed_work(&cpts->overflow_work, cpts->ov_check_period);
+
 	return 0;

 err_ptp:
@@ -741,6 +743,54 @@ static void cpts_calc_mult_shift(struct cpts *cpts)
 		 freq, cpts->cc_mult, cpts->cc.shift, (ns - NSEC_PER_SEC));
 }

+static int cpts_of_mux_clk_setup(struct cpts *cpts, struct device_node *node)
+{
+	unsigned int num_parents;
+	const char **parent_names;
+	struct device_node *refclk_np;
+	void __iomem *reg;
+	struct clk *clk;
+	u32 *mux_table;
+	int ret;
+
+	refclk_np = of_get_child_by_name(node, "cpts_refclk_mux");
+	if (!refclk_np)
+		return -EINVAL;
+
+	num_parents = of_clk_get_parent_count(refclk_np);
+	if (num_parents < 1) {
+		dev_err(cpts->dev, "mux-clock %s must have parents\n",
+			refclk_np->name);
+		return -EINVAL;
+	}
+	parent_names = devm_kzalloc(cpts->dev, (sizeof(char *) * num_parents),
+				    GFP_KERNEL);
+	if (!parent_names)
+		return -ENOMEM;
+
+	of_clk_parent_fill(refclk_np, parent_names, num_parents);
+
+	mux_table = devm_kzalloc(cpts->dev, sizeof(*mux_table) * (32 + 1),
+				 GFP_KERNEL);
+	if (!mux_table)
+		return -ENOMEM;
+
+	ret = of_property_read_variable_u32_array(refclk_np, "cpts-mux-tbl",
+						  mux_table, 1, 32);
+	if (ret < 0)
+		return ret;
+
+	reg = &cpts->reg->rftclk_sel;
+
+	clk = clk_register_mux_table(cpts->dev, refclk_np->name,
+				     parent_names, num_parents,
+				     0, reg, 0, 0x1F, 0, mux_table, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	return of_clk_add_provider(refclk_np, of_clk_src_simple_get, clk);
+}
+
 static int cpts_of_parse(struct cpts *cpts, struct device_node *node)
 {
 	int ret = -EINVAL;
@@ -787,7 +837,7 @@ static int cpts_of_parse(struct cpts *cpts, struct device_node *node)
 	if (!of_property_read_u32(node, "cpts-ext-ts-inputs", &prop))
 		cpts->ext_ts_inputs = prop;

-	return 0;
+	return cpts_of_mux_clk_setup(cpts, node);

 of_error:
 	dev_err(cpts->dev, "CPTS: Missing property in the DT.\n");
-- 
2.10.1



-- 
regards,
-grygorii

^ permalink raw reply related

* Re: [PATCH 2/2] arm64: dts: NS2: add support for XMC form factor
From: Jon Mason @ 2016-12-06 19:35 UTC (permalink / raw)
  To: kbuild test robot
  Cc: kbuild-all, Rob Herring, Mark Rutland, Florian Fainelli,
	devicetree, bcm-kernel-feedback-list, linux-arm-kernel,
	linux-kernel
In-Reply-To: <201612061647.t3qcDWHR%fengguang.wu@intel.com>

On Tue, Dec 06, 2016 at 04:37:48PM +0800, kbuild test robot wrote:
> Hi Jon,
> 
> [auto build test ERROR on robh/for-next]
> [also build test ERROR on v4.9-rc8]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Jon-Mason/arm64-dts-NS2-reserve-memory-for-Nitro-firmware/20161206-125631
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> config: arm64-allmodconfig (attached as .config)
> compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=arm64 
> 
> All errors (new ones prefixed by >>):
> 
> >> Error: arch/arm64/boot/dts/broadcom/ns2-xmc.dts:56.1-6 Label or path enet not found
> >> Error: arch/arm64/boot/dts/broadcom/ns2-xmc.dts:132.1-7 Label or path pcie8 not found
> >> Error: arch/arm64/boot/dts/broadcom/ns2-xmc.dts:148.1-6 Label or path qspi not found
> >> FATAL ERROR: Syntax error parsing input tree

This patch series is based on top of others queued in Florian's tree.
The errors referenced above are not present when based on this tree,
as those labels are already in the NS2 DT files there.

Thanks,
Jon

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

^ permalink raw reply

* Re: [PATCH 5/5] arm64: dts: exynos5433: Add support of bus frequency using VDD_INT on TM2
From: Krzysztof Kozlowski @ 2016-12-06 19:24 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: krzk-DgEjT+Ai2ygdnm+yROfE0A, javier-JPH+aEBZ4P+UEJcrhfAQsw,
	kgene-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ,
	tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w,
	myungjoo.ham-Sze3O3UU22JBDgjK7y7TUQ,
	kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480663087-4590-6-git-send-email-cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

On Fri, Dec 02, 2016 at 04:18:07PM +0900, Chanwoo Choi wrote:
> This patch adds the bus Device-tree nodes for INT (Internal) block
> to enable the bus frequency scaling.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  arch/arm64/boot/dts/exynos/exynos5433-tm2.dts | 72 +++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2.dts b/arch/arm64/boot/dts/exynos/exynos5433-tm2.dts
> index c08589970134..7b37aae336b1 100644
> --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2.dts
> +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2.dts
> @@ -170,6 +170,58 @@
>  	};
>  };
>  
> +&bus_g2d_400 {
> +	devfreq-events = <&ppmu_event0_d0_general>, <&ppmu_event0_d1_general>;
> +	vdd-supply = <&buck4_reg>;
> +	exynos,saturation-ratio = <10>;
> +	status = "okay";
> +};
> +
> +&bus_mscl {
> +	devfreq = <&bus_g2d_400>;
> +	status = "okay";
> +};
> +
> +&bus_jpeg {

Except the first entry (which is a parent), are there any objections to
order these nodes alphabetically? This also applies to the previously
patch.

Beside that nit, looks good. I will have to wait anyway to next merge
window, so for the reference:

Reviewed-by: Krzysztof Kozlowski <krzk-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Best regards,
Krzysztof


> +	devfreq = <&bus_g2d_400>;
> +	status = "okay";
> +};
> +
> +&bus_mfc {
> +	devfreq = <&bus_g2d_400>;
> +	status = "okay";
> +};
> +
> +&bus_g2d_266 {
> +	devfreq = <&bus_g2d_400>;
> +	status = "okay";
> +};
> +
> +&bus_gscl {
> +	devfreq = <&bus_g2d_400>;
> +	status = "okay";
> +};
> +
> +&bus_hevc {
> +	devfreq = <&bus_g2d_400>;
> +	status = "okay";
> +};
> +
> +&bus_bus0 {
> +	devfreq = <&bus_g2d_400>;
> +	status = "okay";
> +};
> +
> +&bus_bus1 {
> +	devfreq = <&bus_g2d_400>;
> +	status = "okay";
> +};
> +
> +&bus_bus2 {
> +	devfreq = <&bus_g2d_400>;
> +	status = "okay";
> +};
> +
>  &cmu_aud {
>  	assigned-clocks = <&cmu_aud CLK_MOUT_AUD_PLL_USER>;
>  	assigned-clock-parents = <&cmu_top CLK_FOUT_AUD_PLL>;
> @@ -794,6 +846,26 @@
>  	bus-width = <4>;
>  };
>  
> +&ppmu_d0_general {
> +	status = "okay";
> +
> +	events {
> +		ppmu_event0_d0_general: ppmu-event0-d0-general {
> +			event-name = "ppmu-event0-d0-general";
> +		};
> +	};
> +};
> +
> +&ppmu_d1_general {
> +	status = "okay";
> +
> +	events {
> +		ppmu_event0_d1_general: ppmu-event0-d1-general {
> +		       event-name = "ppmu-event0-d1-general";
> +	       };
> +       };
> +};
> +
>  &pinctrl_alive {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&initial_alive>;
> -- 
> 1.9.1
> 
--
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

* [PATCH v2 2/2] ASoC: atmel: tse850: rely on the ssc to register as a cpu dai by itself
From: Peter Rosin @ 2016-12-06 19:22 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mark Rutland, devicetree, alsa-devel, Arnd Bergmann,
	Greg Kroah-Hartman, Takashi Iwai, Nicolas Ferre, Liam Girdwood,
	Rob Herring, Jaroslav Kysela, Mark Brown, Peter Rosin,
	linux-arm-kernel
In-Reply-To: <1481052157-23400-1-git-send-email-peda@axentia.se>

This breaks devicetree compatibility, but in this case that is ok. All
affected units are either on my desk, or running an even older version
of the driver that is not compatible with the upstreamed version anyway
(and when these other units are eventually updated, they will get a
fresh dtb as well, so that is not a significant problem either).

All of that is of course assuming that noone else has managed to build
something that can use this driver, but that seems extremely improbable.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../bindings/sound/axentia,tse850-pcm5142.txt      | 11 ++++++++---
 sound/soc/atmel/tse850-pcm5142.c                   | 23 +++-------------------
 2 files changed, 11 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/axentia,tse850-pcm5142.txt b/Documentation/devicetree/bindings/sound/axentia,tse850-pcm5142.txt
index 5b9b38f578bb..fdb25b492514 100644
--- a/Documentation/devicetree/bindings/sound/axentia,tse850-pcm5142.txt
+++ b/Documentation/devicetree/bindings/sound/axentia,tse850-pcm5142.txt
@@ -2,8 +2,7 @@ Devicetree bindings for the Axentia TSE-850 audio complex
 
 Required properties:
   - compatible: "axentia,tse850-pcm5142"
-  - axentia,ssc-controller: The phandle of the atmel SSC controller used as
-    cpu dai.
+  - axentia,cpu-dai: The phandle of the cpu dai.
   - axentia,audio-codec: The phandle of the PCM5142 codec.
   - axentia,add-gpios: gpio specifier that controls the mixer.
   - axentia,loop1-gpios: gpio specifier that controls loop relays on channel 1.
@@ -43,6 +42,12 @@ the PCM5142 codec.
 
 Example:
 
+	&ssc0 {
+		#sound-dai-cells = <0>;
+
+		status = "okay";
+	};
+
 	&i2c {
 		codec: pcm5142@4c {
 			compatible = "ti,pcm5142";
@@ -77,7 +82,7 @@ Example:
 	sound {
 		compatible = "axentia,tse850-pcm5142";
 
-		axentia,ssc-controller = <&ssc0>;
+		axentia,cpu-dai = <&ssc0>;
 		axentia,audio-codec = <&codec>;
 
 		axentia,add-gpios = <&pioA 8 GPIO_ACTIVE_LOW>;
diff --git a/sound/soc/atmel/tse850-pcm5142.c b/sound/soc/atmel/tse850-pcm5142.c
index ac6a814c8ecf..a72c7d642026 100644
--- a/sound/soc/atmel/tse850-pcm5142.c
+++ b/sound/soc/atmel/tse850-pcm5142.c
@@ -51,11 +51,7 @@
 #include <sound/soc.h>
 #include <sound/pcm_params.h>
 
-#include "atmel_ssc_dai.h"
-
 struct tse850_priv {
-	int ssc_id;
-
 	struct gpio_desc *add;
 	struct gpio_desc *loop1;
 	struct gpio_desc *loop2;
@@ -329,23 +325,20 @@ static int tse850_dt_init(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *codec_np, *cpu_np;
-	struct snd_soc_card *card = &tse850_card;
 	struct snd_soc_dai_link *dailink = &tse850_dailink;
-	struct tse850_priv *tse850 = snd_soc_card_get_drvdata(card);
 
 	if (!np) {
 		dev_err(&pdev->dev, "only device tree supported\n");
 		return -EINVAL;
 	}
 
-	cpu_np = of_parse_phandle(np, "axentia,ssc-controller", 0);
+	cpu_np = of_parse_phandle(np, "axentia,cpu-dai", 0);
 	if (!cpu_np) {
-		dev_err(&pdev->dev, "failed to get dai and pcm info\n");
+		dev_err(&pdev->dev, "failed to get cpu dai\n");
 		return -EINVAL;
 	}
 	dailink->cpu_of_node = cpu_np;
 	dailink->platform_of_node = cpu_np;
-	tse850->ssc_id = of_alias_get_id(cpu_np, "ssc");
 	of_node_put(cpu_np);
 
 	codec_np = of_parse_phandle(np, "axentia,audio-codec", 0);
@@ -415,23 +408,14 @@ static int tse850_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = atmel_ssc_set_audio(tse850->ssc_id);
-	if (ret != 0) {
-		dev_err(dev,
-			"failed to set SSC %d for audio\n", tse850->ssc_id);
-		goto err_disable_ana;
-	}
-
 	ret = snd_soc_register_card(card);
 	if (ret) {
 		dev_err(dev, "snd_soc_register_card failed\n");
-		goto err_put_audio;
+		goto err_disable_ana;
 	}
 
 	return 0;
 
-err_put_audio:
-	atmel_ssc_put_audio(tse850->ssc_id);
 err_disable_ana:
 	regulator_disable(tse850->ana);
 	return ret;
@@ -443,7 +427,6 @@ static int tse850_remove(struct platform_device *pdev)
 	struct tse850_priv *tse850 = snd_soc_card_get_drvdata(card);
 
 	snd_soc_unregister_card(card);
-	atmel_ssc_put_audio(tse850->ssc_id);
 	regulator_disable(tse850->ana);
 
 	return 0;
-- 
2.1.4

^ permalink raw reply related

* [PATCH v2 1/2] misc: atmel-ssc: register as sound DAI if #sound-dai-cells is present
From: Peter Rosin @ 2016-12-06 19:22 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mark Rutland, devicetree, alsa-devel, Arnd Bergmann,
	Greg Kroah-Hartman, Takashi Iwai, Nicolas Ferre, Liam Girdwood,
	Rob Herring, Jaroslav Kysela, Mark Brown, Peter Rosin,
	linux-arm-kernel
In-Reply-To: <1481052157-23400-1-git-send-email-peda@axentia.se>

The SSC is currently not usable with the ASoC simple-audio-card, as
every SSC audio user has to build a platform driver that may do as
little as calling atmel_ssc_set_audio/atmel_ssc_put_audio (which
allocates the SSC and registers a DAI with the ASoC subsystem).

So, have that happen automatically, if the #sound-dai-cells property
is present in devicetree, which it has to be anyway for simple audio
card to work.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 .../devicetree/bindings/misc/atmel-ssc.txt         |  2 +
 drivers/misc/atmel-ssc.c                           | 50 ++++++++++++++++++++++
 include/linux/atmel-ssc.h                          |  1 +
 3 files changed, 53 insertions(+)

diff --git a/Documentation/devicetree/bindings/misc/atmel-ssc.txt b/Documentation/devicetree/bindings/misc/atmel-ssc.txt
index efc98ea1f23d..f8629bb73945 100644
--- a/Documentation/devicetree/bindings/misc/atmel-ssc.txt
+++ b/Documentation/devicetree/bindings/misc/atmel-ssc.txt
@@ -24,6 +24,8 @@ Optional properties:
        this parameter to choose where the clock from.
      - By default the clock is from TK pin, if the clock from RK pin, this
        property is needed.
+  - #sound-dai-cells: Should contain <0>.
+     - This property makes the SSC into an automatically registered DAI.
 
 Examples:
 - PDC transfer:
diff --git a/drivers/misc/atmel-ssc.c b/drivers/misc/atmel-ssc.c
index 0516ecda54d3..b2a0340f277e 100644
--- a/drivers/misc/atmel-ssc.c
+++ b/drivers/misc/atmel-ssc.c
@@ -20,6 +20,8 @@
 
 #include <linux/of.h>
 
+#include "../../sound/soc/atmel/atmel_ssc_dai.h"
+
 /* Serialize access to ssc_list and user count */
 static DEFINE_SPINLOCK(user_lock);
 static LIST_HEAD(ssc_list);
@@ -145,6 +147,49 @@ static inline const struct atmel_ssc_platform_data * __init
 		platform_get_device_id(pdev)->driver_data;
 }
 
+#ifdef CONFIG_SND_ATMEL_SOC_SSC
+static int ssc_sound_dai_probe(struct ssc_device *ssc)
+{
+	struct device_node *np = ssc->pdev->dev.of_node;
+	int ret;
+	int id;
+
+	ssc->sound_dai = false;
+
+	if (!of_property_read_bool(np, "#sound-dai-cells"))
+		return 0;
+
+	id = of_alias_get_id(np, "ssc");
+	if (id < 0)
+		return id;
+
+	ret = atmel_ssc_set_audio(id);
+	ssc->sound_dai = !ret;
+
+	return ret;
+}
+
+static void ssc_sound_dai_remove(struct ssc_device *ssc)
+{
+	if (!ssc->sound_dai)
+		return;
+
+	atmel_ssc_put_audio(of_alias_get_id(ssc->pdev->dev.of_node, "ssc"));
+}
+#else
+static inline int ssc_sound_dai_probe(struct ssc_device *ssc)
+{
+	if (of_property_read_bool(ssc->pdev->dev.of_node, "#sound-dai-cells"))
+		return -ENOTSUPP;
+
+	return 0;
+}
+
+static inline void ssc_sound_dai_remove(struct ssc_device *ssc)
+{
+}
+#endif
+
 static int ssc_probe(struct platform_device *pdev)
 {
 	struct resource *regs;
@@ -204,6 +249,9 @@ static int ssc_probe(struct platform_device *pdev)
 	dev_info(&pdev->dev, "Atmel SSC device at 0x%p (irq %d)\n",
 			ssc->regs, ssc->irq);
 
+	if (ssc_sound_dai_probe(ssc))
+		dev_err(&pdev->dev, "failed to auto-setup ssc for audio\n");
+
 	return 0;
 }
 
@@ -211,6 +259,8 @@ static int ssc_remove(struct platform_device *pdev)
 {
 	struct ssc_device *ssc = platform_get_drvdata(pdev);
 
+	ssc_sound_dai_remove(ssc);
+
 	spin_lock(&user_lock);
 	list_del(&ssc->list);
 	spin_unlock(&user_lock);
diff --git a/include/linux/atmel-ssc.h b/include/linux/atmel-ssc.h
index 7c0f6549898b..fdb545101ede 100644
--- a/include/linux/atmel-ssc.h
+++ b/include/linux/atmel-ssc.h
@@ -20,6 +20,7 @@ struct ssc_device {
 	int			user;
 	int			irq;
 	bool			clk_from_rk_pin;
+	bool			sound_dai;
 };
 
 struct ssc_device * __must_check ssc_request(unsigned int ssc_num);
-- 
2.1.4

^ permalink raw reply related

* [PATCH v2 0/2] register atmel-ssc as sound DAI w/o platform driver
From: Peter Rosin @ 2016-12-06 19:22 UTC (permalink / raw)
  To: linux-kernel
  Cc: Peter Rosin, Rob Herring, Mark Rutland, Liam Girdwood, Mark Brown,
	Nicolas Ferre, Arnd Bergmann, Greg Kroah-Hartman, Jaroslav Kysela,
	Takashi Iwai, devicetree, alsa-devel, linux-arm-kernel

Hi!

v1 -> v2 changes
- add ack from Rob Herring on patch 1/2.
- add reasons why breaking compatibility is ok for patch 2/2 (requested
  by Rob).
- add #sound-dai-cells to the ssc0 node in the devcietree example.

The Atmel SSC is currently not usable as an audio DAI unless someone
registers it with ASoC. This is currently delegated to a platform
driver for every possible audio use, and prevents the SSC from being
used as a cpu DAI with the simple-audio-card driver.

The first patch fixes this.

The second patch simplifies one of these platform drivers, since it
can now rely on the SSC to register itself with ASoC. However, this
may not be a possible simplification for other, older, drivers since
it also requires device tree changes.

Cheers,
Peter

Peter Rosin (2):
  misc: atmel-ssc: register as sound DAI if #sound-dai-cells is present
  ASoC: atmel: tse850: rely on the ssc to register as a cpu dai by
    itself

 .../devicetree/bindings/misc/atmel-ssc.txt         |  2 +
 .../bindings/sound/axentia,tse850-pcm5142.txt      | 11 +++--
 drivers/misc/atmel-ssc.c                           | 50 ++++++++++++++++++++++
 include/linux/atmel-ssc.h                          |  1 +
 sound/soc/atmel/tse850-pcm5142.c                   | 23 ++--------
 5 files changed, 64 insertions(+), 23 deletions(-)

-- 
2.1.4

^ permalink raw reply

* Re: [PATCH 4/5] arm64: dts: exynos5433: Add bus dt node using VDD_INT for Exynos5433
From: Krzysztof Kozlowski @ 2016-12-06 19:21 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: krzk-DgEjT+Ai2ygdnm+yROfE0A, javier-JPH+aEBZ4P+UEJcrhfAQsw,
	kgene-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ,
	tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w,
	myungjoo.ham-Sze3O3UU22JBDgjK7y7TUQ,
	kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480663087-4590-5-git-send-email-cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

On Fri, Dec 02, 2016 at 04:18:06PM +0900, Chanwoo Choi wrote:
> This patch adds the bus nodes using VDD_INT for Exynos5433 SoC.
> Exynos5433 has the following AMBA AXI buses to translate data
> between DRAM and sub-blocks.
> 
> Following list specify the detailed correlation between sub-block and clock:
> - CLK_ACLK_G2D_{400|266}  : Bus clock for G2D
> - CLK_ACLK_MSCL_400       : Bus clock for MSCL (Mobile Scaler)
> - CLK_ACLK_GSCL_333       : Bus clock for GSCL (General Scaler)
> - CLK_SCLK_JPEG_MSCL      : Bus clock for JPEG
> - CLK_ACLK_MFC_400        : Bus clock for MFC (Multi Format Codec)
> - CLK_ACLK_HEVC_400       : Bus clock for HEVC (High Effective Video Codec)
> - CLK_ACLK_BUS0_400       : NoC(Network On Chip)'s bus clock for PERIC/PERIS/FSYS/MSCL
> - CLK_ACLK_BUS1_400       : NoC's bus clock for MFC/HEVC/G3D
> - CLK_ACLK_BUS2_400       : NoC's bus clock for GSCL/DISP/G2D/CAM0/CAM1/ISP
> 
> Signed-off-by: Chanwoo Choi <cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  arch/arm64/boot/dts/exynos/exynos5433-bus.dtsi | 208 +++++++++++++++++++++++++
>  arch/arm64/boot/dts/exynos/exynos5433.dtsi     |   1 +
>  2 files changed, 209 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/exynos/exynos5433-bus.dtsi
> 
> diff --git a/arch/arm64/boot/dts/exynos/exynos5433-bus.dtsi b/arch/arm64/boot/dts/exynos/exynos5433-bus.dtsi
> new file mode 100644
> index 000000000000..b1e1d9c622e1
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/exynos5433-bus.dtsi
> @@ -0,0 +1,208 @@
> +/*
> + * Samsung's Exynos5433 SoC Memory interface and AMBA bus device tree source
> + *
> + * Copyright (c) 2016 Samsung Electronics Co., Ltd.
> + * Chanwoo Choi <cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> + *
> + * Samsung's Exynos5433 SoC Memory interface and AMBA buses are listed
> + * as device tree nodes are listed in this file.

This duplicates the introduction line and does not make sense.

> + *
> + * 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.
> + */
> +
> +/ {

Shouldn't these be under soc node? It looks like property of SoC itself.

> +	/* INT (Internal) block using VDD_INT */
> +	bus_g2d_400: bus_g2d_400 {

In node name, the dash '-' is preferred. The name should describe
general class of device so probably this should be just "bus"... but I
don't see a way how to do it reasonable anyway.

> +		compatible = "samsung,exynos-bus";
> +		clocks = <&cmu_top CLK_ACLK_G2D_400>;
> +		clock-names = "bus";
> +		operating-points-v2 = <&bus_g2d_400_opp_table>;
> +		status ="disable";

Hm?


> +	};
> +
> +	bus_mscl: bus_mscl {
> +		compatible = "samsung,exynos-bus";
> +		clocks = <&cmu_top CLK_ACLK_MSCL_400>;
> +		clock-names = "bus";
> +		operating-points-v2 = <&bus_g2d_400_opp_table>;
> +		status ="disable";
> +	};
> +
> +	bus_jpeg: bus_jpeg {
> +		compatible = "samsung,exynos-bus";
> +		clocks = <&cmu_top CLK_SCLK_JPEG_MSCL>;
> +		clock-names = "bus";
> +		operating-points-v2 = <&bus_g2d_400_opp_table>;
> +		status ="disable";
> +	};
> +
> +	bus_mfc: bus_mfc {
> +		compatible = "samsung,exynos-bus";
> +		clocks = <&cmu_top CLK_ACLK_MFC_400>;
> +
> +		clock-names = "bus";
> +		operating-points-v2 = <&bus_g2d_400_opp_table>;
> +		status ="disable";
> +	};
> +
> +	bus_g2d_266: bus_g2d_266 {
> +		compatible = "samsung,exynos-bus";
> +		clocks = <&cmu_top CLK_ACLK_G2D_266>;
> +		clock-names = "bus";
> +		operating-points-v2 = <&bus_g2d_266_opp_table>;
> +		status ="disable";
> +	};
> +
> +	bus_gscl: bus_gscl {
> +		compatible = "samsung,exynos-bus";
> +		clocks = <&cmu_top CLK_ACLK_GSCL_333>;
> +		clock-names = "bus";
> +		operating-points-v2 = <&bus_gscl_opp_table>;
> +		status ="disable";
> +	};
> +
> +	bus_hevc: bus_hevc {
> +		compatible = "samsung,exynos-bus";
> +		clocks = <&cmu_top CLK_ACLK_HEVC_400>;
> +		clock-names = "bus";
> +		operating-points-v2 = <&bus_hevc_opp_table>;
> +		status ="disable";
> +	};
> +
> +	bus_bus0: bus_bus0 {

bus, bus, bus, bus, jackpot! Let's try to find better name and label for
these. :)

Best regards,
Krzysztof
--
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 3/5] arm64: dts: exynos5433: Add PPMU dt node
From: Krzysztof Kozlowski @ 2016-12-06 19:07 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: krzk-DgEjT+Ai2ygdnm+yROfE0A, javier-JPH+aEBZ4P+UEJcrhfAQsw,
	kgene-DgEjT+Ai2ygdnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ,
	tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w,
	myungjoo.ham-Sze3O3UU22JBDgjK7y7TUQ,
	kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480663087-4590-4-git-send-email-cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

On Fri, Dec 02, 2016 at 04:18:05PM +0900, Chanwoo Choi wrote:
> This patch adds PPMU (Platform Performance Monitoring Unit) Device-tree node
> to measure the utilization of each IP in Exynos SoC.
> 
> - PPMU_D{0|1}_CPU are used to measure the utilization of MIF (Memory Interface)
>   block with VDD_MIF power source.
> - PPMU_D{0|1}_GENERAL are used to measure the utilization of INT(Internal)
>   block with VDD_INT power source.
> 
> Signed-off-by: Chanwoo Choi <cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  arch/arm64/boot/dts/exynos/exynos5433.dtsi | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/exynos/exynos5433.dtsi b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> index 64226d5ae471..8c4ee84d5232 100644
> --- a/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> +++ b/arch/arm64/boot/dts/exynos/exynos5433.dtsi
> @@ -599,6 +599,30 @@
>  			clock-names = "fin_pll", "mct";
>  		};
>  
> +		ppmu_d0_cpu: ppmu@10480000 {
> +			compatible = "samsung,exynos-ppmu-v2";
> +			reg = <0x10480000 0x2000>;
> +			status = "disabled";

Why these are disabled? They have some external dependencies?

Best regards,
Krzysztof

> +		};
> +
> +		ppmu_d0_general: ppmu@10490000 {
> +			compatible = "samsung,exynos-ppmu-v2";
> +			reg = <0x10490000 0x2000>;
> +			status = "disabled";
> +		};
> +
> +		ppmu_d1_cpu: ppmu@104b0000 {
> +			compatible = "samsung,exynos-ppmu-v2";
> +			reg = <0x104b0000 0x2000>;
> +			status = "disabled";
> +		};
> +
> +		ppmu_d1_general: ppmu@104c0000 {
> +			compatible = "samsung,exynos-ppmu-v2";
> +			reg = <0x104c0000 0x2000>;
> +			status = "disabled";
> +		};
> +
>  		pinctrl_alive: pinctrl@10580000 {
>  			compatible = "samsung,exynos5433-pinctrl";
>  			reg = <0x10580000 0x1a20>, <0x11090000 0x100>;
> -- 
> 1.9.1
> 
--
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 4/6] net: ethernet: ti: cpts: add ptp pps support
From: Richard Cochran @ 2016-12-06 18:08 UTC (permalink / raw)
  To: Grygorii Strashko
  Cc: David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA, Mugunthan V N,
	Sekhar Nori, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Murali Karicheri, Wingman Kwok
In-Reply-To: <20161130100519.GD28680-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>

On Wed, Nov 30, 2016 at 11:05:19AM +0100, Richard Cochran wrote:
> Can you adjust the frequency of the keystone devices in hardware?  If
> so, then please implement it, and just disable PPS for the CPSW.
> 
> The only reason I used the timecounter for frequency adjustment was
> because the am335x HW is broken.  But this shouldn't hold back other
> newer HW without the same silicon flaws.

I am talking here about the ADPLLLJ units.  Are they usable on the
keystone?

If so, please implement the frequency adjustment with them.

Thanks,
Richard
--
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 v4 09/13] net: ethernet: ti: cpts: rework initialization/deinitialization
From: Richard Cochran @ 2016-12-06 18:04 UTC (permalink / raw)
  To: Grygorii Strashko
  Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori, linux-kernel,
	linux-omap, devicetree, Murali Karicheri, Wingman Kwok,
	Thomas Gleixner
In-Reply-To: <c0d6d670-cf44-c113-3443-3b5209e68ee2@ti.com>

On Tue, Dec 06, 2016 at 11:49:14AM -0600, Grygorii Strashko wrote:
> But we do reset whole cpsw :( and that's required to support PM use cases as
> suspend/resume.

The code is resetting the clock unconditionally on ifup/down.  That
sucks.  If you reset the clock *only* after resume, that would be ok.
 
> There are also PM requirement to shutdown cpsw in case all interfaces are down.

Well, those requirements are not too smart.  As an end user, I expect
that ifdown/up does not change the time.  There isn't any reason to
reset the clock in this case.

> More over, there are requirement to minimize cpsw power consumption in case all links are
> disconnected (and cpts is special case here).
> 
> So, at least resetting of the timecounter still required.

Only if you follow that poorly conceived PM plan.  Anyhow, I agree
that it isn't the task of your present series to fix that.

> Ok. I'll try to optimize it following your directions.

What I would like to see is: initialize the cyclecounter fields
exactly once.

Thanks,
Richard

^ permalink raw reply

* Re: [PATCH v4 09/13] net: ethernet: ti: cpts: rework initialization/deinitialization
From: Grygorii Strashko @ 2016-12-06 17:49 UTC (permalink / raw)
  To: Richard Cochran
  Cc: David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA, Mugunthan V N,
	Sekhar Nori, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Murali Karicheri, Wingman Kwok,
	Thomas Gleixner
In-Reply-To: <20161206171802.GA19646-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>

Hi Richard,

On 12/06/2016 11:18 AM, Richard Cochran wrote:
> On Tue, Dec 06, 2016 at 10:45:55AM -0600, Grygorii Strashko wrote:
>> On 12/06/2016 07:40 AM, Richard Cochran wrote:
>>> [ BTW, resetting the timecounter here makes no sense either.  Why
>>>   reset the clock just because the interface goes down?  ]
>>>
>>
>> Huh. This is how it works now (even before my changes) - this is just refactoring!
>> (really new thing is the only cpts_calc_mult_shift()).
> 
> The cpts_register() used to be called from probe(), but this changed
> without any review in f280e89ad.  That wasn't your fault, but still...
>  
>> Also there are additional questions such as:
>> - is there guarantee that cpsw port will be connected to the same network after ifup?
> 
> The network is not relevant.  PTP time is a global standard, just like
> with NTP.  We don't reset the NTP clock with ifup/down, do we?

But we do reset whole cpsw :( and that's required to support PM use cases as
suspend/resume.

There are also PM requirement to shutdown cpsw in case all interfaces are down.

More over, there are requirement to minimize cpsw power consumption in case all links are
disconnected (and cpts is special case here).

So, at least resetting of the timecounter still required.



> 
>> - should there be possibility to reset cc.mult if it's value will be kept from the previous run?
> 
> cc.mult will change naturally according to the operation of the user
> space PTP stack.  There is no need to reset it that I can see.
>  
>>> Secondly, you have made the initialization order of these fields hard
>>> to follow.  With the whole series applied:
>>>
>>> probe()
>>> 	cpts_create()
>>> 		cpts_of_parse()
>>> 		{
>>> 			/* Set cc_mult but not cc.mult! */
>>> 			set cc_mult
>>> 			set cc.shift
>>> 		}
>>> 		cpts_calc_mult_shift()
>>> 		{
>>> 			/* Set them both. */
>>> 			cpts->cc_mult = mult;
>>> 			cpts->cc.mult = mult; 
>>
>> ^^ this assignment of cpts->cc.mult not required.
> 
> You wrote the code, not me.

Sry, I meant, thank for catching this.

>  
>>> 			cpts->cc.shift = shift;
>> 			
>>
>> only in case there were not set in DT before
>> (I have a requirement to support both - DT and cpts_calc_mult_shift and
>>  that introduces a bit of complexity)
>>
>> Also, I've tried not to add more fields in struct cpts.
>>
>>> 		}
>>> /* later on */
>>> cpts_register()
>>> 	cpts->cc.mult = cpts->cc_mult;
>>>
>>> There is no need for such complexity.  Simply set cc.mult in
>>> cpts_create() _once_, immediately after the call to
>>> cpts_calc_mult_shift().
>>>
>>> You can remove the assignment from cpts_calc_mult_shift() and
>>> cpts_register().
>>
>> Just to clarify: do you propose to get rid of cpts->cc_mult at all?
> 
> No.  Read what I said before:
> 
>    There is no need for such complexity.  Simply set cc.mult in
>    cpts_create() _once_, immediately after the call to
>    cpts_calc_mult_shift().
> 
>> Honestly, i'd not prefer to change functional behavior of ptp clock as part of
>> this series.
> 
> Fair enough.  The bogus clock reset existed before your series.
> 
> But please don't obfuscate simple initialization routines.  The way
> you set cc.mult and cc_mult is illogical and convoluted.
> 

Ok. I'll try to optimize it following your directions.

Thanks.

-- 
regards,
-grygorii
--
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: [RFC] New Device Tree property - "bonding"
From: Ramesh Shanmugasundaram @ 2016-12-06 17:45 UTC (permalink / raw)
  To: Rob Herring
  Cc: Laurent Pinchart,
	frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
	mark.rutland-5wv7dgnIgG8@public.gmane.org,
	pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org,
	Chris Paterson, Geert Uytterhoeven,
	laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Maxime Ripard
In-Reply-To: <CAL_JsqKJcEmNUzOm-3j3FODkN1faXMAMmURRxfRpHfiGs_a+qA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6188 bytes --]

Hi Rob,

> >> On Monday 05 Dec 2016 09:57:32 Rob Herring wrote:
> >> > On Mon, Dec 5, 2016 at 8:40 AM, Laurent Pinchart wrote:
> >> > > On Monday 05 Dec 2016 08:18:34 Rob Herring wrote:
> >> > >> On Fri, Nov 25, 2016 at 10:55 AM, Ramesh Shanmugasundaram wrote:
> >> > >>> Hello DT maintainers,
> >> > >>>
> >> > >>> In one of the Renesas SoCs we have a device called DRIF
> >> > >>> (Digital Radio
> >> > >>> Interface) controller. A DRIF channel contains 4 external pins
> >> > >>> - SCK, SYNC, Data pins D0 & D1.
> >> > >>>
> >> > >>> Internally a DRIF channel is made up of two SPI slave devices
> >> > >>> (also called sub-channels here) that share common CLK & SYNC
> >> > >>> signals but have their own resource set. The DRIF channel can
> >> > >>> have either one of the sub-channel active at a time or both.
> >> > >>> When both sub-channels are active, they need to be managed
> >> > >>> together as one device as they share same CLK & SYNC. We plan
> >> > >>> to tie these two sub-channels together with a new property called
> "renesas,bonding".
> >> > >>
> >> > >> Is there no need to describe the master device? No GPIOs,
> >> > >> regulators or other sideband controls needed? If that's never
> >> > >> needed (which seems doubtful), then I would do something
> >> > >> different here probably with the master device as a child of one
> >> > >> DRIF and then phandles to master from the other DRIFs.
> >> > >> Otherwise, this looks
> >> fine to me.
> >> > >
> >> > > Here's a bit of background.
> >> > >
> >> > > The DRIF is an SPI receiver. It has three input pins, a clock
> >> > > line, a data line and a sync signal. The device is designed to be
> >> > > connected to a variety of data sources, usually plain SPI (1 data
> >> > > line), IIS (1 data
> >> > > line) but also radio tuners that output I/Q data
> >> > > (http://www.ni.com/tutorial/4805/en/) over two data lines.
> >> > >
> >> > > In the case of IQ each data sample is split in two I and Q values
> >> > > (typically 16 to 20 bits each in this case), and the values are
> >> > > transmitted serially over one data line each. The synchronization
> >> > > and clock signals are common to both data lines. The DRIF is
> >> > > optimized for this use case as the DRIF instances in the SoC
> >> > > (each of them having independent clocks, interrupts and control
> >> > > registers) are grouped by two, and the two instances in a group
> >> > > handle a single data line each but share the same clock and sync
> input.
> >> > >
> >> > > On the software side we need to group the I and Q values, which
> >> > > are DMA'ed to memory by the two DRIF instances, and make them
> >> > > available to userspace. The V4L2 API used here in SDR (Software
> >> > > Defined Radio) mode supports such use cases and exposes a single
> >> > > device node to userspace that allows control of the two DRIF
> >> > > instances as a single device. To be able to implement this we
> >> > > need kernel code to be aware of DRIF groups and, while binding to
> >> > > the DRIF instances separately, expose only one V4L2 device to
> userspace for each group.
> >> > >
> >> > > There's no master or slave instance from a hardware point of
> >> > > view, but the two instances are not interchangeable as they carry
> >> > > separate
> >> information.
> >> > > They must thus be identified at the driver level.
> >> >
> >> > By master, I meant the external master device that generates the IQ
> >> > data, not which of the internal DRIF blocks is a master of the other.
> >> > So back to my question, does the external master device need to be
> >> > described? I worry the answer now for a simple case is no, but then
> >> > later people are going to have cases needing to describe more. We
> >> > need to answer this question first before we can decide what this
> >> > binding should look like.
> >>
> >> Oh yes the external device certainly needs to be described. As it is
> >> controlled through a separate, general-purpose I2C or SPI controller,
> >> it should be a child node of that controller. The DRIF handles the
> >> data interface only, not the control interface of the external device.
> >
> > Yes, as Laurent mentioned, the external master will be described
> separately. The data interface with the master is described through port
> nodes. E.g.
> >
> >         port {
> >                 drif0_ep: endpoint {
> >                      remote-endpoint = <&tuner_ep>;
> >                 };
> >         };
> >
> > Do we agree on this model please?
>
> Well, that's not complete as you should have both DRIF0 and DRIF1 having
> connections to the tuner. Then you can walk the graph and find everything,
> and you then don't need the bonding property.

Assuming the third party tuner exposes it's two data lines as two endpoints, it seems possible with of_graph.h apis to walk through tuner end points and get the phandle of the other DRIF device. However, there are couple of points coming to mind.

- The ctrl pins shared between two DRIFs needs to be enabled in one of the DRIF device. Do we choose this device arbitrarily? Do we expose the CTRL signal properties (msb/lsb first, polarity etc) on both DRIF devices? Should we think about scalability?

- It mandates the third party tuner device to expose it's two data lines as two endpoints. It assumes that a single third party master device controls both the data lines coming to each DRIF device.

The bonding property looks a bit cleaner on these aspects because it describes only the DRIF device.

Thanks,
Ramesh





[https://www2.renesas.eu/media/email/unicef.gif]

This Christmas, instead of sending out cards, Renesas Electronics Europe have decided to support Unicef with a donation. For further details click here<https://www.unicef.org/> to find out about the valuable work they do, helping children all over the world.
We would like to take this opportunity to wish you a Merry Christmas and a prosperous New Year.
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·zøœzÚÞz)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢j.ïÛ°\½½MŽúgjÌæa×\x02››–' ™©Þ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾\a«‘êçzZ+ƒùšŽŠÝ¢j"ú!¶i

^ permalink raw reply

* Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT
From: Kevin Hilman @ 2016-12-06 17:40 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Laurent Pinchart, Sakari Ailus,
	linux-media-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Sekhar Nori, Axel Haslam,
	Bartosz Gołaszewski, Alexandre Bailon, David Lechner
In-Reply-To: <d7aaa1d5-f11a-e361-b2fe-f0cf86d92008-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org>

Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> writes:

> On 12/01/2016 10:16 AM, Laurent Pinchart wrote:
>> Hello,
>> 
>> On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:
>>> On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
>>>> Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org> writes:
>>>>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>>>>>> Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org> writes:
>>>>>>> On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
>>>>>>>> Allow getting of subdevs from DT ports and endpoints.
>>>>>>>>
>>>>>>>> The _get_pdata() function was larely inspired by (i.e. stolen from)
>>>>>>>> am437x-vpfe.c
>>>>>>>>
>>>>>>>> Signed-off-by: Kevin Hilman <khilman-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>>>>>>> ---
>>>>>>>>
>>>>>>>>  drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
>>>>>>>>  include/media/davinci/vpif_types.h     
>>>>>>>>        |   9 +-
>>>>>>>>  2 files changed, 133 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>> b/drivers/media/platform/davinci/vpif_capture.c index
>>>>>>>> 94ee6cf03f02..47a4699157e7 100644
>>>>>>>> --- a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>>>>>>>> @@ -26,6 +26,8 @@
>>>>>>>>  #include <linux/slab.h>
>>>>>>>>
>>>>>>>>  #include <media/v4l2-ioctl.h>
>>>>>>>> +#include <media/v4l2-of.h>
>>>>>>>> +#include <media/i2c/tvp514x.h>
>>>>>>>
>>>>>>> Do you need this header?
>>>>>>
>>>>>> Yes, based on discussion with Hans, since there is no DT binding for
>>>>>> selecting the input pins of the TVP514x, I have to select it in the
>>>>>> driver, so I need the defines from this header.  More on this below...
>> 
>> That's really ugly :-( The problem should be fixed properly instead of adding 
>> one more offender.
>
> Do you have time for that, Laurent? I don't. Until that time we just need to
> make do with this workaround.
>
>> 
>>>>>>>>  #include "vpif.h"
>>>>>>>>  #include "vpif_capture.h"
>>>>>>>> @@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
>>>>>>>>
>>>>>>>>  	vpif_dbg(2, debug, "vpif_input_to_subdev\n");
>>>>>>>>
>>>>>>>> +	if (!chan_cfg)
>>>>>>>> +		return -1;
>>>>>>>> +	if (input_index >= chan_cfg->input_count)
>>>>>>>> +		return -1;
>>>>>>>>  	subdev_name = chan_cfg->inputs[input_index].subdev_name;
>>>>>>>>  	if (subdev_name == NULL)
>>>>>>>>  		return -1;
>>>>>>>> @@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
>>>>>>>>  	/* loop through the sub device list to get the sub device info
>>>>>>>>  	*/
>>>>>>>>  	for (i = 0; i < vpif_cfg->subdev_count; i++) {
>>>>>>>>  		subdev_info = &vpif_cfg->subdev_info[i];
>>>>>>>> -		if (!strcmp(subdev_info->name, subdev_name))
>>>>>>>> +		if (subdev_info && !strcmp(subdev_info->name,
>>>>>>>> subdev_name))
>>>>>>>>  			return i;
>>>>>>>>  	}
>>>>>>>>  	return -1;
>>>>>>>> @@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct
>>>>>>>> v4l2_async_notifier *notifier,> >> >> 
>>>>>>>>  {
>>>>>>>>  	int i;
>>>>>>>>
>>>>>>>> +	for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
>>>>>>>> +		struct v4l2_async_subdev *_asd = vpif_obj.config
>>>>>>>> ->asd[i];
>>>>>>>> +		const struct device_node *node = _asd->match.of.node;
>>>>>>>> +
>>>>>>>> +		if (node == subdev->of_node) {
>>>>>>>> +			vpif_obj.sd[i] = subdev;
>>>>>>>> +			vpif_obj.config->chan_config
>>>>>>>> ->inputs[i].subdev_name =
>>>>>>>> +				(char *)subdev->of_node->full_name;
>> 
>> Can subdev_name be made const instead of blindly casting the full_name pointer 
>> ? If not this is probably unsafe, and if yes it should be done :-)
>> 
>>>>>>>> +			vpif_dbg(2, debug,
>>>>>>>> +				 "%s: setting input %d subdev_name =
>>>>>>>> %s\n",
>>>>>>>> +				 __func__, i, subdev->of_node
>>>>>>>> ->full_name);
>>>>>>>> +			return 0;
>>>>>>>> +		}
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>>  	for (i = 0; i < vpif_obj.config->subdev_count; i++)
>>>>>>>>  		if (!strcmp(vpif_obj.config->subdev_info[i].name,
>>>>>>>>  			    subdev->name)) {
>>>>>>>> @@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct
>>>>>>>> v4l2_async_notifier *notifier)
>>>>>>>>  	return vpif_probe_complete();
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +static struct vpif_capture_config *
>>>>>>>> +vpif_capture_get_pdata(struct platform_device *pdev)
>>>>>>>> +{
>>>>>>>> +	struct device_node *endpoint = NULL;
>>>>>>>> +	struct v4l2_of_endpoint bus_cfg;
>>>>>>>> +	struct vpif_capture_config *pdata;
>>>>>>>> +	struct vpif_subdev_info *sdinfo;
>>>>>>>> +	struct vpif_capture_chan_config *chan;
>>>>>>>> +	unsigned int i;
>>>>>>>> +
>>>>>>>> +	dev_dbg(&pdev->dev, "vpif_get_pdata\n");
>>>>>>>> +
>>>>>>>> +	if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
>>>>>>>> +		return pdev->dev.platform_data;
>>>>>>>> +
>>>>>>>> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>>>>>>>> +	if (!pdata)
>>>>>>>> +		return NULL;
>>>>>>>> +	pdata->subdev_info =
>>>>>>>> +		devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
>>>>>>>> +			     VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
>>>>>>>> +
>>>>>>>> +	if (!pdata->subdev_info)
>>>>>>>> +		return NULL;
>>>>>>>> +	dev_dbg(&pdev->dev, "%s\n", __func__);
>>>>>>>> +
>>>>>>>> +	for (i = 0; ; i++) {
>>>>>>>> +		struct device_node *rem;
>>>>>>>> +		unsigned int flags;
>>>>>>>> +		int err;
>>>>>>>> +
>>>>>>>> +		endpoint = of_graph_get_next_endpoint(pdev
>>>>>>>> ->dev.of_node,
>>>>>>>> +						      endpoint);
>>>>>>>> +		if (!endpoint)
>>>>>>>> +			break;
>>>>>>>> +
>>>>>>>> +		sdinfo = &pdata->subdev_info[i];
>>>>>>>
>>>>>>> subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.
>>>>>>
>>>>>> Right, I need to make the loop only go for a max of
>>>>>> VPIF_CAPTURE_MAX_CHANNELS iterations.
>>>>>>
>>>>>>>> +		chan = &pdata->chan_config[i];
>>>>>>>> +		chan->inputs = devm_kzalloc(&pdev->dev,
>>>>>>>> +					    sizeof(*chan->inputs) *
>>>>>>>> +					    VPIF_DISPLAY_MAX_CHANNELS,
>>>>>>>> +					    GFP_KERNEL);
>>>>>>>> +
>>>>>>>> +		chan->input_count++;
>>>>>>>> +		chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;
>>>>>>>
>>>>>>> I wonder what's the purpose of using index i on this array as well.
>>>>>>
>>>>>> The number of endpoints in DT is the number of input channels
>>>>>> configured (up to a max of VPIF_CAPTURE_MAX_CHANNELS.)
>>>>>>
>>>>>>> If you use that to access a corresponding entry in a different array,
>>>>>>> I'd just create a struct that contains the port configuration and the
>>>>>>> async sub-device. The omap3isp driver does that, for instance; see
>>>>>>> isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if
>>>>>>> you're interested. Up to you.
>>>>>>
>>>>>> OK, I'll have a look at that driver. The goal here with this series is
>>>>>> just to get this working with DT, but also not break the existing
>>>>>> legacy platform_device support, so I'm trying not to mess with the
>>>>>> driver-interal data structures too much.
>>>>>
>>>>> Ack.
>>>>>
>>>>>>>> +		chan->inputs[i].input.std = V4L2_STD_ALL;
>>>>>>>> +		chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
>>>>>>>> +
>>>>>>>> +		/* FIXME: need a new property? ch0:composite ch1:
>>>>>>>> s-video */
>>>>>>>> +		if (i == 0)
>>>>>>>
>>>>>>> Can you assume that the first endopoint has got a particular kind of
>>>>>>> input? What if it's not connected?
>>>>>>
>>>>>> On all the boards I know of (there aren't many using this SoC), it's a
>>>>>> safe assumption.
>>>>>>
>>>>>>> If this is a different physical port (not in the meaning another) in
>>>>>>> the device, I'd use the reg property for this. Please see
>>>>>>> Documentation/devicetree/bindings/media/video-interfaces.txt .
>>>>>>
>>>>>> My understanding (which is admittedly somewhat fuzzy) of the TVP514x is
>>>>>> that it's not physically a different port.  Instead, it's just telling
>>>>>> the TVP514x which pin(s) will be active inputs (and what kind of signal
>>>>>> will be present.)
>>>>>>
>>>>>> I'm open to a better way to describe this input select from DT, but
>>>>>> based on what I heard from Hans, there isn't currently a good way to do
>>>>>> that except for in the driver:
>>>>>> (c.f. https://marc.info/?l=linux-arm-kernel&m=147887871615788)
>>>>>>
>>>>>> Based on further discussion in that thread, it sounds like there may be
>>>>>> a way forward coming soon, and I'll be glad to switch to that when it
>>>>>> arrives.
>> 
>> I'm afraid I have to disappoint Hans here, I don't have code for that yet.
>> 
>>>>> I'm not sure that properly supporting connectors will provide any help
>>>>> here.
>>>>>
>>>>> Looking at the s_routing() API, it's the calling driver that has to be
>>>>> aware of sub-device specific function parameters. As such it's not a
>>>>> very good idea to require that a driver is aware of the value range of
>>>>> another driver's parameter. I wonder if a simple enumeration interface
>>>>> would help here --- if I understand correctly, the purpose is just to
>>>>> provide a way to choose the input using VIDIOC_S_INPUT.
>>>>>
>>>>> I guess that's somehow ok as long as you have no other combinations of
>>>>> these devices but this is hardly future-proof. (And certainly not a
>>>>> problem created by this patch.)
>>>>
>>>> Yeah, this is far from future proof.
>>>>
>>>>> It'd be still nice to fix that as presumably we don't have the option of
>>>>> reworking how we expect the device tree to look like.
>>>>
>>>> Agreed.
>>>>
>>>> I'm just hoping someone can shed som light on "how we expect the device
>>>> tree to look".  ;)
>>>
>>> :-)
>>>
>>> For the tvp514x, do you need more than a single endpoint on the receiver
>>> side? Does the input that's selected affect the bus parameters?
>>>
>>> If it doesn't, you could create a custom endpoint property for the possible
>>> input values. The s_routing() really should be fixed though, but that could
>>> be postponed I guess. There are quite a few drivers using it.
>> 
>> There's two ways to look at s_routing() in my opinion, as the calling driver 
>> should really not hardcode any knowledge specific to a particular subdev. We 
>> can either have the calling driver discover the possible routing options at 
>> runtime through the subdev API, or modify the s_routing() API.
>> 
>
> Some historical perspective: s_routing was added well before the device tree
> was ever used for ARM. And at that time the vast majority of drivers were PCI
> or USB drivers, very few platform drivers existed (and those typically used
> sensors, not video receivers).
>
> Before s_routing existed the situation was even worse.
>
> Basically what s_routing does is a poor-man's device tree entry, telling the
> subdev how to route video or audio from connector to the output of the chip.
> Typically the card tables in PCI or USB drivers contain the correct arguments
> for s_routing. Of course, today we'd do that with the DT, but that was not an
> option years ago.

So I'm still confused on the path forward here.

I do not have the time (or the V4L2 knowledge/experience) to rework the
V4L2 internals to make this work, but I'm happy to test if someone else
is working on it.

In the meantime, what do we do with this series?  I have a couple minor
things to fixup based on review comments, but other than that, the
s_routing decision is blocking this from getting an update for use on DT
platforms.

The alternative is to go the OMAP route for legacy drivers like this and
just use pdata quirks for passing the legacy pdata (which has the input
and output routes hard-coded in platform_data). 

Kevin

--
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/5] ARM: BCM5301X: Specify USB controllers in DT
From: Ray Jui @ 2016-12-06 17:36 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: Florian Fainelli, Arnd Bergmann, Rob Herring, Mark Rutland,
	Russell King, Hauke Mehrtens, bcm-kernel-feedback-list,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	Linux Kernel Mailing List, Rafał Miłecki
In-Reply-To: <CACna6rw4iY_xhEUyWkGrchs0SOn3nLBKYMuA7qa0UHAbxpGN0A@mail.gmail.com>



On 12/6/2016 9:31 AM, Rafał Miłecki wrote:
> On 6 December 2016 at 18:28, Ray Jui <ray.jui@broadcom.com> wrote:
>> On 12/6/2016 9:17 AM, Rafał Miłecki wrote:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> There are 3 separated controllers, one per USB /standard/. With PHY
>>> drivers in place they can be simply supported with generic drivers.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>>  arch/arm/boot/dts/bcm5301x.dtsi | 33 ++++++++++++++++++++++++++++++++-
>>>  1 file changed, 32 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
>>> index f09a2bb..bfc98d19 100644
>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>> @@ -248,8 +248,26 @@
>>>
>>>                       #address-cells = <1>;
>>>                       #size-cells = <1>;
>>> +                     ranges;
>>>
>>> -                     phys = <&usb2_phy>;
>>> +                     interrupt-parent = <&gic>;
>>> +
>>> +                     ohci: ohci@21000 {
>>> +                             #usb-cells = <0>;
>>> +
>>> +                             compatible = "generic-ohci";
>>> +                             reg = <0x00022000 0x1000>;
>>
>> Your label ohci@21000 does not match the 'reg' at 0x22000.
>>
>>> +                             interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
>>> +                     };
>>> +
>>> +                     ehci: ehci@22000 {
>>> +                             #usb-cells = <0>;
>>> +
>>> +                             compatible = "generic-ehci";
>>> +                             reg = <0x00021000 0x1000>;
>>
>> Looks like you got the label of ohci and ehci reversed?
> 
> Nice catch, thanks! I'll fix it in V2 (just let me wait a day to see
> if there will be other comments).
> 

In V2, please remember to put the nodes in ascending order based on the
base address of the registers, i.e., ehci@21000, and then followed by
ohci@22000.

Thanks,

Ray

^ permalink raw reply

* Re: [PATCH 2/5] ARM: BCM5301X: Specify USB controllers in DT
From: Rafał Miłecki @ 2016-12-06 17:31 UTC (permalink / raw)
  To: Ray Jui
  Cc: Florian Fainelli, Arnd Bergmann, Rob Herring, Mark Rutland,
	Russell King, Hauke Mehrtens, bcm-kernel-feedback-list,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Linux Kernel Mailing List, Rafał Miłecki
In-Reply-To: <c721c71c-117f-185d-0544-954981dbcf04-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>

On 6 December 2016 at 18:28, Ray Jui <ray.jui-dY08KVG/lbpWk0Htik3J/w@public.gmane.org> wrote:
> On 12/6/2016 9:17 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>
>> There are 3 separated controllers, one per USB /standard/. With PHY
>> drivers in place they can be simply supported with generic drivers.
>>
>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/bcm5301x.dtsi | 33 ++++++++++++++++++++++++++++++++-
>>  1 file changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
>> index f09a2bb..bfc98d19 100644
>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>> @@ -248,8 +248,26 @@
>>
>>                       #address-cells = <1>;
>>                       #size-cells = <1>;
>> +                     ranges;
>>
>> -                     phys = <&usb2_phy>;
>> +                     interrupt-parent = <&gic>;
>> +
>> +                     ohci: ohci@21000 {
>> +                             #usb-cells = <0>;
>> +
>> +                             compatible = "generic-ohci";
>> +                             reg = <0x00022000 0x1000>;
>
> Your label ohci@21000 does not match the 'reg' at 0x22000.
>
>> +                             interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
>> +                     };
>> +
>> +                     ehci: ehci@22000 {
>> +                             #usb-cells = <0>;
>> +
>> +                             compatible = "generic-ehci";
>> +                             reg = <0x00021000 0x1000>;
>
> Looks like you got the label of ohci and ehci reversed?

Nice catch, thanks! I'll fix it in V2 (just let me wait a day to see
if there will be other comments).

-- 
Rafał
--
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/5] ARM: BCM5301X: Specify USB controllers in DT
From: Ray Jui @ 2016-12-06 17:28 UTC (permalink / raw)
  To: Rafał Miłecki, Florian Fainelli
  Cc: Arnd Bergmann, Rob Herring, Mark Rutland, Russell King,
	Hauke Mehrtens, bcm-kernel-feedback-list, devicetree,
	linux-arm-kernel, linux-kernel, Rafał Miłecki
In-Reply-To: <20161206171714.22738-2-zajec5@gmail.com>



On 12/6/2016 9:17 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> There are 3 separated controllers, one per USB /standard/. With PHY
> drivers in place they can be simply supported with generic drivers.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  arch/arm/boot/dts/bcm5301x.dtsi | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
> index f09a2bb..bfc98d19 100644
> --- a/arch/arm/boot/dts/bcm5301x.dtsi
> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
> @@ -248,8 +248,26 @@
>  
>  			#address-cells = <1>;
>  			#size-cells = <1>;
> +			ranges;
>  
> -			phys = <&usb2_phy>;
> +			interrupt-parent = <&gic>;
> +
> +			ohci: ohci@21000 {
> +				#usb-cells = <0>;
> +
> +				compatible = "generic-ohci";
> +				reg = <0x00022000 0x1000>;

Your label ohci@21000 does not match the 'reg' at 0x22000.

> +				interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
> +			};
> +
> +			ehci: ehci@22000 {
> +				#usb-cells = <0>;
> +
> +				compatible = "generic-ehci";
> +				reg = <0x00021000 0x1000>;

Looks like you got the label of ohci and ehci reversed?

> +				interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
> +				phys = <&usb2_phy>;
> +			};
>  		};
>  
>  		usb3: usb3@23000 {
> @@ -257,6 +275,19 @@
>  
>  			#address-cells = <1>;
>  			#size-cells = <1>;
> +			ranges;
> +
> +			interrupt-parent = <&gic>;
> +
> +			xhci: xhci@23000 {
> +				#usb-cells = <0>;
> +
> +				compatible = "generic-xhci";
> +				reg = <0x00023000 0x1000>;
> +				interrupts = <GIC_SPI 80 IRQ_TYPE_LEVEL_HIGH>;
> +				phys = <&usb3_phy>;
> +				phy-names = "usb";
> +			};
>  		};
>  
>  		spi@29000 {
> 

^ permalink raw reply

* Re: [PATCH v4 09/13] net: ethernet: ti: cpts: rework initialization/deinitialization
From: Richard Cochran @ 2016-12-06 17:18 UTC (permalink / raw)
  To: Grygorii Strashko
  Cc: David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA, Mugunthan V N,
	Sekhar Nori, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-omap-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Murali Karicheri, Wingman Kwok,
	Thomas Gleixner
In-Reply-To: <4e7888b6-9f1c-ca31-e83e-15109bf1df3f-l0cyMroinI0@public.gmane.org>

On Tue, Dec 06, 2016 at 10:45:55AM -0600, Grygorii Strashko wrote:
> On 12/06/2016 07:40 AM, Richard Cochran wrote:
> > [ BTW, resetting the timecounter here makes no sense either.  Why
> >   reset the clock just because the interface goes down?  ]
> > 
> 
> Huh. This is how it works now (even before my changes) - this is just refactoring!
> (really new thing is the only cpts_calc_mult_shift()).

The cpts_register() used to be called from probe(), but this changed
without any review in f280e89ad.  That wasn't your fault, but still...
 
> Also there are additional questions such as:
> - is there guarantee that cpsw port will be connected to the same network after ifup?

The network is not relevant.  PTP time is a global standard, just like
with NTP.  We don't reset the NTP clock with ifup/down, do we?

> - should there be possibility to reset cc.mult if it's value will be kept from the previous run?

cc.mult will change naturally according to the operation of the user
space PTP stack.  There is no need to reset it that I can see.
 
> > Secondly, you have made the initialization order of these fields hard
> > to follow.  With the whole series applied:
> > 
> > probe()
> > 	cpts_create()
> > 		cpts_of_parse()
> > 		{
> > 			/* Set cc_mult but not cc.mult! */
> > 			set cc_mult
> > 			set cc.shift
> > 		}
> > 		cpts_calc_mult_shift()
> > 		{
> > 			/* Set them both. */
> > 			cpts->cc_mult = mult;
> > 			cpts->cc.mult = mult; 
> 
> ^^ this assignment of cpts->cc.mult not required.

You wrote the code, not me.
 
> > 			cpts->cc.shift = shift;
> 			
> 
> only in case there were not set in DT before
> (I have a requirement to support both - DT and cpts_calc_mult_shift and
>  that introduces a bit of complexity)
> 
> Also, I've tried not to add more fields in struct cpts.
> 
> > 		}
> > /* later on */
> > cpts_register()
> > 	cpts->cc.mult = cpts->cc_mult;
> > 
> > There is no need for such complexity.  Simply set cc.mult in
> > cpts_create() _once_, immediately after the call to
> > cpts_calc_mult_shift().
> > 
> > You can remove the assignment from cpts_calc_mult_shift() and
> > cpts_register().
> 
> Just to clarify: do you propose to get rid of cpts->cc_mult at all?

No.  Read what I said before:

   There is no need for such complexity.  Simply set cc.mult in
   cpts_create() _once_, immediately after the call to
   cpts_calc_mult_shift().

> Honestly, i'd not prefer to change functional behavior of ptp clock as part of
> this series.

Fair enough.  The bogus clock reset existed before your series.

But please don't obfuscate simple initialization routines.  The way
you set cc.mult and cc_mult is illogical and convoluted.

Thanks,
Richard
--
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

* [PATCH 5/5] ARM: BCM53573: Specify USB ports of on-SoC controllers
From: Rafał Miłecki @ 2016-12-06 17:17 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Arnd Bergmann, Rob Herring, Mark Rutland, Russell King,
	Hauke Mehrtens, bcm-kernel-feedback-list, devicetree,
	linux-arm-kernel, linux-kernel, Rafał Miłecki
In-Reply-To: <20161206171714.22738-1-zajec5@gmail.com>

From: Rafał Miłecki <rafal@milecki.pl>

Broadcom OHCI and EHCI controllers always have 2 ports each on the root
hub. Describe them in DT to allow specifying extra info or referencing
port nodes.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 arch/arm/boot/dts/bcm53573.dtsi | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm/boot/dts/bcm53573.dtsi b/arch/arm/boot/dts/bcm53573.dtsi
index e2c496a..2da04d0 100644
--- a/arch/arm/boot/dts/bcm53573.dtsi
+++ b/arch/arm/boot/dts/bcm53573.dtsi
@@ -124,6 +124,17 @@
 				reg = <0x4000 0x1000>;
 				interrupt-parent = <&gic>;
 				interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
+
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				ehci_port1: port@1 {
+					reg = <1>;
+				};
+
+				ehci_port2: port@2 {
+					reg = <2>;
+				};
 			};
 
 			ohci: ohci@d000 {
@@ -133,6 +144,17 @@
 				reg = <0xd000 0x1000>;
 				interrupt-parent = <&gic>;
 				interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
+
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				ohci_port1: port@1 {
+					reg = <1>;
+				};
+
+				ohci_port2: port@2 {
+					reg = <2>;
+				};
 			};
 		};
 
-- 
2.10.1

^ 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