[PATCH v9 1/2] media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver documentation

[Niklas Söderlund]

Documentation for Renesas R-Car MIPI CSI-2 receiver. The CSI-2 receivers
are located between the video sources (CSI-2 transmitters) and the video
grabbers (VIN) on Gen3 of Renesas R-Car SoC.

Each CSI-2 device is connected to more then one VIN device which
simultaneously can receive video from the same CSI-2 device. Each VIN
device can also be connected to more then one CSI-2 device. The routing
of which link are used are controlled by the VIN devices. There are only
a few possible routes which are set by hardware limitations, which are
different for each SoC in the Gen3 family.

To work with the limitations of routing possibilities it is necessary
for the DT bindings to describe which VIN device is connected to which
CSI-2 device. This is why port 1 needs to to assign reg numbers for each
VIN device that be connected to it. To setup and to know which links are
valid for each SoC is the responsibility of the VIN driver since the
register to configure it belongs to the VIN hardware.

Signed-off-by: Niklas Söderlund 
---
 .../devicetree/bindings/media/rcar-csi2.txt| 103 +
 MAINTAINERS|   1 +
 2 files changed, 104 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/rcar-csi2.txt

diff --git a/Documentation/devicetree/bindings/media/rcar-csi2.txt 
b/Documentation/devicetree/bindings/media/rcar-csi2.txt
new file mode 100644
index ..39d41d82b71b60eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/rcar-csi2.txt
@@ -0,0 +1,103 @@
+Renesas R-Car MIPI CSI-2
+
+
+The rcar-csi2 device provides MIPI CSI-2 capabilities for the Renesas R-Car
+family of devices. It is to be used in conjunction with the R-Car VIN module,
+which provides the video capture capabilities.
+
+Mandatory properties
+
+ - compatible: Must be one or more of the following
+   - "renesas,r8a7795-csi2" for the R8A7795 device.
+   - "renesas,r8a7796-csi2" for the R8A7796 device.
+
+ - reg: the register base and size for the device registers
+ - interrupts: the interrupt for the device
+ - clocks: Reference to the parent clock
+
+The device node shall contain two 'port' child nodes according to the
+bindings defined in Documentation/devicetree/bindings/media/
+video-interfaces.txt. Port 0 shall connect the node that is the video
+source for to the CSI-2. Port 1 shall connect all the R-Car VIN
+modules, which can make use of the CSI-2 module.
+
+- Port 0 - Video source (Mandatory)
+   - Endpoint 0 - sub-node describing the endpoint that is the video source
+
+- Port 1 - VIN instances (Mandatory for all VIN present in the SoC)
+   - Endpoint 0 - sub-node describing the endpoint that is VIN0
+   - Endpoint 1 - sub-node describing the endpoint that is VIN1
+   - Endpoint 2 - sub-node describing the endpoint that is VIN2
+   - Endpoint 3 - sub-node describing the endpoint that is VIN3
+   - Endpoint 4 - sub-node describing the endpoint that is VIN4
+   - Endpoint 5 - sub-node describing the endpoint that is VIN5
+   - Endpoint 6 - sub-node describing the endpoint that is VIN6
+   - Endpoint 7 - sub-node describing the endpoint that is VIN7
+
+Example:
+
+   csi20: csi2@fea8 {
+   compatible = "renesas,r8a7796-csi2", "renesas,rcar-gen3-csi2";
+   reg = <0 0xfea8 0 0x1>;
+   interrupts = <0 184 IRQ_TYPE_LEVEL_HIGH>;
+   clocks = <&cpg CPG_MOD 714>;
+   power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   reg = <0>;
+
+   csi20_in: endpoint@0 {
+   clock-lanes = <0>;
+   data-lanes = <1>;
+   remote-endpoint = <&adv7482_txb>;
+   };
+   };
+
+   port@1 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   reg = <1>;
+
+   csi20vin0: endpoint@0 {
+   reg = <0>;
+   remote-endpoint = <&vin0csi20>;
+   };
+   csi20vin1: endpoint@1 {
+   reg = <1>;
+   remote-endpoint = <&vin1csi20>;
+   };
+   csi20vin2: endpoint@2 {
+   reg = <2>;
+   remote-endpoint = <&vin2csi20>;
+

[PATCH v9 0/2] media: rcar-csi2: add Renesas R-Car MIPI CSI-2 support

[Niklas Söderlund]

Hi,

This is the latest incarnation of R-Car MIPI CSI-2 receiver driver. It's
based on top of the media-tree and are tested on Renesas Salvator-X 
together with the out-of-tree patches for rcar-vin to add support for 
Gen3 VIN. If anyone is interested to test video grabbing using these out 
of tree patches please see [1].

Changes since v8:
- Updated bindings documentation, thanks Rob!
- Make use of the now in media-tree sub-notifier V4L2 API
- Add delay when resetting the IP to allow for a proper reset
- Fix bug in s_stream error path where the usage count was off if an 
  error was hit.
- Add support for H3 ES2.0

Changes since v7:
- Rebase on top of the latest incremental async patches.
- Fix comments on DT documentation.
- Use v4l2_ctrl_g_ctrl_int64() instead of v4l2_g_ext_ctrls().
- Handle try formats in .set_fmt() and .get_fmt().
- Don't call v4l2_device_register_subdev_nodes() as this is not needed
  with the complete() callbacks synchronized.
- Fix line over 80 chars.
- Fix varies spelling mistakes.

Changes since v6:
- Rebased on top of Sakaris fwnode patches.
- Changed of RCAR_CSI2_PAD_MAX to NR_OF_RCAR_CSI2_PAD.
- Remove assumption about unknown media bus type, thanks Sakari for
  pointing this out.
- Created table for supported format information instead of scattering
  this information around the driver, thanks Sakari!
- Small newline fixes and reduce some indentation levels.

Changes since v5:
- Make use of the incremental async subnotifer and helper to map DT
  endpoint to media pad number. This moves functionality which
  previously in the Gen3 patches for R-Car VIN driver to this R-Car
  CSI-2 driver. This is done in preparation to support the ADV7482
  driver in development by Kieran which will register more then one
  subdevice and the CSI-2 driver needs to cope wit this. Further more it
  prepares the driver for another use-case where more then one subdevice
  is present upstream for the CSI-2.
- Small cleanups.
- Add explicit include for linux/io.h, thanks Kieran.

Changes since v4:
- Match SoC part numbers and drop trailing space in documentation,
  thanks Geert for pointing this out.
- Clarify that the driver is a CSI-2 receiver by supervised
  s/interface/receiver/, thanks Laurent.
- Add entries in Kconfig and Makefile alphabetically instead of append.
- Rename struct rcar_csi2 member swap to lane_swap.
- Remove macros to wrap calls to dev_{dbg,info,warn,err}.
- Add wrappers for ioread32 and iowrite32.
- Remove unused interrupt handler, but keep checking in probe that there
  are a interrupt define in DT.
- Rework how to wait for LP-11 state, thanks Laurent for the great idea!
- Remove unneeded delay in rcar_csi2_reset()
- Remove check for duplicated lane id:s from DT parsing. Broken out to a
  separate patch adding this check directly to v4l2_of_parse_endpoint().
- Fixed rcar_csi2_start() to ask it's source subdevice for information
  about pixel rate and frame format. With this change having
  {set,get}_fmt operations became redundant, it was only used for
  figuring out this out so dropped them.
- Tabulated frequency settings map.
- Dropped V4L2_SUBDEV_FL_HAS_DEVNODE it should never have been set.
- Switched from MEDIA_ENT_F_ATV_DECODER to
  MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER as entity function. I can't
  find a more suitable function, and what the hardware do is to fetch
  video from an external chip and passes it on to a another SoC internal
  IP it's sort of a formatter.
- Break out DT documentation and code in two patches.

Changes since v3:
- Update DT binding documentation with input from Geert Uytterhoeven,
  thanks!

Changes since v2:
- Added media control pads as this is needed by the new rcar-vin driver.
- Update DT bindings after review comments and to add r8a7796 support.
- Add get_fmt handler.
- Fix media bus format error s/YUYV8/UYVY8/

Changes since v1:
- Drop dependency on a pad aware s_stream operation.
- Use the DT bindings format "renesas,-", thanks Geert
  for pointing this out.

1. http://elinux.org/R-Car/Tests:rcar-vin

Niklas Söderlund (2):
  media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver documentation
  media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

 .../devicetree/bindings/media/rcar-csi2.txt| 103 +++
 MAINTAINERS|   1 +
 drivers/media/platform/rcar-vin/Kconfig|  12 +
 drivers/media/platform/rcar-vin/Makefile   |   1 +
 drivers/media/platform/rcar-vin/rcar-csi2.c| 933 +
 5 files changed, 1050 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/rcar-csi2.txt
 create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c

-- 
2.15.0

[PATCH v9 2/2] media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver

[Niklas Söderlund]

A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
supports the rcar-vin driver on R-Car Gen3 SoCs where separate CSI-2
hardware blocks are connected between the video sources and the video
grabbers (VIN).

Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.

Signed-off-by: Niklas Söderlund 
---
 drivers/media/platform/rcar-vin/Kconfig |  12 +
 drivers/media/platform/rcar-vin/Makefile|   1 +
 drivers/media/platform/rcar-vin/rcar-csi2.c | 933 
 3 files changed, 946 insertions(+)
 create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c

diff --git a/drivers/media/platform/rcar-vin/Kconfig 
b/drivers/media/platform/rcar-vin/Kconfig
index af4c98b44d2e22cb..6875f30c1ae42631 100644
--- a/drivers/media/platform/rcar-vin/Kconfig
+++ b/drivers/media/platform/rcar-vin/Kconfig
@@ -1,3 +1,15 @@
+config VIDEO_RCAR_CSI2
+   tristate "R-Car MIPI CSI-2 Receiver"
+   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF
+   depends on ARCH_RENESAS || COMPILE_TEST
+   select V4L2_FWNODE
+   ---help---
+ Support for Renesas R-Car MIPI CSI-2 receiver.
+ Supports R-Car Gen3 SoCs.
+
+ To compile this driver as a module, choose M here: the
+ module will be called rcar-csi2.
+
 config VIDEO_RCAR_VIN
tristate "R-Car Video Input (VIN) Driver"
depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF && HAS_DMA && 
MEDIA_CONTROLLER
diff --git a/drivers/media/platform/rcar-vin/Makefile 
b/drivers/media/platform/rcar-vin/Makefile
index 48c5632c21dc060b..5ab803d3e7c1aa57 100644
--- a/drivers/media/platform/rcar-vin/Makefile
+++ b/drivers/media/platform/rcar-vin/Makefile
@@ -1,3 +1,4 @@
 rcar-vin-objs = rcar-core.o rcar-dma.o rcar-v4l2.o
 
+obj-$(CONFIG_VIDEO_RCAR_CSI2) += rcar-csi2.o
 obj-$(CONFIG_VIDEO_RCAR_VIN) += rcar-vin.o
diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c 
b/drivers/media/platform/rcar-vin/rcar-csi2.c
new file mode 100644
index ..f3c17545c12dc057
--- /dev/null
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -0,0 +1,933 @@
+/*
+ * Driver for Renesas R-Car MIPI CSI-2 Receiver
+ *
+ * Copyright (C) 2017 Renesas Electronics Corp.
+ *
+ * This program 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.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Register offsets and bits */
+
+/* Control Timing Select */
+#define TREF_REG   0x00
+#define TREF_TREF  (1 << 0)
+
+/* Software Reset */
+#define SRST_REG   0x04
+#define SRST_SRST  (1 << 0)
+
+/* PHY Operation Control */
+#define PHYCNT_REG 0x08
+#define PHYCNT_SHUTDOWNZ   (1 << 17)
+#define PHYCNT_RSTZ(1 << 16)
+#define PHYCNT_ENABLECLK   (1 << 4)
+#define PHYCNT_ENABLE_3(1 << 3)
+#define PHYCNT_ENABLE_2(1 << 2)
+#define PHYCNT_ENABLE_1(1 << 1)
+#define PHYCNT_ENABLE_0(1 << 0)
+
+/* Checksum Control */
+#define CHKSUM_REG 0x0c
+#define CHKSUM_ECC_EN  (1 << 1)
+#define CHKSUM_CRC_EN  (1 << 0)
+
+/*
+ * Channel Data Type Select
+ * VCDT[0-15]:  Channel 1 VCDT[16-31]:  Channel 2
+ * VCDT2[0-15]: Channel 3 VCDT2[16-31]: Channel 4
+ */
+#define VCDT_REG   0x10
+#define VCDT2_REG  0x14
+#define VCDT_VCDTN_EN  (1 << 15)
+#define VCDT_SEL_VC(n) (((n) & 0x3) << 8)
+#define VCDT_SEL_DTN_ON(1 << 6)
+#define VCDT_SEL_DT(n) (((n) & 0x3f) << 0)
+
+/* Frame Data Type Select */
+#define FRDT_REG   0x18
+
+/* Field Detection Control */
+#define FLD_REG0x1c
+#define FLD_FLD_NUM(n) (((n) & 0xff) << 16)
+#define FLD_FLD_EN4(1 << 3)
+#define FLD_FLD_EN3(1 << 2)
+#define FLD_FLD_EN2(1 << 1)
+#define FLD_FLD_EN (1 << 0)
+
+/* Automatic Standby Control */
+#define ASTBY_REG  0x20
+
+/* Long Data Type Setting 0 */
+#define LNGDT0_REG 0x28
+
+/* Long Data Type Setting 1 */
+#define LNGDT1_REG 0x2c
+
+/* Interrupt Enable */
+#define INTEN_REG  0x30
+
+/* Interrupt Source Mask */
+#define INTCLOSE_REG   0x34
+
+/* Interrupt Status Monitor */
+#define INTSTATE_REG   0x38
+#define INTSTATE_INT_ULPS_START(1 << 7)
+#define INTSTATE_INT_ULPS_END  (1 <<

[RFT] i2c: sh_mobile: avoid unnecessary register read

[Wolfram Sang]

There is no data when the first WAIT interrupt arrives. No need to read
something then.

Signed-off-by: Wolfram Sang 
---

I am not super happy with (real pos >= 0) done twice, but I didn't find a
better solution yet. The compiler will make this cheap anyhow, I guess.

Jacopo: can you please test this on top of all other patches?

 drivers/i2c/busses/i2c-sh_mobile.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-sh_mobile.c 
b/drivers/i2c/busses/i2c-sh_mobile.c
index 80561ffbcf7b46..40a66d466c3c49 100644
--- a/drivers/i2c/busses/i2c-sh_mobile.c
+++ b/drivers/i2c/busses/i2c-sh_mobile.c
@@ -433,8 +433,9 @@ static int sh_mobile_i2c_isr_rx(struct sh_mobile_i2c_data 
*pd)
break;
}
data = i2c_op(pd, OP_RX_STOP_DATA, 0);
-   } else
+   } else if (real_pos >= 0) {
data = i2c_op(pd, OP_RX, 0);
+   }
 
if (real_pos >= 0)
pd->msg->buf[real_pos] = data;
-- 
2.11.0

Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Rafael J. Wysocki]

On Thu, Nov 9, 2017 at 3:41 PM, Geert Uytterhoeven  wrote:
> Hi Ulf,
>
> On Thu, Nov 9, 2017 at 3:28 PM, Ulf Hansson  wrote:
>> [...]
>>
> The Ethernet driver can still call device_set_wakeup_enable(... , false)
> to control this.  If WoL is disabled by the user (or deemed not usable, 
> see
> below), it already does so.

 I don't think that API is intended to be used like that and I wonder
 if it even works as expected.

 Quoting the doc:
 "If device wakeup mechanisms are enabled or disabled directly by
 drivers, they also should use :c:func:`device_may_wakeup()` to decide what 
 to do
 during a system sleep transition.  Device drivers, however, are not 
 expected to
 call :c:func:`device_set_wakeup_enable()` directly in any case."

 Rafael, can you comment on this?
>>>
>>> There are ca. 100 callers in drivers.
>>
>> Yeah, but those doesn't normally use it to toggle the setting, but
>> instead only to set a default value during ->probe() or whatever
>> initialization code that runs.
>>
>> I think that makes a big difference, doesn't it?
>
> The few Ethernet drivers I looked at change the state in their
> ethtool_ops.set_wol() callback, not during probe.
> This is to be configured from userspace using ethtool.

Which is the case I was talking about.

Since changing the WoL setting via ethtool is expected to cause the
sysfs knob to reflect its status, using device_set_wakeup_enable() in
there is not actually confusing.

The same applies to setting the default from ->probe().

It will be confusing in all of the other cases, though.

Thanks,
Rafael

Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Geert Uytterhoeven]

Hi Ulf,

On Thu, Nov 9, 2017 at 3:28 PM, Ulf Hansson  wrote:
> [...]
>
 The Ethernet driver can still call device_set_wakeup_enable(... , false)
 to control this.  If WoL is disabled by the user (or deemed not usable, see
 below), it already does so.
>>>
>>> I don't think that API is intended to be used like that and I wonder
>>> if it even works as expected.
>>>
>>> Quoting the doc:
>>> "If device wakeup mechanisms are enabled or disabled directly by
>>> drivers, they also should use :c:func:`device_may_wakeup()` to decide what 
>>> to do
>>> during a system sleep transition.  Device drivers, however, are not 
>>> expected to
>>> call :c:func:`device_set_wakeup_enable()` directly in any case."
>>>
>>> Rafael, can you comment on this?
>>
>> There are ca. 100 callers in drivers.
>
> Yeah, but those doesn't normally use it to toggle the setting, but
> instead only to set a default value during ->probe() or whatever
> initialization code that runs.
>
> I think that makes a big difference, doesn't it?

The few Ethernet drivers I looked at change the state in their
ethtool_ops.set_wol() callback, not during probe.
This is to be configured from userspace using ethtool.

 In addition, keeping WoL enabled for cases 1 and 2 may be desirable
 (e.g allow wake-up if a cable is plugged in during system suspend and
  a magic packet is received afterwards), depending on the hardware.
 But the driver can already control that through device_set_wakeup_enable().
>>>
>>> Ehh, that sounds weird. :-) If the Ethernet interface is down, how
>>> would such packet be received?
>>
>> It depends on your meaning of "up".  My interpretation is that "up" means
>> ready to handle packets between physical media and the Linux networking 
>> stack.
>>
>> So even when "down", the actual Ethernet controller may still be able to
>> receive a magic packet if WoL is enabled.  The magic packet is really a
>> magic packet not intended to be transmitted to the networking stack, but
>> merely serves as a wakeup signal.
>
> I see! So, in the end this seems like a combination of what the HW
> supports and what the user policy is set to.
>
> Out of curiosity, can you tell how those Renesas Ethernet devices
> works in this regards?

I don't know, I was just playing the devil's advocate ;-)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Ulf Hansson]

[...]

>>> The Ethernet driver can still call device_set_wakeup_enable(... , false)
>>> to control this.  If WoL is disabled by the user (or deemed not usable, see
>>> below), it already does so.
>>
>> I don't think that API is intended to be used like that and I wonder
>> if it even works as expected.
>>
>> Quoting the doc:
>> "If device wakeup mechanisms are enabled or disabled directly by
>> drivers, they also should use :c:func:`device_may_wakeup()` to decide what 
>> to do
>> during a system sleep transition.  Device drivers, however, are not expected 
>> to
>> call :c:func:`device_set_wakeup_enable()` directly in any case."
>>
>> Rafael, can you comment on this?
>
> There are ca. 100 callers in drivers.

Yeah, but those doesn't normally use it to toggle the setting, but
instead only to set a default value during ->probe() or whatever
initialization code that runs.

I think that makes a big difference, doesn't it?

>
>>> In addition, keeping WoL enabled for cases 1 and 2 may be desirable
>>> (e.g allow wake-up if a cable is plugged in during system suspend and
>>>  a magic packet is received afterwards), depending on the hardware.
>>> But the driver can already control that through device_set_wakeup_enable().
>>
>> Ehh, that sounds weird. :-) If the Ethernet interface is down, how
>> would such packet be received?
>
> It depends on your meaning of "up".  My interpretation is that "up" means
> ready to handle packets between physical media and the Linux networking stack.
>
> So even when "down", the actual Ethernet controller may still be able to
> receive a magic packet if WoL is enabled.  The magic packet is really a
> magic packet not intended to be transmitted to the networking stack, but
> merely serves as a wakeup signal.

I see! So, in the end this seems like a combination of what the HW
supports and what the user policy is set to.

Out of curiosity, can you tell how those Renesas Ethernet devices
works in this regards?

Anyway, thanks for clarifying!

Kind regards
Uffe

[PATCH/RFC 3/3] gpio: rcar: Use wakeup_path i.s.o. explicit clock handling

[Geert Uytterhoeven]

Since commit ab82fa7da4dce5c7 ("gpio: rcar: Prevent module clock disable
when wake-up is enabled"), when a GPIO is used for wakeup, the GPIO
block's module clock (if exists) is manually kept running during system
suspend, to make sure the device stays active.

However, this explicit clock handling is merely a workaround for a
failure to properly communicate wakeup information to the device core.

Instead, set the device's power.wakeup_path field, to indicate this
device is part of the wakeup path.  Depending on the PM Domain's
active_wakeup configuration, the genpd core code will keep the device
enabled (and the clock running) during system suspend when needed.
This allows for the removal of all explicit clock handling code from the
driver.

Note that the dev_pm_info.wakeup_path field exists only if
CONFIG_PM_SLEEP is enabled, hence the whole suspend infrastructure is
protected by #ifdef CONFIG_PM_SLEEP.

Signed-off-by: Geert Uytterhoeven 
---
To avoid regressions, this must not be applied before "soc: renesas:
rcar-sysc: Keep wakeup sources active during system suspend" has landed
upstream, hence the "RFC"!

This driver is used on Renesas R-Car and RZ/G1 platforms.
While the GPIO block on R-Car Gen1 doesn't have a controllable module
clock and is thus fine, the rcar-sysc driver doesn't keep wakeup sources
active during system suspend yet on R-Car Gen2 and Gen3, and on RZ/G1.

v3:
  - New.
---
 drivers/gpio/gpio-rcar.c | 43 ++-
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
index 2cf5f458928b2af7..12bf87414213614d 100644
--- a/drivers/gpio/gpio-rcar.c
+++ b/drivers/gpio/gpio-rcar.c
@@ -14,7 +14,6 @@
  * GNU General Public License for more details.
  */
 
-#include 
 #include 
 #include 
 #include 
@@ -37,10 +36,9 @@ struct gpio_rcar_priv {
struct platform_device *pdev;
struct gpio_chip gpio_chip;
struct irq_chip irq_chip;
-   struct clk *clk;
unsigned int irq_parent;
bool has_both_edge_trigger;
-   bool needs_clk;
+   bool wakeup_path;
 };
 
 #define IOINTSEL 0x00  /* General IO/Interrupt Switching Register */
@@ -186,14 +184,7 @@ static int gpio_rcar_irq_set_wake(struct irq_data *d, 
unsigned int on)
}
}
 
-   if (!p->clk)
-   return 0;
-
-   if (on)
-   clk_enable(p->clk);
-   else
-   clk_disable(p->clk);
-
+   p->wakeup_path = on;
return 0;
 }
 
@@ -330,17 +321,14 @@ static int gpio_rcar_direction_output(struct gpio_chip 
*chip, unsigned offset,
 
 struct gpio_rcar_info {
bool has_both_edge_trigger;
-   bool needs_clk;
 };
 
 static const struct gpio_rcar_info gpio_rcar_info_gen1 = {
.has_both_edge_trigger = false,
-   .needs_clk = false,
 };
 
 static const struct gpio_rcar_info gpio_rcar_info_gen2 = {
.has_both_edge_trigger = true,
-   .needs_clk = true,
 };
 
 static const struct of_device_id gpio_rcar_of_table[] = {
@@ -403,7 +391,6 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, 
unsigned int *npins)
ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &args);
*npins = ret == 0 ? args.args[2] : RCAR_MAX_GPIO_PER_BANK;
p->has_both_edge_trigger = info->has_both_edge_trigger;
-   p->needs_clk = info->needs_clk;
 
if (*npins == 0 || *npins > RCAR_MAX_GPIO_PER_BANK) {
dev_warn(&p->pdev->dev,
@@ -415,6 +402,21 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, 
unsigned int *npins)
return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int gpio_rcar_suspend(struct device *dev)
+{
+   struct gpio_rcar_priv *p = dev_get_drvdata(dev);
+
+   dev->power.wakeup_path = p->wakeup_path;
+   return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(gpio_rcar_pm_ops, gpio_rcar_suspend, NULL);
+#define DEV_PM_OPS &gpio_rcar_pm_ops
+#else
+#define DEV_PM_OPS NULL
+#endif
+
 static int gpio_rcar_probe(struct platform_device *pdev)
 {
struct gpio_rcar_priv *p;
@@ -440,16 +442,6 @@ static int gpio_rcar_probe(struct platform_device *pdev)
 
platform_set_drvdata(pdev, p);
 
-   p->clk = devm_clk_get(dev, NULL);
-   if (IS_ERR(p->clk)) {
-   if (p->needs_clk) {
-   dev_err(dev, "unable to get clock\n");
-   ret = PTR_ERR(p->clk);
-   goto err0;
-   }
-   p->clk = NULL;
-   }
-
pm_runtime_enable(dev);
 
irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
@@ -536,6 +528,7 @@ static struct platform_driver gpio_rcar_device_driver = {
.remove = gpio_rcar_remove,
.driver = {
.name   = "gpio_rcar",
+   .pm = DEV_PM_OPS,
.of_match_table = of_match_ptr(gpio_rcar_of_table),
}
 };
-- 
2.7.4

[PATCH/RFC 0/3] renesas: irqchip: Use wakeup_path i.s.o. explicit clock handling

[Geert Uytterhoeven]

Hi all,

If an interrupt controller in a Renesas ARM SoC is part of a Clock
Domain, and it is part of the wakeup path, it must be kept active during
system suspend.

Currently this is handled in all interrupt controller drivers by
explicitly increasing the use count of the module clock when the device
is part of the wakeup path.  However, this explicit clock handling is
merely a workaround for a failure to properly communicate wakeup
information to the device core.

Hence this series fixes the affected drivers by setting the devices'
power.wakeup_path fields instead, to indicate they are part of the
wakeup path.  Depending on the PM Domain's active_wakeup configuration,
the genpd core code will keep the device enabled (and the clock running)
during system suspend when needed.

Note that most of these patches depend on the series "[PATCH v2 0/3] PM
/ Domain: renesas: Fix active wakeup behavior", hence they should not be
applied yet.

This has been tested on r8a73a4/ape6evm, r8a7740/armadillo,
r8a7791/koelsch, r8a7795/salvator-x and -xs, r8a7796/salvator-x, and
sh73a0/kzm9g.

Thanks for your comments!

Geert Uytterhoeven (3):
  irqchip/renesas-intc-irqpin: Use wakeup_path i.s.o. explicit clock
handling
  irqchip/renesas-irqc: Use wakeup_path i.s.o. explicit clock handling
  gpio: rcar: Use wakeup_path i.s.o. explicit clock handling

 drivers/gpio/gpio-rcar.c  | 43 +
 drivers/irqchip/irq-renesas-intc-irqpin.c | 45 +--
 drivers/irqchip/irq-renesas-irqc.c| 35 
 3 files changed, 54 insertions(+), 69 deletions(-)

-- 
2.7.4

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[PATCH/RFC 1/3] irqchip/renesas-intc-irqpin: Use wakeup_path i.s.o. explicit clock handling

[Geert Uytterhoeven]

Since commit 705bc96c2c15313c ("irqchip: renesas-intc-irqpin: Add
minimal runtime PM support"), when an IRQ is used for wakeup, the INTC
block's module clock (if exists) is manually kept running during system
suspend, to make sure the device stays active.

However, this explicit clock handling is merely a workaround for a
failure to properly communicate wakeup information to the device core.

Instead, set the device's power.wakeup_path field, to indicate this
device is part of the wakeup path.  Depending on the PM Domain's
active_wakeup configuration, the genpd core code will keep the device
enabled (and the clock running) during system suspend when needed.
This allows for the removal of all explicit clock handling code from the
driver.

Note that the dev_pm_info.wakeup_path field exists only if
CONFIG_PM_SLEEP is enabled, hence the whole suspend infrastructure is
protected by #ifdef CONFIG_PM_SLEEP.

Signed-off-by: Geert Uytterhoeven 
---
This driver is used on Renesas R-Mobile and R-Car Gen1 platforms.

As (1) the pm-rmobile driver already keeps wakeup sources active during
system suspend, and (2) the INTC block on R-Car Gen1 doesn't have a
controllable module clock, this patch should be safe to apply.

v3:
  - New.
---
 drivers/irqchip/irq-renesas-intc-irqpin.c | 45 +--
 1 file changed, 18 insertions(+), 27 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-intc-irqpin.c 
b/drivers/irqchip/irq-renesas-intc-irqpin.c
index 06f29cf5018a151f..85ed4c9b9fd5b90f 100644
--- a/drivers/irqchip/irq-renesas-intc-irqpin.c
+++ b/drivers/irqchip/irq-renesas-intc-irqpin.c
@@ -17,7 +17,6 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
-#include 
 #include 
 #include 
 #include 
@@ -78,16 +77,14 @@ struct intc_irqpin_priv {
struct platform_device *pdev;
struct irq_chip irq_chip;
struct irq_domain *irq_domain;
-   struct clk *clk;
unsigned shared_irqs:1;
-   unsigned needs_clk:1;
+   unsigned wakeup_path:1;
u8 shared_irq_mask;
 };
 
 struct intc_irqpin_config {
unsigned int irlm_bit;
unsigned needs_irlm:1;
-   unsigned needs_clk:1;
 };
 
 static unsigned long intc_irqpin_read32(void __iomem *iomem)
@@ -287,15 +284,7 @@ static int intc_irqpin_irq_set_wake(struct irq_data *d, 
unsigned int on)
int hw_irq = irqd_to_hwirq(d);
 
irq_set_irq_wake(p->irq[hw_irq].requested_irq, on);
-
-   if (!p->clk)
-   return 0;
-
-   if (on)
-   clk_enable(p->clk);
-   else
-   clk_disable(p->clk);
-
+   p->wakeup_path = on;
return 0;
 }
 
@@ -365,12 +354,10 @@ static const struct irq_domain_ops 
intc_irqpin_irq_domain_ops = {
 static const struct intc_irqpin_config intc_irqpin_irlm_r8a777x = {
.irlm_bit = 23, /* ICR0.IRLM0 */
.needs_irlm = 1,
-   .needs_clk = 0,
 };
 
 static const struct intc_irqpin_config intc_irqpin_rmobile = {
.needs_irlm = 0,
-   .needs_clk = 1,
 };
 
 static const struct of_device_id intc_irqpin_dt_ids[] = {
@@ -422,18 +409,6 @@ static int intc_irqpin_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, p);
 
config = of_device_get_match_data(dev);
-   if (config)
-   p->needs_clk = config->needs_clk;
-
-   p->clk = devm_clk_get(dev, NULL);
-   if (IS_ERR(p->clk)) {
-   if (p->needs_clk) {
-   dev_err(dev, "unable to get clock\n");
-   ret = PTR_ERR(p->clk);
-   goto err0;
-   }
-   p->clk = NULL;
-   }
 
pm_runtime_enable(dev);
pm_runtime_get_sync(dev);
@@ -602,12 +577,28 @@ static int intc_irqpin_remove(struct platform_device 
*pdev)
return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int intc_irqpin_suspend(struct device *dev)
+{
+   struct intc_irqpin_priv *p = dev_get_drvdata(dev);
+
+   dev->power.wakeup_path = p->wakeup_path;
+   return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(intc_irqpin_pm_ops, intc_irqpin_suspend, NULL);
+#define DEV_PM_OPS &intc_irqpin_pm_ops
+#else
+#define DEV_PM_OPS NULL
+#endif
+
 static struct platform_driver intc_irqpin_device_driver = {
.probe  = intc_irqpin_probe,
.remove = intc_irqpin_remove,
.driver = {
.name   = "renesas_intc_irqpin",
.of_match_table = intc_irqpin_dt_ids,
+   .pm = DEV_PM_OPS,
}
 };
 
-- 
2.7.4

[PATCH/RFC 2/3] irqchip/renesas-irqc: Use wakeup_path i.s.o. explicit clock handling

[Geert Uytterhoeven]

Since commit 6f46aedb9c85873b ("irqchip: renesas-irqc: Add wake-up
support"), when an IRQ is used for wakeup, the INTC
block's module clock is manually kept running during system suspend, to
make sure the device stays active.

However, this explicit clock handling is merely a workaround for a
failure to properly communicate wakeup information to the device core.

Instead, set the device's power.wakeup_path field, to indicate this
device is part of the wakeup path.  Depending on the PM Domain's
active_wakeup configuration, the genpd core code will keep the device
enabled (and the clock running) during system suspend when needed.
This allows for the removal of all explicit clock handling code from the
driver.

Note that the dev_pm_info.wakeup_path field exists only if
CONFIG_PM_SLEEP is enabled, hence the whole suspend infrastructure is
protected by #ifdef CONFIG_PM_SLEEP.

Signed-off-by: Geert Uytterhoeven 
---
To avoid regressions, this must not be applied before "soc: renesas:
rcar-sysc: Keep wakeup sources active during system suspend" has landed
upstream, hence the "RFC"!

This driver is used on Renesas R-Mobile, R-Car Gen2/3, and RZ/G1
platforms.  While the pm-rmobile driver already keeps wakeup sources
active during system suspend, this is not the case on R-Car Gen2/3 and
RZ/G1 yet.

v3:
  - New.
---
 drivers/irqchip/irq-renesas-irqc.c | 35 ++-
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/irqchip/irq-renesas-irqc.c 
b/drivers/irqchip/irq-renesas-irqc.c
index 52304b139aa46a60..d91dab43268f9d0a 100644
--- a/drivers/irqchip/irq-renesas-irqc.c
+++ b/drivers/irqchip/irq-renesas-irqc.c
@@ -17,7 +17,6 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
-#include 
 #include 
 #include 
 #include 
@@ -64,7 +63,7 @@ struct irqc_priv {
struct platform_device *pdev;
struct irq_chip_generic *gc;
struct irq_domain *irq_domain;
-   struct clk *clk;
+   unsigned wakeup_path:1;
 };
 
 static struct irqc_priv *irq_data_to_priv(struct irq_data *data)
@@ -111,15 +110,7 @@ static int irqc_irq_set_wake(struct irq_data *d, unsigned 
int on)
int hw_irq = irqd_to_hwirq(d);
 
irq_set_irq_wake(p->irq[hw_irq].requested_irq, on);
-
-   if (!p->clk)
-   return 0;
-
-   if (on)
-   clk_enable(p->clk);
-   else
-   clk_disable(p->clk);
-
+   p->wakeup_path = on;
return 0;
 }
 
@@ -159,12 +150,6 @@ static int irqc_probe(struct platform_device *pdev)
p->pdev = pdev;
platform_set_drvdata(pdev, p);
 
-   p->clk = devm_clk_get(&pdev->dev, NULL);
-   if (IS_ERR(p->clk)) {
-   dev_warn(&pdev->dev, "unable to get clock\n");
-   p->clk = NULL;
-   }
-
pm_runtime_enable(&pdev->dev);
pm_runtime_get_sync(&pdev->dev);
 
@@ -276,6 +261,21 @@ static int irqc_remove(struct platform_device *pdev)
return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int irqc_suspend(struct device *dev)
+{
+   struct irqc_priv *p = dev_get_drvdata(dev);
+
+   dev->power.wakeup_path = p->wakeup_path;
+   return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(irqc_pm_ops, irqc_suspend, NULL);
+#define DEV_PM_OPS &irqc_pm_ops
+#else
+#define DEV_PM_OPS NULL
+#endif
+
 static const struct of_device_id irqc_dt_ids[] = {
{ .compatible = "renesas,irqc", },
{},
@@ -288,6 +288,7 @@ static struct platform_driver irqc_device_driver = {
.driver = {
.name   = "renesas_irqc",
.of_match_table = irqc_dt_ids,
+   .pm = DEV_PM_OPS,
}
 };
 
-- 
2.7.4

[PATCH v2 1/3] clk: renesas: mstp: Keep wakeup sources active during system suspend

[Geert Uytterhoeven]

If a device is part of the CPG/MSTP Clock Domain and to be used as a
wakeup source, it must be kept active during system suspend.

Currently this is handled in device-specific drivers by explicitly
increasing the use count of the module clock when the device is
configured as a wakeup source.  However, the proper way to prevent the
device from being stopped is to inform this requirement to the genpd
core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.

Note that this will only affect devices configured as wakeup sources.

Signed-off-by: Geert Uytterhoeven 
---
v2:
  - Integrate "clk: renesas: mstp: Use GENPD_FLAG_ACTIVE_WAKEUP",
---
 drivers/clk/renesas/clk-mstp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 500a9e4e03c48957..0b222197a4576fd7 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -344,7 +344,7 @@ void __init cpg_mstp_add_clk_domain(struct device_node *np)
return;
 
pd->name = np->name;
-   pd->flags = GENPD_FLAG_PM_CLK;
+   pd->flags = GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
pd->attach_dev = cpg_mstp_attach_dev;
pd->detach_dev = cpg_mstp_detach_dev;
pm_genpd_init(pd, &pm_domain_always_on_gov, false);
-- 
2.7.4

[PATCH v2 2/3] clk: renesas: cpg-mssr: Keep wakeup sources active during system suspend

[Geert Uytterhoeven]

If a device is part of the CPG/MSSR Clock Domain and to be used as a
wakeup source, it must be kept active during system suspend.

Currently this is handled in device-specific drivers by explicitly
increasing the use count of the module clock when the device is
configured as a wakeup source.  However, the proper way to prevent the
device from being stopped is to inform this requirement to the genpd
core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.

Note that this will only affect devices configured as wakeup sources.

Signed-off-by: Geert Uytterhoeven 
---
v2:
  - Integrate "clk: renesas: cpg-mssr: Use GENPD_FLAG_ACTIVE_WAKEUP",
---
 drivers/clk/renesas/renesas-cpg-mssr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c 
b/drivers/clk/renesas/renesas-cpg-mssr.c
index e580a5e6346c2533..99699b715d7cf2de 100644
--- a/drivers/clk/renesas/renesas-cpg-mssr.c
+++ b/drivers/clk/renesas/renesas-cpg-mssr.c
@@ -500,7 +500,7 @@ static int __init cpg_mssr_add_clk_domain(struct device 
*dev,
 
genpd = &pd->genpd;
genpd->name = np->name;
-   genpd->flags = GENPD_FLAG_PM_CLK;
+   genpd->flags = GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
genpd->attach_dev = cpg_mssr_attach_dev;
genpd->detach_dev = cpg_mssr_detach_dev;
pm_genpd_init(genpd, &pm_domain_always_on_gov, false);
-- 
2.7.4

[PATCH v2 0/3] PM / Domain: renesas: Fix active wakeup behavior

[Geert Uytterhoeven]

Hi Rafael, Ulf, Kevin,

If a device in a Renesas ARM SoC is part of a Clock Domain, and it is
used as a wakeup source, it must be kept active during system suspend.

Currently this is handled in device-specific drivers by explicitly
increasing the use count of the module clock when the device is
configured as a wakeup source, or if it is part of the wakeup path.

However, this is merely a workaround.  The proper way to prevent the
device from being stopped is to inform this requirement to the genpd
core, using the new GENPD_FLAG_ACTIVE_WAKEUP flag introduced in commit
95a20ef6f7e54c6a ("PM / Domains: Allow genpd users to specify default
active wakeup behavior").

Hence this series does that for PM Domain drivers used on R-Car, RZ/A1,
RZ/G1 SoCs, mimicking what is already done succesfully on SH/R-Mobile
SoCs.  This will allow for the workarounds can be removed later.

This series was extracted from "[PATCH 00/10] PM / Domain: renesas: Fix
active wakeup behavior", and retains only fixes for Renesas PM Domain
drivers.

Changes compared to v1:
  - Integrate follow-up patches to use GENPD_FLAG_ACTIVE_WAKEUP instead
of adding an "always true" callback.

As GENPD_FLAG_ACTIVE_WAKEUP exists in pm/linux-next only, and this
series is a dependency for the removal of workarounds in drivers of
multiple subsystems (net, irqchip, and gpio), I think it is a good idea
to still queue this for v4.15 in the PM tree, if possible.

This has been tested on r8a73a4/ape6evm, r8a7740/armadillo,
r8a7791/koelsch, r8a7795/salvator-x and -xs, r8a7795/salvator-x, and
sh73a0/kzm9g.

Thanks for applying!

Geert Uytterhoeven (3):
  clk: renesas: mstp: Keep wakeup sources active during system suspend
  clk: renesas: cpg-mssr: Keep wakeup sources active during system
suspend
  soc: renesas: rcar-sysc: Keep wakeup sources active during system
suspend

 drivers/clk/renesas/clk-mstp.c | 2 +-
 drivers/clk/renesas/renesas-cpg-mssr.c | 2 +-
 drivers/soc/renesas/rcar-sysc.c| 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

-- 
2.7.4

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[PATCH v2 3/3] soc: renesas: rcar-sysc: Keep wakeup sources active during system suspend

[Geert Uytterhoeven]

If an R-Car SYSC slave device is part of the CPG/MSTP or CPG/MSSR Clock
Domain and to be used as a wakeup source, it must be kept active during
system suspend.

Currently this is handled in device-specific drivers by explicitly
increasing the use count of the module clock when the device is
configured as a wakeup source.  However, the proper way to prevent the
device from being stopped is to inform this requirement to the genpd
core, by setting the GENPD_FLAG_ACTIVE_WAKEUP flag.

Note that this will only affect devices configured as wakeup sources.

Signed-off-by: Geert Uytterhoeven 
---
v2:
  - Integrate "soc: renesas: rcar-sysc: Use GENPD_FLAG_ACTIVE_WAKEUP",
---
 drivers/soc/renesas/rcar-sysc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/renesas/rcar-sysc.c b/drivers/soc/renesas/rcar-sysc.c
index c8406e81640f6560..ac4df1c43b2fbdbe 100644
--- a/drivers/soc/renesas/rcar-sysc.c
+++ b/drivers/soc/renesas/rcar-sysc.c
@@ -224,7 +224,7 @@ static void __init rcar_sysc_pd_setup(struct rcar_sysc_pd 
*pd)
 
if (!(pd->flags & (PD_CPU | PD_SCU))) {
/* Enable Clock Domain for I/O devices */
-   genpd->flags |= GENPD_FLAG_PM_CLK;
+   genpd->flags |= GENPD_FLAG_PM_CLK | GENPD_FLAG_ACTIVE_WAKEUP;
if (has_cpg_mstp) {
genpd->attach_dev = cpg_mstp_attach_dev;
genpd->detach_dev = cpg_mstp_detach_dev;
-- 
2.7.4

Re: [PATCH v2] gpio: rcar: Add r8a77995 (R-Car D3) support

[Linus Walleij]

On Thu, Nov 9, 2017 at 11:39 AM, Geert Uytterhoeven
 wrote:

> From: Yoshihiro Shimoda 
>
> This patch adds binding for r8a77995 (R-Car D3). This SoC can use
> "renesas,rcar-gen3-gpio" fallback compatibility. So, this patch
> doesn't modify the gpio-rcar driver.
>
> Signed-off-by: Yoshihiro Shimoda 
> Acked-by: Simon Horman 
> Acked-by: Rob Herring 
> Signed-off-by: Geert Uytterhoeven 
> ---
> Linus: it seems this one fell through the cracks, as the later
> f76a2d9d7f9524c5 ("gpio-rcar: document R8A77970 bindings") did get
> applied.
>
> Thx!
>
> v2:
>   - Add Acked-by, rebased.

Oopsie. Maybe my oversight.

Patch applied!

Yours,
Linus Walleij

Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Rafael J. Wysocki]

On Thu, Nov 9, 2017 at 12:59 PM, Rafael J. Wysocki  wrote:
> On Thu, Nov 9, 2017 at 11:08 AM, Ulf Hansson  wrote:
>> On 9 November 2017 at 10:02, Geert Uytterhoeven  wrote:
>>> Hi Ulf,
>>>
>>> On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson  wrote:
 On 8 November 2017 at 16:41, Geert Uytterhoeven  
 wrote:
> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson  
> wrote:
>> The generic problem this series is trying to solve, is that for some bus 
>> types
>> and PM domains, it's not sufficient to only check the return value from
>> device_may_wakeup(), to fully understand how to treat the device during 
>> system
>> suspend.
>>
>> One particular case that suffers from this, is the generic PM domain 
>> (aka genpd)
>> and that is taken care of in the final change in this series.
>>
>> The special case this series address, is to enable drivers to instruct 
>> bus types
>> and PM domains, that the device need to stay powered in case wakeup 
>> signals
>> is enabled for it.
>>>
>> Geert Uytterhoeven, has been working on some related problems for some 
>> Renesas
>> SoCs [1], to be able to properly configure WakeOnLAN, for some ethernet
>> devices/drivers, which are used together with genpd. My intent is that 
>> this
>> series enables a solution for those problems.
>>
>> [1]
>> https://www.spinics.net/lists/linux-renesas-soc/msg19319.html
>
> While your new WAKEUP_POWERED definitely serves a purpose, I don't think
> it's the right solution for the Renesas SoCs.  I can just set the recently
> added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
> drivers to fix the issue for all Renesas drivers.  After all, all devices 
> in
> the clock/power domain must be kept enabled if they're a wakeup source, or
> part of the wakeup path.

 Right, that would work! However, to me, I don't think it's fine grained 
 enough.

 Let's take the Ethernet device/driver using WoL as an example, similar
 to your cases.

 First, let's assume device_may_wakeup() returns true, meaning that the
 Ethernet device is wakeup capable and that userspace has requested
 wakeup to be enabled.

 Then we have three scenarios to consider when the Ethernet driver
 becomes suspended (typically when its ->suspend() callback gets
 invoked).
 1) The Ethernet interface is down.
 2) The Ethernet interface is up, but no connection established.
 3) The Ethernet interface is up, connection established.

 By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean
 that we can't distinguish between any of the the scenarios above, but
 instead always keep the Ethernet device powered on and thus the PM
 domain also.

 In the more fine grained solution, we can change the Ethernet driver
 to consider under what scenario it's being suspended. For 1) and 2),
 there is no need to keep the Ethernet device being powered, but
 instead only enable WoL in 3) - via also using the WAKEUP_POWERED
 flag.
>>>
>>> The Ethernet driver can still call device_set_wakeup_enable(... , false)
>>> to control this.  If WoL is disabled by the user (or deemed not usable, see
>>> below), it already does so.
>>
>> I don't think that API is intended to be used like that and I wonder
>> if it even works as expected.
>>
>> Quoting the doc:
>> "If device wakeup mechanisms are enabled or disabled directly by
>> drivers, they also should use :c:func:`device_may_wakeup()` to decide what 
>> to do
>> during a system sleep transition.  Device drivers, however, are not expected 
>> to
>> call :c:func:`device_set_wakeup_enable()` directly in any case."
>>
>> Rafael, can you comment on this?
>
> Well, it means what it says.
>
> If you call device_set_wakeup_enable() from a driver, user space will
> see a change in sysfs and may be confused by that and that's why doing
> so is not recommended.
>
> Not that people listen to recommendations too much, though. :-)

But setting up WoL via ethtool from user space is an exception,
because user space actually *does* expect to see a change in sysfs in
this particular case.

It's basically two different ways to change the same setting and both
should result in the same behavior, ideally.

Thanks,
Rafael

Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Rafael J. Wysocki]

On Thu, Nov 9, 2017 at 11:08 AM, Ulf Hansson  wrote:
> On 9 November 2017 at 10:02, Geert Uytterhoeven  wrote:
>> Hi Ulf,
>>
>> On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson  wrote:
>>> On 8 November 2017 at 16:41, Geert Uytterhoeven  
>>> wrote:
 On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson  wrote:
> The generic problem this series is trying to solve, is that for some bus 
> types
> and PM domains, it's not sufficient to only check the return value from
> device_may_wakeup(), to fully understand how to treat the device during 
> system
> suspend.
>
> One particular case that suffers from this, is the generic PM domain (aka 
> genpd)
> and that is taken care of in the final change in this series.
>
> The special case this series address, is to enable drivers to instruct 
> bus types
> and PM domains, that the device need to stay powered in case wakeup 
> signals
> is enabled for it.
>>
> Geert Uytterhoeven, has been working on some related problems for some 
> Renesas
> SoCs [1], to be able to properly configure WakeOnLAN, for some ethernet
> devices/drivers, which are used together with genpd. My intent is that 
> this
> series enables a solution for those problems.
>
> [1]
> https://www.spinics.net/lists/linux-renesas-soc/msg19319.html

 While your new WAKEUP_POWERED definitely serves a purpose, I don't think
 it's the right solution for the Renesas SoCs.  I can just set the recently
 added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
 drivers to fix the issue for all Renesas drivers.  After all, all devices 
 in
 the clock/power domain must be kept enabled if they're a wakeup source, or
 part of the wakeup path.
>>>
>>> Right, that would work! However, to me, I don't think it's fine grained 
>>> enough.
>>>
>>> Let's take the Ethernet device/driver using WoL as an example, similar
>>> to your cases.
>>>
>>> First, let's assume device_may_wakeup() returns true, meaning that the
>>> Ethernet device is wakeup capable and that userspace has requested
>>> wakeup to be enabled.
>>>
>>> Then we have three scenarios to consider when the Ethernet driver
>>> becomes suspended (typically when its ->suspend() callback gets
>>> invoked).
>>> 1) The Ethernet interface is down.
>>> 2) The Ethernet interface is up, but no connection established.
>>> 3) The Ethernet interface is up, connection established.
>>>
>>> By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean
>>> that we can't distinguish between any of the the scenarios above, but
>>> instead always keep the Ethernet device powered on and thus the PM
>>> domain also.
>>>
>>> In the more fine grained solution, we can change the Ethernet driver
>>> to consider under what scenario it's being suspended. For 1) and 2),
>>> there is no need to keep the Ethernet device being powered, but
>>> instead only enable WoL in 3) - via also using the WAKEUP_POWERED
>>> flag.
>>
>> The Ethernet driver can still call device_set_wakeup_enable(... , false)
>> to control this.  If WoL is disabled by the user (or deemed not usable, see
>> below), it already does so.
>
> I don't think that API is intended to be used like that and I wonder
> if it even works as expected.
>
> Quoting the doc:
> "If device wakeup mechanisms are enabled or disabled directly by
> drivers, they also should use :c:func:`device_may_wakeup()` to decide what to 
> do
> during a system sleep transition.  Device drivers, however, are not expected 
> to
> call :c:func:`device_set_wakeup_enable()` directly in any case."
>
> Rafael, can you comment on this?

Well, it means what it says.

If you call device_set_wakeup_enable() from a driver, user space will
see a change in sysfs and may be confused by that and that's why doing
so is not recommended.

Not that people listen to recommendations too much, though. :-)

>>
>> In addition, keeping WoL enabled for cases 1 and 2 may be desirable
>> (e.g allow wake-up if a cable is plugged in during system suspend and
>>  a magic packet is received afterwards), depending on the hardware.
>> But the driver can already control that through device_set_wakeup_enable().
>>
>
> Ehh, that sounds weird. :-) If the Ethernet interface is down, how
> would such packet be received?

In PCI NICs, if wakeup power is provided, the NIC can detect activity
on the port and generate a PCI PME even if the I/F is down otherwise.

Thanks,
Rafael

Re: [PATCH 2/3] PM / core: Add WAKEUP_POWERED driver flag

[Rafael J. Wysocki]

On Thu, Nov 9, 2017 at 9:53 AM, Ulf Hansson  wrote:
> On 9 November 2017 at 01:41, Rafael J. Wysocki  wrote:
>> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson  wrote:
>>> For some bus types and PM domains, it's not sufficient to only check the
>>> return value from device_may_wakeup(), to fully understand how to treat the
>>> device during system suspend.
>>>
>>> In particular, sometimes the device may need to stay in full power state,
>>> to have wakeup signals enabled for it. Therefore, define and document a
>>> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
>>> exactly about that.
>>>
>>> During __device_suspend() in the PM core, let's make sure to also propagate
>>> the setting of the flag to the parent device, when wakeup signals are
>>> enabled and unless the parent has the "ignore_children" flag set. This
>>> makes it also consistent with how the existing "wakeup_path" flag is being
>>> assigned.
>>>
>>> Signed-off-by: Ulf Hansson 
>>> ---
>>>  Documentation/driver-api/pm/devices.rst | 12 
>>>  drivers/base/power/main.c   |  6 +-
>>>  include/linux/pm.h  |  5 +
>>>  3 files changed, 22 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/driver-api/pm/devices.rst 
>>> b/Documentation/driver-api/pm/devices.rst
>>> index 53c1b0b..1ca2d0f 100644
>>> --- a/Documentation/driver-api/pm/devices.rst
>>> +++ b/Documentation/driver-api/pm/devices.rst
>>> @@ -414,6 +414,18 @@ when the system is in the sleep state.  For example, 
>>> :c:func:`enable_irq_wake()`
>>>  might identify GPIO signals hooked up to a switch or other external 
>>> hardware,
>>>  and :c:func:`pci_enable_wake()` does something similar for the PCI PME 
>>> signal.
>>>
>>> +Moreover, in case wakeup signals are enabled for a device, some bus types 
>>> and
>>> +PM domains may manage the device slightly differently during system 
>>> suspend. For
>>> +example, sometimes the device needs to stay in full power state, to have 
>>> wakeup
>>> +signals enabled for it. In cases when the wakeup settings are mostly 
>>> managed by
>>> +the driver, it may not be sufficient for bus types and PM domains to only 
>>> check
>>> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what 
>>> actions
>>> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
>>> +:c:member:`power.driver_flags`, by passing the flag to
>>> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
>>> +domains to leave the device in full power state, when wakeup signals are 
>>> enabled
>>> +for it.
>>> +
>>>  If any of these callbacks returns an error, the system won't enter the 
>>> desired
>>>  low-power state.  Instead, the PM core will unwind its actions by resuming 
>>> all
>>>  the devices that were suspended.
>>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>>> index 8089e72..f64f945 100644
>>> --- a/drivers/base/power/main.c
>>> +++ b/drivers/base/power/main.c
>>> @@ -1432,9 +1432,13 @@ static void dpm_propagate_to_parent(struct device 
>>> *dev)
>>> spin_lock_irq(&parent->power.lock);
>>>
>>> parent->power.direct_complete = false;
>>> -   if (dev->power.wakeup_path && !parent->power.ignore_children)
>>> +   if (dev->power.wakeup_path && !parent->power.ignore_children) {
>>> parent->power.wakeup_path = true;
>>>
>>> +   if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED))
>>> +   parent->power.driver_flags |= 
>>> DPM_FLAG_WAKEUP_POWERED;
>>
>> No, sorry.
>>
>> The flag cannot be propagated this way, because that effectively
>> overrides the choices made by the parent driver and up.
>
> Yes, but that is the hole point.
>
> If a child device needs to stay powered as to deal with wakeup, so is
> required by the parent.

So you need a flag and a status bit.

The flag says what the driver wants (and what it wants for a
particular device) and the status bit reflects the current situation
(taking dependencies into account).

Say a device has two children, A and B, and both of them have the new flag set.

If either A or B is configured for system wakeup, power should not be
removed from the parent.  However, if neither A nor B is configured
for system wakeup, power can be removed from the parent on suspend
unless the parent driver itself has set the new flag.
Thus setting the new flag by the child drivers alone doesn't imply the
specific handling of the parent unless additional conditions occur.

That can be represented by a status bit that will be set or unset on
every suspend individually taking the current configuration into
account every time.

>>
>> Besides, wakeup_path already had a similar purpose.  What has happened to 
>> that?
>
> Yes, but wakeup_path is only telling half of what is needed.
>
> Because even if wakeup_path becomes set for a parent device, doesn't
> mean that it must stay in full power during system suspend to serve
>

Re: [PATCH 2/3] PM / core: Add WAKEUP_POWERED driver flag

[Rafael J. Wysocki]

On Thu, Nov 9, 2017 at 9:44 AM, Ulf Hansson  wrote:
> On 9 November 2017 at 01:24, Rafael J. Wysocki  wrote:
>> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson  wrote:
>>> For some bus types and PM domains, it's not sufficient to only check the
>>> return value from device_may_wakeup(), to fully understand how to treat the
>>> device during system suspend.
>>>
>>> In particular, sometimes the device may need to stay in full power state,
>>> to have wakeup signals enabled for it. Therefore, define and document a
>>> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
>>> exactly about that.
>>>
>>> During __device_suspend() in the PM core, let's make sure to also propagate
>>> the setting of the flag to the parent device, when wakeup signals are
>>> enabled and unless the parent has the "ignore_children" flag set. This
>>> makes it also consistent with how the existing "wakeup_path" flag is being
>>> assigned.
>>>
>>> Signed-off-by: Ulf Hansson 
>>> ---
>>>  Documentation/driver-api/pm/devices.rst | 12 
>>>  drivers/base/power/main.c   |  6 +-
>>>  include/linux/pm.h  |  5 +
>>>  3 files changed, 22 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/driver-api/pm/devices.rst 
>>> b/Documentation/driver-api/pm/devices.rst
>>> index 53c1b0b..1ca2d0f 100644
>>> --- a/Documentation/driver-api/pm/devices.rst
>>> +++ b/Documentation/driver-api/pm/devices.rst
>>> @@ -414,6 +414,18 @@ when the system is in the sleep state.  For example, 
>>> :c:func:`enable_irq_wake()`
>>>  might identify GPIO signals hooked up to a switch or other external 
>>> hardware,
>>>  and :c:func:`pci_enable_wake()` does something similar for the PCI PME 
>>> signal.
>>>
>>> +Moreover, in case wakeup signals are enabled for a device, some bus types 
>>> and
>>> +PM domains may manage the device slightly differently during system 
>>> suspend. For
>>> +example, sometimes the device needs to stay in full power state, to have 
>>> wakeup
>>> +signals enabled for it. In cases when the wakeup settings are mostly 
>>> managed by
>>> +the driver, it may not be sufficient for bus types and PM domains to only 
>>> check
>>> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what 
>>> actions
>>> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
>>> +:c:member:`power.driver_flags`, by passing the flag to
>>> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
>>> +domains to leave the device in full power state, when wakeup signals are 
>>> enabled
>>> +for it.
>>
>> IMO this is a bit unclear.
>>
>> First off, how does the driver know that the device has to be in full
>> power for wakeup to work?
>
> Because the device is accessed as part of dealing with the wakeup.

Yes, it is, In the working state of the system.  In the system wakeup
case it may not be.

Essentially, what happens then is that driver-provided interrupt
handlers don't run as a rule and system wakeup is triggered by the
low-level handler at the IRQ chip level.  Next, the PM callbacks
invoked for the device are expected to clean up the wakeup status etc.

Of course, power still is necessary for that to work, but it may be
not be in-band.  That may be either in-band power used for normal
operations and provided through the interconnect used by the device or
it may be special wakeup power provided out-of-band.

Also the wakeup signal itself may be an in-band device interrupt (like
the ones used for signaling IO events during normal operation) or it
may be a special wakeup signal (like PCI PME) in which case, from the
driver's perspective, the wakeup signaling is out-of-band.

Usually, the driver doesn't know how this is set up for the particular
device in the particular platform and hence my question. :-)

The case at hand seems to be when the wakeup power is in-band or the
wakeup signal is an in-band interrupt (in which case power needs to be
provided to the interconnect at least).

If they both are out-of-band, the middle layer should know that,
because as a rule it will be involved in setting up both of them.
Otherwise, in principle, it should assume that in-band power needs to
be provided for wakeup to work and avoid turning things off if wakeup
is enabled.

>>
>> Second, this requirement sort of implies that the device cannot go
>> into a low-power state during runtime suspend too, so it basically
>> remains stays at full power even when runtime-suspended.
>
> No, not really, because that depends on the current situation.
>
> An Ethernet device can surely go into a low power state, at runtime
> suspend, when the interface is down, for example.

But then it is not expected to generate wakeup signals I suppose.

[BTW, I wonder how it detects when the cable is connected again to it.
I know what happens in PCI NICs, but that clearly is not the case
here.]

Well, anyway, it looks like the case when the device is
runtime-suspended right before

[PATCH v2] gpio: rcar: Add r8a77995 (R-Car D3) support

[Geert Uytterhoeven]

From: Yoshihiro Shimoda 

This patch adds binding for r8a77995 (R-Car D3). This SoC can use
"renesas,rcar-gen3-gpio" fallback compatibility. So, this patch
doesn't modify the gpio-rcar driver.

Signed-off-by: Yoshihiro Shimoda 
Acked-by: Simon Horman 
Acked-by: Rob Herring 
Signed-off-by: Geert Uytterhoeven 
---
Linus: it seems this one fell through the cracks, as the later
f76a2d9d7f9524c5 ("gpio-rcar: document R8A77970 bindings") did get
applied.

Thx!

v2:
  - Add Acked-by, rebased.
---
 Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt 
b/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
index 41137a1cc099b7e7..a7ac460ad6572526 100644
--- a/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
+++ b/Documentation/devicetree/bindings/gpio/renesas,gpio-rcar.txt
@@ -15,6 +15,7 @@ Required Properties:
 - "renesas,gpio-r8a7795": for R8A7795 (R-Car H3) compatible GPIO 
controller.
 - "renesas,gpio-r8a7796": for R8A7796 (R-Car M3-W) compatible GPIO 
controller.
 - "renesas,gpio-r8a77970": for R8A77970 (R-Car V3M) compatible GPIO 
controller.
+- "renesas,gpio-r8a77995": for R8A77995 (R-Car D3) compatible GPIO 
controller.
 - "renesas,rcar-gen1-gpio": for a generic R-Car Gen1 GPIO controller.
 - "renesas,rcar-gen2-gpio": for a generic R-Car Gen2 or RZ/G1 GPIO 
controller.
 - "renesas,rcar-gen3-gpio": for a generic R-Car Gen3 GPIO controller.
-- 
2.7.4

Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Geert Uytterhoeven]

Hi Ulf,

On Thu, Nov 9, 2017 at 11:08 AM, Ulf Hansson  wrote:
> On 9 November 2017 at 10:02, Geert Uytterhoeven  wrote:
>> On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson  wrote:
>>> On 8 November 2017 at 16:41, Geert Uytterhoeven  
>>> wrote:
 On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson  wrote:
> The generic problem this series is trying to solve, is that for some bus 
> types
> and PM domains, it's not sufficient to only check the return value from
> device_may_wakeup(), to fully understand how to treat the device during 
> system
> suspend.
>
> One particular case that suffers from this, is the generic PM domain (aka 
> genpd)
> and that is taken care of in the final change in this series.
>
> The special case this series address, is to enable drivers to instruct 
> bus types
> and PM domains, that the device need to stay powered in case wakeup 
> signals
> is enabled for it.
>>
> Geert Uytterhoeven, has been working on some related problems for some 
> Renesas
> SoCs [1], to be able to properly configure WakeOnLAN, for some ethernet
> devices/drivers, which are used together with genpd. My intent is that 
> this
> series enables a solution for those problems.
>
> [1]
> https://www.spinics.net/lists/linux-renesas-soc/msg19319.html

 While your new WAKEUP_POWERED definitely serves a purpose, I don't think
 it's the right solution for the Renesas SoCs.  I can just set the recently
 added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
 drivers to fix the issue for all Renesas drivers.  After all, all devices 
 in
 the clock/power domain must be kept enabled if they're a wakeup source, or
 part of the wakeup path.
>>>
>>> Right, that would work! However, to me, I don't think it's fine grained 
>>> enough.
>>>
>>> Let's take the Ethernet device/driver using WoL as an example, similar
>>> to your cases.
>>>
>>> First, let's assume device_may_wakeup() returns true, meaning that the
>>> Ethernet device is wakeup capable and that userspace has requested
>>> wakeup to be enabled.
>>>
>>> Then we have three scenarios to consider when the Ethernet driver
>>> becomes suspended (typically when its ->suspend() callback gets
>>> invoked).
>>> 1) The Ethernet interface is down.
>>> 2) The Ethernet interface is up, but no connection established.
>>> 3) The Ethernet interface is up, connection established.
>>>
>>> By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean
>>> that we can't distinguish between any of the the scenarios above, but
>>> instead always keep the Ethernet device powered on and thus the PM
>>> domain also.
>>>
>>> In the more fine grained solution, we can change the Ethernet driver
>>> to consider under what scenario it's being suspended. For 1) and 2),
>>> there is no need to keep the Ethernet device being powered, but
>>> instead only enable WoL in 3) - via also using the WAKEUP_POWERED
>>> flag.
>>
>> The Ethernet driver can still call device_set_wakeup_enable(... , false)
>> to control this.  If WoL is disabled by the user (or deemed not usable, see
>> below), it already does so.
>
> I don't think that API is intended to be used like that and I wonder
> if it even works as expected.
>
> Quoting the doc:
> "If device wakeup mechanisms are enabled or disabled directly by
> drivers, they also should use :c:func:`device_may_wakeup()` to decide what to 
> do
> during a system sleep transition.  Device drivers, however, are not expected 
> to
> call :c:func:`device_set_wakeup_enable()` directly in any case."
>
> Rafael, can you comment on this?

There are ca. 100 callers in drivers.

>> In addition, keeping WoL enabled for cases 1 and 2 may be desirable
>> (e.g allow wake-up if a cable is plugged in during system suspend and
>>  a magic packet is received afterwards), depending on the hardware.
>> But the driver can already control that through device_set_wakeup_enable().
>
> Ehh, that sounds weird. :-) If the Ethernet interface is down, how
> would such packet be received?

It depends on your meaning of "up".  My interpretation is that "up" means
ready to handle packets between physical media and the Linux networking stack.

So even when "down", the actual Ethernet controller may still be able to
receive a magic packet if WoL is enabled.  The magic packet is really a
magic packet not intended to be transmitted to the networking stack, but
merely serves as a wakeup signal.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Ulf Hansson]

On 9 November 2017 at 10:02, Geert Uytterhoeven  wrote:
> Hi Ulf,
>
> On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson  wrote:
>> On 8 November 2017 at 16:41, Geert Uytterhoeven  wrote:
>>> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson  wrote:
 The generic problem this series is trying to solve, is that for some bus 
 types
 and PM domains, it's not sufficient to only check the return value from
 device_may_wakeup(), to fully understand how to treat the device during 
 system
 suspend.

 One particular case that suffers from this, is the generic PM domain (aka 
 genpd)
 and that is taken care of in the final change in this series.

 The special case this series address, is to enable drivers to instruct bus 
 types
 and PM domains, that the device need to stay powered in case wakeup signals
 is enabled for it.
>
 Geert Uytterhoeven, has been working on some related problems for some 
 Renesas
 SoCs [1], to be able to properly configure WakeOnLAN, for some ethernet
 devices/drivers, which are used together with genpd. My intent is that this
 series enables a solution for those problems.

 [1]
 https://www.spinics.net/lists/linux-renesas-soc/msg19319.html
>>>
>>> While your new WAKEUP_POWERED definitely serves a purpose, I don't think
>>> it's the right solution for the Renesas SoCs.  I can just set the recently
>>> added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
>>> drivers to fix the issue for all Renesas drivers.  After all, all devices in
>>> the clock/power domain must be kept enabled if they're a wakeup source, or
>>> part of the wakeup path.
>>
>> Right, that would work! However, to me, I don't think it's fine grained 
>> enough.
>>
>> Let's take the Ethernet device/driver using WoL as an example, similar
>> to your cases.
>>
>> First, let's assume device_may_wakeup() returns true, meaning that the
>> Ethernet device is wakeup capable and that userspace has requested
>> wakeup to be enabled.
>>
>> Then we have three scenarios to consider when the Ethernet driver
>> becomes suspended (typically when its ->suspend() callback gets
>> invoked).
>> 1) The Ethernet interface is down.
>> 2) The Ethernet interface is up, but no connection established.
>> 3) The Ethernet interface is up, connection established.
>>
>> By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean
>> that we can't distinguish between any of the the scenarios above, but
>> instead always keep the Ethernet device powered on and thus the PM
>> domain also.
>>
>> In the more fine grained solution, we can change the Ethernet driver
>> to consider under what scenario it's being suspended. For 1) and 2),
>> there is no need to keep the Ethernet device being powered, but
>> instead only enable WoL in 3) - via also using the WAKEUP_POWERED
>> flag.
>
> The Ethernet driver can still call device_set_wakeup_enable(... , false)
> to control this.  If WoL is disabled by the user (or deemed not usable, see
> below), it already does so.

I don't think that API is intended to be used like that and I wonder
if it even works as expected.

Quoting the doc:
"If device wakeup mechanisms are enabled or disabled directly by
drivers, they also should use :c:func:`device_may_wakeup()` to decide what to do
during a system sleep transition.  Device drivers, however, are not expected to
call :c:func:`device_set_wakeup_enable()` directly in any case."

Rafael, can you comment on this?

>
> In addition, keeping WoL enabled for cases 1 and 2 may be desirable
> (e.g allow wake-up if a cable is plugged in during system suspend and
>  a magic packet is received afterwards), depending on the hardware.
> But the driver can already control that through device_set_wakeup_enable().
>

Ehh, that sounds weird. :-) If the Ethernet interface is down, how
would such packet be received?

Kind regards
Uffe

Re: [PATCH 1/2] ARM: dts: r8a7745: Add DU support

[Laurent Pinchart]

Hi Simon,

On Wednesday, 8 November 2017 06:48:23 EET Simon Horman wrote:
> On Tue, Nov 07, 2017 at 06:05:37AM +0200, Laurent Pinchart wrote:
> > Hi Fabrizio,
> > 
> > Thank you for the patch.
> > 
> > On Monday, 6 November 2017 20:26:53 EET Fabrizio Castro wrote:
> > > Add du node to r8a7745 SoC DT. Boards that want to enable the DU
> > > need to specify the output topology.
> > > 
> > > Signed-off-by: Fabrizio Castro 
> > > Reviewed-by: Biju Das 
> > 
> > Reviewed-by: Laurent Pinchart 
> > 
> > > ---
> > > 
> > >  arch/arm/boot/dts/r8a7745.dtsi | 27 +++
> > >  1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/r8a7745.dtsi
> > > b/arch/arm/boot/dts/r8a7745.dtsi index 846c27a..53eb1ce4 100644
> > > --- a/arch/arm/boot/dts/r8a7745.dtsi
> > > +++ b/arch/arm/boot/dts/r8a7745.dtsi
> > > @@ -821,6 +821,33 @@
> > > 
> > >   status = "disabled";
> > >   
> > >   };
> > > 
> > > + du: display@feb0 {
> > > + compatible = "renesas,du-r8a7745";
> 
> Hi,
> 
> could you provide some information on the status of upstreaming
> the binding used above?

I'll send a pull request for v4.16, it's too late for v4.15.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Geert Uytterhoeven]

Hi Ulf,

On Thu, Nov 9, 2017 at 9:28 AM, Ulf Hansson  wrote:
> On 8 November 2017 at 16:41, Geert Uytterhoeven  wrote:
>> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson  wrote:
>>> The generic problem this series is trying to solve, is that for some bus 
>>> types
>>> and PM domains, it's not sufficient to only check the return value from
>>> device_may_wakeup(), to fully understand how to treat the device during 
>>> system
>>> suspend.
>>>
>>> One particular case that suffers from this, is the generic PM domain (aka 
>>> genpd)
>>> and that is taken care of in the final change in this series.
>>>
>>> The special case this series address, is to enable drivers to instruct bus 
>>> types
>>> and PM domains, that the device need to stay powered in case wakeup signals
>>> is enabled for it.

>>> Geert Uytterhoeven, has been working on some related problems for some 
>>> Renesas
>>> SoCs [1], to be able to properly configure WakeOnLAN, for some ethernet
>>> devices/drivers, which are used together with genpd. My intent is that this
>>> series enables a solution for those problems.
>>>
>>> [1]
>>> https://www.spinics.net/lists/linux-renesas-soc/msg19319.html
>>
>> While your new WAKEUP_POWERED definitely serves a purpose, I don't think
>> it's the right solution for the Renesas SoCs.  I can just set the recently
>> added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
>> drivers to fix the issue for all Renesas drivers.  After all, all devices in
>> the clock/power domain must be kept enabled if they're a wakeup source, or
>> part of the wakeup path.
>
> Right, that would work! However, to me, I don't think it's fine grained 
> enough.
>
> Let's take the Ethernet device/driver using WoL as an example, similar
> to your cases.
>
> First, let's assume device_may_wakeup() returns true, meaning that the
> Ethernet device is wakeup capable and that userspace has requested
> wakeup to be enabled.
>
> Then we have three scenarios to consider when the Ethernet driver
> becomes suspended (typically when its ->suspend() callback gets
> invoked).
> 1) The Ethernet interface is down.
> 2) The Ethernet interface is up, but no connection established.
> 3) The Ethernet interface is up, connection established.
>
> By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean
> that we can't distinguish between any of the the scenarios above, but
> instead always keep the Ethernet device powered on and thus the PM
> domain also.
>
> In the more fine grained solution, we can change the Ethernet driver
> to consider under what scenario it's being suspended. For 1) and 2),
> there is no need to keep the Ethernet device being powered, but
> instead only enable WoL in 3) - via also using the WAKEUP_POWERED
> flag.

The Ethernet driver can still call device_set_wakeup_enable(... , false)
to control this.  If WoL is disabled by the user (or deemed not usable, see
below), it already does so.

In addition, keeping WoL enabled for cases 1 and 2 may be desirable
(e.g allow wake-up if a cable is plugged in during system suspend and
 a magic packet is received afterwards), depending on the hardware.
But the driver can already control that through device_set_wakeup_enable().

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 2/3] PM / core: Add WAKEUP_POWERED driver flag

[Ulf Hansson]

On 9 November 2017 at 01:41, Rafael J. Wysocki  wrote:
> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson  wrote:
>> For some bus types and PM domains, it's not sufficient to only check the
>> return value from device_may_wakeup(), to fully understand how to treat the
>> device during system suspend.
>>
>> In particular, sometimes the device may need to stay in full power state,
>> to have wakeup signals enabled for it. Therefore, define and document a
>> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
>> exactly about that.
>>
>> During __device_suspend() in the PM core, let's make sure to also propagate
>> the setting of the flag to the parent device, when wakeup signals are
>> enabled and unless the parent has the "ignore_children" flag set. This
>> makes it also consistent with how the existing "wakeup_path" flag is being
>> assigned.
>>
>> Signed-off-by: Ulf Hansson 
>> ---
>>  Documentation/driver-api/pm/devices.rst | 12 
>>  drivers/base/power/main.c   |  6 +-
>>  include/linux/pm.h  |  5 +
>>  3 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/driver-api/pm/devices.rst 
>> b/Documentation/driver-api/pm/devices.rst
>> index 53c1b0b..1ca2d0f 100644
>> --- a/Documentation/driver-api/pm/devices.rst
>> +++ b/Documentation/driver-api/pm/devices.rst
>> @@ -414,6 +414,18 @@ when the system is in the sleep state.  For example, 
>> :c:func:`enable_irq_wake()`
>>  might identify GPIO signals hooked up to a switch or other external 
>> hardware,
>>  and :c:func:`pci_enable_wake()` does something similar for the PCI PME 
>> signal.
>>
>> +Moreover, in case wakeup signals are enabled for a device, some bus types 
>> and
>> +PM domains may manage the device slightly differently during system 
>> suspend. For
>> +example, sometimes the device needs to stay in full power state, to have 
>> wakeup
>> +signals enabled for it. In cases when the wakeup settings are mostly 
>> managed by
>> +the driver, it may not be sufficient for bus types and PM domains to only 
>> check
>> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what 
>> actions
>> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
>> +:c:member:`power.driver_flags`, by passing the flag to
>> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
>> +domains to leave the device in full power state, when wakeup signals are 
>> enabled
>> +for it.
>> +
>>  If any of these callbacks returns an error, the system won't enter the 
>> desired
>>  low-power state.  Instead, the PM core will unwind its actions by resuming 
>> all
>>  the devices that were suspended.
>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> index 8089e72..f64f945 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -1432,9 +1432,13 @@ static void dpm_propagate_to_parent(struct device 
>> *dev)
>> spin_lock_irq(&parent->power.lock);
>>
>> parent->power.direct_complete = false;
>> -   if (dev->power.wakeup_path && !parent->power.ignore_children)
>> +   if (dev->power.wakeup_path && !parent->power.ignore_children) {
>> parent->power.wakeup_path = true;
>>
>> +   if (dev_pm_test_driver_flags(dev, DPM_FLAG_WAKEUP_POWERED))
>> +   parent->power.driver_flags |= 
>> DPM_FLAG_WAKEUP_POWERED;
>
> No, sorry.
>
> The flag cannot be propagated this way, because that effectively
> overrides the choices made by the parent driver and up.

Yes, but that is the hole point.

If a child device needs to stay powered as to deal with wakeup, so is
required by the parent.

>
> Besides, wakeup_path already had a similar purpose.  What has happened to 
> that?

Yes, but wakeup_path is only telling half of what is needed.

Because even if wakeup_path becomes set for a parent device, doesn't
mean that it must stay in full power during system suspend to serve
wakeups for a child. That's why I think the DPM_FLAG_WAKEUP_POWERED
flag needs to be propagated also.

Kind regards
Uffe

Re: [PATCH 2/3] PM / core: Add WAKEUP_POWERED driver flag

[Ulf Hansson]

On 9 November 2017 at 01:24, Rafael J. Wysocki  wrote:
> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson  wrote:
>> For some bus types and PM domains, it's not sufficient to only check the
>> return value from device_may_wakeup(), to fully understand how to treat the
>> device during system suspend.
>>
>> In particular, sometimes the device may need to stay in full power state,
>> to have wakeup signals enabled for it. Therefore, define and document a
>> WAKEUP_POWERED flag, to enable drivers to instruct bus types and PM domains
>> exactly about that.
>>
>> During __device_suspend() in the PM core, let's make sure to also propagate
>> the setting of the flag to the parent device, when wakeup signals are
>> enabled and unless the parent has the "ignore_children" flag set. This
>> makes it also consistent with how the existing "wakeup_path" flag is being
>> assigned.
>>
>> Signed-off-by: Ulf Hansson 
>> ---
>>  Documentation/driver-api/pm/devices.rst | 12 
>>  drivers/base/power/main.c   |  6 +-
>>  include/linux/pm.h  |  5 +
>>  3 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/driver-api/pm/devices.rst 
>> b/Documentation/driver-api/pm/devices.rst
>> index 53c1b0b..1ca2d0f 100644
>> --- a/Documentation/driver-api/pm/devices.rst
>> +++ b/Documentation/driver-api/pm/devices.rst
>> @@ -414,6 +414,18 @@ when the system is in the sleep state.  For example, 
>> :c:func:`enable_irq_wake()`
>>  might identify GPIO signals hooked up to a switch or other external 
>> hardware,
>>  and :c:func:`pci_enable_wake()` does something similar for the PCI PME 
>> signal.
>>
>> +Moreover, in case wakeup signals are enabled for a device, some bus types 
>> and
>> +PM domains may manage the device slightly differently during system 
>> suspend. For
>> +example, sometimes the device needs to stay in full power state, to have 
>> wakeup
>> +signals enabled for it. In cases when the wakeup settings are mostly 
>> managed by
>> +the driver, it may not be sufficient for bus types and PM domains to only 
>> check
>> +the return value of :c:func:`device_may_wakeup(dev)`, to understand what 
>> actions
>> +are needed. Therefore, drivers can set ``DPM_FLAG_WAKEUP_POWERED`` in
>> +:c:member:`power.driver_flags`, by passing the flag to
>> +:c:func:`dev_pm_set_driver_flags` helper. This instructs bus types and PM
>> +domains to leave the device in full power state, when wakeup signals are 
>> enabled
>> +for it.
>
> IMO this is a bit unclear.
>
> First off, how does the driver know that the device has to be in full
> power for wakeup to work?

Because the device is accessed as part of dealing with the wakeup.

>
> Second, this requirement sort of implies that the device cannot go
> into a low-power state during runtime suspend too, so it basically
> remains stays at full power even when runtime-suspended.

No, not really, because that depends on the current situation.

An Ethernet device can surely go into a low power state, at runtime
suspend, when the interface is down, for example.

>
> Does it then mean that the middle layer is expected to simply avoid
> changing the power state of the device when enabled to wake up the
> system, or is there more to that?  In the former case, it may be
> better to rename the flag to something along the lines of "don't
> change power state if wakeup enabled".

Yes, correct.

I can try to clarify that in the description and unless you have a
suggestion for a better name of the flag, I try to come up with
something for that too.

>>  #define DPM_FLAG_NEVER_SKIPBIT(0)
>>  #define DPM_FLAG_SMART_PREPARE BIT(1)
>>  #define DPM_FLAG_SMART_SUSPEND BIT(2)
>> +#define DPM_FLAG_WAKEUP_POWEREDBIT(3)
>
> I'd prefer this to be BIT(4).

OK.

I guess you can always also amend my patch, depending on in what order
you merge things. :-)

[...]

Kind regards
Uffe

Re: [PATCH 0/3] PM / core: Invent a WAKEUP_POWERED driver flag

[Ulf Hansson]

On 8 November 2017 at 16:41, Geert Uytterhoeven  wrote:
> Hi Ulf,
>
> On Wed, Nov 8, 2017 at 4:15 PM, Ulf Hansson  wrote:
>> The generic problem this series is trying to solve, is that for some bus 
>> types
>> and PM domains, it's not sufficient to only check the return value from
>> device_may_wakeup(), to fully understand how to treat the device during 
>> system
>> suspend.
>>
>> One particular case that suffers from this, is the generic PM domain (aka 
>> genpd)
>> and that is taken care of in the final change in this series.
>>
>> The special case this series address, is to enable drivers to instruct bus 
>> types
>> and PM domains, that the device need to stay powered in case wakeup signals
>> is enabled for it.
>
> Thanks for your patches!
> They look good to me, hence my Reviewed-by.

Hi Geert,

Thanks for reviewing, much appreciated!

>
>> Geert Uytterhoeven, has been working on some related problems for some 
>> Renesas
>> SoCs [1], to be able to properly configure WakeOnLAN, for some ethernet
>> devices/drivers, which are used together with genpd. My intent is that this
>> series enables a solution for those problems.
>>
>> [1]
>> https://www.spinics.net/lists/linux-renesas-soc/msg19319.html
>
> While your new WAKEUP_POWERED definitely serves a purpose, I don't think
> it's the right solution for the Renesas SoCs.  I can just set the recently
> added flag GENPD_FLAG_ACTIVE_WAKEUP in all Renesas clock/power domain
> drivers to fix the issue for all Renesas drivers.  After all, all devices in
> the clock/power domain must be kept enabled if they're a wakeup source, or
> part of the wakeup path.

Right, that would work! However, to me, I don't think it's fine grained enough.

Let's take the Ethernet device/driver using WoL as an example, similar
to your cases.

First, let's assume device_may_wakeup() returns true, meaning that the
Ethernet device is wakeup capable and that userspace has requested
wakeup to be enabled.

Then we have three scenarios to consider when the Ethernet driver
becomes suspended (typically when its ->suspend() callback gets
invoked).
1) The Ethernet interface is down.
2) The Ethernet interface is up, but no connection established.
3) The Ethernet interface is up, connection established.

By following your approach, using GENPD_FLAG_ACTIVE_WAKEUP, would mean
that we can't distinguish between any of the the scenarios above, but
instead always keep the Ethernet device powered on and thus the PM
domain also.

In the more fine grained solution, we can change the Ethernet driver
to consider under what scenario it's being suspended. For 1) and 2),
there is no need to keep the Ethernet device being powered, but
instead only enable WoL in 3) - via also using the WAKEUP_POWERED
flag.

>
> Not using GENPD_FLAG_ACTIVE_WAKEUP means I would have to add the
> WAKEUP_POWERED flag to every single driver that can either be a wakeup
> source itself, or be part of the wakeup path.

Right.

First, is that really that many? Second, nothing prevent us from doing
the migration of each driver in step by step.

Kind regards
Uffe