Devicetree
 help / color / mirror / Atom feed
* RE: [PATCH v2 1/2] dt-binding: regulator: anatop: make regulator name property required
From: A.S. Dong @ 2017-04-19 15:33 UTC (permalink / raw)
  To: A.S. Dong, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
  Cc: Rob Herring, Mark Rutland,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1492180234-2496-1-git-send-email-aisheng.dong-3arQi8VN3Tc@public.gmane.org>

> -----Original Message-----
> From: Dong Aisheng [mailto:aisheng.dong-3arQi8VN3Tc@public.gmane.org]
> Sent: Friday, April 14, 2017 10:31 PM
> To: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: A.S. Dong; Rob Herring; Mark Rutland; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: [PATCH v2 1/2] dt-binding: regulator: anatop: make regulator name
> property required
> 
> We actually can't allow the missing of the regualor name, thus update the
> binding doc to make regulator-name property to be required.
> 
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Dong Aisheng <aisheng.dong-3arQi8VN3Tc@public.gmane.org>

Sorry, Mark, missed you about this one.

Regards
Dong Aisheng

> ---
>  Documentation/devicetree/bindings/regulator/anatop-regulator.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/anatop-
> regulator.txt b/Documentation/devicetree/bindings/regulator/anatop-
> regulator.txt
> index 1d58c8c..a3106c7 100644
> --- a/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
> +++ b/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
> @@ -2,6 +2,7 @@ Anatop Voltage regulators
> 
>  Required properties:
>  - compatible: Must be "fsl,anatop-regulator"
> +- regulator-name: A string used as a descriptive name for regulator
> +outputs
>  - anatop-reg-offset: Anatop MFD register offset
>  - anatop-vol-bit-shift: Bit shift for the register
>  - anatop-vol-bit-width: Number of bits used in the register
> --
> 2.7.4

--
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 v6 6/8] coresight: add support for CPU debug module
From: Leo Yan @ 2017-04-19 15:30 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
	Catalin Marinas, Will Deacon, Andy Gross, David Brown,
	Suzuki K Poulose, Stephen Boyd, linux-doc,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-arm-msm, linux-soc,
	Mike Leach, Sudeep Holla
In-Reply-To: <CANLsYkyheVJkEjpcWAd5HHFCimfTPhgihioE7bYo1RAL=8Shdw@mail.gmail.com>

On Wed, Apr 19, 2017 at 08:52:12AM -0600, Mathieu Poirier wrote:

[...]

> >> > +static bool debug_enable;
> >> > +module_param_named(enable, debug_enable, bool, 0600);
> >> > +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
> >> > +            "(default is 0, which means is disabled by default)");
> >>
> >> For this driver we have a debugFS interface so I question the validity of a
> >> kernel module parameter.  Other than adding complexity to the code it offers no
> >> real added value.  If a user is to insmod a module, it is just as easy to switch
> >> on the functionality using debugFS in a second step.
> >
> > This module parameter can be used for kernel command line, so
> > it's useful when user wants to dynamically turn on/off the
> > functionality at boot up time.
> >
> > Does this make sense for you? Removing this parameter is okay for
> > me, but this means users need to decide if use it by Kernel config
> > with static building in. This is a bit contradictory with before's
> > discussion.
> 
> My hope was to use the kernel command line and the debugFS interface,
> avoiding the module parameter.  Look at what the baycom_par and
> blacklist drivers are doing with the "__setup()" function and see if
> we can void a module parameter.  If not then let it be, unless someone
> else has a better idea.
> 
> [1]. drivers/net/hamradio/baycom_par.c
> [2]. drivers/s390/cio/blacklist.c

This driver supports module mode. So we can choose to use either module
parameter or __setup(). But as described in the file
Documentation/admin-guide/kernel-parameters.rst, the module parameter
is more flexible to be used at boot time or insmod the module:

"Parameters for modules which are built into the kernel need to be
specified on the kernel command line.  modprobe looks through the 
kernel command line (/proc/cmdline) and collects module parameters
when it loads a module, so the kernel command line can be used for 
loadable modules too."

__setup() cannot support module mode, and when use __setup(), we need
register one callback function for it. The callback function is
friendly to parse complex parameters rather than module parameter.
but it's not necessary for this case.

So I'm bias to use module parameter :)

Thanks,
Leo Yan

^ permalink raw reply

* Re: [RFC 2/2] mux: mmio-based syscon mux controller
From: Philipp Zabel @ 2017-04-19 15:27 UTC (permalink / raw)
  To: Peter Rosin
  Cc: Steve Longerbeam, Rob Herring, Mark Rutland, Sakari Ailus,
	devicetree, linux-kernel, kernel
In-Reply-To: <e4d4b515-e799-9594-f88a-de9d8a5d14bb@axentia.se>

On Wed, 2017-04-19 at 13:58 +0200, Peter Rosin wrote:
> On 2017-04-19 13:50, Philipp Zabel wrote:
> > On Thu, 2017-04-13 at 18:09 -0700, Steve Longerbeam wrote:
> >>
> >> On 04/13/2017 08:48 AM, Philipp Zabel wrote:
> >>> This adds a driver for mmio-based syscon multiplexers controlled by a
> >>> single bitfield in a syscon register range.
> >>>
> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >>> ---
> >>>   drivers/mux/Kconfig      |  13 +++++
> >>>   drivers/mux/Makefile     |   1 +
> >>>   drivers/mux/mux-syscon.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++
> >>>   3 files changed, 144 insertions(+)
> >>>   create mode 100644 drivers/mux/mux-syscon.c
> >>>
> >>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> >>> index 86668b4d2fc52..a5e6a3b01ac24 100644
> >>> --- a/drivers/mux/Kconfig
> >>> +++ b/drivers/mux/Kconfig
> >>> @@ -43,4 +43,17 @@ config MUX_GPIO
> >>>   	  To compile the driver as a module, choose M here: the module will
> >>>   	  be called mux-gpio.
> >>>   
> >>> +config MUX_SYSCON
> >>
> >> my preference would be CONFIG_MUX_MMIO.
> >>
> >>> +	tristate "MMIO bitfield-controlled Multiplexer"
> >>
> >> "MMIO register bitfield-controlled Multiplexer"
> >>
> >> The rest looks good to me.
> > 
> > I'll change those. mux-syscon.c should probably be renamed to
> > mux-mmio.c, too.
> 
> I think I disagree. But I'm not familiar with syscon so I don't know.
> IIUC, syscon uses regmap to do mmio and this driver requires syscon
> to get at the regmap, and in the end this driver doesn't know anything
> about mmio. All it knows is syscon/regmap.

That is a good point. Right now there is nothing MMIO about the driver
except for the hardware that I want it to handle.

>  If some warped syscon
> thing shows up that wraps something other than mmio in its regmap,
> this driver wouldn't care about it. And syscon is something that
> is also known in the DT world. Given that, I think everything in this
> driver should be named syscon and not mmio.
> 
> Or?

Well, ...

the driver could be extended to do actual MMIO if the syscon is not
found. This would work only if it has exclusive access to its register.

On the other hand, the driver could also be made to match against
    compatible = "bitfield-mux",
for example, and allow handling muxes inside SPI or I2C controlled MFD
devices that provide a syscon regmap, as you describe:

	spi-host {
		mfd-device {
			compatible = "some-spi-regmap-device";

			mux {
				compatible = "bitfield-mux";
			};
		};
	};

regards
Philipp

^ permalink raw reply

* Re: [PATCH net-next v3] bindings: net: stmmac: add missing note about LPI interrupt
From: Joao Pinto @ 2017-04-19 15:22 UTC (permalink / raw)
  To: Niklas Cassel, Rob Herring, Mark Rutland, David S. Miller,
	Joao Pinto, Niklas Cassel, Alexandre TORGUE, Giuseppe CAVALLARO,
	Thierry Reding, Eric Engestrom
  Cc: netdev, devicetree, linux-kernel
In-Reply-To: <20170418123955.21335-1-niklass@axis.com>

Hi Niklas,

Às 1:39 PM de 4/18/2017, Niklas Cassel escreveu:
> From: Niklas Cassel <niklas.cassel@axis.com>
> 
> The hardware has a LPI interrupt.
> There is already code in the stmmac driver to parse and handle the
> interrupt. However, this information was missing from the DT binding.
> 
> At the same time, improve the description of the existing interrupts.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  Documentation/devicetree/bindings/net/stmmac.txt | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
> index f652b0c384ce..c3a7be6615c5 100644
> --- a/Documentation/devicetree/bindings/net/stmmac.txt
> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
> @@ -7,9 +7,12 @@ Required properties:
>  - interrupt-parent: Should be the phandle for the interrupt controller
>    that services interrupts for this device
>  - interrupts: Should contain the STMMAC interrupts
> -- interrupt-names: Should contain the interrupt names "macirq"
> -  "eth_wake_irq" if this interrupt is supported in the "interrupts"
> -  property
> +- interrupt-names: Should contain a list of interrupt names corresponding to
> +	the interrupts in the interrupts property, if available.
> +	Valid interrupt names are:
> +  - "macirq" (combined signal for various interrupt events)
> +  - "eth_wake_irq" (the interrupt to manage the remote wake-up packet detection)
> +  - "eth_lpi" (the interrupt that occurs when Tx or Rx enters/exits LPI state)
>  - phy-mode: See ethernet.txt file in the same directory.
>  - snps,reset-gpio 	gpio number for phy reset.
>  - snps,reset-active-low boolean flag to indicate if phy reset is active low.
> @@ -152,8 +155,8 @@ Examples:
>  		compatible = "st,spear600-gmac";
>  		reg = <0xe0800000 0x8000>;
>  		interrupt-parent = <&vic1>;
> -		interrupts = <24 23>;
> -		interrupt-names = "macirq", "eth_wake_irq";
> +		interrupts = <24 23 22>;
> +		interrupt-names = "macirq", "eth_wake_irq", "eth_lpi";
>  		mac-address = [000000000000]; /* Filled in by U-Boot */
>  		max-frame-size = <3800>;
>  		phy-mode = "gmii";
> 

Acked-By: Joao Pinto <jpinto@synopsys.com>

Regards!

^ permalink raw reply

* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
From: Leo Yan @ 2017-04-19 15:07 UTC (permalink / raw)
  To: Suzuki K Poulose
  Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
	Catalin Marinas, Will Deacon, Andy Gross, David Brown,
	Mathieu Poirier, Stephen Boyd, linux-doc, linux-kernel,
	devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
	Mike Leach, Sudeep Holla
In-Reply-To: <f42088f6-aa53-06b1-b332-22198744ab7a@arm.com>

On Wed, Apr 19, 2017 at 03:32:39PM +0100, Suzuki K Poulose wrote:

[...]

> >>>+static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> >>>+{
> >>>+	void __iomem *base;
> >>>+	struct device *dev = &adev->dev;
> >>>+	struct debug_drvdata *drvdata;
> >>>+	struct resource *res = &adev->res;
> >>>+	struct device_node *np = adev->dev.of_node;
> >>>+	int ret;
> >>>+
> >>>+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> >>>+	if (!drvdata)
> >>>+		return -ENOMEM;
> >>>+
> >>>+	drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> >>>+	if (per_cpu(debug_drvdata, drvdata->cpu)) {
> >>>+		dev_err(dev, "CPU%d drvdata has been initialized\n",
> >>>+			drvdata->cpu);
> >>
> >>May be we could warn about a possible issue in the DT ?
> >
> >So can I understand the suggestion is to add warning in function
> >of_coresight_get_cpu() when cannot find CPU number, and here directly
> >bail out?
> 
> No. One could have single CPU DT, where he doesn't need to provide the CPU number.
> Hence, it doesn't make sense to WARN  in of_coresight_get_cpu().
> 
> But when we hit the case above, we find that the some node was registered for
> the given CPU (be it 0 or any other), which is definitely an error in DT. Due to
> 
> 1) Hasn't specified the CPU number for more than one node
> 
> OR
> 
> 2) CPU number duplicated in the more than one nodes.

Thanks for explaination. It's clear for me now.

> Cheers
> Suzuki

^ permalink raw reply

* Re: [PATCH 0/4] ARM: sunxi: device tree pinctrl clean up and H3 OTG
From: Maxime Ripard @ 2017-04-19 14:54 UTC (permalink / raw)
  To: Chen-Yu Tsai
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20170419050919.6762-1-wens-jdAy2FN1RRM@public.gmane.org>

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

On Wed, Apr 19, 2017 at 01:09:15PM +0800, Chen-Yu Tsai wrote:
> Hi Maxime,
> 
> This series has 2 parts. The parts are largely unrelated, though the
> second part should be applied after the first part, so we don't
> accidentally mux pins that we shouldn't. Hence I'm sending them
> together.
> 
> The first 2 patches clean up the sunxi device tree files, removing
> pinmux settings for common GPIO pins. These include the enable pins
> for the common regulators, and the mmc0 card detect pin from the
> reference designs.
> 
> The second part, the latter 2 patches, enable USB OTG on the Orangepi
> PC, PC Plus, Plus 2E, and the Bananapi M2+. The first 3 boards are
> bunched together, due to how the PC Plus and Plus 2E device trees include
> the device tree of the Opi PC.

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply

* Re: [PATCH] arm64: dts: allwinner: pine64: Prepare optional UART nodes with pinctrl
From: Maxime Ripard @ 2017-04-19 14:52 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Chen-Yu Tsai, Rob Herring, Mark Rutland, Catalin Marinas,
	Will Deacon, linux-arm-kernel, devicetree, linux-kernel
In-Reply-To: <20170418192538.24174-1-afaerber@suse.de>

[-- Attachment #1: Type: text/plain, Size: 780 bytes --]

On Tue, Apr 18, 2017 at 09:25:38PM +0200, Andreas Färber wrote:
> Pine64 exposes all A64 UARTs, not just UART0.
> 
> Since the pins can be used as GPIO, don't enable the new UART nodes by
> default, but prepare the pinctrl settings to aid in activating them via
> overlays, i.e., overriding the status property of &uartX nodes.
> 
> For UART4 (Euler) the safer route of not including RTS/CTS pins is chosen,
> whereas for UART1 (Bluetooth) they are included.
> 
> Add the corresponding pinctrl nodes where missing.
> 
> Suggested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply

* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
From: Mathieu Poirier @ 2017-04-19 14:52 UTC (permalink / raw)
  To: Leo Yan
  Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
	Catalin Marinas, Will Deacon, Andy Gross, David Brown,
	Suzuki K Poulose, Stephen Boyd, linux-doc,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-arm-msm, linux-soc,
	Mike Leach, Sudeep Holla
In-Reply-To: <20170419001807.GB8524@leoy-linaro>

On 18 April 2017 at 18:18, Leo Yan <leo.yan@linaro.org> wrote:
> On Tue, Apr 18, 2017 at 11:40:50AM -0600, Mathieu Poirier wrote:
>> On Thu, Apr 06, 2017 at 09:30:59PM +0800, Leo Yan wrote:
>> > Coresight includes debug module and usually the module connects with CPU
>> > debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
>> > description for related info in "Part H: External Debug".
>> >
>> > Chapter H7 "The Sample-based Profiling Extension" introduces several
>> > sampling registers, e.g. we can check program counter value with
>> > combined CPU exception level, secure state, etc. So this is helpful for
>> > analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
>> > loop with IRQ disabled. In this case the CPU cannot switch context and
>> > handle any interrupt (including IPIs), as the result it cannot handle
>> > SMP call for stack dump.
>> >
>> > This patch is to enable coresight debug module, so firstly this driver
>> > is to bind apb clock for debug module and this is to ensure the debug
>> > module can be accessed from program or external debugger. And the driver
>> > uses sample-based registers for debug purpose, e.g. when system triggers
>> > panic, the driver will dump program counter and combined context
>> > registers (EDCIDSR, EDVIDSR); by parsing context registers so can
>> > quickly get to know CPU secure state, exception level, etc.
>> >
>> > Some of the debug module registers are located in CPU power domain, so
>> > this requires the CPU power domain stays on when access related debug
>> > registers, but the power management for CPU power domain is quite
>> > dependent on SoC integration for power management. For the platforms
>> > which with sane power controller implementations, this driver follows
>> > the method to set EDPRCR to try to pull the CPU out of low power state
>> > and then set 'no power down request' bit so the CPU has no chance to
>> > lose power.
>> >
>> > If the SoC has not followed up this design well for power management
>> > controller, the user should use the command line parameter or sysfs
>> > to constrain all or partial idle states to ensure the CPU power
>> > domain is enabled and access coresight CPU debug component safely.
>> >
>> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
>>
>> This is coming along well - a few comment below.  In your next revision please
>> add GKH to the 'To' list.
>
> Sure. Will send out in this week and add Greg in 'To' list.
>
>> > ---
>> >  drivers/hwtracing/coresight/Kconfig               |  14 +
>> >  drivers/hwtracing/coresight/Makefile              |   1 +
>> >  drivers/hwtracing/coresight/coresight-cpu-debug.c | 667 ++++++++++++++++++++++
>> >  3 files changed, 682 insertions(+)
>> >  create mode 100644 drivers/hwtracing/coresight/coresight-cpu-debug.c
>> >
>> > diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> > index 130cb21..8d55d6d 100644
>> > --- a/drivers/hwtracing/coresight/Kconfig
>> > +++ b/drivers/hwtracing/coresight/Kconfig
>> > @@ -89,4 +89,18 @@ config CORESIGHT_STM
>> >       logging useful software events or data coming from various entities
>> >       in the system, possibly running different OSs
>> >
>> > +config CORESIGHT_CPU_DEBUG
>> > +   tristate "CoreSight CPU Debug driver"
>> > +   depends on ARM || ARM64
>> > +   depends on DEBUG_FS
>> > +   help
>> > +     This driver provides support for coresight debugging module. This
>> > +     is primarily used to dump sample-based profiling registers when
>> > +     system triggers panic, the driver will parse context registers so
>> > +     can quickly get to know program counter (PC), secure state,
>> > +     exception level, etc. Before use debugging functionality, platform
>> > +     needs to ensure the clock domain and power domain are enabled
>> > +     properly, please refer Documentation/trace/coresight-cpu-debug.txt
>> > +     for detailed description and the example for usage.
>> > +
>> >  endif
>> > diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
>> > index af480d9..433d590 100644
>> > --- a/drivers/hwtracing/coresight/Makefile
>> > +++ b/drivers/hwtracing/coresight/Makefile
>> > @@ -16,3 +16,4 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
>> >                                     coresight-etm4x-sysfs.o
>> >  obj-$(CONFIG_CORESIGHT_QCOM_REPLICATOR) += coresight-replicator-qcom.o
>> >  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
>> > +obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
>> > diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> > new file mode 100644
>> > index 0000000..8470e31
>> > --- /dev/null
>> > +++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
>> > @@ -0,0 +1,667 @@
>> > +/*
>> > + * Copyright (c) 2017 Linaro Limited. All rights reserved.
>> > + *
>> > + * Author: Leo Yan <leo.yan@linaro.org>
>> > + *
>> > + * 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.
>> > + *
>> > + * This program is distributed in the hope that it will be useful, but WITHOUT
>> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> > + * more details.
>> > + *
>> > + * You should have received a copy of the GNU General Public License along with
>> > + * this program.  If not, see <http://www.gnu.org/licenses/>.
>> > + *
>> > + */
>> > +#include <linux/amba/bus.h>
>> > +#include <linux/coresight.h>
>> > +#include <linux/cpu.h>
>> > +#include <linux/debugfs.h>
>> > +#include <linux/delay.h>
>> > +#include <linux/device.h>
>> > +#include <linux/err.h>
>> > +#include <linux/init.h>
>> > +#include <linux/io.h>
>> > +#include <linux/iopoll.h>
>> > +#include <linux/kernel.h>
>> > +#include <linux/module.h>
>> > +#include <linux/moduleparam.h>
>> > +#include <linux/pm_qos.h>
>> > +#include <linux/slab.h>
>> > +#include <linux/smp.h>
>> > +#include <linux/types.h>
>> > +#include <linux/uaccess.h>
>> > +
>> > +#include "coresight-priv.h"
>> > +
>> > +#define EDPCSR                             0x0A0
>> > +#define EDCIDSR                            0x0A4
>> > +#define EDVIDSR                            0x0A8
>> > +#define EDPCSR_HI                  0x0AC
>> > +#define EDOSLAR                            0x300
>> > +#define EDPRCR                             0x310
>> > +#define EDPRSR                             0x314
>> > +#define EDDEVID1                   0xFC4
>> > +#define EDDEVID                            0xFC8
>> > +
>> > +#define EDPCSR_PROHIBITED          0xFFFFFFFF
>> > +
>> > +/* bits definition for EDPCSR */
>> > +#define EDPCSR_THUMB                       BIT(0)
>> > +#define EDPCSR_ARM_INST_MASK               GENMASK(31, 2)
>> > +#define EDPCSR_THUMB_INST_MASK             GENMASK(31, 1)
>> > +
>> > +/* bits definition for EDPRCR */
>> > +#define EDPRCR_COREPURQ                    BIT(3)
>> > +#define EDPRCR_CORENPDRQ           BIT(0)
>> > +
>> > +/* bits definition for EDPRSR */
>> > +#define EDPRSR_DLK                 BIT(6)
>> > +#define EDPRSR_PU                  BIT(0)
>> > +
>> > +/* bits definition for EDVIDSR */
>> > +#define EDVIDSR_NS                 BIT(31)
>> > +#define EDVIDSR_E2                 BIT(30)
>> > +#define EDVIDSR_E3                 BIT(29)
>> > +#define EDVIDSR_HV                 BIT(28)
>> > +#define EDVIDSR_VMID                       GENMASK(7, 0)
>> > +
>> > +/*
>> > + * bits definition for EDDEVID1:PSCROffset
>> > + *
>> > + * NOTE: armv8 and armv7 have different definition for the register,
>> > + * so consolidate the bits definition as below:
>> > + *
>> > + * 0b0000 - Sample offset applies based on the instruction state, we
>> > + *          rely on EDDEVID to check if EDPCSR is implemented or not
>> > + * 0b0001 - No offset applies.
>> > + * 0b0010 - No offset applies, but do not use in AArch32 mode
>> > + *
>> > + */
>> > +#define EDDEVID1_PCSR_OFFSET_MASK  GENMASK(3, 0)
>> > +#define EDDEVID1_PCSR_OFFSET_INS_SET       (0x0)
>> > +#define EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32        (0x2)
>> > +
>> > +/* bits definition for EDDEVID */
>> > +#define EDDEVID_PCSAMPLE_MODE              GENMASK(3, 0)
>> > +#define EDDEVID_IMPL_EDPCSR                (0x1)
>> > +#define EDDEVID_IMPL_EDPCSR_EDCIDSR        (0x2)
>> > +#define EDDEVID_IMPL_FULL          (0x3)
>> > +
>> > +#define DEBUG_WAIT_SLEEP           1000
>> > +#define DEBUG_WAIT_TIMEOUT         32000
>> > +
>> > +struct debug_drvdata {
>> > +   void __iomem    *base;
>> > e   struct device   *dev;
>> > +   int             cpu;
>> > +
>> > +   bool            edpcsr_present;
>> > +   bool            edcidsr_present;
>> > +   bool            edvidsr_present;
>> > +   bool            pc_has_offset;
>> > +
>> > +   u32             edpcsr;
>> > +   u32             edpcsr_hi;
>> > +   u32             edprsr;
>> > +   u32             edvidsr;
>> > +   u32             edcidsr;
>> > +};
>> > +
>> > +static DEFINE_MUTEX(debug_lock);
>> > +static DEFINE_PER_CPU(struct debug_drvdata *, debug_drvdata);
>> > +static int debug_count;
>> > +static struct dentry *debug_debugfs_dir;
>> > +
>> > +static bool debug_enable;
>> > +module_param_named(enable, debug_enable, bool, 0600);
>> > +MODULE_PARM_DESC(enable, "Knob to enable debug functionality "
>> > +            "(default is 0, which means is disabled by default)");
>>
>> For this driver we have a debugFS interface so I question the validity of a
>> kernel module parameter.  Other than adding complexity to the code it offers no
>> real added value.  If a user is to insmod a module, it is just as easy to switch
>> on the functionality using debugFS in a second step.
>
> This module parameter can be used for kernel command line, so
> it's useful when user wants to dynamically turn on/off the
> functionality at boot up time.
>
> Does this make sense for you? Removing this parameter is okay for
> me, but this means users need to decide if use it by Kernel config
> with static building in. This is a bit contradictory with before's
> discussion.

My hope was to use the kernel command line and the debugFS interface,
avoiding the module parameter.  Look at what the baycom_par and
blacklist drivers are doing with the "__setup()" function and see if
we can void a module parameter.  If not then let it be, unless someone
else has a better idea.

[1]. drivers/net/hamradio/baycom_par.c
[2]. drivers/se90/cio/blacklist.c

>
>> > +static void debug_os_unlock(struct debug_drvdata *drvdata)
>> > +{
>> > +   /* Unlocks the debug registers */
>> > +   writel_relaxed(0x0, drvdata->base + EDOSLAR);
>>
>> Here the wmb is to make sure reordering (either at compile time or in the CPU)
>> doesn't happend.  You need a comment here otherwise checkpatch will complain.
>> Speaking of which, did you run this through checkpatch?
>
> Right. Everytime I run checkpatch before sending patch and it has
> complain for this. Will add comment. Sorry for imprecise working.
>
>> > +   wmb();
>> > +}
>> > +
>> > +/*
>> > + * According to ARM DDI 0487A.k, before access external debug
>> > + * registers should firstly check the access permission; if any
>> > + * below condition has been met then cannot access debug
>> > + * registers to avoid lockup issue:
>> > + *
>> > + * - CPU power domain is powered off;
>> > + * - The OS Double Lock is locked;
>> > + *
>> > + * By checking EDPRSR can get to know if meet these conditions.
>> > + */
>> > +static bool debug_access_permitted(struct debug_drvdata *drvdata)
>> > +{
>> > +   /* CPU is powered off */
>> > +   if (!(drvdata->edprsr & EDPRSR_PU))
>> > +           return false;
>> > +
>> > +   /* The OS Double Lock is locked */
>> > +   if (drvdata->edprsr & EDPRSR_DLK)
>> > +           return false;
>> > +
>> > +   return true;
>> > +}
>> > +
>> > +static void debug_force_cpu_powered_up(struct debug_drvdata *drvdata)
>> > +{
>> > +   bool retried = false;
>> > +   u32 edprcr;
>> > +
>> > +try_again:
>> > +
>> > +   /*
>> > +    * Send request to power management controller and assert
>> > +    * DBGPWRUPREQ signal; if power management controller has
>> > +    * sane implementation, it should enable CPU power domain
>> > +    * in case CPU is in low power state.
>> > +    */
>> > +   edprcr = readl_relaxed(drvdata->base + EDPRCR);
>> > +   edprcr |= EDPRCR_COREPURQ;
>> > +   writel_relaxed(edprcr, drvdata->base + EDPRCR);
>> > +
>> > +   /* Wait for CPU to be powered up (timeout~=32ms) */
>> > +   if (readx_poll_timeout_atomic(readl_relaxed, drvdata->base + EDPRSR,
>> > +                   drvdata->edprsr, (drvdata->edprsr & EDPRSR_PU),
>> > +                   DEBUG_WAIT_SLEEP, DEBUG_WAIT_TIMEOUT)) {
>> > +           /*
>> > +            * Unfortunately the CPU cannot be powered up, so return
>> > +            * back and later has no permission to access other
>> > +            * registers. For this case, should disable CPU low power
>> > +            * states to ensure CPU power domain is enabled!
>> > +            */
>> > +           pr_err("%s: power up request for CPU%d failed\n",
>> > +                   __func__, drvdata->cpu);
>> > +           return;
>> > +   }
>> > +
>> > +   /*
>> > +    * At this point the CPU is powered up, so set the no powerdown
>> > +    * request bit so we don't lose power and emulate power down.
>> > +    */
>> > +   edprcr = readl_relaxed(drvdata->base + EDPRCR);
>> > +   edprcr |= EDPRCR_COREPURQ | EDPRCR_CORENPDRQ;
>> > +   writel_relaxed(edprcr, drvdata->base + EDPRCR);
>> > +
>> > +   drvdata->edprsr = readl_relaxed(drvdata->base + EDPRSR);
>> > +
>> > +   /* Bail out if CPU is powered up */
>> > +   if (likely(drvdata->edprsr & EDPRSR_PU))
>> > +           return;
>>
>>         /* The core power domain got switched off on use, try again */
>>         if (unlikely(!drvdata->edprsr & EDPRSR_PU))
>>                 goto try_again;
>>
>> I understand you don't want to introduce a infinite loop but if that happens
>> here, something else has gone very wrong.  The above readx_poll_timeout_atomic
>> loop should take care of bailing out in case of problems.  That way you also get
>> to rid of the retried variable and the code is more simple.
>
> Okay. I will follow your method. Thinking the duration of CPU power
> on/off is not same magnitude with this piece code, the infinite loop
> will not happen.
>
>> > +
>> > +   /*
>> > +    * Handle race condition if CPU has been waken up but it sleeps
>> > +    * again if EDPRCR_CORENPDRQ has been flipped, so try to run
>> > +    * waken flow one more time.
>> > +    */
>> > +   if (!retried) {
>> > +           retried = true;
>> > +           goto try_again;
>> > +   }
>> > +}
>> > +
>> > +static void debug_read_regs(struct debug_drvdata *drvdata)
>> > +{
>> > +   u32 save_edprcr;
>> > +
>> > +   CS_UNLOCK(drvdata->base);
>> > +
>> > +   /* Unlock os lock */
>> > +   debug_os_unlock(drvdata);
>> > +
>> > +   /* Save EDPRCR register */
>> > +   save_edprcr = readl_relaxed(drvdata->base + EDPRCR);
>> > +
>> > +   /*
>> > +    * Ensure CPU power domain is enabled to let registers
>> > +    * are accessiable.
>> > +    */
>> > +   debug_force_cpu_powered_up(drvdata);
>> > +
>> > +   if (!debug_access_permitted(drvdata))
>> > +           goto out;
>> > +
>> > +   drvdata->edpcsr = readl_relaxed(drvdata->base + EDPCSR);
>> > +
>> > +   /*
>> > +    * As described in ARM DDI 0487A.k, if the processing
>> > +    * element (PE) is in debug state, or sample-based
>> > +    * profiling is prohibited, EDPCSR reads as 0xFFFFFFFF;
>> > +    * EDCIDSR, EDVIDSR and EDPCSR_HI registers also become
>> > +    * UNKNOWN state. So directly bail out for this case.
>> > +    */
>> > +   if (drvdata->edpcsr == EDPCSR_PROHIBITED)
>> > +           goto out;
>> > +
>> > +   /*
>> > +    * A read of the EDPCSR normally has the side-effect of
>> > +    * indirectly writing to EDCIDSR, EDVIDSR and EDPCSR_HI;
>> > +    * at this point it's safe to read value from them.
>> > +    */
>> > +   if (IS_ENABLED(CONFIG_64BIT))
>> > +           drvdata->edpcsr_hi = readl_relaxed(drvdata->base + EDPCSR_HI);
>> > +
>> > +   if (drvdata->edcidsr_present)
>> > +           drvdata->edcidsr = readl_relaxed(drvdata->base + EDCIDSR);
>> > +
>> > +   if (drvdata->edvidsr_present)
>> > +           drvdata->edvidsr = readl_relaxed(drvdata->base + EDVIDSR);
>> > +
>> > +out:
>> > +   /* Restore EDPRCR register */
>> > +   writel_relaxed(save_edprcr, drvdata->base + EDPRCR);
>> > +
>> > +   CS_LOCK(drvdata->base);
>> > +}
>> > +
>> > +static unsigned long debug_adjust_pc(struct debug_drvdata *drvdata)
>> > +{
>> > +   unsigned long arm_inst_offset = 0, thumb_inst_offset = 0;
>> > +   unsigned long pc;
>> > +
>> > +   if (IS_ENABLED(CONFIG_64BIT))
>> > +           return (unsigned long)drvdata->edpcsr_hi << 32 |
>> > +                  (unsigned long)drvdata->edpcsr;
>> > +
>> > +   pc = (unsigned long)drvdata->edpcsr;
>> > +
>> > +   if (drvdata->pc_has_offset) {
>> > +           arm_inst_offset = 8;
>> > +           thumb_inst_offset = 4;
>> > +   }
>> > +
>> > +   /* Handle thumb instruction */
>> > +   if (pc & EDPCSR_THUMB) {
>> > +           pc = (pc & EDPCSR_THUMB_INST_MASK) - thumb_inst_offset;
>> > +           return pc;
>> > +   }
>> > +
>> > +   /*
>> > +    * Handle arm instruction offset, if the arm instruction
>> > +    * is not 4 byte alignment then it's possible the case
>> > +    * for implementation defined; keep original value for this
>> > +    * case and print info for notice.
>> > +    */
>> > +   if (pc & BIT(1))
>> > +           pr_emerg("Instruction offset is implementation defined\n");
>> > +   else
>> > +           pc = (pc & EDPCSR_ARM_INST_MASK) - arm_inst_offset;
>> > +
>> > +   return pc;
>> > +}
>> > +
>> > +static void debug_dump_regs(struct debug_drvdata *drvdata)
>> > +{
>> > +   unsigned long pc;
>> > +
>> > +   pr_emerg("\tEDPRSR:  %08x (Power:%s DLK:%s)\n", drvdata->edprsr,
>> > +            drvdata->edprsr & EDPRSR_PU ? "On" : "Off",
>> > +            drvdata->edprsr & EDPRSR_DLK ? "Lock" : "Unlock");
>> > +
>> > +   if (!debug_access_permitted(drvdata)) {
>> > +           pr_emerg("No permission to access debug registers!\n");
>> > +           return;
>> > +   }
>> > +
>> > +   if (drvdata->edpcsr == EDPCSR_PROHIBITED) {
>> > +           pr_emerg("CPU is in Debug state or profiling is prohibited!\n");
>> > +           return;
>> > +   }
>> > +
>> > +   pc = debug_adjust_pc(drvdata);
>> > +   pr_emerg("\tEDPCSR:  [<%p>] %pS\n", (void *)pc, (void *)pc);
>> > +
>> > +   if (drvdata->edcidsr_present)
>> > +           pr_emerg("\tEDCIDSR: %08x\n", drvdata->edcidsr);
>> > +
>> > +   if (drvdata->edvidsr_present)
>> > +           pr_emerg("\tEDVIDSR: %08x (State:%s Mode:%s Width:%dbits VMID:%x)\n",
>> > +                    drvdata->edvidsr,
>> > +                    drvdata->edvidsr & EDVIDSR_NS ? "Non-secure" : "Secure",
>> > +                    drvdata->edvidsr & EDVIDSR_E3 ? "EL3" :
>> > +                           (drvdata->edvidsr & EDVIDSR_E2 ? "EL2" : "EL1/0"),
>> > +                    drvdata->edvidsr & EDVIDSR_HV ? 64 : 32,
>> > +                    drvdata->edvidsr & (u32)EDVIDSR_VMID);
>> > +}
>> > +
>> > +static void debug_init_arch_data(void *info)
>> > +{
>> > +   struct debug_drvdata *drvdata = info;
>> > +   u32 mode, pcsr_offset;
>> > +   u32 eddevid, eddevid1;
>> > +
>> > +   CS_UNLOCK(drvdata->base);
>> > +
>> > +   /* Read device info */
>> > +   eddevid  = readl_relaxed(drvdata->base + EDDEVID);
>> > +   eddevid1 = readl_relaxed(drvdata->base + EDDEVID1);
>> > +
>> > +   CS_LOCK(drvdata->base);
>> > +
>> > +   /* Parse implementation feature */
>> > +   mode = eddevid & EDDEVID_PCSAMPLE_MODE;
>> > +   pcsr_offset = eddevid1 & EDDEVID1_PCSR_OFFSET_MASK;
>> > +
>> > +   drvdata->edpcsr_present  = false;
>> > +   drvdata->edcidsr_present = false;
>> > +   drvdata->edvidsr_present = false;
>> > +   drvdata->pc_has_offset   = false;
>> > +
>> > +   switch (mode) {
>> > +   case EDDEVID_IMPL_FULL:
>> > +           drvdata->edvidsr_present = true;
>> > +           /* Fall through */
>> > +   case EDDEVID_IMPL_EDPCSR_EDCIDSR:
>> > +           drvdata->edcidsr_present = true;
>> > +           /* Fall through */
>> > +   case EDDEVID_IMPL_EDPCSR:
>> > +           /*
>> > +            * In ARM DDI 0487A.k, the EDDEVID1.PCSROffset is used to
>> > +            * define if has the offset for PC sampling value; if read
>> > +            * back EDDEVID1.PCSROffset == 0x2, then this means the debug
>> > +            * module does not sample the instruction set state when
>> > +            * armv8 CPU in AArch32 state.
>> > +            */
>> > +           drvdata->edpcsr_present = (IS_ENABLED(CONFIG_64BIT) ||
>> > +                   (pcsr_offset != EDDEVID1_PCSR_NO_OFFSET_DIS_AARCH32));
>> > +
>> > +           drvdata->pc_has_offset =
>> > +                   (pcsr_offset == EDDEVID1_PCSR_OFFSET_INS_SET);
>> > +           break;
>> > +   default:
>> > +           break;
>> > +   }
>> > +}
>> > +
>> > +/*
>> > + * Dump out information on panic.
>> > + */
>> > +static int debug_notifier_call(struct notifier_block *self,
>> > +                          unsigned long v, void *p)
>> > +{
>> > +   int cpu;
>> > +   struct debug_drvdata *drvdata;
>> > +
>> > +   pr_emerg("ARM external debug module:\n");
>> > +
>> > +   for_each_possible_cpu(cpu) {
>> > +           drvdata = per_cpu(debug_drvdata, cpu);
>> > +           if (!drvdata)
>> > +                   continue;
>> > +
>> > +           pr_emerg("CPU[%d]:\n", drvdata->cpu);
>> > +
>> > +           debug_read_regs(drvdata);
>> > +           debug_dump_regs(drvdata);
>> > +   }
>> > +
>> > +   return 0;
>> > +}
>> > +
>> > +static struct notifier_block debug_notifier = {
>> > +   .notifier_call = debug_notifier_call,
>> > +};
>> > +
>> > +static int debug_enable_func(void)
>> > +{
>> > +   struct debug_drvdata *drvdata;
>> > +   int cpu;
>> > +
>> > +   for_each_possible_cpu(cpu) {
>> > +           drvdata = per_cpu(debug_drvdata, cpu);
>> > +           if (!drvdata)
>> > +                   continue;
>> > +
>> > +           pm_runtime_get_sync(drvdata->dev);
>> > +   }
>> > +
>> > +   return atomic_notifier_chain_register(&panic_notifier_list,
>> > +                                         &debug_notifier);
>> > +}
>> > +
>> > +static int debug_disable_func(void)
>> > +{
>> > +   struct debug_drvdata *drvdata;
>> > +   int cpu;
>> > +
>> > +   for_each_possible_cpu(cpu) {
>> > +           drvdata = per_cpu(debug_drvdata, cpu);
>> > +           if (!drvdata)
>> > +                   continue;
>> > +
>> > +           pm_runtime_put(drvdata->dev);
>> > +   }
>> > +
>> > +   return atomic_notifier_chain_unregister(&panic_notifier_list,
>> > +                                           &debug_notifier);
>> > +}
>> > +
>> > +static ssize_t debug_func_knob_write(struct file *f,
>> > +           const char __user *buf, size_t count, loff_t *ppos)
>> > +{
>> > +   u8 val;
>> > +   int ret;
>> > +
>> > +   ret = kstrtou8_from_user(buf, count, 2, &val);
>> > +   if (ret)
>> > +           return ret;
>> > +
>> > +   mutex_lock(&debug_lock);
>> > +
>> > +   if (val == debug_enable)
>> > +           goto out;
>> > +
>> > +   if (val)
>> > +           ret = debug_enable_func();
>> > +   else
>> > +           ret = debug_disable_func();
>>
>> I don't think you need to install the handler every time the functionality is
>> switched on/off.  I suggest to install the handler at boot time (or module
>> insertion time) and in the notifier handler, check the debug_enable flag before
>> moving on with the output.
>
> Will do.
>
>> > +
>> > +   if (ret) {
>> > +           pr_err("%s: unable to %s debug function: %d\n",
>> > +                  __func__, val ? "enable" : "disable", ret);
>> > +           goto err;
>> > +   }
>> > +
>> > +   debug_enable = val;
>>
>> Using a true/false value is probably better here.  That way you don't end up
>> with miscellaneous values in debugFS.
>
> From my testing here will not assign other values rather than 0 or 1.
> This is controlled by function kstrtou8_from_user(), the 3rd parameter
> '2' is to define the value range [0..1], so other values will report
> error after return back from kstrtou8_from_user().

Ok that works.

>
>> > +out:
>> > +   ret = count;
>> > +err:
>> > +   mutex_unlock(&debug_lock);
>> > +   return ret;
>> > +}
>> > +
>> > +static ssize_t debug_func_knob_read(struct file *f,
>> > +           char __user *ubuf, size_t count, loff_t *ppos)
>> > +{
>> > +   ssize_t ret;
>> > +   char buf[2];
>> > +
>> > +   mutex_lock(&debug_lock);
>> > +
>> > +   buf[0] = '0' + debug_enable;
>> > +   buf[1] = '\n';
>>
>>         snprintf()
>
> Will fix.
>
>> > +   ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
>> > +
>> > +   mutex_unlock(&debug_lock);
>> > +   return ret;
>> > +}
>> > +
>> > +static const struct file_operations debug_func_knob_fops = {
>> > +   .open   = simple_open,
>> > +   .read   = debug_func_knob_read,
>> > +   .write  = debug_func_knob_write,
>> > +};
>> > +
>> > +static int debug_func_init(void)
>> > +{
>> > +   struct dentry *file;
>> > +   int ret;
>> > +
>> > +   /* Create debugfs node */
>> > +   debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
>> > +   if (!debug_debugfs_dir) {
>> > +           pr_err("%s: unable to create debugfs directory\n", __func__);
>> > +           return -ENOMEM;
>> > +   }
>> > +
>> > +   file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
>> > +                   debug_debugfs_dir, NULL, &debug_func_knob_fops);
>>
>> Please align this properly.
>
> Will fix. Thanks a lot for detailed reviewing.
>
> [...]
>
> Thanks,
> Leo Yan

^ permalink raw reply

* Re: [PATCH 0/4] staging: add ccree crypto driver
From: Gilad Ben-Yossef @ 2017-04-19 14:40 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Herbert Xu, David S. Miller, Rob Herring, Greg Kroah-Hartman,
	devel, linux-crypto, devicetree, Linux kernel mailing list,
	Gilad Ben-Yossef, Binoy Jayan, Ofir Drang
In-Reply-To: <20170418154349.GJ17866@leverpostej>

On Tue, Apr 18, 2017 at 6:43 PM, Mark Rutland <mark.rutland@arm.com> wrote:
...
>> >>
>> >> The code still needs some cleanup before maturing to a proper
>> >> upstream driver, which I am in the process of doing. However,
>> >> as discussion of some of the capabilities of the hardware and
>> >> its application to some dm-crypt and dm-verity features recently
>> >> took place I though it is better to do this in the open via the
>> >> staging tree.
>> >>
>> >> A Git repository based off of Linux 4.11-rc7 is available at
>> >> https://github.com/gby/linux.git branch ccree.
>> >>
...
>> >>  .../devicetree/bindings/crypto/arm-cryptocell.txt  |   23 +
>> >
>> > I'm interested in looking at the DT binding, but for some reason I've
>> > only recevied the cover letter so far.
>> >
>> > Are my mail servers being particularly slow today, or has something gone
>> > wrong with the Cc list for the remaining patches?
>> >
>>
>> Thanks a bunch for the willingness to review this.
>>
>> My apologies for not communicating this clearly enough -  Since it is
>> an pre-existing driver it is rather big.
>> I did not want to inflict a 600K patch on the mailing list so opted to
>> provide a git repo instead, as stated in the
>> email.
>
> Should this have been a [GIT PULL], then?
>
> I was confused by the [PATCH 0/4].

Yes, it should have. Sorry about that.
>
>> The patch in question is here:
>> https://github.com/gby/linux/commit/12d92e0bc19aa9bbd891cdab765eaaeabe0b6967
>>
>> Do let me know if you prefer that I will send at least the smaller
>> patch file via email in the normal fashion.
>
> Sending patches is the usual way to have things reviewed. Linking to an
> external site doesn't fit with the workflows of everyone.
>
> If you wish to have patches reviewed, please send them as patches in the
> usual fashion.

Thanks for the feedback.
I will break the driver up and post patches in the normal fashion.

>
> I will note that on the patch you linked, the compatible string is far
> too vague, per common conventions. Per your description above, that
> should be something like "arm,cryptocell-712-ree" (and each variant
> should have its own string).

Got it. Will change.

Thanks for the review even in this unconventional form :-) !

>
> I don't have documentation to hand to attempt to review the rest.
>
> I would appreciate if you could ensure that the DT binding was reviewed
> as part of the staging TODOs. That needs to follow the usual DT review
> process of sending patches to the list. Until that has occurred, it
> shouldn't be in Documentation/devicetree/.

Of course, will do.

Thanks,
Gilad
>
> Thanks,
> Mark.



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

^ permalink raw reply

* Re: [PATCH 0/4] staging: add ccree crypto driver
From: Gilad Ben-Yossef @ 2017-04-19 14:34 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Herbert Xu, David S. Miller, Rob Herring, Mark Rutland, devel,
	linux-crypto, devicetree, Linux kernel mailing list,
	Gilad Ben-Yossef, Binoy Jayan, Ofir Drang
In-Reply-To: <20170418153951.GA31214@kroah.com>

On Tue, Apr 18, 2017 at 6:39 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Apr 18, 2017 at 05:07:50PM +0300, Gilad Ben-Yossef wrote:
>> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware
>> accelerators. It is supported by a long lived series of out of tree
>> drivers, which I am now in the process of unifying and upstreaming.
>> This is the first drop, supporting the new CryptoCell 712 REE.
>>
>> The code still needs some cleanup before maturing to a proper
>> upstream driver, which I am in the process of doing. However,
>> as discussion of some of the capabilities of the hardware and
>> its application to some dm-crypt and dm-verity features recently
>> took place I though it is better to do this in the open via the
>> staging tree.
>>
>> A Git repository based off of Linux 4.11-rc7 is available at
>> https://github.com/gby/linux.git branch ccree.
>>
>> Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com>
>> CC: Binoy Jayan <binoy.jayan@linaro.org>
>> CC: Ofir Drang <ofir.drang@arm.com>
>>
>> Gilad Ben-Yossef (4):
>>   staging: add ccree crypto driver
>>   staging: ccree: add TODO list
>>   staging: ccree: add devicetree bindings
>>   MAINTAINERS: add Gilad BY as maintainer for ccree
>
> We can't do much without a real patch, sorry.  Digging in random git
> trees doesn't work :(
>
> I can't take this as-is, I need patches.

Got it.

I'll break the driver to a series of patches and post them .
Thanks for the feedback.

Gilad



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

^ permalink raw reply

* Re: [PATCH v6 6/8] coresight: add support for CPU debug module
From: Suzuki K Poulose @ 2017-04-19 14:32 UTC (permalink / raw)
  To: Leo Yan
  Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
	Catalin Marinas, Will Deacon, Andy Gross, David Brown,
	Mathieu Poirier, Stephen Boyd, linux-doc-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-msm-u79uwXL29TY76Z2rM5mHXA,
	linux-soc-u79uwXL29TY76Z2rM5mHXA, Mike Leach, Sudeep Holla
In-Reply-To: <20170419142812.GA16160@leoy-linaro>

On 19/04/17 15:28, Leo Yan wrote:
> Hi Suzuki,
>
> On Wed, Apr 19, 2017 at 02:23:04PM +0100, Suzuki K Poulose wrote:
>> Hi Leo,
>>
>> This version looks good to me. I have two minor comments below.
>
> Thanks for reviewing. Will take the suggestions. Just check a bit for
> last comment.
>
> [...]
>
>>> +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
>>> +{
>>> +	void __iomem *base;
>>> +	struct device *dev = &adev->dev;
>>> +	struct debug_drvdata *drvdata;
>>> +	struct resource *res = &adev->res;
>>> +	struct device_node *np = adev->dev.of_node;
>>> +	int ret;
>>> +
>>> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>>> +	if (!drvdata)
>>> +		return -ENOMEM;
>>> +
>>> +	drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
>>> +	if (per_cpu(debug_drvdata, drvdata->cpu)) {
>>> +		dev_err(dev, "CPU%d drvdata has been initialized\n",
>>> +			drvdata->cpu);
>>
>> May be we could warn about a possible issue in the DT ?
>
> So can I understand the suggestion is to add warning in function
> of_coresight_get_cpu() when cannot find CPU number, and here directly
> bail out?

No. One could have single CPU DT, where he doesn't need to provide the CPU number.
Hence, it doesn't make sense to WARN  in of_coresight_get_cpu().

But when we hit the case above, we find that the some node was registered for
the given CPU (be it 0 or any other), which is definitely an error in DT. Due to

1) Hasn't specified the CPU number for more than one node

OR

2) CPU number duplicated in the more than one nodes.

Cheers
Suzuki

--
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 v6 6/8] coresight: add support for CPU debug module
From: Leo Yan @ 2017-04-19 14:28 UTC (permalink / raw)
  To: Suzuki K Poulose
  Cc: Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
	Catalin Marinas, Will Deacon, Andy Gross, David Brown,
	Mathieu Poirier, Stephen Boyd, linux-doc, linux-kernel,
	devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
	Mike Leach, Sudeep Holla
In-Reply-To: <5c5cb6f8-1dcb-8a9d-1605-c006656005eb@arm.com>

Hi Suzuki,

On Wed, Apr 19, 2017 at 02:23:04PM +0100, Suzuki K Poulose wrote:
> Hi Leo,
> 
> This version looks good to me. I have two minor comments below.

Thanks for reviewing. Will take the suggestions. Just check a bit for
last comment.

[...]

> >+static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> >+{
> >+	void __iomem *base;
> >+	struct device *dev = &adev->dev;
> >+	struct debug_drvdata *drvdata;
> >+	struct resource *res = &adev->res;
> >+	struct device_node *np = adev->dev.of_node;
> >+	int ret;
> >+
> >+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> >+	if (!drvdata)
> >+		return -ENOMEM;
> >+
> >+	drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> >+	if (per_cpu(debug_drvdata, drvdata->cpu)) {
> >+		dev_err(dev, "CPU%d drvdata has been initialized\n",
> >+			drvdata->cpu);
> 
> May be we could warn about a possible issue in the DT ?

So can I understand the suggestion is to add warning in function
of_coresight_get_cpu() when cannot find CPU number, and here directly
bail out?

Thanks,
Leo Yan

^ permalink raw reply

* Re: [PATCH V5 1/4] arm64: dts: Add basic DT to support Spreadtrum's SP9860G
From: Olof Johansson @ 2017-04-19 14:24 UTC (permalink / raw)
  To: Lyra Zhang
  Cc: Arnd Bergmann, arm-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Mark Rutland,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org,
	Catalin Marinas, Will Deacon, Mathieu Poirier,
	Orson Zhai (??????),
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Chunyan Zhang
In-Reply-To: <1492603726250.cnoz1xemvxpkp3sd4toaurm2-z5hGa2qSFaSios+IgheDglaTQe2KTcn/@public.gmane.org>

Hi,

On Wed, Apr 19, 2017 at 08:09:00PM +0800, Lyra Zhang wrote:
> Hi Arnd, Olof, Could you please take this patch through arm-soc git if
> there are no further comments? (The three other patches in this series
> have been taken by Greg.) Thanks, Chunyan On

First, something bad seems to have happened with this email, the formatting
was all wrapped and it was hard to read. You might want to look into it so
you can avoid more problems like it in the future.

That being said, we can apply the patches but please re-send the series to
arm-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org with any acks received added, etc.


Thanks!


-Olof
--
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] PM / OPP: Use - instead of @ for DT entries
From: Olof Johansson @ 2017-04-19 14:22 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael Wysocki, arm, Chanwoo Choi, MyungJoo Ham, Kyungmin Park,
	Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas,
	Viresh Kumar, Nishanth Menon, Stephen Boyd, Beno??t Cousson,
	Tony Lindgren, Rob Herring, Mark Rutland, Daniel Mack,
	Haojian Zhuang, Robert Jarzmik, Maxime Ripard, Chen-Yu Tsai,
	Masahiro
In-Reply-To: <734c5d5a66ef8a5505f368fc68a8ab9c02d710f9.1492492253.git.viresh.kumar@linaro.org>

Hi Viresh,

On Tue, Apr 18, 2017 at 10:44:50AM +0530, Viresh Kumar wrote:
> Compiling the DT file with W=1, DTC warns like follows:
> 
> Warning (unit_address_vs_reg): Node /opp_table0/opp@1000000000 has a
> unit name, but no reg property
> 
> Fix this by replacing '@' with '-' as the OPP nodes will never have a
> "reg" property.
> 
> Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
> Reported-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> (sunxi)
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> Acked-by: Masahiro Yamada <yamada.masahiro@socionext.com> (uniphier)
> Acked-by: Rob Herring <robh@kernel.org>
> Acked-by: Tony Lindgren <tony@atomide.com>

We've already turned down other patches that does this in a sweeping manner
like this, since they tend to be conflict prone with other DT changes.

Please split per platform and merge with each maintainer.


Thanks,

-Olof

^ permalink raw reply

* Re: [PATCH V4 1/9] PM / OPP: Allow OPP table to be used for power-domains
From: Sudeep Holla @ 2017-04-19 13:58 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Sudeep Holla, Rafael Wysocki, ulf.hansson, Kevin Hilman,
	Viresh Kumar, Nishanth Menon, Stephen Boyd, linaro-kernel,
	linux-pm, linux-kernel, Vincent Guittot, robh+dt, lina.iyer,
	rnayak, devicetree
In-Reply-To: <20170419114740.GD5436@vireshk-i7>



On 19/04/17 12:47, Viresh Kumar wrote:
> On 18-04-17, 17:01, Sudeep Holla wrote:
>> Understood. I would incline towards reusing regulators we that's what is
> 
> It can be just a regulator, but it can be anything else as well. That
> entity may have its own clock/volt/current tunables, etc.
> 

Agreed.

>> changed behind the scene. Calling this operating performance point
>> is misleading and doesn't align well with existing specs/features.
> 
> Yeah, but there are no voltage levels available here and that doesn't
> fit as a regulator then.
> 

We can't dismiss just based on that. We do have systems where
performance index is mapped to clocks though it may not be 1:1 mapping.
I am not disagreeing here, just trying to understand it better.

>> Understood. We have exactly same thing with SCPI but it controls both
>> frequency and voltage referred as operating points. In general, this OPP
>> terminology is used in SCPI/ACPI/SCMI specifications as both frequency
>> and voltage control. I am bit worried that this binding might introduce
>> confusions on the definitions. But it can be reworded/renamed easily if
>> required.
> 
> Yeah, so far we have been looking at OPPs as freq-voltage pairs ONLY
> and that is changing. I am not sure if it going in the wrong
> direction really. Without frequency also it is an operating point for
> the domain. Isn't it?
> 

Yes, I completely agree. I am not saying the direction is wrong. I am
saying it's confusing and binding needs to be more clear.

On the contrary(playing devil's advocate here), we can treat all
existing regulators alone as OPP then if you strip the voltages and
treat it as abstract number. So if the firmware handles more than just
regulators, I agree. At the same time, I would have preferred firmware
to even abstract the frequency like ACPI CPPC. It would be good to get
more information on what exactly that firmware handles.

I am just more cautious here since we are designing generic bindings and
changing generic code, we need to understand what that firmware supports
and how it may evolve(so that we can maintain DT compatibility)

I did a brief check and wanted to check if this is SMD/RPM regulators ?

-- 
Regards,
Sudeep

^ permalink raw reply

* Re: [PATCH v13 03/10] mux: minimal mux subsystem and gpio-based mux controller
From: Philipp Zabel @ 2017-04-19 13:49 UTC (permalink / raw)
  To: Peter Rosin
  Cc: linux-kernel, Greg Kroah-Hartman, Wolfram Sang, Rob Herring,
	Mark Rutland, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Jonathan Corbet,
	linux-i2c, devicetree, linux-iio, linux-doc, Andrew Morton,
	Colin Ian King, Paul Gortmaker, kernel
In-Reply-To: <da90526b-7274-c52c-07e8-9f0d75cf1cbf@axentia.se>

On Wed, 2017-04-19 at 14:00 +0200, Peter Rosin wrote:
[...]
> >> +int mux_control_select(struct mux_control *mux, int state)
> > 
> > If we let two of these race, ...
>
> The window for this "race" is positively huge. If there are several
> mux consumers of a single mux controller, it is self-evident that
> if one of them grabs the mux for a long time, the others will suffer.
> 
> The design is that the rwsem is reader-locked for the full duration
> of a select/deselect operation by the mux consumer.

I was not clear. I meant: I think this can also happen if we let them
race with the same state target.

> >> +{
> >> +	int ret;
> >> +
> >> +	if (down_read_trylock(&mux->lock)) {
> >> +		if (mux->cached_state == state)
> >> +			return 0;

This check makes it clear that a second select call is not intended to
block if the intended state is already selected. But if the instance we
will lose the race against has not yet updated cached_state, ...

> >> +		/* Sigh, the mux needs updating... */
> >> +		up_read(&mux->lock);
> >> +	}
> >> +
> >> +	/* ...or it's just contended. */
> >> +	down_write(&mux->lock);

... we are blocking here until the other instance calls up_read. Even
though in this case (same state target) we would only have to block
until the other instance calls downgrade_write after the mux control is
set to the correct state.

Basically there is a small window before down_write with no lock at all,
where multiple instances can already have decided they must change the
mux (to the same state). If this happens, they go on to block each other
unnecessarily.

> > ... then the last to get to down_write will just wait here forever (or
> > until the first consumer calls mux_control_deselect, which may never
> > happen)?
> 
> It is vital that the mux consumer call _deselect when it is done with
> the mux. Not doing so will surely starve out any other mux consumers.
> The whole thing is designed around the fact that mux consumers should
> deselect the mux as soon as it's no longer needed.

I'd like to use this for video bus multiplexers. Those would not be
selected for the duration of an i2c transfer, but for the duration of a
running video capture stream, or for the duration of an enabled display
path. While I currently have no use case for multiple consumers
controlling the same mux, this scenario is what shapes my perspective.
For such long running selections the consumer should have the option to
return -EBUSY instead of blocking when the lock can't be taken.

regards
Philipp

^ permalink raw reply

* [PATCH] ARM: dts: aspeed-g4: fix AHB window size of the SMC controllers
From: Cédric Le Goater @ 2017-04-19 13:43 UTC (permalink / raw)
  To: Joel Stanley
  Cc: Mark Rutland, devicetree, Russell King, Rob Herring,
	Cédric Le Goater, linux-arm-kernel

The window of the Aspeed AST2400 SMC Controllers to map chips on the
AHB Bus has a 256MB size. The full window range is

    [ 0x20000000 - 0x2FFFFFFF ] for the FMC controller
    [ 0x30000000 - 0x3FFFFFFF ] for the SPI controller

This change requires CONFIG_VMSPLIT_2G to be set.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 arch/arm/boot/dts/aspeed-g4.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index 4e3f055b365c..a7b6e04d5f1b 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -26,7 +26,7 @@
 
 		fmc: flash-controller@1e620000 {
 			reg = < 0x1e620000 0x94
-				0x20000000 0x02000000 >;
+				0x20000000 0x10000000 >;
 			#address-cells = <1>;
 			#size-cells = <0>;
 			compatible = "aspeed,ast2400-fmc";
@@ -41,7 +41,7 @@
 
 		spi: flash-controller@1e630000 {
 			reg = < 0x1e630000 0x18
-				0x30000000 0x02000000 >;
+				0x30000000 0x10000000 >;
 			#address-cells = <1>;
 			#size-cells = <0>;
 			compatible = "aspeed,ast2400-spi";
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related

* Re: [PATCH 0/4] of: remove *phandle properties from expanded device tree
From: Rob Herring @ 2017-04-19 13:38 UTC (permalink / raw)
  To: Frank Rowand
  Cc: Stephen Boyd, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <58F6804A.3080206-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

On Tue, Apr 18, 2017 at 4:08 PM, Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Rob,
>
> Please do not apply this patch series.

What about patches 2-4? Those seem unrelated. But #2 didn't apply for
me, so resend if I should apply.

Rob

>
> The more context I look at, the less this approach seems good.
>
> I hope to have a simpler version completed quickly.
>
> Thanks,
>
> - Frank
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 1/6] arm64: dts: Add symlinks for cros-ec-keyboard and cros-ec-sbs
From: Olof Johansson @ 2017-04-19 13:31 UTC (permalink / raw)
  To: Heiko Stuebner
  Cc: Brian Norris, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Doug Anderson,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	open list:ARM/Rockchip SoC..., Rob Herring, Chris Zhong,
	Stephen Barber,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Caesar Wang, Maxime Ripard
In-Reply-To: <2579212.374vCSe9xi@phil>

On Wed, Apr 19, 2017 at 10:25 PM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:
> Am Mittwoch, 19. April 2017, 21:54:09 CEST schrieb Olof Johansson:
>> Hi,
>>
>> On Tue, Feb 28, 2017 at 3:20 AM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:
>> > Hi Olof,
>> >
>> > Am Dienstag, 21. Februar 2017, 15:47:31 CET schrieb Olof Johansson:
>> >> On Thu, Feb 9, 2017 at 5:05 PM, Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> >> > From: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> >> >
>> >> > We'd like to be able to use the cros-ec-keyboard.dtsi and
>> >> > cros-ec-sbs.dtsi snippets for arm64 devices.  Currently those files live
>> >> > in the arm/boot/dts directory.
>> >> >
>> >> > Let's follow the convention set by commit 8ee57b8182c4 ("ARM64: dts:
>> >> > vexpress: Use a symlink to vexpress-v2m-rs1.dtsi from arch=arm") and use
>> >> > a symlink.  Note that in this case we put the files in a new
>> >> > "include/common" directory since these snippets may need to be
>> >> > referenced by dts files in many different subdirectories.
>> >>
>> >> I'd rather have something like this:
>> >>
>> >> https://marc.info/?m=147547436324674&w=2
>> >>
>> >> Instead of having everybody move things over. I.e. make it easy to
>> >> refer to the arm version from arm64 instead of creating a "common"
>> >> layer inbetween.
>> >
>> > just so it gets noticed, I've done and tested [0], which hopefully should
>> > implement your suggestions above.
>> >
>> > If that looks ok, how do you want that picked up? Should I just include
>> > them in my regular rockchip branches or do you to pick them into some
>> > immutable branch, if other surprise-users turn up in time for 4.12?
>>
>> Sigh. I completely dropped the ball on this, and I didn't see it
>> included in any of your pull requests for 4.12 since I never actually
>> acked that approach.
>>
>> I've applied the patches onto a dt/include-paths stable branch, but
>> we're late for merging dependent code on top of it for 4.12.
>
> Didn't you merge the patches into a branch already and the rk3399-gru
> support on top of it?
>
> Aka
> https://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git/log/?h=shared/dt-symlinks
>
> Which one of my previous pull requests was already based on.

Yeah, ignore the above (the original branch is the one to use). I must
be turning blind.

Adding Maxime on Cc since I asked him to move over to the new include
model post-rc1 for some of his stuff.


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

^ permalink raw reply

* Re: [PATCH v2 1/6] arm64: dts: Add symlinks for cros-ec-keyboard and cros-ec-sbs
From: Heiko Stuebner @ 2017-04-19 13:25 UTC (permalink / raw)
  To: Olof Johansson
  Cc: Brian Norris, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Doug Anderson,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	open list:ARM/Rockchip SoC..., Rob Herring, Chris Zhong,
	Stephen Barber,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Caesar Wang
In-Reply-To: <CAOesGMirj2j5P7_Ri4CSCux+O3hH_wxvrra7-p33AdXO4Ej5Hg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

Am Mittwoch, 19. April 2017, 21:54:09 CEST schrieb Olof Johansson:
> Hi,
> 
> On Tue, Feb 28, 2017 at 3:20 AM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:
> > Hi Olof,
> >
> > Am Dienstag, 21. Februar 2017, 15:47:31 CET schrieb Olof Johansson:
> >> On Thu, Feb 9, 2017 at 5:05 PM, Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> >> > From: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> >
> >> > We'd like to be able to use the cros-ec-keyboard.dtsi and
> >> > cros-ec-sbs.dtsi snippets for arm64 devices.  Currently those files live
> >> > in the arm/boot/dts directory.
> >> >
> >> > Let's follow the convention set by commit 8ee57b8182c4 ("ARM64: dts:
> >> > vexpress: Use a symlink to vexpress-v2m-rs1.dtsi from arch=arm") and use
> >> > a symlink.  Note that in this case we put the files in a new
> >> > "include/common" directory since these snippets may need to be
> >> > referenced by dts files in many different subdirectories.
> >>
> >> I'd rather have something like this:
> >>
> >> https://marc.info/?m=147547436324674&w=2
> >>
> >> Instead of having everybody move things over. I.e. make it easy to
> >> refer to the arm version from arm64 instead of creating a "common"
> >> layer inbetween.
> >
> > just so it gets noticed, I've done and tested [0], which hopefully should
> > implement your suggestions above.
> >
> > If that looks ok, how do you want that picked up? Should I just include
> > them in my regular rockchip branches or do you to pick them into some
> > immutable branch, if other surprise-users turn up in time for 4.12?
> 
> Sigh. I completely dropped the ball on this, and I didn't see it
> included in any of your pull requests for 4.12 since I never actually
> acked that approach.
> 
> I've applied the patches onto a dt/include-paths stable branch, but
> we're late for merging dependent code on top of it for 4.12.

Didn't you merge the patches into a branch already and the rk3399-gru
support on top of it?

Aka
https://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git/log/?h=shared/dt-symlinks

Which one of my previous pull requests was already based on.


Heiko

--
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 v6 6/8] coresight: add support for CPU debug module
From: Suzuki K Poulose @ 2017-04-19 13:23 UTC (permalink / raw)
  To: Leo Yan, Jonathan Corbet, Rob Herring, Mark Rutland, Wei Xu,
	Catalin Marinas, Will Deacon, Andy Gross, David Brown,
	Mathieu Poirier, Stephen Boyd, linux-doc, linux-kernel,
	devicetree, linux-arm-kernel, linux-arm-msm, linux-soc,
	Mike Leach, Sudeep Holla
In-Reply-To: <1491485461-22800-7-git-send-email-leo.yan@linaro.org>

On 06/04/17 14:30, Leo Yan wrote:
> Coresight includes debug module and usually the module connects with CPU
> debug logic. ARMv8 architecture reference manual (ARM DDI 0487A.k) has
> description for related info in "Part H: External Debug".
>
> Chapter H7 "The Sample-based Profiling Extension" introduces several
> sampling registers, e.g. we can check program counter value with
> combined CPU exception level, secure state, etc. So this is helpful for
> analysis CPU lockup scenarios, e.g. if one CPU has run into infinite
> loop with IRQ disabled. In this case the CPU cannot switch context and
> handle any interrupt (including IPIs), as the result it cannot handle
> SMP call for stack dump.
>
> This patch is to enable coresight debug module, so firstly this driver
> is to bind apb clock for debug module and this is to ensure the debug
> module can be accessed from program or external debugger. And the driver
> uses sample-based registers for debug purpose, e.g. when system triggers
> panic, the driver will dump program counter and combined context
> registers (EDCIDSR, EDVIDSR); by parsing context registers so can
> quickly get to know CPU secure state, exception level, etc.
>
> Some of the debug module registers are located in CPU power domain, so
> this requires the CPU power domain stays on when access related debug
> registers, but the power management for CPU power domain is quite
> dependent on SoC integration for power management. For the platforms
> which with sane power controller implementations, this driver follows
> the method to set EDPRCR to try to pull the CPU out of low power state
> and then set 'no power down request' bit so the CPU has no chance to
> lose power.
>
> If the SoC has not followed up this design well for power management
> controller, the user should use the command line parameter or sysfs
> to constrain all or partial idle states to ensure the CPU power
> domain is enabled and access coresight CPU debug component safely.

Hi Leo,

This version looks good to me. I have two minor comments below.


> +
> +static struct notifier_block debug_notifier = {
> +	.notifier_call = debug_notifier_call,
> +};
> +
> +static int debug_enable_func(void)
> +{
> +	struct debug_drvdata *drvdata;
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		drvdata = per_cpu(debug_drvdata, cpu);
> +		if (!drvdata)
> +			continue;
> +
> +		pm_runtime_get_sync(drvdata->dev);
> +	}
> +
> +	return atomic_notifier_chain_register(&panic_notifier_list,
> +					      &debug_notifier);
> +}
> +
> +static int debug_disable_func(void)
> +{
> +	struct debug_drvdata *drvdata;
> +	int cpu;
> +
> +	for_each_possible_cpu(cpu) {
> +		drvdata = per_cpu(debug_drvdata, cpu);
> +		if (!drvdata)
> +			continue;
> +
> +		pm_runtime_put(drvdata->dev);
> +	}
> +
> +	return atomic_notifier_chain_unregister(&panic_notifier_list,
> +						&debug_notifier);
> +}

I believe you should, reverse the order of these operations in debug_disable_func()
to prevent getting a panic notifier after we have released the power domain for the
debug.
i.e, :
	atomic_notifier_chain_unregister(...);
	
	for_each_possible_cpu(cpu) {}



> +
> +static ssize_t debug_func_knob_write(struct file *f,
> +		const char __user *buf, size_t count, loff_t *ppos)
> +{
> +	u8 val;
> +	int ret;
> +
> +	ret = kstrtou8_from_user(buf, count, 2, &val);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&debug_lock);
> +
> +	if (val == debug_enable)
> +		goto out;
> +
> +	if (val)
> +		ret = debug_enable_func();
> +	else
> +		ret = debug_disable_func();
> +
> +	if (ret) {
> +		pr_err("%s: unable to %s debug function: %d\n",
> +		       __func__, val ? "enable" : "disable", ret);
> +		goto err;
> +	}
> +
> +	debug_enable = val;
> +out:
> +	ret = count;
> +err:
> +	mutex_unlock(&debug_lock);
> +	return ret;
> +}
> +
> +static ssize_t debug_func_knob_read(struct file *f,
> +		char __user *ubuf, size_t count, loff_t *ppos)
> +{
> +	ssize_t ret;
> +	char buf[2];
> +
> +	mutex_lock(&debug_lock);
> +
> +	buf[0] = '0' + debug_enable;
> +	buf[1] = '\n';
> +	ret = simple_read_from_buffer(ubuf, count, ppos, buf, sizeof(buf));
> +
> +	mutex_unlock(&debug_lock);
> +	return ret;
> +}
> +
> +static const struct file_operations debug_func_knob_fops = {
> +	.open	= simple_open,
> +	.read	= debug_func_knob_read,
> +	.write	= debug_func_knob_write,
> +};
> +
> +static int debug_func_init(void)
> +{
> +	struct dentry *file;
> +	int ret;
> +
> +	/* Create debugfs node */
> +	debug_debugfs_dir = debugfs_create_dir("coresight_cpu_debug", NULL);
> +	if (!debug_debugfs_dir) {
> +		pr_err("%s: unable to create debugfs directory\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	file = debugfs_create_file("enable", S_IRUGO | S_IWUSR,
> +			debug_debugfs_dir, NULL, &debug_func_knob_fops);
> +	if (!file) {
> +		pr_err("%s: unable to create enable knob file\n", __func__);
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	/* Use sysfs node to enable functionality */
> +	if (!debug_enable)
> +		return 0;
> +
> +	/* Register function to be called for panic */
> +	ret = atomic_notifier_chain_register(&panic_notifier_list,
> +					     &debug_notifier);
> +	if (ret) {
> +		pr_err("%s: unable to register notifier: %d\n",
> +		       __func__, ret);
> +		goto err;
> +	}
> +

Since we depend on the value of debug_enable above, below in debug_probe()
and in debug_remove(), we should protect these paths using the debug_lock mutex,
like we do above, to make sure we don't create a race.

> +	return 0;
> +
> +err:
> +	debugfs_remove_recursive(debug_debugfs_dir);
> +	return ret;
> +}
> +
> +static void debug_func_exit(void)
> +{
> +	debugfs_remove_recursive(debug_debugfs_dir);
> +
> +	/* Unregister panic notifier callback */
> +	if (debug_enable)
> +		atomic_notifier_chain_unregister(&panic_notifier_list,
> +						 &debug_notifier);
> +}
> +
> +static int debug_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> +	void __iomem *base;
> +	struct device *dev = &adev->dev;
> +	struct debug_drvdata *drvdata;
> +	struct resource *res = &adev->res;
> +	struct device_node *np = adev->dev.of_node;
> +	int ret;
> +
> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	drvdata->cpu = np ? of_coresight_get_cpu(np) : 0;
> +	if (per_cpu(debug_drvdata, drvdata->cpu)) {
> +		dev_err(dev, "CPU%d drvdata has been initialized\n",
> +			drvdata->cpu);

May be we could warn about a possible issue in the DT ?


Cheers
Suzuki

^ permalink raw reply

* Re: [PATCH V5 1/4] arm64: dts: Add basic DT to support Spreadtrum's SP9860G
From: Chunyan Zhang @ 2017-04-19 13:12 UTC (permalink / raw)
  To: arm-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
  Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Mark Rutland,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org,
	Catalin Marinas, Will Deacon, Arnd Bergmann, Mathieu Poirier,
	Orson Zhai (翟京),
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Chunyan Zhang
In-Reply-To: <1490599960-27330-1-git-send-email-chunyan.zhang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>

Hi Arnd, Olof

Could you please take this patch through arm-soc git if there are no
further comments? (The three other patches in this series have been
taken by Greg.)

Thanks,
Chunyan

On 27 March 2017 at 15:32, Chunyan Zhang <chunyan.zhang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org> wrote:
> From: Orson Zhai <orson.zhai-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
>
> SC9860G is a 8 cores of A53 SoC with 4G LTE support SoC from Spreadtrum.
>
> According to regular hierarchy of sprd dts, whale2.dtsi contains SoC
> peripherals IP nodes, sc9860.dtsi contains stuff related to ARM core stuff
> and sp9860g dts is for the board level.
>
> Signed-off-by: Orson Zhai <orson.zhai-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
> Signed-off-by: Chunyan Zhang <chunyan.zhang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
> Reviewed-by: Mathieu Poirier <mathieu.poirier-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm64/boot/dts/sprd/Makefile         |   3 +-
>  arch/arm64/boot/dts/sprd/sc9860.dtsi      | 569 ++++++++++++++++++++++++++++++
>  arch/arm64/boot/dts/sprd/sp9860g-1h10.dts |  56 +++
>  arch/arm64/boot/dts/sprd/whale2.dtsi      |  71 ++++
>  4 files changed, 698 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/boot/dts/sprd/sc9860.dtsi
>  create mode 100644 arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
>  create mode 100644 arch/arm64/boot/dts/sprd/whale2.dtsi
>
> diff --git a/arch/arm64/boot/dts/sprd/Makefile b/arch/arm64/boot/dts/sprd/Makefile
> index b658c5e..f0535e6 100644
> --- a/arch/arm64/boot/dts/sprd/Makefile
> +++ b/arch/arm64/boot/dts/sprd/Makefile
> @@ -1,4 +1,5 @@
> -dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb
> +dtb-$(CONFIG_ARCH_SPRD) += sc9836-openphone.dtb \
> +                       sp9860g-1h10.dtb
>
>  always         := $(dtb-y)
>  subdir-y       := $(dts-dirs)
> diff --git a/arch/arm64/boot/dts/sprd/sc9860.dtsi b/arch/arm64/boot/dts/sprd/sc9860.dtsi
> new file mode 100644
> index 0000000..7b7d8ce
> --- /dev/null
> +++ b/arch/arm64/boot/dts/sprd/sc9860.dtsi
> @@ -0,0 +1,569 @@
> +/*
> + * Spreadtrum SC9860 SoC
> + *
> + * Copyright (C) 2016, Spreadtrum Communications Inc.
> + *
> + * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include "whale2.dtsi"
> +
> +/ {
> +       cpus {
> +               #address-cells = <2>;
> +               #size-cells = <0>;
> +
> +               cpu-map {
> +                       cluster0 {
> +                               core0 {
> +                                       cpu = <&CPU0>;
> +                               };
> +                               core1 {
> +                                       cpu = <&CPU1>;
> +                               };
> +                               core2 {
> +                                       cpu = <&CPU2>;
> +                               };
> +                               core3 {
> +                                       cpu = <&CPU3>;
> +                               };
> +                       };
> +
> +                       cluster1 {
> +                               core0 {
> +                                       cpu = <&CPU4>;
> +                               };
> +                               core1 {
> +                                       cpu = <&CPU5>;
> +                               };
> +                               core2 {
> +                                       cpu = <&CPU6>;
> +                               };
> +                               core3 {
> +                                       cpu = <&CPU7>;
> +                               };
> +                       };
> +               };
> +
> +               CPU0: cpu@530000 {
> +                       device_type = "cpu";
> +                       compatible = "arm,cortex-a53", "arm,armv8";
> +                       reg = <0x0 0x530000>;
> +                       enable-method = "psci";
> +                       cpu-idle-states = <&CORE_PD &CLUSTER_PD>;
> +               };
> +
> +               CPU1: cpu@530001 {
> +                       device_type = "cpu";
> +                       compatible = "arm,cortex-a53", "arm,armv8";
> +                       reg = <0x0 0x530001>;
> +                       enable-method = "psci";
> +                       cpu-idle-states = <&CORE_PD &CLUSTER_PD>;
> +               };
> +
> +               CPU2: cpu@530002 {
> +                       device_type = "cpu";
> +                       compatible = "arm,cortex-a53", "arm,armv8";
> +                       reg = <0x0 0x530002>;
> +                       enable-method = "psci";
> +                       cpu-idle-states = <&CORE_PD &CLUSTER_PD>;
> +               };
> +
> +               CPU3: cpu@530003 {
> +                       device_type = "cpu";
> +                       compatible = "arm,cortex-a53", "arm,armv8";
> +                       reg = <0x0 0x530003>;
> +                       enable-method = "psci";
> +                       cpu-idle-states = <&CORE_PD &CLUSTER_PD>;
> +               };
> +
> +               CPU4: cpu@530100 {
> +                       device_type = "cpu";
> +                       compatible = "arm,cortex-a53", "arm,armv8";
> +                       reg = <0x0 0x530100>;
> +                       enable-method = "psci";
> +                       cpu-idle-states = <&CORE_PD &CLUSTER_PD>;
> +               };
> +
> +               CPU5: cpu@530101 {
> +                       device_type = "cpu";
> +                       compatible = "arm,cortex-a53", "arm,armv8";
> +                       reg = <0x0 0x530101>;
> +                       enable-method = "psci";
> +                       cpu-idle-states = <&CORE_PD &CLUSTER_PD>;
> +               };
> +
> +               CPU6: cpu@530102 {
> +                       device_type = "cpu";
> +                       compatible = "arm,cortex-a53", "arm,armv8";
> +                       reg = <0x0 0x530102>;
> +                       enable-method = "psci";
> +                       cpu-idle-states = <&CORE_PD &CLUSTER_PD>;
> +               };
> +
> +               CPU7: cpu@530103 {
> +                       device_type = "cpu";
> +                       compatible = "arm,cortex-a53", "arm,armv8";
> +                       reg = <0x0 0x530103>;
> +                       enable-method = "psci";
> +                       cpu-idle-states = <&CORE_PD &CLUSTER_PD>;
> +               };
> +       };
> +
> +       idle-states{
> +               entry-method = "arm,psci";
> +
> +               CORE_PD: core_pd {
> +                       compatible = "arm,idle-state";
> +                       entry-latency-us = <1000>;
> +                       exit-latency-us = <700>;
> +                       min-residency-us = <2500>;
> +                       local-timer-stop;
> +                       arm,psci-suspend-param = <0x00010002>;
> +               };
> +
> +               CLUSTER_PD: cluster_pd {
> +                       compatible = "arm,idle-state";
> +                       entry-latency-us = <1000>;
> +                       exit-latency-us = <1000>;
> +                       min-residency-us = <3000>;
> +                       local-timer-stop;
> +                       arm,psci-suspend-param = <0x01010003>;
> +               };
> +       };
> +
> +       gic: interrupt-controller@12001000 {
> +               compatible = "arm,gic-400";
> +               reg = <0 0x12001000 0 0x1000>,
> +                     <0 0x12002000 0 0x2000>,
> +                     <0 0x12004000 0 0x2000>,
> +                     <0 0x12006000 0 0x2000>;
> +               #interrupt-cells = <3>;
> +               interrupt-controller;
> +               interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(8)
> +                                       | IRQ_TYPE_LEVEL_HIGH)>;
> +       };
> +
> +       psci {
> +               compatible = "arm,psci-0.2";
> +               method = "smc";
> +       };
> +
> +       timer {
> +               compatible = "arm,armv8-timer";
> +               interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8)
> +                                        | IRQ_TYPE_LEVEL_LOW)>,
> +                            <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8)
> +                                        | IRQ_TYPE_LEVEL_LOW)>,
> +                            <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8)
> +                                        | IRQ_TYPE_LEVEL_LOW)>,
> +                            <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8)
> +                                        | IRQ_TYPE_LEVEL_LOW)>;
> +       };
> +
> +       pmu {
> +               compatible = "arm,cortex-a53-pmu", "arm,armv8-pmuv3";
> +               interrupts = <GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>,
> +                            <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
> +                            <GIC_SPI 124 IRQ_TYPE_LEVEL_HIGH>,
> +                            <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>,
> +                            <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>,
> +                            <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>,
> +                            <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>,
> +                            <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>;
> +               interrupt-affinity = <&CPU0>,
> +                                    <&CPU1>,
> +                                    <&CPU2>,
> +                                    <&CPU3>,
> +                                    <&CPU4>,
> +                                    <&CPU5>,
> +                                    <&CPU6>,
> +                                    <&CPU7>;
> +       };
> +
> +       soc {
> +               funnel@10001000 { /* SoC Funnel */
> +                       compatible = "arm,coresight-funnel", "arm,primecell";
> +                       reg = <0 0x10001000 0 0x1000>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               port@0 {
> +                                       reg = <0>;
> +                                       soc_funnel_out_port: endpoint {
> +                                               remote-endpoint = <&etb_in>;
> +                                       };
> +                               };
> +
> +                               port@1 {
> +                                       reg = <0>;
> +                                       soc_funnel_in_port0: endpoint {
> +                                               slave-mode;
> +                                               remote-endpoint =
> +                                               <&main_funnel_out_port>;
> +                                       };
> +                               };
> +
> +                               port@2 {
> +                                       reg = <4>;
> +                                       soc_funnel_in_port1: endpoint {
> +                                               slave-mode;
> +                                               remote-endpioint =
> +                                                       <&stm_out_port>;
> +                                       };
> +                               };
> +                       };
> +               };
> +
> +               etb@10003000 {
> +                       compatible = "arm,coresight-tmc", "arm,primecell";
> +                       reg = <0 0x10003000 0 0x1000>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +                       port {
> +                               etb_in: endpoint {
> +                                       slave-mode;
> +                                       remote-endpoint =
> +                                               <&soc_funnel_out_port>;
> +                               };
> +                       };
> +               };
> +
> +               stm@10006000 {
> +                       compatible = "arm,coresight-stm", "arm,primecell";
> +                       reg = <0 0x10006000 0 0x1000>,
> +                             <0 0x01000000 0 0x180000>;
> +                       reg-names = "stm-base", "stm-stimulus-base";
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +                       port {
> +                               stm_out_port: endpoint {
> +                                       remote-endpoint =
> +                                               <&soc_funnel_in_port1>;
> +                               };
> +                       };
> +               };
> +
> +               funnel@11001000 { /* Cluster0 Funnel */
> +                       compatible = "arm,coresight-funnel", "arm,primecell";
> +                       reg = <0 0x11001000 0 0x1000>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               port@0 {
> +                                       reg = <0>;
> +                                       cluster0_funnel_out_port: endpoint {
> +                                               remote-endpoint =
> +                                                       <&cluster0_etf_in>;
> +                                       };
> +                               };
> +
> +                               port@1 {
> +                                       reg = <0>;
> +                                       cluster0_funnel_in_port0: endpoint {
> +                                               slave-mode;
> +                                               remote-endpoint = <&etm0_out>;
> +                                       };
> +                               };
> +
> +                               port@2 {
> +                                       reg = <1>;
> +                                       cluster0_funnel_in_port1: endpoint {
> +                                               slave-mode;
> +                                               remote-endpoint = <&etm1_out>;
> +                                       };
> +                               };
> +
> +                               port@3 {
> +                                       reg = <2>;
> +                                       cluster0_funnel_in_port2: endpoint {
> +                                               slave-mode;
> +                                               remote-endpoint = <&etm2_out>;
> +                                       };
> +                               };
> +
> +                               port@4 {
> +                                       reg = <4>;
> +                                       cluster0_funnel_in_port3: endpoint {
> +                                               slave-mode;
> +                                               remote-endpoint = <&etm3_out>;
> +                                       };
> +                               };
> +                       };
> +               };
> +
> +               funnel@11002000 { /* Cluster1 Funnel */
> +                       compatible = "arm,coresight-funnel", "arm,primecell";
> +                       reg = <0 0x11002000 0 0x1000>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               port@0 {
> +                                       reg = <0>;
> +                                       cluster1_funnel_out_port: endpoint {
> +                                               remote-endpoint =
> +                                                       <&cluster1_etf_in>;
> +                                       };
> +                               };
> +
> +                               port@1 {
> +                                       reg = <0>;
> +                                       cluster1_funnel_in_port0: endpoint {
> +                                               slave-mode;
> +                                               remote-endpoint = <&etm4_out>;
> +                                       };
> +                               };
> +
> +                               port@2 {
> +                                       reg = <1>;
> +                                       cluster1_funnel_in_port1: endpoint {
> +                                               slave-mode;
> +                                               remote-endpoint = <&etm5_out>;
> +                                       };
> +                               };
> +
> +                               port@3 {
> +                                       reg = <2>;
> +                                       cluster1_funnel_in_port2: endpoint {
> +                                               slave-mode;
> +                                               remote-endpoint = <&etm6_out>;
> +                                       };
> +                               };
> +
> +                               port@4 {
> +                                       reg = <3>;
> +                                       cluster1_funnel_in_port3: endpoint {
> +                                               slave-mode;
> +                                               remote-endpoint = <&etm7_out>;
> +                                       };
> +                               };
> +                       };
> +               };
> +
> +               etf@11003000 { /*  ETF on Cluster0 */
> +                       compatible = "arm,coresight-tmc", "arm,primecell";
> +                       reg = <0 0x11003000 0 0x1000>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               port@0 {
> +                                       reg = <0>;
> +                                       cluster0_etf_out: endpoint {
> +                                               remote-endpoint =
> +                                               <&main_funnel_in_port0>;
> +                                       };
> +                               };
> +
> +                               port@1 {
> +                                       reg = <0>;
> +                                       cluster0_etf_in: endpoint {
> +                                               slave-mode;
> +                                               remote-endpoint =
> +                                               <&cluster0_funnel_out_port>;
> +                                       };
> +                               };
> +                       };
> +               };
> +
> +               etf@11004000 { /* ETF on Cluster1 */
> +                       compatible = "arm,coresight-tmc", "arm,primecell";
> +                       reg = <0 0x11004000 0 0x1000>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               port@0 {
> +                                       reg = <0>;
> +                                       cluster1_etf_out: endpoint {
> +                                               remote-endpoint =
> +                                               <&main_funnel_in_port1>;
> +                                       };
> +                               };
> +
> +                               port@1 {
> +                                       reg = <0>;
> +                                       cluster1_etf_in: endpoint {
> +                                               slave-mode;
> +                                               remote-endpoint =
> +                                               <&cluster1_funnel_out_port>;
> +                                       };
> +                               };
> +                       };
> +               };
> +
> +               funnel@11005000 { /* Main Funnel */
> +                       compatible = "arm,coresight-funnel", "arm,primecell";
> +                       reg = <0 0x11005000 0 0x1000>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +
> +                       ports {
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               port@0 {
> +                                       reg = <0>;
> +                                       main_funnel_out_port: endpoint {
> +                                               remote-endpoint =
> +                                                       <&soc_funnel_in_port0>;
> +                                       };
> +                               };
> +
> +                               port@1 {
> +                                       reg = <0>;
> +                                       main_funnel_in_port0: endpoint {
> +                                               slave-mode;
> +                                               remote-endpoint =
> +                                                       <&cluster0_etf_out>;
> +                                       };
> +                               };
> +
> +                               port@2 {
> +                                       reg = <1>;
> +                                       main_funnel_in_port1: endpoint {
> +                                               slave-mode;
> +                                               remote-endpoint =
> +                                                       <&cluster1_etf_out>;
> +                                       };
> +                               };
> +                       };
> +               };
> +
> +               etm@11440000 {
> +                       compatible = "arm,coresight-etm4x", "arm,primecell";
> +                       reg = <0 0x11440000 0 0x1000>;
> +                       cpu = <&CPU0>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +
> +                       port {
> +                               etm0_out: endpoint {
> +                                       remote-endpoint =
> +                                               <&cluster0_funnel_in_port0>;
> +                               };
> +                       };
> +               };
> +
> +               etm@11540000 {
> +                       compatible = "arm,coresight-etm4x", "arm,primecell";
> +                       reg = <0 0x11540000 0 0x1000>;
> +                       cpu = <&CPU1>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +
> +                       port {
> +                               etm1_out: endpoint {
> +                                       remote-endpoint =
> +                                               <&cluster0_funnel_in_port1>;
> +                               };
> +                       };
> +               };
> +
> +               etm@11640000 {
> +                       compatible = "arm,coresight-etm4x", "arm,primecell";
> +                       reg = <0 0x11640000 0 0x1000>;
> +                       cpu = <&CPU2>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +
> +                       port {
> +                               etm2_out: endpoint {
> +                                       remote-endpoint =
> +                                               <&cluster0_funnel_in_port2>;
> +                               };
> +                       };
> +               };
> +
> +               etm@11740000 {
> +                       compatible = "arm,coresight-etm4x", "arm,primecell";
> +                       reg = <0 0x11740000 0 0x1000>;
> +                       cpu = <&CPU3>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +
> +                       port {
> +                               etm3_out: endpoint {
> +                                       remote-endpoint =
> +                                               <&cluster0_funnel_in_port3>;
> +                               };
> +                       };
> +               };
> +
> +               etm@11840000 {
> +                       compatible = "arm,coresight-etm4x", "arm,primecell";
> +                       reg = <0 0x11840000 0 0x1000>;
> +                       cpu = <&CPU4>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +
> +                       port {
> +                               etm4_out: endpoint {
> +                                       remote-endpoint =
> +                                               <&cluster1_funnel_in_port0>;
> +                               };
> +                       };
> +               };
> +
> +               etm@11940000 {
> +                       compatible = "arm,coresight-etm4x", "arm,primecell";
> +                       reg = <0 0x11940000 0 0x1000>;
> +                       cpu = <&CPU5>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +
> +                       port {
> +                               etm5_out: endpoint {
> +                                       remote-endpoint =
> +                                               <&cluster1_funnel_in_port1>;
> +                               };
> +                       };
> +               };
> +
> +               etm@11a40000 {
> +                       compatible = "arm,coresight-etm4x", "arm,primecell";
> +                       reg = <0 0x11a40000 0 0x1000>;
> +                       cpu = <&CPU6>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +
> +                       port {
> +                               etm6_out: endpoint {
> +                                       remote-endpoint =
> +                                               <&cluster1_funnel_in_port2>;
> +                               };
> +                       };
> +               };
> +
> +               etm@11b40000 {
> +                       compatible = "arm,coresight-etm4x", "arm,primecell";
> +                       reg = <0 0x11b40000 0 0x1000>;
> +                       cpu = <&CPU7>;
> +                       clocks = <&ext_26m>;
> +                       clock-names = "apb_pclk";
> +
> +                       port {
> +                               etm7_out: endpoint {
> +                                       remote-endpoint =
> +                                               <&cluster1_funnel_in_port3>;
> +                               };
> +                       };
> +               };
> +       };
> +};
> diff --git a/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts b/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
> new file mode 100644
> index 0000000..ae0b28c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/sprd/sp9860g-1h10.dts
> @@ -0,0 +1,56 @@
> +/*
> + * Spreadtrum SP9860g board
> + *
> + * Copyright (C) 2017, Spreadtrum Communications Inc.
> + *
> + * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> + */
> +
> +/dts-v1/;
> +
> +#include "sc9860.dtsi"
> +
> +/ {
> +       model = "Spreadtrum SP9860G 3GFHD Board";
> +
> +       compatible = "sprd,sp9860g-1h10", "sprd,sc9860";
> +
> +       aliases {
> +               serial0 = &uart0; /* for Bluetooth */
> +               serial1 = &uart1; /* UART console */
> +               serial2 = &uart2; /* Reserved */
> +               serial3 = &uart3; /* for GPS */
> +       };
> +
> +       memory{
> +               device_type = "memory";
> +               reg = <0x0 0x80000000 0 0x60000000>,
> +                     <0x1 0x80000000 0 0x60000000>;
> +       };
> +
> +       chosen {
> +               stdout-path = "serial1:115200n8";
> +       };
> +
> +       reserved-memory {
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               ranges;
> +       };
> +};
> +
> +&uart0 {
> +       status = "okay";
> +};
> +
> +&uart1 {
> +       status = "okay";
> +};
> +
> +&uart2 {
> +       status = "okay";
> +};
> +
> +&uart3 {
> +       status = "okay";
> +};
> diff --git a/arch/arm64/boot/dts/sprd/whale2.dtsi b/arch/arm64/boot/dts/sprd/whale2.dtsi
> new file mode 100644
> index 0000000..7c217c5
> --- /dev/null
> +++ b/arch/arm64/boot/dts/sprd/whale2.dtsi
> @@ -0,0 +1,71 @@
> +/*
> + * Spreadtrum Whale2 platform peripherals
> + *
> + * Copyright (C) 2016, Spreadtrum Communications Inc.
> + *
> + * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> + */
> +
> +/ {
> +       interrupt-parent = <&gic>;
> +       #address-cells = <2>;
> +       #size-cells = <2>;
> +
> +       soc: soc {
> +               compatible = "simple-bus";
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               ranges;
> +
> +               ap-apb {
> +                       compatible = "simple-bus";
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +                       ranges = <0 0x0 0x70000000 0x10000000>;
> +
> +                       uart0: serial@0 {
> +                               compatible = "sprd,sc9860-uart",
> +                                            "sprd,sc9836-uart";
> +                               reg = <0x0 0x100>;
> +                               interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
> +                               clocks = <&ext_26m>;
> +                               status = "disabled";
> +                       };
> +
> +                       uart1: serial@100000 {
> +                               compatible = "sprd,sc9860-uart",
> +                                            "sprd,sc9836-uart";
> +                               reg = <0x100000 0x100>;
> +                               interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> +                               clocks = <&ext_26m>;
> +                               status = "disabled";
> +                       };
> +
> +                       uart2: serial@200000 {
> +                               compatible = "sprd,sc9860-uart",
> +                                            "sprd,sc9836-uart";
> +                               reg = <0x200000 0x100>;
> +                               interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>;
> +                               clocks = <&ext_26m>;
> +                               status = "disabled";
> +                       };
> +
> +                       uart3: serial@300000 {
> +                               compatible = "sprd,sc9860-uart",
> +                                            "sprd,sc9836-uart";
> +                               reg = <0x300000 0x100>;
> +                               interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
> +                               clocks = <&ext_26m>;
> +                               status = "disabled";
> +                       };
> +               };
> +
> +       };
> +
> +       ext_26m: ext-26m {
> +               compatible = "fixed-clock";
> +               #clock-cells = <0>;
> +               clock-frequency = <26000000>;
> +               clock-output-names = "ext_26m";
> +       };
> +};
> --
> 2.7.4
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2] ARM: dts: at91: sama5d2: add m_can nodes
From: Quentin Schulz @ 2017-04-19 13:07 UTC (permalink / raw)
  To: Wenyou Yang, Nicolas.Ferre, Alexandre Belloni, Rob Herring,
	Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Russell King
  Cc: devicetree, Wenyou Yang, Oliver Hartkopp, linux-kernel, linux-can,
	linux-arm-kernel
In-Reply-To: <20170419084606.27543-1-wenyou.yang@atmel.com>

Hi,

On 19/04/2017 10:46, Wenyou Yang wrote:
> Add nodes to support the Controller Area Network(M_CAN) on SAMA5D2.
> The version of M_CAN IP core is 3.1.0 (CREL = 0x31040730).
> 
> As said in SAMA5D2 datasheet, the CAN clock is recommended to use
> frequencies of 20, 40 or 80 MHz. To achieve these frequencies,
> PMC GCLK3 must select the UPLLCK(480 MHz) as source clock and
> divide by 24, 12, or 6. So, the "assigned-clock-rates" property
> has three options: 20000000, 40000000, and 80000000.
> The "assigned-clock-parents" property should be referred to utmi
> fixedly.
> 
> The MSBs [bits 31:16] of the CAN Message RAM for CAN0 and CAN1 are
> default configured in 0x00200000. To avoid conflict with SRAM map
> for PM, change them to 0x00210000 in the AT91Bootstrap via setting
> the CAN Memories Address-based Register(SFR_CAN) of SFR.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
> The patch is tested on SAMA5D2 Xplained and based on the patch set,
>  1. [PATCH v4 1/7] can: m_can: Disabled Interrupt Line 1
>     http://marc.info/?l=linux-can&m=149165343604033&w=2
> 
> Changes in v2:
>  - Configures 10 TX Event FIFO elements and 10 TX Buffers/FIFO slots,
>    because the TXE FIFO is needed to be configured.
>  - Configure the offset of Message RAM for CAN1 followed from CAN0's.
> 


I've tested with the v4 patch series of Mario Hüttel on a SAMA5D2
Xplained and did the following:

# ip link set can0 down
# ip link set can0 up type can bitrate 125000
# cansend can0 500#1E.10.10

Last line at least 10 times, every message is received on my computer.
Same with can1. Tested with candump can0 as well, everything looks fine.

Tested-by: Quentin Schulz <quentin.schulz@free-electrons.com>

Thanks,
Quentin


>  arch/arm/boot/dts/at91-sama5d2_xplained.dts | 24 +++++++++++++
>  arch/arm/boot/dts/sama5d2.dtsi              | 56 +++++++++++++++++++++++++++++
>  2 files changed, 80 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
> index 9f7f8a7d8ff9..2f19b08dc226 100644
> --- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
> +++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
> @@ -257,6 +257,12 @@
>  				status = "okay";
>  			};
>  
> +			can0: can@f8054000 {
> +				pinctrl-names = "default";
> +				pinctrl-0 = <&pinctrl_can0_default>;
> +				status = "okay";
> +			};
> +
>  			uart3: serial@fc008000 {
>  				atmel,use-dma-rx;
>  				atmel,use-dma-tx;
> @@ -321,6 +327,18 @@
>  					bias-disable;
>  				};
>  
> +				pinctrl_can0_default: can0_default {
> +					pinmux = <PIN_PC10__CANTX0>,
> +						 <PIN_PC11__CANRX0>;
> +					bias-disable;
> +				};
> +
> +				pinctrl_can1_default: can1_default {
> +					pinmux = <PIN_PC26__CANTX1>,
> +						 <PIN_PC27__CANRX1>;
> +					bias-disable;
> +				};
> +
>  				pinctrl_charger_chglev: charger_chglev {
>  					pinmux = <PIN_PA12__GPIO>;
>  					bias-disable;
> @@ -468,6 +486,12 @@
>  				};
>  
>  			};
> +
> +			can1: can@fc050000 {
> +				pinctrl-names = "default";
> +				pinctrl-0 = <&pinctrl_can1_default>;
> +				status = "okay";
> +			};
>  		};
>  	};
>  
> diff --git a/arch/arm/boot/dts/sama5d2.dtsi b/arch/arm/boot/dts/sama5d2.dtsi
> index 22332be72140..383ca9307edf 100644
> --- a/arch/arm/boot/dts/sama5d2.dtsi
> +++ b/arch/arm/boot/dts/sama5d2.dtsi
> @@ -762,6 +762,18 @@
>  						atmel,clk-output-range = <0 83000000>;
>  					};
>  
> +					can0_clk: can0_clk {
> +						#clock-cells = <0>;
> +						reg = <56>;
> +						atmel,clk-output-range = <0 83000000>;
> +					};
> +
> +					can1_clk: can1_clk {
> +						#clock-cells = <0>;
> +						reg = <57>;
> +						atmel,clk-output-range = <0 83000000>;
> +					};
> +
>  					classd_clk: classd_clk {
>  						#clock-cells = <0>;
>  						reg = <59>;
> @@ -890,6 +902,18 @@
>  						#clock-cells = <0>;
>  						reg = <55>;
>  					};
> +
> +					can0_gclk: can0_gclk {
> +						#clock-cells = <0>;
> +						reg = <56>;
> +						atmel,clk-output-range = <0 80000000>;
> +					};
> +
> +					can1_gclk: can1_gclk {
> +						#clock-cells = <0>;
> +						reg = <57>;
> +						atmel,clk-output-range = <0 80000000>;
> +					};
>  				};
>  			};
>  
> @@ -1144,6 +1168,22 @@
>  				clocks = <&clk32k>;
>  			};
>  
> +			can0: can@f8054000 {
> +				compatible = "bosch,m_can";
> +				reg = <0xf8054000 0x4000>, <0x210000 0x4000>;
> +				reg-names = "m_can", "message_ram";
> +				interrupts = <56 IRQ_TYPE_LEVEL_HIGH 7>,
> +					     <64 IRQ_TYPE_LEVEL_HIGH 7>;
> +				interrupt-names = "int0", "int1";
> +				clocks = <&can0_clk>, <&can0_gclk>;
> +				clock-names = "hclk", "cclk";
> +				assigned-clocks = <&can0_gclk>;
> +				assigned-clock-parents = <&utmi>;
> +				assigned-clock-rates = <40000000>;
> +				bosch,mram-cfg = <0x0 0 0 32 0 0 10 10>;
> +				status = "disabled";
> +			};
> +
>  			spi1: spi@fc000000 {
>  				compatible = "atmel,at91rm9200-spi";
>  				reg = <0xfc000000 0x100>;
> @@ -1305,6 +1345,22 @@
>  				status = "okay";
>  			};
>  
> +			can1: can@fc050000 {
> +				compatible = "bosch,m_can";
> +				reg = <0xfc050000 0x4000>, <0x210000 0x4000>;
> +				reg-names = "m_can", "message_ram";
> +				interrupts = <57 IRQ_TYPE_LEVEL_HIGH 7>,
> +					     <65 IRQ_TYPE_LEVEL_HIGH 7>;
> +				interrupt-names = "int0", "int1";
> +				clocks = <&can1_clk>, <&can1_gclk>;
> +				clock-names = "hclk", "cclk";
> +				assigned-clocks = <&can1_gclk>;
> +				assigned-clock-parents = <&utmi>;
> +				assigned-clock-rates = <40000000>;
> +				bosch,mram-cfg = <0x1100 0 0 32 0 0 10 10>;
> +				status = "disabled";
> +			};
> +
>  			chipid@fc069000 {
>  				compatible = "atmel,sama5d2-chipid";
>  				reg = <0xfc069000 0x8>;
> 

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply

* Re: [PATCH v2 1/6] arm64: dts: Add symlinks for cros-ec-keyboard and cros-ec-sbs
From: Olof Johansson @ 2017-04-19 12:54 UTC (permalink / raw)
  To: Heiko Stuebner
  Cc: Brian Norris, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Doug Anderson,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	open list:ARM/Rockchip SoC..., Rob Herring, Chris Zhong,
	Stephen Barber,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Caesar Wang
In-Reply-To: <1529949.5djtX72aeJ@phil>

Hi,

On Tue, Feb 28, 2017 at 3:20 AM, Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> wrote:
> Hi Olof,
>
> Am Dienstag, 21. Februar 2017, 15:47:31 CET schrieb Olof Johansson:
>> On Thu, Feb 9, 2017 at 5:05 PM, Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>> > From: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> >
>> > We'd like to be able to use the cros-ec-keyboard.dtsi and
>> > cros-ec-sbs.dtsi snippets for arm64 devices.  Currently those files live
>> > in the arm/boot/dts directory.
>> >
>> > Let's follow the convention set by commit 8ee57b8182c4 ("ARM64: dts:
>> > vexpress: Use a symlink to vexpress-v2m-rs1.dtsi from arch=arm") and use
>> > a symlink.  Note that in this case we put the files in a new
>> > "include/common" directory since these snippets may need to be
>> > referenced by dts files in many different subdirectories.
>>
>> I'd rather have something like this:
>>
>> https://marc.info/?m=147547436324674&w=2
>>
>> Instead of having everybody move things over. I.e. make it easy to
>> refer to the arm version from arm64 instead of creating a "common"
>> layer inbetween.
>
> just so it gets noticed, I've done and tested [0], which hopefully should
> implement your suggestions above.
>
> If that looks ok, how do you want that picked up? Should I just include
> them in my regular rockchip branches or do you to pick them into some
> immutable branch, if other surprise-users turn up in time for 4.12?

Sigh. I completely dropped the ball on this, and I didn't see it
included in any of your pull requests for 4.12 since I never actually
acked that approach.

I've applied the patches onto a dt/include-paths stable branch, but
we're late for merging dependent code on top of it for 4.12.


-Olof
--
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 v3] usb: dwc3: add disable u2mac linestate check quirk
From: William Wu @ 2017-04-19 12:11 UTC (permalink / raw)
  To: balbi-DgEjT+Ai2ygdnm+yROfE0A,
	gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
  Cc: huangtao-TNX95d0MmH7DzftRWevZcw,
	devicetree-u79uwXL29TY76Z2rM5mHXA, heiko-4mtYJXux2i+zQB+pC5nmwQ,
	groeck-hpIqsD4AKlfQT0dZR+AlfA, frank.wang-TNX95d0MmH7DzftRWevZcw,
	linux-usb-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	dianders-hpIqsD4AKlfQT0dZR+AlfA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, william.wu-TNX95d0MmH7DzftRWevZcw,
	briannorris-hpIqsD4AKlfQT0dZR+AlfA,
	John.Youn-HKixBCOQz3hWk0Htik3J/w,
	daniel.meng-TNX95d0MmH7DzftRWevZcw

This patch adds a quirk to disable USB 2.0 MAC linestate check
during HS transmit. Refer the dwc3 databook, we can use it for
some special platforms if the linestate not reflect the expected
line state(J) during transmission.

When use this quirk, the controller implements a fixed 40-bit
TxEndDelay after the packet is given on UTMI and ignores the
linestate during the transmit of a token (during token-to-token
and token-to-data IPGAP).

On some rockchip platforms (e.g. rk3399), it requires to disable
the u2mac linestate check to decrease the SSPLIT token to SETUP
token inter-packet delay from 566ns to 466ns, and fix the issue
that FS/LS devices not recognized if inserted through USB 3.0 HUB.

Signed-off-by: William Wu <william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
Changes in v3:
- change quirk name
- only read and write GUCTL1 if dwc3 version >= 2.50a

Changes in v2:
- fix coding style

 Documentation/devicetree/bindings/usb/dwc3.txt |  2 ++
 drivers/usb/dwc3/core.c                        | 20 ++++++++++++++------
 drivers/usb/dwc3/core.h                        |  4 ++++
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index f658f39..52fb410 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -45,6 +45,8 @@ Optional properties:
 			a free-running PHY clock.
  - snps,dis-del-phy-power-chg-quirk: when set core will change PHY power
 			from P0 to P1/P2/P3 without delay.
+ - snps,dis-tx-ipgap-linecheck-quirk: when set, disable u2mac linestate check
+			during HS transmit.
  - snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
 			utmi_l1_suspend_n, false when asserts utmi_sleep_n
  - snps,hird-threshold: HIRD threshold
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 455d89a..9d5a67c 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -796,13 +796,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
 		dwc3_writel(dwc->regs, DWC3_GUCTL2, reg);
 	}
 
-	/*
-	 * Enable hardware control of sending remote wakeup in HS when
-	 * the device is in the L1 state.
-	 */
-	if (dwc->revision >= DWC3_REVISION_290A) {
+	if (dwc->revision >= DWC3_REVISION_250A) {
 		reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
-		reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
+
+		/*
+		 * Enable hardware control of sending remote wakeup
+		 * in HS when the device is in the L1 state.
+		 */
+		if (dwc->revision >= DWC3_REVISION_290A)
+			reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
+
+		if (dwc->dis_tx_ipgap_linecheck_quirk)
+			reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
+
 		dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
 	}
 
@@ -1023,6 +1029,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 				"snps,dis-u2-freeclk-exists-quirk");
 	dwc->dis_del_phy_power_chg_quirk = device_property_read_bool(dev,
 				"snps,dis-del-phy-power-chg-quirk");
+	dwc->dis_tx_ipgap_linecheck_quirk = device_property_read_bool(dev,
+				"snps,dis-tx-ipgap-linecheck-quirk");
 
 	dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
 				"snps,tx_de_emphasis_quirk");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 981c77f..6f6294d 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -204,6 +204,7 @@
 #define DWC3_GCTL_DSBLCLKGTNG		BIT(0)
 
 /* Global User Control 1 Register */
+#define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS	BIT(28)
 #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW	BIT(24)
 
 /* Global USB2 PHY Configuration Register */
@@ -850,6 +851,8 @@ struct dwc3_scratchpad_array {
  *			provide a free-running PHY clock.
  * @dis_del_phy_power_chg_quirk: set if we disable delay phy power
  *			change quirk.
+ * @dis_tx_ipgap_linecheck_quirk: set if we disable u2mac linestate
+ *			check during HS transmit.
  * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
  * @tx_de_emphasis: Tx de-emphasis value
  * 	0	- -6dB de-emphasis
@@ -1004,6 +1007,7 @@ struct dwc3 {
 	unsigned		dis_rxdet_inp3_quirk:1;
 	unsigned		dis_u2_freeclk_exists_quirk:1;
 	unsigned		dis_del_phy_power_chg_quirk:1;
+	unsigned		dis_tx_ipgap_linecheck_quirk:1;
 
 	unsigned		tx_de_emphasis_quirk:1;
 	unsigned		tx_de_emphasis:2;
-- 
2.0.0

^ 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