Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: M'boumba Cedric Madianga @ 2017-01-12 16:17 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Wolfram Sang, Rob Herring, Maxime Coquelin, Alexandre Torgue,
	Linus Walleij, Patrice Chotard, Russell King, linux-i2c,
	devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <CAOAejn2pW20VPP_yGtvJ_ufvj6Xj1poBiiA2WqkALiaLyyONug@mail.gmail.com>

>>> > I don't understand scl_period = 1 µs for Fast Mode. For a bus freqency
>>> > of 400 kHz we need low + high = 2.5 µs. Is there a factor 10 missing
>>> > somewhere?
>>>
>>> As CCR = SCL_period * I2C parent clk frequency with minimal freq =
>>> 2Mhz and SCL_period = 1 we have:
>>> CCR = 1 * 2Mhz = 2.
>>> But to compute, scl_low and scl_high in Fast mode, we have to do the
>>> following thing as Duty=1:
>>> scl_high = 9 * CCR * I2C parent clk period
>>> scl_low = 16 * CCR * I2C parent clk period
>>> In our example:
>>> scl_high = 9 * 2 * 0,0000005 = 0,000009 sec = 9 µs
>>> scl_low = 16 * 2 * 0.0000005 = 0,000016 sec = 16 µs
>>> So low + high = 27 µs > 2,5 µs
>>
>> For me 9 µs + 16 µs is 25 µs, resulting in 40 kHz. That's why I wondered
>> if there is a factor 10 missing somewhere.
>
> Hum ok. I am going to double-check what is wrong because when I check
> with the scope I always reach 400Khz for SCL.
> I will let you know.

There is one point I miss here that is described in the reference manual:
To reach the 400 kHz maximum I²C fast mode clock, the I2C parent rate
must be a multiple of 10 MHz.
So, contrary to what we said in a previous thread, 400 kHz could not
be reached with low frequencies.
In that way, we could compute CCR with duty = 0 by default.
So, I find another formula very close to the first one I pushed in the
first version:

In fast mode, we compute CCR with duty = 0:
t_scl_high = CCR * I2C parent clk period
t_scl_low = 2 *CCR * I2C parent clk period
So, CCR = I2C parent rate / 400 kHz / 3

For example with parent rate = 40 MHz:
CCR = 40000000 / 400000 / 3 = 33.333333333 = 33
t_scl_high = 33 * (1 / 2000000) = 825 ns > 600 ns
t_scl_low = 2 * 16 * (1 / 2000000) = 1650 ns > 1300 ns

It seems ok now.

Best regards,

Cedric

^ permalink raw reply

* Re: [PATCH] pinctrl: qcom: Add msm8998 pinctrl driver
From: Bjorn Andersson @ 2017-01-12 16:20 UTC (permalink / raw)
  To: Imran Khan
  Cc: andy.gross, Linus Walleij, Rob Herring, Mark Rutland, David Brown,
	open list:PIN CONTROL SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, open list:ARM/QUALCOMM SUPPORT,
	open list:ARM/QUALCOMM SUPPORT
In-Reply-To: <1483974019-8235-1-git-send-email-kimran@codeaurora.org>

On Mon 09 Jan 07:00 PST 2017, Imran Khan wrote:

> diff --git a/drivers/pinctrl/qcom/pinctrl-msm8998.c b/drivers/pinctrl/qcom/pinctrl-msm8998.c
[..]
> +
> +#define NORTH	0x500000
> +#define WEST	0x100000
> +#define EAST	0x900000

So the control registers are now laid out in 3 non-contiguous ranges?
Please move them to the top of the file.

> +#define REG_SIZE 0x1000

Just inline 0x1000 in the macro below.

> +#define PINGROUP(id, base, f1, f2, f3, f4, f5, f6, f7, f8, f9)	\
> +	{					        \
> +		.name = "gpio" #id,			\
> +		.pins = gpio##id##_pins,		\
> +		.npins = (unsigned int)ARRAY_SIZE(gpio##id##_pins),	\
> +		.funcs = (int[]){			\
> +			msm_mux_gpio, /* gpio mode */	\
> +			msm_mux_##f1,			\
> +			msm_mux_##f2,			\
> +			msm_mux_##f3,			\
> +			msm_mux_##f4,			\
> +			msm_mux_##f5,			\
> +			msm_mux_##f6,			\
> +			msm_mux_##f7,			\
> +			msm_mux_##f8,			\
> +			msm_mux_##f9			\
> +		},				        \
> +		.nfuncs = 10,				\
> +		.ctl_reg = base + REG_SIZE * id,	\
> +		.io_reg = base + 0x4 + REG_SIZE * id,		\
> +		.intr_cfg_reg = base + 0x8 + REG_SIZE * id,	\
> +		.intr_status_reg = base + 0xc + REG_SIZE * id,	\
> +		.intr_target_reg = base + 0x8 + REG_SIZE * id,	\
> +		.mux_bit = 2,			\
> +		.pull_bit = 0,			\
> +		.drv_bit = 6,			\
> +		.oe_bit = 9,			\
> +		.in_bit = 0,			\
> +		.out_bit = 1,			\
> +		.intr_enable_bit = 0,		\
> +		.intr_status_bit = 0,		\
> +		.intr_target_bit = 5,		\
> +		.intr_target_kpss_val = 3,  \
> +		.intr_raw_status_bit = 4,	\
> +		.intr_polarity_bit = 1,		\
> +		.intr_detection_bit = 2,	\
> +		.intr_detection_width = 2,	\
> +	}
> +
> +#define SDC_QDSD_PINGROUP(pg_name, ctl, pull, drv)	\
> +	{					        \
> +		.name = #pg_name,			\
> +		.pins = pg_name##_pins,			\
> +		.npins = (unsigned int)ARRAY_SIZE(pg_name##_pins),	\
> +		.ctl_reg = ctl,				\
> +		.io_reg = 0,				\
> +		.intr_cfg_reg = 0,			\
> +		.intr_status_reg = 0,			\
> +		.intr_target_reg = 0,			\
> +		.mux_bit = -1,				\
> +		.pull_bit = pull,			\
> +		.drv_bit = drv,				\
> +		.oe_bit = -1,				\
> +		.in_bit = -1,				\
> +		.out_bit = -1,				\
> +		.intr_enable_bit = -1,			\
> +		.intr_status_bit = -1,			\
> +		.intr_target_bit = -1,			\
> +		.intr_raw_status_bit = -1,		\
> +		.intr_polarity_bit = -1,		\
> +		.intr_detection_bit = -1,		\
> +		.intr_detection_width = -1,		\
> +	}
> +
> +#define UFS_RESET(pg_name, offset)				\
> +	{					        \
> +		.name = #pg_name,			\
> +		.pins = pg_name##_pins,			\
> +		.npins = (unsigned int)ARRAY_SIZE(pg_name##_pins),	\
> +		.ctl_reg = offset,			\
> +		.io_reg = offset + 0x4,			\
> +		.intr_cfg_reg = 0,			\
> +		.intr_status_reg = 0,			\
> +		.intr_target_reg = 0,			\
> +		.mux_bit = -1,				\
> +		.pull_bit = 3,				\
> +		.drv_bit = 0,				\
> +		.oe_bit = -1,				\
> +		.in_bit = -1,				\
> +		.out_bit = 0,				\
> +		.intr_enable_bit = -1,			\
> +		.intr_status_bit = -1,			\
> +		.intr_target_bit = -1,			\
> +		.intr_raw_status_bit = -1,		\
> +		.intr_polarity_bit = -1,		\
> +		.intr_detection_bit = -1,		\
> +		.intr_detection_width = -1,		\
> +	}

Please add an empty line here.

> +static const struct pinctrl_pin_desc msm8998_pins[] = {
[..]
> +enum msm8998_functions {
[..]
> +	msm_mux_phase_flag6,
> +	msm_mux_phase_flag29,
> +	msm_mux_phase_flag30,
> +	msm_mux_phase_flag31,

With the Qualcomm pinctrl driver it's possible to specify configuration
in DT for a subset of pins of a group. So I think you should squash the
"phase_flag"s and "atest_char" into one group each.

> +	msm_mux_pa_indicator,
> +	msm_mux_ssbi1,
> +	msm_mux_isense_dbg,
> +	msm_mux_mss_lte,
> +	msm_mux_gpio,
> +	msm_mux_NA,
> +};
[..]
> +static const struct msm_pingroup msm8998_groups[] = {
> +	PINGROUP(0, EAST, blsp_spi1, blsp_uart1_a, blsp_uim1_a, NA, NA, NA, NA,
> +		 NA, NA),

Please do ignore the 80-char "rule" and skip the line break on
these - it makes the table easier to read.

[..]
> +};
> +
> +static const struct msm_pinctrl_soc_data msm8998_pinctrl = {
> +	.pins = msm8998_pins,
> +	.npins = ARRAY_SIZE(msm8998_pins),
> +	.functions = msm8998_functions,
> +	.nfunctions = ARRAY_SIZE(msm8998_functions),
> +	.groups = msm8998_groups,
> +	.ngroups = ARRAY_SIZE(msm8998_groups),
> +	.ngpios = 153,

ngpios is 150

> +};
> +

Regards,
Bjorn

^ permalink raw reply

* Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver
From: Greg KH @ 2017-01-12 16:26 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Cyril Bur, devicetree-u79uwXL29TY76Z2rM5mHXA,
	jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w, arnd-r2nGTMty4D4,
	joel-U3u1mxZcP9KHXe+LvDLADg, mark.rutland-5wv7dgnIgG8,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
	andrew-zrmu5oMJ5Fs, xow-hpIqsD4AKlfQT0dZR+AlfA,
	jk-mnsaURCQ41sdnm+yROfE0A
In-Reply-To: <1484237253.2492.43.camel-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>

On Thu, Jan 12, 2017 at 10:07:33AM -0600, Benjamin Herrenschmidt wrote:
> On Thu, 2017-01-12 at 17:00 +0100, Greg KH wrote:
> > > How so ? I mean, access_ok followed by __get/__put_user is still a
> > > classic, what's wrong with it ?
> > 
> > No "normal" driver should do that, just call copy_to/from_user and be
> > done with it.  That way all of the proper locking and validation checks
> > like this are done correctly for you.  Why would a driver ever call the
> > "raw" __get/__put_user functions?
> 
> I supposed historically it was considered faster for some things :-)
> 
> Not a huge deal, and yes it's probably cleaner, I was just wondering
> what was "racy" about access_ok() that I might have missed...

I think, you can change things after access_ok() happens, there used to
be bugs in that area a few years ago.  I think we fixed them by moving
the offending drivers to use copy_*() instead.

thanks,

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

^ permalink raw reply

* Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver
From: Greg KH @ 2017-01-12 16:27 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Cyril Bur, devicetree-u79uwXL29TY76Z2rM5mHXA,
	jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w, arnd-r2nGTMty4D4,
	joel-U3u1mxZcP9KHXe+LvDLADg, mark.rutland-5wv7dgnIgG8,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
	andrew-zrmu5oMJ5Fs, xow-hpIqsD4AKlfQT0dZR+AlfA,
	jk-mnsaURCQ41sdnm+yROfE0A
In-Reply-To: <1484235315.2492.41.camel-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>

On Thu, Jan 12, 2017 at 09:35:15AM -0600, Benjamin Herrenschmidt wrote:
> On Thu, 2017-01-12 at 21:16 +1100, Cyril Bur wrote:
> > My aim here was to only have one process playing with the LPC mapping
> > registers at a time.
> > 
> > > Again, use UIO, it will save you from yourself...
> > > 
> > 
> > Thank-you! This is the first I've heard of UIO and I'll investigate
> > furthur!
> 
> Greg, I don't think UIO is the answer here either. Note, this isn't an
> exploit so much as root shooting itself in the foot as this driver
> should never be accessed by anybody but root, but see below.
> 
> This is a BMC, ie, the system controller of a x86 or POWER based
> system.
> 
> The LPC controller controls the LPC bus which is mastered by the "host"
> (ie. the x86 or PPC) and acts as a slave on the BMC side.
> 
> It has a bunch of registers that need to be configured in more/less
> system specific ways by the BMC, but more so, it has a pair of
> registers that allow "mapping" of a region of the BMC physical address 
> space into the host address space.
> 
> This is by definition dangerous to configure since it gives you a
> window to any part of the BMC, kernel space, any IOs, etc... however it
> needs to be configured by a userspace daemon which communicates with
> the host via a mailbox in order to map either different portions of the
> system flash controller address space or reserved memory.
> 
> So in fact it should be done by the kernel, not userspace.
> 
> What Cyril needs to do to make it more secure is:
> 
>   - For random register accesses, white list what registers
> specifically are allowed (and if necessary filter values). These
> registers aren't dangerous from the BMC perspective and need to be set
> appropriately for the host to operate correctly.
> 
>   - For the mapping of the LPC FW space <-> BMC space, use ioctl's to
> explicit establish the mapping  to a portion of the flash (and nowhere
> else) or one of the known reserved memory areas. IE, dont have
> userspace just pass raw physical addresses through, but tell the kernel
> driver what portion (offset/size) of what area (flash space or reserved
> memory region) to configure the HW window for.

Yes, something more needs to be documented here, as what was proposed
isn't acceptable at all.

thanks,

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

^ permalink raw reply

* Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver
From: Benjamin Herrenschmidt @ 2017-01-12 16:29 UTC (permalink / raw)
  To: Cyril Bur, Greg KH
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w, arnd-r2nGTMty4D4,
	joel-U3u1mxZcP9KHXe+LvDLADg, mark.rutland-5wv7dgnIgG8,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
	andrew-zrmu5oMJ5Fs, xow-hpIqsD4AKlfQT0dZR+AlfA,
	jk-mnsaURCQ41sdnm+yROfE0A
In-Reply-To: <1484235315.2492.41.camel-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>

On Thu, 2017-01-12 at 09:35 -0600, Benjamin Herrenschmidt wrote:
> Greg, I don't think UIO is the answer here either. Note, this isn't an
> exploit so much as root shooting itself in the foot as this driver
> should never be accessed by anybody but root, but see below.

Reading back my previous email I realize that the lack of coffee
made my prose a lot less clear than I intended it to be :-)

I think some background is in order here, it will help whoever
reviews this and Cyril, skip to the bottom to how I think you should
articulate the driver.

So on a bunch of server systems, you have a system controller typically
known as a BMC controller all sort of things such as power to various
elements, sometimes fans, often the system flash, etc...

The Aspeed BMC family which is what is used on OpenPOWER machines and a
number of x86 as well is typically connected to the host via an LPC
bus. (among others).

This is an ISA bus on steroids, it has IO and MEM/FW cycles (different
address spaces, the subtle differences between MEM and FW can be
ignored for the sake of this discussion). It's generally used by the
BMC chip to provide the host with access to the system flash that
contains the BIOS or other host firmware (via MEM/FW space) along with
a number of SuperIO-style IOs (via IO space) such as UARTs, IPMI
controllers, etc....

On the BMC chip side, this is all configured via a bunch of registers
whose content is related to a given policy of what devices are exposed
how and where on a given system, which is system/vendor specific, so we
don't want to bolt that into the BMC kernel. So this started with a
need to provide something nicer than /dev/mem for user space to
configure these things. At that point, something like UIO could have
still made sense. However...

One important aspect of the configuration is how the MEM/FW space is
exposed to the host (ie, the x86 or POWER). Some registers in that
bridge can define a window remapping all or portion of the LPC MEM/FW
space to a portion of the BMC internal bus, with no specific limits
imposed in HW.

As you can see, this can be pretty nasty. So for this, I think it makes
sense to ensure that this window is configured by a kernel driver that
can apply some serious sanity checks on what it is configured to map.

In practice, user space wants to control this by flipping the mapping
between essentially two types of portions of the BMC address space:

   - The flash space. This is a region of the BMC MMIO space that
more/less directly maps the system flash (at least for reads, writes
are somewhat more complicated).

   - One (or more) reserved area(s) of the BMC physical memory.

The latter is needed for a number of things, such as avoiding letting
the host manipulate the innards of the BMC flash controller via some
evil backdoor, we want to do flash updates by routing the window to a
portion of memory (under control of a mailbox protocol via some
separate set of registers) which the host can use to write new data in
bulk and then request the BMC to flash it. There are other uses, such
as allowing the host to boot from an in-memory flash image rather than
the one in flash (very handy for continuous integration and test, the
BMC can just download new images), etc...

So I think the best approach here is:

	- A pair of ioctls to read and write random registers in the
LPC bridge for all the "generally configuration gunk". These have a
filter to ensure that the registers controlling the above mapping
cannot be accessed that way.

	- An ioctl to control the above mapping window. It takes as
arguments the location in LPC space, the window type (flash vs.
memory), for memory, maybe an ID (several windows to chose from), and
the offset& size in the latter. The driver can enforce that the windows
are one of the specially reserved areas of memory etc...

	- An mmap function to map those reserved windows into userspace
so the daemon can communicate appropriately (only needed for the memory
windows, the flash space is accessed via the normal /dev/mtd drivers)

Greg, does that make sense ?

Cheers,
Ben.

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

^ permalink raw reply

* Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver
From: Benjamin Herrenschmidt @ 2017-01-12 16:31 UTC (permalink / raw)
  To: Greg KH
  Cc: Cyril Bur, devicetree-u79uwXL29TY76Z2rM5mHXA,
	jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w, arnd-r2nGTMty4D4,
	joel-U3u1mxZcP9KHXe+LvDLADg, mark.rutland-5wv7dgnIgG8,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
	andrew-zrmu5oMJ5Fs, xow-hpIqsD4AKlfQT0dZR+AlfA,
	jk-mnsaURCQ41sdnm+yROfE0A
In-Reply-To: <20170112162619.GB10283-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>

On Thu, 2017-01-12 at 17:26 +0100, Greg KH wrote:
> I think, you can change things after access_ok() happens, there used to
> be bugs in that area a few years ago.  I think we fixed them by moving
> the offending drivers to use copy_*() instead.

Ok, I'm surprised though, we still have a metric ton of code,
especially in filesystems, who do access_ok. Generally the idea here is
that the enforcement is done by the MMU normally via the permission in
the page tables. access_ok() is simply needed to make sure we access
the portion of the page tables representing user space, not kernel
space, and that is a pretty fixed dichotomy.

Anyway, this is academic, I agree that copy_to/from_... is nicer.

Cheers,
Ben.

--
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: Re: [PATCH 4/5] ARM: dts: sunxi: add dtsi file for V3s SoC
From: Maxime Ripard @ 2017-01-12 16:52 UTC (permalink / raw)
  To: Icenowy Zheng
  Cc: linux-clk-u79uwXL29TY76Z2rM5mHXA, linux-kernel,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Stephen Boyd,
	linux-doc-u79uwXL29TY76Z2rM5mHXA, Linus Walleij,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Chen-Yu Tsai
In-Reply-To: <20170111065638.uW1SQ7Kq-jDEamKawf7I0PDqKvflMoHmW9unr2Ajn@public.gmane.org>

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

On Wed, Jan 11, 2017 at 11:56:32AM +0800, Icenowy Zheng wrote:
> 
> 2017年1月11日 02:21于 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>写道:
> >
> > On Tue, Jan 03, 2017 at 11:16:28PM +0800, Icenowy Zheng wrote: 
> > > + uart0_pins_a: uart0@0 { 
> > > + pins = "PB8", "PB9"; 
> > > + function = "uart0"; 
> > > + bias-pull-up; 
> >
> > Why do you need a pullup here? 
> 
> I think TX needs one, but RX do not need.

That's (at best) board specific. So it belongs in the DTS if it's
truely needed.

Maxime

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

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

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

^ permalink raw reply

* Re: [PATCH] i2c: core: helper function to detect slave mode
From: Andy Shevchenko @ 2017-01-12 17:01 UTC (permalink / raw)
  To: Vladimir Zapolskiy, Andy Shevchenko
  Cc: Luis Oliveira, Wolfram Sang, Rob Herring, Mark Rutland,
	Jarkko Nikula, Mika Westerberg, linux-i2c, devicetree,
	linux-kernel@vger.kernel.org, Ramiro.Oliveira, Joao Pinto,
	CARLOS.PALMINHA
In-Reply-To: <3748130b-5321-12eb-ec75-e2637dd9fc54@mleia.com>

On Sat, 2017-01-07 at 03:24 +0200, Vladimir Zapolskiy wrote:
> On 01/07/2017 02:19 AM, Andy Shevchenko wrote:
> > On Sat, Jan 7, 2017 at 1:43 AM, Vladimir Zapolskiy <vz@mleia.com>
> > wrote:
> > > On 01/07/2017 12:45 AM, Andy Shevchenko wrote:

> > > > +             }
> > > > > > +     } else if (IS_BUILTIN(CONFIG_ACPI) &&
> > > > > > ACPI_HANDLE(dev)) {
> > > > > > +             dev_dbg(dev, "ACPI slave is not supported
> > > > > > yet\n");
> > > > > > +     }
> > > > > 
> > > > > If so, then it might be better to drop else-if stub for now.
> > > > 
> > > > Please, don't.
> > > > 
> > > 
> > > Why do you ask for this stub to be added?
> > 
> > 1. Exactly the reason you asked above. Here is the code which has
> > built differently on different platforms. x86 usually is not using
> > CONFIG_OF, ARM doesn't ACPI (versus ARM64). Check GPIO library for
> > existing examples.
> 
> From the context by the stub I mean dev_dbg() in
> i2c_slave_mode_detect()
> function, I don't see a connection to GPIO library, please clarify.

I agree that is not good proof for using IS_ENABLED/IS_BUILTIN macro.

> > 2. We might add that support later, but here is again, just no-op.
> > 
> > So, what is your strong argument here against that?
> 
> When the support is ready for ACPI case, you'll remove the added
> dev_dbg(), and I don't see a good point by adding it temporarily.

It would remind me to look at it at some point.

> What is wrong with the approach of adding the ACPI case handling
> branch when it is ready and remove any kind of stubs right now?

I will not object. Here is maintainer, let him speak.

> On ACPI platforms the function returns 'false' always, will the
> function work correctly (= corresponding to its description) as is?

Yes.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply

* [PATCH] ARM: dts: sun8i-h3: Add dts for the Beelink X2 STB
From: codekipper-Re5JQEeQqe8AvxtiuMwx3w @ 2017-01-12 17:11 UTC (permalink / raw)
  To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Marcus Cooper

From: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

The Beelink X2 is an STB based on the Allwinner H3 SoC with a uSD slot,
2 USB ports( 1 * USB-2 Host, 1 USB OTG), a 10/100M ethernet port using the
SoC's integrated PHY, Wifi via a RTL8189ETV sdio wifi chip, HDMI, an IR
receiver, 1 LEDs and an optical S/PDIF connector.

Signed-off-by: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/arm/boot/dts/Makefile                |   1 +
 arch/arm/boot/dts/sun8i-h3-beelink-x2.dts | 187 ++++++++++++++++++++++++++++++
 2 files changed, 188 insertions(+)
 create mode 100644 arch/arm/boot/dts/sun8i-h3-beelink-x2.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 78a94b747059..75960b1468a4 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -857,6 +857,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
 	sun8i-a83t-cubietruck-plus.dtb \
 	sun8i-h2-plus-orangepi-zero.dtb \
 	sun8i-h3-bananapi-m2-plus.dtb \
+	sun8i-h3-beelink-x2.dtb \
 	sun8i-h3-nanopi-m1.dtb	\
 	sun8i-h3-nanopi-neo.dtb \
 	sun8i-h3-orangepi-2.dtb \
diff --git a/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts b/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
new file mode 100644
index 000000000000..bc5aed52cb1d
--- /dev/null
+++ b/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
@@ -0,0 +1,187 @@
+/*
+ * Copyright (C) 2017 Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ *  a) This file is free software; you can redistribute it and/or
+ *     modify it under the terms of the GNU General Public License as
+ *     published by the Free Software Foundation; either version 2 of the
+ *     License, or (at your option) any later version.
+ *
+ *     This file 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.
+ *
+ * Or, alternatively,
+ *
+ *  b) Permission is hereby granted, free of charge, to any person
+ *     obtaining a copy of this software and associated documentation
+ *     files (the "Software"), to deal in the Software without
+ *     restriction, including without limitation the rights to use,
+ *     copy, modify, merge, publish, distribute, sublicense, and/or
+ *     sell copies of the Software, and to permit persons to whom the
+ *     Software is furnished to do so, subject to the following
+ *     conditions:
+ *
+ *     The above copyright notice and this permission notice shall be
+ *     included in all copies or substantial portions of the Software.
+ *
+ *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ *     OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+#include "sun8i-h3.dtsi"
+#include "sunxi-common-regulators.dtsi"
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/pinctrl/sun4i-a10.h>
+
+/ {
+	model = "Beelink X2";
+	compatible = "roofull,beelink-x2", "allwinner,sun8i-h3";
+
+	aliases {
+		serial0 = &uart0;
+		/* ethernet0 is the H3 emac, defined in sun8i-h3.dtsi */
+		ethernet1 = &rtl8189ftv;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	leds {
+		compatible = "gpio-leds";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwr_led_x2>;
+
+		pwr_led {
+			label = "beelink-x2:red:pwr";
+			gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>; /* PL10 */
+			default-state = "on";
+		};
+	};
+
+	gpio_keys {
+		compatible = "gpio-keys";
+		pinctrl-names = "default";
+		pinctrl-0 = <&sw_r_x2>;
+
+		sw4 {
+			label = "power";
+			linux,code = <BTN_0>;
+			gpios = <&r_pio 0 3 GPIO_ACTIVE_LOW>;
+		};
+	};
+
+	wifi_pwrseq: wifi_pwrseq {
+		compatible = "mmc-pwrseq-simple";
+		pinctrl-names = "default";
+		pinctrl-0 = <&wifi_en_x2>;
+		reset-gpios = <&r_pio 0 7 GPIO_ACTIVE_LOW>; /* PL7 */
+	};
+};
+
+&ehci1 {
+	status = "okay";
+};
+
+&ir {
+	pinctrl-names = "default";
+	pinctrl-0 = <&ir_pins_a>;
+	status = "okay";
+};
+
+&mmc0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin>;
+	vmmc-supply = <&reg_vcc3v3>;
+	bus-width = <4>;
+	cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
+	cd-inverted;
+	status = "okay";
+};
+
+&mmc1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc1_pins_a>;
+	vmmc-supply = <&reg_vcc3v3>;
+	bus-width = <4>;
+	non-removable;
+	status = "okay";
+
+	/*
+	 * Explicitly define the sdio device, so that we can add an ethernet
+	 * alias for it (which e.g. makes u-boot set a mac-address).
+	 */
+	rtl8189ftv: sdio_wifi@1 {
+		reg = <1>;
+	};
+};
+
+&mmc2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc2_8bit_pins>;
+	vmmc-supply = <&reg_vcc3v3>;
+	bus-width = <8>;
+	non-removable;
+	cap-mmc-hw-reset;
+	status = "okay";
+};
+
+&mmc2_8bit_pins {
+	/* Increase drive strength for DDR modes */
+	drive-strength = <40>;
+	/* eMMC is missing pull-ups */
+	bias-pull-up;
+};
+
+&ohci1 {
+	status = "okay";
+};
+
+&r_pio {
+	pwr_led_x2: led_pins@0 {
+		pins = "PL10";
+		function = "gpio_out";
+	};
+
+	sw_r_x2: key_pins@0 {
+		pins = "PL3";
+		function = "gpio_in";
+	};
+
+	wifi_en_x2: wifi_en_pin {
+		pins = "PL7";
+		function = "gpio_out";
+	};
+};
+
+&uart0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart0_pins_a>;
+	status = "okay";
+};
+
+&uart1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
+	status = "okay";
+};
+
+&usbphy {
+	/* USB VBUS is on as long as VCC-IO is on */
+	status = "okay";
+};
-- 
2.11.0

^ permalink raw reply related

* Re: Re: [PATCH 2/5] clk: sunxi-ng: add support for V3s CCU
From: Maxime Ripard @ 2017-01-12 17:13 UTC (permalink / raw)
  To: Icenowy Zheng
  Cc: linux-clk-u79uwXL29TY76Z2rM5mHXA, linux-kernel,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Stephen Boyd,
	linux-doc-u79uwXL29TY76Z2rM5mHXA, Linus Walleij,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Chen-Yu Tsai
In-Reply-To: <20170111065524.tILSH8cS-V6dNpkQgAPs0PDqKvflMoHmW9unr2Ajn@public.gmane.org>

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

Hi,

On Wed, Jan 11, 2017 at 11:55:16AM +0800, Icenowy Zheng wrote:
> 
> 2017年1月11日 02:10于 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>写道:
> >
> > On Tue, Jan 03, 2017 at 11:16:26PM +0800, Icenowy Zheng wrote: 
> > > V3s has a similar but cut-down CCU to H3. 
> > > 
> > > Add support for it. 
> > > 
> > > Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org> 
> >
> > It looks like there's nothing different but the clocks that you 
> > register with the H3, please just use the H3 driver. 
> 
> USB gate part is different, and many things have gone.

As far as I can see, you're only adding a few clocks, and removing a
bunch. This is definitely something that can be dealt with by simply
registering a slightly different set of clocks, like we're doing on
sun5i.

Maxime

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

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

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

^ permalink raw reply

* Re: [PATCH 1/5] arm: sunxi: add support for V3s SoC
From: Maxime Ripard @ 2017-01-12 17:17 UTC (permalink / raw)
  To: Icenowy Zheng
  Cc: Chen-Yu Tsai, Stephen Boyd, Linus Walleij,
	linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
In-Reply-To: <2706391484163632-/w1NKm4lK9luio3avFS2gg@public.gmane.org>

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

1;4601;0c
On Thu, Jan 12, 2017 at 03:40:32AM +0800, Icenowy Zheng wrote:
> 
> 
> 11.01.2017, 02:09, "Maxime Ripard" <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> > On Tue, Jan 03, 2017 at 11:16:25PM +0800, Icenowy Zheng wrote:
> >>  Allwinner V3s is a low-end single-core Cortex-A7 SoC, with 64MB
> >>  integrated DRAM, and several peripherals.
> >>
> >>  Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> >>  ---
> >>   Documentation/arm/sunxi/README | 4 ++++
> >>   arch/arm/mach-sunxi/sunxi.c | 1 +
> >>   2 files changed, 5 insertions(+)
> >>
> >>  diff --git a/Documentation/arm/sunxi/README b/Documentation/arm/sunxi/README
> >>  index cd0243302bc1..91ec8f2055be 100644
> >>  --- a/Documentation/arm/sunxi/README
> >>  +++ b/Documentation/arm/sunxi/README
> >>  @@ -67,6 +67,10 @@ SunXi family
> >>           + Datasheet
> >>             http://dl.linux-sunxi.org/H3/Allwinner_H3_Datasheet_V1.0.pdf
> >>
> >>  + - Allwinner V3s (sun8i)
> >>  + + Datasheet
> >>  + https://www.goprawn.com/forum/allwinner-cams/783-allwinner-v3s-soc-datasheet
> >>  +
> >
> > Please don't put random links in there, but at least something that we
> > know will be there in a couple of weeks/monthes/years
> 
> Is http://linux-sunxi.org/File:Allwinner_V3s_Datasheet_V1.0.pdf acceptable?

It's much better, yes.

Thanks!
Maxime

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

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

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

^ permalink raw reply

* Re: [PATCH v5 4/5] powernv: Pass PSSCR value and mask to power9_idle_stop
From: Balbir Singh @ 2017-01-12 17:18 UTC (permalink / raw)
  To: Gautham R. Shenoy
  Cc: Stewart Smith, devicetree, Michael Neuling, linux-pm,
	Shreyas B. Prabhu, Daniel Lezcano, Rafael J. Wysocki,
	linux-kernel, Rob Herring, Paul Mackerras, Shilpasri G Bhat,
	Oliver O'Halloran, mark.rutland, linuxppc-dev
In-Reply-To: <1484039224-5630-5-git-send-email-ego@linux.vnet.ibm.com>

On Tue, Jan 10, 2017 at 02:37:03PM +0530, Gautham R. Shenoy wrote:
> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> 
> The arch300_idle_stop method currently takes only the requested stop
      power9_idle_stop (see subject :) and second paragraph)
> level as a parameter and picks up the rest of the PSSCR bits from a
> hand-coded macro. This is not a very flexible design, especially when
> the firmware has the capability to communicate the psscr value and the
> mask associated with a particular stop state via device tree.
> 
> This patch modifies the power9_idle_stop API to take as parameters the
> PSSCR value and the PSSCR mask corresponding to the stop state that
> needs to be set. These PSSCR value and mask are respectively obtained
> by parsing the "ibm,cpu-idle-state-psscr" and
> "ibm,cpu-idle-state-psscr-mask" fields from the device tree.
> 
> In addition to this, the patch adds support for handling stop states
> for which ESL and EC bits in the PSSCR are zero. As per the
> architecture, a wakeup from these stop states resumes execution from
> the subsequent instruction as opposed to waking up at the System
> Vector.
> 
> The older firmware sets only the Requested Level (RL) field in the
> psscr and psscr-mask exposed in the device tree. For older firmware
> where psscr-mask=0xf, this patch will set the default sane values that
> the set for for remaining PSSCR fields (i.e PSLL, MTL, ESL, EC, and
> TR). For the new firmware, the patch will validate that the invariants
> required by the ISA for the psscr values are maintained by the
> firmware.
> 
> This skiboot patch that exports fully populated PSSCR values and the
> mask for all the stop states can be found here:
> https://lists.ozlabs.org/pipermail/skiboot/2016-September/004869.html
> 
> [Optimize the number of instructions before entering STOP with
> ESL=EC=0, validate the PSSCR values provided by the firimware
> maintains the invariants required as per the ISA suggested by Balbir
> Singh]
> 
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---

Acked-by: Balbir Singh <bsingharora@gmail.com>

^ permalink raw reply

* Re: [PATCH 2/5] clk: sunxi-ng: add support for V3s CCU
From: Maxime Ripard @ 2017-01-12 17:19 UTC (permalink / raw)
  To: Icenowy Zheng
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linus Walleij,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, Stephen Boyd,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Chen-Yu Tsai,
	linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <2714901484163893-/w1NKm4lK9luio3avFS2gg@public.gmane.org>

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

On Thu, Jan 12, 2017 at 03:44:53AM +0800, Icenowy Zheng wrote:
> 
> 
> 12.01.2017, 03:40, "Icenowy Zheng" <icenowy-ymACFijhrKM@public.gmane.org>:
> > 11.01.2017, 02:10, "Maxime Ripard" <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>:
> >>  On Tue, Jan 03, 2017 at 11:16:26PM +0800, Icenowy Zheng wrote:
> >>>   V3s has a similar but cut-down CCU to H3.
> >>>
> >>>   Add support for it.
> >>>
> >>>   Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> >>
> >>  It looks like there's nothing different but the clocks that you
> >>  register with the H3, please just use the H3 driver.
> >
> > Nope.
> >
> > It has a different PLL (PLL_ISP) at different address, and some
> > different muxes.
> 
> Forgot to mention the missing of PLL_DE and related misses.

Those are not conflicting, it's just a slightly different set of
clocks.

So there's really nothing undoable.

Maxime

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

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

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

^ permalink raw reply

* Re: [PATCH 3/4] drivers/misc: Add ASpeed LPC control driver
From: Greg KH @ 2017-01-12 17:27 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: Cyril Bur, devicetree-u79uwXL29TY76Z2rM5mHXA,
	jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w, arnd-r2nGTMty4D4,
	joel-U3u1mxZcP9KHXe+LvDLADg, mark.rutland-5wv7dgnIgG8,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, openbmc-uLR06cmDAlY/bJ5BZ2RsiQ,
	andrew-zrmu5oMJ5Fs, xow-hpIqsD4AKlfQT0dZR+AlfA,
	jk-mnsaURCQ41sdnm+yROfE0A
In-Reply-To: <1484238577.2492.45.camel-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>

On Thu, Jan 12, 2017 at 10:29:37AM -0600, Benjamin Herrenschmidt wrote:
> So I think the best approach here is:
> 
> 	- A pair of ioctls to read and write random registers in the
> LPC bridge for all the "generally configuration gunk". These have a
> filter to ensure that the registers controlling the above mapping
> cannot be accessed that way.
> 
> 	- An ioctl to control the above mapping window. It takes as
> arguments the location in LPC space, the window type (flash vs.
> memory), for memory, maybe an ID (several windows to chose from), and
> the offset& size in the latter. The driver can enforce that the windows
> are one of the specially reserved areas of memory etc...
> 
> 	- An mmap function to map those reserved windows into userspace
> so the daemon can communicate appropriately (only needed for the memory
> windows, the flash space is accessed via the normal /dev/mtd drivers)
> 
> Greg, does that make sense ?

Yes, that makes a lot more sense to me.  Thanks for writing it up,
hopefully it survives into the next driver submission, otherwise I'll
ask the same questions again due to not having a short-term memory at
all :)

thanks,

greg k-h
--
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] ARM: dts: sun8i-h3: Add dts for the Beelink X2 STB
From: Maxime Ripard @ 2017-01-12 17:28 UTC (permalink / raw)
  To: codekipper-Re5JQEeQqe8AvxtiuMwx3w
  Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-sunxi-/JYPxA39Uh5TLH3MbocFFw
In-Reply-To: <20170112171135.7822-1-codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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

Hi,

On Thu, Jan 12, 2017 at 06:11:35PM +0100, codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> From: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> The Beelink X2 is an STB based on the Allwinner H3 SoC with a uSD slot,
> 2 USB ports( 1 * USB-2 Host, 1 USB OTG), a 10/100M ethernet port using the
> SoC's integrated PHY, Wifi via a RTL8189ETV sdio wifi chip, HDMI, an IR
> receiver, 1 LEDs and an optical S/PDIF connector.
> 
> Signed-off-by: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  arch/arm/boot/dts/Makefile                |   1 +
>  arch/arm/boot/dts/sun8i-h3-beelink-x2.dts | 187 ++++++++++++++++++++++++++++++
>  2 files changed, 188 insertions(+)
>  create mode 100644 arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 78a94b747059..75960b1468a4 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -857,6 +857,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>  	sun8i-a83t-cubietruck-plus.dtb \
>  	sun8i-h2-plus-orangepi-zero.dtb \
>  	sun8i-h3-bananapi-m2-plus.dtb \
> +	sun8i-h3-beelink-x2.dtb \
>  	sun8i-h3-nanopi-m1.dtb	\
>  	sun8i-h3-nanopi-neo.dtb \
>  	sun8i-h3-orangepi-2.dtb \
> diff --git a/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts b/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
> new file mode 100644
> index 000000000000..bc5aed52cb1d
> --- /dev/null
> +++ b/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
> @@ -0,0 +1,187 @@
> +/*
> + * Copyright (C) 2017 Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + *  a) This file is free software; you can redistribute it and/or
> + *     modify it under the terms of the GNU General Public License as
> + *     published by the Free Software Foundation; either version 2 of the
> + *     License, or (at your option) any later version.
> + *
> + *     This file 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.
> + *
> + * Or, alternatively,
> + *
> + *  b) Permission is hereby granted, free of charge, to any person
> + *     obtaining a copy of this software and associated documentation
> + *     files (the "Software"), to deal in the Software without
> + *     restriction, including without limitation the rights to use,
> + *     copy, modify, merge, publish, distribute, sublicense, and/or
> + *     sell copies of the Software, and to permit persons to whom the
> + *     Software is furnished to do so, subject to the following
> + *     conditions:
> + *
> + *     The above copyright notice and this permission notice shall be
> + *     included in all copies or substantial portions of the Software.
> + *
> + *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + *     OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/dts-v1/;
> +#include "sun8i-h3.dtsi"
> +#include "sunxi-common-regulators.dtsi"
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/pinctrl/sun4i-a10.h>
> +
> +/ {
> +	model = "Beelink X2";
> +	compatible = "roofull,beelink-x2", "allwinner,sun8i-h3";
> +
> +	aliases {
> +		serial0 = &uart0;
> +		/* ethernet0 is the H3 emac, defined in sun8i-h3.dtsi */
> +		ethernet1 = &rtl8189ftv;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pwr_led_x2>;

Please drop all the pinctrl nodes for the GPIOs. They're useless, and
will be harmful when we'll switch to a stricter pinctrl driver.

> +
> +		pwr_led {
> +			label = "beelink-x2:red:pwr";
> +			gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>; /* PL10 */
> +			default-state = "on";
> +		};
> +	};
> +
> +	gpio_keys {
> +		compatible = "gpio-keys";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&sw_r_x2>;
> +
> +		sw4 {
> +			label = "power";
> +			linux,code = <BTN_0>;

Should that be BTN_0 then if the label is that one?

> +			gpios = <&r_pio 0 3 GPIO_ACTIVE_LOW>;
> +		};
> +	};
> +
> +	wifi_pwrseq: wifi_pwrseq {
> +		compatible = "mmc-pwrseq-simple";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&wifi_en_x2>;
> +		reset-gpios = <&r_pio 0 7 GPIO_ACTIVE_LOW>; /* PL7 */
> +	};
> +};
> +
> +&ehci1 {
> +	status = "okay";
> +};
> +
> +&ir {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&ir_pins_a>;
> +	status = "okay";
> +};
> +
> +&mmc0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin>;
> +	vmmc-supply = <&reg_vcc3v3>;
> +	bus-width = <4>;
> +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
> +	cd-inverted;
> +	status = "okay";
> +};
> +
> +&mmc1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc1_pins_a>;
> +	vmmc-supply = <&reg_vcc3v3>;
> +	bus-width = <4>;
> +	non-removable;
> +	status = "okay";
> +
> +	/*
> +	 * Explicitly define the sdio device, so that we can add an ethernet
> +	 * alias for it (which e.g. makes u-boot set a mac-address).
> +	 */
> +	rtl8189ftv: sdio_wifi@1 {
> +		reg = <1>;
> +	};
> +};
> +
> +&mmc2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc2_8bit_pins>;
> +	vmmc-supply = <&reg_vcc3v3>;
> +	bus-width = <8>;
> +	non-removable;
> +	cap-mmc-hw-reset;
> +	status = "okay";
> +};
> +
> +&mmc2_8bit_pins {
> +	/* Increase drive strength for DDR modes */
> +	drive-strength = <40>;

Have you actually tested that it was needed?

> +	/* eMMC is missing pull-ups */
> +	bias-pull-up;

This is already enabled in the DTSI.

Thanks!
Maxime

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

^ permalink raw reply

* Re: [PATCH 2/5] clk: sunxi-ng: add support for V3s CCU
From: Icenowy Zheng @ 2017-01-12 17:31 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: linux-kernel, linux-sunxi,
		linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
		linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Stephen Boyd, devicetree-u79uwXL29TY76Z2rM5mHXA, Linus Walleij,
		linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
		linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Chen-Yu Tsai


2017年1月13日 01:19于 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>写道:
>
> On Thu, Jan 12, 2017 at 03:44:53AM +0800, Icenowy Zheng wrote: 
> > 
> > 
> > 12.01.2017, 03:40, "Icenowy Zheng" <icenowy-ymACFijhrKM@public.gmane.org>: 
> > > 11.01.2017, 02:10, "Maxime Ripard" <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>: 
> > >>  On Tue, Jan 03, 2017 at 11:16:26PM +0800, Icenowy Zheng wrote: 
> > >>>   V3s has a similar but cut-down CCU to H3. 
> > >>> 
> > >>>   Add support for it. 
> > >>> 
> > >>>   Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org> 
> > >> 
> > >>  It looks like there's nothing different but the clocks that you 
> > >>  register with the H3, please just use the H3 driver. 
> > > 
> > > Nope. 
> > > 
> > > It has a different PLL (PLL_ISP) at different address, and some 
> > > different muxes. 
> > 
> > Forgot to mention the missing of PLL_DE and related misses. 
>
> Those are not conflicting, it's just a slightly different set of 
> clocks. 

If saying so, we can have only one ccu driver, and make every ccu register different set ;-)

V3s itself is a totally different SoC with H3.

The relationship of V3s and H3 can be farther than the relationship of A33 and H3😃

>
> So there's really nothing undoable. 
>
> Maxime 
>
> -- 
> Maxime Ripard, Free Electrons 
> Embedded Linux and Kernel engineering 
> http://free-electrons.com 

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

^ permalink raw reply

* Re: [PATCH] ARM: dts: sun8i-h3: Add dts for the Beelink X2 STB
From: Code Kipper @ 2017-01-12 17:39 UTC (permalink / raw)
  To: Maxime Ripard; +Cc: linux-arm-kernel, devicetree, linux-sunxi
In-Reply-To: <20170112172813.cbwqfpjlprkxdzcp@lukather>

On 12 January 2017 at 18:28, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi,
>
> On Thu, Jan 12, 2017 at 06:11:35PM +0100, codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
>> From: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>
>> The Beelink X2 is an STB based on the Allwinner H3 SoC with a uSD slot,
>> 2 USB ports( 1 * USB-2 Host, 1 USB OTG), a 10/100M ethernet port using the
>> SoC's integrated PHY, Wifi via a RTL8189ETV sdio wifi chip, HDMI, an IR
>> receiver, 1 LEDs and an optical S/PDIF connector.
>>
>> Signed-off-by: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/Makefile                |   1 +
>>  arch/arm/boot/dts/sun8i-h3-beelink-x2.dts | 187 ++++++++++++++++++++++++++++++
>>  2 files changed, 188 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index 78a94b747059..75960b1468a4 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -857,6 +857,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>>       sun8i-a83t-cubietruck-plus.dtb \
>>       sun8i-h2-plus-orangepi-zero.dtb \
>>       sun8i-h3-bananapi-m2-plus.dtb \
>> +     sun8i-h3-beelink-x2.dtb \
>>       sun8i-h3-nanopi-m1.dtb  \
>>       sun8i-h3-nanopi-neo.dtb \
>>       sun8i-h3-orangepi-2.dtb \
>> diff --git a/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts b/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
>> new file mode 100644
>> index 000000000000..bc5aed52cb1d
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
>> @@ -0,0 +1,187 @@
>> +/*
>> + * Copyright (C) 2017 Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> + *
>> + * This file is dual-licensed: you can use it either under the terms
>> + * of the GPL or the X11 license, at your option. Note that this dual
>> + * licensing only applies to this file, and not this project as a
>> + * whole.
>> + *
>> + *  a) This file is free software; you can redistribute it and/or
>> + *     modify it under the terms of the GNU General Public License as
>> + *     published by the Free Software Foundation; either version 2 of the
>> + *     License, or (at your option) any later version.
>> + *
>> + *     This file 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.
>> + *
>> + * Or, alternatively,
>> + *
>> + *  b) Permission is hereby granted, free of charge, to any person
>> + *     obtaining a copy of this software and associated documentation
>> + *     files (the "Software"), to deal in the Software without
>> + *     restriction, including without limitation the rights to use,
>> + *     copy, modify, merge, publish, distribute, sublicense, and/or
>> + *     sell copies of the Software, and to permit persons to whom the
>> + *     Software is furnished to do so, subject to the following
>> + *     conditions:
>> + *
>> + *     The above copyright notice and this permission notice shall be
>> + *     included in all copies or substantial portions of the Software.
>> + *
>> + *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> + *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
>> + *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>> + *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
>> + *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
>> + *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + *     OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +/dts-v1/;
>> +#include "sun8i-h3.dtsi"
>> +#include "sunxi-common-regulators.dtsi"
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/pinctrl/sun4i-a10.h>
>> +
>> +/ {
>> +     model = "Beelink X2";
>> +     compatible = "roofull,beelink-x2", "allwinner,sun8i-h3";
>> +
>> +     aliases {
>> +             serial0 = &uart0;
>> +             /* ethernet0 is the H3 emac, defined in sun8i-h3.dtsi */
>> +             ethernet1 = &rtl8189ftv;
>> +     };
>> +
>> +     chosen {
>> +             stdout-path = "serial0:115200n8";
>> +     };
>> +
>> +     leds {
>> +             compatible = "gpio-leds";
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&pwr_led_x2>;
>
> Please drop all the pinctrl nodes for the GPIOs. They're useless, and
> will be harmful when we'll switch to a stricter pinctrl driver.
>
ACK
>> +
>> +             pwr_led {
>> +                     label = "beelink-x2:red:pwr";
>> +                     gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>; /* PL10 */
>> +                     default-state = "on";
>> +             };
>> +     };
>> +
>> +     gpio_keys {
>> +             compatible = "gpio-keys";
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&sw_r_x2>;
>> +
>> +             sw4 {
>> +                     label = "power";
>> +                     linux,code = <BTN_0>;
>
> Should that be BTN_0 then if the label is that one?
>
ACK
>> +                     gpios = <&r_pio 0 3 GPIO_ACTIVE_LOW>;
>> +             };
>> +     };
>> +
>> +     wifi_pwrseq: wifi_pwrseq {
>> +             compatible = "mmc-pwrseq-simple";
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&wifi_en_x2>;
>> +             reset-gpios = <&r_pio 0 7 GPIO_ACTIVE_LOW>; /* PL7 */
>> +     };
>> +};
>> +
>> +&ehci1 {
>> +     status = "okay";
>> +};
>> +
>> +&ir {
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&ir_pins_a>;
>> +     status = "okay";
>> +};
>> +
>> +&mmc0 {
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin>;
>> +     vmmc-supply = <&reg_vcc3v3>;
>> +     bus-width = <4>;
>> +     cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
>> +     cd-inverted;
>> +     status = "okay";
>> +};
>> +
>> +&mmc1 {
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&mmc1_pins_a>;
>> +     vmmc-supply = <&reg_vcc3v3>;
>> +     bus-width = <4>;
>> +     non-removable;
>> +     status = "okay";
>> +
>> +     /*
>> +      * Explicitly define the sdio device, so that we can add an ethernet
>> +      * alias for it (which e.g. makes u-boot set a mac-address).
>> +      */
>> +     rtl8189ftv: sdio_wifi@1 {
>> +             reg = <1>;
>> +     };
>> +};
>> +
>> +&mmc2 {
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&mmc2_8bit_pins>;
>> +     vmmc-supply = <&reg_vcc3v3>;
>> +     bus-width = <8>;
>> +     non-removable;
>> +     cap-mmc-hw-reset;
>> +     status = "okay";
>> +};
>> +
>> +&mmc2_8bit_pins {
>> +     /* Increase drive strength for DDR modes */
>> +     drive-strength = <40>;
>
> Have you actually tested that it was needed?
>
I'll confess that I've rushed this one as I just wanted to verify my
spdif changes. I'll respin this,
BR,
CK
>> +     /* eMMC is missing pull-ups */
>> +     bias-pull-up;
>
> This is already enabled in the DTSI.
>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

^ permalink raw reply

* Re: [PATCH 4/4] ARM: dts: sun8i: add OTG function to Lichee Pi Zero
From: Maxime Ripard @ 2017-01-12 17:39 UTC (permalink / raw)
  To: Bin Liu, Icenowy Zheng, Chen-Yu Tsai, Kishon Vijay Abraham I,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
In-Reply-To: <20170112145014.GC16865@uda0271908>


[-- Attachment #1.1: Type: text/plain, Size: 3329 bytes --]

Hi Bin,

On Thu, Jan 12, 2017 at 08:50:14AM -0600, Bin Liu wrote:
> On Wed, Jan 11, 2017 at 10:06:38PM +0100, Maxime Ripard wrote:
> > On Wed, Jan 11, 2017 at 02:08:11PM -0600, Bin Liu wrote:
> > > On Thu, Jan 12, 2017 at 03:55:33AM +0800, Icenowy Zheng wrote:
> > > > 
> > > > 
> > > > 11.01.2017, 04:24, "Bin Liu" <b-liu@ti.com>:
> > > > > On Tue, Jan 03, 2017 at 11:25:34PM +0800, Icenowy Zheng wrote:
> > > > >>  Lichee Pi Zero features a USB OTG port.
> > > > >>
> > > > >>  Add support for it.
> > > > >>
> > > > >>  Note: in order to use the Host mode, the board must be powered via the
> > > > >>  +5V and GND pins.
> > > > >>
> > > > >>  Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> > > > >>  ---
> > > > >>   arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts | 10 ++++++++++
> > > > >>   1 file changed, 10 insertions(+)
> > > > >>
> > > > >>  diff --git a/arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts b/arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts
> > > > >>  index 0099affc6ce3..3d9168cbaeca 100644
> > > > >>  --- a/arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts
> > > > >>  +++ b/arch/arm/boot/dts/sun8i-v3s-licheepi-zero.dts
> > > > >>  @@ -71,3 +71,13 @@
> > > > >>           pinctrl-names = "default";
> > > > >>           status = "okay";
> > > > >>   };
> > > > >>  +
> > > > >>  +&usb_otg {
> > > > >>  + dr_mode = "otg";
> > > > >
> > > > > Why not set this default mode in dtsi instead?
> > > > >
> > > > > Regards,
> > > > > -Bin.
> > > > 
> > > > There's possibly boards which do not have OTG functions.
> > > 
> > > That is board specific.
> > 
> > Exactly, and this is why it should be done in the board DT.
> 
> I am just suggesting based on the common practice. If a .dtsi exists for
> a family, the .dtsi describes the device and common properties for all
> possible boards, and each board .dts adds or overrides its specific
> implementation. Kernel has many devices/boards done in this way - define
> the default dr_mode in .dtsi.
> 
> In this case, I suggest to set the common dr_mode in .dtsi, then each
> board .dts only overrides it if the implementation is different. 
> 
> > 
> > The controller in the Allwinner SoCs do not handle directly the ID pin
> > and VBUS, but rather rely on a GPIO to do so.
> > 
> > So boards with OTG will need setup anyway, at least to tell which
> > GPIOs are used. There's no point in enforcing a default if it doesn't
> > work by default.
> 
> Then define a default which supposes to work for most boards.
> 
> Why I suggest this, is because defining a default dr_mode which works
> for most cases in dtsi could prevent a little surprise in MUSB function.
> If someone designs a new board but forgets to define dr_mode in the new
> board DT, the MUSB driver will default to org mode, which might not be
> intended.

The point is that there is no sensible default. Some boards don't have
an ID pin and no VBUS (peripheral), some don't have an ID pin but VBUS
(host), and some have an ID pin but no controllable VBUS, some have an
ID pin and a controllable VBUS, but we have no idea which GPIOs are
used.

There's no way we can have something that works on most cases.

Maxime

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

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

^ permalink raw reply

* Re: [PATCH v2] ARM64: dts: meson-gxbb-odroidc2: Disable SCPI DVFS
From: Martin Blumenstingl @ 2017-01-12 17:40 UTC (permalink / raw)
  To: Neil Armstrong, brian.kim
  Cc: khilman, carlo, linux-amlogic, devicetree, linux-kernel,
	linux-arm-kernel, moon.linux
In-Reply-To: <1483689872-30389-1-git-send-email-narmstrong@baylibre.com>

Hi Neil,

(adding Brian Kim, one of the Hardkernel developers to this conversation)

On Fri, Jan 6, 2017 at 9:04 AM, Neil Armstrong <narmstrong@baylibre.com> wrote:
> The current hardware is not able to run with all cores enabled at a
> cluster frequency superior at 1536MHz.
> But the currently shipped u-boot for the platform still reports an OPP
> table with possible DVFS frequency up to 2GHz, and will not change since
> the off-tree linux tree supports limiting the OPPs with a kernel parameter.
> A recent u-boot change reports the boot-time DVFS around 100MHz and
> the default performance cpufreq governor sets the maximum frequency.
> Previous version of u-boot reported to be already at the max OPP and
> left the OPP as is.
> Nevertheless, other governors like ondemand could setup the max frequency
> and make the system crash.
>
> This patch disables the DVFS clock and disables cpufreq.
I don't have any Odroid-C2 board, but having to live without cpufreq
sounds bad for the Odroid-C2 users.
What would we expect from a kernel perspective (maybe the Hardkernel
guys would adjust their u-boot instead of us adjusting to the behavior
of one specific device? one solution that I could think of involves
the "maxcpus" kernel parameter (see [0]), if this is not set u-boot
should report a max CPU frequency of 1536MHz (= max frequency for 4
active cores). Based on the "maxcpus" value additional frequencies can
be unlocked (this could be step-by-step if there are different
frequencies for one core/two cores/etc.). However, I'd like to hear
other opinions as well.


Regards,
Martin


[0] http://lxr.free-electrons.com/source/Documentation/kernel-parameters.txt?v=4.8#L2163

^ permalink raw reply

* Re: [PATCH 2/5] clk: sunxi-ng: add support for V3s CCU
From: Maxime Ripard @ 2017-01-12 17:41 UTC (permalink / raw)
  To: Icenowy Zheng
  Cc: linux-kernel, linux-sunxi,
		linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
		linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	Stephen Boyd, devicetree-u79uwXL29TY76Z2rM5mHXA, Linus Walleij,
		linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
		linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Chen-Yu Tsai
In-Reply-To: <20170112203149.VhtuR9dM-fU7thrOZ3Cs0PDqKvflMoHmW9unr2Ajn@public.gmane.org>

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

On Fri, Jan 13, 2017 at 01:31:41AM +0800, Icenowy Zheng wrote:
> 
> 2017年1月13日 01:19于 Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>写道:
> >
> > On Thu, Jan 12, 2017 at 03:44:53AM +0800, Icenowy Zheng wrote: 
> > > 
> > > 
> > > 12.01.2017, 03:40, "Icenowy Zheng" <icenowy-ymACFijhrKM@public.gmane.org>: 
> > > > 11.01.2017, 02:10, "Maxime Ripard" <maxime.ripard-wi1+55ScJUtKEb57/3fJTGGXanvQGlWp@public.gmane.orgm>: 
> > > >>  On Tue, Jan 03, 2017 at 11:16:26PM +0800, Icenowy Zheng wrote: 
> > > >>>   V3s has a similar but cut-down CCU to H3. 
> > > >>> 
> > > >>>   Add support for it. 
> > > >>> 
> > > >>>   Signed-off-by: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org> 
> > > >> 
> > > >>  It looks like there's nothing different but the clocks that you 
> > > >>  register with the H3, please just use the H3 driver. 
> > > > 
> > > > Nope. 
> > > > 
> > > > It has a different PLL (PLL_ISP) at different address, and some 
> > > > different muxes. 
> > > 
> > > Forgot to mention the missing of PLL_DE and related misses. 
> >
> > Those are not conflicting, it's just a slightly different set of 
> > clocks. 
> 
> If saying so, we can have only one ccu driver, and make every ccu
> register different set ;-)
> 
> V3s itself is a totally different SoC with H3.
> 
> The relationship of V3s and H3 can be farther than the relationship
> of A33 and H3😃

A33 and H3 are an entirely different story. The H3 and A33 have
conflicting clocks (ie same clocks with different parameters). This is
not your case.

Maxime

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

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

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

^ permalink raw reply

* Re: [PATCH v2 2/2] Support for DW CSI-2 Host IPK
From: Ramiro Oliveira @ 2017-01-12 17:43 UTC (permalink / raw)
  To: Hans Verkuil, Ramiro Oliveira, robh+dt, mark.rutland, mchehab,
	devicetree, linux-kernel, linux-media
  Cc: davem, gregkh, geert+renesas, akpm, linux,
	laurent.pinchart+renesas, arnd, sudipm.mukherjee, tiffany.lin,
	minghsiu.tsai, jean-christophe.trotin, andrew-ct.chen,
	simon.horman, songjun.wu, bparrot, CARLOS.PALMINHA, Sakari Ailus
In-Reply-To: <eb89af79-f868-ceba-ac69-558bac77613d@xs4all.nl>

Hi Hans,

Thank you for your feedback.

On 1/11/2017 11:54 AM, Hans Verkuil wrote:
> Hi Ramiro,
> 
> See my review comments below:
> 
> On 12/12/16 16:00, Ramiro Oliveira wrote:
>> Add support for the DesignWare CSI-2 Host IP Prototyping Kit
>>
>> Signed-off-by: Ramiro Oliveira <roliveir@synopsys.com>

[snip]

>> +static int
>> +dw_mipi_csi_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg,
>> +            struct v4l2_subdev_format *fmt)
>> +{
>> +    struct mipi_csi_dev *dev = sd_to_mipi_csi_dev(sd);
>> +    struct mipi_fmt const *dev_fmt;
>> +    struct v4l2_mbus_framefmt *mf;
>> +    unsigned int i = 0;
>> +    const struct v4l2_bt_timings *bt_r = &v4l2_dv_timings_presets[0].bt;
>> +
>> +    mf = __dw_mipi_csi_get_format(dev, cfg, fmt->which);
>> +
>> +    dev_fmt = dw_mipi_csi_try_format(&fmt->format);
>> +    if (dev_fmt) {
>> +        *mf = fmt->format;
>> +        if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>> +            dev->fmt = dev_fmt;
>> +        dw_mipi_csi_set_ipi_fmt(dev);
>> +    }
>> +    while (v4l2_dv_timings_presets[i].bt.width) {
>> +        const struct v4l2_bt_timings *bt =
>> +            &v4l2_dv_timings_presets[i].bt;
>> +        if (mf->width == bt->width && mf->height == bt->width) {
>> +            __dw_mipi_csi_fill_timings(dev, bt);
>> +            return 0;
>> +        }
>> +        i++;
>> +    }
>> +
>> +    __dw_mipi_csi_fill_timings(dev, bt_r);
> 
> This code is weird. The video source can be either from a sensor or from an
> HDMI input, right?
> 
> But if it is from a sensor, then using v4l2_dv_timings_presets since that's for
> an HDMI input. Sensors will typically not follow these preset timings.
> 
> For HDMI input I expect that this driver supports the s_dv_timings op and will
> just use the timings set there and override the width/height in v4l2_subdev_format.
> 
> For sensors I am actually not quite certain how this is done. I've CC-ed Sakari
> since he'll know. But let us know first whether it is indeed the intention that
> this should also work with a sensor.
> 

Actually the video source, at the moment, can only be from a sensor. I'm using
v4l2_dv_timings_presets as a reference since we usually use this setup with a
Test Equipment in which we can configure every parameter.

I'll wait for Sakari to answer, and change it to what he recommends.

>> +    return 0;
>> +
>> +}
>> +
>> +static int
>> +dw_mipi_csi_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_pad_config *cfg,
>> +            struct v4l2_subdev_format *fmt)
>> +{
>> +    struct mipi_csi_dev *dev = sd_to_mipi_csi_dev(sd);
>> +    struct v4l2_mbus_framefmt *mf;
>> +
>> +    mf = __dw_mipi_csi_get_format(dev, cfg, fmt->which);
>> +    if (!mf)
>> +        return -EINVAL;
>> +
>> +    mutex_lock(&dev->lock);
>> +    fmt->format = *mf;
>> +    mutex_unlock(&dev->lock);
>> +    return 0;
>> +}
>> +
>> +static int
>> +dw_mipi_csi_s_power(struct v4l2_subdev *sd, int on)
>> +{
>> +    struct mipi_csi_dev *dev = sd_to_mipi_csi_dev(sd);
>> +
>> +    if (on) {
>> +        dw_mipi_csi_hw_stdby(dev);
>> +        dw_mipi_csi_start(dev);
>> +    } else {
>> +        dw_mipi_csi_mask_irq_power_off(dev);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int
>> +dw_mipi_csi_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>> +{
>> +    struct v4l2_mbus_framefmt *format =
>> +        v4l2_subdev_get_try_format(sd, fh->pad, 0);
>> +
>> +    format->colorspace = V4L2_COLORSPACE_SRGB;
>> +    format->code = dw_mipi_csi_formats[0].code;
>> +    format->width = MIN_WIDTH;
>> +    format->height = MIN_HEIGHT;
>> +    format->field = V4L2_FIELD_NONE;
> 
> Don't do this. Instead implement the init_cfg pad op and initialize this there.
> 
> You can then drop this function.
> 

I'll do that.

>> +
>> +    return 0;
>> +}
>> +
>> +static const struct v4l2_subdev_internal_ops dw_mipi_csi_sd_internal_ops = {
>> +    .open = dw_mipi_csi_open,
>> +};
>> +
>> +static struct v4l2_subdev_core_ops dw_mipi_csi_core_ops = {
>> +    .s_power = dw_mipi_csi_s_power,
>> +};
>> +
>> +static struct v4l2_subdev_pad_ops dw_mipi_csi_pad_ops = {
>> +    .enum_mbus_code = dw_mipi_csi_enum_mbus_code,
>> +    .get_fmt = dw_mipi_csi_get_fmt,
>> +    .set_fmt = dw_mipi_csi_set_fmt,
>> +};
>> +
>> +static struct v4l2_subdev_ops dw_mipi_csi_subdev_ops = {
>> +    .core = &dw_mipi_csi_core_ops,
>> +    .pad = &dw_mipi_csi_pad_ops,
>> +};
>> +
>> +static irqreturn_t
>> +dw_mipi_csi_irq1(int irq, void *dev_id)
>> +{
>> +    struct mipi_csi_dev *csi_dev = dev_id;
>> +    u32 global_int_status, i_sts;
>> +    unsigned long flags;
>> +    struct device *dev = &csi_dev->pdev->dev;
>> +
>> +    global_int_status = dw_mipi_csi_read(csi_dev, R_CSI2_INTERRUPT);
>> +    spin_lock_irqsave(&csi_dev->slock, flags);
>> +
>> +    if (global_int_status & CSI2_INT_PHY_FATAL) {
>> +        i_sts = dw_mipi_csi_read(csi_dev, R_CSI2_INT_PHY_FATAL);
>> +        dev_dbg_ratelimited(dev, "CSI INT PHY FATAL: %08X\n", i_sts);
>> +    }
>> +
>> +    if (global_int_status & CSI2_INT_PKT_FATAL) {
>> +        i_sts = dw_mipi_csi_read(csi_dev, R_CSI2_INT_PKT_FATAL);
>> +        dev_dbg_ratelimited(dev, "CSI INT PKT FATAL: %08X\n", i_sts);
>> +    }
>> +
>> +    if (global_int_status & CSI2_INT_FRAME_FATAL) {
>> +        i_sts = dw_mipi_csi_read(csi_dev, R_CSI2_INT_FRAME_FATAL);
>> +        dev_dbg_ratelimited(dev, "CSI INT FRAME FATAL: %08X\n", i_sts);
>> +    }
>> +
>> +    if (global_int_status & CSI2_INT_PHY) {
>> +        i_sts = dw_mipi_csi_read(csi_dev, R_CSI2_INT_PHY);
>> +        dev_dbg_ratelimited(dev, "CSI INT PHY: %08X\n", i_sts);
>> +    }
>> +
>> +    if (global_int_status & CSI2_INT_PKT) {
>> +        i_sts = dw_mipi_csi_read(csi_dev, R_CSI2_INT_PKT);
>> +        dev_dbg_ratelimited(dev, "CSI INT PKT: %08X\n", i_sts);
>> +    }
>> +
>> +    if (global_int_status & CSI2_INT_LINE) {
>> +        i_sts = dw_mipi_csi_read(csi_dev, R_CSI2_INT_LINE);
>> +        dev_dbg_ratelimited(dev, "CSI INT LINE: %08X\n", i_sts);
>> +    }
>> +
>> +    if (global_int_status & CSI2_INT_IPI) {
>> +        i_sts = dw_mipi_csi_read(csi_dev, R_CSI2_INT_IPI);
>> +        dev_dbg_ratelimited(dev, "CSI INT IPI: %08X\n", i_sts);
>> +    }
>> +    spin_unlock_irqrestore(&csi_dev->slock, flags);
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +static int
>> +dw_mipi_csi_parse_dt(struct platform_device *pdev, struct mipi_csi_dev *dev)
>> +{
>> +    struct device_node *node = pdev->dev.of_node;
>> +    int reg;
>> +    int ret = 0;
>> +
>> +    /* Device tree information */
> 
> I would expect to see a call to v4l2_of_parse_endpoint here.
> 

You're right. I'll add it.

>> +    ret = of_property_read_u32(node, "data-lanes", &dev->hw.num_lanes);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Couldn't read data-lanes\n");
>> +        return ret;
>> +    }
>> +
>> +    ret = of_property_read_u32(node, "output-type", &dev->hw.output_type);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Couldn't read output-type\n");
>> +        return ret;
>> +    }
>> +
>> +    ret = of_property_read_u32(node, "ipi-mode", &dev->hw.ipi_mode);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Couldn't read ipi-mode\n");
>> +        return ret;
>> +    }
>> +
>> +    ret =
>> +        of_property_read_u32(node, "ipi-auto-flush",
>> +                 &dev->hw.ipi_auto_flush);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Couldn't read ipi-auto-flush\n");
>> +        return ret;
>> +    }
>> +
>> +    ret =
>> +        of_property_read_u32(node, "ipi-color-mode",
>> +                 &dev->hw.ipi_color_mode);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Couldn't read ipi-color-mode\n");
>> +        return ret;
>> +    }
>> +
>> +    ret =
>> +        of_property_read_u32(node, "virtual-channel", &dev->hw.virtual_ch);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Couldn't read virtual-channel\n");
>> +        return ret;
>> +    }
>> +
>> +    node = of_get_child_by_name(node, "port");
>> +    if (!node)
>> +        return -EINVAL;
>> +
>> +    ret = of_property_read_u32(node, "reg", &reg);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Couldn't read reg value\n");
>> +        return ret;
>> +    }
>> +    dev->index = reg - 1;
>> +
>> +    if (dev->index >= CSI_MAX_ENTITIES)
>> +        return -ENXIO;
>> +
>> +    return 0;
>> +}
>> +

[snip]

>> diff --git a/drivers/media/platform/dwc/plat_ipk.c
>> b/drivers/media/platform/dwc/plat_ipk.c
>> new file mode 100644
>> index 0000000..02dcf36
>> --- /dev/null
>> +++ b/drivers/media/platform/dwc/plat_ipk.c
>> @@ -0,0 +1,818 @@
>> +/**
>> + * DWC MIPI CSI-2 Host IPK platform device driver
> 
> What does IPK stand for?
> 

IPK stands for IP Prototyping Kit. However any reference to this will probably
disappear in the next patchset.

[snip]


>> +
>> +static const struct plat_ipk_fmt *
>> +vid_dev_find_format(struct v4l2_format *f, int index)
>> +{
>> +    const struct plat_ipk_fmt *fmt = NULL;
>> +    unsigned int i;
>> +
>> +    if (index >= (int) ARRAY_SIZE(vid_dev_formats))
>> +        return NULL;
> 
> ???
> 
> What's the purpose of the index argument? I get the feeling it is
> a left-over from older code.
> 

Yes. It's a left-over. I'll remove it.

>> +
>> +    for (i = 0; i < ARRAY_SIZE(vid_dev_formats); ++i) {
>> +        fmt = &vid_dev_formats[i];
>> +        if (fmt->fourcc == f->fmt.pix.pixelformat)
>> +            return fmt;
>> +    }
>> +    return NULL;
>> +}
>> +
>> +/*
>> + * Video node ioctl operations
>> + */
>> +static int
>> +vidioc_querycap(struct file *file, void *priv, struct v4l2_capability *cap)
>> +{
>> +    struct video_device_dev *vid_dev = video_drvdata(file);
>> +
>> +    strlcpy(cap->driver, VIDEO_DEVICE_NAME, sizeof(cap->driver));
>> +    strlcpy(cap->card, VIDEO_DEVICE_NAME, sizeof(cap->card));
>> +    snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
>> +         dev_name(&vid_dev->pdev->dev));
>> +
>> +    cap->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
>> +    cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;
> 
> Set the device_caps in struct video_device and drop these two lines.
> The core will fill those in for you.
> 

I'll change them to where I configure the struct video_device.

>> +    return 0;
>> +}
>> +
>> +static int
>> +vidioc_enum_fmt_vid_cap(struct file *file, void *priv, struct v4l2_fmtdesc *f)
>> +{
>> +    const struct plat_ipk_fmt *p_fmt;
>> +
>> +    if (f->index >= ARRAY_SIZE(vid_dev_formats))
>> +        return -EINVAL;
>> +
>> +    p_fmt = &vid_dev_formats[f->index];
>> +
>> +    strlcpy(f->description, p_fmt->name, sizeof(f->description));
> 
> Don't set the description, the core will do that for you.
> 

OK.

>> +    f->pixelformat = p_fmt->fourcc;
>> +
>> +    return 0;
>> +}
>> +
>> +static int vidioc_g_fmt_vid_cap(struct file *file, void *priv,
>> +                    struct v4l2_format *f)
>> +{
>> +    struct video_device_dev *dev = video_drvdata(file);
>> +
>> +    memcpy(&f->fmt.pix, &dev->format.fmt.pix,
>> +           sizeof(struct v4l2_pix_format));
> 
> Use f->fmt.pix = dev->format.fmt.pix;
> 

I'll do that

>> +
>> +    return 0;
>> +}
>> +
>> +static int
>> +vidioc_try_fmt_vid_cap(struct file *file, void *priv, struct v4l2_format *f)
>> +{
>> +    const struct plat_ipk_fmt *fmt;
>> +
>> +    fmt = vid_dev_find_format(f, -1);
>> +    if (!fmt) {
>> +        f->fmt.pix.pixelformat = V4L2_PIX_FMT_RGB565;
>> +        fmt = vid_dev_find_format(f, -1);
>> +    }
>> +
>> +    f->fmt.pix.field = V4L2_FIELD_NONE;
>> +    v4l_bound_align_image(&f->fmt.pix.width, 48, MAX_WIDTH, 2,
>> +                  &f->fmt.pix.height, 32, MAX_HEIGHT, 0, 0);
>> +
>> +    f->fmt.pix.bytesperline = (f->fmt.pix.width * fmt->depth) >> 3;
>> +    f->fmt.pix.sizeimage = f->fmt.pix.height * f->fmt.pix.bytesperline;
>> +    f->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
>> +    return 0;
>> +}
>> +
>> +static int vidioc_s_fmt_vid_cap(struct file *file, void *priv,
>> +                    struct v4l2_format *f)
>> +{
>> +    struct video_device_dev *dev = video_drvdata(file);
>> +    int ret;
>> +    struct v4l2_subdev_format fmt;
>> +    struct v4l2_pix_format *dev_fmt_pix = &dev->format.fmt.pix;
>> +
>> +    if (vb2_is_busy(&dev->vb_queue))
>> +        return -EBUSY;
>> +
>> +    ret = vidioc_try_fmt_vid_cap(file, dev, f);
>> +    if (ret)
>> +        return ret;
>> +
>> +    dev->fmt = vid_dev_find_format(f, -1);
>> +    dev_fmt_pix->pixelformat = f->fmt.pix.pixelformat;
>> +    dev_fmt_pix->width = f->fmt.pix.width;
>> +    dev_fmt_pix->height  = f->fmt.pix.height;
>> +    dev_fmt_pix->bytesperline = dev_fmt_pix->width * (dev->fmt->depth / 8);
>> +    dev_fmt_pix->sizeimage =
>> +            dev_fmt_pix->height * dev_fmt_pix->bytesperline;
>> +
>> +    fmt.format.colorspace = V4L2_COLORSPACE_SRGB;
>> +    fmt.format.code = dev->fmt->mbus_code;
>> +
>> +    fmt.format.width = dev_fmt_pix->width;
>> +    fmt.format.height = dev_fmt_pix->height;
>> +
>> +    ret = plat_ipk_pipeline_call(&dev->ve, set_format, &fmt);
>> +
>> +    return 0;
>> +}
>> +
>> +static int vidioc_enum_framesizes(struct file *file, void *fh,
>> +               struct v4l2_frmsizeenum *fsize)
>> +{
>> +    static const struct v4l2_frmsize_stepwise sizes = {
>> +        48, MAX_WIDTH, 4,
>> +        32, MAX_HEIGHT, 1
>> +    };
>> +    int i;
>> +
>> +    if (fsize->index)
>> +        return -EINVAL;
>> +    for (i = 0; i < ARRAY_SIZE(vid_dev_formats); i++)
>> +        if (vid_dev_formats[i].fourcc == fsize->pixel_format)
>> +            break;
>> +    if (i == ARRAY_SIZE(vid_dev_formats))
>> +        return -EINVAL;
>> +    fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
>> +    fsize->stepwise = sizes;
>> +    return 0;
>> +}
>> +
>> +static int vidioc_enum_input(struct file *file, void *priv,
>> +            struct v4l2_input *input)
>> +{
>> +    if (input->index != 0)
>> +        return -EINVAL;
>> +
>> +    input->type = V4L2_INPUT_TYPE_CAMERA;
>> +    input->std = V4L2_STD_ALL;    /* Not sure what should go here */
> 
> Set this to 0, or just drop the line.
> 

Thanks.

>> +    strcpy(input->name, "Camera");
>> +    return 0;
>> +}
>> +

[snip]

>> +
>> +static int vid_dev_subdev_s_power(struct v4l2_subdev *sd, int on)
>> +{
>> +    return 0;
>> +}
> 
> Just drop this empty function, shouldn't be needed.
> 

When I start my system I'm hoping all the subdevs have s_power registered. If it
doesn't exist should I change the way I handle it, or will the core handle it
for me?

>> +
>> +static int vid_dev_subdev_registered(struct v4l2_subdev *sd)
>> +{
>> +    struct video_device_dev *vid_dev = v4l2_get_subdevdata(sd);
>> +    struct vb2_queue *q = &vid_dev->vb_queue;
>> +    struct video_device *vfd = &vid_dev->ve.vdev;
>> +    int ret;
>> +
>> +    memset(vfd, 0, sizeof(*vfd));
>> +
>> +    strlcpy(vfd->name, VIDEO_DEVICE_NAME, sizeof(vfd->name));
>> +
>> +    vfd->fops = &vid_dev_fops;
>> +    vfd->ioctl_ops = &vid_dev_ioctl_ops;
>> +    vfd->v4l2_dev = sd->v4l2_dev;
>> +    vfd->minor = -1;
>> +    vfd->release = video_device_release_empty;
>> +    vfd->queue = q;
>> +
>> +    INIT_LIST_HEAD(&vid_dev->vidq.active);
>> +    init_waitqueue_head(&vid_dev->vidq.wq);
>> +    memset(q, 0, sizeof(*q));
>> +    q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +    q->io_modes = VB2_MMAP | VB2_USERPTR;
> 
> Add VB2_DMABUF and VB2_READ.
> 

I'll add them, but I'm not using them, is it standard procedure to add them all
even if they aren't used?

>> +    q->ops = &vb2_video_qops;
>> +    q->mem_ops = &vb2_vmalloc_memops;
> 
> Why is vmalloc used? Can't you use dma_contig or dma_sg and avoid having to copy
> the image data? That's a really bad design given the amount of video data that
> you have to copy.
> 

When I started development, the arch I was using (ARC) didn't support
dma_contig, so I was forced to use vmalloc.

Since then things have changed and I'm already using dma_contig, however it
wasn't included in this patch. I'll add it to the next patch.

>> +    q->buf_struct_size = sizeof(struct rx_buffer);
>> +    q->drv_priv = vid_dev;
>> +    q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>> +    q->lock = &vid_dev->lock;
>> +
>> +    ret = vb2_queue_init(q);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    vid_dev->vd_pad.flags = MEDIA_PAD_FL_SINK;
>> +    ret = media_entity_pads_init(&vfd->entity, 1, &vid_dev->vd_pad);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    video_set_drvdata(vfd, vid_dev);
>> +    vid_dev->ve.pipe = v4l2_get_subdev_hostdata(sd);
>> +
>> +    ret = video_register_device(vfd, VFL_TYPE_GRABBER, -1);
>> +    if (ret < 0) {
>> +        media_entity_cleanup(&vfd->entity);
>> +        vid_dev->ve.pipe = NULL;
>> +        return ret;
>> +    }
>> +
>> +    v4l2_info(sd->v4l2_dev, "Registered %s as /dev/%s\n",
>> +          vfd->name, video_device_node_name(vfd));
>> +    return 0;
>> +}
>> +

[snip]

> 
> Regards,
> 
>     Hans

BRs,
Ramiro

^ permalink raw reply

* Re: [PATCH v8 2/5] i2c: Add STM32F4 I2C driver
From: Uwe Kleine-König @ 2017-01-12 17:49 UTC (permalink / raw)
  To: M'boumba Cedric Madianga
  Cc: devicetree, Alexandre Torgue, Wolfram Sang, linux-kernel,
	Linus Walleij, Patrice Chotard, Russell King, Rob Herring,
	linux-i2c, Maxime Coquelin, linux-arm-kernel
In-Reply-To: <CAOAejn289GLOSP-nPJnO_VpXLpyhTsF1bWQ7Ns9OfgPQCa8YTw@mail.gmail.com>

On Thu, Jan 12, 2017 at 02:47:42PM +0100, M'boumba Cedric Madianga wrote:
> 2017-01-12 13:03 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > Hello Cedric,
> >
> > On Thu, Jan 12, 2017 at 12:23:12PM +0100, M'boumba Cedric Madianga wrote:
> >> 2017-01-11 16:39 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> > On Wed, Jan 11, 2017 at 02:58:44PM +0100, M'boumba Cedric Madianga wrote:
> >> >> 2017-01-11 9:22 GMT+01:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >> >> > This is surprising. I didn't recheck the manual, but that looks very
> >> >> > uncomfortable.
> >> >>
> >> >> I agree but this exactly the hardware way of working described in the
> >> >> reference manual.
> >> >
> >> > IMHO that's a hw bug. This makes it for example impossible to implement
> >> > SMBus block transfers (I think).
> >>
> >> This is not correct.
> >> Setting STOP/START bit does not mean the the pulse will be sent right now.
> >> Here we have just to prepare the hardware for the 2 next pulse but the
> >> STOP/START/ACK pulse will be generated at the right time as required
> >> by I2C specification.
> >> So SMBus block transfer will be possible.
> >
> > A block transfer consists of a byte that specifies the count of bytes
> > yet to come. So the device sends for example:
> >
> >         0x01 0xab
> >
> > So when you read the 1 in the first byte it's already too late to set
> > STOP to get it after the 2nd byte.
> >
> > Not sure I got all the required details right, though.
> 
> Ok I understand your use case but I always think that the harware manages it.
> If I take the above example, the I2C SMBus block read transaction will
> be as below:
> S Addr Wr [A] Comm [A]
>            S Addr Rd [A] [Count] A [Data1] A [Data2] NA P
> 
> The first message is a single byte-transmission so there is no problem.
> 
> The second message is a N-byte reception with N = 3
> 
> When the I2C controller has finished to send the device address (S
> Addr Rd), the ADDR flag is set and an interrupt is raised.
> In the routine that handles ADDR event, we set ACK bit in order to
> generate ACK pulse as soon as a data byte is received in the shift
> register and then we clear the ADDR flag.
> Please note that the SCL line is stretched low until ADDR flag is cleared.
> So, as far I understand, the device could not sent any data as long as
> the SCL line is stretched low. Right ?
> 
> Then, as soon as the SCL line is high, the device could send the first
> data byte (Count).
> When this byte is received in the shift register, an ACK is
> automatically generated as defined during adress match phase and the
> data byte is pushed in DR (data register).
> Then, an interrupt is raised as RXNE (RX not empty) flag is set.
> In the routine that handles RXNE event, as N=3, we just clear all
> buffer interrupts in order to avoid another system preemption due to
> RXNE event but we does not read the data in DR.

In my example I want to receive a block of length 1, so only two bytes
are read, a 1 (the length) and the data byte (0xab in my example). I
think that as soon as you read the 1 it's already to late to schedule
the NA after the next byte?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply

* [PATCH v5 1/3] Bluetooth: btusb: Use an error label for error paths
From: Rajat Jain @ 2017-01-12 18:01 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Marcel Holtmann, Gustavo Padovan,
	Johan Hedberg, Amitkumar Karwar, Wei-Ning Huang, Xinming Hu,
	netdev, devicetree, linux-bluetooth, Brian Norris, linux-kernel
  Cc: Rajat Jain, rajatxjain

Use a label to remove the repetetive cleanup, for error cases.

Signed-off-by: Rajat Jain <rajatja@google.com>
Reviewed-by: Brian Norris <briannorris@chromium.org>
---
v5: same as v4
v4: same as v3
v3: Added Brian's "Reviewed-by"
v2: same as v1

 drivers/bluetooth/btusb.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 2f633df9f4e6..ce22cefceed1 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2991,18 +2991,15 @@ static int btusb_probe(struct usb_interface *intf,
 		err = usb_set_interface(data->udev, 0, 0);
 		if (err < 0) {
 			BT_ERR("failed to set interface 0, alt 0 %d", err);
-			hci_free_dev(hdev);
-			return err;
+			goto out_free_dev;
 		}
 	}
 
 	if (data->isoc) {
 		err = usb_driver_claim_interface(&btusb_driver,
 						 data->isoc, data);
-		if (err < 0) {
-			hci_free_dev(hdev);
-			return err;
-		}
+		if (err < 0)
+			goto out_free_dev;
 	}
 
 #ifdef CONFIG_BT_HCIBTUSB_BCM
@@ -3016,14 +3013,16 @@ static int btusb_probe(struct usb_interface *intf,
 #endif
 
 	err = hci_register_dev(hdev);
-	if (err < 0) {
-		hci_free_dev(hdev);
-		return err;
-	}
+	if (err < 0)
+		goto out_free_dev;
 
 	usb_set_intfdata(intf, data);
 
 	return 0;
+
+out_free_dev:
+	hci_free_dev(hdev);
+	return err;
 }
 
 static void btusb_disconnect(struct usb_interface *intf)
-- 
2.11.0.390.gc69c2f50cf-goog

^ permalink raw reply related

* [PATCH v5 2/3] Bluetooth: btusb: Add out-of-band wakeup support
From: Rajat Jain @ 2017-01-12 18:01 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Marcel Holtmann, Gustavo Padovan,
	Johan Hedberg, Amitkumar Karwar, Wei-Ning Huang, Xinming Hu,
	netdev, devicetree, linux-bluetooth, Brian Norris, linux-kernel
  Cc: Rajat Jain, rajatxjain
In-Reply-To: <20170112180107.63244-1-rajatja@google.com>

Some onboard BT chips (e.g. Marvell 8997) contain a wakeup pin that
can be connected to a gpio on the CPU side, and can be used to wakeup
the host out-of-band. This can be useful in situations where the
in-band wakeup is not possible or not preferable (e.g. the in-band
wakeup may require the USB host controller to remain active, and
hence consuming more system power during system sleep).

The oob gpio interrupt to be used for wakeup on the CPU side, is
read from the device tree node, (using standard interrupt descriptors).
A devcie tree binding document is also added for the driver. The
compatible string is in compliance with
Documentation/devicetree/bindings/usb/usb-device.txt

Signed-off-by: Rajat Jain <rajatja@google.com>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Acked-by: Rob Herring <robh@kernel.org>
---
v5: Move the call to pm_wakeup_event() to the begining of irq handler.
v4: Move the set_bit(BTUSB_OOB_WAKE_DISABLED,..) call to the beginning of
    btusb_config_oob_wake()
v3: Add Brian's "Reviewed-by"
v2: * Use interrupt-names ("wakeup") instead of assuming first interrupt.
    * Leave it on device tree to specify IRQ flags (level /edge triggered)
    * Mark the device as non wakeable on exit.

 Documentation/devicetree/bindings/net/btusb.txt | 40 ++++++++++++
 drivers/bluetooth/btusb.c                       | 85 +++++++++++++++++++++++++
 2 files changed, 125 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/btusb.txt

diff --git a/Documentation/devicetree/bindings/net/btusb.txt b/Documentation/devicetree/bindings/net/btusb.txt
new file mode 100644
index 000000000000..2c0355c85972
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/btusb.txt
@@ -0,0 +1,40 @@
+Generic Bluetooth controller over USB (btusb driver)
+---------------------------------------------------
+
+Required properties:
+
+  - compatible : should comply with the format "usbVID,PID" specified in
+		 Documentation/devicetree/bindings/usb/usb-device.txt
+		 At the time of writing, the only OF supported devices
+		 (more may be added later) are:
+
+		  "usb1286,204e" (Marvell 8997)
+
+Optional properties:
+
+  - interrupt-parent: phandle of the parent interrupt controller
+  - interrupt-names: (see below)
+  - interrupts : The interrupt specified by the name "wakeup" is the interrupt
+		 that shall be used for out-of-band wake-on-bt. Driver will
+		 request this interrupt for wakeup. During system suspend, the
+		 irq will be enabled so that the bluetooth chip can wakeup host
+		 platform out of band. During system resume, the irq will be
+		 disabled to make sure unnecessary interrupt is not received.
+
+Example:
+
+Following example uses irq pin number 3 of gpio0 for out of band wake-on-bt:
+
+&usb_host1_ehci {
+    status = "okay";
+    #address-cells = <1>;
+    #size-cells = <0>;
+
+    mvl_bt1: bt@1 {
+	compatible = "usb1286,204e";
+	reg = <1>;
+	interrupt-parent = <&gpio0>;
+	interrupt-name = "wakeup";
+	interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+    };
+};
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index ce22cefceed1..0a777bb407b1 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -24,6 +24,8 @@
 #include <linux/module.h>
 #include <linux/usb.h>
 #include <linux/firmware.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
 #include <asm/unaligned.h>
 
 #include <net/bluetooth/bluetooth.h>
@@ -369,6 +371,7 @@ static const struct usb_device_id blacklist_table[] = {
 #define BTUSB_BOOTING		9
 #define BTUSB_RESET_RESUME	10
 #define BTUSB_DIAG_RUNNING	11
+#define BTUSB_OOB_WAKE_DISABLED	12
 
 struct btusb_data {
 	struct hci_dev       *hdev;
@@ -416,6 +419,8 @@ struct btusb_data {
 	int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
 
 	int (*setup_on_usb)(struct hci_dev *hdev);
+
+	int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
 };
 
 static inline void btusb_free_frags(struct btusb_data *data)
@@ -2728,6 +2733,66 @@ static int btusb_bcm_set_diag(struct hci_dev *hdev, bool enable)
 }
 #endif
 
+#ifdef CONFIG_PM
+static irqreturn_t btusb_oob_wake_handler(int irq, void *priv)
+{
+	struct btusb_data *data = priv;
+
+	pm_wakeup_event(&data->udev->dev, 0);
+
+	/* Disable only if not already disabled (keep it balanced) */
+	if (!test_and_set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags)) {
+		disable_irq_nosync(irq);
+		disable_irq_wake(irq);
+	}
+	return IRQ_HANDLED;
+}
+
+static const struct of_device_id btusb_match_table[] = {
+	{ .compatible = "usb1286,204e" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, btusb_match_table);
+
+/* Use an oob wakeup pin? */
+static int btusb_config_oob_wake(struct hci_dev *hdev)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+	struct device *dev = &data->udev->dev;
+	int irq, ret;
+
+	set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags);
+
+	if (!of_match_device(btusb_match_table, dev))
+		return 0;
+
+	/* Move on if no IRQ specified */
+	irq = of_irq_get_byname(dev->of_node, "wakeup");
+	if (irq <= 0) {
+		bt_dev_dbg(hdev, "%s: no OOB Wakeup IRQ in DT", __func__);
+		return 0;
+	}
+
+	ret = devm_request_irq(&hdev->dev, irq, btusb_oob_wake_handler,
+			       0, "OOB Wake-on-BT", data);
+	if (ret) {
+		bt_dev_err(hdev, "%s: IRQ request failed", __func__);
+		return ret;
+	}
+
+	ret = device_init_wakeup(dev, true);
+	if (ret) {
+		bt_dev_err(hdev, "%s: failed to init_wakeup\n", __func__);
+		return ret;
+	}
+
+	data->oob_wake_irq = irq;
+	disable_irq(irq);
+	bt_dev_info(hdev, "OOB Wake-on-BT configured at IRQ %u\n", irq);
+	return 0;
+}
+#endif
+
 static int btusb_probe(struct usb_interface *intf,
 		       const struct usb_device_id *id)
 {
@@ -2849,6 +2914,11 @@ static int btusb_probe(struct usb_interface *intf,
 	hdev->send   = btusb_send_frame;
 	hdev->notify = btusb_notify;
 
+#ifdef CONFIG_PM
+	err = btusb_config_oob_wake(hdev);
+	if (err)
+		goto out_free_dev;
+#endif
 	if (id->driver_info & BTUSB_CW6622)
 		set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks);
 
@@ -3061,6 +3131,9 @@ static void btusb_disconnect(struct usb_interface *intf)
 			usb_driver_release_interface(&btusb_driver, data->isoc);
 	}
 
+	if (data->oob_wake_irq)
+		device_init_wakeup(&data->udev->dev, false);
+
 	hci_free_dev(hdev);
 }
 
@@ -3089,6 +3162,12 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
 	btusb_stop_traffic(data);
 	usb_kill_anchored_urbs(&data->tx_anchor);
 
+	if (data->oob_wake_irq && device_may_wakeup(&data->udev->dev)) {
+		clear_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags);
+		enable_irq_wake(data->oob_wake_irq);
+		enable_irq(data->oob_wake_irq);
+	}
+
 	/* Optionally request a device reset on resume, but only when
 	 * wakeups are disabled. If wakeups are enabled we assume the
 	 * device will stay powered up throughout suspend.
@@ -3126,6 +3205,12 @@ static int btusb_resume(struct usb_interface *intf)
 	if (--data->suspend_count)
 		return 0;
 
+	/* Disable only if not already disabled (keep it balanced) */
+	if (!test_and_set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags)) {
+		disable_irq(data->oob_wake_irq);
+		disable_irq_wake(data->oob_wake_irq);
+	}
+
 	if (!test_bit(HCI_RUNNING, &hdev->flags))
 		goto done;
 
-- 
2.11.0.390.gc69c2f50cf-goog

^ permalink raw reply related

* [PATCH v5 3/3] Bluetooth: btusb: Configure Marvell to use one of the pins for oob wakeup
From: Rajat Jain @ 2017-01-12 18:01 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland, Marcel Holtmann, Gustavo Padovan,
	Johan Hedberg, Amitkumar Karwar, Wei-Ning Huang, Xinming Hu,
	netdev, devicetree, linux-bluetooth, Brian Norris, linux-kernel
  Cc: Rajat Jain, rajatxjain
In-Reply-To: <20170112180107.63244-1-rajatja@google.com>

The Marvell devices may have many gpio pins, and hence for wakeup
on these out-of-band pins, the chip needs to be told which pin is
to be used for wakeup, using an hci command.

Thus, we read the pin number etc from the device tree node and send
a command to the chip.

Signed-off-by: Rajat Jain <rajatja@google.com>
Reviewed-by: Brian Norris <briannorris@chromium.org>
Acked-by: Rob Herring <robh@kernel.org>
---
v5: same as v5
v4: same as v3
v3: * remove the Marvell specific id table and check
    * Add reference to marvell-bt-8xxx.txt in btusb.txt
    * Add "Reviewed-by" and "Acked-by"    
v2: Fix the binding document to specify to use "wakeup" interrupt-name

 Documentation/devicetree/bindings/net/btusb.txt    |  3 ++
 .../{marvell-bt-sd8xxx.txt => marvell-bt-8xxx.txt} | 46 +++++++++++++++----
 drivers/bluetooth/btusb.c                          | 51 ++++++++++++++++++++++
 3 files changed, 92 insertions(+), 8 deletions(-)
 rename Documentation/devicetree/bindings/net/{marvell-bt-sd8xxx.txt => marvell-bt-8xxx.txt} (50%)

diff --git a/Documentation/devicetree/bindings/net/btusb.txt b/Documentation/devicetree/bindings/net/btusb.txt
index 2c0355c85972..01fa2d4188d4 100644
--- a/Documentation/devicetree/bindings/net/btusb.txt
+++ b/Documentation/devicetree/bindings/net/btusb.txt
@@ -10,6 +10,9 @@ Required properties:
 
 		  "usb1286,204e" (Marvell 8997)
 
+Also, vendors that use btusb may have device additional properties, e.g:
+Documentation/devicetree/bindings/net/marvell-bt-8xxx.txt
+
 Optional properties:
 
   - interrupt-parent: phandle of the parent interrupt controller
diff --git a/Documentation/devicetree/bindings/net/marvell-bt-sd8xxx.txt b/Documentation/devicetree/bindings/net/marvell-bt-8xxx.txt
similarity index 50%
rename from Documentation/devicetree/bindings/net/marvell-bt-sd8xxx.txt
rename to Documentation/devicetree/bindings/net/marvell-bt-8xxx.txt
index 6a9a63cb0543..9be1059ff03f 100644
--- a/Documentation/devicetree/bindings/net/marvell-bt-sd8xxx.txt
+++ b/Documentation/devicetree/bindings/net/marvell-bt-8xxx.txt
@@ -1,16 +1,21 @@
-Marvell 8897/8997 (sd8897/sd8997) bluetooth SDIO devices
+Marvell 8897/8997 (sd8897/sd8997) bluetooth devices (SDIO or USB based)
 ------
+The 8997 devices supports multiple interfaces. When used on SDIO interfaces,
+the btmrvl driver is used and when used on USB interface, the btusb driver is
+used.
 
 Required properties:
 
   - compatible : should be one of the following:
-	* "marvell,sd8897-bt"
-	* "marvell,sd8997-bt"
+	* "marvell,sd8897-bt" (for SDIO)
+	* "marvell,sd8997-bt" (for SDIO)
+	* "usb1286,204e"      (for USB)
 
 Optional properties:
 
   - marvell,cal-data: Calibration data downloaded to the device during
 		      initialization. This is an array of 28 values(u8).
+		      This is only applicable to SDIO devices.
 
   - marvell,wakeup-pin: It represents wakeup pin number of the bluetooth chip.
 		        firmware will use the pin to wakeup host system (u16).
@@ -18,10 +23,15 @@ Optional properties:
 		      platform. The value will be configured to firmware. This
 		      is needed to work chip's sleep feature as expected (u16).
   - interrupt-parent: phandle of the parent interrupt controller
-  - interrupts : interrupt pin number to the cpu. Driver will request an irq based
-		 on this interrupt number. During system suspend, the irq will be
-		 enabled so that the bluetooth chip can wakeup host platform under
-		 certain condition. During system resume, the irq will be disabled
+  - interrupt-names: Used only for USB based devices (See below)
+  - interrupts : specifies the interrupt pin number to the cpu. For SDIO, the
+		 driver will use the first interrupt specified in the interrupt
+		 array. For USB based devices, the driver will use the interrupt
+		 named "wakeup" from the interrupt-names and interrupt arrays.
+		 The driver will request an irq based on this interrupt number.
+		 During system suspend, the irq will be enabled so that the
+		 bluetooth chip can wakeup host platform under certain
+		 conditions. During system resume, the irq will be disabled
 		 to make sure unnecessary interrupt is not received.
 
 Example:
@@ -29,7 +39,9 @@ Example:
 IRQ pin 119 is used as system wakeup source interrupt.
 wakeup pin 13 and gap 100ms are configured so that firmware can wakeup host
 using this device side pin and wakeup latency.
-calibration data is also available in below example.
+
+Example for SDIO device follows (calibration data is also available in
+below example).
 
 &mmc3 {
 	status = "okay";
@@ -54,3 +66,21 @@ calibration data is also available in below example.
 		marvell,wakeup-gap-ms = /bits/ 16 <0x64>;
 	};
 };
+
+Example for USB device:
+
+&usb_host1_ohci {
+    status = "okay";
+    #address-cells = <1>;
+    #size-cells = <0>;
+
+    mvl_bt1: bt@1 {
+	compatible = "usb1286,204e";
+	reg = <1>;
+	interrupt-parent = <&gpio0>;
+	interrupt-names = "wakeup";
+	interrupts = <119 IRQ_TYPE_LEVEL_LOW>;
+	marvell,wakeup-pin = /bits/ 16 <0x0d>;
+	marvell,wakeup-gap-ms = /bits/ 16 <0x64>;
+    };
+};
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 0a777bb407b1..a1079c19b96f 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2343,6 +2343,50 @@ static int btusb_shutdown_intel(struct hci_dev *hdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+/* Configure an out-of-band gpio as wake-up pin, if specified in device tree */
+static int marvell_config_oob_wake(struct hci_dev *hdev)
+{
+	struct sk_buff *skb;
+	struct btusb_data *data = hci_get_drvdata(hdev);
+	struct device *dev = &data->udev->dev;
+	u16 pin, gap, opcode;
+	int ret;
+	u8 cmd[5];
+
+	/* Move on if no wakeup pin specified */
+	if (of_property_read_u16(dev->of_node, "marvell,wakeup-pin", &pin) ||
+	    of_property_read_u16(dev->of_node, "marvell,wakeup-gap-ms", &gap))
+		return 0;
+
+	/* Vendor specific command to configure a GPIO as wake-up pin */
+	opcode = hci_opcode_pack(0x3F, 0x59);
+	cmd[0] = opcode & 0xFF;
+	cmd[1] = opcode >> 8;
+	cmd[2] = 2; /* length of parameters that follow */
+	cmd[3] = pin;
+	cmd[4] = gap; /* time in ms, for which wakeup pin should be asserted */
+
+	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
+	if (!skb) {
+		bt_dev_err(hdev, "%s: No memory\n", __func__);
+		return -ENOMEM;
+	}
+
+	memcpy(skb_put(skb, sizeof(cmd)), cmd, sizeof(cmd));
+	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
+
+	ret = btusb_send_frame(hdev, skb);
+	if (ret) {
+		bt_dev_err(hdev, "%s: configuration failed\n", __func__);
+		kfree_skb(skb);
+		return ret;
+	}
+
+	return 0;
+}
+#endif
+
 static int btusb_set_bdaddr_marvell(struct hci_dev *hdev,
 				    const bdaddr_t *bdaddr)
 {
@@ -2918,6 +2962,13 @@ static int btusb_probe(struct usb_interface *intf,
 	err = btusb_config_oob_wake(hdev);
 	if (err)
 		goto out_free_dev;
+
+	/* Marvell devices may need a specific chip configuration */
+	if (id->driver_info & BTUSB_MARVELL && data->oob_wake_irq) {
+		err = marvell_config_oob_wake(hdev);
+		if (err)
+			goto out_free_dev;
+	}
 #endif
 	if (id->driver_info & BTUSB_CW6622)
 		set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks);
-- 
2.11.0.390.gc69c2f50cf-goog

^ 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